1281171 - Implement a debug mode for the find highlighter

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[Mike de Boer [:mikedeboer]]

In order to understand the performance regressions better and better inspect the resulting layout using the devtools inspector tool.
At present this is impossible, because we're using the Anonymous Content API, which does not allow to keep node references.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: implement a debug mode for the finder highlighter

Comment 2

[Mike de Boer [:mikedeboer]]

Comment on attachment 8764924 [details] [diff] [review]
Patch 1: implement a debug mode for the finder highlighter

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

::: toolkit/modules/FinderHighlighter.jsm
@@ +13,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "Color", "resource://gre/modules/Color.jsm");
>  
> +const kDebug = true;

This should be true in the patch that lands, of course.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Comment on attachment 8764924 [details] [diff] [review]
> Patch 1: implement a debug mode for the finder highlighter
> 
> Review of attachment 8764924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/modules/FinderHighlighter.jsm
> @@ +13,5 @@
> >  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> >  
> >  XPCOMUtils.defineLazyModuleGetter(this, "Color", "resource://gre/modules/Color.jsm");
> >  
> > +const kDebug = true;
> 
> This should be true in the patch that lands, of course.

This should be *** false *** in the patch that lands, of course ;)

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8764924 [details] [diff] [review]
Patch 1: implement a debug mode for the finder highlighter

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

::: toolkit/modules/FinderHighlighter.jsm
@@ +13,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "Color", "resource://gre/modules/Color.jsm");
>  
> +const kDebug = true;

So this will require someone to recompile Firefox? I don't think it should be checked in then.

If you want to add a debug mode then create a hidden pref that allows it to be enabled if the pref has been created.

@@ +48,5 @@
>    padding-inline-end: 2px;
>    padding-bottom: 0;
>    padding-inline-start: 4px;
>    pointer-events: none;
> +  z-index: ${kDebug ? 2147483647 : 2};

My personal preference is that this looks a bit ugly combining this ternary inside of the big block of text here.

What do you think about using element.style.setProperty after the styles have been applied? Or setting a .debug className which will then apply different pre-defined styles?

@@ +643,5 @@
>      this._repaintHighlightAllMask(window);
>  
> +    this._modalHighlightOutline = kDebug ? mockAnonymousContentNode(
> +      document.body.appendChild(container.firstChild)) :
> +      document.insertAnonymousContent(container);

The indentation here is a bit confusing.

> this._modalHighlightOutline = kDebug ?
>   mockAnonymousContentNode(document.body.appendChild(container.firstChild)) :
>   document.insertAnonymousContent(container);

@@ +686,5 @@
>      // free to alter DOM nodes inside the CanvasFrame.
>      this._removeHighlightAllMask(window);
>  
> +    this._modalHighlightAllMask = kDebug ? mockAnonymousContentNode(
> +      document.body.appendChild(maskNode)) : document.insertAnonymousContent(maskNode);

ditto

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1281171 - implement a debug mode for the finder highlighter.

Review commit: https://reviewboard.mozilla.org/r/62664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62664/

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8768402 [details]
Bug 1281171 - implement a debug mode for the finder highlighter.

https://reviewboard.mozilla.org/r/62664/#review60184

Sorry for the delay in reviewing.

::: toolkit/modules/FinderHighlighter.jsm:658
(Diff revision 1)
>      container.appendChild(outlineBox);
>  
>      this._repaintHighlightAllMask(window);
>  
> -    this._modalHighlightOutline = document.insertAnonymousContent(container);
> +    this._modalHighlightOutline = kDebug ?
> +      mockAnonymousContentNode(document.body.appendChild(container.firstChild)) :

Was there something similar to mockAnonymousContentNode in the tests for these? Simply searching our source tree for 'mockAnonymousContentNode' doesn't show up, but if it already exists in the tests, it would be better to remove it from the tests and have the tests rely on this code.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8768402 [details]
Bug 1281171 - implement a debug mode for the finder highlighter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62664/diff/1-2/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/915257f0f74c
implement a debug mode for the finder highlighter. r=jaws

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/915257f0f74c