Closed Bug 703031 Opened 13 years ago Closed 12 years ago

[highlighter] Refactor the highlighter code. Create highlighter.jsm

Categories

(DevTools :: Inspector, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 12

People

(Reporter: paul, Unassigned)

References

Details

Attachments

(4 files, 10 obsolete files)

43.77 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
10.28 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
48.26 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review
33.50 KB, patch
Details | Diff | Splinter Review
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.
Blocks: 653545
Attached file Proposed refactoring (obsolete) —
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.
Attachment #574959 - Flags: feedback?(rcampbell)
Blocks: 663830
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?
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
Attached patch Proposed refactoring - v0.2 (obsolete) — Splinter Review
Addressed Joe's comments.
Attachment #574959 - Attachment is obsolete: true
Attachment #574959 - Flags: feedback?(rcampbell)
Attachment #575125 - Flags: feedback?(rcampbell)
Attachment #575125 - Attachment mime type: application/javascript → text/javascript
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 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.
Attachment #575125 - Attachment is patch: true
Attachment #575125 - Attachment mime type: text/javascript → text/plain
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector
(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.
(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
Attached file Proposed refactoring - v0.3 (obsolete) —
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.
Attachment #575125 - Attachment is obsolete: true
Attachment #575125 - Flags: feedback?(rcampbell)
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.
Attached file Proposed refactoring - v0.4 (obsolete) —
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?
Attachment #576716 - Attachment is obsolete: true
Attachment #576745 - Flags: feedback?(jwalker)
Attachment #576745 - Flags: feedback?(rcampbell)
Comment on attachment 576745 [details]
Proposed refactoring - v0.4

marking it as a patch for starters.
Attachment #576745 - Attachment is patch: true
Attachment #576745 - Attachment mime type: application/javascript → text/plain
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.
Attachment #576745 - Flags: feedback?(rcampbell) → feedback+
one other observation:

You have a hide() method but no corresponding show().
(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.
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.
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()?
That could also be interesting for the Firebug team. So we could reuse the Inspector, but use our own highlighter.
(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.
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.
(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?
(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.
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?
(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.
(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.
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>
 *
 */
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()
(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).
(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.
Assignee: nobody → paul
(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?
(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.
Attached patch patch-A split - WIP (obsolete) — Splinter Review
Attached patch patch-B reorder - WIP (obsolete) — Splinter Review
Attached patch patch-C refactoring - WIP (obsolete) — Splinter Review
Attached patch patch-D tests - WIP (obsolete) — Splinter Review
Attachment #579327 - Flags: feedback?(rcampbell)
Attachment #579331 - Flags: feedback?(rcampbell)
Attachment #579332 - Flags: feedback?(rcampbell)
Attachment #579333 - Flags: feedback?(rcampbell)
Attached patch patch-C refactoring - WIP (obsolete) — Splinter Review
Attachment #579332 - Attachment is obsolete: true
Attachment #579332 - Flags: feedback?(rcampbell)
Attachment #579785 - Flags: feedback?(rcampbell)
sorry for the delay. I plan on getting to this within the next couple of days.
Status: NEW → ASSIGNED
Blocks: 714443
Attachment #579327 - Flags: feedback?(rcampbell) → feedback+
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...
Attachment #579331 - Flags: feedback?(rcampbell) → feedback+
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.
Attachment #579333 - Flags: feedback?(rcampbell) → feedback+
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!
Attachment #579785 - Flags: feedback?(rcampbell) → feedback+
Priority: -- → P2
Blocks: 713676
Attachment #576745 - Attachment is patch: false
Attachment #579331 - Attachment is obsolete: true
Attached patch patch-D tests - v1 (obsolete) — Splinter Review
Attachment #588460 - Attachment description: patch-B reorder - v2 → patch-B reorder - v1
Attachment #576745 - Attachment is obsolete: true
Attachment #576745 - Flags: feedback?(jwalker)
Attachment #579327 - Attachment is obsolete: true
Attachment #579333 - Attachment is obsolete: true
Attachment #579785 - Attachment is obsolete: true
Attachment #588459 - Flags: review?(rcampbell)
Attachment #588460 - Flags: review?(rcampbell)
Attachment #588464 - Flags: review?(rcampbell)
Attachment #588465 - Flags: review?(rcampbell)
Blocks: 566092
Attachment #588459 - Flags: review?(rcampbell) → review+
Attachment #588460 - Flags: review?(rcampbell) → review+
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!
Attachment #588464 - Flags: review?(rcampbell) → review+
Attachment #588465 - Flags: review?(rcampbell) → review+
Whiteboard: [land-in-fx-team]
Whiteboard: [land-in-fx-team]
Thx for the r+.

Removing land-in-fx-team. Waiting for: https://tbpl.mozilla.org/?tree=Try&rev=001ce2cb9cb3
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js | highlighter is highlighting
Attachment #588465 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
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.
Attachment #588459 - Flags: approval-mozilla-aurora?
Attachment #588460 - Flags: approval-mozilla-aurora?
Attachment #588464 - Flags: approval-mozilla-aurora?
Attachment #589143 - Flags: approval-mozilla-aurora?
Attachment #588459 - Flags: approval-mozilla-aurora?
Attachment #588460 - Flags: approval-mozilla-aurora?
Attachment #588464 - Flags: approval-mozilla-aurora?
Attachment #589143 - Flags: approval-mozilla-aurora?
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: