...
Created attachment 423757 [details] Patch
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.
I wonder if this is not equivalent to the client calling endAnimatedResize from a doAfterNextPresentationUpdate:^{} block?? It seems like it might be.
(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.
(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.
(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?
(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.
It seems we can fix the issue with doAfterNextPresentationUpdate like Tim said, so we don't need the SPI!
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.
(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.
Created attachment 423957 [details] Patch
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)
(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 #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.
(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.
(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.
Created attachment 424053 [details] Patch
Comment on attachment 424053 [details] Patch Given the results of Sihui's experiments, let's give it a shot.
<rdar://problem/75788772>
Committed r274941: <https://commits.webkit.org/r274941> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424053 [details].