Add support for media attribute on link

Add support for media attribute on link

This CL adds support to the `media` attribute on link preload tags and
equivalent Link headers. It avoids preloading the resources when media
does not match.

In order to accurately do that, even when preload is in the headers, and
the actual viewport definition is in markup, this CL delays the Link
preload header processing until the prelodScanner processed the first chunk,
gets the ViewportDescription from the preloadScanner, and applies it
before determining if a resource should be preloaded.

BUG=590188
Committed: https://crrev.com/bab2e5a737dae7594d25d90bf27965590d3406b3
Cr-Commit-Position: refs/heads/master@{#382603}

[Yoav Weiss, 2016-02-26 12:35:43]

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

[Yoav Weiss, 2016-02-26 12:35:44]

Hey Nate! :)

Can you take a look? The CL here:
* Enables the media attribute on Link preloads (markup and headers)
* Gets a ViewportDescription from the HTMLPreloadScanner
* Delays the Link header preloading in order to be able to apply the
ViewportDescription
 - That last part better aligns with what I've seen devs do with preload (i.e.
they usually don't want Link loaded resources to beat the critical resources at
the head of HTML)
 - I believe it will resolve the double download issues as seen in
https://bugs.chromium.org/p/chromium/issues/detail?id=589767

Let me know what you think

[Yoav Weiss, 2016-02-26 16:05:44]

On 2016/02/26 12:35:44, Yoav Weiss wrote:
> Hey Nate! :)
> 
> Can you take a look? The CL here:
> * Enables the media attribute on Link preloads (markup and headers)
> * Gets a ViewportDescription from the HTMLPreloadScanner
> * Delays the Link header preloading in order to be able to apply the
> ViewportDescription
>  - That last part better aligns with what I've seen devs do with preload (i.e.
> they usually don't want Link loaded resources to beat the critical resources
at
> the head of HTML)
>  - I believe it will resolve the double download issues as seen in
> https://bugs.chromium.org/p/chromium/issues/detail?id=589767
> 
> Let me know what you think

I'm having trouble figuring out a way to test the mix of Link headers and
viewport directives. We have a JS interface that can turn on meta viewport
support in the desktop tests, but by the time it runs, the Link headers would be
long processed.
Any ideas would be appreciated.

[Nate Chapin, 2016-02-29 20:45:55]

On 2016/02/26 16:05:44, Yoav Weiss wrote:
> On 2016/02/26 12:35:44, Yoav Weiss wrote:
> > Hey Nate! :)
> > 
> > Can you take a look? The CL here:
> > * Enables the media attribute on Link preloads (markup and headers)
> > * Gets a ViewportDescription from the HTMLPreloadScanner
> > * Delays the Link header preloading in order to be able to apply the
> > ViewportDescription
> >  - That last part better aligns with what I've seen devs do with preload
(i.e.
> > they usually don't want Link loaded resources to beat the critical resources
> at
> > the head of HTML)
> >  - I believe it will resolve the double download issues as seen in
> > https://bugs.chromium.org/p/chromium/issues/detail?id=589767
> > 
> > Let me know what you think
> 
> I'm having trouble figuring out a way to test the mix of Link headers and
> viewport directives. We have a JS interface that can turn on meta viewport
> support in the desktop tests, but by the time it runs, the Link headers would
be
> long processed.
> Any ideas would be appreciated.

Would it work to have a main frame that sets the testing state you need, then
run the test in an iframe or new window?

[Yoav Weiss, 2016-02-29 23:20:03]

On 2016/02/29 20:45:55, Nate Chapin wrote:
> On 2016/02/26 16:05:44, Yoav Weiss wrote:
> > On 2016/02/26 12:35:44, Yoav Weiss wrote:
> > > Hey Nate! :)
> > > 
> > > Can you take a look? The CL here:
> > > * Enables the media attribute on Link preloads (markup and headers)
> > > * Gets a ViewportDescription from the HTMLPreloadScanner
> > > * Delays the Link header preloading in order to be able to apply the
> > > ViewportDescription
> > >  - That last part better aligns with what I've seen devs do with preload
> (i.e.
> > > they usually don't want Link loaded resources to beat the critical
resources
> > at
> > > the head of HTML)
> > >  - I believe it will resolve the double download issues as seen in
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=589767
> > > 
> > > Let me know what you think
> > 
> > I'm having trouble figuring out a way to test the mix of Link headers and
> > viewport directives. We have a JS interface that can turn on meta viewport
> > support in the desktop tests, but by the time it runs, the Link headers
would
> be
> > long processed.
> > Any ideas would be appreciated.
> 
> Would it work to have a main frame that sets the testing state you need, then
> run the test in an iframe or new window?

Can an iframe set its own meta viewport?

Otherwise, a new window/tab might work. Didn't know content shell test mode can
run multiple tabs.

