529098 - Inform embedded Flash objects where to render

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Benjamin Stover (:stechz)]

Attachment: WIP patch

Lets embedded objects skip the gecko rendering pipeline.  WIP patch tries to set clip rectangle to empty whenever dialogs might be on top of it.

Comment 1

[Doug Turner (:dougt)]

couple of things:

* no longer needed to call getInterface to get access to nsIObjectLoadingContent.  Just QI now.

* on the mozilla-1.9.2 branch, the interface you need is nsIObjectLoadingContent_MOZILLA_1_9_2_BRANCH.

Comment 2

[Doug Turner (:dougt)]

also, see bug 529996

Comment 3

[Benjamin Stover (:stechz)]

Attachment: WIP v2

Not much is different, but now flash is not shown when preferences are open and updated for doug's patches that have landed in trunk and 1.9.2.  Pretty ugly still.

Todo: integrate mfinkle's broadcaster stuff from bug 524479, use broadcasters that doug mentioned, and put flash stuff in its own self contained object as much as possible.

Comment 4

[Benjamin Stover (:stechz)]

Attachment: WIP v3

This patch has my basic idea for cleanly doing this.  There is a self-contained object in browser.js that listens for events and updates flash objects if there are any.  I think I need to add events for when rendering is paused or started again.

Right now, calling doug's setAbsoluteScreenPosition is currently crashing the browser, but my old patch didn't.  Doug: can you try to debug this?  Mark: what do you think of this patch?

Comment 5

[Doug Turner (:dougt)]

crash most probably bug 530378.

Comment 6

[Benjamin Stover (:stechz)]

Attachment: Listen for render changes

This patch is now as functional as the ugly patch.  Covered areas: embed size changes, new embeds, tab select, pauserender, resumerender, zoom, open url bar, preferences.

All without adding flash hacks all over the place.  Yay!

Comment 7

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

Comment on attachment 413980 [details] [diff] [review]
Listen for render changes

>diff --git a/chrome/content/BrowserView.js b/chrome/content/BrowserView.js

