Closed Bug 529098 Opened 13 years ago Closed 13 years ago

Inform embedded Flash objects where to render

Categories

(Firefox for Android Graveyard :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
fennec1.0

People

(Reporter: stechz, Assigned: stechz)

References

Details

Attachments

(1 file, 7 obsolete files)

Attached patch WIP patch (obsolete) — Splinter Review
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.
Depends on: 528551
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.
also, see bug 529996
Attached patch WIP v2 (obsolete) — Splinter Review
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.
Attachment #412674 - Attachment is obsolete: true
Attached patch WIP v3 (obsolete) — Splinter Review
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?
Attachment #413781 - Attachment is obsolete: true
Attachment #413863 - Flags: review?(mark.finkle)
crash most probably bug 530378.
Attached patch Listen for render changes (obsolete) — Splinter Review
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!
Attachment #413863 - Attachment is obsolete: true
Attachment #413980 - Flags: review?(mark.finkle)
Attachment #413863 - Flags: review?(mark.finkle)
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.
Attachment #413980 - Flags: review?(mark.finkle) → review-
>   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"  ?
Attached patch Missed isToolbarLocked (obsolete) — Splinter Review
Attachment #413980 - Attachment is obsolete: true
Attached patch Cache with document (obsolete) — Splinter Review
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.
Attachment #413998 - Attachment is obsolete: true
Attachment #414084 - Flags: review?(mark.finkle)
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.
Attached patch Listen for paint events (obsolete) — Splinter Review
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.
Attachment #414084 - Attachment is obsolete: true
Attachment #414123 - Flags: review?(mark.finkle)
Attachment #414084 - Flags: review?(mark.finkle)
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"))
Attachment #414123 - Flags: review?(mark.finkle) → review-
Attachment #414123 - Attachment is obsolete: true
Attachment #414205 - Flags: review?(mark.finkle)
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.
Attachment #414205 - Flags: review?(mark.finkle) → review+
pushed with changes:
https://hg.mozilla.org/mobile-browser/rev/a20e808dd193
Assignee: nobody → webapps
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Post-B5
You need to log in before you can comment on or make changes to this bug.