Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496343002:
Support as=<UPPERCASE VALUE> for link preloads in preload scanner (Closed)
|
Created:
4 years, 1 month ago by Charlie Harrison Modified:
4 years ago Reviewers:
Yoav Weiss CC:
chromium-reviews, blink-reviews-html_chromium.org, loading-reviews+parser_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport as=<UPPERCASE VALUE> for link preloads in preload scanner
The LinkLoader code assumes all link preloads will have lowercased
'as' values. This is not the case for preloads coming in via the preload
scanner.
This patch fixes that issue and adds a unit test that reproduces this
failure.
BUG=664744
Committed: https://crrev.com/50ccef041685519550b26723c1e6bec74b9f9fc3
Cr-Commit-Position: refs/heads/master@{#434978}
Patch Set 1 #Patch Set 2 : add another test case #
Messages
Total messages: 25 (14 generated)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Support as=<uppercase as value> for link preloads in preload scanner The LinkLoader code assumes all link preloads will have lowercased 'as' values. This is not the case for preloads coming in via the preload scanner. This patch fixes that issue and adds a unit test that reproduces this failure. BUG=664744 ========== to ========== Support as=<UPPERCASE VALUE> for link preloads in preload scanner The LinkLoader code assumes all link preloads will have lowercased 'as' values. This is not the case for preloads coming in via the preload scanner. This patch fixes that issue and adds a unit test that reproduces this failure. BUG=664744 ==========
csharrison@chromium.org changed reviewers: + yoav@yoav.ws
Yoav, PTAL. Low priority as this is just a bad DCHECK bug with no real implications in release builds.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Yoav: friendly ping
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/28 17:33:21, Charlie Harrison wrote: > Yoav: friendly ping Sorry, missed this one.. We were actually talking about changing the behavior, and change LinkLoader to only accept lower case values. I need to send an email to blink-dev asking for guidance on that. Since this is not urgent, let's punt this until resolution on that front?
On 2016/11/28 19:58:48, Yoav Weiss wrote: > On 2016/11/28 17:33:21, Charlie Harrison wrote: > > Yoav: friendly ping > > Sorry, missed this one.. > We were actually talking about changing the behavior, and change LinkLoader to > only accept lower case values. > > I need to send an email to blink-dev asking for guidance on that. > > Since this is not urgent, let's punt this until resolution on that front? Happy to punt, but I'm not sure I'm following what you're saying. Are you thinking of disallowing uppercase as values supplied by the user in all cases?
On 2016/11/28 20:03:08, Charlie Harrison wrote: > On 2016/11/28 19:58:48, Yoav Weiss wrote: > > On 2016/11/28 17:33:21, Charlie Harrison wrote: > > > Yoav: friendly ping > > > > Sorry, missed this one.. > > We were actually talking about changing the behavior, and change LinkLoader to > > only accept lower case values. > > > > I need to send an email to blink-dev asking for guidance on that. > > > > Since this is not urgent, let's punt this until resolution on that front? > Happy to punt, but I'm not sure I'm following what you're saying. Are you > thinking of disallowing uppercase as values supplied by the user in all cases? That's where the spec seems to be going... https://github.com/whatwg/html/issues/1665
Thanks for the link! Maybe we should just remove the DCHECK in LinkLoader in the meantime, so we don't have bad DCHECKs lying around. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/28 21:20:54, Charlie Harrison wrote: > Thanks for the link! Maybe we should just remove the DCHECK in LinkLoader in the > meantime, so we don't have bad DCHECKs lying around. WDYT? On second thought, let's land this and revert if needed due to the spec changes. It's better to have preloadScanner and LinkLoader aligned. LGTM
On 2016/11/29 11:19:34, Yoav Weiss wrote: > On 2016/11/28 21:20:54, Charlie Harrison wrote: > > Thanks for the link! Maybe we should just remove the DCHECK in LinkLoader in > the > > meantime, so we don't have bad DCHECKs lying around. WDYT? > > On second thought, let's land this and revert if needed due to the spec changes. > It's better to have preloadScanner and LinkLoader aligned. > LGTM I'm fine with that too, landing!
The CQ bit was checked by csharrison@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1480427291444050,
"parent_rev": "631da07933d8f688075eeca2bd781b0f703d362f", "commit_rev":
"4cbdacbb0996980d8b686276d8d25fbe131087c3"}
Message was sent while issue was closed.
Description was changed from ========== Support as=<UPPERCASE VALUE> for link preloads in preload scanner The LinkLoader code assumes all link preloads will have lowercased 'as' values. This is not the case for preloads coming in via the preload scanner. This patch fixes that issue and adds a unit test that reproduces this failure. BUG=664744 ========== to ========== Support as=<UPPERCASE VALUE> for link preloads in preload scanner The LinkLoader code assumes all link preloads will have lowercased 'as' values. This is not the case for preloads coming in via the preload scanner. This patch fixes that issue and adds a unit test that reproduces this failure. BUG=664744 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Support as=<UPPERCASE VALUE> for link preloads in preload scanner The LinkLoader code assumes all link preloads will have lowercased 'as' values. This is not the case for preloads coming in via the preload scanner. This patch fixes that issue and adds a unit test that reproduces this failure. BUG=664744 ========== to ========== Support as=<UPPERCASE VALUE> for link preloads in preload scanner The LinkLoader code assumes all link preloads will have lowercased 'as' values. This is not the case for preloads coming in via the preload scanner. This patch fixes that issue and adds a unit test that reproduces this failure. BUG=664744 Committed: https://crrev.com/50ccef041685519550b26723c1e6bec74b9f9fc3 Cr-Commit-Position: refs/heads/master@{#434978} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/50ccef041685519550b26723c1e6bec74b9f9fc3 Cr-Commit-Position: refs/heads/master@{#434978} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
