Summary: | Add Automator service to copy permalink to Clipboard | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||
Component: | Tools / Tests | Assignee: | Daniel Bates <dbates> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | aestes, darin, ddkilzer, joepeck, lforschler, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Mac | ||||||
OS: | macOS 10.12 | ||||||
Attachments: |
|
Description
Daniel Bates
2017-04-18 20:22:35 PDT
Created attachment 307458 [details]
Patch
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. (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. This Automator service works under Xcode 8 or Xcode 9 beta 5 (9M202q) or later. Committed r220705: <http://trac.webkit.org/changeset/220705> |