Add <meta viewport> support to the preloader

Add <meta viewport> support to the preloader

This CL adds <meta viewport> support to the HTMLPreloadScanner, avoiding the risk of double download or downloads based on the wrong viewport dimensions when <meta viewport> is present in the page.

BUG=471249
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194838

[Yoav Weiss, 2015-04-28 06:45:17]

yoav@yoav.ws changed reviewers:
+ mkwst@chromium.org

[Yoav Weiss, 2015-04-28 06:50:12]

Hey Mike!

I know you're traveling but just in case you're jet lagged and bored, I've
prepared a CL just for you :) If you're too busy, let me know and I'll find
another reviewer.

This CL still needs some more tests IMO, but I'm not sure if it should be in
LayoutTests, or if it's time to create a unit test for the preload scanner and
the meta viewport parser.
And if we got the unit tests route, not sure if they should be part of this CL
or a separate one. Let me know if you have a preference, and what test cases you
think should be covered by such tests.

[Yoav Weiss, 2015-04-28 14:48:39]

yoav@yoav.ws changed reviewers:
+ japhet@chromium.org, kouhei@chromium.org

[kouhei (in TOK), 2015-04-30 12:45:39]

https://codereview.chromium.org/1104103002/diff/20001/Source/core/html/parser...
File Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1104103002/diff/20001/Source/core/html/parser...
Source/core/html/parser/HTMLDocumentParser.cpp:678: document()->settings() &&
document()->settings()->viewportMetaZeroValuesQuirk()));
Can these 3 parameters be made a struct? with a helper method to initialize it
from document()?
I'm a bit concerned about duplicated code.

[Yoav Weiss, 2015-04-30 12:49:40]

On 2015/04/30 12:45:39, kouhei wrote:
>
https://codereview.chromium.org/1104103002/diff/20001/Source/core/html/parser...
> File Source/core/html/parser/HTMLDocumentParser.cpp (right):
> 
>
https://codereview.chromium.org/1104103002/diff/20001/Source/core/html/parser...
> Source/core/html/parser/HTMLDocumentParser.cpp:678: document()->settings() &&
> document()->settings()->viewportMetaZeroValuesQuirk()));
> Can these 3 parameters be made a struct? with a helper method to initialize it
> from document()?
> I'm a bit concerned about duplicated code.

Sure. I'll do that

Also - can you take a look at https://codereview.chromium.org/1108303002/ ? I'd
like to land it before this CL, so I can add viewport related unit tests to this
CL.

Thanks! :)

[blink-reviews, 2015-04-30 12:51:12]

I'm taking a look at it right now :) Feel free to loop me in for any
parser/ reviews.

On Thu, Apr 30, 2015 at 9:49 PM, <yoav@yoav.ws> wrote:

> On 2015/04/30 12:45:39, kouhei wrote:
>
>
>
https://codereview.chromium.org/1104103002/diff/20001/Source/core/html/parser...
>
>> File Source/core/html/parser/HTMLDocumentParser.cpp (right):
>>
>
>
>
>
https://codereview.chromium.org/1104103002/diff/20001/Source/core/html/parser...
>
>> Source/core/html/parser/HTMLDocumentParser.cpp:678:
>> document()->settings() &&
>> document()->settings()->viewportMetaZeroValuesQuirk()));
>> Can these 3 parameters be made a struct? with a helper method to
>> initialize it
>> from document()?
>> I'm a bit concerned about duplicated code.
>>
>
> Sure. I'll do that
>
> Also - can you take a look at https://codereview.chromium.org/1108303002/
> ? I'd
> like to land it before this CL, so I can add viewport related unit tests
> to this
> CL.
>
> Thanks! :)
>
> https://codereview.chromium.org/1104103002/
>



-- 
Kouhei Ueno

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Nate Chapin, 2015-04-30 20:19:04]

https://codereview.chromium.org/1104103002/diff/60001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/1104103002/diff/60001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.h:59:
CachedDocumentParameters(Document*, PassRefPtr<MediaValues>);
Can we have two create() methods, but just one constructor? The bodies of these
constructors are pretty redundant.

[Yoav Weiss, 2015-04-30 21:08:45]

On 2015/04/30 20:19:04, Nate Chapin wrote:
>
https://codereview.chromium.org/1104103002/diff/60001/Source/core/html/parser...
> File Source/core/html/parser/HTMLPreloadScanner.h (right):
> 
>
https://codereview.chromium.org/1104103002/diff/60001/Source/core/html/parser...
> Source/core/html/parser/HTMLPreloadScanner.h:59:
> CachedDocumentParameters(Document*, PassRefPtr<MediaValues>);
> Can we have two create() methods, but just one constructor? The bodies of
these
> constructors are pretty redundant.

Removed both extra constructor and create(), since both were in fact redundant.

[Nate Chapin, 2015-05-01 22:10:28]

lgtm if kouhei is happy.

https://codereview.chromium.org/1104103002/diff/80001/Source/core/html/parser...
File Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/1104103002/diff/80001/Source/core/html/parser...
Source/core/html/parser/HTMLPreloadScanner.h:53:
CachedDocumentParameters(Document*, PassRefPtr<MediaValues>);
We should either just call the constructor directly or make this private.

[Yoav Weiss, 2015-05-01 22:18:30]

On 2015/05/01 22:10:28, Nate Chapin wrote:
> lgtm if kouhei is happy.

kouhei - are you happy? :)

> 
>
https://codereview.chromium.org/1104103002/diff/80001/Source/core/html/parser...
> File Source/core/html/parser/HTMLPreloadScanner.h (right):
> 
>
https://codereview.chromium.org/1104103002/diff/80001/Source/core/html/parser...
> Source/core/html/parser/HTMLPreloadScanner.h:53:
> CachedDocumentParameters(Document*, PassRefPtr<MediaValues>);
> We should either just call the constructor directly or make this private.

Moved to private

[kouhei (in TOK), 2015-05-02 01:26:14]

lgtm. Thanks!

[Yoav Weiss, 2015-05-02 04:58:39]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2015-05-02 04:58:40]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1104103002/#ps100001
(title: "moved CachedDocumentParameters constructor to private")

[commit-bot: I haz the power, 2015-05-02 04:58:48]

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

[commit-bot: I haz the power, 2015-05-02 05:02:44]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194838