Remove plumbing for inert-visual-viewport flag

Remove plumbing for the inert-visual-viewport flag.

Note that the Blink setting inertVisualViewport still exists and is set to false by default.
A follow up CL will set this to true once dependent tests are fixed.

BUG=489206
Committed: https://crrev.com/58d42ae1d6690a9f625e23f160b346e9cebeab01
Cr-Commit-Position: refs/heads/master@{#355907}

[ymalik, 2015-10-18 20:03:10]

ymalik@chromium.org changed reviewers:
+ bokan@chromium.org

[bokan, 2015-10-19 15:01:15]

Comments inline

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html
(right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:37:
shouldBe('window.innerWidth', '1600');
Can you add a comment here (or above at the setPageScaleFactorLimits call maybe)
that the 0.5 minimum scale causes the layout viewport to become twice what we'd
normally expect because the layout viewport is based on minimum page scale.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:62:
shouldBe('window.scrollY', '300');
You can get rid of most of these scrollBy calls since we needed them before to
indirectly cause scrolling of one or both viewports. One or two scrolls (perhaps
one to some (x,y) and one past the edge, to make sure we don't bubble any)
should be enough now. Also, now that we have explicit methods on Internals to
check the visual viewport offset, check that it doesn't change because of these
scrollTo/By calls. Ditto for the below functions as well.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:105:
// First scroll should only scroll the visual viewport because the
None of the scrolls should be going to the visual viewport anymore, right? Is it
just the comment that's out of date?

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:123:
window.scrollBy(scrollAmount, 0);
scrollBy won't ever scroll the inner viewport. This function is testing that
scroll events are properly delivered. I don't think we ever explicitly talked
about this, but I think the right thing now would be to only send onScrolls for
layout viewport scrolling, and not for visual.

In that case, we still want to make sure that's the case so you'll probably want
to use the event sender to send wheel/touch events that cause scrolling. That'll
ensure that onScroll isn't triggered for visual scrolls.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/dom/window-inner-size-scaling.html
(left):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/window-inner-size-scaling.html:4:
description("This test ensures window.innerWidth/innerHeight return the size of
the visual viewport in CSS pixels.");
It's fine to remove this test but lets add a check to the
window-scaled-viewport-properties test that the innerWidth and Height don't
change.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/events/gesture-pinch-zoom-scroll-bubble.html
(right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/events/gesture-pinch-zoom-scroll-bubble.html:30:
// result up to the pinch virtual viewport, where it will change the pinch
While you're here, please change "pinch virtual viewport" -> "visual viewport"

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/repaint/fixed-right-bottom-in-page-scale.html
(right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/repaint/fixed-right-bottom-in-page-scale.html:33:
<div style="position: fixed; bottom: 400px; right: 500px; z-index: 1">
Why did this change?

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/elements/highlight/highlight-node-scaled-and-scrolled.html
(right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/elements/highlight/highlight-node-scaled-and-scrolled.html:29:
internals.setVisualViewportOffset(1000, 1000);
This doesn't bubble like window.scrollTo does but there's no layout viewport
scrolling which is why this works. Please add a comment that we're just fully
scrolling the visual viewport.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.h:373: // The scroll position
of the visual viewport relative to scroll origin of
I think it's clearer to just say "relative to document origin"

[ymalik, 2015-10-20 21:27:14]

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html
(right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:37:
shouldBe('window.innerWidth', '1600');
On 2015/10/19 15:01:14, bokan wrote:
> Can you add a comment here (or above at the setPageScaleFactorLimits call
maybe)
> that the 0.5 minimum scale causes the layout viewport to become twice what
we'd
> normally expect because the layout viewport is based on minimum page scale.

Done.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:62:
shouldBe('window.scrollY', '300');
On 2015/10/19 15:01:14, bokan wrote:
> You can get rid of most of these scrollBy calls since we needed them before to
> indirectly cause scrolling of one or both viewports. One or two scrolls
(perhaps
> one to some (x,y) and one past the edge, to make sure we don't bubble any)
> should be enough now. Also, now that we have explicit methods on Internals to
> check the visual viewport offset, check that it doesn't change because of
these
> scrollTo/By calls. Ditto for the below functions as well.

Done.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/dom/window-inner-size-scaling.html
(left):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/window-inner-size-scaling.html:4:
description("This test ensures window.innerWidth/innerHeight return the size of
the visual viewport in CSS pixels.");
On 2015/10/19 15:01:14, bokan wrote:
> It's fine to remove this test but lets add a check to the
> window-scaled-viewport-properties test that the innerWidth and Height don't
> change.

Acknowledged. I believe that window-scaled-viewport-properties.html already
checks that initially in testPinchedIn()

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/repaint/fixed-right-bottom-in-page-scale.html
(right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/repaint/fixed-right-bottom-in-page-scale.html:33:
<div style="position: fixed; bottom: 400px; right: 500px; z-index: 1">
On 2015/10/19 15:01:14, bokan wrote:
> Why did this change?

The call to window.scrollTo on Line 14 would previously scroll the visual
viewport to the bottom right position, where the div with "background:black"
would be visible. Because window.scrollTo now only scrolls the layout viewport,
the test expectations (seeing a black box in the bottom right corner) were no
longer met. I just moved the position of the div so that when you scale and
scroll, the black box div is still in the bottom right.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/inspector/elements/highlight/highlight-node-scaled-and-scrolled.html
(right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/inspector/elements/highlight/highlight-node-scaled-and-scrolled.html:29:
internals.setVisualViewportOffset(1000, 1000);
On 2015/10/19 15:01:14, bokan wrote:
> This doesn't bubble like window.scrollTo does but there's no layout viewport
> scrolling which is why this works. Please add a comment that we're just fully
> scrolling the visual viewport.

Done.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/testing/Internals.h:373: // The scroll position
of the visual viewport relative to scroll origin of
On 2015/10/19 15:01:14, bokan wrote:
> I think it's clearer to just say "relative to document origin"

Done.

[ymalik, 2015-10-20 23:36:00]

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html
(right):

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:62:
shouldBe('window.scrollY', '300');
On 2015/10/20 21:27:13, ymalik1 wrote:
> On 2015/10/19 15:01:14, bokan wrote:
> > You can get rid of most of these scrollBy calls since we needed them before
to
> > indirectly cause scrolling of one or both viewports. One or two scrolls
> (perhaps
> > one to some (x,y) and one past the edge, to make sure we don't bubble any)
> > should be enough now. Also, now that we have explicit methods on Internals
to
> > check the visual viewport offset, check that it doesn't change because of
> these
> > scrollTo/By calls. Ditto for the below functions as well.
> 
> Done.

To be more clear about the change. I refactored the scrolls to testScrolls.
testScrolls scrolls to an (x, y) and past the edge.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:105:
// First scroll should only scroll the visual viewport because the
On 2015/10/19 15:01:14, bokan wrote:
> None of the scrolls should be going to the visual viewport anymore, right? Is
it
> just the comment that's out of date?

Yes, fixed the comment.

https://codereview.chromium.org/1415513002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:123:
window.scrollBy(scrollAmount, 0);
On 2015/10/19 15:01:14, bokan wrote:
> scrollBy won't ever scroll the inner viewport. This function is testing that
> scroll events are properly delivered. I don't think we ever explicitly talked
> about this, but I think the right thing now would be to only send onScrolls
for
> layout viewport scrolling, and not for visual.
> 
> In that case, we still want to make sure that's the case so you'll probably
want
> to use the event sender to send wheel/touch events that cause scrolling.
That'll
> ensure that onScroll isn't triggered for visual scrolls.

Yes, this test doesn't cover the case when we actually scroll the visual
viewport. Added a new test, see visual-viewport-scroll-no-onscroll-event.html
below.

[bokan, 2015-10-21 13:32:15]

Looks good, just a few more cleanups (mostly to my old code).

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html
(right):

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:39:
debug('===Unscaled===');
This is misleading since it's actually at 0.5 scale - can you change to "Initial
Scale" or something of that nature.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:143:
testFailed("Received visual viewport scroll event");
The failure message is wrong (and success message below). If we failed here it's
not because the visual viewport scrolled. It's because attempting to scroll the
layout viewport (which can't be scrolled more) caused an onScroll.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:155:
window.scrollBy(scrolls[0], 0);
On second thought, this has nothing to do with page scaling so I think it makes
sense to remove this whole onScroll test in this file (I think this
functionality is tested elsewhere anyway). Your new
visual-viewport-scroll-no-onscroll-event.html is testing the essential bits
under page scale.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:1:
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
Please change the doctype on any of these tests to:
<!DOCTYPE html>

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:10:
var shouldTrigger;
unused.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:61:
} else if (numRAFCalls == scrolls.length && numScrollsReceived < numRAFCalls) {
I know I wrote this, but this is incredibly hard to read now and overly
complicated. I'd replace this with something like:

if (numRAFCalls == 0) {
  // send first scroll
} else if (numRAFCalls == 1) {
  // check numScrollsReceived
  // send second scroll
} else {
  // check numScrollsReceived and end test
}

and make the onscroll handler just increment numScrollsReceived.

[ymalik, 2015-10-21 15:14:08]

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html
(right):

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:39:
debug('===Unscaled===');
On 2015/10/21 13:32:14, bokan wrote:
> This is misleading since it's actually at 0.5 scale - can you change to
"Initial
> Scale" or something of that nature.

Done.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:143:
testFailed("Received visual viewport scroll event");
On 2015/10/21 13:32:14, bokan wrote:
> The failure message is wrong (and success message below). If we failed here
it's
> not because the visual viewport scrolled. It's because attempting to scroll
the
> layout viewport (which can't be scrolled more) caused an onScroll.

Done.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:155:
window.scrollBy(scrolls[0], 0);
On 2015/10/21 13:32:14, bokan wrote:
> On second thought, this has nothing to do with page scaling so I think it
makes
> sense to remove this whole onScroll test in this file (I think this
> functionality is tested elsewhere anyway). Your new
> visual-viewport-scroll-no-onscroll-event.html is testing the essential bits
> under page scale.

Removed test.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:1:
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
On 2015/10/21 13:32:15, bokan wrote:
> Please change the doctype on any of these tests to:
> <!DOCTYPE html>

Done.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:10:
var shouldTrigger;
On 2015/10/21 13:32:15, bokan wrote:
> unused.

Done.

https://codereview.chromium.org/1415513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:61:
} else if (numRAFCalls == scrolls.length && numScrollsReceived < numRAFCalls) {
On 2015/10/21 13:32:15, bokan wrote:
> I know I wrote this, but this is incredibly hard to read now and overly
> complicated. I'd replace this with something like:
> 
> if (numRAFCalls == 0) {
>   // send first scroll
> } else if (numRAFCalls == 1) {
>   // check numScrollsReceived
>   // send second scroll
> } else {
>   // check numScrollsReceived and end test
> }
> 
> and make the onscroll handler just increment numScrollsReceived.

Done. Thanks for simplifying it.

[bokan, 2015-10-21 15:23:07]

Sorry for nit picking, last round I think :)

Could you also add a link to the intent-to-ship in the description.

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html
(right):

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:38:
function testUnscaled () {
Nit: Function name too :)

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:34:
if (numRAFCalls == 0)
You can remove this too now.

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:47:
if (numScrollsReceived < numRAFCalls) {
We know there should be exactly one scroll here, I'd just replace this with
shouldBe('numScrollsReceived', '1')

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:57:
if (numScrollsReceived >= numRAFCalls) {
Ditto here.

[ymalik, 2015-10-21 16:29:07]

Description was changed from

==========
Make window.scroll properties relative to the layout viewport by default.

This CL removes the inert-visual-viewport flag and fixes the LayoutTests that
depend on the window.scroll properties being relative to the visual viewport.
The following APIs in the CSSOM spec are affected: scrollTo, scrollBy,
innerWidth, innerHeight, scrollX, scrollY, element.scrollTop,
element.scrollLeft.

Some notes:
- window-scaled-viewport-properties.html is the LayoutTest that tests that the
above APIs are relative to the layout viewport.

- This CL adds the following methods to Internals.cpp for tests that need the
visual viewport scroll position and dimensions: visualViewportHeight, 
visualViewportWidth, visualViewportScrollX, visualViewportScrollY


BUG=489206
==========

to

==========
Make window.scroll properties relative to the layout viewport by default.

This CL removes the inert-visual-viewport flag and fixes the LayoutTests that
depend on the window.scroll properties being relative to the visual viewport.
The following APIs in the CSSOM spec are affected: scrollTo, scrollBy,
innerWidth, innerHeight, scrollX, scrollY, element.scrollTop,
element.scrollLeft.

Some notes:
- window-scaled-viewport-properties.html is the LayoutTest that tests that the
above APIs are relative to the layout viewport.

- This CL adds the following methods to Internals.cpp for tests that need the
visual viewport scroll position and dimensions: visualViewportHeight, 
visualViewportWidth, visualViewportScrollX, visualViewportScrollY

Intent-to-ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/mt1uciFuvWQ/ecQi0f8k...

BUG=489206
==========

[ymalik, 2015-10-21 16:29:28]

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html
(right):

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/Window/window-scaled-viewport-properties.html:38:
function testUnscaled () {
On 2015/10/21 15:23:06, bokan wrote:
> Nit: Function name too :)

Oops. Totally my carelessness. Done :)

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html
(right):

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:34:
if (numRAFCalls == 0)
On 2015/10/21 15:23:06, bokan wrote:
> You can remove this too now.

Done.

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:47:
if (numScrollsReceived < numRAFCalls) {
On 2015/10/21 15:23:06, bokan wrote:
> We know there should be exactly one scroll here, I'd just replace this with
> shouldBe('numScrollsReceived', '1')

Done.

https://codereview.chromium.org/1415513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/scroll-behavior/visual-viewport-scroll-no-onscroll-event.html:57:
if (numScrollsReceived >= numRAFCalls) {
On 2015/10/21 15:23:06, bokan wrote:
> Ditto here.

Done.

[bokan, 2015-10-21 16:41:29]

lgtm, thanks!

[ymalik, 2015-10-21 17:19:22]

ymalik@chromium.org changed reviewers:
+ avi@chromium.org, isherman@chromium.org, kenrb@chromium.org,
rbyers@chromium.org

[ymalik, 2015-10-21 17:19:22]

+rbyers for source/core/testing/Internals*
+kenrb@ for common_param_traits_macros
+avi@ for content/public and content/renderer content/browser
+isherman@ for histograms.xml

[Rick Byers, 2015-10-21 17:26:40]

What's the plan for if we find this breaks some sites (eg. during beta) and need
to back it out / delay for a milestone while we try to get a site fixed?  Just
revert this entire CL?  It might be smart to keep the flag enabled by default
until it's had one milestone to bake.

https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.h:375: double
visualViewportScrollY();
would this be simpler and most consistent with other APIs here if you collapsed
all 4 APIs into a 'visualViewport()' API that returns just a ClientRect?

[kenrb, 2015-10-21 17:30:03]

ipc lgtm

[Avi (use Gerrit), 2015-10-21 17:53:58]

On 2015/10/21 17:19:22, ymalik1 wrote:
> +rbyers for source/core/testing/Internals*
> +kenrb@ for common_param_traits_macros
> +avi@ for content/public and content/renderer content/browser
> +isherman@ for histograms.xml

content lgtm

[ymalik, 2015-10-21 20:45:06]

On 2015/10/21 17:26:40, Rick Byers wrote:
> What's the plan for if we find this breaks some sites (eg. during beta) and
need
> to back it out / delay for a milestone while we try to get a site fixed?  Just
> revert this entire CL?  It might be smart to keep the flag enabled by default
> until it's had one milestone to bake.
> 
>
https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/testing/Internals.h (right):
> 
>
https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/testing/Internals.h:375: double
> visualViewportScrollY();
> would this be simpler and most consistent with other APIs here if you
collapsed
> all 4 APIs into a 'visualViewport()' API that returns just a ClientRect?

I agree that keeping the flag is a good idea so we don't have to revert the
entire CL. I removed all the other plumbing and changed the setting in
Settings.in to true by default. Does that sound reasonable?

[ymalik, 2015-10-21 21:14:17]

@bokan Not sending scroll events when scrolling the visual viewport broke
another test. I updated touch-gesture-fling-with-page-scale.html. I also kept
the inert-visual-viewport flag in case something goes wrong and we need to turn
of inert-visual viewport.

PTAL :)

https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/testing/Internals.h (right):

https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/testing/Internals.h:375: double
visualViewportScrollY();
On 2015/10/21 17:26:40, Rick Byers wrote:
> would this be simpler and most consistent with other APIs here if you
collapsed
> all 4 APIs into a 'visualViewport()' API that returns just a ClientRect?

I was originally thinking of that but thought that this is more consistent with
the original API which had window.scrollX/Y window.innerHeight/Width. I don't
feel strongly about it, but that would mean changing all my test fixes above :).

[bokan, 2015-10-21 21:15:46]

On 2015/10/21 20:45:06, ymalik1 wrote:
> On 2015/10/21 17:26:40, Rick Byers wrote:
> > What's the plan for if we find this breaks some sites (eg. during beta) and
> need
> > to back it out / delay for a milestone while we try to get a site fixed? 
Just
> > revert this entire CL?  It might be smart to keep the flag enabled by
default
> > until it's had one milestone to bake.
> > 
> >
>
https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/testing/Internals.h (right):
> > 
> >
>
https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/testing/Internals.h:375: double
> > visualViewportScrollY();
> > would this be simpler and most consistent with other APIs here if you
> collapsed
> > all 4 APIs into a 'visualViewport()' API that returns just a ClientRect?
> 
> I agree that keeping the flag is a good idea so we don't have to revert the
> entire CL. I removed all the other plumbing and changed the setting in
> Settings.in to true by default. Does that sound reasonable?

It might make it hard to tell if the issue is actually caused by the setting.
Though I suppose we could always try in stable channel by turning the flag on
and off there. I'm ok with this if Rick thinks that's enough.

[ymalik, 2015-10-21 21:18:40]

The CQ bit was checked by ymalik@chromium.org to run a CQ dry run

[bokan, 2015-10-21 21:19:30]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-21 21:19:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415513002/80001

[bokan, 2015-10-21 21:48:36]

https://codereview.chromium.org/1415513002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/touch/gesture/touch-gesture-fling-with-page-scale.html
(right):

https://codereview.chromium.org/1415513002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/gesture/touch-gesture-fling-with-page-scale.html:90:
window.scrollTo(10, 10);
I'm a bit worried that this could cause flakiness if we fire the scroll handler
before the scroll reaches one of the container boxes.

Could you instead scroll the window in reset() and adjust the startX/Y position
(or if you scroll to (1, 1) you probably don't even have to adjust the startX/Y)
so that when the container boxes finish scrolling, they'll bubble up and cause a
scroll on the window?

[commit-bot: I haz the power, 2015-10-21 21:58:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-21 21:58:05]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Rick Byers, 2015-10-21 21:58:28]

On 2015/10/21 21:14:17, ymalik1 wrote:
> @bokan Not sending scroll events when scrolling the visual viewport broke
> another test. I updated touch-gesture-fling-with-page-scale.html. I also kept
> the inert-visual-viewport flag in case something goes wrong and we need to
turn
> of inert-visual viewport.
> 
> PTAL :)
> 
>
https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/testing/Internals.h (right):
> 
>
https://codereview.chromium.org/1415513002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/testing/Internals.h:375: double
> visualViewportScrollY();
> On 2015/10/21 17:26:40, Rick Byers wrote:
> > would this be simpler and most consistent with other APIs here if you
> collapsed
> > all 4 APIs into a 'visualViewport()' API that returns just a ClientRect?
> 
> I was originally thinking of that but thought that this is more consistent
with
> the original API which had window.scrollX/Y window.innerHeight/Width. I don't
> feel strongly about it, but that would mean changing all my test fixes above
:).

Minimizing churn to tests (eg. making relatively simple search/replace work) is
a good enough reason for me to keep these split into separate APIs.

Source/core/testing/ LGTM

[Ilya Sherman, 2015-10-21 22:27:59]

https://codereview.chromium.org/1415513002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1415513002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:64546: -  <int value="-631740127"
label="inert-visual-viewport"/>
Please preserve this value -- there's no need to remove it.

[ymalik, 2015-10-21 22:38:24]

ymalik@chromium.org changed reviewers:
+ fsamuel@chromium.org

[ymalik, 2015-10-21 22:38:25]

ymalik@chromium.org changed reviewers:
+ fsamuel@chromium.org

[ymalik, 2015-10-21 22:38:25]

+fsamuel for components/html_viewer/blink_settings.cc

[ymalik, 2015-10-21 22:38:26]

+fsamuel for components/html_viewer/blink_settings.cc

[ymalik, 2015-10-21 22:38:50]

On 2015/10/21 22:27:59, Ilya Sherman wrote:
>
https://codereview.chromium.org/1415513002/diff/100001/tools/metrics/histogra...
> File tools/metrics/histograms/histograms.xml (left):
> 
>
https://codereview.chromium.org/1415513002/diff/100001/tools/metrics/histogra...
> tools/metrics/histograms/histograms.xml:64546: -  <int value="-631740127"
> label="inert-visual-viewport"/>
> Please preserve this value -- there's no need to remove it.

Done.

[ymalik, 2015-10-21 22:39:16]

The CQ bit was checked by ymalik@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-21 22:41:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415513002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415513002/140001

[Fady Samuel, 2015-10-21 22:46:03]

html_viewer lgtm

[ymalik, 2015-10-21 23:36:30]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-10-21 23:36:31]

The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org,
bokan@chromium.org, avi@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1415513002/#ps140001
(title: "undo change to histogram.xml")

[commit-bot: I haz the power, 2015-10-21 23:38:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415513002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415513002/140001

[commit-bot: I haz the power, 2015-10-22 01:34:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-22 01:34:24]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[ymalik, 2015-10-22 01:39:35]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2015-10-22 01:39:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415513002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415513002/140001

[commit-bot: I haz the power, 2015-10-22 02:54:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-22 02:54:50]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[ymalik, 2015-10-23 19:41:24]

Description was changed from

==========
Make window.scroll properties relative to the layout viewport by default.

This CL removes the inert-visual-viewport flag and fixes the LayoutTests that
depend on the window.scroll properties being relative to the visual viewport.
The following APIs in the CSSOM spec are affected: scrollTo, scrollBy,
innerWidth, innerHeight, scrollX, scrollY, element.scrollTop,
element.scrollLeft.

Some notes:
- window-scaled-viewport-properties.html is the LayoutTest that tests that the
above APIs are relative to the layout viewport.

- This CL adds the following methods to Internals.cpp for tests that need the
visual viewport scroll position and dimensions: visualViewportHeight, 
visualViewportWidth, visualViewportScrollX, visualViewportScrollY

Intent-to-ship:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/mt1uciFuvWQ/ecQi0f8k...

BUG=489206
==========

to

==========
Remove plumbing for the inert-visual-viewport flag.

Note that the Setting inertVisualViewport still exists and is set to false by
default. 
A follow up CL will set this to true once dependent tests are fixed.

BUG=489206
==========

[ymalik, 2015-10-23 20:18:29]

Description was changed from

==========
Remove plumbing for the inert-visual-viewport flag.

Note that the Setting inertVisualViewport still exists and is set to false by
default. 
A follow up CL will set this to true once dependent tests are fixed.

BUG=489206
==========

to

==========
Remove plumbing for the inert-visual-viewport flag.

Note that the Blink setting inertVisualViewport still exists and is set to false
by default. 
A follow up CL will set this to true once dependent tests are fixed.

BUG=489206
==========

[ymalik, 2015-10-23 20:26:10]

@bokan 

PTAL :)

[bokan, 2015-10-23 20:27:03]

still-lgtm

[ymalik, 2015-10-23 20:28:54]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2015-10-23 20:28:54]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
kenrb@chromium.org, rbyers@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1415513002/#ps230001
(title: " ")

[commit-bot: I haz the power, 2015-10-23 20:29:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1415513002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1415513002/230001

[commit-bot: I haz the power, 2015-10-23 22:30:37]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2015-10-23 22:31:19]

Patchset 13 (id:??) landed as
https://crrev.com/58d42ae1d6690a9f625e23f160b346e9cebeab01
Cr-Commit-Position: refs/heads/master@{#355907}