Bug 227549

Summary: [results.webkit.org] linkify urls in commit messages
Product: WebKit Reporter: Kevin Neal <kevin_neal>
Component: Tools / TestsAssignee: Kevin Neal <kevin_neal>
Status: RESOLVED FIXED    
Severity: Normal CC: jbedard, kevin_neal, webkit-bug-importer, zhifei_fang
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
fast-cq Patch none

Description Kevin Neal 2021-06-30 15:28:53 PDT
Commit messages usually contain URLs pointing to the bug; however, the URLs do not have hyperlinks to the bug.
Comment 1 Radar WebKit Bug Importer 2021-06-30 15:30:36 PDT
<rdar://problem/79988762>
Comment 2 Kevin Neal 2021-06-30 16:02:46 PDT
Created attachment 432638 [details]
Patch
Comment 3 Kevin Neal 2021-07-09 10:07:20 PDT
Created attachment 433222 [details]
Patch
Comment 4 Zhifei Fang 2021-07-09 11:32:48 PDT
Comment on attachment 433222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433222&action=review

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:122
> +                            let linkify = (function(commitMessage) {

better to make this a top level function, not a IIFE. If you put the code here, actually there is no need for have a IIFE. Originally line 117 we used the IIFE for making javascript string template can support normal js statements not just expressions.

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:123
> +                                let reg = /\b(https?|rdar):\/{2}[^\s<>&]+/gmi;

May also need to consider http, and I am not sure if we have some links without leading protocol, may worth to find if they are existed.

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:125
> +                                let newMessageText = messageText.replace(reg, `<a href="$&">$&</a>`);

It will be better to open a new tab, add target="_blank"
Comment 5 Jonathan Bedard 2021-07-12 08:42:30 PDT
Comment on attachment 433222 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433222&action=review

>> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:123
>> +                                let reg = /\b(https?|rdar):\/{2}[^\s<>&]+/gmi;
> 
> May also need to consider http, and I am not sure if we have some links without leading protocol, may worth to find if they are existed.

Existing code does consider http, I think, the regex is `https?` which should match http and https
Comment 6 Kevin Neal 2021-07-12 15:53:06 PDT
Created attachment 433366 [details]
Patch
Comment 7 Jonathan Bedard 2021-07-12 16:02:45 PDT
Comment on attachment 433366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=433366&action=review

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:121
>                                  return `<br><div>${escapeHTML(cell.commit.message.split('\n')[0])}</div>`;

We should lankily this text as well, in case someone has a link in the first line

> Tools/Scripts/libraries/resultsdbpy/resultsdbpy/view/static/js/commit.js:122
> +                            return linkify(escapeHTML(cell.commit.message));

We still want to keep the `<br><div></div>` surrounding this text
Comment 8 Kevin Neal 2021-07-13 09:24:30 PDT
Created attachment 433409 [details]
Patch
Comment 9 Kevin Neal 2021-07-13 09:32:23 PDT
Created attachment 433410 [details]
fast-cq Patch
Comment 10 EWS 2021-07-13 10:05:48 PDT
Committed r279876 (239629@main): <https://commits.webkit.org/239629@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433410 [details].