Bug 170978 - Add Automator service to copy permalink to Clipboard
Summary: Add Automator service to copy permalink to Clipboard
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac macOS 10.12
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-18 20:22 PDT by Daniel Bates
Modified: 2017-08-14 10:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.92 KB, patch)
2017-04-18 20:32 PDT, Daniel Bates
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2017-04-18 20:22:35 PDT
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.
Comment 1 Daniel Bates 2017-04-18 20:32:00 PDT
Created attachment 307458 [details]
Patch
Comment 2 Joseph Pecoraro 2017-04-21 18:57:58 PDT
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 &lt; 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.
Comment 3 Daniel Bates 2017-08-14 10:19:31 PDT
(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 &lt; 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.
Comment 4 Daniel Bates 2017-08-14 10:20:05 PDT
This Automator service works under Xcode 8 or Xcode 9 beta 5 (9M202q) or later.
Comment 5 Daniel Bates 2017-08-14 10:25:26 PDT
Committed r220705: <http://trac.webkit.org/changeset/220705>
Comment 6 Radar WebKit Bug Importer 2017-08-14 10:25:51 PDT
<rdar://problem/33877779>