Fix tests to avoid page scale reset when setting page scale.

Fix tests to avoid page scale reset when setting page scale.

This CL fixes an error that breaks layout tests when an default override scale is specified for desk top. (See https://chromiumcodereview.appspot.com/17830003/). It also combines eventSender.scalePageBy and internals.setPageScaleFactor into a single call-location, eventSender.setPageScaleFactor.

BUG=162482
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155784

[wjmaclean, 2013-07-02 16:04:28]

aelias@ - can you please take a look and see if this is reasonable? This is
based on my investigations about why the 1-line reset scale cl was breaking
layout tests.

Also, can you suggest someone to do the owner's review (assuming you cannot)?

[wjmaclean, 2013-07-02 17:14:59]

Found that the other failing tests seem to directly set the page scale factor
without going through WebViewImpl, which bypasses the cancelling of the page
scale reset, so these should probably be added to this CL.

I'm not sure if there may be more affected tests ... will try to get this cl
plus https://chromiumcodereview.appspot.com/17830003/ running on the bots
together to see what else, if anything, is affected.

[wjmaclean, 2013-07-02 18:46:14]

After running layout tests locally (on Linux) came up with three more tests that
fail ... fixed these as well.

[jochen (gone - plz use gerrit), 2013-07-02 19:03:55]

https://codereview.chromium.org/18546003/diff/7001/LayoutTests/fast/events/sc...
File LayoutTests/fast/events/scale-and-scroll-iframe-body.html (left):

https://codereview.chromium.org/18546003/diff/7001/LayoutTests/fast/events/sc...
LayoutTests/fast/events/scale-and-scroll-iframe-body.html:28:
window.internals.setPageScaleFactor(scaleFactor, scaleOffset, scaleOffset);
should we delete window.internals.setPageScaleFactor entirely?

[wjmaclean, 2013-07-02 19:06:03]

On 2013/07/02 19:03:55, jochen wrote:
>
https://codereview.chromium.org/18546003/diff/7001/LayoutTests/fast/events/sc...
> File LayoutTests/fast/events/scale-and-scroll-iframe-body.html (left):
> 
>
https://codereview.chromium.org/18546003/diff/7001/LayoutTests/fast/events/sc...
> LayoutTests/fast/events/scale-and-scroll-iframe-body.html:28:
> window.internals.setPageScaleFactor(scaleFactor, scaleOffset, scaleOffset);
> should we delete window.internals.setPageScaleFactor entirely?

I wondered about that ... it would be nice to get some feedback from aelias@ on
this. I ran "git gs" and it's not an impossibly long list, so long as it doesn't
break other stuff.

[wjmaclean, 2013-07-02 19:52:50]

On 2013/07/02 19:06:03, wjmaclean wrote:
> On 2013/07/02 19:03:55, jochen wrote:
> >
>
https://codereview.chromium.org/18546003/diff/7001/LayoutTests/fast/events/sc...
> > File LayoutTests/fast/events/scale-and-scroll-iframe-body.html (left):
> > 
> >
>
https://codereview.chromium.org/18546003/diff/7001/LayoutTests/fast/events/sc...
> > LayoutTests/fast/events/scale-and-scroll-iframe-body.html:28:
> > window.internals.setPageScaleFactor(scaleFactor, scaleOffset, scaleOffset);
> > should we delete window.internals.setPageScaleFactor entirely?
> 
> I wondered about that ... it would be nice to get some feedback from aelias@
on
> this. I ran "git gs" and it's not an impossibly long list, so long as it
doesn't
> break other stuff.

There are (including this CL as it currently stands), about 66 instances of
window.internals.setPageScaleFactor left.

[aelias_OOO_until_Jul13, 2013-07-02 22:59:33]

https://codereview.chromium.org/18546003/diff/7001/LayoutTests/fast/events/sc...
File LayoutTests/fast/events/scale-and-scroll-iframe-body.html (left):

https://codereview.chromium.org/18546003/diff/7001/LayoutTests/fast/events/sc...
LayoutTests/fast/events/scale-and-scroll-iframe-body.html:28:
window.internals.setPageScaleFactor(scaleFactor, scaleOffset, scaleOffset);
On 2013/07/02 19:03:55, jochen wrote:
> should we delete window.internals.setPageScaleFactor entirely?

Yes, I think we should converge to a single page-scale-factor-setting call in
the layout tests.  There are two for some kind of historical reason.  I think
setPageScaleFactor is a clearer name than scalePageBy so I'd rather keep that
one (or if eventSender is a better place for some reason, we can rename that one
to setPageScaleFactor).

It occurs to me me that in the long run, ideally these tests would use @viewport
(although we need to have support for that first on desktop)... cc'ing bokan@ to
keep that in mind.

https://codereview.chromium.org/18546003/diff/7001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/18546003/diff/7001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:2876:
m_pageScaleConstraintsSet.setNeedsReset(false);
This is a somewhat subtle initialization-timing change to production code and I
think we should avoid this kind of thing to fix tests.  How about instead making
the test code call setInitialPageScaleOverride(-1) when it tries to set the
scale?

[wjmaclean, 2013-07-03 14:12:23]

> Yes, I think we should converge to a single page-scale-factor-setting call in
> the layout tests.  There are two for some kind of historical reason.  I think
> setPageScaleFactor is a clearer name than scalePageBy so I'd rather keep that
> one (or if eventSender is a better place for some reason, we can rename that
one
> to setPageScaleFactor).

I think eventSender seems like a better one to keep, since most of the test I
saw were already using it, and only using internals for this one function.

But, I'm willing to be convinced otherwise.

https://codereview.chromium.org/18546003/diff/7001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/18546003/diff/7001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:2876:
m_pageScaleConstraintsSet.setNeedsReset(false);
On 2013/07/02 22:59:33, aelias wrote:
> This is a somewhat subtle initialization-timing change to production code and
I
> think we should avoid this kind of thing to fix tests.  How about instead
making
> the test code call setInitialPageScaleOverride(-1) when it tries to set the
> scale?

OK, but are we sure there's no way this "subtle timing" issue can ever bite us
in production web sites? If so, then I'm fine with this approach.

[wjmaclean, 2013-07-05 14:03:23]

I've used the approach suggested by aelias@, and fixed the problematic tests by
suppressing the default page scale by calling
eventSender.setInitialPageScaleOverride

I've also rename eventSender.scalePageBy to be eventSender.setPageScaleFactor,
and remove internals.setPageScaleFactor.

I felt it was better to keep the eventSender version, since it invokes
additional machinery (in the WebView) that should be part of any scaling tests
we run.

I suspect this CL will have a short shelf life as it touches a lot of files, and
I leave on vacation after today, so if it looks OK (i.e. is LGTM-able), can some
please check the commit checkbox?

After this lands, it should be possible to re-land
https://codereview.chromium.org/14643011/ .

[aelias_OOO_until_Jul13, 2013-07-08 00:53:20]

https://codereview.chromium.org/18546003/diff/7001/Source/WebKit/chromium/src...
File Source/WebKit/chromium/src/WebViewImpl.cpp (right):

https://codereview.chromium.org/18546003/diff/7001/Source/WebKit/chromium/src...
Source/WebKit/chromium/src/WebViewImpl.cpp:2876:
m_pageScaleConstraintsSet.setNeedsReset(false);
On 2013/07/03 14:12:23, wjmaclean wrote:
> OK, but are we sure there's no way this "subtle timing" issue can ever bite us
> in production web sites? If so, then I'm fine with this approach.

I don't currently know of a bug here, but it's possible one may be discovered in
the future.  If that happens, it should be tested by a C++ unit test anyway.  I
think the layout tests should all pass regardless of the timing details.

https://codereview.chromium.org/18546003/diff/16001/Tools/DumpRenderTree/chro...
File Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.h (right):

https://codereview.chromium.org/18546003/diff/16001/Tools/DumpRenderTree/chro...
Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.h:89: void
setInitialPageScaleOverride(const CppArgumentList&, CppVariant*);
Instead of adding this new test-facing API, could you just make
EventSender::setPageScaleFactor call WebViewImpl::setInitialPageScaleOverride? 
The idea is that when a test explicitly sets the page scale factor, it should
stay there.

[wjmaclean, 2013-08-06 06:38:00]

This new cl addresses all outstanding comments (I think!).

https://codereview.chromium.org/18546003/diff/16001/Tools/DumpRenderTree/chro...
File Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.h (right):

https://codereview.chromium.org/18546003/diff/16001/Tools/DumpRenderTree/chro...
Tools/DumpRenderTree/chromium/TestRunner/src/EventSender.h:89: void
setInitialPageScaleOverride(const CppArgumentList&, CppVariant*);
On 2013/07/08 00:53:20, aelias wrote:
> Instead of adding this new test-facing API, could you just make
> EventSender::setPageScaleFactor call WebViewImpl::setInitialPageScaleOverride?

> The idea is that when a test explicitly sets the page scale factor, it should
> stay there.

Done.

[aelias_OOO_until_Jul13, 2013-08-06 17:17:45]

https://codereview.chromium.org/18546003/diff/22001/Source/testing/runner/Eve...
File Source/testing/runner/EventSender.cpp (right):

https://codereview.chromium.org/18546003/diff/22001/Source/testing/runner/Eve...
Source/testing/runner/EventSender.cpp:788:
webview()->setInitialPageScaleOverride(-1);
Could you change this to webview()->setPageScaleFactorLimits(scaleFactor,
scaleFactor)?

The reason is that when viewport tag support is enabled for desktop,
"initialScale = -1" will mean "initialize page scale to minimum fitting scale". 
The only reason that isn't already the case is to work around yet more layout
test problems that need cleaning up.

[wjmaclean, 2013-08-07 00:41:11]

PTAL

https://codereview.chromium.org/18546003/diff/22001/Source/testing/runner/Eve...
File Source/testing/runner/EventSender.cpp (right):

https://codereview.chromium.org/18546003/diff/22001/Source/testing/runner/Eve...
Source/testing/runner/EventSender.cpp:788:
webview()->setInitialPageScaleOverride(-1);
On 2013/08/06 17:17:46, aelias wrote:
> Could you change this to webview()->setPageScaleFactorLimits(scaleFactor,
> scaleFactor)?
> 
> The reason is that when viewport tag support is enabled for desktop,
> "initialScale = -1" will mean "initialize page scale to minimum fitting
scale". 
> The only reason that isn't already the case is to work around yet more layout
> test problems that need cleaning up.

Done.

[aelias_OOO_until_Jul13, 2013-08-07 00:45:20]

lgtm, thanks.  jochen@, could you review for OWNERS?

[jochen (gone - plz use gerrit), 2013-08-07 02:09:59]

lgtm

[commit-bot: I haz the power, 2013-08-08 12:35:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/18546003/28001

[commit-bot: I haz the power, 2013-08-08 20:46:59]

Change committed as 155784