Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
466743002:
DevTools: Show doc context menu items for matching methods on all objects (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@master Project:
blink Visibility:
Public. |
DescriptionDevTools: Show doc context menu items for matching methods on all objects
If a few target objects have a method with the same name as the selection,
we should create context menu items for all of them.
BUG=391593
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180161
Patch Set 1 #
Total comments: 20
Patch Set 2 : Comments addressed #
Total comments: 9
Patch Set 3 : Test updated #Patch Set 4 : Submenu item added #
Total comments: 9
Patch Set 5 : Removed whitespace from test #Patch Set 6 : Comments addressed #
Total comments: 4
Patch Set 7 : Submenu is not created in case of single property variant #
Total comments: 1
Patch Set 8 : Final changes #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:42: itemPath: function(searchTerm) documentationUrls https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:47: if (sourceRef.source[searchTerm] instanceof Function) { typeof sourceRef.source[searchTerm] !== "undefined", otherwise we will return nothing for properties (e.g. length). Also please extend the test with this case. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:55: var contextMenuItemTitle = WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Show documentation for " + possiblePaths[i].name : "Show Documentation for " + possiblePaths[i].name); Please use string sunstitution: WebInspector.UIString("Show documentation for %s", ...)
https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:14: * @type {!Array.<!Object, string, string>} This annotation is invalid. Should be: @type {!Array.<{source: !Object, url: string, name: string}>} https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:42: itemPath: function(searchTerm) This no longer returns a "path". Let's rename it to e.g. itemDescriptors() or just items()? https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:48: var property = {} You can initialize the fields right in the literal: var property = { url: String.sprintf(...), name: sourceRef.name + "." + searchTerm }; https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:15: * @param {string} searchURL just "url" https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:17: WebInspector.DocumentationView.showSearchURL = function(searchURL) showDocumentationURL() ? https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:54: for (var i = 0; i < possiblePaths.length; i++) { we prefer the pre-increment across the code: ++i https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:55: var contextMenuItemTitle = WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Show documentation for " + possiblePaths[i].name : "Show Documentation for " + possiblePaths[i].name); This is a wrong way to use UIString. You should use the string formatting: var formatString = WebInspector.useLowerCaseMenuTitles() ? "Show documentation for '%s'" : "Show Documentation for '%s'"; var contextMenuItemTitle = WebInspector.UIString(formatString, possiblePaths[i].name); https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:56: contextMenu.appendItem(contextMenuItemTitle, WebInspector.DocumentationView.showSearchURL.bind(null, possiblePaths[i].url)); We seem to have agreed on having a submenu, not on a series of top-level items, IIRC? This should look like this: Show documentation for 'remove' Array.prototype.remove Element.prototype.remove
https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:14: * type {!Array.<{source: !Object, url: string, name: string}>} @type https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:35: WebInspector.DocumentationURLProvider._urlFormat = "http://docs.webplatform.org/w/api.php?action=query&titles=%s%s&prop=revisions&rvprop=timestamp|content&format=json" semicolon https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:51: } semicolon https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:55: var formatString = WebInspector.useLowerCaseMenuTitles() ? "Show documentation for '%s'" : "Show Documentation for '%s'"; this should be moved out of for-loop https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:57: contextMenu.appendItem(contextMenuItemTitle, WebInspector.DocumentationView.showDocumentationURL.bind(null, possibleProperties[i].url)); this doesn't add into context menu submenu, does it? I believe we want to have a submenu.
https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:14: * @type {!Array.<!Object, string, string>} On 2014/08/12 12:54:16, apavlov wrote: > This annotation is invalid. Should be: > > @type {!Array.<{source: !Object, url: string, name: string}>} Done. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:42: itemPath: function(searchTerm) On 2014/08/12 12:49:59, yurys wrote: > documentationUrls Done. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:47: if (sourceRef.source[searchTerm] instanceof Function) { On 2014/08/12 12:49:59, yurys wrote: > typeof sourceRef.source[searchTerm] !== "undefined", otherwise we will return > nothing for properties (e.g. length). Also please extend the test with this > case. Currently we are working only with methods, not properties. We will add properties compatibility in one of the next patches. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:48: var property = {} On 2014/08/12 12:54:16, apavlov wrote: > You can initialize the fields right in the literal: > > var property = { > url: String.sprintf(...), > name: sourceRef.name + "." + searchTerm > }; Done. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:15: * @param {string} searchURL On 2014/08/12 12:54:16, apavlov wrote: > just "url" Done. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:17: WebInspector.DocumentationView.showSearchURL = function(searchURL) On 2014/08/12 12:54:17, apavlov wrote: > showDocumentationURL() ? Done. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:54: for (var i = 0; i < possiblePaths.length; i++) { On 2014/08/12 12:54:16, apavlov wrote: > we prefer the pre-increment across the code: ++i Done. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:55: var contextMenuItemTitle = WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Show documentation for " + possiblePaths[i].name : "Show Documentation for " + possiblePaths[i].name); On 2014/08/12 12:54:17, apavlov wrote: > This is a wrong way to use UIString. You should use the string formatting: > > var formatString = WebInspector.useLowerCaseMenuTitles() ? "Show documentation > for '%s'" : "Show Documentation for '%s'"; > var contextMenuItemTitle = WebInspector.UIString(formatString, > possiblePaths[i].name); Done. https://codereview.chromium.org/466743002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationView.js:56: contextMenu.appendItem(contextMenuItemTitle, WebInspector.DocumentationView.showSearchURL.bind(null, possiblePaths[i].url)); On 2014/08/12 12:54:17, apavlov wrote: > We seem to have agreed on having a submenu, not on a series of top-level items, > IIRC? > > This should look like this: > > Show documentation for 'remove' > Array.prototype.remove > Element.prototype.remove Done. https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:14: * type {!Array.<{source: !Object, url: string, name: string}>} On 2014/08/12 13:21:06, lushnikov wrote: > @type Done. https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:51: } On 2014/08/12 13:21:06, lushnikov wrote: > semicolon Done. https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:55: var formatString = WebInspector.useLowerCaseMenuTitles() ? "Show documentation for '%s'" : "Show Documentation for '%s'"; On 2014/08/12 13:21:06, lushnikov wrote: > this should be moved out of for-loop Done. https://codereview.chromium.org/466743002/diff/20001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:57: contextMenu.appendItem(contextMenuItemTitle, WebInspector.DocumentationView.showDocumentationURL.bind(null, possibleProperties[i].url)); On 2014/08/12 13:21:06, lushnikov wrote: > this doesn't add into context menu submenu, does it? I believe we want to have a > submenu. Done.
lgtm
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/466743002/100001
The CQ bit was unchecked by apavlov@chromium.org
https://codereview.chromium.org/466743002/diff/80001/LayoutTests/inspector/do... File LayoutTests/inspector/documentation/documentation-url-provider.html (right): https://codereview.chromium.org/466743002/diff/80001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/documentation-url-provider.html:22: ]; This should be indented same as "var testCases", i.e. shift this 4 spaces back. https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:50: name: sourceRef.name + "." + searchTerm Appending searchTerm here is undesirable, as this is low-level functionality, and "name" is used as-is to provide the context menu item label. Instead, the label should be constructed by the entity that contributes context menu items. https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:56: var subMenuItem = contextMenu.appendSubMenuItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Show documentation for" : "Show Documentation for")); You have lost the search term part here. Please refer to my original comment. https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:58: var contextMenuItemTitle = WebInspector.UIString(possibleProperties[i].name); The property names are not localizable (cannot be translated to different languages), so you should not use UIString here. Just inline "possibleProperties[i].name" into the next line.
https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:56: var subMenuItem = contextMenu.appendSubMenuItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Show documentation for" : "Show Documentation for")); On 2014/08/12 14:45:16, apavlov wrote: > You have lost the search term part here. Please refer to my original comment. Alternatively, you can display "Show documentation for..." (with an ellipsis), as is common for context menu items with sub-items.
https://codereview.chromium.org/466743002/diff/80001/LayoutTests/inspector/do... File LayoutTests/inspector/documentation/documentation-url-provider.html (right): https://codereview.chromium.org/466743002/diff/80001/LayoutTests/inspector/do... LayoutTests/inspector/documentation/documentation-url-provider.html:22: ]; On 2014/08/12 14:45:16, apavlov wrote: > This should be indented same as "var testCases", i.e. shift this 4 spaces back. Done. https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationURLProvider.js:50: name: sourceRef.name + "." + searchTerm On 2014/08/12 14:45:16, apavlov wrote: > Appending searchTerm here is undesirable, as this is low-level functionality, > and "name" is used as-is to provide the context menu item label. Instead, the > label should be constructed by the entity that contributes context menu items. Done. https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:56: var subMenuItem = contextMenu.appendSubMenuItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Show documentation for" : "Show Documentation for")); On 2014/08/12 14:50:45, apavlov wrote: > On 2014/08/12 14:45:16, apavlov wrote: > > You have lost the search term part here. Please refer to my original comment. > > Alternatively, you can display "Show documentation for..." (with an ellipsis), > as is common for context menu items with sub-items. Done. https://codereview.chromium.org/466743002/diff/80001/Source/devtools/front_en... Source/devtools/front_end/documentation/DocumentationView.js:58: var contextMenuItemTitle = WebInspector.UIString(possibleProperties[i].name); On 2014/08/12 14:45:16, apavlov wrote: > The property names are not localizable (cannot be translated to different > languages), so you should not use UIString here. Just inline > "possibleProperties[i].name" into the next line. Done.
https://codereview.chromium.org/466743002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:54: if (possibleProperties.length === 0) if (!possibleProperties.length) return; https://codereview.chromium.org/466743002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:56: var subMenuItem = contextMenu.appendSubMenuItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Show documentation for..." : "Show Documentation for...")); a submenu should only be present when there are multiple alternatives, otherwise the activator item should be found immediately at the top level
https://codereview.chromium.org/466743002/diff/120001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:54: if (possibleProperties.length === 0) On 2014/08/12 15:35:45, apavlov wrote: > if (!possibleProperties.length) > return; Done. https://codereview.chromium.org/466743002/diff/120001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:56: var subMenuItem = contextMenu.appendSubMenuItem(WebInspector.UIString(WebInspector.useLowerCaseMenuTitles() ? "Show documentation for..." : "Show Documentation for...")); On 2014/08/12 15:35:46, apavlov wrote: > a submenu should only be present when there are multiple alternatives, otherwise > the activator item should be found immediately at the top level Done.
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/466743002/130001
lgtm with a nit https://codereview.chromium.org/466743002/diff/130001/Source/devtools/front_e... File Source/devtools/front_end/documentation/DocumentationView.js (right): https://codereview.chromium.org/466743002/diff/130001/Source/devtools/front_e... Source/devtools/front_end/documentation/DocumentationView.js:58: contextMenu.appendItem(String.sprintf(formatString, possibleProperties[0].name, selectedText), WebInspector.DocumentationView.showDocumentationURL.bind(null, possibleProperties[0].url)); Since we are dealing with localizable strings ("Show documentation for"), WebInspector.UIString() should be used here in place of String.sprintf.
Please read http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html - it explains how to write good commit messages (which is what the issue description eventually becomes) and fix the issue subject/first line of description. In particular: > Write your commit message in the imperative: "Fix bug" and > not "Fixed bug" or "Fixes bug." This convention matches up > with commit messages generated by commands > like git merge and git revert.
The CQ bit was unchecked by semeny@google.com
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/466743002/150001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19597) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22482)
Message was sent while issue was closed.
Change committed as 180161 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
