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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: xidorn, Assigned: xidorn)

Tracking

(Blocks 1 bug)

Trunk
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox42 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

1.86 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.34 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.53 KB, patch
khuey
: review+
Details | Diff | Splinter Review
7.46 KB, patch
smaug
: review+
dao
: review+
Details | Diff | Splinter Review
Assignee

Description

4 years ago
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.
Assignee

Comment 1

4 years ago
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.
Assignee

Comment 4

4 years ago
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.
Attachment #8613728 - Flags: review?(bugs) → review+
Assignee

Comment 5

4 years ago
The code this patch depends on should be clear now. Request for review.
Attachment #8615046 - Flags: review?(bugs)
Assignee

Comment 6

4 years ago
Wrong patch, sorry.
Attachment #8615046 - Attachment is obsolete: true
Attachment #8615046 - Flags: review?(bugs)
Attachment #8615047 - Flags: review?(bugs)
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
Attachment #8615047 - Flags: review?(bugs) → review+
Attachment #8616540 - Flags: review?(bugs) → review+
For the patch 1 we probably want to add mPendingEvents to nsRefreshDriver::ObserverCount so that
::Tick doesn't return early.
Assignee

Comment 10

4 years ago
Assignee: nobody → quanxunzhen
Attachment #8613724 - Attachment is obsolete: true
Attachment #8617004 - Flags: review?(roc)
Assignee

Comment 11

4 years ago
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().
Attachment #8616540 - Attachment is obsolete: true
Attachment #8617631 - Flags: review?(bugs)
Attachment #8617631 - Flags: review?(bugs) → review+
Assignee

Comment 12

4 years ago
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.
Assignee

Comment 13

4 years ago
Fix the "ObserverCount() == 0" assertion.
Attachment #8617004 - Attachment is obsolete: true
Attachment #8621357 - Flags: review?(roc)
Attachment #8621357 - Flags: review?(nfroyd)
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.
Attachment #8621357 - Flags: review?(roc) → review+
Assignee

Comment 15

4 years ago
Changed that place to a for loop. Looks better?
Attachment #8621357 - Attachment is obsolete: true
Attachment #8621357 - Flags: review?(nfroyd)
Attachment #8621413 - Flags: review?(nfroyd)
Attachment #8621413 - Flags: feedback?(roc)
Assignee

Comment 16

4 years ago
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?
Attachment #8621413 - Flags: review?(nfroyd)
Assignee

Comment 18

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

Updated

4 years ago
Attachment #8621413 - Flags: review?(khuey)
Assignee

Updated

4 years ago
Depends on: 1174323
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?
Attachment #8621413 - Flags: review?(khuey) → review+
Assignee

Comment 20

4 years ago
(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.
Assignee

Updated

4 years ago
Depends on: 1175485
Assignee

Updated

4 years ago
Blocks: 1177155
Assignee

Comment 21

4 years ago
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.
Assignee

Comment 22

4 years ago
Attachment #8615047 - Attachment is obsolete: true
Attachment #8627061 - Flags: review?(dao)
Attachment #8627061 - Flags: review?(bugs)
Assignee

Comment 23

4 years ago
Attachment #8627061 - Attachment is obsolete: true
Attachment #8627061 - Flags: review?(dao)
Attachment #8627061 - Flags: review?(bugs)
Attachment #8627063 - Flags: review?(dao)
Attachment #8627063 - Flags: review?(bugs)
Attachment #8627063 - Flags: review?(dao) → review+
Attachment #8627063 - Flags: review?(bugs) → review+
You need to log in before you can comment on or make changes to this bug.