Closed
Bug 1415780
Opened 7 years ago
Closed 6 years ago
Fix timeout in test_event-dispatch.html with conformant Promise handling
Categories
(Core :: DOM: Animation, enhancement, P2)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla60
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(13 files)
59 bytes,
text/x-review-board-request
|
smaug
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
masayuki
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
hiro
:
review+
birtles
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
hiro
:
review+
|
Details |
Below is the test cases that cause the timeout: 'Active -> Idle, setting Animation.timeline = null' 'Set timeline and play transition after clearing the timeline.' It seems setting null timeline caused it?
Assignee | ||
Comment 1•7 years ago
|
||
In case of transitions tests: 'Before -> Idle (Animation.timeline = null)' 'Active -> Idle, no delay (Animation.timeline = null)' 'Active -> Idle, with positive delay (Animation.timeline = null)' 'Active -> Idle, with negative delay (Animation.timeline = null)'
Assignee | ||
Comment 2•7 years ago
|
||
I will start looking this. As far as I cal tell, this is the last failure caused by unknown reason.
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 3•7 years ago
|
||
Hmm, it seems to me that cancel event will never be fired if the target animation is the last one since we unregister refresh driver in DocumentTimeline::RemoveAnimation() if the removing animation is the last one. I have no idea why the failure cases don't fail on Stylo though.
Flags: needinfo?(hikezoe)
Comment 4•7 years ago
|
||
Is this bug about css-animations/test_event-dispatch.html or css-transitions/test_event-dispatch.html ? Cancel events should end up being dispatched on the next tick due to: http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/animation/Animation.cpp#808-817 i.e. that should make us the document timeline re-register with the refresh driver. Perhaps the ordering is different now and we end up re-registering but then unregistering.
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #4) > Is this bug about css-animations/test_event-dispatch.html or > css-transitions/test_event-dispatch.html ? > > Cancel events should end up being dispatched on the next tick due to: > > > http://searchfox.org/mozilla-central/rev/ > c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/animation/Animation.cpp#808-817 Thanks! It helped me to understand what's wrong. The test case set null timeline, not using cancel().
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > (In reply to Brian Birtles (:birtles) from comment #4) > > Is this bug about css-animations/test_event-dispatch.html or > > css-transitions/test_event-dispatch.html ? > > > > Cancel events should end up being dispatched on the next tick due to: > > > > > > http://searchfox.org/mozilla-central/rev/ > > c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/animation/Animation.cpp#808-817 > > Thanks! It helped me to understand what's wrong. The test case set null > timeline, not using cancel(). Oh wait. SetTimelineNoUpdate() also calls UpdateTiming() there..
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > > (In reply to Brian Birtles (:birtles) from comment #4) > > > Is this bug about css-animations/test_event-dispatch.html or > > > css-transitions/test_event-dispatch.html ? > > > > > > Cancel events should end up being dispatched on the next tick due to: > > > > > > > > > http://searchfox.org/mozilla-central/rev/ > > > c99d035f00dd894feff38e4ad28a73fb679c63a6/dom/animation/Animation.cpp#808-817 > > > > Thanks! It helped me to understand what's wrong. The test case set null > > timeline, not using cancel(). > > Oh wait. SetTimelineNoUpdate() also calls UpdateTiming() there.. I guess re-registering refresh driver fails if we have no timeline.
Assignee | ||
Comment 8•7 years ago
|
||
It seems to me that we don't need to eagerly remove the animation from the old timeline in Animation::SetTimelineNoUpdate(). We end up removing it in DocumentTimeline::WillRefresh() in the next tick.
Assignee | ||
Comment 9•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > It seems to me that we don't need to eagerly remove the animation from the > old timeline in Animation::SetTimelineNoUpdate(). We end up removing it in > DocumentTimeline::WillRefresh() in the next tick. Gah, it breaks bug 1277272. So nsAnimationManager or nsTransitionManager observes refresh driver directly?
Comment 10•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > > It seems to me that we don't need to eagerly remove the animation from the > > old timeline in Animation::SetTimelineNoUpdate(). We end up removing it in > > DocumentTimeline::WillRefresh() in the next tick. > > Gah, it breaks bug 1277272. So nsAnimationManager or nsTransitionManager > observes refresh driver directly? No, they are called from within nsRefreshDriver::Tick: http://searchfox.org/mozilla-central/rev/c99d035f00dd894feff38e4ad28a73fb679c63a6/layout/base/nsRefreshDriver.cpp#1614-1620
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #10) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #9) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > > > It seems to me that we don't need to eagerly remove the animation from the > > > old timeline in Animation::SetTimelineNoUpdate(). We end up removing it in > > > DocumentTimeline::WillRefresh() in the next tick. > > > > Gah, it breaks bug 1277272. So nsAnimationManager or nsTransitionManager > > observes refresh driver directly? > > No, they are called from within nsRefreshDriver::Tick: > > > http://searchfox.org/mozilla-central/rev/ > c99d035f00dd894feff38e4ad28a73fb679c63a6/layout/base/nsRefreshDriver. > cpp#1614-1620 What I meant is that nsAnimationManager or nsTransitionManager should keep the nsRefreshDriver awake, here in nsRefreshDriver::Tick for (uint32_t i = 0; i < ArrayLength(mObservers); ++i) { .. .. DispatchAnimationEvents(); If mObservers is empty the refresh driver never calls DispatchAnimationEvents(), that's the problem of this bug.
Assignee | ||
Comment 12•7 years ago
|
||
To be more clear, when setting null timeline for an animation, the DocumentTimeline unregisters itself from nsRefreshDriver (mObservers there). And in this test case we fail to re-register document timeline to the refresh driver since the animation has no timeline. But even if we succeeded to re-register document timeline to the refresh driver, we will hit by bug 1277272 in some cases.
Assignee | ||
Comment 13•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12) > To be more clear, when setting null timeline for an animation, the > DocumentTimeline unregisters itself from nsRefreshDriver (mObservers there). > > And in this test case we fail to re-register document timeline to the > refresh driver since the animation has no timeline. But even if we > succeeded to re-register document timeline to the refresh driver, we will > hit by bug 1277272 in some cases. (In reply to Hiroyuki Ikezoe (:hiro) from comment #11) > (In reply to Brian Birtles (:birtles) from comment #10) > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #9) > > > (In reply to Hiroyuki Ikezoe (:hiro) from comment #8) > > > > It seems to me that we don't need to eagerly remove the animation from the > > > > old timeline in Animation::SetTimelineNoUpdate(). We end up removing it in > > > > DocumentTimeline::WillRefresh() in the next tick. > > > > > > Gah, it breaks bug 1277272. So nsAnimationManager or nsTransitionManager > > > observes refresh driver directly? > > > > No, they are called from within nsRefreshDriver::Tick: > > > > > > http://searchfox.org/mozilla-central/rev/ > > c99d035f00dd894feff38e4ad28a73fb679c63a6/layout/base/nsRefreshDriver. > > cpp#1614-1620 > > What I meant is that nsAnimationManager or nsTransitionManager should keep > the nsRefreshDriver awake, here in nsRefreshDriver::Tick keep awake *if the manager has queued events, of course*.
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 14•7 years ago
|
||
I noticed that observing refresh driver by nsAnimationManager also solves bug 1356533 since we don't need to enumerate documents in each tick, but as a side effect, we can't dispatch transition events prior to animation events. :/
Comment 15•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #14) > I noticed that observing refresh driver by nsAnimationManager also solves > bug 1356533 since we don't need to enumerate documents in each tick, but as > a side effect, we can't dispatch transition events prior to animation > events. :/ Yeah, and apart from that I think there are a number of ordering requirements in the HTML spec that we currently don't get right. We should see how much Chrome follows that ordering and find a way to align where we need to.
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #15) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #14) > > I noticed that observing refresh driver by nsAnimationManager also solves > > bug 1356533 since we don't need to enumerate documents in each tick, but as > > a side effect, we can't dispatch transition events prior to animation > > events. :/ > > Yeah, and apart from that I think there are a number of ordering > requirements in the HTML spec that we currently don't get right. We should > see how much Chrome follows that ordering and find a way to align where we > need to. It seems to me that they don't care about the document order[1] nor the event order[2] so far. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/PageAnimator.cpp?type=cs&q=ServiceScriptedAnimations&l=31 [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/ScriptedAnimationController.cpp?l=96 As for fixing this bug, I think deferring to remove animation from timeline to the next tick when setting null timeline and unregistering refresh driver in DocumentTimeline dtor is a feasible way for now. This way works fine on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=46c0c71ffb7f72b559b87effa6a1f01fef2ff07b Also I tried to write a reftest that animationcancel event failed to be fired reliably. But after the try, I noticed that we can't write reftest since reftest harness uses setTimeout(0) repeatedly so refresh driver keeps ticking.
Assignee | ||
Comment 17•7 years ago
|
||
The reason why this failure doesn't appear on stylo is that PostRestyleEventForAnimations call [1] in EffectCompositor::PreTraverseInSubtree() and the call makes nsRefreshDriver::AddStyleFlushObserver() happen. What happens in the case of stylo is; 1) Set timeline for the animation to null inside the callback of animationstart event (precisely it's inside a Promise callback) 2) Call RequestRestyle(Layer) 3) The animated element is stored in EffectCompositor::mElementsToRestyle (Above 1 - 3 processed in DispatchAnimationEvents()) 4) Clear nsRefreshDriver::mStyleFlushObservers just before processing restyle 5) Start restyling process 6) Call PostRestyleEventForAnimations in EffectCompositor::PreTraverseInSubtree() 7) PostRestyleEventForAnimations ends up calling nsIPresShell::EnsureStyleFlush() in NoteDirtyElement(), it leads to nsRefreshDriver::AddStyleFlushObserver() 8) So, in the next tick the refresh driver still have a style flush observer, then cancel event happens So, if we don't drop the PostRestyleEventForAnimations call, this test will never fail. Actually, IIRC, the call is for processing the final styling for CSS animations/transitions that have been removed in the first traversal (e.g. setting animation-name is none, etc.), so it's not so good for this failure, but anyway, it's not a critical issue. Downgrading to P3 priority. [1] https://hg.mozilla.org/mozilla-central/file/f2cf6d147380/dom/animation/EffectCompositor.cpp#l1084
Priority: P2 → P3
Assignee | ||
Comment 18•6 years ago
|
||
In the latest try bug 1193394 comment 81, this test failed on stylo too. Something might have been changed.
Flags: needinfo?(hikezoe)
Summary: Fix timeout in test_event-dispatch.html with conformant Promise handling and with disabling stylo → Fix timeout in test_event-dispatch.html with conformant Promise handling
Assignee | ||
Comment 19•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #18) > In the latest try bug 1193394 comment 81, this test failed on stylo too. > Something might have been changed. The change is this <https://hg.mozilla.org/mozilla-central/rev/dae099622091#l1.17>. We bail out in the failure case (setting null timeline case). So we can no longer rely on the PostRestyleEventForAnimations call, we have to observe refresh driver, it's a big deal.
Flags: needinfo?(hikezoe)
Assignee | ||
Updated•6 years ago
|
Priority: P3 → P2
Assignee | ||
Comment 20•6 years ago
|
||
My basic idea here is to create a new instance that handles CSS animation/transition events (and eventually web animation events as well) per each document (I plan to let nsPresContext have the instance), and when we queue any animation events, nsRefreshDriver adds it into an array just like we do for mStyleFlushObservers and calls EnsureTimerStarted().
Assignee | ||
Comment 21•6 years ago
|
||
Mostly done. :) https://treeherder.mozilla.org/#/jobs?repo=try&revision=d024d36eaf64dcbcdf0039f80ce0267f738c86ec Need to write more commit messages there.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8945308 [details] Bug 1415780 - Don't count all observers for nsRefreshDriver. https://reviewboard.mozilla.org/r/215502/#review221252 ::: layout/base/nsRefreshDriver.h:399 (Diff revision 1) > eNeverAdjustTimer = 1 << 2, > }; > void EnsureTimerStarted(EnsureTimerStartedFlags aFlags = eNone); > void StopTimer(); > > + bool HasObserver() const; I think I'd call this HasObservers() ::: layout/base/nsRefreshDriver.h:401 (Diff revision 1) > void EnsureTimerStarted(EnsureTimerStartedFlags aFlags = eNone); > void StopTimer(); > > + bool HasObserver() const; > uint32_t ObserverCount() const; > - uint32_t ImageRequestCount() const; > + bool HasImageRequest() const; And this HasImageRequests()
Attachment #8945308 -
Flags: review?(bugs) → review+
Comment 33•6 years ago
|
||
mozreview-review |
Comment on attachment 8945309 [details] Bug 1415780 - Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements. https://reviewboard.mozilla.org/r/215504/#review221516 ::: layout/style/nsCSSPseudoElements.cpp:162 (Diff revision 1) > return PseudoElementHasFlags(aType, > CSS_PSEUDO_ELEMENT_SUPPORTS_USER_ACTION_STATE); > } > + > +/* static */ nsString > +nsCSSPseudoElements::PseudoTypeAsString(CSSPseudoElementType aPseudoType) Should this be s/CSSPseudoElementType/Type/ here for consistency with the rest of this file and the header file?
Attachment #8945309 -
Flags: review?(bbirtles) → review+
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8945310 [details] Bug 1415780 - Drop the comment for nsAnimationManager::DispatchEvents(). https://reviewboard.mozilla.org/r/215506/#review221518
Attachment #8945310 -
Flags: review?(bbirtles) → review+
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8945311 [details] Bug 1415780 - Rename DelayedEventsDispatcher to AnimationEventDispatcher. https://reviewboard.mozilla.org/r/215508/#review221520 ::: commit-message-d878e:5 (Diff revision 1) > +which is for full-screen event, into this in near future. Though we should > +integrate it in the end. (Nit: I'm not sure what this last sentence means. I suggest we could drop it.)
Attachment #8945311 -
Flags: review?(bbirtles) → review+
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8945312 [details] Bug 1415780 - Split AnimationEventDipatcher into an independent file. https://reviewboard.mozilla.org/r/215510/#review221522 ::: dom/animation/AnimationEventDispatcher.h:138 (Diff revision 1) > +} > + > +} // namespace mozilla > + > +#endif // mozilla_AnimationEventDispatcher_h > + Nit: blank line here
Attachment #8945312 -
Flags: review?(bbirtles) → review+
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8945313 [details] Bug 1415780 - De-templatize AnimationEventDispatcher. https://reviewboard.mozilla.org/r/215512/#review221530 ::: dom/animation/AnimationEventDispatcher.h:26 (Diff revision 1) > > -template <class EventInfo> > +struct AnimationEventInfo { > + enum class EventType : uint8_t { > + CSSTransition, > + CSSAnimation, > + WebAnimation, I love that you're fixing the web animations bug at the same time! However, it might be better to leave this enum value out until we actually start using this class for Web Animations too? ::: dom/animation/AnimationEventDispatcher.h:32 (Diff revision 1) > + union { > + InternalTransitionEvent mCSSTransitionEvent; > + InternalAnimationEvent mCSSAnimationEvent; > + }; Oh, interesting. I guess this works because C++11 allows non-POD unions (including types that have virtual functions like these). And furthermore, because we define constructors for the union itself. The other option would be to use mozilla::Variant, but as far as I can tell, this looks to be ok. It might be worth getting masayuki or smaug to double-check this patch too. ::: dom/animation/AnimationEventDispatcher.h:51 (Diff revision 1) > + , mAnimation(aAnimation) > + , mTimeStamp(aTimeStamp) > + , mCSSAnimationEvent(true, aMessage) > + { > + aAnimationName->ToString(mCSSAnimationEvent.mAnimationName); > + // XXX Looks like nobody initialize WidgetEvent::time (We really need to do bug 1026806 :/) ::: dom/animation/AnimationEventDispatcher.h:109 (Diff revision 1) > + void FreeUnionValue() > + { > + switch (mType) { > + case EventType::CSSTransition: > + mCSSTransitionEvent.~InternalTransitionEvent(); > + break; > + case EventType::CSSAnimation: > + mCSSAnimationEvent.~InternalAnimationEvent(); > + break; > + case EventType::WebAnimation: > + MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE(""); > + break; > + } > + } I think mozilla::Variant would give to use for free but I think it also assumes that its members types are copy-constructible and move-constructible. I wonder if there's anyway to specialize VariantImplementation::copyConstruct and have it call Duplicate / AssignXXXEventData as needed? Just a thought. Anyway, this looks good so far but I'll have a proper look after lunch.
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #37) > ::: dom/animation/AnimationEventDispatcher.h:32 > (Diff revision 1) > > + union { > > + InternalTransitionEvent mCSSTransitionEvent; > > + InternalAnimationEvent mCSSAnimationEvent; > > + }; > > Oh, interesting. I guess this works because C++11 allows non-POD unions > (including types that have virtual functions like these). And furthermore, > because we define constructors for the union itself. Yeah, "unrestricted unions". > ::: dom/animation/AnimationEventDispatcher.h:109 > (Diff revision 1) > > + void FreeUnionValue() > > + { > > + switch (mType) { > > + case EventType::CSSTransition: > > + mCSSTransitionEvent.~InternalTransitionEvent(); > > + break; > > + case EventType::CSSAnimation: > > + mCSSAnimationEvent.~InternalAnimationEvent(); > > + break; > > + case EventType::WebAnimation: > > + MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE(""); > > + break; > > + } > > + } > > I think mozilla::Variant would give to use for free but I think it also > assumes that its members types are copy-constructible and > move-constructible. I wonder if there's anyway to specialize > VariantImplementation::copyConstruct and have it call Duplicate / > AssignXXXEventData as needed? I didn't know mozilla::Variant, will look. Thanks!
Assignee | ||
Comment 39•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #38) > > ::: dom/animation/AnimationEventDispatcher.h:109 > > (Diff revision 1) > > > + void FreeUnionValue() > > > + { > > > + switch (mType) { > > > + case EventType::CSSTransition: > > > + mCSSTransitionEvent.~InternalTransitionEvent(); > > > + break; > > > + case EventType::CSSAnimation: > > > + mCSSAnimationEvent.~InternalAnimationEvent(); > > > + break; > > > + case EventType::WebAnimation: > > > + MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE(""); > > > + break; > > > + } > > > + } > > > > I think mozilla::Variant would give to use for free but I think it also > > assumes that its members types are copy-constructible and > > move-constructible. I wonder if there's anyway to specialize > > VariantImplementation::copyConstruct and have it call Duplicate / > > AssignXXXEventData as needed? > > I didn't know mozilla::Variant, will look. Thanks! I did rewrite the code with Variant, it gets much simpler! I will revise this patch.
Comment 40•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #37) > I think mozilla::Variant would give to use for free but I think it also... Wow, my English is so bad recently. I *meant* to say "I think mozilla::Variant would give us this for free..." Anyway, I think you worked it out!
Assignee | ||
Comment 41•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #37) > ::: dom/animation/AnimationEventDispatcher.h:109 > (Diff revision 1) > > + void FreeUnionValue() > > + { > > + switch (mType) { > > + case EventType::CSSTransition: > > + mCSSTransitionEvent.~InternalTransitionEvent(); > > + break; > > + case EventType::CSSAnimation: > > + mCSSAnimationEvent.~InternalAnimationEvent(); > > + break; > > + case EventType::WebAnimation: > > + MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE(""); > > + break; > > + } > > + } > > I think mozilla::Variant would give to use for free but I think it also > assumes that its members types are copy-constructible and > move-constructible. I wonder if there's anyway to specialize > VariantImplementation::copyConstruct and have it call Duplicate / > AssignXXXEventData as needed? Hmm, I can't solve this. Variant requires the copy-contructor for InternalTransitionEvent and InternalAnimationEvent in any way. We can, of course, use the default copy constructor for them, but it sesms not a bit cost wise (it looks negligible though). That's said the cost will be eliminated once we make WidgetEvent move friendly (bug 1433008). So I'd use the default copy-constructor and copy-assignment here. Brian what do you think? FWIW, here is a patch for bug 1433008. https://pastebin.mozilla.org/9076696
Comment 42•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #41) > (In reply to Brian Birtles (:birtles) from comment #37) > > I think mozilla::Variant would give to use for free but I think it also > > assumes that its members types are copy-constructible and > > move-constructible. I wonder if there's anyway to specialize > > VariantImplementation::copyConstruct and have it call Duplicate / > > AssignXXXEventData as needed? > > Hmm, I can't solve this. Variant requires the copy-contructor for > InternalTransitionEvent and InternalAnimationEvent in any way. Yeah, I thought you might be able to specialize VariantImplementation::copyConstruct somehow. I guess not. Thanks for trying though. > We can, of > course, use the default copy constructor for them, but it sesms not a bit > cost wise (it looks negligible though). That's said the cost will be > eliminated once we make WidgetEvent move friendly (bug 1433008). So I'd use > the default copy-constructor and copy-assignment here. Brian what do you > think? > > FWIW, here is a patch for bug 1433008. https://pastebin.mozilla.org/9076696 I think Olli of Masayuki would be better able to comment on how we should approach that.
Assignee | ||
Comment 43•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #42) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #41) > > (In reply to Brian Birtles (:birtles) from comment #37) > > > I think mozilla::Variant would give to use for free but I think it also > > > assumes that its members types are copy-constructible and > > > move-constructible. I wonder if there's anyway to specialize > > > VariantImplementation::copyConstruct and have it call Duplicate / > > > AssignXXXEventData as needed? > > > > Hmm, I can't solve this. Variant requires the copy-contructor for > > InternalTransitionEvent and InternalAnimationEvent in any way. > > Yeah, I thought you might be able to specialize > VariantImplementation::copyConstruct somehow. That was using the default copy-constructor, copied every members and then AssignXXXEventData. :/ > I think Olli of Masayuki would be better able to comment on how we should > approach that. Thanks for the suggestion, I will ping Masayuki first.
Comment 44•6 years ago
|
||
mozreview-review |
Comment on attachment 8945316 [details] Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime. https://reviewboard.mozilla.org/r/215518/#review221540
Attachment #8945316 -
Flags: review?(bbirtles) → review+
Comment 45•6 years ago
|
||
mozreview-review |
Comment on attachment 8945315 [details] Bug 1415780 - Let nsPresContext have AnimationEventDispatcher. https://reviewboard.mozilla.org/r/215516/#review221524 ::: dom/animation/AnimationEventDispatcher.h:176 (Diff revision 1) > }; > > class AnimationEventDispatcher > { > public: > AnimationEventDispatcher() : mIsSorted(true) { } Does this constructor need to initialize mPresContext to nullptr? ::: dom/animation/AnimationEventDispatcher.h:198 (Diff revision 1) > mIsSorted = false; > } > > - // This is exposed as a separate method so that when we are dispatching > - // *both* transition events and animation events we can sort both lists > - // once using the current state of the document before beginning any > + // Sort all pending CSS animation/transition events by scheduled event time > + // and composite order. > + // Section 3.3.5 in https://drafts.csswg.org/web-animations-1/#timelines Probably better to link to the specific algorithm involved, drop the section number (since it will probably change), and use the unversioned spec link: https://drafts.csswg.org/web-animations/#update-animations-and-send-events ::: layout/style/nsAnimationManager.cpp:321 (Diff revision 1) > Animation::UpdateTiming(aSeekFlag, aSyncNotifyFlag); > } > > ////////////////////////// nsAnimationManager //////////////////////////// > > -NS_IMPL_CYCLE_COLLECTION(nsAnimationManager, mEventDispatcher) > +NS_IMPL_CYCLE_COLLECTION_0(nsAnimationManager); Is this needed? Shouldn't we just make nsAnimationManager stop participating in cycle collection and just be a regular ref-counted class? See: https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/xpcom/base/nsCycleCollectionParticipant.h#923-925 ::: layout/style/nsTransitionManager.cpp:426 (Diff revision 1) > } > } > > ////////////////////////// nsTransitionManager //////////////////////////// > > -NS_IMPL_CYCLE_COLLECTION(nsTransitionManager, mEventDispatcher) > +NS_IMPL_CYCLE_COLLECTION_0(nsTransitionManager); Likewise here.
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8945314 [details] Bug 1415780 - Make AnimationEventDispatcher refcountable. https://reviewboard.mozilla.org/r/215514/#review221538 ::: commit-message-0f7ce:6 (Diff revision 1) > +Instead, if we were trying to make nsPresContext have the > +AnimationEventDispatcher as data object (not RefPtr<>) just like we did in > +CommonAnimationManager, we will fall into header inclusion hell since Element.h > +includes nsPresContext.h and AnimationEventDispatcher.h ends up including > +Element.h. Even if we could solve the inclusion hell, we will suffer from Rust > +bindgen issues for some reasons. I guess Maybe<> requires a complete type too. ::: dom/animation/AnimationEventDispatcher.h:173 (Diff revision 1) > } > return nullptr; > } > }; > > class AnimationEventDispatcher We should declare this class `final`. (Because we do decide to subclass it, we'll want to make the dtor virtual and hopefully dropping the `final` will remind us to do that.) ::: dom/animation/AnimationEventDispatcher.cpp:30 (Diff revision 1) > + > +NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE(AnimationEventDispatcher, AddRef) > +NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(AnimationEventDispatcher, Release) > + > +} // namespace mozilla > + Nit: blank line here
Attachment #8945314 -
Flags: review?(bbirtles) → review+
Comment 47•6 years ago
|
||
mozreview-review |
Comment on attachment 8945317 [details] Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver. https://reviewboard.mozilla.org/r/215520/#review221548 I haven't reviewed this properly yet but just a few comments in passing. ::: dom/animation/AnimationEventDispatcher.h:177 (Diff revision 1) > }; > > class AnimationEventDispatcher > { > public: > AnimationEventDispatcher() : mIsSorted(true) { } Should we initialize mIsObserving here as well? ::: dom/animation/AnimationEventDispatcher.h:198 (Diff revision 1) > { > mPendingEvents.AppendElements(Move(aEvents)); > mIsSorted = false; > + if (!mIsObserving) { > + mIsObserving = > + mPresContext->RefreshDriver()->ScheduleAnimatioinEventDispatch(this); s/Animatioin/Animation/ ::: layout/base/nsRefreshDriver.h:255 (Diff revision 1) > + bool appended = > + mAnimationEventFlushObservers.AppendElement(aDispatcher) != nullptr; Will this ever be nullptr? Isn't AutoTArray infallible? ::: layout/base/nsRefreshDriver.cpp:2395 (Diff revision 1) > +void > +nsRefreshDriver::CancelPendingAnimationEvents(AnimationEventDispatcher* aDispatcher) > +{ > + MOZ_ASSERT(aDispatcher); > + aDispatcher->ClearEventQueue(); > + mAnimationEventFlushObservers.RemoveElement(aDispatcher); Is this ok performance wise? If we have N subdocuments watching this then does this end up being O(N^2) when we close the root document?
Assignee | ||
Comment 48•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #42) > That was using the default copy-constructor, copied every members and then > AssignXXXEventData. :/ > > > I think Olli of Masayuki would be better able to comment on how we should > > approach that. > > Thanks for the suggestion, I will ping Masayuki first. Discussed with Masayuki on IRC, he's fine with either way. So I'd use the default copy-constructor here and drop it in bug 1433008.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 59•6 years ago
|
||
mozreview-review |
Comment on attachment 8945313 [details] Bug 1415780 - De-templatize AnimationEventDispatcher. https://reviewboard.mozilla.org/r/215512/#review221554 ::: dom/animation/AnimationEventDispatcher.h:23 (Diff revision 2) > > class nsPresContext; > > namespace mozilla { > > -template <class EventInfo> > +struct AnimationEventInfo { nit: |{| should be in next line. ::: dom/animation/AnimationEventDispatcher.h:86 (Diff revision 2) > + > + WidgetEvent* AsWidgetEvent() > + { > + if (mEvent.is<InternalTransitionEvent>()) { > + return &mEvent.as<InternalTransitionEvent>(); > + } else if (mEvent.is<InternalAnimationEvent>()) { nit: This can be just new |if| statement since the previous |if| block always return.
Attachment #8945313 -
Flags: review?(masayuki) → review+
Comment 60•6 years ago
|
||
mozreview-review |
Comment on attachment 8945313 [details] Bug 1415780 - De-templatize AnimationEventDispatcher. https://reviewboard.mozilla.org/r/215512/#review221556
Attachment #8945313 -
Flags: review?(bbirtles) → review+
Comment 61•6 years ago
|
||
mozreview-review |
Comment on attachment 8945315 [details] Bug 1415780 - Let nsPresContext have AnimationEventDispatcher. https://reviewboard.mozilla.org/r/215516/#review221558 r=me with the issues addressed
Attachment #8945315 -
Flags: review?(bbirtles) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 68•6 years ago
|
||
mozreview-review |
Comment on attachment 8945317 [details] Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver. https://reviewboard.mozilla.org/r/215520/#review221562 I'm going to cancel the review request on this patch for now because I'd like to review it with the comments raised in comment 47 addressed. I suspect if we don't need to null-check the result of AppendElement the logic will change a little, and I'd like to hear your thoughts about the performance considerations.
Attachment #8945317 -
Flags: review?(bbirtles)
Assignee | ||
Comment 69•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8945315 [details] Bug 1415780 - Let nsPresContext have AnimationEventDispatcher. https://reviewboard.mozilla.org/r/215516/#review221524 > Does this constructor need to initialize mPresContext to nullptr? I did forget to drop this constructor. :/ > Is this needed? Shouldn't we just make nsAnimationManager stop participating in cycle collection and just be a regular ref-counted class? > > See: > https://searchfox.org/mozilla-central/rev/062e1cf551f5bf3f0af33671b818f75a55ac497b/xpcom/base/nsCycleCollectionParticipant.h#923-925 Indeed. Fixed.
Assignee | ||
Comment 70•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8945317 [details] Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver. https://reviewboard.mozilla.org/r/215520/#review221548 > Will this ever be nullptr? Isn't AutoTArray infallible? I did innocently copy from AddStyleFlushObserver(). Fixed. > Is this ok performance wise? If we have N subdocuments watching this then does this end up being O(N^2) when we close the root document? Yeah, indeed. I didn't notice the fact, I copied this part from others as well. Anyway in the case of destroying the root document, that's not a good thing. We should solve this somehow. I'd like to defer this issue in a followup bug to fix also other part (i.e. style flush observers, etc.). Is that fine with you?
Comment 71•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #70) > > Is this ok performance wise? If we have N subdocuments watching this then does this end up being O(N^2) when we close the root document? > > Yeah, indeed. I didn't notice the fact, I copied this part from others as > well. Anyway in the case of destroying the root document, that's not a good > thing. We should solve this somehow. I'd like to defer this issue in a > followup bug to fix also other part (i.e. style flush observers, etc.). Is > that fine with you? If we're already doing that (and it seems we are) then that seems ok.
Assignee | ||
Comment 72•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #71) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #70) > > > Is this ok performance wise? If we have N subdocuments watching this then does this end up being O(N^2) when we close the root document? > > > > Yeah, indeed. I didn't notice the fact, I copied this part from others as > > well. Anyway in the case of destroying the root document, that's not a good > > thing. We should solve this somehow. I'd like to defer this issue in a > > followup bug to fix also other part (i.e. style flush observers, etc.). Is > > that fine with you? > > If we're already doing that (and it seems we are) then that seems ok. Filed bug 1433362.
Assignee | ||
Comment 73•6 years ago
|
||
Comment on attachment 8945317 [details] Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver. MozReview did not allow me to add r? flag.
Attachment #8945317 -
Flags: review?(bbirtles)
Comment 74•6 years ago
|
||
mozreview-review |
Comment on attachment 8945317 [details] Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver. https://reviewboard.mozilla.org/r/215520/#review221566 Is there any code we no longer need now? I mean, code that only existed to make sure we ticked the refresh driver once more to dispatch cancel events? (I know there used to be but I think we refactored things in such a way that we no longer needed it.) I think I want to take one last look at this with the following comments addressed, where applicable. ::: dom/animation/AnimationEventDispatcher.h:120 (Diff revision 3) > void QueueEvents(nsTArray<AnimationEventInfo>&& aEvents) > { > mPendingEvents.AppendElements(Move(aEvents)); > mIsSorted = false; > + if (!mIsObserving) { > + mPresContext->RefreshDriver()->ScheduleAnimationEventDispatch(this); Shouldn't this null-check mPresContext? And does it make sense to disassociate ourselves from the refresh driver in Disconnect()? And if we're doing that, presumably we could have a debug-only dtor that asserts that mIsObserving is false? (Although perhaps that's unnecessary.) ::: layout/base/PresShell.cpp:1364 (Diff revision 3) > if (mPresContext) { > - mPresContext->AnimationEventDispatcher()->ClearEventQueue(); > + mPresContext->CancelPendingAnimationEvents(); > } Would it be a bit less convoluted (and make my comment about the refresh driver below unnecessary) to just do: if (mPresContext) { rd->CancelPendingAnimationEvents(AnimationEventDispatcher()); } ? Then we wouldn't need to add CancelPendingAnimationEvents to nsPresContext. ::: layout/base/nsPresContext.cpp:3186 (Diff revision 3) > } > > +void > +nsPresContext::CancelPendingAnimationEvents() > +{ > + MOZ_ASSERT(mRefreshDriver); This could probably do with a comment explaining why we're sure we'll have a refresh driver here, e.g. "This is expected to be called when the corresponding pres shell is destroyed but before the refresh driver is destroyed." ::: layout/base/nsRefreshDriver.h:253 (Diff revision 3) > + NS_ASSERTION(!mAnimationEventFlushObservers.Contains(aDispatcher), > + "Double-adding animation event flush observer"); (Any reason to prefer NS_ASSERTION here? Do we do that elsewhere in this file?) ::: layout/base/nsRefreshDriver.cpp:1624 (Diff revision 3) > - for (uint32_t i = 0; i < documents.Length(); ++i) { > - nsIDocument* doc = documents[i]; > + RefPtr<AnimationEventDispatcher> kungFuDeathGrip(dispatcher); > + dispatcher->DispatchEvents(); What ensures that the next dispatcher in dispatchers is not destroyed while processing the previous dispatcher? i.e. do we need to take an owning ref to each of these dispatchers *before* iterating over them?
Attachment #8945317 -
Flags: review?(bbirtles)
Assignee | ||
Comment 75•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #74) > Comment on attachment 8945317 [details] > Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver. > > https://reviewboard.mozilla.org/r/215520/#review221566 > > Is there any code we no longer need now? I mean, code that only existed to > make sure we ticked the refresh driver once more to dispatch cancel events? > (I know there used to be but I think we refactored things in such a way that > we no longer needed it.) Ah right. We do that in Animation::CancelNoUpdate(). I will add an additional patch to remove that hack. > ::: dom/animation/AnimationEventDispatcher.h:120 > (Diff revision 3) > > void QueueEvents(nsTArray<AnimationEventInfo>&& aEvents) > > { > > mPendingEvents.AppendElements(Move(aEvents)); > > mIsSorted = false; > > + if (!mIsObserving) { > > + mPresContext->RefreshDriver()->ScheduleAnimationEventDispatch(this); > > Shouldn't this null-check mPresContext? I think we shouldn't. Instead I think we should add an assertion since QueueEvents() should only be called from DocumentTimeline::WillRefresh(), it means there is no possibility to destroy the pres context. > And does it make sense to disassociate ourselves from the refresh driver in > Disconnect()? Disconnect() seems to be bit late for me. Disconnect() is called from the dtor of nsPresContext. But hmm, is there any chance to destroy nsPresContext before we disassociate this event dispatcher in PresShell::Destory()? I am not sure but I understand your concern. So > And if we're doing that, presumably we could have a debug-only dtor that > asserts that mIsObserving is false? (Although perhaps that's unnecessary.) Yep, I'd add the assertion in the dtor and disassociate the dispatcher in Disconnect() as well just in case. Thanks for the suggestion! > ::: layout/base/PresShell.cpp:1364 > (Diff revision 3) > > if (mPresContext) { > > - mPresContext->AnimationEventDispatcher()->ClearEventQueue(); > > + mPresContext->CancelPendingAnimationEvents(); > > } > > Would it be a bit less convoluted (and make my comment about the refresh > driver below unnecessary) to just do: > > if (mPresContext) { > rd->CancelPendingAnimationEvents(AnimationEventDispatcher()); > } > > ? > > Then we wouldn't need to add CancelPendingAnimationEvents to nsPresContext. Sounds nice. I will do it. > ::: layout/base/nsRefreshDriver.h:253 > (Diff revision 3) > > + NS_ASSERTION(!mAnimationEventFlushObservers.Contains(aDispatcher), > > + "Double-adding animation event flush observer"); > > (Any reason to prefer NS_ASSERTION here? Do we do that elsewhere in this > file?) Yes, we do it in other places as well. > ::: layout/base/nsRefreshDriver.cpp:1624 > (Diff revision 3) > > - for (uint32_t i = 0; i < documents.Length(); ++i) { > > - nsIDocument* doc = documents[i]; > > + RefPtr<AnimationEventDispatcher> kungFuDeathGrip(dispatcher); > > + dispatcher->DispatchEvents(); > > What ensures that the next dispatcher in dispatchers is not destroyed while > processing the previous dispatcher? > > i.e. do we need to take an owning ref to each of these dispatchers *before* > iterating over them? Ah right. indeed. This will lead a crash. Good catch!
Comment 76•6 years ago
|
||
mozreview-review |
Comment on attachment 8945316 [details] Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime. https://reviewboard.mozilla.org/r/215518/#review221572 ::: commit-message-15596:1 (Diff revision 3) > +Bug 1415780 - Make AnimationEventDispatcher::Sort() private. r?birtles nit: s/Sort/SortEvents/
Comment 77•6 years ago
|
||
mozreview-review |
Comment on attachment 8945315 [details] Bug 1415780 - Let nsPresContext have AnimationEventDispatcher. https://reviewboard.mozilla.org/r/215516/#review221574 ::: commit-message-50321:1 (Diff revision 3) > +Bug 1415780 - Let nsPresContext have AnimationEventDipatcher. r?birtles s/AnimationEventDipatcher/AnimationEventDispatcher/
Comment 78•6 years ago
|
||
mozreview-review |
Comment on attachment 8945314 [details] Bug 1415780 - Make AnimationEventDispatcher refcountable. https://reviewboard.mozilla.org/r/215514/#review221576 ::: commit-message-0f7ce:1 (Diff revision 3) > +Bug 1415780 - Make AnimationEventDipatcher refcountable. r?birtles Here too: AnimationEventDispatcher (missing 's')
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 91•6 years ago
|
||
mozreview-review |
Comment on attachment 8945317 [details] Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver. https://reviewboard.mozilla.org/r/215520/#review221590 ::: dom/animation/AnimationEventDispatcher.cpp:46 (Diff revision 4) > +} > + > +void > +AnimationEventDispatcher::QueueEvents(nsTArray<AnimationEventInfo>&& aEvents) > +{ > + MOZ_ASSERT(mPresContext, ""); Any reason for the blank comment? ::: layout/base/nsPresContext.h:244 (Diff revision 4) > > mozilla::AnimationEventDispatcher* AnimationEventDispatcher() > { > return mAnimationEventDispatcher; > } > + (ReviewBoard doesn't tell me what changed here but it doesn't look intentional) ::: layout/base/nsRefreshDriver.h:517 (Diff revision 4) > + AutoTArray<RefPtr<mozilla::AnimationEventDispatcher>, 16> > + mAnimationEventFlushObservers; Do we really want this to take an owning reference? ::: layout/base/nsRefreshDriver.cpp:1621 (Diff revision 4) > - AutoTArray<nsCOMPtr<nsIDocument>, 32> documents; > - CollectDocuments(mPresContext->Document(), &documents); > + AutoTArray<RefPtr<AnimationEventDispatcher>, 16> dispatchers; > + mAnimationEventFlushObservers.SwapElements(dispatchers); Can we just take an owning reference here instead?
Comment 92•6 years ago
|
||
mozreview-review |
Comment on attachment 8945712 [details] Bug 1415780 - Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate. https://reviewboard.mozilla.org/r/215830/#review221598
Attachment #8945712 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 93•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #91) > ::: layout/base/nsRefreshDriver.cpp:1621 > (Diff revision 4) > > - AutoTArray<nsCOMPtr<nsIDocument>, 32> documents; > > - CollectDocuments(mPresContext->Document(), &documents); > > + AutoTArray<RefPtr<AnimationEventDispatcher>, 16> dispatchers; > > + mAnimationEventFlushObservers.SwapElements(dispatchers); > > Can we just take an owning reference here instead? We can. But do you mean doing AppendElement each AnimationEventDispatcher* in mAnimationEventFlushObservers to AutoTArray<RefPtr<AnimationEventDispatcher>> and then clearing mAnimationEventFlushObservers? I am not sure it's better than SwapElements().
Comment 94•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #93) > We can. But do you mean doing AppendElement each AnimationEventDispatcher* > in mAnimationEventFlushObservers to > AutoTArray<RefPtr<AnimationEventDispatcher>> and then clearing > mAnimationEventFlushObservers? I am not sure it's better than > SwapElements(). Right. Otherwise it seems like nsRefreshDriver should participate in cycle collection since the AnimationEffectDispatchers are included in cycle collection via the nsPresContext.
Comment 95•6 years ago
|
||
mozreview-review |
Comment on attachment 8945317 [details] Bug 1415780 - Let AnimationEventDispatcher observe nsRefreshDriver. https://reviewboard.mozilla.org/r/215520/#review221824 I'm going to be traveling from today, so r=me with the issues around the AnimationEventDispatcher lifetimes resolved. I think it's fine to keep non-owning references in the nsRefreshDriver since we have a fairly straightforward out-of-band setup for disassociating the dispatchers before either side dies. However, we should probably take an owning reference to each of them before iterating over the copy of list. That probably means we can't use swap which is a shame but the algorithm is already O(n) so iterating over the list once more is not an increase in complexity.
Attachment #8945317 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 96•6 years ago
|
||
Thanks! I've already revised the part of dispatching animation events, and pushed a try. I planed to push a new review request once the try finished. https://hg.mozilla.org/try/rev/bd840383d46ce77adf206b60a13ccaeafd72e09e#l5.48 https://treeherder.mozilla.org/#/jobs?repo=try&revision=188dceb99d22bc2ce30b5ff6329c8d258f5e0283 I am hoping this works for you. I have to run from now on as well, don't have time to push a new patch set unfortunately.
Comment 97•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #96) > Thanks! > > I've already revised the part of dispatching animation events, and pushed a > try. I planed to push a new review request once the try finished. > > https://hg.mozilla.org/try/rev/bd840383d46ce77adf206b60a13ccaeafd72e09e#l5.48 > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=188dceb99d22bc2ce30b5ff6329c8d258f5e0283 > > I am hoping this works for you. I have to run from now on as well, don't > have time to push a new patch set unfortunately. That looks great. Thanks!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 109•6 years ago
|
||
A final try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=12c14145ecfc5e7f8ed611f4f9a21dc47974367e
Comment 110•6 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/433a6f0d0ac3 Don't count all observers for nsRefreshDriver. r=smaug https://hg.mozilla.org/integration/autoland/rev/027a7ed3c948 Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements. r=birtles https://hg.mozilla.org/integration/autoland/rev/e298f242ce7c Drop the comment for nsAnimationManager::DispatchEvents(). r=birtles https://hg.mozilla.org/integration/autoland/rev/621b9aaf4a8f Rename DelayedEventsDispatcher to AnimationEventDispatcher. r=birtles https://hg.mozilla.org/integration/autoland/rev/ae7be65f3c88 Split AnimationEventDipatcher into an independent file. r=birtles https://hg.mozilla.org/integration/autoland/rev/c6567e4196f6 De-templatize AnimationEventDispatcher. r=birtles,masayuki https://hg.mozilla.org/integration/autoland/rev/f0cc29e7ccd7 Make AnimationEventDispatcher refcountable. r=birtles https://hg.mozilla.org/integration/autoland/rev/79e980195ee8 Let nsPresContext have AnimationEventDispatcher. r=birtles https://hg.mozilla.org/integration/autoland/rev/43d42ca7308f Make AnimationEventDispatcher::SortEvents() private. r=birtles https://hg.mozilla.org/integration/autoland/rev/a88250ad7a3e Let AnimationEventDispatcher observe nsRefreshDriver. r=birtles https://hg.mozilla.org/integration/autoland/rev/6bad89a17566 Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate. r=birtles
Comment 111•6 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1fa2f138319e Drop CollectDocuments in nsRefreshDriver.cpp. r=me CLOSED TREE
Comment 112•6 years ago
|
||
Backed out for build bustages on nsRefreshDriver.cpp:1606:1 and AnimationCommon.h:168:51 Treeherder link: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6bad89a1756671ae8583c1c0e44b2254a13e45d1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158838487&repo=autoland&lineNumber=18301 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158838473&repo=autoland&lineNumber=19495 Backout link: https://hg.mozilla.org/integration/autoland/rev/71ac93eb3d4cdcdcbca8188dd827a36991ff54e6
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 113•6 years ago
|
||
I think on mingw winbase.h is included after dom/animation/Animaion.h. I have no idea how to avoid this but it's a fundamental issue.
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 114•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #113) > I think on mingw winbase.h is included after dom/animation/Animaion.h. I > have no idea how to avoid this but it's a fundamental issue. To be clear, it's a fundamental issue regardless of these patches.
Assignee | ||
Comment 115•6 years ago
|
||
To eliminate these impacts by the macros defined in windows.h, we need to fix bug 834505, I think we should introduce WindowsUndefineMacros.h or some such just like we do in X11UndefineNone.h. Anyway, for this bug I am trying add '#undef GetCurrentTime' in some places.
Assignee | ||
Comment 116•6 years ago
|
||
There were two places that include window.h and cause this build failure; layout/generic/nsPluginFrame.h ipc/chromium/src/base/process_util.h In the former case we should add '#undef GetCurrentTime' in the undefining macro list [1]. But the latter case, I am not sure we should add the macro there. So I decided to add the undef macro in the template function , GetAnimationPhaseWithoutEffect where the build error happened. [1] https://hg.mozilla.org/mozilla-central/file/232dbd5acc3f/layout/generic/nsPluginFrame.h#l26
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 130•6 years ago
|
||
mozreview-review |
Comment on attachment 8946009 [details] Bug 1415780 - Drop CollectDocuments in nsRefreshDriver.cpp. https://reviewboard.mozilla.org/r/216078/#review221862
Attachment #8946009 -
Flags: review?(hikezoe) → review+
Assignee | ||
Comment 131•6 years ago
|
||
Comment on attachment 8945316 [details] Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime. MozReview got confused. Anyway this should work. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c245e53719c51e32beeb703bb024f597ccd22e85
Attachment #8945316 -
Flags: review+
Assignee | ||
Updated•6 years ago
|
Attachment #8946008 -
Flags: review?(bbirtles) → review+
Comment 132•6 years ago
|
||
I have seen the GetCurrentTime issue before, transiently, and then it went away. My best guess is that it's a Unified build issue. I don't care too much one way or the other about how it's fixed; but maybe the most permanent solution is renaming the method?
Assignee | ||
Comment 133•6 years ago
|
||
(In reply to Tom Ritter [:tjr] from comment #132) > I have seen the GetCurrentTime issue before, transiently, and then it went > away. My best guess is that it's a Unified build issue. Yeah, I think so. > I don't care too much one way or the other about how it's fixed; but maybe > the most permanent solution is renaming the method? Renaming sounds a good way, but in the animation case it will be awkward. The Animation object has a Web API 'attribute double? currentTime', we implement it in C++ as 'GetCurrentTimeAsDouble'. The Animation class also has 'Nullable<TimeDuration> GetCurrentTime()' for internal use in C++. Unfortunately X11 defines CurrentTime as well [1], so we have to name the function GetCurrentTimeAsTimeDuration or something. It sounds bit long... Oh, I think GetCurrentTimeDuration might be better. Thanks for the suggestion! The renaming will work for me. :) [1] https://hg.mozilla.org/mozilla-central/file/9c0bcf801521/dom/animation/Animation.h#l25
Assignee | ||
Comment 134•6 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #133) > Renaming sounds a good way, but in the animation case it will be awkward. > The Animation object has a Web API 'attribute double? currentTime', we > implement it in C++ as 'GetCurrentTimeAsDouble'. The Animation class also > has 'Nullable<TimeDuration> GetCurrentTime()' for internal use in C++. > Unfortunately X11 defines CurrentTime as well [1], so we have to name the > function GetCurrentTimeAsTimeDuration or something. It sounds bit long... > Oh, I think GetCurrentTimeDuration might be better. Filed bug 1433705.
Assignee | ||
Updated•6 years ago
|
Attachment #8945316 -
Flags: review?(bbirtles)
Comment 135•6 years ago
|
||
mozreview-review |
Comment on attachment 8945316 [details] Bug 1415780 - Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime. https://reviewboard.mozilla.org/r/215518/#review221884 Feels a little odd to do this inside the function definition itself. It doesn't work if pushed above the function definition?
Attachment #8945316 -
Flags: review?(bbirtles) → review+
Comment 136•6 years ago
|
||
mozreview-review |
Comment on attachment 8946008 [details] Bug 1415780 - Make AnimationEventDispatcher::SortEvents() private. https://reviewboard.mozilla.org/r/216076/#review221886
Attachment #8946008 -
Flags: review+
Assignee | ||
Comment 137•6 years ago
|
||
(In reply to Brian Birtles (:birtles) from comment #135) > Comment on attachment 8945316 [details] > Bug 1415780 - Undef GetCurrentTime in the template function that uses > Animation::GetCurrentTime. > > https://reviewboard.mozilla.org/r/215518/#review221884 > > Feels a little odd to do this inside the function definition itself. It > doesn't work if pushed above the function definition? I haven't try it but if it worked, the build error on mingw didn't happen I guess.
Comment 138•6 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cd9bab66f1f4 Undef GetCurrentTime in the template function that uses Animation::GetCurrentTime. r=birtles https://hg.mozilla.org/integration/autoland/rev/59606dd5c52a Don't count all observers for nsRefreshDriver. r=smaug https://hg.mozilla.org/integration/autoland/rev/2c769e6d110b Move AnimationCollection::PseudoTypeAsString into nsCSSPseudoElements. r=birtles https://hg.mozilla.org/integration/autoland/rev/346c95ffa745 Drop the comment for nsAnimationManager::DispatchEvents(). r=birtles https://hg.mozilla.org/integration/autoland/rev/5a4068a1cc40 Rename DelayedEventsDispatcher to AnimationEventDispatcher. r=birtles https://hg.mozilla.org/integration/autoland/rev/ca2c833ff617 Split AnimationEventDipatcher into an independent file. r=birtles https://hg.mozilla.org/integration/autoland/rev/3e89823dbf46 De-templatize AnimationEventDispatcher. r=birtles,masayuki https://hg.mozilla.org/integration/autoland/rev/71e7e9e19c25 Make AnimationEventDispatcher refcountable. r=birtles https://hg.mozilla.org/integration/autoland/rev/47160fce2944 Let nsPresContext have AnimationEventDispatcher. r=birtles https://hg.mozilla.org/integration/autoland/rev/cebc1a9e1770 Make AnimationEventDispatcher::SortEvents() private. r=birtles https://hg.mozilla.org/integration/autoland/rev/fbb98433e050 Let AnimationEventDispatcher observe nsRefreshDriver. r=birtles https://hg.mozilla.org/integration/autoland/rev/600af8b23ec1 Call UpdateTiming() prior to remove the animtion from the timeline in Animation::CancelNoUpdate. r=birtles https://hg.mozilla.org/integration/autoland/rev/7e8c97401cb2 Drop CollectDocuments in nsRefreshDriver.cpp. r=hiro
Comment 139•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd9bab66f1f4 https://hg.mozilla.org/mozilla-central/rev/59606dd5c52a https://hg.mozilla.org/mozilla-central/rev/2c769e6d110b https://hg.mozilla.org/mozilla-central/rev/346c95ffa745 https://hg.mozilla.org/mozilla-central/rev/5a4068a1cc40 https://hg.mozilla.org/mozilla-central/rev/ca2c833ff617 https://hg.mozilla.org/mozilla-central/rev/3e89823dbf46 https://hg.mozilla.org/mozilla-central/rev/71e7e9e19c25 https://hg.mozilla.org/mozilla-central/rev/47160fce2944 https://hg.mozilla.org/mozilla-central/rev/cebc1a9e1770 https://hg.mozilla.org/mozilla-central/rev/fbb98433e050 https://hg.mozilla.org/mozilla-central/rev/600af8b23ec1 https://hg.mozilla.org/mozilla-central/rev/7e8c97401cb2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Updated•6 years ago
|
status-firefox59:
--- → wontfix
You need to log in
before you can comment on or make changes to this bug.
Description
•