Bug 94828 - [chromium] Removing order dependency from setters
Summary: [chromium] Removing order dependency from setters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-23 11:06 PDT by alexst
Modified: 2012-08-29 19:02 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.77 KB, patch)
2012-08-27 08:03 PDT, alexst
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-04 (599.70 KB, application/zip)
2012-08-27 08:40 PDT, WebKit Review Bot
no flags Details
Patch (3.60 KB, patch)
2012-08-29 11:56 PDT, alexst
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description alexst 2012-08-23 11:06:30 PDT
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.
Comment 1 alexst 2012-08-27 08:03:40 PDT
Created attachment 160720 [details]
Patch
Comment 2 Dana Jansens 2012-08-27 08:09:31 PDT
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.
Comment 3 WebKit Review Bot 2012-08-27 08:40:52 PDT
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
Comment 4 WebKit Review Bot 2012-08-27 08:40:54 PDT
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
Comment 5 James Robinson 2012-08-27 11:12:01 PDT
Comment on attachment 160720 [details]
Patch

Definitely need a test.  What is the "something else" you allude to that corrects for this?
Comment 6 alexst 2012-08-27 11:46:42 PDT
(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.
Comment 7 James Robinson 2012-08-27 15:59:48 PDT
(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.
Comment 8 alexst 2012-08-28 06:32:06 PDT
> 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.
Comment 9 alexst 2012-08-29 11:56:32 PDT
Created attachment 161279 [details]
Patch
Comment 10 Adrienne Walker 2012-08-29 12:03:20 PDT
Comment on attachment 161279 [details]
Patch

R=me.  Thanks for the unit test.  :)
Comment 11 WebKit Review Bot 2012-08-29 19:02:15 PDT
Comment on attachment 161279 [details]
Patch

Clearing flags on attachment: 161279

Committed r127081: <http://trac.webkit.org/changeset/127081>
Comment 12 WebKit Review Bot 2012-08-29 19:02:19 PDT
All reviewed patches have been landed.  Closing bug.