Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2125143002:
[Devtools] Improved URL parser for console (Closed)
|
Created:
4 years, 5 months ago by allada Modified:
4 years, 5 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, sergeyv+blink_chromium.org, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Improved URL parser for console
URL linkifier in console will now match a bit smarter. It
will now allow things in double quotes single quotes and tick
characters with something that beings to look like a url and
continue to match (ie: allow spaces and punctuation). If
there is no quote characters it will require punctuation or
space before and after something that looks like a url.
BUG=535463
Committed: https://crrev.com/33b11974ba7343fbf2ad6673447488a1563cc8e3
Cr-Commit-Position: refs/heads/master@{#404724}
Patch Set 1 #Patch Set 2 : Better url parser for console #
Total comments: 12
Patch Set 3 : wq #
Total comments: 2
Patch Set 4 : Better url parser for console #Patch Set 5 : bug fix in test #Patch Set 6 : fixed test bug #
Messages
Total messages: 42 (23 generated)
Description was changed from ========== Better url parser for console BUG= ========== to ========== [Devtools] Improved URL parser for console URL linkifier in console will now match a bit smarter. It will now allow things in double quotes single quotes and tick characters with something that beings to look like a url and continue to match (ie: allow spaces and punctuation). If there is no quote characters it will require punctuation or space before and after something that looks like a url. BUG=535463 ==========
allada@chromium.org changed reviewers: + luoe@chromium.org, lushnikov@chromium.org
PTL
allada@chromium.org changed reviewers: + dgozman@chromium.org
Can you please format the issue description so that lines don't span more then 80 chars? https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:484: + "]*|" + mayContainPunctuationIfNoSpaceAfter + "|" + encodedURIPattern + ")*)?"; nit: indent https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:487: var dataURI = "data:[^,]*," nit:semicolon is missing here and below https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:499: var firstChar = linkString.substr(0, 1); linkString[0] https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:501: match.index += 1; why do we truncate only from beginning? What about closing quote? https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:504: if (match[1]) { What is match[1]? Is this another group captured by the regex? If yes, can you please extract it into a named variable? https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:505: // Unmatches/removes beginInPunctuationOrSpace from match nit: comments should end with dot
Looks good. Can we make this regex a constant and just build it once outside this function?
On 2016/07/06 23:00:23, luoe wrote:
> Looks good. Can we make this regex a constant and just build it once outside
> this function?
Hmm, can we update this to fix this case too:
"'data:text/html,alert('a')'"
?
Why do we do this?
Description was changed from ========== [Devtools] Improved URL parser for console URL linkifier in console will now match a bit smarter. It will now allow things in double quotes single quotes and tick characters with something that beings to look like a url and continue to match (ie: allow spaces and punctuation). If there is no quote characters it will require punctuation or space before and after something that looks like a url. BUG=535463 ========== to ========== [Devtools] Improved URL parser for console URL linkifier in console will now match a bit smarter. It will now allow things in double quotes single quotes and tick characters with something that beings to look like a url and continue to match (ie: allow spaces and punctuation). If there is no quote characters it will require punctuation or space before and after something that looks like a url. BUG=535463 ==========
PTL https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:484: + "]*|" + mayContainPunctuationIfNoSpaceAfter + "|" + encodedURIPattern + ")*)?"; On 2016/07/06 22:56:56, lushnikov wrote: > nit: indent Done. https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:487: var dataURI = "data:[^,]*," On 2016/07/06 22:56:56, lushnikov wrote: > nit:semicolon is missing here and below Done. https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:499: var firstChar = linkString.substr(0, 1); On 2016/07/06 22:56:56, lushnikov wrote: > linkString[0] Done. https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:501: match.index += 1; On 2016/07/06 22:56:56, lushnikov wrote: > why do we truncate only from beginning? What about closing quote? Because it uses lookaheads in the regex instead of actually consuming them. https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:504: if (match[1]) { On 2016/07/06 22:56:56, lushnikov wrote: > What is match[1]? Is this another group captured by the regex? If yes, can you > please extract it into a named variable? Done. https://codereview.chromium.org/2125143002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:505: // Unmatches/removes beginInPunctuationOrSpace from match On 2016/07/06 22:56:56, lushnikov wrote: > nit: comments should end with dot Done.
lgtm https://codereview.chromium.org/2125143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2125143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:504: var ensureNonAlphaNumericFirstCharacter = match[1] nit: nonAlphaNumbericFirstCharacter (Variables should be nouns)
allada@google.com changed reviewers: + allada@google.com
done https://codereview.chromium.org/2125143002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2125143002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:504: var ensureNonAlphaNumericFirstCharacter = match[1] On 2016/07/08 17:56:55, lushnikov wrote: > nit: nonAlphaNumbericFirstCharacter > > (Variables should be nouns) Done.
The CQ bit was checked by allada@google.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by allada@google.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2125143002/#ps100001 (title: "fixed test bug")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by allada@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by allada@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by allada@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Improved URL parser for console URL linkifier in console will now match a bit smarter. It will now allow things in double quotes single quotes and tick characters with something that beings to look like a url and continue to match (ie: allow spaces and punctuation). If there is no quote characters it will require punctuation or space before and after something that looks like a url. BUG=535463 ========== to ========== [Devtools] Improved URL parser for console URL linkifier in console will now match a bit smarter. It will now allow things in double quotes single quotes and tick characters with something that beings to look like a url and continue to match (ie: allow spaces and punctuation). If there is no quote characters it will require punctuation or space before and after something that looks like a url. BUG=535463 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Improved URL parser for console URL linkifier in console will now match a bit smarter. It will now allow things in double quotes single quotes and tick characters with something that beings to look like a url and continue to match (ie: allow spaces and punctuation). If there is no quote characters it will require punctuation or space before and after something that looks like a url. BUG=535463 ========== to ========== [Devtools] Improved URL parser for console URL linkifier in console will now match a bit smarter. It will now allow things in double quotes single quotes and tick characters with something that beings to look like a url and continue to match (ie: allow spaces and punctuation). If there is no quote characters it will require punctuation or space before and after something that looks like a url. BUG=535463 Committed: https://crrev.com/33b11974ba7343fbf2ad6673447488a1563cc8e3 Cr-Commit-Position: refs/heads/master@{#404724} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/33b11974ba7343fbf2ad6673447488a1563cc8e3 Cr-Commit-Position: refs/heads/master@{#404724}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2151943002/ by allada@chromium.org. The reason for reverting is: Regex can lockup on some console messages.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
