Fix HTMLPreloadScanner handling of type in link preload.

Fix HTMLPreloadScanner handling of type in link preload.

Handling of the `type` attribute in `<link rel=preload>` was not
properly added, which meant preloads that included a valid type were
dismissed due to rel=stylesheet related logic.

This CL adds proper handling for the `type` attribute in
`<link rel=preload>` for fonts, images and styles.

BUG=659640
Review-Url: https://codereview.chromium.org/2620993002
Cr-Commit-Position: refs/heads/master@{#442887}
Committed: https://chromium.googlesource.com/chromium/src/+/5b86dfe66171ac6b9641ea596c74369e515de917

[Yoav Weiss, 2017-01-10 11:59:54]

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

[commit-bot: I haz the power, 2017-01-10 12:00:03]

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

[Yoav Weiss, 2017-01-10 12:04:21]

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

[Yoav Weiss, 2017-01-10 12:04:21]

Hey Charlie,

Can you take a look at that preload related HTMLPreloadScanner fix? Thanks! :)

[commit-bot: I haz the power, 2017-01-10 13:55:34]

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

[commit-bot: I haz the power, 2017-01-10 13:55:35]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-01-10 14:35:07]

Generally looks good, just some style nits.

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

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:208:
Optional<Resource::Type> type;
#include "wtf/Optional.h"

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:483: if
(!m_typeAttributeValue.isEmpty() &&
This inner if could be rewritten as

return m_typeAttributeValue.isEmpty() ||
MIMETypeRegistry::isSupportedStyleSheetMIMEType(ContentType(m_typeAttributeValue).type());

This way we avoid the implicit "return true" at the bottom of the method for
this condition. I also think it's more readable.

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:488: if
(!m_typeAttributeValue.isEmpty()) {
optional: It might make sense to also have an early return for
m_typeAttributeValue.isEmpty() at the top of the method, so we don't so many
need nested conditionals.

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:490: if (type
== Resource::Font &&
These three conditions could be combined to:

if (type == Resource::Font || type == Resource::Image || type ==
Resource::CSSStyleSheet) {
  return
MIMETypeRegistry::isSupportedStyleSheetMIMEType(ContentType(m_typeAttributeValue).type());
}

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:201: if (!as.isEmpty())
Can this condition be an "else if" in the outer else if chain?

[Yoav Weiss, 2017-01-11 05:09:44]

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

[commit-bot: I haz the power, 2017-01-11 05:10:10]

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

[Yoav Weiss, 2017-01-11 05:14:36]

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

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:208:
Optional<Resource::Type> type;
On 2017/01/10 14:35:06, Charlie Harrison wrote:
> #include "wtf/Optional.h"

Added

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:483: if
(!m_typeAttributeValue.isEmpty() &&
On 2017/01/10 14:35:06, Charlie Harrison wrote:
> This inner if could be rewritten as
> 
> return m_typeAttributeValue.isEmpty() ||
>
MIMETypeRegistry::isSupportedStyleSheetMIMEType(ContentType(m_typeAttributeValue).type());
> 
> This way we avoid the implicit "return true" at the bottom of the method for
> this condition. I also think it's more readable.

done

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:488: if
(!m_typeAttributeValue.isEmpty()) {
On 2017/01/10 14:35:06, Charlie Harrison wrote:
> optional: It might make sense to also have an early return for
> m_typeAttributeValue.isEmpty() at the top of the method, so we don't so many
> need nested conditionals.

done

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:490: if (type
== Resource::Font &&
On 2017/01/10 14:35:06, Charlie Harrison wrote:
> These three conditions could be combined to:
> 
> if (type == Resource::Font || type == Resource::Image || type ==
> Resource::CSSStyleSheet) {
>   return
>
MIMETypeRegistry::isSupportedStyleSheetMIMEType(ContentType(m_typeAttributeValue).type());
> }

Not really, as the isSupported* function call in each is a different one. Still
I tried to make the condition more compact.

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/loader/LinkLoader.cpp (right):

https://codereview.chromium.org/2620993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/loader/LinkLoader.cpp:201: if (!as.isEmpty())
On 2017/01/10 14:35:06, Charlie Harrison wrote:
> Can this condition be an "else if" in the outer else if chain?

Yeah (more or less). done

[commit-bot: I haz the power, 2017-01-11 06:33:36]

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

[commit-bot: I haz the power, 2017-01-11 06:33:37]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-01-11 13:04:29]

LGTM looks great

[Yoav Weiss, 2017-01-11 13:08:30]

On 2017/01/11 13:04:29, Charlie Harrison wrote:
> LGTM looks great

Thanks for reviewing! :)

[Yoav Weiss, 2017-01-11 13:08:35]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2017-01-11 13:08:48]

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, 2017-01-11 13:12:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484140115585440,
"parent_rev": "d3e6557ef017c1583d589907af48cf1761393abe", "commit_rev":
"5b86dfe66171ac6b9641ea596c74369e515de917"}

[commit-bot: I haz the power, 2017-01-11 13:12:57]

Description was changed from

==========
Fix HTMLPreloadScanner handling of type in link preload.

Handling of the `type` attribute in `<link rel=preload>` was not
properly added, which meant preloads that included a valid type were
dismissed due to rel=stylesheet related logic.

This CL adds proper handling for the `type` attribute in
`<link rel=preload>` for fonts, images and styles.

BUG=659640
==========

to

==========
Fix HTMLPreloadScanner handling of type in link preload.

Handling of the `type` attribute in `<link rel=preload>` was not
properly added, which meant preloads that included a valid type were
dismissed due to rel=stylesheet related logic.

This CL adds proper handling for the `type` attribute in
`<link rel=preload>` for fonts, images and styles.

BUG=659640

Review-Url: https://codereview.chromium.org/2620993002
Cr-Commit-Position: refs/heads/master@{#442887}
Committed:
https://chromium.googlesource.com/chromium/src/+/5b86dfe66171ac6b9641ea596c74...
==========

[commit-bot: I haz the power, 2017-01-11 13:12:59]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5b86dfe66171ac6b9641ea596c74...