Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
471583003:
DevTools: [Documentation] Add property token autodetection (Closed)
|
Created:
6 years, 4 months ago by semeny Modified:
6 years, 4 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@sources-patch Project:
blink Visibility:
Public. |
DescriptionDevTools: [Documentation] Add property token autodetection
Now documentation context menu item appears without token selection.
BUG=391593
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180362
Patch Set 1 #
Total comments: 4
Patch Set 2 : Comments addressed #Patch Set 3 : Add selection option and more user-friendly token detection #
Total comments: 5
Patch Set 4 : Comments addressed #
Total comments: 4
Patch Set 5 : Comments addressed #
Total comments: 4
Patch Set 6 : Comments addressed #
Total comments: 6
Patch Set 7 : Simplify code #
Total comments: 6
Patch Set 8 : Comments addressed #
Total comments: 1
Patch Set 9 : Created descriptor record type #
Messages
Total messages: 20 (0 generated)
Looks reasonable. Let's see what Andrey says regarding the token text extraction. https://codereview.chromium.org/471583003/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:53: var lineNumber = textPosition.startLine; inline this https://codereview.chromium.org/471583003/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:55: var tokenContent = line.substring(token.startColumn, token.endColumn + 1); tokenText is a bit shorter :)
https://codereview.chromium.org/471583003/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:53: var lineNumber = textPosition.startLine; On 2014/08/13 15:11:17, apavlov wrote: > inline this Done. https://codereview.chromium.org/471583003/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:55: var tokenContent = line.substring(token.startColumn, token.endColumn + 1); On 2014/08/13 15:11:17, apavlov wrote: > tokenText is a bit shorter :) Done.
https://codereview.chromium.org/471583003/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:57: var token = leftToken ? leftToken : rightToken; var token = leftToken || rightToken; Also, you seem to handle only one token of the two. You should process the second token if the first one hasn't yielded any results (e.g. it is a dot '.') https://codereview.chromium.org/471583003/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:61: propertyName = line.substring(token.startColumn, token.endColumn + 1); I'm about to send out a patch that makes the token endColumn exclusive, so you won't have to +1.
https://codereview.chromium.org/471583003/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:57: var token = leftToken ? leftToken : rightToken; On 2014/08/14 10:37:04, apavlov wrote: > var token = leftToken || rightToken; > > Also, you seem to handle only one token of the two. You should process the > second token if the first one hasn't yielded any results (e.g. it is a dot '.') Take a look at how this is done in WI.CSSSourceFrame._handleUnitModification
https://codereview.chromium.org/471583003/diff/40001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:57: var token = leftToken ? leftToken : rightToken; On 2014/08/14 10:39:04, apavlov wrote: > On 2014/08/14 10:37:04, apavlov wrote: > > var token = leftToken || rightToken; > > > > Also, you seem to handle only one token of the two. You should process the > > second token if the first one hasn't yielded any results (e.g. it is a dot > '.') > > Take a look at how this is done in WI.CSSSourceFrame._handleUnitModification Done. https://codereview.chromium.org/471583003/diff/40001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:61: propertyName = line.substring(token.startColumn, token.endColumn + 1); On 2014/08/14 10:37:04, apavlov wrote: > I'm about to send out a patch that makes the token endColumn exclusive, so you > won't have to +1. Done.
https://codereview.chromium.org/471583003/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:54: var textPosition = textEditor.coordinatesToCursorPosition(event.x, event.y); let's switch to cursor position instead - context menu could be summoned via keyboard. https://codereview.chromium.org/471583003/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:55: var token = textEditor.tokenAtTextPosition(textPosition.startLine, textPosition.startColumn); you're not only interested in token existence, but in existence of documentation per token. I.e. if both left and right tokens exist, but there's no documentation for left token, and there's one for right, you should use right.
https://codereview.chromium.org/471583003/diff/60001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:54: var textPosition = textEditor.coordinatesToCursorPosition(event.x, event.y); On 2014/08/14 11:44:14, lushnikov wrote: > let's switch to cursor position instead - context menu could be summoned via > keyboard. Done. https://codereview.chromium.org/471583003/diff/60001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:55: var token = textEditor.tokenAtTextPosition(textPosition.startLine, textPosition.startColumn); On 2014/08/14 11:44:14, lushnikov wrote: > you're not only interested in token existence, but in existence of documentation > per token. > I.e. if both left and right tokens exist, but there's no documentation for left > token, and there's one for right, you should use right. Done.
https://codereview.chromium.org/471583003/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:52: if (textSelection && !textSelection.isEmpty() && textSelection.startLine === textSelection.endLine) { Please remove the first conjunct, since textSelection is always there. https://codereview.chromium.org/471583003/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:58: if (!token || !urlProvider.itemDescriptors(tokenText).length) { Let's avoid multiple itemDescriptors() invocations for the same text.
https://codereview.chromium.org/471583003/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:52: if (textSelection && !textSelection.isEmpty() && textSelection.startLine === textSelection.endLine) { On 2014/08/14 12:49:52, apavlov wrote: > Please remove the first conjunct, since textSelection is always there. Done. https://codereview.chromium.org/471583003/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:58: if (!token || !urlProvider.itemDescriptors(tokenText).length) { On 2014/08/14 12:49:52, apavlov wrote: > Let's avoid multiple itemDescriptors() invocations for the same text. Done.
https://codereview.chromium.org/471583003/diff/100001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/100001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:49: var propertyName; The new code should now become a new method due to the increased complexity... https://codereview.chromium.org/471583003/diff/100001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:53: if (!textSelection.isEmpty() && textSelection.startLine === textSelection.endLine) { Add if (textSelection.startLine !== textSelection.endLine) return; before this "if" and remove that check here (we don't want to handle multi-line selections, right?) https://codereview.chromium.org/471583003/diff/100001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:70: descriptors = urlProvider.itemDescriptors(propertyName); Assuming the new code is inside a new method, let's extract an inner function to determine the descriptors given a token range? if (!textSelection.isEmpty() && ..) return urlProvider.itemDescriptors(textEditor.copyRange(textSelection)); var descriptors = computeDescriptors(textSelection.startColumn); if (!descriptors.length && textSelection.startColumn > 0) descriptors = computeDescriptors(textSelection.startColumn - 1); if (!descriptors.length) return; ... and so on ... function computeDescriptors(column) { var token = textEditor.tokenAtTextPosition(textSelection.startLine, column); if (!token) return []; var tokenText = textEditor.line(textSelection.startLine).substring(token.startColumn, token.endColumn); return urlProvider.itemDescriptors(tokenText); }
https://codereview.chromium.org/471583003/diff/100001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/100001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:49: var propertyName; On 2014/08/14 13:44:56, apavlov wrote: > The new code should now become a new method due to the increased complexity... Done. https://codereview.chromium.org/471583003/diff/100001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:53: if (!textSelection.isEmpty() && textSelection.startLine === textSelection.endLine) { On 2014/08/14 13:44:55, apavlov wrote: > Add > if (textSelection.startLine !== textSelection.endLine) > return; > before this "if" and remove that check here (we don't want to handle multi-line > selections, right?) Done. https://codereview.chromium.org/471583003/diff/100001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:70: descriptors = urlProvider.itemDescriptors(propertyName); On 2014/08/14 13:44:55, apavlov wrote: > Assuming the new code is inside a new method, let's extract an inner function to > determine the descriptors given a token range? > > if (!textSelection.isEmpty() && ..) > return urlProvider.itemDescriptors(textEditor.copyRange(textSelection)); > > var descriptors = computeDescriptors(textSelection.startColumn); > if (!descriptors.length && textSelection.startColumn > 0) > descriptors = computeDescriptors(textSelection.startColumn - 1); > > if (!descriptors.length) > return; > > ... and so on ... > > > function computeDescriptors(column) > { > var token = textEditor.tokenAtTextPosition(textSelection.startLine, column); > if (!token) > return []; > var tokenText = > textEditor.line(textSelection.startLine).substring(token.startColumn, > token.endColumn); > return urlProvider.itemDescriptors(tokenText); > } Done.
https://codereview.chromium.org/471583003/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:61: /** blank line above https://codereview.chromium.org/471583003/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:77: if (descriptors && descriptors.length) |descriptors| is never null, please remove the first check https://codereview.chromium.org/471583003/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:82: function computeDescriptors(column) Please annotate
https://codereview.chromium.org/471583003/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:61: /** On 2014/08/15 10:48:58, apavlov wrote: > blank line above Done. https://codereview.chromium.org/471583003/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:77: if (descriptors && descriptors.length) On 2014/08/15 10:48:58, apavlov wrote: > |descriptors| is never null, please remove the first check Done. https://codereview.chromium.org/471583003/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:82: function computeDescriptors(column) On 2014/08/15 10:48:58, apavlov wrote: > Please annotate Done.
lgtm
lgtm https://codereview.chromium.org/471583003/diff/140001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/471583003/diff/140001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:85: * @return {!Array.<{url: string, name: string, searchItem: string}>} Since this record type is used in the provider API, we should perhaps create a @typedef (like WebInspector.DocumentationURLProvider.ItemDescriptor)
The CQ bit was checked by semeny@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/semeny@google.com/471583003/160001
Message was sent while issue was closed.
Committed patchset #9 (160001) as 180362 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
