958456 - [Highlighter] Hovering over comment nodes should make the highlighter disapear

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Steps to reproduce :

1 Go to http://www.mozilla.org/en-US/
2 Open Devtools Inspector Panel
3 In this panel, mouse hover #outer-wrapper. The highlighter correctly display #outer-wrapper in the main window.
4 In the Inspector Panel, now mouse hover the "<!-- close #outer-wrapper -->" just below. The highligher still displays #outer-wrapper.

Expected: the hightligher shoud disappear on hover HTML comment nodes.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: bug958456-hide-highlighter-on-comments.patch

Paul,
This patch is 2 lines of code!
+ a test of course.

ongoing try: https://tbpl.mozilla.org/?tree=Try&rev=083162024128

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8359797 [details] [diff] [review]
bug958456-hide-highlighter-on-comments.patch

Joe, Paul isn't going to have time right now to review this.

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 8359797 [details] [diff] [review]
bug958456-hide-highlighter-on-comments.patch

Review of attachment 8359797 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/inspector/test/browser_inspector_bug_958456_highlight_comments.js
@@ +54,5 @@
> +    yield hoverElement("#id4");
> +    assertHighlighterHidden();
> +
> +    finishTest();
> +  }).then(null, Cu.reportError);

This is mega picky, but since we're working out pattern for Task.jsm, you could also do:

    }).then(null, Cu.reportError).then(finishTest);

Which has the advantage that if one of the assert functions throws an exception, then the test fails rather than hanging.

Does Cu.reportError actually fail the test?

I wonder if we should have a convention of some boiler plate like:

    function test() {
      return Task.spawn(realTest).then(null, ok.bind(null, false)).then(finishTest);
    }

Comment 4

[Patrick Brosset <:pbro>]

Attachment: bug958456-hide-highlighter-on-comments v2.patch

Thanks Joe. I changed the test as advised. Indeed it's a better pattern.
Applied the R+ on this new patch.

Comment 5

[Patrick Brosset <:pbro>]

Try is green, going to check this in fx-team.

Comment 6

[Patrick Brosset <:pbro>]

Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/60395c47c63b

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/60395c47c63b