Last Comment Bug 703031 - [highlighter] Refactor the highlighter code. Create highlighter.jsm
: [highlighter] Refactor the highlighter code. Create highlighter.jsm
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 566092 653545 663830 713676 714443
  Show dependency treegraph
 
Reported: 2011-11-16 12:05 PST by Paul Rouget [:paul]
Modified: 2012-01-28 16:03 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed refactoring (3.37 KB, text/plain)
2011-11-16 12:13 PST, Paul Rouget [:paul]
no flags Details
Proposed refactoring - v0.2 (3.45 KB, patch)
2011-11-17 02:22 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
Proposed refactoring - v0.3 (5.34 KB, application/javascript)
2011-11-24 04:06 PST, Paul Rouget [:paul]
no flags Details
Proposed refactoring - v0.4 (3.74 KB, text/plain)
2011-11-24 06:26 PST, Paul Rouget [:paul]
rcampbell: feedback+
Details
patch-A split - WIP (42.42 KB, patch)
2011-12-06 09:11 PST, Paul Rouget [:paul]
rcampbell: feedback+
Details | Diff | Review
patch-B reorder - WIP (9.77 KB, patch)
2011-12-06 09:19 PST, Paul Rouget [:paul]
rcampbell: feedback+
Details | Diff | Review
patch-C refactoring - WIP (40.64 KB, patch)
2011-12-06 09:19 PST, Paul Rouget [:paul]
no flags Details | Diff | Review
patch-D tests - WIP (32.88 KB, patch)
2011-12-06 09:20 PST, Paul Rouget [:paul]
rcampbell: feedback+
Details | Diff | Review
patch-C refactoring - WIP (48.06 KB, patch)
2011-12-07 12:10 PST, Paul Rouget [:paul]
rcampbell: feedback+
Details | Diff | Review
patch-A split - v1 (43.77 KB, patch)
2012-01-13 10:53 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
patch-B reorder - v1 (10.28 KB, patch)
2012-01-13 10:53 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
patch-C refactoring - v1 (48.26 KB, patch)
2012-01-13 10:55 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
patch-D tests - v1 (33.43 KB, patch)
2012-01-13 10:56 PST, Paul Rouget [:paul]
rcampbell: review+
Details | Diff | Review
patch-D tests - v1.1 (33.50 KB, patch)
2012-01-17 03:09 PST, Paul Rouget [:paul]
no flags Details | Diff | Review

Description Paul Rouget [:paul] 2011-11-16 12:05:36 PST
With the multiselection code coming (bug 653545), it would be good to extract the highlighter code from inspector.jsm to a new file (highlighter.jsm). Inspector.jsm is getting big, and the highlighter code is not really modular.
Comment 1 Paul Rouget [:paul] 2011-11-16 12:13:07 PST
Created attachment 574959 [details]
Proposed refactoring

This is a proposed API for the highlighter. It is quite basic, but should cover what's needed by the inspector code, and for the multi-selection code.

This is not a "proper" API, just a rough idea.
Comment 2 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-16 13:14:00 PST
Comment on attachment 574959 [details]
Proposed refactoring


If I'm allowed to perform a drive-by ...

>    /**
>    * Hide the selectors, the veil and the infobar.
>    */
>    disable: function() {},

disable has the connotation (to me) that you're preventing it from working rather than hiding it. For example <button disabled> is still there, just greyed out. Perhaps hide()?

>    showSingleSelector: function(aNode = document.body, aLocked = false, aScroll = true) {},
>    selectSingleNode: function(aNode, aScroll = true) {},

I'm a little unclear, is showSingleSelector(node, true) the same as selectSingleNode(node), and where does lock(node) fit in?

>    showMultiSelector: function(aNodeList, aCallback) {},
>    selectMultipleNodes: function(aNodeList, aCallback) {},

This perhaps is part of my possible misunderstanding above, but is multi-selection using the mouse, needed? I can imagine that it's hard to understand.

>    /**
>    * Is a node highlightable?
>    *
>    * @param aNode Node to test.
>    */
>    isNodeHighlightable: function(aNode) {},

I'm not sure what is highlightable - is it a combination of visible, and something that we can scroll to?

And this is a nit:

>    on: function(aEventName, aCallback) {},
>    removeListener: function(aEventName, aCallback){},

Is the asymmetry between these method names normal? Wouldn't it fit with the web way to have addListener?
Comment 3 Paul Rouget [:paul] 2011-11-17 02:16:28 PST
Thank you for the feedback Joe.
I'll update the API and the initial comment to make this more clear.

