Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2716683006:
DevTools: add entry points for command menu (Closed)
|
Created:
3 years, 10 months ago by luoe Modified:
3 years, 7 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: add entry points for command menu
This adds a 'Open file' option in the DevTools 3 dot menu and updates the
style of the Sources placeholder text with a new entry
'Ctrl-Shift-P Run command'.
BUG=700184
Review-Url: https://codereview.chromium.org/2716683006
Cr-Commit-Position: refs/heads/master@{#467805}
Committed: https://chromium.googlesource.com/chromium/src/+/c7c2befd794fe76f229dab8bd68026c73470d5de
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase and ac #Patch Set 3 : no wrap/scroll #
Total comments: 7
Patch Set 4 : ac #
Total comments: 2
Patch Set 5 : ac #
Total comments: 2
Patch Set 6 : ac #
Messages
Total messages: 26 (12 generated)
Description was changed from ========== DevTools: add entry points for command menu and toggle drawer BUG=none ========== to ========== DevTools: add entry points for command menu and toggle drawer BUG=none ==========
luoe@chromium.org changed reviewers: + chenwilliam@chromium.org
https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sources/sourcesView.css (right): https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/sourcesView.css:99: white-space: pre; we don't need this anymore right? https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:49: contentTag.createChild('span', 'content child'); remove this? https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:511: delete this._noTabsMessageElement; We don't want to delete this property.
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
ptal https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sources/sourcesView.css (right): https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sources/sourcesView.css:99: white-space: pre; On 2017/02/25 00:56:43, chenwilliam wrote: > we don't need this anymore right? Done. https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:49: contentTag.createChild('span', 'content child'); On 2017/02/25 00:56:43, chenwilliam wrote: > remove this? Done. https://codereview.chromium.org/2716683006/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:511: delete this._noTabsMessageElement; On 2017/02/25 00:56:43, chenwilliam wrote: > We don't want to delete this property. Done.
lgtm % minor comment https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/Main.js:734: contextMenu.appendAction('commandMenu.show', 'Run command'); Common.UIString
could you please add screenshots and improve CL description to motivate the change?
Description was changed from ========== DevTools: add entry points for command menu and toggle drawer BUG=none ========== to ========== DevTools: add entry points for command menu and toggle drawer This adds a new item to 'Run command' (ctrl-shift-P) in the 3 dot menu. Also added along with 'Toggle console drawer' into the placeholder hint text when no source file is open. Screenshot: http://imgur.com/a/1C7Av BUG=700184 ==========
luoe@chromium.org changed reviewers: + pfeldman@chromium.org
Description updated with screenshot
https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/Main.js:734: contextMenu.appendAction('commandMenu.show', 'Run command'); You should not indirectly refer to the action ids unless necessary. It is better to provide action into to names menu using descriptor. https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js (right): https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:93: placeholderElement() { It should be annotated and private. https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:496: element.classList.add('fill'); You should not add classes to provided elements - they might be incompatible with the ones you set. Either defer to placeholder to position itself or introduce intermediate container. Overall, keeping it in light dom might be not the best idea - there is now no way placeholders are consistent.
Description was changed from ========== DevTools: add entry points for command menu and toggle drawer This adds a new item to 'Run command' (ctrl-shift-P) in the 3 dot menu. Also added along with 'Toggle console drawer' into the placeholder hint text when no source file is open. Screenshot: http://imgur.com/a/1C7Av BUG=700184 ========== to ========== DevTools: add entry points for command menu This adds a 'Go to file...' option in the DevTools 3 dot menu and updates the style of the Sources placeholder text with a new entry 'Ctrl-Shift-P Run a command'. BUG=700184 ==========
Ptal https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/main/Main.js (right): https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/main/Main.js:734: contextMenu.appendAction('commandMenu.show', 'Run command'); On 2017/03/28 00:59:34, pfeldman wrote: > You should not indirectly refer to the action ids unless necessary. It is better > to provide action into to names menu using descriptor. Done. https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js (right): https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:93: placeholderElement() { On 2017/03/28 00:59:34, pfeldman wrote: > It should be annotated and private. Done. https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2716683006/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:496: element.classList.add('fill'); On 2017/03/28 00:59:35, pfeldman wrote: > You should not add classes to provided elements - they might be incompatible > with the ones you set. Either defer to placeholder to position itself or > introduce intermediate container. > > Overall, keeping it in light dom might be not the best idea - there is now no > way placeholders are consistent. Consistency sounds great to me, I'll keep it in the shadow.
https://codereview.chromium.org/2716683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js (right): https://codereview.chromium.org/2716683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:99: defaultShortcut: Common.UIString('Ctrl+P'), Since shortcuts are defined in quickopen and could be changed, you should do: UI.shortcutRegistry.shortcutTitleForAction('') instead. Hardcoding id here seems necessary.
Description was changed from ========== DevTools: add entry points for command menu This adds a 'Go to file...' option in the DevTools 3 dot menu and updates the style of the Sources placeholder text with a new entry 'Ctrl-Shift-P Run a command'. BUG=700184 ========== to ========== DevTools: add entry points for command menu This adds a 'Open file' option in the DevTools 3 dot menu and updates the style of the Sources placeholder text with a new entry 'Ctrl-Shift-P Run command'. BUG=700184 ==========
Please take another look https://codereview.chromium.org/2716683006/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js (right): https://codereview.chromium.org/2716683006/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/SourcesView.js:99: defaultShortcut: Common.UIString('Ctrl+P'), On 2017/04/24 18:29:25, pfeldman wrote: > Since shortcuts are defined in quickopen and could be changed, you should do: > > UI.shortcutRegistry.shortcutTitleForAction('') instead. Hardcoding id here seems > necessary. Done.
https://codereview.chromium.org/2716683006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2716683006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:494: setPlaceholderElement(element) { This call seems to have no effect on its own.
Ptal https://codereview.chromium.org/2716683006/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2716683006/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:494: setPlaceholderElement(element) { On 2017/04/26 00:02:21, pfeldman wrote: > This call seems to have no effect on its own. Semantically, calling setPlaceholderElement() after an old placeholderElement has been rendered should update the container with the new one.
lgtm
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chenwilliam@chromium.org Link to the patchset: https://codereview.chromium.org/2716683006/#ps100001 (title: "ac")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493329916407810,
"parent_rev": "f8c865910c21c379df39eb057530e13630d2f00d", "commit_rev":
"c7c2befd794fe76f229dab8bd68026c73470d5de"}
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493329916407810,
"parent_rev": "f8c865910c21c379df39eb057530e13630d2f00d", "commit_rev":
"c7c2befd794fe76f229dab8bd68026c73470d5de"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: add entry points for command menu This adds a 'Open file' option in the DevTools 3 dot menu and updates the style of the Sources placeholder text with a new entry 'Ctrl-Shift-P Run command'. BUG=700184 ========== to ========== DevTools: add entry points for command menu This adds a 'Open file' option in the DevTools 3 dot menu and updates the style of the Sources placeholder text with a new entry 'Ctrl-Shift-P Run command'. BUG=700184 Review-Url: https://codereview.chromium.org/2716683006 Cr-Commit-Position: refs/heads/master@{#467805} Committed: https://chromium.googlesource.com/chromium/src/+/c7c2befd794fe76f229dab8bd680... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/c7c2befd794fe76f229dab8bd680... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
