Closed Bug 1161802 Opened 5 years ago Closed 4 years ago

Put document in fullscreen state and trigger the event after sizemodechange

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(8 files, 15 obsolete files)

5.26 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.73 KB, patch
smaug
: review+
Details | Diff | Splinter Review
8.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.89 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
19.18 KB, patch
smaug
: review+
dao
: review+
Margaret
: review+
Details | Diff | Splinter Review
29.31 KB, patch
dao
: review+
Details | Diff | Splinter Review
1.84 KB, patch
cbook
: checkin+
Details | Diff | Splinter Review
Before we can have the fade-through-black transition, we need to delay putting document in fullscreen state and triggering the mozfullscreenchange event, otherwise, the document would change at the start of the transition, which would look weird.
Depends on: 1053413
Blocks: 1157685
No longer blocks: 1157685
Depends on: 1157685
Depends on: 1167607
If we also want to make the fullscreen events for exiting fullscreen always be triggered after the window gets changed, there would be some code dependency to code in bug 1167890.
Depends on: 1167890
Assignee: nobody → quanxunzhen
Status: NEW → ASSIGNED
Attachment #8609963 - Flags: review?(dao)
Attachment #8609963 - Flags: review?(bugs)
Attachment #8609967 - Attachment description: patch 3 - move bool params into FullScreenOptions → patch 4 - move bool params into FullScreenOptions
Attachment #8609969 - Flags: review?(bugs)
Attachment #8609970 - Flags: review?(dao)
Attachment #8609970 - Flags: review?(bugs)
Blocks: 1168028
Moved the second half to bug 1168028. Remove bug 1167890 from blockers.
No longer depends on: 1167890
Comment on attachment 8609964 [details] [diff] [review]
patch 3 - make fullscreen element ready check independent


>+bool
>+nsDocument::IsFullscreenElementReady(Element* aElement, bool aWasCallerChrome)
"IsFullscreenElementReady" sounds a bit odd.
Perhaps IsValidElementForFullscreen(...) ? 


>+  // Do the "fullscreen element ready check" from the fullscreen spec.
oh, it is coming from the spec. Odd terminology.
But ok, fine. Then call the method FullscreenElementReadyCheck(...)
Attachment #8609964 - Flags: review?(bugs) → review+
Comment on attachment 8609967 [details] [diff] [review]
patch 4 - move bool params into FullScreenOptions

