Initial viewport is not the same as FrameView rect.

Initial viewport is not the same as FrameView rect.

FrameView rect is also changed by the visual viewport.

Set the initial viewport size on FrameView whenever it changes in
WebViewImpl and use it for matching media queries when collecting
@viewport rules. See [2] for spec reference.

This is fixing what was introduced in [1].

[1] https://codereview.chromium.org/2414343002/
[2] https://www.w3.org/TR/css-device-adapt-1/#media-queries

R=bokan@chromium.org,timloh@chromium.org
BUG=332763
Committed: https://crrev.com/d2002126e0773ec4cc5bdc01d992ed37e6fbed61
Cr-Commit-Position: refs/heads/master@{#426424}

[rune, 2016-10-18 10:18:33]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 10:18:46]

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

[rune, 2016-10-18 10:19:31]

Description was changed from

==========
Initial viewport is not the same as FrameView rect.

FrameView rect is also changed by the visual viewport.

Set the initial viewport size on FrameView whenever it changes in
WebViewImpl and use it for matching media queries when collecting
@viewport rules.

This is fixing what was introduced in [1].

[1] https://codereview.chromium.org/2414343002/

R=bokan@chromium.org,timloh@chromium.org
BUG=332763
==========

to

==========
Initial viewport is not the same as FrameView rect.

FrameView rect is also changed by the visual viewport.

Set the initial viewport size on FrameView whenever it changes in
WebViewImpl and use it for matching media queries when collecting
@viewport rules. See [2] for spec reference.

This is fixing what was introduced in [1].

[1] https://codereview.chromium.org/2414343002/
[2] https://www.w3.org/TR/css-device-adapt-1/#media-queries

R=bokan@chromium.org,timloh@chromium.org
BUG=332763
==========

[rune, 2016-10-18 10:19:59]

ptal

[commit-bot: I haz the power, 2016-10-18 11:40:22]

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

[commit-bot: I haz the power, 2016-10-18 11:40:23]

Dry run: This issue passed the CQ dry run.

[rune, 2016-10-18 15:36:31]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 15:36:58]

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-10-18 16:50:15]

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

[commit-bot: I haz the power, 2016-10-18 16:50:16]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-10-18 17:19:26]

https://codereview.chromium.org/2431613002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/MediaValuesInitialViewport.cpp (right):

https://codereview.chromium.org/2431613002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/MediaValuesInitialViewport.cpp:23: return
m_frame->view()->initialViewportWidth();
I'm a stranger around these parts, but m_frame can be an iframe, right? I don't
see where we set the initialViewport size for non-main frames.

[rune, 2016-10-18 22:09:53]

https://codereview.chromium.org/2431613002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/MediaValuesInitialViewport.cpp (right):

https://codereview.chromium.org/2431613002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/MediaValuesInitialViewport.cpp:23: return
m_frame->view()->initialViewportWidth();
On 2016/10/18 17:19:26, bokan wrote:
> I'm a stranger around these parts, but m_frame can be an iframe, right? I
don't
> see where we set the initialViewport size for non-main frames.

Right, MediaValuesInitialViewport are only used for @viewport descriptors and
@media evaluation for collecting @viewport rules.

@viewport rules are only collected for main frames. That is, they will only be
collected for main frames after [1]. Before [1] they are collected, but setting
the viewport description has no effect since we're checking for main frame
before doing anything with it.

It would be more consistent if we initialized FrameView's initialViewportSize
for non-main frames as well?

Or perhaps DCHECKs for being called from a main frame?

[1] https://codereview.chromium.org/2420413005/

[bokan, 2016-10-18 22:28:06]

https://codereview.chromium.org/2431613002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/MediaValuesInitialViewport.cpp (right):

https://codereview.chromium.org/2431613002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/MediaValuesInitialViewport.cpp:23: return
m_frame->view()->initialViewportWidth();
On 2016/10/18 22:09:53, rune wrote:
> On 2016/10/18 17:19:26, bokan wrote:
> > I'm a stranger around these parts, but m_frame can be an iframe, right? I
> don't
> > see where we set the initialViewport size for non-main frames.
> 
> Right, MediaValuesInitialViewport are only used for @viewport descriptors and
> @media evaluation for collecting @viewport rules.
> 
> @viewport rules are only collected for main frames. That is, they will only be
> collected for main frames after [1]. Before [1] they are collected, but
setting
> the viewport description has no effect since we're checking for main frame
> before doing anything with it.
> 
> It would be more consistent if we initialized FrameView's initialViewportSize
> for non-main frames as well?
> 
> Or perhaps DCHECKs for being called from a main frame?
> 
> [1] https://codereview.chromium.org/2420413005/

