The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Unassigned)

Tracking

Trunk
Firefox 12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 10 obsolete attachments)

43.77 KB, patch
Details | Diff | Splinter Review
10.28 KB, patch
Details | Diff | Splinter Review
48.26 KB, patch
Details | Diff | Splinter Review
33.50 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 653545
(Reporter)

Comment 1

5 years ago
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.
Attachment #574959 - Flags: feedback?(rcampbell)
(Reporter)

Updated

5 years ago
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?
(Reporter)

Comment 3

5 years ago
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
(Reporter)

Comment 4

5 years ago
Created attachment 575125 [details] [diff] [review]
Proposed refactoring - v0.2

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
(Reporter)

Updated

5 years ago
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector
(Reporter)

Comment 7

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
(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
(Reporter)

Comment 9

5 years ago
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.
Attachment #575125 - Attachment is obsolete: true
Attachment #575125 - Flags: feedback?(rcampbell)
(Reporter)

Comment 10

5 years ago
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.
(Reporter)

Comment 11

5 years ago
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?
Attachment #576716 - Attachment is obsolete: true
Attachment #576745 - Flags: feedback?(jwalker)
(Reporter)

Updated

5 years ago
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().
filed bug 705131
(Reporter)

Comment 16

5 years ago
(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()?

Comment 19

5 years ago
That could also be interesting for the Firebug team. So we could reuse the Inspector, but use our own highlighter.
(Reporter)

Comment 20

5 years ago
(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.
(Reporter)

Comment 21

5 years ago
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?
(Reporter)

Comment 23

5 years ago
(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.
(Reporter)

Comment 24

5 years ago
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.
(Reporter)

Comment 27

5 years ago
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()
(Reporter)

Comment 29

5 years ago
(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.
(Reporter)

Updated

5 years ago
Assignee: nobody → paul
(Reporter)

Comment 31

5 years ago
(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.
(Reporter)

Comment 33

5 years ago
Created attachment 579327 [details] [diff] [review]
patch-A split - WIP
(Reporter)

Comment 34

5 years ago
Created attachment 579331 [details] [diff] [review]
patch-B reorder - WIP
(Reporter)

Comment 35

5 years ago
Created attachment 579332 [details] [diff] [review]
patch-C refactoring - WIP
(Reporter)

Comment 36

5 years ago
Created attachment 579333 [details] [diff] [review]
patch-D tests - WIP
(Reporter)

Updated

5 years ago
Attachment #579327 - Flags: feedback?(rcampbell)
(Reporter)

Updated

5 years ago
Attachment #579331 - Flags: feedback?(rcampbell)
(Reporter)

Updated

5 years ago
Attachment #579332 - Flags: feedback?(rcampbell)
(Reporter)

Updated

5 years ago
Attachment #579333 - Flags: feedback?(rcampbell)
(Reporter)

Comment 37

5 years ago
Created attachment 579785 [details] [diff] [review]
patch-C refactoring - WIP
(Reporter)

Updated

5 years ago
Attachment #579332 - Attachment is obsolete: true
Attachment #579332 - Flags: feedback?(rcampbell)
(Reporter)

Updated

5 years ago
Attachment #579785 - Flags: feedback?(rcampbell)
sorry for the delay. I plan on getting to this within the next couple of days.
Status: NEW → ASSIGNED
(Reporter)

Updated

5 years ago
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+
(Reporter)

Updated

5 years ago
Priority: -- → P2
(Reporter)

Updated

5 years ago
Blocks: 713676
(Reporter)

Updated

5 years ago
Attachment #576745 - Attachment is patch: false
(Reporter)

Comment 42

5 years ago
Created attachment 588459 [details] [diff] [review]
patch-A split - v1
(Reporter)

Comment 43

5 years ago
Created attachment 588460 [details] [diff] [review]
patch-B reorder - v1
(Reporter)

Updated

5 years ago
Attachment #579331 - Attachment is obsolete: true
(Reporter)

Comment 44

5 years ago
Created attachment 588464 [details] [diff] [review]
patch-C refactoring - v1
(Reporter)

Comment 45

5 years ago
Created attachment 588465 [details] [diff] [review]
patch-D tests - v1
(Reporter)

Updated

5 years ago
Attachment #588460 - Attachment description: patch-B reorder - v2 → patch-B reorder - v1
(Reporter)

Updated

5 years ago
Attachment #576745 - Attachment is obsolete: true
Attachment #576745 - Flags: feedback?(jwalker)
(Reporter)

Updated

5 years ago
Attachment #579327 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #579333 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #579785 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #588459 - Flags: review?(rcampbell)
(Reporter)

Updated

5 years ago
Attachment #588460 - Flags: review?(rcampbell)
(Reporter)

Updated

5 years ago
Attachment #588464 - Flags: review?(rcampbell)
(Reporter)

Updated

5 years ago
Attachment #588465 - Flags: review?(rcampbell)
(Reporter)

Updated

5 years ago
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]
(Reporter)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
(Reporter)

Comment 47

5 years ago
Thx for the r+.

Removing land-in-fx-team. Waiting for: https://tbpl.mozilla.org/?tree=Try&rev=001ce2cb9cb3
(Reporter)

Comment 48

5 years ago
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser_webconsole_bug_653531_highlighter_console_helper.js | highlighter is highlighting
(Reporter)

Comment 49

5 years ago
Created attachment 589143 [details] [diff] [review]
patch-D tests - v1.1
(Reporter)

Updated

5 years ago
Attachment #588465 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/15d0a2e0c3c8

https://hg.mozilla.org/integration/fx-team/rev/3e736f012ce8

https://hg.mozilla.org/integration/fx-team/rev/a62e69926f83

https://hg.mozilla.org/integration/fx-team/rev/3446a54199ab
Assignee: paul → nobody
Priority: P2 → --
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/15d0a2e0c3c8
https://hg.mozilla.org/mozilla-central/rev/3e736f012ce8
https://hg.mozilla.org/mozilla-central/rev/a62e69926f83
https://hg.mozilla.org/mozilla-central/rev/3446a54199ab
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 12
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?
You need to log in before you can comment on or make changes to this bug.