Visual viewport API initial implementation.

Visual viewport API initial implementation.

Adds a new visualViewport object to document that contains visual
viewport information.

A visualviewportchanged event fires everytime the visual viewport is
resized or scrolled.

WICG proposal: https://github.com/WICG/ViewportAPI

BUG=595826
Committed: https://crrev.com/e7ff0a19a888c470f0fa149e922f35b4f8fe5539
Cr-Commit-Position: refs/heads/master@{#385047}

[ymalik, 2016-03-17 20:16:16]

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

[ymalik, 2016-03-17 20:16:17]

@bokan, PTAL and let me know what you think :)

[ymalik, 2016-03-18 13:30:57]

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:209:
updateLayoutIgnorePendingStylesheets();
@bokan, I am not sure whether we need to call
updateLayoutIgnorePendingStylesheets(). I think it makes sense and I see it
being called from Element::scrollLeft and friends. WDYT?

[bokan, 2016-03-18 16:05:07]

Looks good overall, thanks!

There's still some discussion over whether we want it on Element or elsewhere.
Lets give that another day or so to settle before landing this.

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html
(right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html:16:
returned by JS visual viewport API.");
nit: "by JS" -> "by the JS"

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html:51:
document.scrollingElement.viewport.scrollTop = 20;
scrollingElement->documentElement

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/viewportchanged-event-fired.html
(right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewportchanged-event-fired.html:14:
when the visual viewport is scaled or scrolled.");
Please also add a separate test that viewportchanged is called when we resize
the window. See window-resize-scrollbars-test.html for how to resize window in a
test.

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewportchanged-event-fired.html:40:
internals.setPageScaleFactor(2);
Could you make this test so that we verify the event after each call? e.g.
something in this shape:

internals.setPageScaleFactor(2);
requestAnimationFrame(function() {
    // Check viewportchanged was/was not called
    runNextCheck(); // setVisualViewportOffset(10, 10) + rAF check
});

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:729:
document().updateLayoutIgnorePendingStylesheets();
You don't need this here. Having a VisualViewport element doesn't depend on
layout.

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:209:
updateLayoutIgnorePendingStylesheets();
On 2016/03/18 13:30:57, ymalik1 wrote:
> @bokan, I am not sure whether we need to call
> updateLayoutIgnorePendingStylesheets(). I think it makes sense and I see it
> being called from Element::scrollLeft and friends. WDYT?

I think we need it. The reason being that the visual viewport's scroll position
depends on the size of the layout viewport. The layout viewport's size depends
on the layout since it's calculated based on the minimum possible page scale and
the minimum page scale depends on content width. This means that a really wide
element can increase/decrease the layout viewport's width which means the max
scroll position will change.

[bokan, 2016-03-18 16:06:26]

Also, please remove the go/ link from the description since non-googlers wont be
able to see it. Linking to the github when I create it probably makes more
sense.

[ymalik, 2016-03-22 16:34:56]

Description was changed from

==========
Visual viewport API initial implementation.

Adds a new viewport objet to document.scrollingElement that contains visual
viewport information.

A viewportchanged event fires everytime the visual viewport is resized or
scrolled.

Design-doc: go/vv-api

BUG=595826
==========

to

==========
Visual viewport API initial implementation.

Adds a new viewport objet to document.scrollingElement that contains visual
viewport information.

A viewportchanged event fires everytime the visual viewport is resized or
scrolled.

Design-doc: go/vv-api

WICG proposal: https://discourse.wicg.io/t/explicit-api-for-visual-viewport/

BUG=595826
==========

[ymalik, 2016-03-22 16:35:06]

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html
(right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html:16:
returned by JS visual viewport API.");
On 2016/03/18 16:05:07, bokan wrote:
> nit: "by JS" -> "by the JS"

Done.

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html:51:
document.scrollingElement.viewport.scrollTop = 20;
On 2016/03/18 16:05:07, bokan wrote:
> scrollingElement->documentElement

Done.

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/viewportchanged-event-fired.html
(right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewportchanged-event-fired.html:14:
when the visual viewport is scaled or scrolled.");
On 2016/03/18 16:05:07, bokan wrote:
> Please also add a separate test that viewportchanged is called when we resize
> the window. See window-resize-scrollbars-test.html for how to resize window in
a
> test.

Thanks, wasn't firing one before. Done.

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewportchanged-event-fired.html:40:
internals.setPageScaleFactor(2);
On 2016/03/18 16:05:07, bokan wrote:
> Could you make this test so that we verify the event after each call? e.g.
> something in this shape:
> 
> internals.setPageScaleFactor(2);
> requestAnimationFrame(function() {
>     // Check viewportchanged was/was not called
>     runNextCheck(); // setVisualViewportOffset(10, 10) + rAF check
> });

Done.

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Element.cpp:729:
document().updateLayoutIgnorePendingStylesheets();
On 2016/03/18 16:05:07, bokan wrote:
> You don't need this here. Having a VisualViewport element doesn't depend on
> layout.

Done.

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1814013002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:209:
updateLayoutIgnorePendingStylesheets();
On 2016/03/18 16:05:07, bokan wrote:
> On 2016/03/18 13:30:57, ymalik1 wrote:
> > @bokan, I am not sure whether we need to call
> > updateLayoutIgnorePendingStylesheets(). I think it makes sense and I see it
> > being called from Element::scrollLeft and friends. WDYT?
> 
> I think we need it. The reason being that the visual viewport's scroll
position
> depends on the size of the layout viewport. The layout viewport's size depends
> on the layout since it's calculated based on the minimum possible page scale
and
> the minimum page scale depends on content width. This means that a really wide
> element can increase/decrease the layout viewport's width which means the max
> scroll position will change.

Acknowledged.

[bokan, 2016-03-22 18:30:30]

lgtm % comments

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html
(right):

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html:53:
shouldBe("document.visualViewport.scrollTop", "0");
It might be worth adding a case here with some fractional values.

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html
(right):

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html:47:
// Scroll both viewports. Listner not called.
Check this in a rAF too please. Right now it could be firing but coalesced with
the setPageScaleFactor below

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

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:128:
dispatchChangedEvent();
Hmm...right now we're firing events synchronously. I wonder if we should
coalesce them and fire one at the beginning of each frame? Lets do that in a
follow up if needed.

[ymalik, 2016-03-22 21:28:19]

ymalik@chromium.org changed reviewers:
+ drott@chromium.org, rbyers@chromium.org

[ymalik, 2016-03-22 21:28:20]

+drott for Source/platform/RuntimeEnabledFeatures.in
+rbyers for Source/core

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html
(right):

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/verify-viewport-dimensions.html:53:
shouldBe("document.visualViewport.scrollTop", "0");
On 2016/03/22 18:30:29, bokan wrote:
> It might be worth adding a case here with some fractional values.

Done.

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html
(right):

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html:47:
// Scroll both viewports. Listner not called.
On 2016/03/22 18:30:29, bokan wrote:
> Check this in a rAF too please. Right now it could be firing but coalesced
with
> the setPageScaleFactor below

Spoke to you offline. Don't need rAFs

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

https://codereview.chromium.org/1814013002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:128:
dispatchChangedEvent();
On 2016/03/22 18:30:29, bokan wrote:
> Hmm...right now we're firing events synchronously. I wonder if we should
> coalesce them and fire one at the beginning of each frame? Lets do that in a
> follow up if needed.

Acknowledged.

[bokan, 2016-03-22 21:34:22]

thanks, still lgtm + some final nits. Please update the CL description to remove
the go/ link.

https://codereview.chromium.org/1814013002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html
(right):

https://codereview.chromium.org/1814013002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html:29:
// visualviewportchanged is non-bubbling.
I don't think you need this, document is above documentElement.

https://codereview.chromium.org/1814013002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html:36:
eventSender.continuousMouseScrollBy(100, 100);
check numCalls == 0 here.

[drott, 2016-03-22 23:01:35]

LGTM for third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in

[ymalik, 2016-03-23 00:13:33]

Description was changed from

==========
Visual viewport API initial implementation.

Adds a new viewport objet to document.scrollingElement that contains visual
viewport information.

A viewportchanged event fires everytime the visual viewport is resized or
scrolled.

Design-doc: go/vv-api

WICG proposal: https://discourse.wicg.io/t/explicit-api-for-visual-viewport/

BUG=595826
==========

to

==========
Visual viewport API initial implementation.

Adds a new visualViewport object to document that contains visual
viewport information.

A visualviewportchanged event fires everytime the visual viewport is 
resized or scrolled.

WICG proposal: https://discourse.wicg.io/t/explicit-api-for-visual-viewport/

BUG=595826
==========

[ymalik, 2016-03-23 00:13:44]

https://codereview.chromium.org/1814013002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html
(right):

https://codereview.chromium.org/1814013002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html:29:
// visualviewportchanged is non-bubbling.
On 2016/03/22 21:34:21, bokan wrote:
> I don't think you need this, document is above documentElement.

Done.

https://codereview.chromium.org/1814013002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/visualviewportchanged-event-fired.html:36:
eventSender.continuousMouseScrollBy(100, 100);
On 2016/03/22 21:34:21, bokan wrote:
> check numCalls == 0 here.

Done.

[ymalik, 2016-03-24 18:22:02]

@Rick 

Gentle ping :)

[Rick Byers, 2016-03-24 19:07:54]

Sorry I missed you had asked for my review.
core LGTM with nits

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.idl:159: // Visual Viewport API
link to explainer

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.idl (right):

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.idl:27: // WICG proposal:
https://discourse.wicg.io/t/explicit-api-for-visual-viewport/
nit: link directly to the explainer instead (since it'll presumably evolve).

[esprehn, 2016-03-24 19:20:16]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2016-03-24 19:20:17]

Bunch of comments. This code probably wants to use per frame events. Just like
resize and scroll. Also please don't expose a public layout proxy method. :)

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:93: Document* document =
mainFrame()->document();
Can a frame really have a null document? When does that happen?

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:128:
dispatchChangedEvent();
Did you really mean for this event to be sync? I think you want a perFrame event
here.

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:252:
updateLayoutIgnorePendingStylesheets();
This layout method should be private.

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.h (right):

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.h:205: void
updateLayoutIgnorePendingStylesheets();
I'd rather you didn't proxy this method. Callers should do it directly when
needed. Also why is this public?

[ymalik, 2016-03-29 19:17:28]

@esprehn PTAL :)

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.idl:159: // Visual Viewport API
On 2016/03/24 19:07:54, Rick Byers wrote:
> link to explainer

Done.

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:93: Document* document =
mainFrame()->document();
On 2016/03/24 19:20:17, esprehn wrote:
> Can a frame really have a null document? When does that happen?

I am not sure. I see that LocalFrame::document() returns null if  m_domWindow is
null. I also see it being used in other places in this class.

Do you think it's safe to remove it?

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:128:
dispatchChangedEvent();
On 2016/03/24 19:20:17, esprehn wrote:
> Did you really mean for this event to be sync? I think you want a perFrame
event
> here.

You're right. This should be a per frame event. Fixed.

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:252:
updateLayoutIgnorePendingStylesheets();
On 2016/03/24 19:20:17, esprehn wrote:
> This layout method should be private.

Done.

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.h (right):

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.h:205: void
updateLayoutIgnorePendingStylesheets();
On 2016/03/24 19:20:17, esprehn wrote:
> I'd rather you didn't proxy this method. Callers should do it directly when
> needed. Also why is this public?

You mentioned below that this method should be private. Do you prefer that I
make it private or simply remove it? 

I had it because we need to call it from a bunch of places and I didn't want to
do null checks every time.

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.idl (right):

https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.idl:27: // WICG proposal:
https://discourse.wicg.io/t/explicit-api-for-visual-viewport/
On 2016/03/24 19:07:54, Rick Byers wrote:
> nit: link directly to the explainer instead (since it'll presumably evolve).

Done.

[bokan, 2016-03-29 19:19:34]

Description was changed from

==========
Visual viewport API initial implementation.

Adds a new visualViewport object to document that contains visual
viewport information.

A visualviewportchanged event fires everytime the visual viewport is 
resized or scrolled.

WICG proposal: https://discourse.wicg.io/t/explicit-api-for-visual-viewport/

BUG=595826
==========

to

==========
Visual viewport API initial implementation.

Adds a new visualViewport object to document that contains visual
viewport information.

A visualviewportchanged event fires everytime the visual viewport is 
resized or scrolled.

WICG proposal: https://github.com/WICG/ViewportAPI

BUG=595826
==========

[ymalik, 2016-03-30 18:51:08]

On 2016/03/29 19:17:28, ymalik1 wrote:
> @esprehn PTAL :)
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/dom/Document.idl (right):
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/dom/Document.idl:159: // Visual Viewport API
> On 2016/03/24 19:07:54, Rick Byers wrote:
> > link to explainer
> 
> Done.
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/VisualViewport.cpp:93: Document* document
=
> mainFrame()->document();
> On 2016/03/24 19:20:17, esprehn wrote:
> > Can a frame really have a null document? When does that happen?
> 
> I am not sure. I see that LocalFrame::document() returns null if  m_domWindow
is
> null. I also see it being used in other places in this class.
> 
> Do you think it's safe to remove it?
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/VisualViewport.cpp:128:
> dispatchChangedEvent();
> On 2016/03/24 19:20:17, esprehn wrote:
> > Did you really mean for this event to be sync? I think you want a perFrame
> event
> > here.
> 
> You're right. This should be a per frame event. Fixed.
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/VisualViewport.cpp:252:
> updateLayoutIgnorePendingStylesheets();
> On 2016/03/24 19:20:17, esprehn wrote:
> > This layout method should be private.
> 
> Done.
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/VisualViewport.h (right):
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/VisualViewport.h:205: void
> updateLayoutIgnorePendingStylesheets();
> On 2016/03/24 19:20:17, esprehn wrote:
> > I'd rather you didn't proxy this method. Callers should do it directly when
> > needed. Also why is this public?
> 
> You mentioned below that this method should be private. Do you prefer that I
> make it private or simply remove it? 
> 
> I had it because we need to call it from a bunch of places and I didn't want
to
> do null checks every time.
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/frame/VisualViewport.idl (right):
> 
>
https://codereview.chromium.org/1814013002/diff/100001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/frame/VisualViewport.idl:27: // WICG proposal:
> https://discourse.wicg.io/t/explicit-api-for-visual-viewport/
> On 2016/03/24 19:07:54, Rick Byers wrote:
> > nit: link directly to the explainer instead (since it'll presumably evolve).
> 
> Done.

@esprehn, gentle ping :)

[esprehn, 2016-03-31 05:15:37]

lgtm w/ comments fixed.

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1234: return
&host->visualViewport();
does your spec say that a document without a view has a null viewport?

Your code here means:

document.implementation.createHTMLDocument().viewport is null.

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:97:
document->updateLayoutIgnorePendingStylesheets();
we would normally write:

if (Document* document = mainFrame()->document())
    document->updateLayoutIgnorePendingStylesheets();

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:98: return;
remove return

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:103: if
(RuntimeEnabledFeatures::visualViewportAPIEnabled()) {
early return instead of wrapping the whole function

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.h (right):

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.h:35: #include
"core/events/Event.h"
I don't think you need this header

[bokan, 2016-03-31 15:23:55]

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Document.cpp:1234: return
&host->visualViewport();
On 2016/03/31 05:15:37, esprehn wrote:
> does your spec say that a document without a view has a null viewport?
> 
> Your code here means:
> 
> document.implementation.createHTMLDocument().viewport is null.

That seems correct to me. I've added the wording in the spec.

[ymalik, 2016-04-01 17:15:37]

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right):

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:97:
document->updateLayoutIgnorePendingStylesheets();
On 2016/03/31 05:15:37, esprehn wrote:
> we would normally write:
> 
> if (Document* document = mainFrame()->document())
>     document->updateLayoutIgnorePendingStylesheets();

Done.

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:98: return;
On 2016/03/31 05:15:37, esprehn wrote:
> remove return

Done.

https://codereview.chromium.org/1814013002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/VisualViewport.cpp:103: if
(RuntimeEnabledFeatures::visualViewportAPIEnabled()) {
On 2016/03/31 05:15:37, esprehn wrote:
> early return instead of wrapping the whole function

Done.

[ymalik, 2016-04-01 17:15:43]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-04-01 17:15:44]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
drott@chromium.org, rbyers@chromium.org, esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1814013002/#ps140001
(title: "address review comments")

[commit-bot: I haz the power, 2016-04-01 17:16:03]

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

[commit-bot: I haz the power, 2016-04-01 18:23:17]

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

[commit-bot: I haz the power, 2016-04-01 18:23:18]

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

[ymalik, 2016-04-04 14:18:45]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-04-04 14:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
esprehn@chromium.org, rbyers@chromium.org, drott@chromium.org
Link to the patchset: https://codereview.chromium.org/1814013002/#ps160001
(title: "fix web exposed interface listing layout test")

[commit-bot: I haz the power, 2016-04-04 14:19:03]

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

[commit-bot: I haz the power, 2016-04-04 14:32:11]

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

[commit-bot: I haz the power, 2016-04-04 14:32:12]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[ymalik, 2016-04-04 19:53:22]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-04-04 19:53:23]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
esprehn@chromium.org, rbyers@chromium.org, drott@chromium.org
Link to the patchset: https://codereview.chromium.org/1814013002/#ps180001
(title: " ")

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

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

[commit-bot: I haz the power, 2016-04-04 23:22:23]

Description was changed from

==========
Visual viewport API initial implementation.

Adds a new visualViewport object to document that contains visual
viewport information.

A visualviewportchanged event fires everytime the visual viewport is 
resized or scrolled.

WICG proposal: https://github.com/WICG/ViewportAPI

BUG=595826
==========

to

==========
Visual viewport API initial implementation.

Adds a new visualViewport object to document that contains visual
viewport information.

A visualviewportchanged event fires everytime the visual viewport is 
resized or scrolled.

WICG proposal: https://github.com/WICG/ViewportAPI

BUG=595826
==========

[commit-bot: I haz the power, 2016-04-04 23:22:24]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-04 23:24:39]

Description was changed from

==========
Visual viewport API initial implementation.

Adds a new visualViewport object to document that contains visual
viewport information.

A visualviewportchanged event fires everytime the visual viewport is 
resized or scrolled.

WICG proposal: https://github.com/WICG/ViewportAPI

BUG=595826
==========

to

==========
Visual viewport API initial implementation.

Adds a new visualViewport object to document that contains visual
viewport information.

A visualviewportchanged event fires everytime the visual viewport is
resized or scrolled.

WICG proposal: https://github.com/WICG/ViewportAPI

BUG=595826

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

[commit-bot: I haz the power, 2016-04-04 23:24:40]

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