Ok. I'm fine with either approach though DCHECKs for main frame are probably
better since we don't expect initialViewport to be used from iframes for now.

[rune, 2016-10-18 22:56:12]

https://codereview.chromium.org/2431613002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/MediaValuesInitialViewport.cpp (right):

https://codereview.chromium.org/2431613002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/MediaValuesInitialViewport.cpp:23: return
m_frame->view()->initialViewportWidth();
On 2016/10/18 22:28:06, bokan wrote:
> On 2016/10/18 22:09:53, rune wrote:
> > On 2016/10/18 17:19:26, bokan wrote:
> > > I'm a stranger around these parts, but m_frame can be an iframe, right? I
> > don't
> > > see where we set the initialViewport size for non-main frames.
> > 
> > Right, MediaValuesInitialViewport are only used for @viewport descriptors
and
> > @media evaluation for collecting @viewport rules.
> > 
> > @viewport rules are only collected for main frames. That is, they will only
be
> > collected for main frames after [1]. Before [1] they are collected, but
> setting
> > the viewport description has no effect since we're checking for main frame
> > before doing anything with it.
> > 
> > It would be more consistent if we initialized FrameView's
initialViewportSize
> > for non-main frames as well?
> > 
> > Or perhaps DCHECKs for being called from a main frame?
> > 
> > [1] https://codereview.chromium.org/2420413005/
> 
> Ok. I'm fine with either approach though DCHECKs for main frame are probably
> better since we don't expect initialViewport to be used from iframes for now.

DCHECKs added.

[rune, 2016-10-19 07:38:34]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 09:31:43]

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-10-19 12:30:40]

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

[commit-bot: I haz the power, 2016-10-19 12:30:41]

Dry run: This issue passed the CQ dry run.

[bokan, 2016-10-19 12:51:00]

thanks, lgtm. But please wait for Tim's review.

[Timothy Loh, 2016-10-20 03:15:36]

On 2016/10/19 12:51:00, bokan wrote:
> thanks, lgtm. But please wait for Tim's review.

lgtm

[rune, 2016-10-20 06:48:19]

The CQ bit was checked by rune@opera.com

[commit-bot: I haz the power, 2016-10-20 06:48:33]

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-10-20 07:00:03]

Description was changed from

==========
Initial viewport is not the same as FrameView rect.

FrameView rect is also changed by the visual viewport.

Set the initial viewport size on FrameView whenever it changes in
WebViewImpl and use it for matching media queries when collecting
@viewport rules. See [2] for spec reference.

This is fixing what was introduced in [1].

[1] https://codereview.chromium.org/2414343002/
[2] https://www.w3.org/TR/css-device-adapt-1/#media-queries

R=bokan@chromium.org,timloh@chromium.org
BUG=332763
==========

to

==========
Initial viewport is not the same as FrameView rect.

FrameView rect is also changed by the visual viewport.

Set the initial viewport size on FrameView whenever it changes in
WebViewImpl and use it for matching media queries when collecting
@viewport rules. See [2] for spec reference.

This is fixing what was introduced in [1].

[1] https://codereview.chromium.org/2414343002/
[2] https://www.w3.org/TR/css-device-adapt-1/#media-queries

R=bokan@chromium.org,timloh@chromium.org
BUG=332763
==========

[commit-bot: I haz the power, 2016-10-20 07:00:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-21 13:16:28]

Description was changed from

==========
Initial viewport is not the same as FrameView rect.

FrameView rect is also changed by the visual viewport.

Set the initial viewport size on FrameView whenever it changes in
WebViewImpl and use it for matching media queries when collecting
@viewport rules. See [2] for spec reference.

This is fixing what was introduced in [1].

[1] https://codereview.chromium.org/2414343002/
[2] https://www.w3.org/TR/css-device-adapt-1/#media-queries

R=bokan@chromium.org,timloh@chromium.org
BUG=332763
==========

to

==========
Initial viewport is not the same as FrameView rect.

FrameView rect is also changed by the visual viewport.

Set the initial viewport size on FrameView whenever it changes in
WebViewImpl and use it for matching media queries when collecting
@viewport rules. See [2] for spec reference.

This is fixing what was introduced in [1].

[1] https://codereview.chromium.org/2414343002/
[2] https://www.w3.org/TR/css-device-adapt-1/#media-queries

R=bokan@chromium.org,timloh@chromium.org
BUG=332763

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

[commit-bot: I haz the power, 2016-10-21 13:16:29]

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