># HG changeset patch
># User Xidorn Quan <quanxunzhen@gmail.com>
># Date 1432098448 -43200
>#      Wed May 20 17:07:28 2015 +1200
># Node ID c939b0cb8dfddd14e66f1698cb6243f3468016d9
># Parent  d135802ef71c02bb25deeada39012269bbf749ff
>Bug 1161802 part 4 - Move bool parameters info FullScreenOptions for clearer call sites.
>
>diff --git a/dom/base/Element.cpp b/dom/base/Element.cpp
>--- a/dom/base/Element.cpp
>+++ b/dom/base/Element.cpp
>@@ -3146,18 +3146,19 @@ Element::MozRequestFullScreen(JSContext*
>                                NS_LITERAL_STRING("mozfullscreenerror"),
>                                true,
>                                false);
>     asyncDispatcher->PostDOMEvent();
>     return;
>   }
> 
>   FullScreenOptions opts;
>+  opts.mIsCallerChrome = nsContentUtils::IsCallerChrome();
>+
>   RequestFullscreenOptions fsOptions;
>-
>   // We need to check if options is convertible to a dict first before
>   // trying to init fsOptions; otherwise Init() would throw, and we want to
>   // silently ignore non-dictionary values
>   if (aCx && IsConvertibleToDictionary(aCx, aOptions)) {
>     if (!fsOptions.Init(aCx, aOptions)) {
>       aError.Throw(NS_ERROR_FAILURE);
>       return;
>     }
>diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp
>--- a/dom/base/nsDocument.cpp
>+++ b/dom/base/nsDocument.cpp
>@@ -11339,45 +11339,41 @@ nsDocument::IsFullScreenDoc()
> 
> FullScreenOptions::FullScreenOptions()
> {
> }
> 
> class nsCallRequestFullScreen : public nsRunnable
> {
> public:
>-  explicit nsCallRequestFullScreen(Element* aElement, FullScreenOptions& aOptions)
>+  explicit nsCallRequestFullScreen(Element* aElement,
>+                                   const FullScreenOptions& aOptions)
>     : mElement(aElement),
>       mDoc(aElement->OwnerDoc()),
>-      mWasCallerChrome(nsContentUtils::IsCallerChrome()),
>+      mOptions(aOptions),
>       mHadRequestPending(static_cast<nsDocument*>(mDoc.get())->
>-                         mAsyncFullscreenPending),
>-      mOptions(aOptions)
>+                         mAsyncFullscreenPending)
>   {
>     static_cast<nsDocument*>(mDoc.get())->
>       mAsyncFullscreenPending = true;
>   }
> 
>   NS_IMETHOD Run()
>   {
>     static_cast<nsDocument*>(mDoc.get())->
>       mAsyncFullscreenPending = mHadRequestPending;
>     nsDocument* doc = static_cast<nsDocument*>(mDoc.get());
>-    doc->RequestFullScreen(mElement,
>-                           mOptions,
>-                           mWasCallerChrome,
>-                           /* aNotifyOnOriginChange */ true);
>+    doc->RequestFullScreen(mElement, mOptions);
>     return NS_OK;
>   }
> 
>   nsRefPtr<Element> mElement;
>   nsCOMPtr<nsIDocument> mDoc;
>-  bool mWasCallerChrome;
>+  FullScreenOptions mOptions;
>   bool mHadRequestPending;
>-  FullScreenOptions mOptions;
> };
> 
> void
> nsDocument::AsyncRequestFullScreen(Element* aElement,
>                                    FullScreenOptions& aOptions)
> {
>   NS_ASSERTION(aElement,
>     "Must pass non-null element to nsDocument::AsyncRequestFullScreen");
>@@ -11579,19 +11575,19 @@ IsInActiveTab(nsIDocument* aDoc)
> 
> nsresult nsDocument::RemoteFrameFullscreenChanged(nsIDOMElement* aFrameElement)
> {
>   // Ensure the frame element is the fullscreen element in this document.
>   // If the frame element is already the fullscreen element in this document,
>   // this has no effect.
>   nsCOMPtr<nsIContent> content(do_QueryInterface(aFrameElement));
>   FullScreenOptions opts;
>-  RequestFullScreen(content->AsElement(), opts,
>-                    /* aWasCallerChrome */ false,
>-                    /* aNotifyOnOriginChange */ false);
>+  opts.mIsCallerChrome = false;
>+  opts.mShouldNotifyNewOrigin = false;
>+  RequestFullScreen(content->AsElement(), opts);
> 
>   return NS_OK;
> }
> 
> nsresult nsDocument::RemoteFrameFullscreenReverted()
> {
>   RestorePreviousFullScreenState();
>   return NS_OK;
>@@ -11655,21 +11651,19 @@ nsDocument::IsFullscreenElementReady(Ele
>       return false;
>     }
>   }
>   return true;
> }
> 
> void
> nsDocument::RequestFullScreen(Element* aElement,
>-                              FullScreenOptions& aOptions,
>-                              bool aWasCallerChrome,
>-                              bool aNotifyOnOriginChange)
>-{
>-  if (!IsFullscreenElementReady(aElement, aWasCallerChrome)) {
>+                              const FullScreenOptions& aOptions)
>+{
>+  if (!IsFullscreenElementReady(aElement, aOptions.mIsCallerChrome)) {
>     return;
>   }
> 
>   // Stash a reference to any existing fullscreen doc, we'll use this later
>   // to detect if the origin which is fullscreen has changed.
>   nsCOMPtr<nsIDocument> previousFullscreenDoc = GetFullscreenLeaf(this);
> 
>   AddFullscreenApprovedObserver();
>@@ -11774,17 +11768,17 @@ nsDocument::RequestFullScreen(Element* a
> 
>   // 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 &&
>+  if (aOptions.mShouldNotifyNewOrigin &&
>       !nsContentUtils::HaveEqualPrincipals(previousFullscreenDoc, this)) {
>     nsRefPtr<AsyncEventDispatcher> asyncDispatcher =
>       new AsyncEventDispatcher(
>         this, NS_LITERAL_STRING("MozDOMFullscreen:NewOrigin"),
>         /* Bubbles */ true, /* ChromeOnly */ true);
>     asyncDispatcher->PostDOMEvent();
>   }
> 
>diff --git a/dom/base/nsDocument.h b/dom/base/nsDocument.h
>--- a/dom/base/nsDocument.h
>+++ b/dom/base/nsDocument.h
>@@ -1216,29 +1216,19 @@ public:
> 
>   static void ExitFullscreen(nsIDocument* aDoc);
> 
>   // Do the "fullscreen element ready check" from the fullscreen spec.
>   // It returns true if the given element is allowed to go into fullscreen.
>   bool IsFullscreenElementReady(Element* aElement, bool aWasCallerChrome);
> 
>   // This is called asynchronously by nsIDocument::AsyncRequestFullScreen()
>-  // to move this document into full-screen mode if allowed. aWasCallerChrome
>-  // should be true when nsIDocument::AsyncRequestFullScreen() was called
>-  // by chrome code. aNotifyOnOriginChange denotes whether we should trigger
>-  // a MozFullscreenOriginChanged event if requesting fullscreen in this
>-  // document causes the origin which is fullscreen to change. We may want to
>-  // *not* send this notification if we're calling RequestFullScreen() as part
>-  // of a continuation of a request in a subdocument, whereupon the caller will
>-  // need to send some notification itself with the origin of the document
>-  // which originally requested fullscreen, not *this* document's origin.
>+  // to move this document into full-screen mode if allowed.
>   void RequestFullScreen(Element* aElement,
>-                         mozilla::dom::FullScreenOptions& aOptions,
>-                         bool aWasCallerChrome,
>-                         bool aNotifyOnOriginChange);
>+                         const mozilla::dom::FullScreenOptions& aOptions);
> 
>   // Removes all elements from the full-screen stack, removing full-scren
>   // styles from the top element in the stack.
>   void CleanupFullscreenState();
> 
>   // Add/remove "fullscreen-approved" observer service notification listener.
>   // Chrome sends us a notification when fullscreen is approved for a
>   // document, with the notification subject as the document that was approved.
>diff --git a/dom/base/nsIDocument.h b/dom/base/nsIDocument.h
>--- a/dom/base/nsIDocument.h
>+++ b/dom/base/nsIDocument.h
>@@ -140,16 +140,26 @@ template<typename> class OwningNonNull;
> template<typename> class Sequence;
> 
> template<typename, typename> class CallbackObjectHolder;
> typedef CallbackObjectHolder<NodeFilter, nsIDOMNodeFilter> NodeFilterHolder;
> 
> struct FullScreenOptions {
>   FullScreenOptions();
>   nsRefPtr<gfx::VRHMDInfo> mVRHMDDevice;
>+  // This value should be true if the fullscreen request is
>+  // originated from chrome code.
>+  bool mIsCallerChrome = false;
>+  // This value denotes whether we should trigger a NewOrigin event if
>+  // requesting fullscreen in its document causes the origin which is
>+  // fullscreen to change. We may want *not* to trigger that event if
>+  // we're calling RequestFullScreen() as part of a continuation of a
>+  // request in a subdocument in different process, whereupon the caller
>+  // need to send some notification itself with the real origin.
>+  bool mShouldNotifyNewOrigin = true;
> };
> 
> } // namespace dom
> } // namespace mozilla
> 
> #define NS_IDOCUMENT_IID \
> { 0xdcfa30f2, 0x2197, 0x421f, \
>   { 0xa7, 0x5a, 0x3e, 0x70, 0x18, 0x08, 0xde, 0x81 } }
Attachment #8609967 - Flags: review?(bugs) → review+
(oops sorry about that.)
Attachment #8609968 - Flags: review?(bugs) → review+
Comment on attachment 8609969 [details] [diff] [review]
patch 6 - move FullScreenRoots manipulate code

