Closed Bug 1053413 Opened 5 years ago Closed 5 years ago

Switch DOM Fullscreen API from using nsIObserverService to nsIMessageManager or similar

Categories

(Core :: DOM: Content Processes, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
e10s - ---
firefox41 --- fixed

People

(Reporter: mconley, Assigned: xidorn)

References

Details

Attachments

(8 files, 4 obsolete files)

4.38 KB, patch
dao
: review+
Details | Diff | Splinter Review
15.28 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.97 KB, patch
dao
: review+
Details | Diff | Splinter Review
5.32 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
14.73 KB, patch
smaug
: review+
Details | Diff | Splinter Review
20.02 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.88 KB, patch
smaug
: review+
Details | Diff | Splinter Review
I'm currently fixing bug 961362, which fixes the DOM Fullscreen API for e10s on Firefox Desktop.

I'm re-using the cross-process infrastructure that the B2G folk kindly put together so that we've got One Common Way for all of this stuff to happen - either in B2G, Firefox (with e10s), or Firefox (no e10s).

There are, unfortunately, some drawbacks with the current cross-process infrastructure for DOM Fullscreen API. Specifically, its reliance on nsIObserverService to communicate that we should be switching to fullscreen.

This is fine for B2G, which has a single root window to worry about. It's tougher for Firefox Desktop, which can have many root windows, when only one of them needs to care about entering fullscreen mode.

My patch in bug 961362 does a little dance where it detects the DOM element (either the frame loader with e10s, or the actual requesting element with non-e10s) that is requesting fullscreen, and then comparing chrome event handlers to determine which browser should react to the notification.

I think a better, long-term fix would be to use nsIMessageManager messages instead, so that only the browser that wants to enter DOM Fullscreen gets the message about it.

So, filing this as follow-up work once I get bug 961362 landed.
Note that I spoke with fabrice in #b2g, who said that they'd welcome patches to switch this over to messages instead of notifications, assuming that it didn't break them (naturally).

I was afraid that there might have been some technical limitation which required shell.js to use observer notifications for everything, but it looks like that's not the case.
This is definitely not an M2 blocker, but this would help clean up some technical debt.
tracking-e10s: --- → ?
NeilAway also suggested we go the other route, and have B2G use the MozEnteredDomFullscreen stuff that desktop currently uses.
Probably we need to do this cleanup before bug 1161802, so that the interaction between processes could be clearer.
Assignee: nobody → quanxunzhen
Blocks: 1161802
Attachment #8604916 - Flags: review?(dao)
Attachment #8604916 - Flags: review?(bugs)
Attachment #8604926 - Attachment description: patch 5 - remove usage of ask-parent-to-* notification in browser elem → patch 5 - remove usage of fullscreen notification in browser elem
Attachment #8604915 - Flags: review?(dao) → review+
Comment on attachment 8604916 [details] [diff] [review]
patch 2 - separate origin change into an independent event

>--- a/browser/base/content/browser-fullScreen.js
>+++ b/browser/base/content/browser-fullScreen.js

>+const FULLSCREEN_MESSAGES = [
>+  "DOMFullscreen:Entered",
>+  "DOMFullscreen:OriginChanged",
>+  "DOMFullscreen:Exited"
>+];
>+
> var FullScreen = {

browser-fullScreen.js doesn't have its own scope but shares it with lots of other scripts, so please tack FULLSCREEN_MESSAGES onto the FullScreen object (e.g. as MESSAGES) rather than the global scope.
Attachment #8604916 - Flags: review?(dao) → review-
Comment on attachment 8604925 [details] [diff] [review]
patch 4 - remove usage of ask-parent-to-* notification in e10s

>--- a/browser/base/content/browser-fullScreen.js
>+++ b/browser/base/content/browser-fullScreen.js

>+  _isRemoteBrowser: function (aBrowser) {
>+    return gMultiProcessBrowser && aBrowser.getAttribute("remote") === "true";

nit: use ==, since === adds no value here
Attachment #8604925 - Flags: review?(dao) → review+
Ah... It seems I shouldn't have split patch 2 and patch 3... Patch 2 doesn't work on its own...
Attachment #8604916 - Attachment is obsolete: true
Attachment #8604916 - Flags: review?(bugs)
Attachment #8605534 - Flags: review?(dao)
Attachment #8605534 - Flags: review?(bugs)
Comment on attachment 8605534 [details] [diff] [review]
patch 2 - separate origin change into an independent event

>+    this._MESSAGES.forEach(
>+      type => window.messageManager.addMessageListener(type, this));

I'd prefer:

for (let type of this._MESSAGES)
  window.messageManager.addMessageListener(type, this);

>   uninit: function() {
>-    window.messageManager.removeMessageListener("MozEnteredDomFullscreen", this);
>-    window.messageManager.removeMessageListener("MozExitedDomFullscreen", this);
>+    this._MESSAGES.forEach(
>+      type => window.messageManager.removeMessageListener(type, this));
>     this.cleanup();
>   },

ditto
Attachment #8605534 - Flags: review?(dao) → review+
This patchset causes some test failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=923b4074f386

Some of them are simply because they tested the old behavior, some of them are more tricky. But anyway, I've fixed all of them expect the pointerlock test on OS X 10.6.

The pointerlock test is interesting because it has been effectively disabled on all other platforms (that's why it doesn't fail anywhere else in the treeherder)

I probably should open another bug and try to fix pointerlock (as well as its tests) in most platforms before continuing this bug.
OK I now know how I broke the pointerlock test. But anyway, I filed bug 1165158 to enable the pointerlock tests. Probably I could look into that when I have time later.
Add the test change.
Attachment #8605534 - Attachment is obsolete: true
Attachment #8605534 - Flags: review?(bugs)
Attachment #8606062 - Flags: review?(bugs)
Comment on attachment 8606061 [details] [diff] [review]
patch 0 - fix some fullscreen tests

>+  function invokeCallback(event) {
>+    // Use double async call to workaround unfinished fullscreen change
>+    // even when the window size has changed on Linux.
>+    setTimeout(() => setTimeout(() => callback(event), 0), 0);
>+  }
Wouldn't 
requestAnimationFrame(() => setTimeout(() => callback(event), 0));
be even a bit safer, and closer to right. Asynchronously do something after we've
got one paint.
Attachment #8606061 - Flags: review?(bugs) → review+
Comment on attachment 8606062 [details] [diff] [review]
patch 2 - separate origin change into an independent event, r=dao

>+  // If it is the first entry of the fullscreen, trigger an event so
>+  // that the UI can response to this change, e.g. hide chrome, or
>+  // notifying parent process to enter fullscreen. Note that chrome
>+  // code may also want to listen to MozFullscreenOriginChanged event
>+  // to pop up warning/approval UI.
>+  if (!previousFullscreenDoc) {
>+    nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
>+      new AsyncEventDispatcher(
>+        this, NS_LITERAL_STRING("MozEnteredDomFullscreen"),
>+        /* Bubbles */ true, /* ChromeOnly */ true);
>+    asyncDispatcher->PostDOMEvent();
>+  }
>+
>+  // The origin which is fullscreen gets changed. Trigger an event so
>+  // that the chrome knows to pop up a warning/approval UI. Note that
>+  // previousFullscreenDoc == nullptr upon first entry, so we always
>+  // take this path on the first entry. Also note that, in a multi-
>+  // process browser, the code in content process is responsible for
>+  // sending message with the origin to its parent, and the parent
>+  // shouldn't rely on this event itself.
>+  if (aNotifyOnOriginChange &&
>       !nsContentUtils::HaveEqualPrincipals(previousFullscreenDoc, this)) {
>     nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
>-      new AsyncEventDispatcher(this,
>-                               NS_LITERAL_STRING("MozEnteredDomFullscreen"),
>-                               true,
>-                               true);
>+      new AsyncEventDispatcher(
>+        this, NS_LITERAL_STRING("MozFullscreenOriginChanged"),
>+        /* Bubbles */ true, /* ChromeOnly */ true);
>     asyncDispatcher->PostDOMEvent();
So do we ever dispatch both MozEnteredDomFullscreen and MozFullscreenOriginChanged?
If yes, then the comment before if (!previousFullscreenDoc) { can't be quite right (since how could there be
origin change if we just enter fullscreen?), and if no, then 
'if (aNotifyOnOriginChange &&' should be 'else if (aNotifyOnOriginChange &&'
Attachment #8606062 - Flags: review?(bugs) → review-
Comment on attachment 8604922 [details] [diff] [review]
patch 3 - remove fullscreen-origin-change notification

update IID of nsIDocument and uuid of nsIDOMWindowUtils.

I assume some other patch deals with ask-parent-to-exit-fullscreen
Attachment #8604922 - Flags: review?(bugs) → review+
Comment on attachment 8604926 [details] [diff] [review]
patch 5 - remove usage of fullscreen notification in browser elem

>+    this._window.document.addEventListener("mozfullscreenchange", evt => {
>+      if (this._isAlive() && evt.target == this._window.document) {
>+        if (!this._window.document.mozFullScreen) {
>+          this._sendAsyncMsg("exit-fullscreen");
>+        }
>+      }
>+    }, /* useCapture = */ true, /* wantsUntrusted = */ false);
>   },
The web page can have a capturing listener in window object and call stopPropagation() there and
we won't get the event here.
I think safest option would be to use system group listener here.
http://hg.mozilla.org/mozilla-central/annotate/c0e709a5baca/browser/base/content/browser.js#l977
Attachment #8604926 - Flags: review?(bugs) → review+
Attachment #8604929 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8606062 [details] [diff] [review]
> patch 2 - separate origin change into an independent event, r=dao
> 
> >+  // If it is the first entry of the fullscreen, trigger an event so
> >+  // that the UI can response to this change, e.g. hide chrome, or
> >+  // notifying parent process to enter fullscreen. Note that chrome
> >+  // code may also want to listen to MozFullscreenOriginChanged event
> >+  // to pop up warning/approval UI.
> >+  if (!previousFullscreenDoc) {
> >+    nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
> >+      new AsyncEventDispatcher(
> >+        this, NS_LITERAL_STRING("MozEnteredDomFullscreen"),
> >+        /* Bubbles */ true, /* ChromeOnly */ true);
> >+    asyncDispatcher->PostDOMEvent();
> >+  }
> >+
> >+  // The origin which is fullscreen gets changed. Trigger an event so
> >+  // that the chrome knows to pop up a warning/approval UI. Note that
> >+  // previousFullscreenDoc == nullptr upon first entry, so we always
> >+  // take this path on the first entry. Also note that, in a multi-
> >+  // process browser, the code in content process is responsible for
> >+  // sending message with the origin to its parent, and the parent
> >+  // shouldn't rely on this event itself.
> >+  if (aNotifyOnOriginChange &&
> >       !nsContentUtils::HaveEqualPrincipals(previousFullscreenDoc, this)) {
> >     nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
> >-      new AsyncEventDispatcher(this,
> >-                               NS_LITERAL_STRING("MozEnteredDomFullscreen"),
> >-                               true,
> >-                               true);
> >+      new AsyncEventDispatcher(
> >+        this, NS_LITERAL_STRING("MozFullscreenOriginChanged"),
> >+        /* Bubbles */ true, /* ChromeOnly */ true);
> >     asyncDispatcher->PostDOMEvent();
> So do we ever dispatch both MozEnteredDomFullscreen and
> MozFullscreenOriginChanged?

Yes, it intends to dispatch both events when we first enter fullscreen.

> If yes, then the comment before if (!previousFullscreenDoc) { can't be quite
> right (since how could there be origin change if we just enter fullscreen?)

When we enter fullscreen, the fullscreen origin changes from nothing to something. It's a change, isn't it?

I have documented it in the comment before MozFullscreenOriginChanged, but if you prefer, I can add more details to the comment before MozEnteredDomFullscreen.
I wouldn't consider not-in-fullscreen to enter-fullscreen to be a change to
fullscreen-origin, because we just weren't in fullscreen before.
Why do we need MozFullscreenOriginChanged when we're about to enter fullscreen?
(In reply to Olli Pettay [:smaug] from comment #26)
> I wouldn't consider not-in-fullscreen to enter-fullscreen to be a change to
> fullscreen-origin, because we just weren't in fullscreen before.
> Why do we need MozFullscreenOriginChanged when we're about to enter
> fullscreen?

Because I think it is clearer to separate the event of different purpose. EnteredFullscreen is used to notify hiding the chrome controls, and OriginChanged is to request approval or show warning. Merging the first OriginChanged to EnteredFullscreen event is probably fine, but would require a little code duplication in js side.
Could we perhaps call the OriginChanged something else?
Like, DOMFullscreenEnteredNewOrigin or DOMFullscreenNewOrigin or some such.
That way keeping the current setup would be fine.
"current" == what the patch has
That sounds reasonable. I guess the new name would probably be MozDomFullscreenNewOrigin.

Actually, since I'm considering adding more events related to fullscreen (for bug 1161802, I need something for request and revert in addition), I'm going to give all of them a new prefixed name for classification. What I'm thinking is something like:

MozEnteredDomFullscreen => MozDOMFullscreen:Entered
MozExitedDomFullscreen => MozDOMFullscreen:Exited

So with this model, the NewOrigin one would be MozDOMFullscreen:NewOrigin.

Does that make sense to you? If so, I'll change them in this patchset.
Sounds ok to me.
Attachment #8606809 - Flags: review?(bugs) → review+
Attachment #8606812 - Flags: review?(bugs) → review+
Comment on attachment 8606814 [details] [diff] [review]
patch 5 - remove usage of fullscreen notification in browser elem

So without the patch child sends rollback-fullscreen, but does it happen
ever with the patch?
And if not, the parent still has listener for that, and it wouldn't actually do anything.
Attachment #8606814 - Flags: review?(bugs) → review-
I didn't really get what you wanted to say.

I think I've removed rollback-fullscreen msg from both sides. Actually, it is replaced by exited-dom-fullscreen msg, which is sent by BrowserElementChild._mozExitedDomFullscreen and handled in BrowserElementParent._exitedDomFullscreen.
Flags: needinfo?(bugs)
Comment on attachment 8606814 [details] [diff] [review]
patch 5 - remove usage of fullscreen notification in browser elem

Indeed, there it is, removed also from parent side.
How did I miss it. Did I get only partial patch or why 
'find' didn't find 'rollback-fullscreen' there.

Sorry about that.
Flags: needinfo?(bugs)
Attachment #8606814 - Flags: review- → review+
Comment on attachment 8606809 [details] [diff] [review]
patch 1.1 - rename Moz*DomFullscreen events to MozDOMFullscreen:*

>     addMessageListener("DOMFullscreen:Approved", this);
>     addMessageListener("DOMFullscreen:CleanUp", this);
>-    addEventListener("MozEnteredDomFullscreen", this);
>-    addEventListener("MozExitedDomFullscreen", this);
>+    addEventListener("MozDOMFullscreen:Entered", this);
>+    addEventListener("MozDOMFullscreen:Exited", this);

The colon form is fine for message names but seems quite unusual for DOM events. I don't think I've ever come across such a name for a DOM event. But if smaug thinks this is ok...
Attachment #8606809 - Flags: review?(dao)
(In reply to Dão Gottwald [:dao] from comment #38)
> The colon form is fine for message names but seems quite unusual for DOM
> events. I don't think I've ever come across such a name for a DOM event. But
> if smaug thinks this is ok...

There are more events coming, so...

By canceling the review with the comment, did you mean that I can land this patch with smaug's review+, although you are not fully happy with it?
You can land that with smaug's r+.
OK, thanks.
Using colon is unusual, historically probably mostly because of onfoo event handlers, but since
event.type can be any string, colon is just fine, and especially for this kind of internal
usage a kind of namespace prefix makes it rather easy to read IMO.
Depends on: 1167890
Depends on: 1167432
Depends on: 1482304
You need to log in before you can comment on or make changes to this bug.