Avoid downloading unsupported `<source>` types in HTMLPreloadScanner

Avoid downloading unsupported `<source>` types in HTMLPreloadScanner

The preloadScanner was picking non-matching sources as it was ignoring the `type` attribute.
That resulted in unnecessary download. This CL fixes that.

BUG=615473
Committed: https://crrev.com/56bef09698c3f96a3b8a0f776f4d964bb49bb2a3
Cr-Commit-Position: refs/heads/master@{#396742}

[Yoav Weiss, 2016-05-30 07:53:00]

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

[Yoav Weiss, 2016-05-30 07:53:00]

Hey Mike and Kouhei,

Could you take a look at this patch? Apparently when I added `type` support for
`<picture>` I forgot to add the related preloadScanner support :/
As long as the first resource is one that Chrome supports (e.g. most examples
start with webp) that's not an issue, but if the first is not supported, we have
a spurious download of an unneeded resource...

Could you please review?

[Mike West, 2016-05-30 09:11:52]

lgtm

[Yoav Weiss, 2016-05-30 09:13:11]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2016-05-30 09:13:31]

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

[kouhei (in TOK), 2016-05-30 10:44:31]

https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:214: if
(!shouldPreload() || !m_matched) {
!m_matched check should go into shouldPreload()

https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:465: bool
m_matched;
Can we use separate flags for those two. Instead of sharing the same bool?

[Yoav Weiss, 2016-05-30 10:55:55]

The CQ bit was unchecked by yoav@yoav.ws

[Yoav Weiss, 2016-05-30 11:17:36]

https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:214: if
(!shouldPreload() || !m_matched) {
On 2016/05/30 10:44:31, kouhei (catching up) wrote:
> !m_matched check should go into shouldPreload()

fair enough

https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:465: bool
m_matched;
On 2016/05/30 10:44:31, kouhei (catching up) wrote:
> Can we use separate flags for those two. Instead of sharing the same bool?

We could but why? Both `media` and `type` impact whether this resource is a
match, and if it isn't, it shouldn't be preloaded. What would breaking it up
give us?

[kouhei (in TOK), 2016-05-30 11:26:22]

https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right):

https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:465: bool
m_matched;
On 2016/05/30 11:17:36, Yoav Weiss wrote:
> On 2016/05/30 10:44:31, kouhei (catching up) wrote:
> > Can we use separate flags for those two. Instead of sharing the same bool?
> 
> We could but why? Both `media` and `type` impact whether this resource is a
> match, and if it isn't, it shouldn't be preloaded. What would breaking it up
> give us?

I thought it was more verbose.
The current processLinkAttribute() implementation worried me a bit, as it used
"m_matched = " which is correct, but inconsistent w/ lines 34{4,6}.
I may be worrying too much. I'm ok with your original here.

[Yoav Weiss, 2016-05-30 11:26:42]

On 2016/05/30 11:17:36, Yoav Weiss wrote:
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
(right):
> 
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:214: if
> (!shouldPreload() || !m_matched) {
> On 2016/05/30 10:44:31, kouhei (catching up) wrote:
> > !m_matched check should go into shouldPreload()
> 
> fair enough
> 
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:465: bool
> m_matched;
> On 2016/05/30 10:44:31, kouhei (catching up) wrote:
> > Can we use separate flags for those two. Instead of sharing the same bool?
> 
> We could but why? Both `media` and `type` impact whether this resource is a
> match, and if it isn't, it shouldn't be preloaded. What would breaking it up
> give us?

Going over that patch once more, I see that for <link> we do have two separate
booleans. I'm now uploading a patch that matches the <link> logic to the one I
uploaded earlier (so using a single bool)

In my view, it is clearer, but let me know if you think otherwise :)

[Yoav Weiss, 2016-05-30 11:49:47]

On 2016/05/30 11:26:22, kouhei (catching up) wrote:
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
(right):
> 
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:465: bool
> m_matched;
> On 2016/05/30 11:17:36, Yoav Weiss wrote:
> > On 2016/05/30 10:44:31, kouhei (catching up) wrote:
> > > Can we use separate flags for those two. Instead of sharing the same bool?
> > 
> > We could but why? Both `media` and `type` impact whether this resource is a
> > match, and if it isn't, it shouldn't be preloaded. What would breaking it up
> > give us?
> 
> I thought it was more verbose.
> The current processLinkAttribute() implementation worried me a bit, as it used
> "m_matched = " which is correct, but inconsistent w/ lines 34{4,6}.
> I may be worrying too much. I'm ok with your original here.

Yeah, the processLinkAttribute bits were inconsistent, and I now changed them to
be consistent with m_matched (while removing an extra boolean).

[kouhei (in TOK), 2016-05-30 13:15:39]

On 2016/05/30 11:49:47, Yoav Weiss wrote:
> On 2016/05/30 11:26:22, kouhei (catching up) wrote:
> >
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:465: bool
> > m_matched;
> > On 2016/05/30 11:17:36, Yoav Weiss wrote:
> > > On 2016/05/30 10:44:31, kouhei (catching up) wrote:
> > > > Can we use separate flags for those two. Instead of sharing the same
bool?
> > > 
> > > We could but why? Both `media` and `type` impact whether this resource is
a
> > > match, and if it isn't, it shouldn't be preloaded. What would breaking it
up
> > > give us?
> > 
> > I thought it was more verbose.
> > The current processLinkAttribute() implementation worried me a bit, as it
used
> > "m_matched = " which is correct, but inconsistent w/ lines 34{4,6}.
> > I may be worrying too much. I'm ok with your original here.
> 
> Yeah, the processLinkAttribute bits were inconsistent, and I now changed them
to
> be consistent with m_matched (while removing an extra boolean).

Would you upload the change?

[Yoav Weiss, 2016-05-30 13:16:54]

On 2016/05/30 13:15:39, kouhei (catching up) wrote:
> On 2016/05/30 11:49:47, Yoav Weiss wrote:
> > On 2016/05/30 11:26:22, kouhei (catching up) wrote:
> > >
> >
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2018353002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:465:
bool
> > > m_matched;
> > > On 2016/05/30 11:17:36, Yoav Weiss wrote:
> > > > On 2016/05/30 10:44:31, kouhei (catching up) wrote:
> > > > > Can we use separate flags for those two. Instead of sharing the same
> bool?
> > > > 
> > > > We could but why? Both `media` and `type` impact whether this resource
is
> a
> > > > match, and if it isn't, it shouldn't be preloaded. What would breaking
it
> up
> > > > give us?
> > > 
> > > I thought it was more verbose.
> > > The current processLinkAttribute() implementation worried me a bit, as it
> used
> > > "m_matched = " which is correct, but inconsistent w/ lines 34{4,6}.
> > > I may be worrying too much. I'm ok with your original here.
> > 
> > Yeah, the processLinkAttribute bits were inconsistent, and I now changed
them
> to
> > be consistent with m_matched (while removing an extra boolean).
> 
> Would you upload the change?

I did. That's what Patch Set #2 is all about.

[kouhei (in TOK), 2016-05-30 15:15:27]

lgtm % layout tests failure

[Yoav Weiss, 2016-05-30 15:49:01]

On 2016/05/30 15:15:27, kouhei (catching up) wrote:
> lgtm % layout tests failure

test failures were legitimate and caught failure to preload CSS. Fixed in latest
patch.

[Yoav Weiss, 2016-05-30 16:53:40]

The CQ bit was checked by yoav@yoav.ws

[Yoav Weiss, 2016-05-30 16:53:41]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2018353002/#ps40001
(title: "Fix CSS preloading issue tests revealed")

[commit-bot: I haz the power, 2016-05-30 16:53:48]

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

[commit-bot: I haz the power, 2016-05-30 17:39:40]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-30 17:40:41]

Description was changed from

==========
Avoid downloading unsupported `<source>` types in HTMLPreloadScanner

The preloadScanner was picking non-matching sources as it was ignoring the
`type` attribute.
That resulted in unnecessary download. This CL fixes that.

BUG=615473
==========

to

==========
Avoid downloading unsupported `<source>` types in HTMLPreloadScanner

The preloadScanner was picking non-matching sources as it was ignoring the
`type` attribute.
That resulted in unnecessary download. This CL fixes that.

BUG=615473

Committed: https://crrev.com/56bef09698c3f96a3b8a0f776f4d964bb49bb2a3
Cr-Commit-Position: refs/heads/master@{#396742}
==========

[commit-bot: I haz the power, 2016-05-30 17:40:42]

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