Bug 223530 – Ignore middle commits during animated resize

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Sihui Liu]

...

[Sihui Liu]

Created attachment 423757 [details]
Patch

[Sam Weinig]

It seems like adding SPI for this would only help Safari? Can we instead infer we are in this state somehow instead? Or if really can’t, seems like it should probably be API instead.

[Tim Horton]

I wonder if this is not equivalent to the client calling endAnimatedResize from a doAfterNextPresentationUpdate:^{} block?? It seems like it might be.

[Tim Horton]

(In reply to Sam Weinig from comment #2)
> It seems like adding SPI for this would only help Safari? Can we instead
> infer we are in this state somehow instead? Or if really can’t, seems like
> it should probably be API instead.

Animated resize is SPI, so this doesn't make sense.

[Sihui Liu]

(In reply to Tim Horton from comment #3)
> I wonder if this is not equivalent to the client calling endAnimatedResize
> from a doAfterNextPresentationUpdate:^{} block?? It seems like it might be.

It may be the same. I thought about delaying endAnimatedResize but don't know how much we are going to delay. Let me test if doAfterNextPresentationUpdate:^{} fixes the problem.

[Sam Weinig]

(In reply to Tim Horton from comment #4)
> (In reply to Sam Weinig from comment #2)
> > It seems like adding SPI for this would only help Safari? Can we instead
> > infer we are in this state somehow instead? Or if really can’t, seems like
> > it should probably be API instead.
> 
> Animated resize is SPI, so this doesn't make sense.

Can you elaborate on what you mean by this?

[Tim Horton]

(In reply to Sam Weinig from comment #6)
> (In reply to Tim Horton from comment #4)
> > (In reply to Sam Weinig from comment #2)
> > > It seems like adding SPI for this would only help Safari? Can we instead
> > > infer we are in this state somehow instead? Or if really can’t, seems like
> > > it should probably be API instead.
> > 
> > Animated resize is SPI, so this doesn't make sense.
> 
> Can you elaborate on what you mean by this?

(I was responding to the last sentence; the second one makes sense to me; the first one is just wrong, this isn’t about Safari, but other SPI clients). Animated resize is SPI so it doesn’t make sense to make a mechanism to control a detail of it API?

It’s possible you meant that the whole animated resize SPI should become API (and I vaguely agree), but as it stands the SPI is poorly designed, confusing, and easy to misuse, so that’s a much bigger project.

[Sihui Liu]

It seems we can fix the issue with doAfterNextPresentationUpdate like Tim said, so we don't need the SPI!

[Sihui Liu]

Nope, _doAfterNextPresentationUpdate will invoke callback after ongoing animated resize (see - (void)_internalDoAfterNextPresentationUpdate:(void (^)(void))updateBlock withoutWaitingForPainting:(BOOL)withoutWaitingForPainting withoutWaitingForAnimatedResize:(BOOL)withoutWaitingForAnimatedResize), so if we put _endAnimatedResize in the callback the animated resize will never be ended.

[Tim Horton]

(In reply to Sihui Liu from comment #9)
> Nope, _doAfterNextPresentationUpdate will invoke callback after ongoing
> animated resize (see - (void)_internalDoAfterNextPresentationUpdate:(void
> (^)(void))updateBlock
> withoutWaitingForPainting:(BOOL)withoutWaitingForPainting
> withoutWaitingForAnimatedResize:(BOOL)withoutWaitingForAnimatedResize), so
> if we put _endAnimatedResize in the callback the animated resize will never
> be ended.

Oh, geez.

[Sihui Liu]

Created attachment 423957 [details]
Patch

[Tim Horton]

Comment on attachment 423957 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423957&action=review

> Source/WebKit/ChangeLog:16
> +        _waitingForEndAnimatedResize. In this case, let's just ignore middle commits and remove corresponding code.

>Safari, also doesn't hit the adative scaling path because it always early returns at _waitingForEndAnimatedResize

Doesn't this just mean that you haven't won the race in your testing?? (receiving a commit /after/ Safari calls -endAnimatedResize)

[Tim Horton]

(In reply to Tim Horton from comment #12)
> Comment on attachment 423957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423957&action=review
> 
> > Source/WebKit/ChangeLog:16
> > +        _waitingForEndAnimatedResize. In this case, let's just ignore middle commits and remove corresponding code.
> 
> >Safari, also doesn't hit the adative scaling path because it always early returns at _waitingForEndAnimatedResize
> 
> Doesn't this just mean that you haven't won the race in your testing??
> (receiving a commit /after/ Safari calls -endAnimatedResize)

(you could probably win the race by either using slower hardware or putting a short sleep in `RemoteLayerTreeDrawingArea::BackingStoreFlusher::flush` before the call to `sendMessage`)

[Sihui Liu]

(In reply to Tim Horton from comment #13)
> (In reply to Tim Horton from comment #12)
> > Comment on attachment 423957 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=423957&action=review
> > 
> > > Source/WebKit/ChangeLog:16
> > > +        _waitingForEndAnimatedResize. In this case, let's just ignore middle commits and remove corresponding code.
> > 
> > >Safari, also doesn't hit the adative scaling path because it always early returns at _waitingForEndAnimatedResize
> > 
> > Doesn't this just mean that you haven't won the race in your testing??
> > (receiving a commit /after/ Safari calls -endAnimatedResize)
> 
> (you could probably win the race by either using slower hardware or putting
> a short sleep in `RemoteLayerTreeDrawingArea::BackingStoreFlusher::flush`
> before the call to `sendMessage`)

(In reply to Tim Horton from comment #12)
> Comment on attachment 423957 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423957&action=review
> 
> > Source/WebKit/ChangeLog:16
> > +        _waitingForEndAnimatedResize. In this case, let's just ignore middle commits and remove corresponding code.
> 
> >Safari, also doesn't hit the adative scaling path because it always early returns at _waitingForEndAnimatedResize
> 
> Doesn't this just mean that you haven't won the race in your testing??
> (receiving a commit /after/ Safari calls -endAnimatedResize)

Yes, I don't see commits arrive after Safari calls endAnimatedResize during testing, so I am not sure whether that can happen in a real-world use case.

Will try sleep in RemoteLayerTreeDrawingArea::BackingStoreFlusher::flush() a bit.

[Tim Horton]

(In reply to Sihui Liu from comment #14)
> (In reply to Tim Horton from comment #13)
> > (In reply to Tim Horton from comment #12)
> > > Comment on attachment 423957 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=423957&action=review
> > > 
> > > > Source/WebKit/ChangeLog:16
> > > > +        _waitingForEndAnimatedResize. In this case, let's just ignore middle commits and remove corresponding code.
> > > 
> > > >Safari, also doesn't hit the adative scaling path because it always early returns at _waitingForEndAnimatedResize
> > > 
> > > Doesn't this just mean that you haven't won the race in your testing??
> > > (receiving a commit /after/ Safari calls -endAnimatedResize)
> > 
> > (you could probably win the race by either using slower hardware or putting
> > a short sleep in `RemoteLayerTreeDrawingArea::BackingStoreFlusher::flush`
> > before the call to `sendMessage`)
> 
> (In reply to Tim Horton from comment #12)
> > Comment on attachment 423957 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=423957&action=review
> > 
> > > Source/WebKit/ChangeLog:16
> > > +        _waitingForEndAnimatedResize. In this case, let's just ignore middle commits and remove corresponding code.
> > 
> > >Safari, also doesn't hit the adative scaling path because it always early returns at _waitingForEndAnimatedResize
> > 
> > Doesn't this just mean that you haven't won the race in your testing??
> > (receiving a commit /after/ Safari calls -endAnimatedResize)
> 
> Yes, I don't see commits arrive after Safari calls endAnimatedResize during
> testing, so I am not sure whether that can happen in a real-world use case.

There is no connection between the two (a commit can be arbitrarily delayed in the WP, on a secondary queue, which is what you're going to test), so it should /totally/ be possible.

[Sihui Liu]

(In reply to Tim Horton from comment #15)
> (In reply to Sihui Liu from comment #14)
> > (In reply to Tim Horton from comment #13)
> > > (In reply to Tim Horton from comment #12)
> > > > Comment on attachment 423957 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=423957&action=review
> > > > 
> > > > > Source/WebKit/ChangeLog:16
> > > > > +        _waitingForEndAnimatedResize. In this case, let's just ignore middle commits and remove corresponding code.
> > > > 
> > > > >Safari, also doesn't hit the adative scaling path because it always early returns at _waitingForEndAnimatedResize
> > > > 
> > > > Doesn't this just mean that you haven't won the race in your testing??
> > > > (receiving a commit /after/ Safari calls -endAnimatedResize)
> > > 
> > > (you could probably win the race by either using slower hardware or putting
> > > a short sleep in `RemoteLayerTreeDrawingArea::BackingStoreFlusher::flush`
> > > before the call to `sendMessage`)
> > 
> > (In reply to Tim Horton from comment #12)
> > > Comment on attachment 423957 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=423957&action=review
> > > 
> > > > Source/WebKit/ChangeLog:16
> > > > +        _waitingForEndAnimatedResize. In this case, let's just ignore middle commits and remove corresponding code.
> > > 
> > > >Safari, also doesn't hit the adative scaling path because it always early returns at _waitingForEndAnimatedResize
> > > 
> > > Doesn't this just mean that you haven't won the race in your testing??
> > > (receiving a commit /after/ Safari calls -endAnimatedResize)
> > 
> > Yes, I don't see commits arrive after Safari calls endAnimatedResize during
> > testing, so I am not sure whether that can happen in a real-world use case.
> 
> There is no connection between the two (a commit can be arbitrarily delayed
> in the WP, on a secondary queue, which is what you're going to test), so it
> should /totally/ be possible.

I see. With 1s sleep in RemoteLayerTreeDrawingArea::BackingStoreFlusher::flush() I can reproduce the case where middle commits (commits before animated size update and arrive in UI process after beginAnimatedResize) arrive after endAnimatedResize. And it seems removing the code is the correct behavior in Safari, otherwise we can stretch too much.

[Sihui Liu]

Created attachment 424053 [details]
Patch

[Tim Horton]

Comment on attachment 424053 [details]
Patch

Given the results of Sihui's experiments, let's give it a shot.

[Radar WebKit Bug Importer]

<rdar://problem/75788772>

[EWS]

Committed r274941: <https://commits.webkit.org/r274941>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424053 [details].