1148107 - isTooSmall() @ PluginHelper.js causes unnecessary reflow

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

As part of PluginHelper.handlePluginBindingAttached(), we call a function isTooSmall() which is used to determine if the plugin is too small for the overlay, etc. This causes a costly page reflow. We should consider just showing the overlay, and going back later to show doorhangers if necessary.

Comment 1

[Benjamin Smedberg]

I don't understand. Are you saying that we're doing an extra reflow that we wouldn't normally do because of this function? Once we've done initial layout, this function shouldn't be causing the entire page to reflow, just the boxes internal to the binding.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Profile

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Code looks like:
127   isTooSmall : function (plugin, overlay) {
128     // Is the <object>'s size too small to hold what we want to show?
129     let pluginRect = plugin.getBoundingClientRect();
130     // XXX bug 446693. The text-shadow on the submitted-report text at
131     //     the bottom causes scrollHeight to be larger than it should be.
132     let overflows = (overlay.scrollWidth > pluginRect.width) ||
133                     (overlay.scrollHeight - 5 > pluginRect.height);
134 
135     return overflows;
136   },


Looks like we could use getBoundsWithoutFlushing [1] to replace the getBoundingClientRect. I assume we'll need to handle the scrollWidth and scrollHeight too.

That siad, I see Desktop using getBoundingClientRect too (We copied them), but I do see they do a scrollWidth check to see if "layout has finished or not" [2]. We could add that.


[1] http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#928
[2] http://mxr.mozilla.org/mozilla-central/source/browser/modules/PluginContent.jsm#173

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: toosmall-tweak v0.1

This patch just aligns mobile to the current desktop code a bit better than before. Looking at the code, we could/should do an audit on the CtP front-end code to find more places to update.

Also, I don't really think this solves our flushing/reflow problem. It looks like Element.scrollWidth calls GetScrollFrame with flags set to force a flush.

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> I don't understand. Are you saying that we're doing an extra reflow that we
> wouldn't normally do because of this function? Once we've done initial
> layout, this function shouldn't be causing the entire page to reflow, just
> the boxes internal to the binding.

I think it's most likely not an additional reflow. It might be possible that this isn't really hurting us at all, but it looks suspicious in the profile. One thing that I'm curious about is if this triggers an uninterruptible layout, vs an interruptible one if the reflow was allowed to happen "normally".

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Does this patch really work? It looks like we'd need to run the stuff in the "PluginClickToPlay" binding handler again, but that won't happen. I think some sort of deferred thing would be a lot better. "OK, everything's loaded, see if we need to ask about small plugins." It doesn't need to be a in critical layout path.

Comment 7

[BMO Automation]

Resolving as wont fix, plugin support deprecated in Firefox 85.