Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867133002:
DevTools: Let the drawer tabs be reordered (Closed)
|
Created:
3 years, 7 months ago by einbinder Modified:
3 years, 6 months ago Reviewers:
dgozman CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Let the drawer tabs be reordered
BUG=none
Review-Url: https://codereview.chromium.org/2867133002
Cr-Commit-Position: refs/heads/master@{#475821}
Committed: https://chromium.googlesource.com/chromium/src/+/65ef1a1a01feffd43c073cc60fb75de97a567ad2
Patch Set 1 #
Total comments: 2
Patch Set 2 : Preserve tab order on appending tabs #
Total comments: 2
Patch Set 3 : Add test #
Total comments: 7
Patch Set 4 : Runtime module for test #
Total comments: 6
Patch Set 5 : showView #
Messages
Total messages: 27 (15 generated)
einbinder@chromium.org changed reviewers: + dgozman@chromium.org
ptal
https://codereview.chromium.org/2867133002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/InspectorView.js (right): https://codereview.chromium.org/2867133002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/InspectorView.js:49: UI.viewManager.createTabbedLocation(this._showDrawer.bind(this, false), 'drawer-view', true, true); Let's fix all ordering issues in this patch.
https://codereview.chromium.org/2867133002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/ui/InspectorView.js (right): https://codereview.chromium.org/2867133002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/ui/InspectorView.js:49: UI.viewManager.createTabbedLocation(this._showDrawer.bind(this, false), 'drawer-view', true, true); On 2017/05/08 at 23:25:57, dgozman wrote: > Let's fix all ordering issues in this patch. Done.
The CQ bit was checked by einbinder@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Let's have a test. https://codereview.chromium.org/2867133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2867133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:210: this.dispatchEventToListeners(UI.TabbedPane.Events.TabOrderChanged, this._tabs); Looks like a band-aid fix. Instead, client should do whatever is needed when calling appendTab().
https://codereview.chromium.org/2867133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2867133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:210: this.dispatchEventToListeners(UI.TabbedPane.Events.TabOrderChanged, this._tabs); On 2017/05/15 at 17:03:36, dgozman wrote: > Looks like a band-aid fix. Instead, client should do whatever is needed when calling appendTab(). Done. https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (left): https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:1037: tabElement.selectTabForTest = this._tabbedPane.selectTab.bind(this._tabbedPane, this.id, true); This was unused. https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:807: this._tabsElement.insertBefore(tab.tabElement, this._tabsElement.childNodes[index]); _tabElement could be null, but insertBefore actually throws an error if the first parameter is null. https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/View.js (left): https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/View.js:755: if (view.isCloseable()) { Tabs were persisted when they were selected, but it makes more sense for them to be persisted when they are opened.
https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js (right): https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js:15: this._views.set(view.viewId(), view); Instead of mock, should we inject real extensions to the system? I'm thinking either defining them in module.json for tests, or introducing Runtime.Model.addExtensionForTest() method. https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/View.js (right): https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/View.js:679: if (view.isCloseable()) { Let's move this persisting block to appendView instead? There is no need to call it from appendApplicableItems.
https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js (right): https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js:15: this._views.set(view.viewId(), view); On 2017/05/25 at 17:12:49, dgozman wrote: > Instead of mock, should we inject real extensions to the system? I'm thinking either defining them in module.json for tests, or introducing Runtime.Model.addExtensionForTest() method. Done. https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/ui/View.js (right): https://codereview.chromium.org/2867133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/ui/View.js:679: if (view.isCloseable()) { On 2017/05/25 at 17:12:49, dgozman wrote: > Let's move this persisting block to appendView instead? There is no need to call it from appendApplicableItems. Done.
https://codereview.chromium.org/2867133002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js (right): https://codereview.chromium.org/2867133002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js:15: "persistence": "closeable", Do you need both this and isCloseable method? In fact, there is no need for SimpleView now, right? https://codereview.chromium.org/2867133002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js:26: tabbedLocation.appendView(new ClosableView('first')); Why manually? Doesn't this clash with ones from extension? Should we do viewManager.showView('first')? https://codereview.chromium.org/2867133002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js:52: tabbedLocation = (new UI.ViewManager()).createTabbedLocation(undefined, 'mock-location', true, true); Why not UI.viewManager?
The CQ bit was checked by einbinder@chromium.org
The CQ bit was unchecked by einbinder@chromium.org
The CQ bit was unchecked by einbinder@chromium.org
https://codereview.chromium.org/2867133002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js (right): https://codereview.chromium.org/2867133002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js:15: "persistence": "closeable", On 2017/05/26 at 19:17:08, dgozman wrote: > Do you need both this and isCloseable method? In fact, there is no need for SimpleView now, right? I did but now I don't. https://codereview.chromium.org/2867133002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js:26: tabbedLocation.appendView(new ClosableView('first')); On 2017/05/26 at 19:17:08, dgozman wrote: > Why manually? Doesn't this clash with ones from extension? Should we do viewManager.showView('first')? It doesn't clash, but your code is much cleaner. https://codereview.chromium.org/2867133002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/inspector-unit/view-location.js:52: tabbedLocation = (new UI.ViewManager()).createTabbedLocation(undefined, 'mock-location', true, true); On 2017/05/26 at 19:17:08, dgozman wrote: > Why not UI.viewManager? UI.viewManager has stale extensions. Also it should be recreated every time to simulate closing and opening devtools.
The CQ bit was checked by einbinder@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by einbinder@chromium.org
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": 80001, "attempt_start_ts": 1496204238488430,
"parent_rev": "748fee3ce72b96c1a0bdf84bb7d6bdffa592e21d", "commit_rev":
"65ef1a1a01feffd43c073cc60fb75de97a567ad2"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Let the drawer tabs be reordered BUG=none ========== to ========== DevTools: Let the drawer tabs be reordered BUG=none Review-Url: https://codereview.chromium.org/2867133002 Cr-Commit-Position: refs/heads/master@{#475821} Committed: https://chromium.googlesource.com/chromium/src/+/65ef1a1a01feffd43c073cc60fb7... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/65ef1a1a01feffd43c073cc60fb7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
