Seperate visualviewportchanged event into scroll and resize events

Seperate visualviewportchanged event into scroll and resize events

These events are be fired on window.visualViewport.

BUG=622817
Committed: https://crrev.com/df3983ce60714cb2b7bf22482946ab8aac31d934
Cr-Commit-Position: refs/heads/master@{#402282}

[ymalik, 2016-06-24 21:35:56]

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

[ymalik, 2016-06-24 21:35:57]

@bokan, note that although I used git mv to rename the tests, it still shows
them as added and removed.

[ymalik, 2016-06-24 21:36:14]

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

[commit-bot: I haz the power, 2016-06-24 21:37:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2016-06-24 21:52:37]

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-event-fired-window-resized.html
(right):

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-event-fired-window-resized.html:8:
var t = async_test('verify that the scroll events get fired when the window is
resized.');
This should be testing the resize event is fired when the window is resized.

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-resize-events-fired.html
(right):

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-resize-events-fired.html:57:
internals.setPageScaleFactor(pageScaleFactor);
Please split up this test into a scroll test and a zoom test. Doesn't
necessarily have to be separate files but separate test functions in
testharness.js

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/DOMVisualViewport.h (right):

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:44: class
DOMVisualViewport final : public EventTargetWithInlineData {
Since EventTarget is GCFinalized and has a virtual destructor, we should define
a virtual destructor here too (even if it's empty). In fact,
EventTargetWithInlineData needs this too so please add there too.

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:136:
enqueueScrollEvent();
Why are we sending a scroll event when the size changes? This should be
enqueueResizeEvent, no?

[commit-bot: I haz the power, 2016-06-24 22:40:36]

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

[commit-bot: I haz the power, 2016-06-24 22:40:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[ymalik, 2016-06-27 14:10:02]

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-event-fired-window-resized.html
(right):

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-event-fired-window-resized.html:8:
var t = async_test('verify that the scroll events get fired when the window is
resized.');
On 2016/06/24 21:52:36, bokan wrote:
> This should be testing the resize event is fired when the window is resized.

Absolutely!

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-resize-events-fired.html
(right):

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-resize-events-fired.html:57:
internals.setPageScaleFactor(pageScaleFactor);
On 2016/06/24 21:52:36, bokan wrote:
> Please split up this test into a scroll test and a zoom test. Doesn't
> necessarily have to be separate files but separate test functions in
> testharness.js

We will always have to use internals to scale then scroll. Note that the
"verifySetViewportOffsets" function above will go away in my next CL since we
decided to make the API readonly for V1.

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:136:
enqueueScrollEvent();
On 2016/06/24 21:52:37, bokan wrote:
> Why are we sending a scroll event when the size changes? This should be
> enqueueResizeEvent, no?

Yes!

[bokan, 2016-06-27 14:17:03]

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-resize-events-fired.html
(right):

https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-resize-events-fired.html:57:
internals.setPageScaleFactor(pageScaleFactor);
On 2016/06/27 14:10:01, ymalik wrote:
> On 2016/06/24 21:52:36, bokan wrote:
> > Please split up this test into a scroll test and a zoom test. Doesn't
> > necessarily have to be separate files but separate test functions in
> > testharness.js
> 
> We will always have to use internals to scale then scroll. Note that the
> "verifySetViewportOffsets" function above will go away in my next CL since we
> decided to make the API readonly for V1.  

My comment above was about splitting this test into one test that scrolls and
checks for the scroll event, and another that scales and checks for the resize
event. It doesn't have anything to do with the internals method.

Testharness failures are pretty tough to narrow down when the tests are large.
It's best to have multiple small, targeted tests.

[ymalik, 2016-06-27 19:03:21]

On 2016/06/27 14:17:03, bokan wrote:
>
https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-resize-events-fired.html
> (right):
> 
>
https://codereview.chromium.org/2096783004/diff/20001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/fast/dom/viewport/scroll-resize-events-fired.html:57:
> internals.setPageScaleFactor(pageScaleFactor);
> On 2016/06/27 14:10:01, ymalik wrote:
> > On 2016/06/24 21:52:36, bokan wrote:
> > > Please split up this test into a scroll test and a zoom test. Doesn't
> > > necessarily have to be separate files but separate test functions in
> > > testharness.js
> > 
> > We will always have to use internals to scale then scroll. Note that the
> > "verifySetViewportOffsets" function above will go away in my next CL since
we
> > decided to make the API readonly for V1.  
> 
> My comment above was about splitting this test into one test that scrolls and
> checks for the scroll event, and another that scales and checks for the resize
> event. It doesn't have anything to do with the internals method.
> 
> Testharness failures are pretty tough to narrow down when the tests are large.
> It's best to have multiple small, targeted tests.

Spoke to you offline. Annotated the tests to narrow down failures.

[ymalik, 2016-06-27 19:03:34]

PTAL :)

[ymalik, 2016-06-27 19:03:42]

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

[commit-bot: I haz the power, 2016-06-27 19:04:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 20:40:54]

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

[commit-bot: I haz the power, 2016-06-27 20:40:55]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-06-27 21:04:38]

thanks, lgtm

[ymalik, 2016-06-27 21:06:54]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-06-27 21:08:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 21:15:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-27 21:16:58]

Description was changed from

==========
Seperate visualviewportchanged event into scroll and resize events

These events are be fired on window.visualViewport.

BUG=622817
==========

to

==========
Seperate visualviewportchanged event into scroll and resize events

These events are be fired on window.visualViewport.

BUG=622817

Committed: https://crrev.com/df3983ce60714cb2b7bf22482946ab8aac31d934
Cr-Commit-Position: refs/heads/master@{#402282}
==========

[commit-bot: I haz the power, 2016-06-27 21:16:59]

Patchset 4 (id:??) landed as
https://crrev.com/df3983ce60714cb2b7bf22482946ab8aac31d934
Cr-Commit-Position: refs/heads/master@{#402282}