Bug 94828 – [chromium] Removing order dependency from setters

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

hostImpl->setViewportSize is using data set in hostImpl->setDeviceScaleFactor and hostImpl->setPageScaleFactorAndLimits but is called before those two functions.

Should be a one line change. Something else corrects for this behavior right now, but we shouldn't rely on it and fix this.

Created attachment 160720 [details]
Patch

[Dana Jansens]

Comment on attachment 160720 [details]
Patch

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

Can you please add a test that demonstrates this was wrong before and right now?

> Source/WebCore/ChangeLog:8
> +        Making sure setters are called in the right order. 

Maybe explain the ordering here.

[WebKit Review Bot]

Comment on attachment 160720 [details]
Patch

Attachment 160720 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13610307

New failing tests:
animations/suspend-resume-animation-events.html

[WebKit Review Bot]

Created attachment 160727 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[James Robinson]

Comment on attachment 160720 [details]
Patch

Definitely need a test.  What is the "something else" you allude to that corrects for this?

(In reply to comment #5)
> (From update of attachment 160720 [details])
> Definitely need a test.  What is the "something else" you allude to that corrects for this?

Writing tests now.

As for something else, updateMaxScrollPosition() which is the function dependent on call order is also invoked in CCLayerTreeHostImpl::commitComplete() once all the values are set.

[James Robinson]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 160720 [details] [details])
> > Definitely need a test.  What is the "something else" you allude to that corrects for this?
> 
> Writing tests now.
> 
> As for something else, updateMaxScrollPosition() which is the function dependent on call order is also invoked in CCLayerTreeHostImpl::commitComplete() once all the values are set.

Can we instead fix this so the order doesn't matter?  It seems like that is practically the case today - we make a bunch of sets and the only use the data after we're done with that.  Relying on a specific call order is a bit suspicious.

> Can we instead fix this so the order doesn't matter?  It seems like that is practically the case today - we make a bunch of sets and the only use the data after we're done with that.  Relying on a specific call order is a bit suspicious.

Well, it is actually safe today because of the additional call inside commitFinished() that flushes any order inconsistencies.

updateMaxScrollPosition ends up getting called multiple times in setters that affect its state, which seems reasonable, and then again at the very end. I think, one of the bugs here is that it *isn't* called inside setDeviceScaleFactor, which alters max scroll position.

A batch of these setters are called in finishCommitOnImplThread, but another place that affects it is page scale animation. It seems like removing this call from setters and only updating max scroll positions at the end is insufficient, unless we change scale animation to do it manually (and do it this way in every new function that dirties its state)

My particular rearrangement is a nitpick that threw me off during debugging. I was unfamiliar with the code and expected the different device scale than I was seeing, and only later discovered that's it's okay and that it's fixed in the later part of the code. Still, just looking at setViewport at the time, it was unclear that it was not a bug.

Created attachment 161279 [details]
Patch

[Adrienne Walker]

Comment on attachment 161279 [details]
Patch

R=me.  Thanks for the unit test.  :)

[WebKit Review Bot]

Comment on attachment 161279 [details]
Patch

Clearing flags on attachment: 161279

Committed r127081: <http://trac.webkit.org/changeset/127081>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.