[Nate Chapin, 2016-02-29 23:23:35]

On 2016/02/29 23:20:03, Yoav Weiss wrote:
> On 2016/02/29 20:45:55, Nate Chapin wrote:
> > On 2016/02/26 16:05:44, Yoav Weiss wrote:
> > > On 2016/02/26 12:35:44, Yoav Weiss wrote:
> > > > Hey Nate! :)
> > > > 
> > > > Can you take a look? The CL here:
> > > > * Enables the media attribute on Link preloads (markup and headers)
> > > > * Gets a ViewportDescription from the HTMLPreloadScanner
> > > > * Delays the Link header preloading in order to be able to apply the
> > > > ViewportDescription
> > > >  - That last part better aligns with what I've seen devs do with preload
> > (i.e.
> > > > they usually don't want Link loaded resources to beat the critical
> resources
> > > at
> > > > the head of HTML)
> > > >  - I believe it will resolve the double download issues as seen in
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=589767
> > > > 
> > > > Let me know what you think
> > > 
> > > I'm having trouble figuring out a way to test the mix of Link headers and
> > > viewport directives. We have a JS interface that can turn on meta viewport
> > > support in the desktop tests, but by the time it runs, the Link headers
> would
> > be
> > > long processed.
> > > Any ideas would be appreciated.
> > 
> > Would it work to have a main frame that sets the testing state you need,
then
> > run the test in an iframe or new window?
> 
> Can an iframe set its own meta viewport?
> 
> Otherwise, a new window/tab might work. Didn't know content shell test mode
can
> run multiple tabs.

Yep, it should Just Work if you include:
        testRunner.setCanOpenWindows(true);
        testRunner.setCloseRemainingWindowsWhenComplete(true);

[Yoav Weiss, 2016-03-10 13:28:14]

On 2016/02/29 23:23:35, Nate Chapin wrote:
> On 2016/02/29 23:20:03, Yoav Weiss wrote:
> > On 2016/02/29 20:45:55, Nate Chapin wrote:
> > > On 2016/02/26 16:05:44, Yoav Weiss wrote:
> > > > On 2016/02/26 12:35:44, Yoav Weiss wrote:
> > > > > Hey Nate! :)
> > > > > 
> > > > > Can you take a look? The CL here:
> > > > > * Enables the media attribute on Link preloads (markup and headers)
> > > > > * Gets a ViewportDescription from the HTMLPreloadScanner
> > > > > * Delays the Link header preloading in order to be able to apply the
> > > > > ViewportDescription
> > > > >  - That last part better aligns with what I've seen devs do with
preload
> > > (i.e.
> > > > > they usually don't want Link loaded resources to beat the critical
> > resources
> > > > at
> > > > > the head of HTML)
> > > > >  - I believe it will resolve the double download issues as seen in
> > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=589767
> > > > > 
> > > > > Let me know what you think
> > > > 
> > > > I'm having trouble figuring out a way to test the mix of Link headers
and
> > > > viewport directives. We have a JS interface that can turn on meta
viewport
> > > > support in the desktop tests, but by the time it runs, the Link headers
> > would
> > > be
> > > > long processed.
> > > > Any ideas would be appreciated.
> > > 
> > > Would it work to have a main frame that sets the testing state you need,
> then
> > > run the test in an iframe or new window?
> > 
> > Can an iframe set its own meta viewport?
> > 
> > Otherwise, a new window/tab might work. Didn't know content shell test mode
> can
> > run multiple tabs.
> 
> Yep, it should Just Work if you include:
>         testRunner.setCanOpenWindows(true);
>         testRunner.setCloseRemainingWindowsWhenComplete(true);

The "open window" part works, but the meta enabling flag doesn't seem to be
transitive to the new window. So the new test is there, but it doesn't pass
locally for that reason. Testing the same manually with devtools emulation mode
works, though. Not sure what to do with that.

