Don't trigger a link resource fetch on media change.

Don't trigger a link resource fetch on media change.

Changing the media attribute called process() which would clear the
current stylesheet and re-fetch it. If a stylesheet is loaded, set the
media queries on that stylesheet and trigger style changes it may cause.
If loading is in progress, the media queries will be set on the
stylesheet when it finishes loading.

Note that setting the media attribute will not change the blocking state
if changed while the sheet is loading. However, we do not change the
blocking state if the mq evaluation change during loading.

If the blocking state needs to happen we need to do pending stylesheet
changes like we do for setDisabledState.

R=horo@chromium.org,tkent@chromium.org
BUG=591654

[rune, 2016-03-13 00:07:08]

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

[rune, 2016-03-13 00:07:18]

ptal

[commit-bot: I haz the power, 2016-03-13 00:07:22]

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

[commit-bot: I haz the power, 2016-03-13 01:05:05]

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

[commit-bot: I haz the power, 2016-03-13 01:05:06]

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_...)

[rune, 2016-03-13 09:45:56]

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

[commit-bot: I haz the power, 2016-03-13 09:46:04]

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

[rune, 2016-03-13 09:49:36]

Since we don't process() on media changes anymore, and stylesheet links are the
only rel types which are affected by media in Blink, the manifest change
dispatching test count is reduced.

[commit-bot: I haz the power, 2016-03-13 10:53:56]

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

[commit-bot: I haz the power, 2016-03-13 10:53:57]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-03-13 23:53:48]

tkent@chromium.org changed reviewers:
+ kochi@chromium.org

[tkent, 2016-03-13 23:54:49]

I don't think I'm a right reviewer of this CL.  kochi@, are you a right person,
or do you know a right person?

[tkent, 2016-03-13 23:55:06]

tkent@chromium.org changed reviewers:
- tkent@chromium.org

[kochi, 2016-03-14 05:59:33]

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

[kochi, 2016-03-14 05:59:34]

The change and tests lgtm but I may not be a good reviewer
for link element and resource loading part.
Yoav, could you take a look?

https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/tests/data/link-manifest-change.html (right):

https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/tests/data/link-manifest-change.html:35: // 1
times (= 9)
s/times/time/ ?

[Yoav Weiss, 2016-03-14 15:23:51]

Are you seeing spurious downloads when you change other attributes on `<link
rel=stylesheet>`? (seems like you should, at least for type, href, etc)

https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right):

https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:196:
m_link->mediaChanged();
Seems to me that we would want "process()" not to trigger a second download if
it's not necessary rather than (or on top of) handling this `media` specific
case.

Maybe "process()" can check current URL and see if it is changed since it was
fetched? (and perhaps special-case media changes as to trigger mediaChanged
rather than a download? Not sure it's necessary)

Long term, I think we may want to move the loading code from here and into
loader (maybe split LinkLoader into LinkStyleLoader, LinkPreloadLoader, etc?)

[horo, 2016-03-15 03:25:47]

Looks good to me. But I'm not familiar enough with HTMLLinkElement's behavior
details.
Please follow yoav@.

[rune, 2016-04-07 07:56:07]

https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right):

