Bug 170978 – Add Automator service to copy permalink to Clipboard

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

I frequently find it helpful to refer to specific line of code in code reviews, IRC and email discussions using a Trac permalink. It's tedious to create such a link using the trac.webkit.org web app. So, I wrote an Automator service workflow for Xcode that copies a Trac permalink to the currently selected line in the active document to the Clipboard. An example of a Trac permalink that is copied to the clipboard is: <http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/Scrolling/RemoteScrollingCoordinatorProxy.cpp?rev=215365#L83>. Holding down the Option key when the service runs will copy a Trac permalink with blame annotations.

[Daniel Bates]

Created attachment 307458 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 307458 [details]
Patch

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

Neat! r=me

> Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:120
> +    return `http://trac.webkit.org/browser/${branch}/${path}${revision}${withBlame}${lineNumber}`;

Nit: `https` will save a redirect.

> Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:145
> +        documentName.substr(0, documentName.lastIndexOf(editedSuffix));

The result of this expression is not saved. You probably wanted to update documentName:

    documentName = documentName.substr(...);

> Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:157
> +    var beginPosition = range[0] - 1;
> +    if (!beginPosition)

Is it possible that the first location would be 0, and this produces -1? Or is the first possible position 1?

> Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:243
> +    return { "branch": branch, "revision": revision, "repositoryURL": repositoryURL };

Style: You could just do:

    return { branch, revision, repositoryURL };

Which is equivalent shorthand for:

    return { "branch": branch, "revision": revision, "repositoryURL": repositoryURL };

I see you use destructuring later, so using ES6 features should not be a concern.

> Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:264
> +    for (var i = 0, length = lines.length; i < length; ++i) {
> +        var [key, value] = lines[i].split(": ", 2);

You could simplify a bit with for..of:

    for (var line of lines) {
        var [key, value] = line.split(": ", 2);

Since you don't use `i` or `length` outside of the loop.

[Daniel Bates]

(In reply to Joseph Pecoraro from comment #2)
> > Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:120
> > +    return `http://trac.webkit.org/browser/${branch}/${path}${revision}${withBlame}${lineNumber}`;
> 
> Nit: `https` will save a redirect.
> 

Will fix.

> > Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:145
> > +        documentName.substr(0, documentName.lastIndexOf(editedSuffix));
> 
> The result of this expression is not saved. You probably wanted to update
> documentName:
> 
>     documentName = documentName.substr(...);
> 

Will fix.

> > Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:157
> > +    var beginPosition = range[0] - 1;
> > +    if (!beginPosition)
> 
> Is it possible that the first location would be 0, and this produces -1? Or
> is the first possible position 1?
> 
> > Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:243
> > +    return { "branch": branch, "revision": revision, "repositoryURL": repositoryURL };
> 
> Style: You could just do:
> 
>     return { branch, revision, repositoryURL };
> 

Will fix.

> > Tools/CopyPermalink/Copy WebKit Permalink.workflow/Contents/document.wflow:264
> > +    for (var i = 0, length = lines.length; i < length; ++i) {
> > +        var [key, value] = lines[i].split(": ", 2);
> 
> You could simplify a bit with for..of:
> 
>     for (var line of lines) {
>         var [key, value] = line.split(": ", 2);
> 
> Since you don't use `i` or `length` outside of the loop.

Will change to use for-of loop.

[Daniel Bates]

This Automator service works under Xcode 8 or Xcode 9 beta 5 (9M202q) or later.

[Daniel Bates]

Committed r220705: <http://trac.webkit.org/changeset/220705>

[Radar WebKit Bug Importer]

<rdar://problem/33877779>