Closed Bug 1281171 Opened 3 years ago Closed 3 years ago

Implement a debug mode for the find highlighter

Categories

(Toolkit :: Find Toolbar, defect)

defect
Not set
Points:
2

Tracking

()

RESOLVED FIXED
mozilla50
Iteration:
50.1 - Jun 20
Tracking Status
firefox50 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Flags: qe-verify-
Flags: firefox-backlog+
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
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.
Attachment #8764924 - Flags: review?(jaws)
(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 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
Attachment #8764924 - Flags: review?(jaws)
Attachment #8764924 - Attachment is obsolete: true
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.
Attachment #8768402 - Flags: review?(jaws) → review+
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/
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/915257f0f74c
implement a debug mode for the finder highlighter. r=jaws
https://hg.mozilla.org/mozilla-central/rev/915257f0f74c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1306320
No longer blocks: 1306320
You need to log in before you can comment on or make changes to this bug.