DevTools: linkifier incorrectly matches port numbers in urls as line numbers

DevTools: linkifier incorrectly matches port numbers in urls as line numbers

When logging a URL with a port number in the console, 'http://localhost:8000',
our linkifier incorrectly treats the '8000' port number as a line number.

Using our existing granular URL regexes, this patch makes the linkifier ignore
the URL part before the 'path' when looking for a line/column number.

BUG=701865
Review-Url: https://codereview.chromium.org/2843073003
Cr-Commit-Position: refs/heads/master@{#467868}
Committed: https://chromium.googlesource.com/chromium/src/+/201eaf7de7e46e23f5955dc8b57db0d76add14c2

[luoe, 2017-04-26 19:21:06]

Description was changed from

==========
DevTools: linkifier incorrectly matches port numbers in urls as line numbers

BUG=701865
==========

to

==========
DevTools: linkifier incorrectly matches port numbers in urls as line numbers

When logging a URL with a port number in the console, 'http://localhost:8000',
our linkifier incorrectly treats the '8000' port number as a line number.

Using our existing granular URL regexes, this patch makes the linkifier ignore
the URL part before the 'path' when looking for a line/column number.

BUG=701865
==========

[luoe, 2017-04-26 19:31:31]

Patchset #1 (id:1) has been deleted

[luoe, 2017-04-26 21:26:54]

Patchset #2 (id:40001) has been deleted

[luoe, 2017-04-26 21:27:22]

luoe@chromium.org changed reviewers:
+ allada@chromium.org, dgozman@chromium.org

[luoe, 2017-04-26 21:27:23]

Please take a look

[alph, 2017-04-26 22:00:05]

alph@chromium.org changed reviewers:
+ alph@chromium.org

[alph, 2017-04-26 22:00:06]

Looks like it's possible to make a test.

https://codereview.chromium.org/2843073003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/ParsedURL.js (right):

https://codereview.chromium.org/2843073003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/ParsedURL.js:215: var
beforePathMatch = string.match(Common.ParsedURL._urlRegex());
Can't you just use lastPathComponentWithFragment() instead?

[luoe, 2017-04-26 23:45:05]

Test added!

https://codereview.chromium.org/2843073003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/common/ParsedURL.js (right):

https://codereview.chromium.org/2843073003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/common/ParsedURL.js:215: var
beforePathMatch = string.match(Common.ParsedURL._urlRegex());
On 2017/04/26 22:00:05, alph wrote:
> Can't you just use lastPathComponentWithFragment() instead?

It doesn't include queryParams, but we could add it.  Even with the
lastPathComponentWithFragment, I think the new regex group makes it simpler to
assemble the string before it.  Without adding a new regex group, we'd need to
introduce something like stringBeforeLastPathComponent =
`match[2] + '://' + match[3] + (match[4] ? (':' + match[4]) : match[4]) +
match[5];`

If patching the old regex with a new matching group isn't good, wdyt about
making a new regex just for this case?

[dgozman, 2017-04-27 17:03:29]

Nice patch!

https://codereview.chromium.org/2843073003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/parse-urls-line-column.js
(right):

https://codereview.chromium.org/2843073003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/parse-urls-line-column.js:8:
testSplitLineColumn("http://www.chromium.org");
Add to existing parsed-url.html test.

[luoe, 2017-04-27 17:43:59]

Test moved.

https://codereview.chromium.org/2843073003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/inspector-unit/parse-urls-line-column.js
(right):

https://codereview.chromium.org/2843073003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/inspector-unit/parse-urls-line-column.js:8:
testSplitLineColumn("http://www.chromium.org");
On 2017/04/27 17:03:29, dgozman wrote:
> Add to existing parsed-url.html test.

Done.

[dgozman, 2017-04-27 22:55:03]

lgtm

[luoe, 2017-04-27 22:55:24]

The CQ bit was checked by luoe@chromium.org

[commit-bot: I haz the power, 2017-04-27 22:56:31]

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-04-28 03:20:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493333724440300,
"parent_rev": "68c4ff049efc98e035445aefb940594a7cfeb054", "commit_rev":
"201eaf7de7e46e23f5955dc8b57db0d76add14c2"}

[commit-bot: I haz the power, 2017-04-28 03:21:08]

Description was changed from

==========
DevTools: linkifier incorrectly matches port numbers in urls as line numbers

When logging a URL with a port number in the console, 'http://localhost:8000',
our linkifier incorrectly treats the '8000' port number as a line number.

Using our existing granular URL regexes, this patch makes the linkifier ignore
the URL part before the 'path' when looking for a line/column number.

BUG=701865
==========

to

==========
DevTools: linkifier incorrectly matches port numbers in urls as line numbers

When logging a URL with a port number in the console, 'http://localhost:8000',
our linkifier incorrectly treats the '8000' port number as a line number.

Using our existing granular URL regexes, this patch makes the linkifier ignore
the URL part before the 'path' when looking for a line/column number.

BUG=701865

Review-Url: https://codereview.chromium.org/2843073003
Cr-Commit-Position: refs/heads/master@{#467868}
Committed:
https://chromium.googlesource.com/chromium/src/+/201eaf7de7e46e23f5955dc8b57d...
==========

[commit-bot: I haz the power, 2017-04-28 03:21:09]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/201eaf7de7e46e23f5955dc8b57d...