Editor is hardcoded

[GoogleCodeExporter]

We are developing IDE for Symbian WRT. Basically, we combine JSDT and 
ChromeDevTools and add 
our plumbing inbetween. So far it works really well. The code will be available 
under EPL and I think 
we will be able to discuss more at a later point.

Our goals is to reuse ChromeDevTools as much as possible. At this point we 
would like to make it 
possible the ChDT to use editors other then the one that is part of ChDT.

Patch modifies two classes:
LineBreakpointAdapter - changes getEditor accessibility from private to 
protected. This allows easy 
creation of the subclasses that work with other editors.
JsDebugModelPresentation - getEditorId now calls IDE.getEditorDescriptor to 
pick the editor for 
provided element. This will pick the Chrome editor for the files with ".chrome" 
extension, JSDT 
editor for JS file, HTML editor for HTML, etc. So far the testing did not show 
any problems.

Original issue reported on code.google.com by eostrouk...@gmail.com on 3 Dec 2009 at 5:03

Attachments:

• no-hardcoded-editor.txt

[GoogleCodeExporter]

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:

• chromium-patch.txt

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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:

• chromium-patch3.txt

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

> 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

• Changed state: Accepted

[GoogleCodeExporter]

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:

• chromium-patch4.txt

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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:

• debugview-wrt.png
• chromium-patch6.txt

[GoogleCodeExporter]

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

[GoogleCodeExporter]

I applied them both before writing comment# 17. Looks great.

Original comment by eostrouk...@gmail.com on 21 Dec 2009 at 7:14

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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:

• LineBreakpointAdapter.java.txt

[GoogleCodeExporter]

Patch with LabelProvider made its way to SVN.

Original comment by peter.ry...@gmail.com on 22 Dec 2009 at 11:16

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

[GoogleCodeExporter]

I mean "Do I understand right ..."

Original comment by peter.ry...@gmail.com on 22 Dec 2009 at 3:21

[GoogleCodeExporter]

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

[GoogleCodeExporter]

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

• Changed state: Fixed