Closed Bug 1472684 Opened Last year Closed Last year

Performance drops when element highlighter is active

Categories

(DevTools :: Inspector: Animations, defect, P2)

defect

Tracking

(firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
Firefox 63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: mbalfanz, Assigned: daisuke)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Attached video performance.mp4
When using the element highlighter from the animation panel, the performance of the animation drops.  This effect doesn't occur when the highlighter is enabled through the rules panel.  The attached video shows the difference of both methods.

Note: might be related to high screen resolution, as I had an easy time to reproduce it on a 27" screen, but it was harder on a 15" screen.

Steps to reproduce:
1. Visit a website with animations, like [1]
2. Inspect the animation and select the animated element
3. Enable the element highlighter

Expected result:
- The animation should continue at least as good as when the highlighter is enabled from other places

Actual result:
- With the highlighter enabled through the animation inspector, the performance drops.  The effect seems to be stronger on higher screen resolutions, though it is noticeable also on small screens.


[1]: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Animations/Using_CSS_animations#Making_it_move_back_and_forth
Thanks, Martin.
I could reproduce this bug.
Priority: -- → P2
Assignee: nobody → dakatsuka
Comment on attachment 8989343 [details]
Bug 1472684: Hide info bar and guides from highlighter.

https://reviewboard.mozilla.org/r/254410/#review261458

::: devtools/client/inspector/animation/animation.js:521
(Diff revision 1)
>     */
> -  async setHighlightedNode(nodeFront) {
> +  async setHighlightedNode(nodeFront, option) {
>      await this.inspector.highlighters.hideBoxModelHighlighter();
>  
>      if (nodeFront) {
> -      await this.inspector.highlighters.showBoxModelHighlighter(nodeFront);
> +      await this.inspector.highlighters.showBoxModelHighlighter(nodeFront, option);

I would just prefer that we do 

await this.inspector.highlighters.showBoxModelHighlighter(nodeFront, { hideInfoBar: true, hideGuides: true })

since the options don't actually change.
Attachment #8989343 - Flags: review?(gl) → review+
Comment on attachment 8989343 [details]
Bug 1472684: Hide info bar and guides from highlighter.

https://reviewboard.mozilla.org/r/254410/#review261458

> I would just prefer that we do 
> 
> await this.inspector.highlighters.showBoxModelHighlighter(nodeFront, { hideInfoBar: true, hideGuides: true })
> 
> since the options don't actually change.

Okay, thanks!
I'd apply same way for showBoxModelHighlighter in AnimationTarget as well.
I had wanted to define the option in one place.
Pushed by dakatsuka@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6b45def3335
Hide info bar and guides from highlighter. r=gl
https://hg.mozilla.org/mozilla-central/rev/d6b45def3335
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Comment on attachment 8989343 [details]
Bug 1472684: Hide info bar and guides from highlighter.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1399830
[User impact if declined]: If user highlights node by animation inspector, performance of the animation will be down.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: It has baked on Nightly for 1 day.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Low
[Why is the change risky/not risky?]: This is only an option changing for highlighter module.
[String changes made/needed]: No
Attachment #8989343 - Flags: approval-mozilla-beta?
Comment on attachment 8989343 [details]
Bug 1472684: Hide info bar and guides from highlighter.

Perf fix, should only affect animation inspector.
OK to uplift for beta 7.
Attachment #8989343 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Based on comment 8, marking as qe-.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.