>-  // Create a copy of the roots array, and iterate over the copy. This is so
>-  // that if an element is removed from mRoots we don't mess up our iteration.
>-  nsTArray<nsWeakPtr> roots(sInstance->mRoots);
>-  // Call aFunction on all entries.
>-  for (uint32_t i = 0; i < roots.Length(); i++) {
>-    nsCOMPtr<nsIDocument> root = do_QueryReferent(roots[i]);
>-    // Check that the root isn't in the manager. This is so that new additions
>-    // while we were running don't get traversed.
>-    if (root && FullscreenRoots::Contains(root)) {
>+  for (nsWeakPtr& item : sInstance->mRoots) {
C++ range for loop is rather security hazard, so I'd prefer if you used 
for (uint32_t i = 0; i < sInstance->mRoots.Length(); ++i) {
which doesn't end up causing out-of-bounds or uaf errors if mRoots is modified during iteration.



It is not yet clear to me why we actually want to do this, but I assume the next patch in queue explains that.
Attachment #8609969 - Flags: review?(bugs) → review+
Comment on attachment 8609963 [details] [diff] [review]
patch 2 - move fullscreen to be dispatched afterward

>+nsGlobalWindow::FinishFullscreenChange()
>+{
please add MOZ_ASSERT here that we're dealing with outer window.

>+++ b/dom/base/nsPIDOMWindow.h
update IID of nsPIDOMWindow


>+  virtual void FullscreenChanged(bool aInFullscreen) override;
Ahaa, this is overriding the method defined in the patch roc reviewed.
Attachment #8609963 - Flags: review?(bugs) → review+
Comment on attachment 8609970 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

>+      case "MozDOMFullscreen:Entered":
>+        if (this._isRemoteBrowser(browser)) {
>+          browser.messageManager.sendAsyncMessage("DOMFullscreen:Entered");
>+        }
Could we for consistency send the message always, also in-process browsers. That way frame scripts would always get the message.

 
> let DOMFullscreenHandler = {
>   _fullscreenDoc: null,
> 
>   init: function() {
>+    addMessageListener("DOMFullscreen:Entered", this);
>     addMessageListener("DOMFullscreen:Approved", this);
>     addMessageListener("DOMFullscreen:CleanUp", this);
>-    addEventListener("MozDOMFullscreen:Entered", this);
>+    addEventListener("MozDOMFullscreen:InvokedRequest", this);
Why the event name is MozDOMFullscreen:InvokedRequest, why not just
MozDOMFullscreen:Request?
>+  sPendingFullscreenRequests.AppendElement(
>+    FullscreenRequest{aElement, aOptions});
>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
>+    // If we are not the top level process, dispatch an event to make
>+    // our parent process go fullscreen first.
>+    (new AsyncEventDispatcher(
>+       this, NS_LITERAL_STRING("MozDOMFullscreen:InvokedRequest"),
>+       /* Bubbles */ true, /* ChromeOnly */ true))->PostDOMEvent();
>+  } else {
>+    // Make the window fullscreen.
>+    SetWindowFullScreen(this, true, aOptions.mVRHMDDevice);
>+  }
I don't understand this. Why do we need to have different setup for e10s and non-e10s? Couldn't both use the e10s setup?
Would make code easier to understand if there weren't different modes.



>+nsIDocument::HandlePendingFullscreenRequests(nsIDocument* aDoc)
>+{
>+  nsCOMPtr<nsIDocShellTreeItem> rootDocShell;
>+  aDoc->GetDocShell()->GetRootTreeItem(getter_AddRefs(rootDocShell));
>+  auto begin = sPendingFullscreenRequests.begin();
>+  auto end = sPendingFullscreenRequests.end();
>+  auto newEnd = std::remove_if(begin, end, [&](FullscreenRequest& request) {
yikes. modern C++ can really make code hard to read.

Don't use auto, please. I want to explicitly see if a variable holds a strong or raw (or something else) reference to an object.



>+    auto elemDoc = static_cast<nsDocument*>(request.mElement->OwnerDoc());
>+    if (!elemDoc) {
>+      // Drop dead fullscreen requests
>+      return true;
>+    }
>+    nsCOMPtr<nsIDocShellTreeItem> elemRootDocShell;
>+    elemDoc->GetDocShell()->GetRootTreeItem(getter_AddRefs(elemRootDocShell));
>+    if (elemRootDocShell == rootDocShell) {
>+      elemDoc->ApplyFullscreen(request.mElement, request.mOptions);
>+      // Drop handled fullscreen requests
>+      return true;
>+    }
>+    return false;
>+  });
All this would be easier to read if there was just a plain old for loop.
Attachment #8609970 - Flags: review?(bugs) → review-
Does this way look safer?

In patch 7, the AddRoot call is moving to some place other than where we call SetWindowFullScreen.
Attachment #8609969 - Attachment is obsolete: true
Attachment #8610267 - Flags: review?(bugs)
Comment on attachment 8610267 [details] [diff] [review]
patch 6 - move FullScreenRoots manipulate code

>+FullscreenRoots::Clear(void(*aFunction)(nsIDocument* aDoc))
> {
>   if (!sInstance) {
>     return;
>   }
>-  // Create a copy of the roots array, and iterate over the copy. This is so
>-  // that if an element is removed from mRoots we don't mess up our iteration.
>-  nsTArray<nsWeakPtr> roots(sInstance->mRoots);
>-  // Call aFunction on all entries.
>-  for (uint32_t i = 0; i < roots.Length(); i++) {
>-    nsCOMPtr<nsIDocument> root = do_QueryReferent(roots[i]);
>-    // Check that the root isn't in the manager. This is so that new additions
>-    // while we were running don't get traversed.
>-    if (root && FullscreenRoots::Contains(root)) {
>+  RootArray roots(Move(sInstance->mRoots));
>+  delete sInstance;
>+  sInstance = nullptr;
>+
>+  for (nsWeakPtr& item : roots) {
>+    if (nsCOMPtr<nsIDocument> root = do_QueryReferent(item)) {
>       aFunction(root);
>     }
>   }
ok fine. (though, IMO Move is nowhere near as self-documenting than an explicit 
SwapElements call. But I can live with Move() I guess.)
Attachment #8610267 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8609970 [details] [diff] [review]
> patch 7 - apply fullscreen state after window changes
> 
> >+      case "MozDOMFullscreen:Entered":
> >+        if (this._isRemoteBrowser(browser)) {
> >+          browser.messageManager.sendAsyncMessage("DOMFullscreen:Entered");
> >+        }
> Could we for consistency send the message always, also in-process browsers.
> That way frame scripts would always get the message.
> 
>  
> >+  sPendingFullscreenRequests.AppendElement(
> >+    FullscreenRequest{aElement, aOptions});
> >+  if (XRE_GetProcessType() == GeckoProcessType_Content) {
> >+    // If we are not the top level process, dispatch an event to make
> >+    // our parent process go fullscreen first.
> >+    (new AsyncEventDispatcher(
> >+       this, NS_LITERAL_STRING("MozDOMFullscreen:InvokedRequest"),
> >+       /* Bubbles */ true, /* ChromeOnly */ true))->PostDOMEvent();
> >+  } else {
> >+    // Make the window fullscreen.
> >+    SetWindowFullScreen(this, true, aOptions.mVRHMDDevice);
> >+  }
> I don't understand this. Why do we need to have different setup for e10s and
> non-e10s? Couldn't both use the e10s setup?
> Would make code easier to understand if there weren't different modes.

It is nothing about whether it is e10s, it is about whether we are on the top level process. The point here is, if we are not at the top level process, we don't have window to go fullscreen. That case we need to ask the parent process to go fullscreen.

We can move the "SetWindowFullScreen" call out from the else block, since that call will be finally rejected by nsGlobalWindow::SetFullScreenInternal when it finds it is not a chrome window. But since the call will eventually be rejected, and there are cost before it gets rejected, I see no reason why we want to call it unconditionally.

It is more costly to make triggering the event unconditional in non-e10s environment: the tab-content script will receive that and redirect to the browser-fullScreen, and the latter will request fullscreen again, which will finally fail in FullscreenElementReadyCheck because we have made its ascendent fullscreen before.
(In reply to Olli Pettay [:smaug] from comment #15)
> Comment on attachment 8609970 [details] [diff] [review]
> patch 7 - apply fullscreen state after window changes
> 
> >+      case "MozDOMFullscreen:Entered":
> >+        if (this._isRemoteBrowser(browser)) {
> >+          browser.messageManager.sendAsyncMessage("DOMFullscreen:Entered");
> >+        }
> Could we for consistency send the message always, also in-process browsers.
> That way frame scripts would always get the message.

I think it's probably ok as it is not that costly for handling a redundant message here, but I don't see why we want the frame script to always get this message. The frame scripts can always listen to MozDOMFullscreen:Entered if they want to be notified after the content being fullscreen.
Patch 2, which moves the fullscreen event to be called after window gets fullscreen, makes that event non-cancelable, which breaks test_fullscreen_preventdefault. But as far as I can see, there is only one place which is written based on this assumption, which is a workaround from bug 1079222. I also see another code for Android is related, though it is easy to solve.

But it makes me wonder whether we really need to delay fullscreen event. I guess not.
Blocks: 1168274
Attachment #8609963 - Attachment is obsolete: true
Attachment #8609963 - Flags: review?(dao)
Attachment #8609970 - Attachment is obsolete: true
Attachment #8609970 - Flags: review?(dao)
It seems to me that with patch 2, the workaround from bug 1079222 is no longer needed, which means we probably can remove that workaround and the failed test with patch 2.
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #19)
> I think it's probably ok as it is not that costly for handling a redundant
> message here, but I don't see why we want the frame script to always get
> this message. The frame scripts can always listen to
> MozDOMFullscreen:Entered if they want to be notified after the content being
> fullscreen.
Just for the sake of consistency if we could always handle fullscreen the same way - 
messaging would be via message manager always. webcontent->events->mm->whatever_chrome_does.
:cbadau, :Gijs, could you confirm that, this build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/quanxunzhen@gmail.com-ac15b2280c9f/try-macosx64/firefox-41.0a1.en-US.mac.dmg fixes bug 1079222? I intend to remove the workaround there.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(camelia.badau)
(In reply to Olli Pettay [:smaug] from comment #22)
> 
> Just for the sake of consistency if we could always handle fullscreen the
> same way - 
> messaging would be via message manager always.
> webcontent->events->mm->whatever_chrome_does.

My concern is that, even if we let it send the message, this path is still not the effective one, which could potentially confuse later reader. I think explicitly making those path conditional indicates how the paths are supposed to work.

If we want e10s and non-e10s to use the completely identical path, we would need more hack so that we can stop the document traverse on the browser border even for in-process case, which I don't think it worths.
Blocks: 1167576
Attachment #8610971 - Flags: review?(margaret.leibovic)
Attachment #8610971 - Flags: review?(dao)
Attachment #8610971 - Flags: review?(bugs)
I gave up rewriting FullScreenRoots::ForEach into FullScreenRoots::Clear. It breaks tests in a way I have no idea how to fix.
Attachment #8610267 - Attachment is obsolete: true
Attachment #8610972 - Flags: review?(bugs)
Attachment #8610974 - Flags: review?(dao)
Attachment #8610974 - Flags: review?(bugs)
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #23)
> :cbadau, :Gijs, could you confirm that, this build:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/quanxunzhen@gmail.
> com-ac15b2280c9f/try-macosx64/firefox-41.0a1.en-US.mac.dmg fixes bug
> 1079222? I intend to remove the workaround there.

The trypushed build ends up having the fullscreen button and "titlebar" borders in the wrong place, and being confused about whether it's in fullscreen. (I get kicked out of fullscreen, and then a horizontal slide to go back to the new window, which isn't in fullscreen but somehow did have a fullscreen-like animation?)

So it doesn't seem fixed with that build. AFAICT your new trypush doesn't have the removal patch so I'm assuming it doesn't help to test with that build.
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(camelia.badau)
(In reply to :Gijs Kruitbosch from comment #29)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #23)
> > :cbadau, :Gijs, could you confirm that, this build:
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/quanxunzhen@gmail.
> > com-ac15b2280c9f/try-macosx64/firefox-41.0a1.en-US.mac.dmg fixes bug
> > 1079222? I intend to remove the workaround there.
> 
> The trypushed build ends up having the fullscreen button and "titlebar"
> borders in the wrong place, and being confused about whether it's in
> fullscreen. (I get kicked out of fullscreen, and then a horizontal slide to
> go back to the new window, which isn't in fullscreen but somehow did have a
> fullscreen-like animation?)

It seems you're right. I was testing a debug build, where I cannot reproduce this problem, however the opt build seems to still have that problem. It was probably because the debug build is too slow that the new window is created after the original fullscreen state exited completely.

> So it doesn't seem fixed with that build. AFAICT your new trypush doesn't
> have the removal patch so I'm assuming it doesn't help to test with that
> build.

In my new trypush, the backout patch has been merged to the patch 2, so this problem is also reproducible there. Actually since the "fullscreen" event is no longer preventable, even if I do not remove that workaround code, it is expected to lose its effectiveness automatically.
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #30)
> In my new trypush, the backout patch has been merged to the patch 2, so this
> problem is also reproducible there. Actually since the "fullscreen" event is
> no longer preventable, even if I do not remove that workaround code, it is
> expected to lose its effectiveness automatically.

I see. Can you address the root of the problem if you're modifying the way the core code deals with fullscreen? There are more details in the bug's earlier comments, but if I recall correctly, the issue was that OSX takes the window out of fullscreen and content never realizes.
(In reply to :Gijs Kruitbosch from comment #31)
> I see. Can you address the root of the problem if you're modifying the way
> the core code deals with fullscreen? There are more details in the bug's
> earlier comments, but if I recall correctly, the issue was that OSX takes
> the window out of fullscreen and content never realizes.

Let's see... I probably succeed "again", but who knows...

I add some code to stop the fullscreen change if the widget reports a different fullscreen state than the global window. It seems to fix that issue in my local opt build.

I've pushed the build here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7bf1eebac73

Could you please test that when it finishes?
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8610972 [details] [diff] [review]
patch 6 - move FullScreenRoots manipulate code

(I wonder if there should still be some helper method which does the Add/Remove, but I don't know remember what part 7 does.... looking that next.)
Attachment #8610972 - Flags: review?(bugs) → review+
Attachment #8610971 - Flags: review?(bugs) → review+
Comment on attachment 8610974 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

>+nsIDocument::HandlePendingFullscreenRequests(nsIDocument* aDoc)
>+{
>+  if (sPendingFullscreenRequests.isEmpty()) {
>+    return false;
>+  }
>+
>+  nsCOMPtr<nsIDocShellTreeItem> rootShell;
>+  aDoc->GetDocShell()->GetRootTreeItem(getter_AddRefs(rootShell));
>+  bool handled = false;
>+  FullscreenRequest* request = sPendingFullscreenRequests.getFirst();
>+  while (request) {
>+    bool drop = true;
>+    auto elemDoc = static_cast<nsDocument*>(request->mElement->OwnerDoc());
No good reason to use auto here.

>+    if (elemDoc) {
>+      nsCOMPtr<nsIDocShellTreeItem> elemRootShell;
>+      elemDoc->GetDocShell()->GetRootTreeItem(getter_AddRefs(elemRootShell));
GetDocShell() may return null. So, you need a null check

>+      if (elemRootShell != rootShell) {
What if elemRootShell is null?  I think we want to remove the request from the list in that case.

>+      } else {
>+        elemDoc->ApplyFullscreen(request->mElement, request->mOptions);
Hmm, ApplyFullscreen doesn't seem to execute any scripts synchronously (like dispatch dom events),
but I'd prefer if elemDoc was actually a strong reference (nsRefPtr<nsDocument>)
Attachment #8610974 - Flags: review?(bugs) → review-
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #32)
> (In reply to :Gijs Kruitbosch from comment #31)
> > I see. Can you address the root of the problem if you're modifying the way
> > the core code deals with fullscreen? There are more details in the bug's
> > earlier comments, but if I recall correctly, the issue was that OSX takes
> > the window out of fullscreen and content never realizes.
> 
> Let's see... I probably succeed "again", but who knows...
> 
> I add some code to stop the fullscreen change if the widget reports a
> different fullscreen state than the global window. It seems to fix that
> issue in my local opt build.
> 
> I've pushed the build here:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7bf1eebac73
> 
> Could you please test that when it finishes?

The animation out of fullscreen is still weird because you see the new window opening before the animation is finished, on the "fullscreen" side of things, and then it snaps into place being non-fullscreen, but it seems at least the window is no longer confused about whether it's in fullscreen or not! \o/
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs Kruitbosch from comment #35)
> The animation out of fullscreen is still weird because you see the new
> window opening before the animation is finished, on the "fullscreen" side of
> things, and then it snaps into place being non-fullscreen, 

Yes, but it doesn't seem to be a regression of the patchset here anyway. I can reproduce this issue in the release version.

> but it seems at
> least the window is no longer confused about whether it's in fullscreen or
> not! \o/

Yeah, great. That's enough at present, I think.
(In reply to Olli Pettay [:smaug] from comment #34)
> Comment on attachment 8610974 [details] [diff] [review]
> patch 7 - apply fullscreen state after window changes
> 
> >+nsIDocument::HandlePendingFullscreenRequests(nsIDocument* aDoc)
> >+{
> >+  if (sPendingFullscreenRequests.isEmpty()) {
> >+    return false;
> >+  }
> >+
> >+  nsCOMPtr<nsIDocShellTreeItem> rootShell;
> >+  aDoc->GetDocShell()->GetRootTreeItem(getter_AddRefs(rootShell));
> >+  bool handled = false;
> >+  FullscreenRequest* request = sPendingFullscreenRequests.getFirst();
> >+  while (request) {
> >+    bool drop = true;
> >+    auto elemDoc = static_cast<nsDocument*>(request->mElement->OwnerDoc());
> No good reason to use auto here.

The good reason is that it makes the code shorter without hurting the readability. The type of the variable here is obvious from the following static_cast. And if we explicitly write the nsDocument* again instead of just using auto, we will have to split the line into two line, which probably even lowers the readability.

> >+    if (elemDoc) {
> >+      nsCOMPtr<nsIDocShellTreeItem> elemRootShell;
> >+      elemDoc->GetDocShell()->GetRootTreeItem(getter_AddRefs(elemRootShell));
> GetDocShell() may return null. So, you need a null check
> 
> >+      if (elemRootShell != rootShell) {
> What if elemRootShell is null?  I think we want to remove the request from
> the list in that case.
> 
> >+      } else {
> >+        elemDoc->ApplyFullscreen(request->mElement, request->mOptions);
> Hmm, ApplyFullscreen doesn't seem to execute any scripts synchronously (like
> dispatch dom events),
> but I'd prefer if elemDoc was actually a strong reference
> (nsRefPtr<nsDocument>)

OK.
Sorry for updating the patch again. Add some code in nsGlobalWindow to address the regression of bug 1079222.
Attachment #8610971 - Attachment is obsolete: true
Attachment #8610971 - Flags: review?(margaret.leibovic)
Attachment #8610971 - Flags: review?(dao)
Attachment #8611500 - Flags: review?(margaret.leibovic)
Attachment #8611500 - Flags: review?(dao)
Attachment #8611500 - Flags: review?(bugs)
Blocks: 1091927
Attachment #8611500 - Attachment is obsolete: true
Attachment #8611500 - Flags: review?(margaret.leibovic)
Attachment #8611500 - Flags: review?(dao)
Attachment #8611500 - Flags: review?(bugs)
Comment on attachment 8611522 [details] [diff] [review]
patch 2 - move fullscreen to be dispatched afterward

This patch can be interdiffed with the previously reviewed patch: https://bugzilla.mozilla.org/attachment.cgi?oldid=8610971&action=interdiff&newid=8611522&headers=1
Attachment #8611522 - Flags: review?(margaret.leibovic)
Attachment #8611522 - Flags: review?(dao)
Attachment #8611522 - Flags: review?(bugs)
I want to explain for the lambda usage in this patch a bit.

I'm not very willing to use lambda here actually. But so many checks make it awkward if I nested them inline directly.

I also tried to separate the lambda here to an independent static function, but ApplyFullscreen is not a public method of nsDocument (and I'm not willing to make it public either) which means I only have two options, then:
1. make that function a new static method of either nsDocument or nsIDocument; or
2. make that function only decide what to do, but leave the actual action to the callsite.

The first option expose the local struct FullscreenRequest to a wider place, which I also feel uncomfortable with.

For the second option, I didn't figure out a meaningful name for that function, which should accept a root doc shell and the target document, then return whether we should apply fullscreen *and* whether we should remove the request from the queue. (There are three different operations: apply, drop, no-op)

I finally feel using the lambda as a nested function seems to make the most sense. But any advice is welcome.
Attachment #8610974 - Attachment is obsolete: true
Attachment #8610974 - Flags: review?(dao)
Attachment #8611597 - Flags: review?(dao)
Attachment #8611597 - Flags: review?(bugs)
For reverting an accidental change to browser-fullScreen.js...
Attachment #8611597 - Attachment is obsolete: true
Attachment #8611597 - Flags: review?(dao)
Attachment #8611597 - Flags: review?(bugs)
Attachment #8612123 - Flags: review?(dao)
Attachment #8612123 - Flags: review?(bugs)
Attachment #8611522 - Flags: review?(dao) → review+
Comment on attachment 8612123 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

>   handleEvent: function (event) {
>+    let browser = gBrowser.selectedBrowser;
>     switch (event.type) {
>       case "activate":
>         if (document.mozFullScreen) {
>           this.showWarning(this.fullscreenOrigin);
>         }
>         break;
>       case "fullscreen":
>         this.toggle();
>         break;
>       case "transitionend":
>         if (event.propertyName == "opacity")
>           this.cancelWarning();
>         break;
>+      case "MozDOMFullscreen:Entered":
>+        this.enterDomFullscreen(browser);
>+        if (this._isRemoteBrowser(browser)) {
>+          browser.messageManager.sendAsyncMessage("DOMFullscreen:Entered");
>+        }
>+        break;
>       case "MozDOMFullscreen:Exited":
>         this.cleanupDomFullscreen();
>         break;
>     }

Can you get the browser off the event? This seems saner than blindly using gBrowser.selectedBrowser. Also, none of the other cases need the browser, so please declare it just where you need it.
Attachment #8611522 - Flags: review?(bugs) → review+
Comment on attachment 8612123 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

I still don't see any reason for using lambda.
Why don't you have 
bool nsIDocument::HandlePendingFullscreenRequest(const FullscreenRequest& aRequest, bool& aHandled);

and then in the loop 
if (HandlePendingFullscreenRequest(*request, handled)) {
  ...
}

Should make nsIDocument::HandlePendingFullscreenRequests(...) much more readable, since it would basically just contain one while-loop.
Even moving getting roottreeitem to the new method should slow things down in practice, since this isn't hot code or anything.


+      nsRefPtr<nsDocument> doc =
+        static_cast<nsDocument*>(aRequest.mElement->OwnerDoc());
+      if (!doc) {
+        return true;
+      }
OwnerDoc() *never* returns null. If it was, it would be called GetOwnerDoc().




+ aDoc->GetDocShell()->GetRootTreeItem(getter_AddRefs(rootShell));
this needs null checks. In DOM code, and docshell too, Get* methods may in general return null.




(feel free to ask review for updated patch even before June 1)
Attachment #8612123 - Flags: review?(bugs) → review-
Attachment #8612123 - Attachment is obsolete: true
Attachment #8612123 - Flags: review?(dao)
Attachment #8612545 - Flags: review?(dao)
Attachment #8612545 - Flags: review?(bugs)
Comment on attachment 8612545 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

>+nsIDocument::HandlePendingFullscreenRequest(const FullscreenRequest& aRequest,
>+                                            nsIDocShellTreeItem* aRootShell,
>+                                            bool* aHandled)
>+{
>+  nsRefPtr<nsDocument> doc =
>+    static_cast<nsDocument*>(aRequest.mElement->OwnerDoc());
>+  nsIDocShellTreeItem* shell = doc->GetDocShell();
>+  if (!shell) {
>+    return true;
>+  }
>+  nsCOMPtr<nsIDocShellTreeItem> rootShell;
>+  shell->GetRootTreeItem(getter_AddRefs(rootShell));
if (!rootShell) {
  return true;
}


>+/* static */ bool
>+nsIDocument::HandlePendingFullscreenRequests(nsIDocument* aDoc)
>+{
>+  if (sPendingFullscreenRequests.isEmpty()) {
>+    return false;
>+  }
>+
>+  bool handled = false;
>+  nsIDocShellTreeItem* shell = aDoc->GetDocShell();
>+  if (!shell) {
>+    return false;
>+  }
Hmm, what if we used to have docshell but don't have anymore.
We should remove the relevant requests then.
So, don't return early here, but let HandlePendingFullscreenRequest() be called
and end up returning true.


>+  nsCOMPtr<nsIDocShellTreeItem> rootShell;
>+  shell->GetRootTreeItem(getter_AddRefs(rootShell));
So this would need then a null check for shell and just leave rootShell null if there is not shell.
Attachment #8612545 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (reviewing overload, no new review requests before June 1, pretty please) from comment #46)
> Comment on attachment 8612545 [details] [diff] [review]
> patch 7 - apply fullscreen state after window changes
> 
> >+nsIDocument::HandlePendingFullscreenRequest(const FullscreenRequest& aRequest,
> >+                                            nsIDocShellTreeItem* aRootShell,
> >+                                            bool* aHandled)
> >+{
> >+  nsRefPtr<nsDocument> doc =
> >+    static_cast<nsDocument*>(aRequest.mElement->OwnerDoc());
> >+  nsIDocShellTreeItem* shell = doc->GetDocShell();
> >+  if (!shell) {
> >+    return true;
> >+  }
> >+  nsCOMPtr<nsIDocShellTreeItem> rootShell;
> >+  shell->GetRootTreeItem(getter_AddRefs(rootShell));
> if (!rootShell) {
>   return true;
> }

I don't think this check is necessary. rootShell is promised not to be null if shell is not null, because in case the shell doesn't have any parent, it just returns itself.
indeed
Comment on attachment 8612545 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

please see comment 43
Attachment #8612545 - Flags: review?(dao) → review-
(In reply to Dão Gottwald [:dao] from comment #49)
> Comment on attachment 8612545 [details] [diff] [review]
> patch 7 - apply fullscreen state after window changes
> 
> please see comment 43

Ahh... I forgot that... OK
Attachment #8612545 - Attachment is obsolete: true
Attachment #8612757 - Flags: review?(dao)
Comment on attachment 8612757 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes, r=smaug

>+      case "MozDOMFullscreen:Entered": {
>+        if (!this._pendingFullscreenBrowser) {
>+          this.mozCancelFullScreen();
>+          break;
>+        }
>+        let browser = this._pendingFullscreenBrowser;

This is better than using gBrowser.selectedBrowser, but again, could you get the browser via the event object?
(In reply to Dão Gottwald [:dao] from comment #52)
> Comment on attachment 8612757 [details] [diff] [review]
> patch 7 - apply fullscreen state after window changes, r=smaug
> 
> >+      case "MozDOMFullscreen:Entered": {
> >+        if (!this._pendingFullscreenBrowser) {
> >+          this.mozCancelFullScreen();
> >+          break;
> >+        }
> >+        let browser = this._pendingFullscreenBrowser;
> 
> This is better than using gBrowser.selectedBrowser, but again, could you get
> the browser via the event object?

I don't think so, because this event targets the document, not the element.

But since MozDOMFullscreen:Entered is an internal event, I guess we probably can make it target the element, but it potentially adds some unnecessary work on the child process and in-process browser.

I'm not sure which is better, anyway.
I'm not even sure why you're listening to that event in browser-fullScreen.js in the first place. Shouldn't this be in a content script that then sends a message?
This is the change of this bug.

TLNR: The key is, previously, the content enters fullscreen state before the window becomes fullscreen, but this bug reverses it. It makes the content enter fullscreen state after the window becomes fullscreen, which means there needs to be some mechanism for the parent to notify the content to finish the change.

Long version: so previously, what we do for DOM fullscreen request is:
1. the content calls requestFullscreen(), then
2. the content process puts the content into fullscreen state and triggers Entered, then
3. the content frame script redirects the event to the parent, then
4. the parent browser script calls something similar to requestFullscreen(), then
5. the parent process puts the browser document into fullscreen, then
6. the window becomes fullscreen.

The new process implemented in this bug is:
1. the content calls requestFullscreen(), then
2. the content process triggers Request event, then
3. the content frame script redirects the event to its parent, then
4. the parent browser script calls something similar to requestFullscreen(), then
5. the window becomes fullscreen, then
6. the parent browser process puts the browser document into fullscreen and triggers Entered, then
7. the parent browser script redirects the event to the child, then
8. the content process puts the content into fullscreen state.
Blocks: 1170369
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #53)
> (In reply to Dão Gottwald [:dao] from comment #52)
> > Comment on attachment 8612757 [details] [diff] [review]
> > patch 7 - apply fullscreen state after window changes, r=smaug
> > 
> > >+      case "MozDOMFullscreen:Entered": {
> > >+        if (!this._pendingFullscreenBrowser) {
> > >+          this.mozCancelFullScreen();
> > >+          break;
> > >+        }
> > >+        let browser = this._pendingFullscreenBrowser;
> > 
> > This is better than using gBrowser.selectedBrowser, but again, could you get
> > the browser via the event object?
> 
> I don't think so, because this event targets the document, not the element.
> 
> But since MozDOMFullscreen:Entered is an internal event, I guess we probably
> can make it target the element, but it potentially adds some unnecessary
> work on the child process and in-process browser.

What kind of work and how much of it are we talking about? I'm really not too thrilled about browser-fullscreen.js keeping track of _pendingFullscreenBrowser...
Comment on attachment 8611522 [details] [diff] [review]
patch 2 - move fullscreen to be dispatched afterward

Review of attachment 8611522 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't test this, but this sounds reasonable given your description of the change.
Attachment #8611522 - Flags: review?(margaret.leibovic) → review+
(In reply to Dão Gottwald [:dao] from comment #56)
> 
> What kind of work and how much of it are we talking about? I'm really not
> too thrilled about browser-fullscreen.js keeping track of
> _pendingFullscreenBrowser...

I tried for a bit to make MozDOMFullscreen:Entered target the <xul:browser> element, but I failed. It seems even if I dispatch the event on that element, the event will later be retargeted to the <tabbrowser>, then we lose the information about who initially requests the fullscreen.

Changing anything inside the event dispatch seems to be non-trivial. I guess the easiest way at present is to track the pending browser.
I meant, keep track of pending fullscreen browser in browser-fullScreen.js.
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #58)
> I tried for a bit to make MozDOMFullscreen:Entered target the <xul:browser>
> element, but I failed. It seems even if I dispatch the event on that
> element, the event will later be retargeted to the <tabbrowser>, then we
> lose the information about who initially requests the fullscreen.
use event.originalTarget?
(In reply to Olli Pettay [:smaug] from comment #60)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #58)
> > I tried for a bit to make MozDOMFullscreen:Entered target the <xul:browser>
> > element, but I failed. It seems even if I dispatch the event on that
> > element, the event will later be retargeted to the <tabbrowser>, then we
> > lose the information about who initially requests the fullscreen.
> use event.originalTarget?

Yeah, browsers are anonymous content, so target will get you the binding parent (tabbrowser in this case) but originalTarget should give you the browser.
The only change in C++ part is make MozDOMFullscreen:Entered event target `aElement` instead of `this` document.
Attachment #8612757 - Attachment is obsolete: true
Attachment #8612757 - Flags: review?(dao)
Attachment #8614575 - Flags: review?(dao)
Attachment #8614575 - Flags: review?(bugs)
Comment on attachment 8614575 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

Thanks!
Attachment #8614575 - Flags: review?(dao) → review+
Comment on attachment 8614575 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

I'm now lost with the target.
What is the event target in e10s and what is it in non-e10s?
Comment on attachment 8614575 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

I wish -p -U 8 was used to create the patch.

So, why does this actually work?
MozDOMFullscreen:Entered's target can be some element in content in case of
 non-e10s, right? nsDocument::RequestFullScreen calls ApplyFullscreen, and that dispatches MozDOMFullscreen:Entered.
And we listen for MozDOMFullscreen:Entered in chrome code, yet the chrome code seems to expect that target or originalTarget is xul:browser.
Attachment #8614575 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #65)
> Comment on attachment 8614575 [details] [diff] [review]
> patch 7 - apply fullscreen state after window changes
> 
> I wish -p -U 8 was used to create the patch.

Oh... probably my new extension doesn't take the default setting of hg export...

> So, why does this actually work?
> MozDOMFullscreen:Entered's target can be some element in content in case of
>  non-e10s, right? nsDocument::RequestFullScreen calls ApplyFullscreen, and
> that dispatches MozDOMFullscreen:Entered.
> And we listen for MozDOMFullscreen:Entered in chrome code, yet the chrome
> code seems to expect that target or originalTarget is xul:browser.

I totally forgot the non-e10s case...

Then I'm not sure what should I do now. I guess probably going back to the previous version is better. It is clear to me how can I always dispatch the event to the xul:browser, or how can I get the browser element quickly from the JavaScript code.

dao, any suggestion?
Flags: needinfo?(dao)
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #66)
> It is clear to me how can I always dispatch the
> event to the xul:browser, or how can I get the browser element quickly from
> the JavaScript code.

s/clear/not clear
Attachment #8612757 - Flags: review?(dao)
Attachment #8612757 - Attachment is obsolete: false
I thought about always dispatching :Request event for non-chrome element, and let the chrome code in browser-fullScreen.js always invoke remoteFrameFullscreenChanged() again.

But that won't work either, because when the window finishes change, the first request in queue is from the non-chrome element, and hence the :Entered event will still be dispatched with the original element, which is not the <xul:browser>.

I feel that tracking the pending browser in browser-fullScreen.js is probably the most reliable way.
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #66)
> It is [not] clear to me how can I always dispatch the
> event to the xul:browser,

This sounds like a question for Olli.

> or how can I get the browser element quickly from
> the JavaScript code.

gBrowser.getBrowserForContentWindow(event.target.ownerDocument.defaultView.top)
Flags: needinfo?(dao)
Attachment #8612757 - Attachment is obsolete: true
Attachment #8614575 - Attachment is obsolete: true
Attachment #8612757 - Flags: review?(dao)
Attachment #8616932 - Flags: review?(dao)
Attachment #8616932 - Flags: review?(bugs)
Comment on attachment 8616932 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes

>+      case "MozDOMFullscreen:Entered": {
>+        let browser = event.originalTarget;
calling event.originalTarget "browser" is rather misleading here.
Why not just say 'let originalTarget = ' and then explicitly assign to a browser variable when you know you have a
browser element.


>+        if (browser.tagName != "xul:browser" || !this._isChromeNode(browser)) {
>+          let topWin = browser.ownerDocument.defaultView.top;
>+          browser = gBrowser.getBrowserForContentWindow(topWin);
I would still check, or perhaps assert if we have some nightly-only js asserts that you have a browser here.
Attachment #8616932 - Flags: review?(bugs) → review+
Attachment #8616932 - Attachment is obsolete: true
Attachment #8616932 - Flags: review?(dao)
Attachment #8616995 - Flags: review?(dao)
Small fix to the previous version
Attachment #8616995 - Attachment is obsolete: true
Attachment #8616995 - Flags: review?(dao)
Attachment #8617003 - Flags: review?(dao)
Comment on attachment 8617003 [details] [diff] [review]
patch 7 - apply fullscreen state after window changes, r=smaug

>+        if (this._isBrowser(originalTarget)) {
>+          browser = originalTarget;
>+        } else {
>+          let topWin = originalTarget.ownerDocument.defaultView.top;
>+          browser = gBrowser.getBrowserForContentWindow(topWin);
>+          if (!browser) {
>+            document.mozCancelFullScreen();
>+            break;
>+          }
>+        }

Please add a comment on why those two cases exist (i.e. remote vs. in-process browsers).

>+        if (this._isRemoteBrowser(browser)) {
>+          browser.messageManager.sendAsyncMessage("DOMFullscreen:Entered");
>+        }

Why is this done only for remote browsers?
Attachment #8617003 - Attachment is obsolete: true
Attachment #8617003 - Flags: review?(dao)
Attachment #8617599 - Flags: review?(dao)
Attachment #8617599 - Flags: review?(dao) → review+
Attachment #8617900 - Flags: checkin?(cbook)
Comment on attachment 8617900 [details] [diff] [review]
followup patch to fix test failure

https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc4df62fcfe
Attachment #8617900 - Flags: checkin?(cbook) → checkin+
Depends on: 1173768
Depends on: 1173866
Depends on: 1173930
Depends on: 1191148
Depends on: 1247265
Duplicate of this bug: 756334
Depends on: CVE-2016-9065
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.