To achieve this, I plan to: 1. add an array of nsIDOMEvent to dispatch in nsRefreshDriver; 2. add another method to AsyncEventDispatcher, probably call RunDOMEventNextFlush or something like that; 3. change the order of the fullscreen events, and make the chrome-only events be posted first; 4. use the new method added in step 2 to post the events. This could potentially contribute to bug 1168274 and bug 1167432.

It is probably also fine to post the whole ApplyFullscreen to be run before the restyle in the next refresh driver tick and trigger those events synchronously. But I suspect posting the events there is potentially simpler, because I feel it is always safer to run events asynchronously, so that the content script won't interrupt any ongoing document state change. In addition, whenever we change the document state, the restyle will be triggered in the next tick, so changing the document state first then posting events to refresh driver should be almost effectively identical to post the whole ApplyFunction to refresh driver and posts the event asynchronously. But I guess we probably shouldn't bother AsyncEventDispatcher for this, because we don't need to have an Runnable for it if we are going to queue the events in the refresh driver directly.

The code touched in this bug actually depends on bug 1161802 and bug 1168028. However, I cannot mark it depend on them because bug 1167432 is a regression from a dependency of those two bug, and this bug is expected to fix bug 1167432. So there is a cyclic dependency here. There are two more patches for actually doing the alignment, but I'll put off them given I haven't landed either of the dependencies.

The code this patch depends on should be clear now. Request for review.

Wrong patch, sorry.

Comment on attachment 8615047 [details] [diff] [review] patch 3 - make Request & Exit event synchronous It is not quite clear to me why these need to be sync, but should be fine. These are chrome-only events anyway. >- (new AsyncEventDispatcher( >- this, NS_LITERAL_STRING("MozDOMFullscreen:Exit"), >- /* Bubbles */ true, /* ChromeOnly */ true))->PostDOMEvent(); >+ nsContentUtils::DispatchEventOnlyToChrome( >+ this, ToSupports(this), NS_LITERAL_STRING("MozDOMFullscreen:Exit"), >+ /* Bubbles */ true, /* ChromeOnly */ true, /* DefaultAction */ nullptr); The second boolean parameter isn't "ChromeOnly" but cancelable. And it should be calse here. >+ nsContentUtils::DispatchEventOnlyToChrome( >+ this, ToSupports(this), NS_LITERAL_STRING("MozDOMFullscreen:Request"), >+ /* Bubbles */ true, /* ChromeOnly */ true, /* DefaultAction */ nullptr); ditto

For the patch 1 we probably want to add mPendingEvents to nsRefreshDriver::ObserverCount so that ::Tick doesn't return early.

Small change to DispatchCustomEventWithFlush(). The first parameter is changed from nsIDocument to nsINode for the update in bug 1161802. And thus aTarget->GetShell() is changed to aTarget->OwnerDoc()->GetShell().

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c11bf50deefa It seems there is an issue with this patchset. Some events may not be dispatched when the refresh driver is released. Not sure how should I fix that problem. One option is just to drop all pending events before releasing the refresh driver. The other option is to purge all events to a document when the document is removed from the presshell. However there is a problem that, in the current patch, I use nsIDOMEventTarget, but we cannot check whether a nsIDOMEventTarget is in a document. We probably can change that to nsINode here. Not sure whether we will align other targets here, but probably doesn't matter. I thought about that, like other things attached to refresh driver, we may store the pending events in nsDocument, and set the document as an observer in the refresh driver. But what I'm concerned is that, there are events we want to keep the dispatch order inter-document (e.g. for nested document), and storing the pending events in document could destroy the order.

Fix the "ObserverCount() == 0" assertion.

Comment on attachment 8621357 [details] [diff] [review] patch 1 - allow dispatching events with refresh driver tick Review of attachment 8621357 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsRefreshDriver.cpp @@ +2161,5 @@ > + auto end = std::remove_if( > + begin, mPendingEvents.end(), [=](const PendingEvent& aEvent) { > + return aEvent.mTarget->OwnerDoc() == aDocument; > + }); > + mPendingEvents.TruncateLength(end - begin); I'd prefer to just write this out as a loop from the back to the front removing the elements. A bit clearer than this way.

Changed that place to a for loop. Looks better?

The other problem in the try push in comment 12 is the pointerlock test failure. I cannot reproduce that failure on my Windows, but it seems to be a permafail on Mac OS X 10.6 (which is the only platform this test is actually enabled on test machines). An initial test shows the problem might be that we synthesize the mouse event during refresh driver tick (in fullscreenchange event handler). Making synthesizeMouse be called asynchronously seems to fix this issue [1], but it's not clear to me how a synchronous call makes that failed. Need more investigation. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=29f35cd7d3b4

Comment on attachment 8621413 [details] [diff] [review] patch 1 - allow dispatching events with refresh driver tick, r=roc Review of attachment 8621413 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a DOM or layout peer, and it looks like roc gave r+ to this patch in comment 14?

Oh, sorry I forgot to check peer list first. I saw your name in the reviwer list of those files so...

Comment on attachment 8621413 [details] [diff] [review] patch 1 - allow dispatching events with refresh driver tick, r=roc Review of attachment 8621413 [details] [diff] [review]: ----------------------------------------------------------------- I think a layout peer would be a better reviewer... ::: dom/base/nsDocument.cpp @@ +3914,5 @@ > mExternalResourceMap.HideViewers(); > if (IsEventHandlingEnabled()) { > RevokeAnimationFrameNotifications(); > } > + mPresShell->GetPresContext()->RefreshDriver()->CancelPendingEvents(this); Do we need to null check GetPresContext here? Or are we guaranteed that if we have a pres shell we have a pres context?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #19) > Comment on attachment 8621413 [details] [diff] [review] > patch 1 - allow dispatching events with refresh driver tick, r=roc > > Review of attachment 8621413 [details] [diff] [review]: > ----------------------------------------------------------------- > > I think a layout peer would be a better reviewer... roc is a layout peer and he has reviewed this. I thought it also needs a DOM peer for the change in nsDocument. > ::: dom/base/nsDocument.cpp > @@ +3914,5 @@ > > mExternalResourceMap.HideViewers(); > > if (IsEventHandlingEnabled()) { > > RevokeAnimationFrameNotifications(); > > } > > + mPresShell->GetPresContext()->RefreshDriver()->CancelPendingEvents(this); > > Do we need to null check GetPresContext here? Or are we guaranteed that if > we have a pres shell we have a pres context? It seems to me RevokeAnimationFrameNotifications() does the same thing. If you think we need a null check here, I can add. Probably we also want to add null check in RevokeAnimationFrameNotifications() as well.

Probably we want to make all internal events be dispatched synchronously. Currently we send the async message to the content process in MozDOMFullscreen:{Entered,Exited}. If we dispatch them asynchronously in the next refresh tick, the content process may have more delay to handle this change.

https://hg.mozilla.org/integration/mozilla-inbound/rev/951116f384da https://hg.mozilla.org/integration/mozilla-inbound/rev/eaee95d5af13 https://hg.mozilla.org/integration/mozilla-inbound/rev/76c1d4339e63 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f3f04e74252

https://hg.mozilla.org/mozilla-central/rev/951116f384da https://hg.mozilla.org/mozilla-central/rev/eaee95d5af13 https://hg.mozilla.org/mozilla-central/rev/76c1d4339e63 https://hg.mozilla.org/mozilla-central/rev/7f3f04e74252