Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557973002:
DevTools: Introduce Landing page for Timeline panel. (Closed)
|
Created:
4 years ago by alph Modified:
4 years 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/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: Introduce Landing page for Timeline panel.
Provide more user-friendly experience as a welcome screen on Timeline panel.
The new UX is put behind an experiment.
BUG=570441
Committed: https://crrev.com/6741acb07eaa3bfd69c6871179511fca278bff62
Cr-Commit-Position: refs/heads/master@{#437979}
Patch Set 1 #
Total comments: 12
Patch Set 2 : addressing comments #
Total comments: 16
Patch Set 3 : addressing comments #
Total comments: 6
Patch Set 4 : addressing comments #
Total comments: 2
Patch Set 5 : use createSettingCheckbox #
Total comments: 2
Patch Set 6 : do TimelinePanel._clear earlier. #
Total comments: 6
Patch Set 7 : addressing comments #
Total comments: 8
Patch Set 8 : addressing comments #
Total comments: 6
Patch Set 9 : addressing comments. #Patch Set 10 : minor tweaks #Messages
Total messages: 30 (6 generated)
alph@chromium.org changed reviewers: + caseq@chromium.org, pfeldman@chromium.org
ptal
https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:17: this.settings = { Why do we make the base class aware of all possible settings? Why is this not outside of the Perspective, perhaps even static? https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:22: setting: 'timelineCaptureNetwork' I wonder whether we should better use actual instance of the setting rather than referring to it by a name. https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:162: ].map(cls => new cls()); let's just call constructors expecitly. https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:165: const div = headerDiv.createChild('div', 'timeline-perspective-header'); Does this fail gracefully when the window is too narrow? Should we re-use some existent control -- perhaps tabbed pane to take advantage of the way it overflows? https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:189: this._perspectiveSetting.removeChangeListener(this._updatePerspective, this); why do you need it? https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js (right): https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:87: this._timelinePane = timelinePane; let's get rid of local var for simplicity.
ptal https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:17: this.settings = { On 2016/12/07 20:17:29, caseq wrote: > Why do we make the base class aware of all possible settings? Why is this not > outside of the Perspective, perhaps even static? Done. https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:22: setting: 'timelineCaptureNetwork' On 2016/12/07 20:17:29, caseq wrote: > I wonder whether we should better use actual instance of the setting rather than > referring to it by a name. I want it to be a config template. No objects allowed. https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:162: ].map(cls => new cls()); On 2016/12/07 20:17:29, caseq wrote: > let's just call constructors expecitly. Done. https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:165: const div = headerDiv.createChild('div', 'timeline-perspective-header'); On 2016/12/07 20:17:29, caseq wrote: > Does this fail gracefully when the window is too narrow? Should we re-use some > existent control -- perhaps tabbed pane to take advantage of the way it > overflows? Done. https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:189: this._perspectiveSetting.removeChangeListener(this._updatePerspective, this); On 2016/12/07 20:17:29, caseq wrote: > why do you need it? Done. https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js (right): https://codereview.chromium.org/2557973002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:87: this._timelinePane = timelinePane; On 2016/12/07 20:17:29, caseq wrote: > let's get rid of local var for simplicity. Done.
https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:11: value: false, So what do these values mean? They are global and each perspective is going to modify a different subset of those. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:53: this.id = id; is this used? https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:54: this.title = title; ditto. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:55: this.description = description; do we need it as a member? https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:56: this._visibleOptions = visibleOptions || []; why is this necessary? https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:59: for (let id of this._visibleOptions) this shadows a function parameter, can we have a different name? I know someone who can get confused by this. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:104: Timeline.RecordingConfig.network.value = true; Does it actually have any effect? https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:140: ['network', 'javascript', 'screenshots', 'memory', 'paints']); I'd prefer variables instead of strings there, so the compiler could do its job.
all done. ptal https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:11: value: false, On 2016/12/08 17:59:04, caseq wrote: > So what do these values mean? They are global and each perspective is going to > modify a different subset of those. These are meta defaults. I have to pass a default when creating a setting object. Each perspective can then adjust them. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:53: this.id = id; On 2016/12/08 17:59:04, caseq wrote: > is this used? Done. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:54: this.title = title; On 2016/12/08 17:59:04, caseq wrote: > ditto. Done. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:55: this.description = description; On 2016/12/08 17:59:04, caseq wrote: > do we need it as a member? Done. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:56: this._visibleOptions = visibleOptions || []; On 2016/12/08 17:59:04, caseq wrote: > why is this necessary? Done. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:59: for (let id of this._visibleOptions) On 2016/12/08 17:59:04, caseq wrote: > this shadows a function parameter, can we have a different name? I know someone > who can get confused by this. Done. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:104: Timeline.RecordingConfig.network.value = true; On 2016/12/08 17:59:04, caseq wrote: > Does it actually have any effect? Done. https://codereview.chromium.org/2557973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:140: ['network', 'javascript', 'screenshots', 'memory', 'paints']); On 2016/12/08 17:59:04, caseq wrote: > I'd prefer variables instead of strings there, so the compiler could do its job. Done.
https://codereview.chromium.org/2557973002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:12: value: false, s/value/forceEnable/? https://codereview.chromium.org/2557973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:62: this._createSettingCheckBox(this.contentElement, config, enable); This queitly looses the "forceEnable" value semantics as we convert it to defaultValue -- so when the setting already exists, the forceEnable is actually not taken into account. https://codereview.chromium.org/2557973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:104: [Timeline.RecordingConfig.screenshots.id], Let's pass Timeline.RecordingConfig.screenshots, Timeline.RecordingConfig.network etc?
All done. ptal https://codereview.chromium.org/2557973002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:12: value: false, On 2016/12/09 02:11:12, caseq wrote: > s/value/forceEnable/? Done. https://codereview.chromium.org/2557973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:62: this._createSettingCheckBox(this.contentElement, config, enable); On 2016/12/09 02:11:12, caseq wrote: > This queitly looses the "forceEnable" value semantics as we convert it to > defaultValue -- so when the setting already exists, the forceEnable is actually > not taken into account. Done. https://codereview.chromium.org/2557973002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:104: [Timeline.RecordingConfig.screenshots.id], On 2016/12/09 02:11:12, caseq wrote: > Let's pass Timeline.RecordingConfig.screenshots, > Timeline.RecordingConfig.network etc? Done.
https://codereview.chromium.org/2557973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:68: _createSettingCheckBox(parent, config) { use UI.SettingsUI.createSettingCheckbox()?
https://codereview.chromium.org/2557973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:68: _createSettingCheckBox(parent, config) { On 2016/12/09 21:53:12, caseq wrote: > use UI.SettingsUI.createSettingCheckbox()? thanks! done
https://codereview.chromium.org/2557973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:54: this._enabledOptions.add(Timeline.RecordingConfig.javascript.id); So can I ever disable JavaScript?
https://codereview.chromium.org/2557973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:54: this._enabledOptions.add(Timeline.RecordingConfig.javascript.id); On 2016/12/09 22:05:25, caseq wrote: > So can I ever disable JavaScript? Sure, you can. These are just defaults we show when the perspective is opened (activated).
https://codereview.chromium.org/2557973002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. The class name should match the file name. You define 6 classes in one file and none of the names match the file name. You should have: Timeline.TimelineLandingPage Timeline.TimelineLandingPage.PresetsTab Timeline.TimelineLandingPage.RecordingOption etc. https://codereview.chromium.org/2557973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:44: Timeline.Perspective = class extends UI.VBox { ...TabWidget? https://codereview.chromium.org/2557973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:90: Timeline.LoadPerspective = class extends Timeline.Perspective { Inheritance should be out last resort, you don't need it here. https://codereview.chromium.org/2557973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:143: this._tabbedPane.registerRequiredCSS('timeline/timelineLandingPage.css'); We should not abuse tabbed pane and style it externally. https://codereview.chromium.org/2557973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:144: this._tabbedPane.registerRequiredCSS('ui_lazy/dialog.css'); ditto https://codereview.chromium.org/2557973002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:145: this._tabbedPane.contentElement.classList.add('timeline-landing-page'); Also don't
all done. ptal
https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js (right): https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:34: [config.network, config.javascript, config.screenshots, config.memory, config.paints], var tab = new Tab(id); tab.setDescription(.....); tab.appendSetting(config.network, true, true); tab.appendSetting(config.javascript, true, false); tab.appendAction(button); tab.appendAction(button); ... appendTab(tab); https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css (right): https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:17: .timeline-landing-page .tabbed-pane-header { This should be removed. https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:23: .timeline-landing-page .tabbed-pane-header-contents { This should be removed. https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:27: .timeline-landing-page .tabbed-pane-header-tab { This should be removed. https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:49: .timeline-landing-page button.action-button { Please use devtools button custom element. https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:61: .timeline-landing-page .recording-setting input { Please use devtools checkbox with label custom element. https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:66: .timeline-landing-page .recording-setting label { ditto https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:826: renderWithNoTabBorders() { Same as slider?
On 2016/12/10 02:15:59, pfeldman wrote: > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js > (right): > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/timeline/TimelineLandingPage.js:34: > [config.network, config.javascript, config.screenshots, config.memory, > config.paints], > var tab = new Tab(id); > tab.setDescription(.....); > tab.appendSetting(config.network, true, true); > tab.appendSetting(config.javascript, true, false); > tab.appendAction(button); > tab.appendAction(button); > ... > appendTab(tab); > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css > (right): > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:17: > .timeline-landing-page .tabbed-pane-header { > This should be removed. > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:23: > .timeline-landing-page .tabbed-pane-header-contents { > This should be removed. > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:27: > .timeline-landing-page .tabbed-pane-header-tab { > This should be removed. > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:49: > .timeline-landing-page button.action-button { > Please use devtools button custom element. > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:61: > .timeline-landing-page .recording-setting input { > Please use devtools checkbox with label custom element. > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:66: > .timeline-landing-page .recording-setting label { > ditto > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js (right): > > https://codereview.chromium.org/2557973002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/devtools/front_end/ui/TabbedPane.js:826: > renderWithNoTabBorders() { > Same as slider? All done. Ptal
lgtm
https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js (right): https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:582: this._clear(); I don't think this should be here.
https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css (right): https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:32: width: 90px; let's not do that, it's a landmine. https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:44: flex: 100px 0 0; we typically put basis at the end of the shorthand.
https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js (right): https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/TimelinePanel.js:582: this._clear(); On 2016/12/12 18:51:53, caseq wrote: > I don't think this should be here. Done. https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css (right): https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:32: width: 90px; On 2016/12/12 19:04:34, caseq wrote: > let's not do that, it's a landmine. I need to specify width to make sure all the [potential] buttons have the same size. Though I can make it less painful, by using min-width. https://codereview.chromium.org/2557973002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/timeline/timelineLandingPage.css:44: flex: 100px 0 0; On 2016/12/12 19:04:34, caseq wrote: > we typically put basis at the end of the shorthand. Indeed. There are 60 instances vs 14.
lgtm
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pfeldman@chromium.org, caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2557973002/#ps180001 (title: "minor tweaks")
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": 180001, "attempt_start_ts": 1481575618658000,
"parent_rev": "b505c372b843f9ee11225abd84e44454e19b27e4", "commit_rev":
"d2e1c86892b6969004493407a61796e63ac58c3f"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: Introduce Landing page for Timeline panel. Provide more user-friendly experience as a welcome screen on Timeline panel. The new UX is put behind an experiment. BUG=570441 ========== to ========== DevTools: Introduce Landing page for Timeline panel. Provide more user-friendly experience as a welcome screen on Timeline panel. The new UX is put behind an experiment. BUG=570441 Review-Url: https://codereview.chromium.org/2557973002 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Introduce Landing page for Timeline panel. Provide more user-friendly experience as a welcome screen on Timeline panel. The new UX is put behind an experiment. BUG=570441 Review-Url: https://codereview.chromium.org/2557973002 ========== to ========== DevTools: Introduce Landing page for Timeline panel. Provide more user-friendly experience as a welcome screen on Timeline panel. The new UX is put behind an experiment. BUG=570441 Committed: https://crrev.com/6741acb07eaa3bfd69c6871179511fca278bff62 Cr-Commit-Position: refs/heads/master@{#437979} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6741acb07eaa3bfd69c6871179511fca278bff62 Cr-Commit-Position: refs/heads/master@{#437979} |
