Closed Bug 540937 Opened 10 years ago Closed 10 years ago

[regression] Render plugin content in iframes

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set

Tracking

(status1.9.2 .1-fixed)

VERIFIED FIXED
Tracking Status
status1.9.2 --- .1-fixed

People

(Reporter: stechz, Assigned: stechz)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Fix (obsolete) — Splinter Review
When we switched to querySelectorAll we stopped rendering plugins in iframes.
tracking-fennec: --- → ?
Depends on: 540795
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.
Attachment #422620 - Flags: review?(mozbugz)
Attachment #422620 - Flags: review?(mark.finkle)
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.
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.
wrong bug
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
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?
> 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 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"
Attachment #422620 - Flags: review?(mark.finkle) → review+
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
Attachment #422620 - Flags: review+ → review-
Attachment #422620 - Attachment is obsolete: true
Attachment #423008 - Flags: review?(mark.finkle)
Attachment #422620 - Flags: review?(mozbugz)
Attachment #423008 - Flags: review?(mark.finkle) → review+
Pushed http://hg.mozilla.org/mobile-browser/rev/9989b0b68b5e
Status: NEW → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Flags: in-litmus? → in-litmus?(ayanshah62)
bugspam
Assignee: nobody → ben
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.