1168705 - Align fullscreenchange event as well as all MozDOMFullscreen:* events align to animation frame

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - allow dispatching events with refresh driver tick

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - move content fullscreen events after chrome-only's

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - make Request & Exit event synchronous

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - make Request & Exit event synchronous

Wrong patch, sorry.

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - align fullscreen events to refresh driver tick

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - allow dispatching events with refresh driver tick

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - align fullscreen events to refresh driver tick

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().

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - allow dispatching events with refresh driver tick

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

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - allow dispatching events with refresh driver tick, r=roc

Changed that place to a for loop. Looks better?

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

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 17

[Nathan Froyd [:froydnj]]

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?

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - make several internal events be dispatched synchronous

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - make several internal events be dispatched synchronous

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b9226a925d1

Comment 25

[Pulsebot]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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