Seperate DOMVisualViewport from Blink's VisualViewport class

Seperate DOMVisualViewport from Blink's VisualViewport class

DOMVisualViewport represents the things exposed via the VisualViewportAPI. This
is what's returned from window.visualViewport. It queries the Blink's
VisualViewport for the main frame and returns sane values for iframes.

BUG=604928
Committed: https://crrev.com/81dd83a5bae93da969e909c58ed3ecb50d50100c
Cr-Commit-Position: refs/heads/master@{#401616}

[ymalik, 2016-06-15 14:25:44]

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

[commit-bot: I haz the power, 2016-06-15 14:25:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2070563002/1

[ymalik, 2016-06-15 14:26:19]

Description was changed from

==========
Seperate DOMVisualViewport from Blink's VisualViewport class

DOMVisualViewport represents the things exposed via the VisualViewportAPI. This
is what's returned from window.visualViewport. It queries the Blink's
VisualViewport for the main frame and returns sane values for iframes.

BUG=604928
==========

to

==========
Seperate DOMVisualViewport from Blink's VisualViewport class

DOMVisualViewport represents the things exposed via the VisualViewportAPI. This
is what's returned from window.visualViewport. It queries the Blink's
VisualViewport for the main frame and returns sane values for iframes.

BUG=604928
==========

[ymalik, 2016-06-15 14:26:19]

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

[commit-bot: I haz the power, 2016-06-15 15:57:54]

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

[commit-bot: I haz the power, 2016-06-15 15:57:55]

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

[bokan, 2016-06-16 07:22:44]

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt
(right):

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt:107:
FAIL window.cached_visualViewport.pageScale should be 0. Was 1.
Lets just make it return 0 in this case.

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt
(right):

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt:218:
FAIL childWindow.visualViewport.clientHeight should be 0. Threw exception
TypeError: Cannot read property 'clientHeight' of null
Why do some other properties not throw? e.g. childWindow.performance

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

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-dimensions-iframe.html:5:
height: 0px;
Lets use normal scrollbars and give the iframe some scrollable content (you can
use the srcdoc iframe attribute), to make sure we're returning
cilentWidth/Height not including scrollbars.

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

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.cpp:96: return
m_window->document()->documentElement()->clientWidth();
This is wrong, it should be the window's FrameView's
visibleContentRect(ExcludeScrollbars). See LocalDOMWindow::innerWidth (but we
have to exclude scrollbars)

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/DOMVisualViewport.h (right):

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:35: #include
"core/frame/LocalDOMWindow.h"
You should be able to forward declare this.

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:43: , public
GarbageCollectedMixin {
You don't need the Mixin class.

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:45:
USING_GARBAGE_COLLECTED_MIXIN(DOMVisualViewport);
Don't need this

[ymalik, 2016-06-20 15:52:56]

PTAL :)

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt
(right):

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt:107:
FAIL window.cached_visualViewport.pageScale should be 0. Was 1.
On 2016/06/16 07:22:43, bokan wrote:
> Lets just make it return 0 in this case.

Done.

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt
(right):

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt:218:
FAIL childWindow.visualViewport.clientHeight should be 0. Threw exception
TypeError: Cannot read property 'clientHeight' of null
On 2016/06/16 07:22:43, bokan wrote:
> Why do some other properties not throw? e.g. childWindow.performance

I think it has to do with whether or not a property is on the OilPan heap. If a
property is a Member<T> of Window, it is GC-ed and throws an exception (e.g
speechSynthesis, visualViewport).

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

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-dimensions-iframe.html:5:
height: 0px;
On 2016/06/16 07:22:43, bokan wrote:
> Lets use normal scrollbars and give the iframe some scrollable content (you
can
> use the srcdoc iframe attribute), to make sure we're returning
> cilentWidth/Height not including scrollbars.

Done.

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

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.cpp:96: return
m_window->document()->documentElement()->clientWidth();
On 2016/06/16 07:22:43, bokan wrote:
> This is wrong, it should be the window's FrameView's
> visibleContentRect(ExcludeScrollbars). See LocalDOMWindow::innerWidth (but we
> have to exclude scrollbars)

Done.

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/DOMVisualViewport.h (right):

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:35: #include
"core/frame/LocalDOMWindow.h"
On 2016/06/16 07:22:44, bokan wrote:
> You should be able to forward declare this.

Done.

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:43: , public
GarbageCollectedMixin {
On 2016/06/16 07:22:43, bokan wrote:
> You don't need the Mixin class.

You're right!

https://codereview.chromium.org/2070563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/DOMVisualViewport.h:45:
USING_GARBAGE_COLLECTED_MIXIN(DOMVisualViewport);
On 2016/06/16 07:22:43, bokan wrote:
> Don't need this

Done.

[bokan, 2016-06-22 18:29:08]

lgtm! (% comment nit)

https://codereview.chromium.org/2070563002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-dimensions-iframe.html
(right):

https://codereview.chromium.org/2070563002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/viewport/viewport-dimensions-iframe.html:26:
// verify visualViewport dimensions on containing iframe.
Describe in the comment why the values are this way. i.e. The page scale on the
root frame shouldn't affect the scale/viewport values in the iframe.

[ymalik, 2016-06-22 19:52:01]

The CQ bit was checked by ymalik@chromium.org

[ymalik, 2016-06-22 19:52:01]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org
Link to the patchset: https://codereview.chromium.org/2070563002/#ps40001
(title: "nit")

[commit-bot: I haz the power, 2016-06-22 19:52:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2070563002/40001

[commit-bot: I haz the power, 2016-06-22 23:03:25]

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

[commit-bot: I haz the power, 2016-06-22 23:03:26]

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-06-23 15:54:49]

The CQ bit was checked by ymalik@chromium.org

[commit-bot: I haz the power, 2016-06-23 15:55:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2070563002/40001

[commit-bot: I haz the power, 2016-06-23 15:59:30]

Description was changed from

==========
Seperate DOMVisualViewport from Blink's VisualViewport class

DOMVisualViewport represents the things exposed via the VisualViewportAPI. This
is what's returned from window.visualViewport. It queries the Blink's
VisualViewport for the main frame and returns sane values for iframes.

BUG=604928
==========

to

==========
Seperate DOMVisualViewport from Blink's VisualViewport class

DOMVisualViewport represents the things exposed via the VisualViewportAPI. This
is what's returned from window.visualViewport. It queries the Blink's
VisualViewport for the main frame and returns sane values for iframes.

BUG=604928
==========

[commit-bot: I haz the power, 2016-06-23 15:59:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-23 16:00:56]

Description was changed from

==========
Seperate DOMVisualViewport from Blink's VisualViewport class

DOMVisualViewport represents the things exposed via the VisualViewportAPI. This
is what's returned from window.visualViewport. It queries the Blink's
VisualViewport for the main frame and returns sane values for iframes.

BUG=604928
==========

to

==========
Seperate DOMVisualViewport from Blink's VisualViewport class

DOMVisualViewport represents the things exposed via the VisualViewportAPI. This
is what's returned from window.visualViewport. It queries the Blink's
VisualViewport for the main frame and returns sane values for iframes.

BUG=604928

Committed: https://crrev.com/81dd83a5bae93da969e909c58ed3ecb50d50100c
Cr-Commit-Position: refs/heads/master@{#401616}
==========

[commit-bot: I haz the power, 2016-06-23 16:00:58]

Patchset 3 (id:??) landed as
https://crrev.com/81dd83a5bae93da969e909c58ed3ecb50d50100c
Cr-Commit-Position: refs/heads/master@{#401616}

[bashi, 2016-07-11 09:17:34]

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

[bashi, 2016-07-11 09:17:35]

https://codereview.chromium.org/2070563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/2070563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:132:
[RuntimeEnabled=VisualViewportAPI] readonly attribute DOMVisualViewport?
visualViewport;
s/DOMVisualViewport/VisualViewport/ ?

"[ImplementedAs=FooBar] interface Foo {}" means the name of the interface is
"Foo" but its implementation inside Blink is "FooBar".

If this is correct, core/frame/VisualViewport.idl should be
core/frame/DOMVisualViewport.idl ?

[ymalik, 2016-07-11 17:14:56]

https://codereview.chromium.org/2070563002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/Window.idl (right):

https://codereview.chromium.org/2070563002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/Window.idl:132:
[RuntimeEnabled=VisualViewportAPI] readonly attribute DOMVisualViewport?
visualViewport;
On 2016/07/11 09:17:35, bashi1 wrote:
> s/DOMVisualViewport/VisualViewport/ ?
> 
> "[ImplementedAs=FooBar] interface Foo {}" means the name of the interface is
> "Foo" but its implementation inside Blink is "FooBar".
> 
> If this is correct, core/frame/VisualViewport.idl should be
> core/frame/DOMVisualViewport.idl ?

Thanks. It should be VisualViewport.

Fix here: https://codereview.chromium.org/2136113002/