Closed GoogleCodeExporter closed 8 years ago
I would also like to ask to introduce more interfaces. In our code we have to
do significant changes to
DebugTargetImpl, ResourceManager, etc. Unfortunately this requires branching a
lot of the code like Value class,
etc. This updated patch introduces one interface that would let ChDT work with
branched Value class.
Original comment by eostrouk...@gmail.com
on 9 Dec 2009 at 11:39
Attachments:
Hi Eugene
Sorry for taking so long.
I'm looking at patch "chromium-patch.txt" and there are some things I'm not
quite
happy about.
1. Patched getEditorId looks strange. New condition on element instanceof IFile
makes
old similar condition below useless. On the other hand the case when element is
breakpoint stays unpatched, which is a bit suspicious, as to whether you
support
navigation from breakpoint view to source editor.
2. IJsValueHolder is fine, but do we need to put it into org.chromium.sdk? Why
not
into org.chromium.debug.core.model?
3. Protected method getEditor. It looks like you are trying to disable some of
our
original functionality, overriding it with your new functionality. This looks
like a
good technique only when you are doing some fine adjustments to a base class.
In
other case component design is more preferable: when there are several
components and
you pick only those that you need. I don't understand what you are trying to do
well
enough, so unfortunately I cannot suggest exact design alternative here.
Anyway, I
think we can afford some additional interfaces if it makes program and design
more
clear.
Maybe it would be useful, if you could overview your plans to use
ChromeDevTools.
E.g. which parts of system do use and to what level:
org.chromium.sdk interfaces
org.chromium.sdk implementation
org.chromium.debug.core
org.chromium.debug.ui
Peter
Original comment by peter.ry...@gmail.com
on 17 Dec 2009 at 6:44
1. Thank you for pointing me at the bug. I updated the code.
2. I believe it is in the org.chromium.debug.core.model - it does not belong to
sdk plugin as it is effectively
an Eclipse Debug support interface.
3. The goal was to allow easy creation of the breakpoint factories. I.e.:
public class WorkspaceLineBreakpointAdapter extends LineBreakpointAdapter {
@SuppressWarnings("restriction")
@Override
protected ITextEditor getEditor(IWorkbenchPart part) {
if (part instanceof JavaEditor) {
return (ITextEditor) part;
} else {
return null;
}
}
}
> Maybe it would be useful, if you could overview your plans to use
ChromeDevTools.
We completely install ChromeDevTools - so users can use them in the unaltered
form just as if they download
it from your site.
But we also introduce our own launch configuration type that will launch Chrome
(and pass arguments),
establish debug connection, etc.
Also we created a code that allows the users to set breakpoints in the JSDT
editor (i.e. in the workspace with
the projects of our type) - pretty much as when you develop in Java.
Original comment by eostrouk...@gmail.com
on 17 Dec 2009 at 7:13
Oh, I see. Let me take a look at your new patch.
I'm sorry about IJsValueHolder, looked at import line instead of package line.
By the way, I wonder why you might need to have this interface, if you are
using
underlying org.chromium.sdk.
Peter
Original comment by peter.ry...@gmail.com
on 17 Dec 2009 at 7:37
We had to create our own debug target to replace the resources and breakpoints
management. Unfortunately,
this also required us to branch DebugElementImpl and its descendants (including
Value) as they take
DebugTargetImpl as a ctor argument. Which, in turn, broke
JsDebugModelPresentation#computeDetail that
explicitely requires Value.
I believe the most clean solution would be to replace DebugTargetImpl with
interface that is passed to
constructors and such - but this would make patch several times bigger making
it harder to review and lowering
the chances of its being accepted ;)
Original comment by eostrouk...@gmail.com
on 17 Dec 2009 at 7:47
Maybe this is doable. Could please send what kind of interface for
DebugTargetImpl you
actually need?
Original comment by peter.ry...@gmail.com
on 17 Dec 2009 at 7:57
I don't have any requirements for DebugTargetImpl - basically all that is
needed is so that DebugTarget interface
can be passed into DebugElementImpl ctor and satisfy all requirements in the
Chromium codebase. Then our
DebugTargetImpl will implement that interface and will be used when running
using our debug configuration
tab.
Original comment by eostrouk...@gmail.com
on 17 Dec 2009 at 8:35
A new patch.
This patch introduces interfaces IDebugTarget and IResourceManager and
refactors the rest of the code to rely
on the interfaces instead of concrete classes.
This way we can provide different resource resolution and breakpoints
management by implementing the
interfaces - while reusing all debugger code. Our implementations of these
interfaces will soon be available as
part of
http://developer.symbian.org/main/source/packages/package/index.php?pk=263
I kept all semantics (method names, signatures) same as they were in the
classes.
Original comment by eostrouk...@gmail.com
on 18 Dec 2009 at 10:10
Attachments:
I uploaded your patch to our code-review site:
http://codereview.chromium.org/503056
I tried to drop unused methods from interfaces (see "Patch Set 2 : simplify
interfaces" section, especially "Delta from patch set" for
DebugTargetImpl.java:463),
though I left "getDebugProject()" because it was unused from the beginning and I
though it might be intentional.
I have a question at this step: are you sure you wish to reject whole
DebugTargetImpl? It looks like there are a lot of code you may reuse from there
and
perhaps will end up copy-pasting. Maybe it would be simpler to sever this class
into
2 parts?
Original comment by peter.ry...@gmail.com
on 18 Dec 2009 at 9:39
Basically, I began by branching many classes from the model package - I was not
able to subclass (I don't
remember by now, but I think there was something with private members, etc.)
What I would suggest is for now to only introduce the interfaces. Once our
sources are available online then
we can discuss if there is anything that you would like to see in Chromium
Developer tools so we decide on
refactoring then.
I believe there are a few of Eclipse-based products that might want a JS
debugger (considering "half-dead"
status of ATF) - so workspace integration might be a good contribution for
Chromium Developer Tools.
Original comment by eostrouk...@gmail.com
on 18 Dec 2009 at 9:55
> I just tried to see if I can subclass your DebugTargetImpl - nope, it doesn't
> seem possible.
I don't actually expect somebody to subclass our classes, including
DebugTargetImpl,
if the purpose of it is to block some aspects of class behavior. We never imply
such
use and such technique tends to break even after small innocent changes.
Instead it's
better to split our class into 2 parts, in our case into "base" and
"virtual-project
specific" parts. We welcome such approach. This way we can avoid a code
duplication
which is good for both of us.
Again, I'm fine with your interfaces. Probably I will put them into SVN on
Monday or
Tuesday.
Peter
Original comment by peter.ry...@gmail.com
on 18 Dec 2009 at 11:14
Peter,
we wanted to subclass DebugTargetImpl only to pass our class into
DebugElementImpl constructors.
Please see updated patch - there is no need in IResourceManager interface if
the getResource(Script) method is
moved to IChromiumDebugTarget interface.
Original comment by eostrouk...@gmail.com
on 19 Dec 2009 at 7:48
Attachments:
Hi Eugene
Could you please take a look at refactoring we are about to make?
http://codereview.chromium.org/508004
(you can view diff on the web or you can download a patch and apply it;
unfortunately
it is not Eclipse format of patch because we use Chromium's git- and SVN-based
toolset).
Basically it strips everything related to workspace out of DebugTargetImpl.
Ideally
you just need to implement the interface.
It would be great if you could give me a feedback about how well the new
interfaces
work for you. E.g. do you need perhaps a getter from DebugTargetImpl to
WorkspaceRelations object?
Original comment by peter.ry...@gmail.com
on 20 Dec 2009 at 10:33
Hardcoded editor patch;
Eugene, is it okay with you?
http://codereview.chromium.org/501147
Original comment by peter.ry...@gmail.com
on 20 Dec 2009 at 10:58
This last patch has some drastic changes so it may take a considerable time to
reimplement our changes on top
of that code... I'm trying to do that - but I can't provide ETA atm.
Original comment by eostrouk...@gmail.com
on 21 Dec 2009 at 4:03
Please note, that it is not in SVN yet. I'm about to do some polishing first.
Anyway it's a perfect moment to ask for some additional callbacks/methods in
interfaces
now. Of course, if there are some obvious ones. However, we still may add them
later,
if needed.
The patch deals virtually only with DebugTargetImpl. Other classes may be
considered
being unchanged.
Original comment by peter.ry...@gmail.com
on 21 Dec 2009 at 4:13
I had two problems with the updated patch:
1. Creating the breakpoints provider adapter for JSDT editor became harder, as
the patch removed our past
change that allowed overriding only one method.
2. (I know we haven't discussed this) changing labels for stack trace, threads
became harder as we don't
subclass DebugTargetImpl.
Attached to this bug is the debug view contents for WRt debugger. You may
notice that instead of URLs we
show workspace resource paths (like in JDT debugger) - to reduce clutter and
show less of our
debugger/Previewer internals.
I also attached a patch that fixes both problems for us.
Original comment by eostrouk...@gmail.com
on 21 Dec 2009 at 6:41
Attachments:
Meanwhile it would be cool if you looked at
http://codereview.chromium.org/501147
Original comment by peter.ry...@gmail.com
on 21 Dec 2009 at 7:09
I applied them both before writing comment# 17. Looks great.
Original comment by eostrouk...@gmail.com
on 21 Dec 2009 at 7:14
Now in SVN.
p.s. We do not like to combine patches and we usually use git in-house to
manage
patches locally.
Original comment by peter.ry...@gmail.com
on 21 Dec 2009 at 7:36
Okay.
First part of WorkspaceRelations is in SVN. Now it is called WorkspaceBridge.
Please take a look at the second part:
http://codereview.chromium.org/501157
Original comment by peter.ry...@gmail.com
on 21 Dec 2009 at 8:24
All the patches look great. Thank you.
There is only one thing that seems to be lost - making the getEditor method in
the breakpoint provider
protected so it is possible to create such factories for other editors.
Consider factory for JSDT editor:
public class WorkspaceLineBreakpointAdapter extends LineBreakpointAdapter {
@SuppressWarnings("restriction")
@Override
protected ITextEditor getEditor(IWorkbenchPart part) {
if (part instanceof JavaEditor) {
return (ITextEditor) part;
} else {
return null;
}
}
}
Original comment by eostrouk...@gmail.com
on 22 Dec 2009 at 7:23
Attachments:
Patch with LabelProvider made its way to SVN.
Original comment by peter.ry...@gmail.com
on 22 Dec 2009 at 11:16
About the last one.
I am looking at LineBreakpointAdapter and I see 2 non-trivial methods:
getEditor and
toggleLineBreakpoints. First one you are going to override. The second one
deals with
ChromiumLineBreakpoint.
I don't quite see why you need to reuse this class at all. Could you please
describe?
Original comment by peter.ry...@gmail.com
on 22 Dec 2009 at 11:57
toggleLineBreakpoints works fine without override. I posted sources of our
breakpoint adapter in the
comment 22.
Basically, there is no difference what the editor class is - breakpoint is a
resource marker that keeps the
breakpoint class in it. So really there is no problem reusing the breakpoint
adapter - the editor requirement is
pretty artificial.
Sure, we had to add breakpoint management actions to the JSDT editor ourselves
- but that's really
straightforward.
Original comment by eostrouk...@gmail.com
on 22 Dec 2009 at 12:13
Please, take a look at http://codereview.chromium.org/503083
Do you understand right that you use the same model id?
Original comment by peter.ry...@gmail.com
on 22 Dec 2009 at 2:10
I mean "Do I understand right ..."
Original comment by peter.ry...@gmail.com
on 22 Dec 2009 at 3:21
Yes, we use the same model ID. Is there any reason to change it? I.e. we don't
have any reason to change the
presentation (I don't even think our plugin depends on ...ui project - only the
...core and ...sdk).
Original comment by eostrouk...@gmail.com
on 22 Dec 2009 at 3:32
As of r306 LineBreakpointAdapter#getEditor method became abstract.
I guess it closes this issue.
Original comment by peter.ry...@gmail.com
on 22 Jan 2010 at 5:07
Original issue reported on code.google.com by
eostrouk...@gmail.com
on 3 Dec 2009 at 5:03Attachments: