540937 - [regression] Render plugin content in iframes

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: Fix

When we switched to querySelectorAll we stopped rendering plugins in iframes.

Comment 1

[Benjamin Stover (:stechz)]

Comment on attachment 422620 [details] [diff] [review]
Fix

Nit: need to remove comments for commit and begin batch ops.  Replaced code does the same thing without causing flash to render every 2 seconds during page load.

Comment 2

[Stuart Parmenter]

I don't understand the nit -- can you post a new patch?  If you're going to try and remove the begin/commit, you're going to also have to get rid of the one in the function that sets up the timer, and that will effect painting during page loads..  I don't think we want to take a change like that right now.

Comment 3

[Benjamin Stover (:stechz)]

The nit is deleting the commented out code in the diff.

No need to get rid of beginBatch in set up timer.

forceViewport does the same thing as calling commitBatch then beginBatch.  Instead of calling commit/begin the viewport changes are applied with forceViewportChange (similar to renderNow for pauseRendering/beginRendering calls).  The only difference is that the plugin observer doesn't get a "render change" event so it doesn't try to draw flash every 2 seconds during load.

The whole reason the change is there is querySelectorAll in all the iframes of a document can get pretty expensive during page load.

If it still scares you we can just check for current tab _loading property in the plugin observer.

Comment 4

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/4d906fab5a87
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bfec812ca05c

Comment 5

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

wrong bug

Comment 6

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

Comment on attachment 422620 [details] [diff] [review]
Fix


>   _resizeAndPaint: function _resizeAndPaint() {
>     let bv = Browser._browserView;
> 
>-    bv.commitBatchOperation();
>+    bv.forceViewportChange();
>+//    bv.commitBatchOperation();
> 
>     // kick ourselves off 2s later while we're still loading
>-    bv.beginBatchOperation();
>+//    bv.beginBatchOperation();
>     this._loadingTimeout = setTimeout(this._resizeAndPaint, 2000);

What's going on here? Why the the change better?

>-        let plugins = doc.querySelectorAll("embed,object");
>-        this.updateEmbedRegions(plugins, rect);
>+      this.updateEmbedRegions(this.getPluginNodes(doc), rect);

>-        let plugins = doc.querySelectorAll("embed,object");
>-        self.updateEmbedRegions(plugins, self.getCriticalRect());
>+        self.updateEmbedRegions(self.getPluginNodes(doc), 

Ca we check for an empty plugin array and bail out of updateEmbedRegions right away?

Comment 7

[Benjamin Stover (:stechz)]

> What's going on here? Why the the change better?

See comment #3.  It makes sure that plugin observer isn't fired at all during page load.

> Ca we check for an empty plugin array and bail out of updateEmbedRegions right
> away?

We could but I don't see the point.  I'm pretty sure that wouldn't save us any time.

Comment 8

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

Comment on attachment 422620 [details] [diff] [review]
Fix


>   _resizeAndPaint: function _resizeAndPaint() {
>     let bv = Browser._browserView;
> 
>-    bv.commitBatchOperation();
>+    bv.forceViewportChange();
>+//    bv.commitBatchOperation();
> 
>     // kick ourselves off 2s later while we're still loading
>-    bv.beginBatchOperation();
>+//    bv.beginBatchOperation();
>     this._loadingTimeout = setTimeout(this._resizeAndPaint, 2000);
>   },

startResizeAndPaint does a beginBatchOperation. stopResizeAndPaint does a commitBatchOperation. resizeAndPaint does both to make sure the order of operations remains unbroken.

You removed the begin/commit from resizeAndPaint so the "RenderStateChanged" event doesn't get fired by the commitBatchOperation, if the batchOps stack was only one level deep. More than one level deep and the "RenderStateChanged" would not be fired anyway. "RenderStateChanged" is caught by the plugin observer and forces a repaint for all plugins on a page.

forceViewportChange does not fire "RenderStateChanged" but does unconditionally execute applyViewportChanges. commitBatchOperations only fires applyViewportChanges if the batchOps depth is 1 when it's called.

Am I on track so far?

So your goal is to stop "RenderStateChanged" events from firing during pageload, which calls a big "find all plugins" function and a "paint all plugins" function. You are also assuming that the commitBatchOperations call in resizeAndPaint _will_ end up calling applyViewportChanges every time. Therefore, forceViewportChange is not causing extra viewport updates.

If this is true, then r+

_but_ add a comment to resizeAndPaint: "forceViewportChange doesn't fire a 'RenderStateChanged' event which avoids rendering plugins while pageloading"

Comment 9

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

Comment on attachment 422620 [details] [diff] [review]
Fix

After talking it over on IRC, let's keep resizeAndPaint as is and add an isLoading check in the plugin observer.

It easier to understand you're intent and less risky. We can look at changing resizeAndPaint after 1.0

Comment 10

[Benjamin Stover (:stechz)]

Attachment: Do it the boring way

Comment 11

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mobile-browser/rev/9989b0b68b5e

Comment 12

[Aakash Desai [:aakashd]]

verified FIXED on build:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2pre) Gecko/20100124 Namoroka/3.6pre Fennec/1.0pre


...over the following sites:

www.clubpenguin.com
www.fandango.com
www.newgrounds.com/toons

Comment 13

[Dão Gottwald [:dao]]

bugspam