>+    if (this._browser) {
>+      let event = document.createEvent("Events");
>+      event.initEvent("Zoom", true, false);

"ZoomChanged"

>   pauseRendering: function pauseRendering() {
>     this._renderMode++;
>+    if (this._renderMode == 1 && this._browser) {
>+      let event = document.createEvent("Events");
>+      event.initEvent("RenderChange", true, false);
>+      this._browser.dispatchEvent(event);
>+    }
>   },
> 
>   /**
>    * Calls to this function need to be one-to-one with calls to
>    * pauseRendering()
>    */
>   resumeRendering: function resumeRendering(renderNow) {
>     if (this._renderMode > 0)
>       this._renderMode--;
> 
>     if (renderNow || this._renderMode == 0)
>       this.renderNow();
> 
>-    if (this._renderMode == 0)
>+    if (this._renderMode == 0) {
>       this._idleServiceObserver.resumeCrawls();
>+
>+      if (this._browser) {
>+        let event = document.createEvent("Events");
>+        event.initEvent("RenderChange", true, false);
>+        this._browser.dispatchEvent(event);
>+      }
>+    }

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>     // If no more dialogs are being displayed, remove the attr for CSS
>-    if (!this._dialogs.length)
>+    if (!this._dialogs.length) {
>       document.getElementById("toolbar-main").removeAttribute("dialog");
>+      document.getElementById("cast_contentShowing").setAttribute("disabled", "false");

removeAttribute please. disabled is one of those painful attributes whose mere presence canbe enough to mean "true"

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>--- a/chrome/content/browser.js
>+++ b/chrome/content/browser.js
>@@ -576,16 +576,18 @@ var Browser = {
>     // Re-enable flash
>     if (gPrefService.prefHasUserValue("temporary.disabledFlash")) {
>       this.setPluginState(true, /flash/i);
>       gPrefService.clearUserPref("temporary.disabledFlash");
>     }
> 
>     // Force commonly used border-images into the image cache
>     ImagePreloader.cache();
>+
>+    (new EmbedObserver(bv)).start();

I think we want this a a member variable and we want a "stop()" method on it, called from "shutdown()" so you can unhook the plugin observer


>     let firstTab = this._selectedTab == null;
>+    let lastTab = this._selectedTab;

firstTab is now looking a bit to ambiguously like it should be a Tab not a boolean. Let's rename it to isFirstTab

>+  /** Starts flash objects fast path. */
>+  start: function() {
>+    document.addEventListener("TabSelect", this, false);

document is a bit high for the listener. Use the tabs-container

>+    this._contentShowing.addEventListener("broadcast", this, false);
>+    let browsers = document.getElementById("browsers");
>+    browsers.addEventListener("Zoom", this, false);

"ZoomChanged"

>+    browsers.addEventListener("VisibleMove", this, false);

Where do we fire this? And "VisibleMove" is horribly vague. What is moving? Has it moved or will it move?

>+    browsers.addEventListener("RenderChange", this, false);
>+    let observerService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
>+    observerService.addObserver(this, "plugin-changed-event", false);

You're not using a weak reference, so you need to explicitly remove this observer. Could be causing crashes. Or use a weak reference.

>+    if (data == "init") {
>+      if (browser.embedCache === undefined)

embedCache? how about pluginCache? I didn't realize you meant the tag name and though it was some embedding thing.

>+        browser.embedCache = [];
>+      browser.embedCache.push(subject);

Where are we removing plugins from the cache? I assume we should clear the cache (or delete the property from the browser object) when a new page is loaded.

>+    let disabled = document.getElementById("cast_contentShowing");
>+    if (disabled.getAttribute("disabled") == "true")

disabled.hasAttribute("disabled")

>+  updateEmbedRegions: function updateEmbedRegions(objects, crit) {
>+    let bv = this._bv;
>+    let interface = Ci.nsIObjectLoadingContent_MOZILLA_1_9_2_BRANCH || Ci.nsIObjectLoadingContent;

Move this out to a global name nsIObjectLoadingContent, we don't need to call Ci.Xxx all the time.

>+    let oprivate, r, dest, clip;
>+    for (let i = objects.length - 1; i >= 0; i--) {
>+      if (!objects[i]) {
>+        objects.splice(i, 1);
>+      } else {
>+        r = bv.browserToViewportRect(Browser.getBoundingContentRect(objects[i]));
>+        dest = Browser.browserViewToClientRect(r);
>+        clip = Browser.browserViewToClientRect(r.intersect(crit)).translate(-dest.left, -dest.top);

Browser.browserViewToClientRect seems better suited as BrowserView.viewportToClientRect. What do you think?

>diff --git a/chrome/content/browser.xul b/chrome/content/browser.xul

>+
>+  <observerset id="observerSet">

<observerset id="observerset">

>-  <popupset id="mainPopupSet">
>+  <popupset id="popupset">
>   </popupset>

Let's just remove this altogether


I noticed that none of this code looks at the plugin.enabled preference. I assume we don't need any of this code to execute if plugins are disabled.

Comment 8

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

>   pauseRendering: function pauseRendering() {

>+    if (this._renderMode == 1 && this._browser) {
>+      let event = document.createEvent("Events");
>+      event.initEvent("RenderChange", true, false);
>+      this._browser.dispatchEvent(event);
>+    }

>   resumeRendering: function resumeRendering(renderNow) {

>+      if (this._browser) {
>+        let event = document.createEvent("Events");
>+        event.initEvent("RenderChange", true, false);
>+        this._browser.dispatchEvent(event);
>+      }

I don't like "RenderChange" as an event name. And you aren't passing any context about whether we paused or resumed.

"RenderPaused" / "RenderResume"  ?

Comment 9

[Benjamin Stover (:stechz)]

Attachment: Missed isToolbarLocked

Comment 10

[Benjamin Stover (:stechz)]

Attachment: Cache with document

Code review changes as well.

Did not change the name for browserViewToClientRect since I'd have to change the names of browserViewToClient and clientToBrowserView for consistency.  I do think that whole transformation mess is pretty ugly, but hopefully we can address that some other time.

Comment 11

[Benjamin Stover (:stechz)]

Also added PrefStartListener that calls object.start() and object.stop() based on a boolean preference's value, and listens for changes.  It does not hold a reference to the pref branch so there is no cyclical reference, and will remain around until pref service is gone.  It seemed unnecessary to store it anywhere.

I kept it pretty modular so that anything that follows a start/stop pattern could use it.  I have no clue if it would be useful anywhere else, but something like it had to be written for EmbedObserver.

Comment 12

[Benjamin Stover (:stechz)]

Attachment: Listen for paint events

Per Doug's request.  Come to think of it EmbedObserver should probably only listen for paint/zoom events on the current browser so that users with lots of tabs aren't penalized.  TBD.

Comment 13

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

Comment on attachment 414123 [details] [diff] [review]
Listen for paint events

>+  isToolbarLocked: function isToolbarLocked() {
>+    return this._toolbarLocked;
>+  },
>   lockToolbar: function lockToolbar() {

Add a blank line between them

>+    this._embedObserver = new EmbedObserver(bv);

_pluginObserver and PluginObserver

>+    new PrefStartListener("plugins.enabled", this._embedObserver);

Sounds like a handy addition to Util?  Util.PreferenceToggle. Nevermind, we can do that later.

>+/** Call obj.start() or stop() based on boolean pref. */
>+function PrefStartListener(pref, obj) {

PreferenceToggle

>+  gPrefService.addObserver(pref, this, false);

false means this is not a weak reference, therefore you should .removeObserver too

A weak reference sounds like the best idea here.

>+PrefStartListener.prototype = {

>+  _check: function _check() {
>+    let value = gPrefService.getBoolPref(this._pref);

Wrap this in a try/catch - getBoolPref will throw an exception if the preference doesn't exist, for whatever reason.

>+const gObjectLoadingContentIFace = Ci.nsIObjectLoadingContent_MOZILLA_1_9_2_BRANCH || Ci.nsIObjectLoadingContent;

Please | const nsIObjectLoadingContent = ... |  is good enough and is more typical.

>+EmbedObserver.prototype = {

PluginObserver

>+  /** Update flash objects */
>+  handleEvent: function handleEvent(ev) {

This is the function where you should probably be filtering on the current browser. You can get the browser from the event using the document to find the browser - or use the selectedBrowser.contentDocument to compare to the document found from the event.

>+    let disabled = document.getElementById("bcast_contentShowing");

remove this line

>+    if (disabled.hasAttribute("disabled"))

becomes:
      if (Elements.contentShowing.hasAttribute("disabled"))

Comment 14

[Benjamin Stover (:stechz)]

Attachment: Fix some flash and code review changes

Comment 15

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

Comment on attachment 414205 [details] [diff] [review]
Fix some flash and code review changes


> let Util = {
>+  WeakReferenceQueryInterface: function(iid) {
>+   if (iid.equals(Ci.nsIObserver) ||
>+       iid.equals(Ci.nsISupportsWeakReference) ||
>+       iid.equals(Ci.nsISupports))
>+     return this;
>+   throw Components.results.NS_ERROR_NO_INTERFACE;
>+  },

Let's not do this. XPCOMUtils.jsm has a factory for QI if we want one.

>+const gObjectLoadingContent = Ci.nsIObjectLoadingContent_MOZILLA_1_9_2_BRANCH || Ci.nsIObjectLoadingContent;

nsIObjectLoadingContent would be the typical naming convention

>+  start: function() {

>+    let browser = Browser.selectedBrowser;
>+    if (browser) {
>+      browser.addEventListener("ZoomChanged", this, false);
>+      browser.addEventListener("MozAfterPaint", this, false);
>+    }

>+  stop: function() {

>+    let browser = Browser.selectedBrowser;
>+    if (browser) {
>+      browser.removeEventListener("ZoomChanged", this, false);
>+      browser.removeEventListener("MozAfterPaint", this, false);
>+    }

>+  handleEvent: function handleEvent(ev) {
>+        browser.removeEventListener("ZoomChanged", this, false);
>+        browser.removeEventListener("MozAfterPaint", this, false);

>+      browser.addEventListener("ZoomChanged", this, false);
>+      browser.addEventListener("MozAfterPaint", this, false);

This is a lot of complexity to keep the events on the selected browser. I'm not sure it's worth the effort. We can look again after landing the patch.


I can make the two changes when landing.

Comment 16

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

pushed with changes:
https://hg.mozilla.org/mobile-browser/rev/a20e808dd193