Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2871593003:
DevTools: correctly parse srcset attribute before linkifying urls (Closed)
|
Created:
3 years, 7 months ago by luoe Modified:
3 years, 7 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: correctly parse srcset attribute before linkifying urls
The "srcset" attribute can take multiple image candidate strings, consisting of
a url and an optional width/density descriptor (e.g. 1x, 2w). DevTools used to
split the value on commas, which broke when valid srcsets had urls containing
commas.
This CL updates the linkifier to more closely resemble the rules for parsing
the attribute:
https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-srcset-attribute
BUG=671723
Review-Url: https://codereview.chromium.org/2871593003
Cr-Commit-Position: refs/heads/master@{#470624}
Committed: https://chromium.googlesource.com/chromium/src/+/c12dbdc00cf31c15ab5ac591c419aa27ad7b9033
Patch Set 1 #
Total comments: 9
Patch Set 2 : ac #
Messages
Total messages: 23 (11 generated)
Description was changed from ========== DevTools: correctly parse srcset attribute before linkifying urls BUG=671723 ========== to ========== DevTools: correctly parse srcset attribute before linkifying urls The "srcset" attribute can take multiple image candidate strings, consisting of a url and an optional width/density descriptor (e.g. 1x, 2w). DevTools used to split the value on commas, which broke when valid srcsets had urls containing commas. This CL updates the linkifier to more closely resemble the rules for parsing the attribute: https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-srcset-a... BUG=671723 ==========
luoe@chromium.org changed reviewers: + dgozman@chromium.org, lushnikov@chromium.org
Please take a look. https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/inspector/elements/elements-linkify-attributes-expected.txt (right): https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/inspector/elements/elements-linkify-attributes-expected.txt:3: a link No functional change on this line. It's only due to the line breaks in between the img's that we have 8 more spaces.
Looks good, but let's make the test readable! https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/inspector/elements/elements-linkify-attributes-expected.txt (right): https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/inspector/elements/elements-linkify-attributes-expected.txt:10: <span class="highlight"><span class="webkit-html-tag"><<span class="webkit-html-tag-name">img</span> <span class="webkit-html-attribute"><span class="webkit-html-attribute-name">id</span>="<span class="webkit-html-attribute-value">linkify-7</span>"</span> <span class="webkit-html-attribute"><span class="webkit-html-attribute-name">srcset</span>="<span class="webkit-html-attribute-value"><span class="devtools-link devtools-link-prevent-click">./kitten-medium.png,</span> <span class="devtools-link devtools-link-prevent-click">./kitten-large.png</span> 2x</span>"</span>></span></span> Can we rewrite this test to be readable? https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1285: if (node && (name === 'src' || name === 'href')) Use nodeName here. https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1315: var indexOfSpace = value.search(/\s/); Comment above calls this 'i'. Let's align https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1324: descriptor = value.substring(indexOfSpace, indexOfComma + 1); Should this be indexOfComma-1 ?
Changed the test to print out positions of links embedded at each tree element. Ptal https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/inspector/elements/elements-linkify-attributes-expected.txt (right): https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/inspector/elements/elements-linkify-attributes-expected.txt:10: <span class="highlight"><span class="webkit-html-tag"><<span class="webkit-html-tag-name">img</span> <span class="webkit-html-attribute"><span class="webkit-html-attribute-name">id</span>="<span class="webkit-html-attribute-value">linkify-7</span>"</span> <span class="webkit-html-attribute"><span class="webkit-html-attribute-name">srcset</span>="<span class="webkit-html-attribute-value"><span class="devtools-link devtools-link-prevent-click">./kitten-medium.png,</span> <span class="devtools-link devtools-link-prevent-click">./kitten-large.png</span> 2x</span>"</span>></span></span> On 2017/05/08 23:37:40, dgozman wrote: > Can we rewrite this test to be readable? Done. https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js (right): https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1285: if (node && (name === 'src' || name === 'href')) On 2017/05/08 23:37:40, dgozman wrote: > Use nodeName here. Done. https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1315: var indexOfSpace = value.search(/\s/); On 2017/05/08 23:37:40, dgozman wrote: > Comment above calls this 'i'. Let's align Done. https://codereview.chromium.org/2871593003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/elements/ElementsTreeElement.js:1324: descriptor = value.substring(indexOfSpace, indexOfComma + 1); On 2017/05/08 23:37:40, dgozman wrote: > Should this be indexOfComma-1 ? No, I actually want to include the comma in the `descriptor`. "foo 1x,bar 2x" => url: "foo", descriptor: " 1x,"
lgtm
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by luoe@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": 1494435748887910,
"parent_rev": "e834f7f1b6aff300392163df783071f8a9a6ec56", "commit_rev":
"c12dbdc00cf31c15ab5ac591c419aa27ad7b9033"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: correctly parse srcset attribute before linkifying urls The "srcset" attribute can take multiple image candidate strings, consisting of a url and an optional width/density descriptor (e.g. 1x, 2w). DevTools used to split the value on commas, which broke when valid srcsets had urls containing commas. This CL updates the linkifier to more closely resemble the rules for parsing the attribute: https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-srcset-a... BUG=671723 ========== to ========== DevTools: correctly parse srcset attribute before linkifying urls The "srcset" attribute can take multiple image candidate strings, consisting of a url and an optional width/density descriptor (e.g. 1x, 2w). DevTools used to split the value on commas, which broke when valid srcsets had urls containing commas. This CL updates the linkifier to more closely resemble the rules for parsing the attribute: https://html.spec.whatwg.org/multipage/embedded-content.html#parse-a-srcset-a... BUG=671723 Review-Url: https://codereview.chromium.org/2871593003 Cr-Commit-Position: refs/heads/master@{#470624} Committed: https://chromium.googlesource.com/chromium/src/+/c12dbdc00cf31c15ab5ac591c419... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/c12dbdc00cf31c15ab5ac591c419... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