https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:196:
m_link->mediaChanged();
On 2016/03/14 15:23:51, Yoav Weiss wrote:
> Seems to me that we would want "process()" not to trigger a second download if
> it's not necessary rather than (or on top of) handling this `media` specific
> case.
> 
> Maybe "process()" can check current URL and see if it is changed since it was
> fetched? (and perhaps special-case media changes as to trigger mediaChanged
> rather than a download? Not sure it's necessary)

Yes, mediaChanged() is necessary to trigger a style update. Having process()
checking attributes without given the knowledge of what changed sounds
cumbersome. Instead of using the process method, we could have an
attributeChanged() for each of the LinkResource types. If there is no
LinkResources, call process(). If the rel type changed in such a way that the
LinkResource type should change, dispose the old one and create a new one with
process().

This code seems broken in so many ways and needs an overhaul. A couple of
issues:

- If we create a LinkResource for one rel-type, it cannot be changed afterwards
(works in Gecko).
- If we set the any of the known attributes to the same value ten times, it will
trigger ten resource loads.

If my media specific fix is not acceptable, I'll defer this to loading-dev
people for now as I need to focus elsewhere.

> Long term, I think we may want to move the loading code from here and into
> loader (maybe split LinkLoader into LinkStyleLoader, LinkPreloadLoader, etc?)

[Yoav Weiss, 2016-04-07 09:51:10]

On 2016/04/07 07:56:07, rune wrote:
>
https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right):
> 
>
https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:196:
> m_link->mediaChanged();
> On 2016/03/14 15:23:51, Yoav Weiss wrote:
> > Seems to me that we would want "process()" not to trigger a second download
if
> > it's not necessary rather than (or on top of) handling this `media` specific
> > case.
> > 
> > Maybe "process()" can check current URL and see if it is changed since it
was
> > fetched? (and perhaps special-case media changes as to trigger mediaChanged
> > rather than a download? Not sure it's necessary)
> 
> Yes, mediaChanged() is necessary to trigger a style update. Having process()
> checking attributes without given the knowledge of what changed sounds
> cumbersome. Instead of using the process method, we could have an
> attributeChanged() for each of the LinkResource types. If there is no
> LinkResources, call process(). If the rel type changed in such a way that the
> LinkResource type should change, dispose the old one and create a new one with
> process().
> 
> This code seems broken in so many ways and needs an overhaul. 

I agree.

> A couple of
> issues:
> 
> - If we create a LinkResource for one rel-type, it cannot be changed
afterwards
> (works in Gecko).
> - If we set the any of the known attributes to the same value ten times, it
will
> trigger ten resource loads.
> 
> If my media specific fix is not acceptable, I'll defer this to loading-dev
> people for now as I need to focus elsewhere.
> 

Fair enough. I'll take over the issue (and probably reuse your tests)

[rune, 2016-04-07 10:21:18]

On 2016/04/07 09:51:10, Yoav Weiss wrote:
> On 2016/04/07 07:56:07, rune wrote:
> >
>
https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/html/HTMLLinkElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1793933002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/HTMLLinkElement.cpp:196:
> > m_link->mediaChanged();
> > On 2016/03/14 15:23:51, Yoav Weiss wrote:
> > > Seems to me that we would want "process()" not to trigger a second
download
> if
> > > it's not necessary rather than (or on top of) handling this `media`
specific
> > > case.
> > > 
> > > Maybe "process()" can check current URL and see if it is changed since it
> was
> > > fetched? (and perhaps special-case media changes as to trigger
mediaChanged
> > > rather than a download? Not sure it's necessary)
> > 
> > Yes, mediaChanged() is necessary to trigger a style update. Having process()
> > checking attributes without given the knowledge of what changed sounds
> > cumbersome. Instead of using the process method, we could have an
> > attributeChanged() for each of the LinkResource types. If there is no
> > LinkResources, call process(). If the rel type changed in such a way that
the
> > LinkResource type should change, dispose the old one and create a new one
with
> > process().
> > 
> > This code seems broken in so many ways and needs an overhaul. 
> 
> I agree.
> 
> > A couple of
> > issues:
> > 
> > - If we create a LinkResource for one rel-type, it cannot be changed
> afterwards
> > (works in Gecko).
> > - If we set the any of the known attributes to the same value ten times, it
> will
> > trigger ten resource loads.
> > 
> > If my media specific fix is not acceptable, I'll defer this to loading-dev
> > people for now as I need to focus elsewhere.
> > 
> 
> Fair enough. I'll take over the issue (and probably reuse your tests)

Thanks. closing.

[rune, 2016-04-07 10:21:30]

Description was changed from

==========
Don't trigger a link resource fetch on media change.

Changing the media attribute called process() which would clear the
current stylesheet and re-fetch it. If a stylesheet is loaded, set the
media queries on that stylesheet and trigger style changes it may cause.
If loading is in progress, the media queries will be set on the
stylesheet when it finishes loading.

Note that setting the media attribute will not change the blocking state
if changed while the sheet is loading. However, we do not change the
blocking state if the mq evaluation change during loading.

If the blocking state needs to happen we need to do pending stylesheet
changes like we do for setDisabledState.

R=horo@chromium.org,tkent@chromium.org
BUG=591654
==========

to

==========
Don't trigger a link resource fetch on media change.

Changing the media attribute called process() which would clear the
current stylesheet and re-fetch it. If a stylesheet is loaded, set the
media queries on that stylesheet and trigger style changes it may cause.
If loading is in progress, the media queries will be set on the
stylesheet when it finishes loading.

Note that setting the media attribute will not change the blocking state
if changed while the sheet is loading. However, we do not change the
blocking state if the mq evaluation change during loading.

If the blocking state needs to happen we need to do pending stylesheet
changes like we do for setDisabledState.

R=horo@chromium.org,tkent@chromium.org
BUG=591654
==========