(In reply to Joe Walker from comment #2)
> Comment on attachment 574959 [details]
> >    /**
> >    * Hide the selectors, the veil and the infobar.
> >    */
> >    disable: function() {},
> 
> disable has the connotation (to me) that you're preventing it from working
> rather than hiding it. For example <button disabled> is still there, just
> greyed out. Perhaps hide()?

Right. Hide is better.

> >    showSingleSelector: function(aNode = document.body, aLocked = false, aScroll = true) {},
> >    selectSingleNode: function(aNode, aScroll = true) {},
> 
> I'm a little unclear, is showSingleSelector(node, true) the same as
> selectSingleNode(node), and where does lock(node) fit in?

Yeah, I realise that selectSingleNode is useless. Lock is enough.

> >    showMultiSelector: function(aNodeList, aCallback) {},
> >    selectMultipleNodes: function(aNodeList, aCallback) {},
> 
> This perhaps is part of my possible misunderstanding above, but is
> multi-selection using the mouse, needed? I can imagine that it's hard to
> understand.

selectMultipleNodes just update the list of selected nodes.
No mouse involved in the multiSelector, beside for the "click" that will trigger the callback.

> >    /**
> >    * Is a node highlightable?
> >    *
> >    * @param aNode Node to test.
> >    */
> >    isNodeHighlightable: function(aNode) {},
> 
> I'm not sure what is highlightable - is it a combination of visible, and
> something that we can scroll to?

Highlightable means potentially visible (not like "title", "head", "script", ...).

> And this is a nit:
> 
> >    on: function(aEventName, aCallback) {},
> >    removeListener: function(aEventName, aCallback){},
> 
> Is the asymmetry between these method names normal? Wouldn't it fit with the
> web way to have addListener?

+1
Comment 4 Paul Rouget [:paul] 2011-11-17 02:22:58 PST
Created attachment 575125 [details] [diff] [review]
Proposed refactoring - v0.2

Addressed Joe's comments.
Comment 5 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-17 02:52:45 PST
Comment on attachment 575125 [details] [diff] [review]
Proposed refactoring - v0.2


>    /**
>    * Show the veil, and select the nodes.
>    * Disable the singleSelector.
>    *
>    * @param aNodeList List of nodes to select.
>    * @param aCallback function(aNode){} - Called when a selection is clicked.
>    */
>    showMultiSelector: function(aNodeList, aCallback) {},

I know this was originally my suggestion, but it was probably a bad one. How about killing aCallback and having a 'nodepicked' event instead. Feels much more consistent...
Comment 6 Rob Campbell [:rc] (:robcee) 2011-11-17 14:55:15 PST
Comment on attachment 575125 [details] [diff] [review]
Proposed refactoring - v0.2

    *   "highlighted"

not a great word or conjugation. "onHighlighted" sounds funny to me.

"highlit" doesn't sound great either.

what about just "highlight"? Highlighter.onHighlight sounds... okay.

Highlighter.onShow might be the best solution here. A "shown" event might make the most sense and do away with ugly variations on "highlight".

    hide: function() {},

+1 on this.

    showMultiSelector: function(aNodeList, aCallback) {},

+1 on Joe's suggestion to do away with the callback and add an event listener for consistency.

    /**
    * Show the veil, and select the nodes.
    * Disable the singleSelector.
    *
    * @param aNodeList List of nodes to select.
    * @param aCallback function(aNode){} - Called when a selection is clicked.
    */
    showMultiSelector: function(aNodeList, aCallback) {},


    /**
     * Update the list of selected nodes.
     *
     * @param aNodeList List of nodes to select.
     */
    selectMultipleNodes: function(aNodeList) {},

These are confusing. Does selectMultipleNodes also perform ... selection (highlighting)?

With showMultiSelector and the event firing, we could do away with the second one. Or is that what Joe said? If so, we should listen to him.

    /**
    * Returns the selected node.
    *
    * @return node
    */
    getNode: function() {}

in the comment, I would suggest: "Returns the selected node or the first node in a list of selected nodes if more than one is selected". Similar to querySelector vs. querySelectorAll.

    * Returns an array of the selected nodes.
    *
    * @return nodeList
    */
    getNodes: function() {}

should we use getAllNodes to be more explicit? Maybe unnecessary, I'm not sure about that.
Comment 7 Paul Rouget [:paul] 2011-11-24 03:22:18 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #6)
> Comment on attachment 575125 [details] [diff] [review] [diff] [details] [review]
> Proposed refactoring - v0.2
> 
>     *   "highlighted"
> 
> not a great word or conjugation. "onHighlighted" sounds funny to me.
> 
> "highlit" doesn't sound great either.
> 
> what about just "highlight"? Highlighter.onHighlight sounds... okay.
> 
> Highlighter.onShow might be the best solution here. A "shown" event might
> make the most sense and do away with ugly variations on "highlight".

"show" is the work used to open a highlighter. That might be confusing.
"highlit" sounds very weird, even if correct.

What about "selected"? According to the definition of what a "selection" is, it's correct.
Comment 8 Paul Rouget [:paul] 2011-11-24 03:59:07 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #6)
>     /**
>     * Show the veil, and select the nodes.
>     * Disable the singleSelector.
>     *
>     * @param aNodeList List of nodes to select.
>     * @param aCallback function(aNode){} - Called when a selection is clicked.
>     */
>     showMultiSelector: function(aNodeList, aCallback) {},
> 
> 
>     /**
>      * Update the list of selected nodes.
>      *
>      * @param aNodeList List of nodes to select.
>      */
>     selectMultipleNodes: function(aNodeList) {},
> 
> These are confusing. Does selectMultipleNodes also perform ... selection
> (highlighting)?

selectMultipleNodes only makes sense in the multiSelector.
It updates the list of selected nodes.

For the singleSelector, we will use lock (because I don't think we will ever need to select a node and not lock it from outside of the highlighter code).

Is that clear?

>     /**
>     * Returns the selected node.
>     *
>     * @return node
>     */
>     getNode: function() {}
> 
> in the comment, I would suggest: "Returns the selected node or the first
> node in a list of selected nodes if more than one is selected". Similar to
> querySelector vs. querySelectorAll.

I don't see any situation where we want tp have the first node of the available nodes.

>     * Returns an array of the selected nodes.
>     *
>     * @return nodeList
>     */
>     getNodes: function() {}
> 
> should we use getAllNodes to be more explicit? Maybe unnecessary, I'm not
> sure about that.

+1
Comment 9 Paul Rouget [:paul] 2011-11-24 04:06:26 PST
Created attachment 576716 [details]
Proposed refactoring - v0.3

Not asking for feedback yet. I am not satisfied with this new version.

I think that bringing this notion of "selection" and "selectors" is too confusing.
Comment 10 Paul Rouget [:paul] 2011-11-24 06:11:50 PST
Definitely wrong. The fact I need to define so many things at the beginning, and that I am re-defining existing terms (highlighting and selecting) is wrong.

I think I should re-use (more or less) the current methods and terms, and add some specific methods for the multi-selection.
Comment 11 Paul Rouget [:paul] 2011-11-24 06:26:48 PST
Created attachment 576745 [details]
Proposed refactoring - v0.4

This is a much simpler approach. It is based on the current highlighter (with events).

I only added a `showNodePicker` method.

Joe, what do you think?
Comment 12 Rob Campbell [:rc] (:robcee) 2011-11-24 06:45:39 PST
Comment on attachment 576745 [details]
Proposed refactoring - v0.4

marking it as a patch for starters.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-11-24 06:51:16 PST
Comment on attachment 576745 [details]
Proposed refactoring - v0.4

yeah, I like this approach and API definition. Very light.

One thing I was discussing with Victor earlier this week (and not something that needs to be done with this first refactoring), is to have a new registerSurface call in inspector.jsm that we could use to declare and register various page overlays for highlighting.

I'm going to file a bug on that and spec out a bit of the API but will ask for feedback there.
Comment 14 Rob Campbell [:rc] (:robcee) 2011-11-24 07:04:15 PST
one other observation:

You have a hide() method but no corresponding show().
Comment 15 Rob Campbell [:rc] (:robcee) 2011-11-24 07:07:47 PST
filed bug 705131
Comment 16 Paul Rouget [:paul] 2011-11-24 07:12:55 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #14)
> one other observation:
> 
> You have a hide() method but no corresponding show().

I was thinking about using `highlight()` for this.
Might wrap this in a show() function.
Comment 17 Rob Campbell [:rc] (:robcee) 2011-11-24 13:30:09 PST
does hide() merely hide the selection or the entire highlighter (aka veil)? If it's equivalent to an "unhighlight()", then highlight() is probably sufficient.

This is sounding good.
Comment 18 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-24 15:14:28 PST
I like the idea of calling highlight() -> show(), but even better for symmetry with showNodePicker(), we could have showHighlighter()

It also feels like aLocked is a more important parameter than aNode, so it should come earlier in the param list. The UI changes more depending on the state of aLocked than for aNode. So:
  highlight: function(aLocked, aNode, aPreventScroll) {},
And this isn't just a nit ...


Perhaps we can delete some functions? Instead of
  highlighter.lock();

Can we can do:
  highlighter.showHighlighter(true);

And in place of:
  highlighter.unlock();

Can we can do:
  highlighter.showHighlighter(false);

Unless I've got the wrong end of the stick???

This is a nit, but I think the sense of the @param aPreventScroll documentation is wrong. If the answer to the question "Should we ensure that the selected node is visible" is yes, I would expect to pass true in that slot, but I don't I pass false. So perhaps the docs should read "Should we avoid scrolling to ensure that the selected node is visible".

And to enter the highlit vs highlighted vs highlight debate, I think the answer has to be highlight. The thing that comes after 'on' is a verb, but we're inconsistent as to whether it's present tense ('click') or past tense ('DOMNodeInserted'). Generally DOM events are present tense unless the present tense version sounds strange (e.g. 'onDOMNodeInsert'). So I think we should prefer 'highlight' over 'highlighted' and 'highlit'.

LayoutHelpers is more opaque to me, it feels from reading it through that you understand what's going on here way better than I do. I'm left with the feeling "why that set of functions" - I'm not doubting that you're right, just feeling that my comments are not much use, beyond this: Could you document the @return for getDirtyRect()?
Comment 19 Sebastian Zartner 2011-11-24 22:18:41 PST
That could also be interesting for the Firebug team. So we could reuse the Inspector, but use our own highlighter.
Comment 20 Paul Rouget [:paul] 2011-11-25 01:24:02 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #17)
> does hide() merely hide the selection or the entire highlighter (aka veil)?
> If it's equivalent to an "unhighlight()", then highlight() is probably
> sufficient.
> 
> This is sounding good.

The problem I am trying to solve with `hide` is: when the mouse is not hovering the web page while inspecting, the last hovered element is highlighted (usually, the <body> element, but not always). I don't think this is good.

`hide` will "disable" the highlighter. I am not sure we should keep the veil or not yet. My first thought was to hide everything (veil + infobar + nodePpicker, everything that's on top of the webpage). But we might want to keep the veil.
Comment 21 Paul Rouget [:paul] 2011-11-25 01:42:57 PST
Thanks for the comment Joe,

(In reply to Joe Walker from comment #18)
> I like the idea of calling highlight() -> show(), but even better for
> symmetry with showNodePicker(), we could have showHighlighter()

That's possible. We can't replace .highlight with showHighlighter though (see below).

> It also feels like aLocked is a more important parameter than aNode, so it
> should come earlier in the param list. The UI changes more depending on the
> state of aLocked than for aNode. So:
>   highlight: function(aLocked, aNode, aPreventScroll) {},
> And this isn't just a nit ...

We "highlight a node". I think the node option is more important here.

> Perhaps we can delete some functions? Instead of
>   highlighter.lock();
> 
> Can we can do:
>   highlighter.showHighlighter(true);
> 
> And in place of:
>   highlighter.unlock();
> 
> Can we can do:
>   highlighter.showHighlighter(false);

The highlighter can be up and running (inspecting for example), and we want to highlight and lock a particular node (from the breadcrumbs for example).

So if we want to remove .lock and .unlock, we should replace them with .highlight().

> Unless I've got the wrong end of the stick???

It is important to understand that when the highlighter is active, it can be controlled by different mechanisms:
- the HTML tree
- the breadcrumbs
- the inspection mechanism (hovering nodes)

These 3 mechanisms use .highlight() to highlight a node, and .lock() to lock a node.

showHighlighter() could be used to active the higlighter, but can't be used to select a node.

> This is a nit, but I think the sense of the @param aPreventScroll
> documentation is wrong. If the answer to the question "Should we ensure that
> the selected node is visible" is yes, I would expect to pass true in that
> slot, but I don't I pass false. So perhaps the docs should read "Should we
> avoid scrolling to ensure that the selected node is visible".

You're right!

> And to enter the highlit vs highlighted vs highlight debate, I think the
> answer has to be highlight. The thing that comes after 'on' is a verb, but
> we're inconsistent as to whether it's present tense ('click') or past tense
> ('DOMNodeInserted'). Generally DOM events are present tense unless the
> present tense version sounds strange (e.g. 'onDOMNodeInsert'). So I think we
> should prefer 'highlight' over 'highlighted' and 'highlit'.

Makes sense.

> LayoutHelpers is more opaque to me, it feels from reading it through that
> you understand what's going on here way better than I do. I'm left with the
> feeling "why that set of functions" - I'm not doubting that you're right,
> just feeling that my comments are not much use, beyond this: Could you
> document the @return for getDirtyRect()?

So I am not 100% sure about this yet. What I am trying to do here is to move to browser/devtools/shared all the layout related functions, that, in the future, could be used by other tools.

In the first place, I thought I will have to extract functions from the highlighter code to build the nodePicker. But since the node picker will be part of the highlighter code, it is not necessary anymore.
Comment 22 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-25 03:28:43 PST
(In reply to Paul Rouget [:paul] from comment #21)
> Thanks for the comment Joe,
> 
> (In reply to Joe Walker from comment #18)
> > I like the idea of calling highlight() -> show(), but even better for
> > symmetry with showNodePicker(), we could have showHighlighter()
> 
> That's possible. We can't replace .highlight with showHighlighter though
> (see below).
> 
...

> > Perhaps we can delete some functions? Instead of
> >   highlighter.lock();
> > 
> > Can we can do:
> >   highlighter.showHighlighter(true);
> > 
> > And in place of:
> >   highlighter.unlock();
> > 
> > Can we can do:
> >   highlighter.showHighlighter(false);
> 
> The highlighter can be up and running (inspecting for example), and we want
> to highlight and lock a particular node (from the breadcrumbs for example).
> 
> So if we want to remove .lock and .unlock, we should replace them with
> .highlight().

Is the logic here that calling showHighlight() when the highlighter is already showing is bizarre?

> It is important to understand that when the highlighter is active, it can be
> controlled by different mechanisms:
> - the HTML tree
> - the breadcrumbs
> - the inspection mechanism (hovering nodes)
> 
> These 3 mechanisms use .highlight() to highlight a node, and .lock() to lock
> a node.
> 
> showHighlighter() could be used to active the higlighter, but can't be used
> to select a node.

Isn't showHighlighter(node, true) a way to select a node?
Comment 23 Paul Rouget [:paul] 2011-11-25 03:49:49 PST
(In reply to Joe Walker from comment #22)
> (In reply to Paul Rouget [:paul] from comment #21)
> > Thanks for the comment Joe,
> > 
> > (In reply to Joe Walker from comment #18)
> > > I like the idea of calling highlight() -> show(), but even better for
> > > symmetry with showNodePicker(), we could have showHighlighter()
> > 
> > That's possible. We can't replace .highlight with showHighlighter though
> > (see below).
> > 
> ...
> 
> > > Perhaps we can delete some functions? Instead of
> > >   highlighter.lock();
> > > 
> > > Can we can do:
> > >   highlighter.showHighlighter(true);
> > > 
> > > And in place of:
> > >   highlighter.unlock();
> > > 
> > > Can we can do:
> > >   highlighter.showHighlighter(false);
> > 
> > The highlighter can be up and running (inspecting for example), and we want
> > to highlight and lock a particular node (from the breadcrumbs for example).
> > 
> > So if we want to remove .lock and .unlock, we should replace them with
> > .highlight().
> 
> Is the logic here that calling showHighlight() when the highlighter is
> already showing is bizarre?

Yes. I think we should use this API like that:

var h = new Highlighter(); // this will init the highlighter. nothing is showed.
h.highlight(node, true); // this will show the veil, and highlight the node (and lock it).

> > It is important to understand that when the highlighter is active, it can be
> > controlled by different mechanisms:
> > - the HTML tree
> > - the breadcrumbs
> > - the inspection mechanism (hovering nodes)
> > 
> > These 3 mechanisms use .highlight() to highlight a node, and .lock() to lock
> > a node.
> > 
> > showHighlighter() could be used to active the higlighter, but can't be used
> > to select a node.
> 
> Isn't showHighlighter(node, true) a way to select a node?

No, not really. I don't think that `showHighlighter` is necessary.
Comment 24 Paul Rouget [:paul] 2011-11-25 03:57:00 PST
I think that the highlighter (veil + selection + infobar) should be able to work independently of InspectorUI.

From the command line, we should be able to create an instance of the highlighter without the Inspector toolbar.

In the case of the inspect command:

if (one_node) {
  InspectorUI.openInspectorUI(node); // Open the inspector (highlighter + toolbar + node tools)
  endCommandLine();
} else {
  var h = new Highlighter();
  h.showNodePicker(nodes);           // Open the highlighter
  h.addListener("nodepicked", function(aNode) {
    h.destroy();
    InspectorUI.openInspectorUI(node, true);
    endCommandLine();
  }
}

What do you think?
Comment 25 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-25 05:20:53 PST
(In reply to Paul Rouget [:paul] from comment #23)
> (In reply to Joe Walker from comment #22)
> > (In reply to Paul Rouget [:paul] from comment #21)
> > > The highlighter can be up and running (inspecting for example), and we want
> > > to highlight and lock a particular node (from the breadcrumbs for example).
> > > 
> > > So if we want to remove .lock and .unlock, we should replace them with
> > > .highlight().
> > 
> > Is the logic here that calling showHighlight() when the highlighter is
> > already showing is bizarre?
> 
> Yes. I think we should use this API like that:
> 
> var h = new Highlighter(); // this will init the highlighter. nothing is
> showed.
> h.highlight(node, true); // this will show the veil, and highlight the node
> (and lock it).

OK - so lock() is now highlight(null, true). Yes?

(Part of the reason for me wanting to swap the param order was that lock() would be highlight(true), but I'm not particularly fussed about this)

Please could you upload how we are now? I think it's looking good.
Thanks.
Comment 26 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-25 05:39:38 PST
(In reply to Paul Rouget [:paul] from comment #24)
> I think that the highlighter (veil + selection + infobar) should be able to
> work independently of InspectorUI.
> 
> From the command line, we should be able to create an instance of the
> highlighter without the Inspector toolbar.
> 
> In the case of the inspect command:
> 
> if (one_node) {
>   InspectorUI.openInspectorUI(node); // Open the inspector (highlighter +
> toolbar + node tools)
>   endCommandLine();
> } else {
>   var h = new Highlighter();
>   h.showNodePicker(nodes);           // Open the highlighter
>   h.addListener("nodepicked", function(aNode) {
>     h.destroy();
>     InspectorUI.openInspectorUI(node, true);
>     endCommandLine();
>   }
> }
> 
> What do you think?

So the code isn't quite like that because we use the higlighter as we're typing.

The highlight command is here:
https://hg.mozilla.org/integration/fx-team/file/2c0174077ab7/browser/devtools/webconsole/GcliCommands.jsm#l121

You can see the call to InspectorUI.openInspectorUI on line 139.

It relies on the 'node' type to highlight as you type.
You can see the code here:
https://github.com/joewalker/gcli/blob/master/lib/gcli/types/node.js#L68

The current hacky highlighter is here: (It has to go for obvious reasons)
https://github.com/joewalker/gcli/blob/master/lib/gcli/host.js

I'll need to adapt my code to your API.
Comment 27 Paul Rouget [:paul] 2011-11-28 06:00:44 PST
This is how it looks like today (WIP). Feel free to comment.

/**
 * A highlighter mechanism.
 *
 * The highlighter is built dynamically into the browser element.
 * The caller is in charge of destroying the highlighter (ie, the highlighter
 * won't be destroy if a new tab is selected for example).
 *
 * API:
 *
 *   // Constructor and destructor.
 *   // @param aWindow - browser.xul window.
 *   Highlighter(aWindow); 
 *   void destroy();
 *
 *   // Highlight a node.
 *   // @param aNode - node to highlight
 *   // @param aScroll - scroll to ensure the node is visible
 *   void highlight(aNode, aScroll);
 *
 *   // Get the selected node.
 *   DOMNode getNode();
 *
 *   // Lock and unlock the select node.
 *   void lock();
 *   void unlock();
 *
 *   // Show and hide the highlighter
 *   void show();
 *   void hide();
 *   boolean isHidden();
 *
 *   // Redraw the highlighter if the visible portion of the node has changed.
 *   void invalidateSize(aScroll);
 *
 *   // Is a node highlightable.
 *   boolean isNodeHighlightable(aNode);
 *
 *   // Add/Remove lsiteners
 *   // @param aEvent - event name
 *   // @param aListener - function callback
 *   void addListener(aEvent, aListener);
 *   void removeListener(aEvent, aListener);
 *
 * Events:
 *
 *   "closed" - Highlighter is closing
 *   "changed" - A new node has been selected
 *   "highlighting" - Highlighter is highlighting
 *   "locked" - The selected node has been locked
 *   "unlocked" - The selected ndoe has been unlocked
 *
 * Structure:
 *
 *   <stack id="highlighter-container">
 *     <vbox id="highlighter-veil-container">...</vbox>
 *     <box id="highlighter-controls>...</vbox>
 *   </stack>
 *
 */
Comment 28 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-29 02:30:25 PST
Correct me if I'm wrong: there are 4 mutually exclusive states the highlighter could be in:
- hidden
- single node highlighting / locked
- single node highlighting / unlocked
- multiple node highlighting

I'm still of the opinion that the transitions between these states could be clearer. For example, it's not clear to me from the API above whether the locked state is cleared by hide()/show(), or if highlight() does show()
Comment 29 Paul Rouget [:paul] 2011-11-29 02:57:57 PST
(In reply to Joe Walker from comment #28)
> Correct me if I'm wrong: there are 4 mutually exclusive states the
> highlighter could be in:
> - hidden
> - single node highlighting / locked
> - single node highlighting / unlocked
> - multiple node highlighting

Correct.

> I'm still of the opinion that the transitions between these states could be
> clearer. For example, it's not clear to me from the API above whether the
> locked state is cleared by hide()/show(), or if highlight() does show()

Fair points.

In my current implementation of the refactoring:
- show()/hide() will just hide the highlighter (visibility:hidden) and disable the keybindings. So the lock state is restored once show() is called.
- the first thing highlight() does is to call show().

I can document that, or make the API clearer. But not sure how.

I am also trying to do not change the current API too much (the HTML tree panel and the inspector toolbar depend on it).
Comment 30 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-11-29 06:51:35 PST
(In reply to Paul Rouget [:paul] from comment #29)
> (In reply to Joe Walker from comment #28)
> > Correct me if I'm wrong: there are 4 mutually exclusive states the
> > highlighter could be in:
> > - hidden
> > - single node highlighting / locked
> > - single node highlighting / unlocked
> > - multiple node highlighting
> 
> Correct.
> 
> > I'm still of the opinion that the transitions between these states could be
> > clearer. For example, it's not clear to me from the API above whether the
> > locked state is cleared by hide()/show(), or if highlight() does show()
> 
> Fair points.
> 
> In my current implementation of the refactoring:
> - show()/hide() will just hide the highlighter (visibility:hidden) and
> disable the keybindings. So the lock state is restored once show() is called.
> - the first thing highlight() does is to call show().
> 
> I can document that, or make the API clearer. But not sure how.

Less functions that mutate state.

I've got 2 suggestions:

  void showHighlighter(aNode, aScroll, aLock)
  void showNodePicker(aNodeList)
  void hide();

  boolean isLocked();
  boolean isHidden();

Or:

  void setState(state)
  state getState()

  Where state is an object:
  {
    mode: HIDDEN|SINGLE_LOCKED|SINGLE_UNLOCKED|MULTIPLE
    selection: [node list]
  }

The nice thing about the latter is that it greatly simplifies the events - now there's only one - onStateChange


> I am also trying to do not change the current API too much (the HTML tree
> panel and the inspector toolbar depend on it).

Ah!

I'm a manic refactorer, so I'd probably change HTML tree etc to fit my new world order, but I perhaps don't know how much work that is.
Comment 31 Paul Rouget [:paul] 2011-12-05 09:06:52 PST
(In reply to Joe Walker from comment #30)
> I've got 2 suggestions:
> 
>   void showHighlighter(aNode, aScroll, aLock)
>   void showNodePicker(aNodeList)
>   void hide();
> 
>   boolean isLocked();
>   boolean isHidden();
> 
> Or:
> 
>   void setState(state)
>   state getState()
> 
>   Where state is an object:
>   {
>     mode: HIDDEN|SINGLE_LOCKED|SINGLE_UNLOCKED|MULTIPLE
>     selection: [node list]
>   }
> 
> The nice thing about the latter is that it greatly simplifies the events -
> now there's only one - onStateChange

setState() is not verbose enough and would make the API a bit too obscure.

showHighlighter(aNode, aScroll, aLock) is misleading. If we just want to lock a node, it sounds weird to call showHighlighter.

The highlighter has 4 type of actions:
1) showing/hiding it
2) selecting a node
3) showing the node picker
4) locking/unlocking a node

With these conditions:
1) Locking is only possible if a node is selected.
2) Selecting is only possible if the highlighter is showed and the nodePicker is not in use.

That's why I came to this API (that I feel is easy to understand and quite explicit):
1) show() / hide()
2) highlight(aNode, aScroll)
3) showNodePicker(aNodes)
4) lock() / unlock()

If one of the condition is not respected, I can force the state of the highlighter (showing the highlighter if hidden, select a node is no node is selected, ...) or raise an exception.

In my current implementation, using this API works fine and doesn't mess too much with the Inspector and the HTML Tree.

What do you think?
Comment 32 Joe Walker [:jwalker] (needinfo me or ping on irc) 2011-12-06 02:24:06 PST
(In reply to Paul Rouget [:paul] from comment #31)
> What do you think?

I think that you're implementing it, that you know more about how it works than me, and that we're debating something that's getting into the realm of aesthetics.

setState has an elegance to me although I agree that it's a bit obscure.
I think you understand the concerns that I had with the API, and I agree that we can overcome them with some documentation.

So lets do your proposal.
Comment 33 Paul Rouget [:paul] 2011-12-06 09:11:51 PST
Created attachment 579327 [details] [diff] [review]
patch-A split - WIP
Comment 34 Paul Rouget [:paul] 2011-12-06 09:19:34 PST
Created attachment 579331 [details] [diff] [review]
patch-B reorder - WIP
Comment 35 Paul Rouget [:paul] 2011-12-06 09:19:55 PST
Created attachment 579332 [details] [diff] [review]
patch-C refactoring - WIP
Comment 36 Paul Rouget [:paul] 2011-12-06 09:20:20 PST
Created attachment 579333 [details] [diff] [review]
patch-D tests - WIP
Comment 37 Paul Rouget [:paul] 2011-12-07 12:10:10 PST
Created attachment 579785 [details] [diff] [review]
patch-C refactoring - WIP
Comment 38 Rob Campbell [:rc] (:robcee) 2011-12-13 13:21:29 PST
sorry for the delay. I plan on getting to this within the next couple of days.
Comment 39 Rob Campbell [:rc] (:robcee) 2012-01-03 09:44:46 PST
Comment on attachment 579331 [details] [diff] [review]
patch-B reorder - WIP

ok, I'm sure there's a good reason for moving these methods to their new locations...
Comment 40 Rob Campbell [:rc] (:robcee) 2012-01-03 09:48:41 PST
Comment on attachment 579333 [details] [diff] [review]
patch-D tests - WIP

   function testNode1() {
-    dump("testNode1\n");

wow, looks like I left a little something extra in there for you. Thanks for removing. :)

moving isHighighting and getHighlitNode to head.js is brilliant.
Comment 41 Rob Campbell [:rc] (:robcee) 2012-01-05 08:15:35 PST
Comment on attachment 579785 [details] [diff] [review]
patch-C refactoring - WIP

- * </stack>
+ * The highlighter is built dynamically into the browser element.
+ * The caller is in charge of destroying the highlighter (ie, the highlighter
+ * won't be destroy if a new tab is selected for example).
  *

minor language nit on the last line:
  * won't be destroyed if a new tab...

Nice new documentation! Thanks for writing that up.

   /**
+   * Returns the selected node.
+   *
+   * @return node

should be @returns

+  show: function() {
+    if (this.hidden === false) return;

why not if (!this.hidden)?

+  hide: function() {
+    if (this.hidden === true) return;

if (this.hidden) ?

I see this elsewhere as well. Seems unnecessary.

+  addListener: function Highlighter_addListener(aEvent, aListener)
+  {
+    if (!(aEvent in this.events))
+        this.events[aEvent] = [];

extra indent there.

-  attachInspectListeners: function Highlighter_attachInspectListeners()
+  attachMouseListeners: function Highlighter_attachInspectListeners()
   {

signature should match the property name.

-  detachInspectListeners: function Highlighter_detachInspectListeners()
+  detachMouseListeners: function Highlighter_d

same here.

Also, if these methods are private, and I think they are, you might want to note that in the comments and maybe prefix them with a '_' character.

Looks good for a first-pass!
Comment 42 Paul Rouget [:paul] 2012-01-13 10:53:07 PST
Created attachment 588459 [details] [diff] [review]
patch-A split - v1
Comment 43 Paul Rouget [:paul] 2012-01-13 10:53:59 PST
Created attachment 588460 [details] [diff] [review]
patch-B reorder - v1
Comment 44 Paul Rouget [:paul] 2012-01-13 10:55:13 PST
Created attachment 588464 [details] [diff] [review]
patch-C refactoring - v1
Comment 45 Paul Rouget [:paul] 2012-01-13 10:56:23 PST
Created attachment 588465 [details] [diff] [review]
patch-D tests - v1
Comment 46 Rob Campbell [:rc] (:robcee) 2012-01-16 12:20:51 PST
Comment on attachment 588464 [details] [diff] [review]
patch-C refactoring - v1

+    if (!aNode) {
+      if (!this.node)
+        this.node = this.win.document.documentElement;

awkward, but I understand the logic and I don't have a better suggestion for you.


This is a really nice reorganization. Thanks!
Comment 47 Paul Rouget [:paul] 2012-01-16 13:30:14 PST
Thx for the r+.

Removing land-in-fx-team. Waiting for: https://tbpl.mozilla.org/?tree=Try&rev=001ce2cb9cb3
Comment 48 Paul Rouget [:paul] 2012-01-17 02:14:48 PST
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js | highlighter is highlighting
Comment 49 Paul Rouget [:paul] 2012-01-17 03:09:05 PST
Created attachment 589143 [details] [diff] [review]
patch-D tests - v1.1
Comment 52 Rob Campbell [:rc] (:robcee) 2012-01-26 12:39:28 PST
Comment on attachment 588459 [details] [diff] [review]
patch-A split - v1

[Approval Request Comment]
Regression caused by (bug #): Not a regression.
User impact if declined: We won't be able to add further patches that make the Inspector even more awesome.
Testing completed (on m-c, etc.): on m-c.
Risk to taking this patch (and alternatives if risky): There are four patches here that make up this request. Most of it is reorganization. There is some minimal change to functionality that this made possible.

bug 566092 relies on these patches and has been approved for aurora. Without these patches, we'll have to rewrite the contents of bug 566092 which may be more difficult because of the complexity of the un-refactored highlighter.

I think we should get this into aurora as it makes the Inspector considerably better.

Note You need to log in before you can comment on or make changes to this bug.