Otherwise, I changed the way viewport enabling works (setting the new viewport
on Document didn't help, at the values were not updated before layout).

[Nate Chapin, 2016-03-15 22:57:41]

Sorry for the delay, one last test idea...

https://codereview.chromium.org/1738133002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/preload/meta-viewport-link-headers.html
(right):

https://codereview.chromium.org/1738133002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/preload/meta-viewport-link-headers.html:24:
window.open("resources/media-link-headers.php");
I don't know if this would work, but: If you changed this to:
var w = window.open("resources/media-link-headers.php");
w.internals.settings.setViewportEnabled(true);
w.internals.settings.setViewportMetaEnabled(true);


...etc. Would that get the settings in the right state before the new window
commits media-link-headers.php?

[Yoav Weiss, 2016-03-18 12:36:12]

On 2016/03/15 22:57:41, Nate Chapin wrote:
> Sorry for the delay, one last test idea...
> 
>
https://codereview.chromium.org/1738133002/diff/40001/third_party/WebKit/Layo...
> File
>
third_party/WebKit/LayoutTests/http/tests/preload/meta-viewport-link-headers.html
> (right):
> 
>
https://codereview.chromium.org/1738133002/diff/40001/third_party/WebKit/Layo...
>
third_party/WebKit/LayoutTests/http/tests/preload/meta-viewport-link-headers.html:24:
> window.open("resources/media-link-headers.php");
> I don't know if this would work, but: If you changed this to:
> var w = window.open("resources/media-link-headers.php");
> w.internals.settings.setViewportEnabled(true);
> w.internals.settings.setViewportMetaEnabled(true);
> 
> 
> ...etc. Would that get the settings in the right state before the new window
> commits media-link-headers.php?

That worked!!! \o/

And the bots are (mostly) green and happy. Nate - can you take a look?

[Nate Chapin, 2016-03-18 19:28:02]

lgtm

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:502:
m_isLoadedLinkHeaders = true;
I have trouble parsing m_isLoadedLinkHeaders. Maybe m_triedLoadingLinkHeaders?

[sof, 2016-03-18 20:03:43]

dbc

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:496:
ViewportDescription oldDescription;
unused?

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h:48: struct
ViewportDescriptionWrapper {
Is there some alternative way of exposing this extra optional argument to
scan()? The added argument looks a bit unusual & quirky.

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:465: if (m_documentLoader)
{
Why isn't this null check needed any longer?

[sof, 2016-03-18 20:40:30]

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:465: if (m_documentLoader)
{
On 2016/03/18 20:03:42, sof wrote:
> Why isn't this null check needed any longer?

please ignore; failed to see the null check further up.

[Yoav Weiss, 2016-03-18 22:02:49]

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right):

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:496:
ViewportDescription oldDescription;
On 2016/03/18 20:03:42, sof wrote:
> unused?

yup, now gone

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:502:
m_isLoadedLinkHeaders = true;
On 2016/03/18 19:28:01, Nate Chapin wrote:
> I have trouble parsing m_isLoadedLinkHeaders. Maybe m_triedLoadingLinkHeaders?

renamed

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h (right):

https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h:48: struct
ViewportDescriptionWrapper {
On 2016/03/18 20:03:42, sof wrote:
> Is there some alternative way of exposing this extra optional argument to
> scan()? The added argument looks a bit unusual & quirky.

Any specific suggestions?

[Yoav Weiss, 2016-03-22 11:53:59]

On 2016/03/18 22:02:49, Yoav Weiss wrote:
> 
>
https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h (right):
> 
>
https://codereview.chromium.org/1738133002/diff/120001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.h:48: struct
> ViewportDescriptionWrapper {
> On 2016/03/18 20:03:42, sof wrote:
> > Is there some alternative way of exposing this extra optional argument to
> > scan()? The added argument looks a bit unusual & quirky.
> 
> Any specific suggestions?

sof - if it's OK with you, I prefer to go ahead and land this. Let's discuss
ways to improve ViewportDescriptorWrapper and I'll refactor it in a followup CL.

[Yoav Weiss, 2016-03-22 14:50:15]

The CQ bit was checked by yoav@yoav.ws to run a CQ dry run

[commit-bot: I haz the power, 2016-03-22 14:50:26]

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

[commit-bot: I haz the power, 2016-03-22 17:32:15]

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

[commit-bot: I haz the power, 2016-03-22 17:32:16]

Dry run: This issue passed the CQ dry run.

[Yoav Weiss, 2016-03-22 17:37:48]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2016-03-22 17:37:48]

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/1738133002/#ps140001
(title: "review nits")

[commit-bot: I haz the power, 2016-03-22 17:38:11]

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

[commit-bot: I haz the power, 2016-03-22 17:44:34]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-22 17:47:03]

Description was changed from

==========
Add support for media attribute on link

This CL adds support to the `media` attribute on link preload tags and
equivalent Link headers. It avoids preloading the resources when media
does not match.

In order to accurately do that, even when preload is in the headers, and
the actual viewport definition is in markup, this CL delays the Link
preload header processing until the prelodScanner processed the first chunk,
gets the ViewportDescription from the preloadScanner, and applies it
before determining if a resource should be preloaded.

BUG=590188
==========

to

==========
Add support for media attribute on link

This CL adds support to the `media` attribute on link preload tags and
equivalent Link headers. It avoids preloading the resources when media
does not match.

In order to accurately do that, even when preload is in the headers, and
the actual viewport definition is in markup, this CL delays the Link
preload header processing until the prelodScanner processed the first chunk,
gets the ViewportDescription from the preloadScanner, and applies it
before determining if a resource should be preloaded.

BUG=590188

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

[commit-bot: I haz the power, 2016-03-22 17:47:04]

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