[webkit-reviews] review granted: [Bug 170978] Add Automator service to copy permalink to Clipboard : [Attachment 307458] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 170978: Add Automator service to copy permalink to Clipboard
https://bugs.webkit.org/show_bug.cgi?id=170978

Attachment 307458: Patch

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




--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 307458
  --> https://bugs.webkit.org/attachment.cgi?id=307458
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}${lineN
umber}`;

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.