* SUMMARY Replace content view container with a content browser. The content browser's navigation bar has advantages for the redesigned UI: - Creates a visual separation between the overview graphs and timeline view. - Provides a place to relocate the filter and scope bar elements that exist in the sidebar. - Show the path components specific to the selection in the timeline view. We currently show these in the main navigation bar above the overview graph. It makes more sense to show it directly above view to which it applies.
<rdar://problem/24195565>
Created attachment 272150 [details] [Video] selection path components UI
Created attachment 272166 [details] [Patch] Proposed Fix
Comment on attachment 272166 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=272166&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineIcons.css:87 > + filter: opacity(0.6); opacity: 0.6; > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:52 > + this._timelineSelectionPathComponent.previousSibling = this._entireRecordingPathComponent; Should also do: this._entireRecordingPathComponent.nextSibling = this._timelineSelectionPathComponent; > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366 > + this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel); Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:685 > + let zeroTime = this._timelineOverview.viewMode === WebInspector.TimelineOverview.ViewMode.Timelines ? this.currentTimelineView.zeroTime : 0; Why won't this.currentTimelineView.zeroTime work for either mode? > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:746 > + this._entireRecordingPathComponent.nextSibling = this._timelineSelectionPathComponent; Maybe this is why you don't set nextSibling at the beginning. If so that might be worth a comment above. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:754 > + startValue += 1; // Convert index to frame number. One space. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:797 > +WebInspector.TimelineRange = class TimelineRange Should be in a new file. One class per file.
Comment on attachment 272166 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=272166&action=review Looking good! I had mostly style nits. > Source/WebInspectorUI/ChangeLog:107 > + (WebInspector.TimelineRange): > + (WebInspector.TimelineRange.prototype.get startValue): > + (WebInspector.TimelineRange.prototype.set startValue): > + (WebInspector.TimelineRange.prototype.get endValue): > + (WebInspector.TimelineRange.prototype.set endValue): > + New represented object for ruler selection path components. We should pull this out into its own Model file then, and it should extend WebInspector.Object. Just for consistency: UserInterface/Models/TimelineRange.js Which should probably also be included by Test.html. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:46 > + let disableBackForward = true; Style: For temps like this that name a parameter I've been using `const`. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:50 > + this._entireRecordingPathComponent = this._createTimelineRangePathComponent(WebInspector.UIString("Entire Recording")); Nit: I think xenon suggested lowercasing the second word here: "Entire recording". > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:349 > + return new WebInspector.GeneralTreeElement(iconClassName, title, representedObject, false); Nit: You could have a `const hasChildren = false` here for clarity. >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366 >> + this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel); > > Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444. We may want to rename "navigationSidebarTreeOutlineLabel" to something more appropriate now that there is no sidebar (right?). > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:396 > + timelineRuler.selectionStartTime = timelineRuler.zeroTime + timelineRange.startValue > + timelineRuler.selectionEndTime = timelineRuler.zeroTime + timelineRange.endValue Style: Semicolon. >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:754 >> + startValue += 1; // Convert index to frame number. > > One space. Style: For a trailing line comment, one space after the semicolon is the norm. // Like so. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:762 > + this._timelineSelectionPathComponent.title = displayName; I think this line is stale. There is setter for `title` in HierarchicalPathComponent, and nothing seems to use it. Also, HierarchicalPathComponent seem to use the displayName as the tooltip title on the element already. >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:797 >> +WebInspector.TimelineRange = class TimelineRange > > Should be in a new file. One class per file. Deserves its own file. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:808 > + get startValue() > + { > + return this._startValue; > + } Style: Lately for trivial getters/setters we've been single lining them: // Public get startValue() { return this._startValue; } set startValue(x) { this._startValue = x; }
Comment on attachment 272166 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=272166&action=review >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:50 >> + this._entireRecordingPathComponent = this._createTimelineRangePathComponent(WebInspector.UIString("Entire Recording")); > > Nit: I think xenon suggested lowercasing the second word here: "Entire recording". I sugested Entire Recording, which is what he did. The video he posted had recording lowercase. He corrected it for the patch.
(In reply to comment #4) > Comment on attachment 272166 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272166&action=review > > > Source/WebInspectorUI/UserInterface/Views/TimelineIcons.css:87 > > + filter: opacity(0.6); > > opacity: 0.6; > > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:52 > > + this._timelineSelectionPathComponent.previousSibling = this._entireRecordingPathComponent; > > Should also do: > this._entireRecordingPathComponent.nextSibling = > this._timelineSelectionPathComponent; > > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366 > > + this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel); > > Might add a FIXME here to localize whole strings sainted of using %s. > Localizers like whole strings in cases line this so they can control it > better. See rdar://problem/24206444. > > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:685 > > + let zeroTime = this._timelineOverview.viewMode === WebInspector.TimelineOverview.ViewMode.Timelines ? this.currentTimelineView.zeroTime : 0; > > Why won't this.currentTimelineView.zeroTime work for either mode? Like all timeline views, the rendering frames view has a zero time equal to that of the recording. Since it isn't used it would probably be cleaner to override it in the view and remove this special case. > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:746 > > + this._entireRecordingPathComponent.nextSibling = this._timelineSelectionPathComponent; > > Maybe this is why you don't set nextSibling at the beginning. If so that > might be worth a comment above. Correct. "Entire Recording" has no siblings until a selection is made. This state is fleeting since a default selection is created immediately, but that's an implementation detail of TimelineOverview. > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:754 > > + startValue += 1; // Convert index to frame number. > > One space. > > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:797 > > +WebInspector.TimelineRange = class TimelineRange > > Should be in a new file. One class per file.
(In reply to comment #5) > Comment on attachment 272166 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=272166&action=review > > Looking good! I had mostly style nits. > > > Source/WebInspectorUI/ChangeLog:107 > > + (WebInspector.TimelineRange): > > + (WebInspector.TimelineRange.prototype.get startValue): > > + (WebInspector.TimelineRange.prototype.set startValue): > > + (WebInspector.TimelineRange.prototype.get endValue): > > + (WebInspector.TimelineRange.prototype.set endValue): > > + New represented object for ruler selection path components. > > We should pull this out into its own Model file then, and it should extend > WebInspector.Object. Just for consistency: > > UserInterface/Models/TimelineRange.js > > Which should probably also be included by Test.html. > > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:46 > > + let disableBackForward = true; > > Style: For temps like this that name a parameter I've been using `const`. Nice. > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:50 > > + this._entireRecordingPathComponent = this._createTimelineRangePathComponent(WebInspector.UIString("Entire Recording")); > > Nit: I think xenon suggested lowercasing the second word here: "Entire > recording". > > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:349 > > + return new WebInspector.GeneralTreeElement(iconClassName, title, representedObject, false); > > Nit: You could have a `const hasChildren = false` here for clarity. > > >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366 > >> + this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel); > > > > Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444. > > We may want to rename "navigationSidebarTreeOutlineLabel" to something more > appropriate now that there is no sidebar (right?). Maybe we should remove this altogether. It's only used in the filter bar placeholder text now, and most (4 of 6) timeline views just return "Records". > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:396 > > + timelineRuler.selectionStartTime = timelineRuler.zeroTime + timelineRange.startValue > > + timelineRuler.selectionEndTime = timelineRuler.zeroTime + timelineRange.endValue > > Style: Semicolon. > > >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:754 > >> + startValue += 1; // Convert index to frame number. > > > > One space. > > Style: For a trailing line comment, one space after the semicolon is the > norm. // Like so. > > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:762 > > + this._timelineSelectionPathComponent.title = displayName; > > I think this line is stale. There is setter for `title` in > HierarchicalPathComponent, and nothing seems to use it. Also, > HierarchicalPathComponent seem to use the displayName as the tooltip title > on the element already. Probably a copy-paste error. HierarchicalPathComponent has no `title` property. > >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:797 > >> +WebInspector.TimelineRange = class TimelineRange > > > > Should be in a new file. One class per file. > > Deserves its own file. > > > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:808 > > + get startValue() > > + { > > + return this._startValue; > > + } > > Style: Lately for trivial getters/setters we've been single lining them: > > // Public > > get startValue() { return this._startValue; } > set startValue(x) { this._startValue = x; }
Comment on attachment 272166 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=272166&action=review >>>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:50 >>>> + this._entireRecordingPathComponent = this._createTimelineRangePathComponent(WebInspector.UIString("Entire Recording")); >>> >>> Nit: I think xenon suggested lowercasing the second word here: "Entire recording". >> >> I sugested Entire Recording, which is what he did. The video he posted had recording lowercase. He corrected it for the patch. > > Maybe we should remove this altogether. It's only used in the filter bar placeholder text now, and most (4 of 6) timeline views just return "Records". Commented on the wrong line. >>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366 >>> + this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel); >> >> Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444. > > We may want to rename "navigationSidebarTreeOutlineLabel" to something more appropriate now that there is no sidebar (right?). Let's remove this and just use "Filter Records". Most timeline views return "Records" anyway, and it's only used in the placeholder text now.
Comment on attachment 272166 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=272166&action=review >>>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366 >>>> + this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel); >>> >>> Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444. >> >> We may want to rename "navigationSidebarTreeOutlineLabel" to something more appropriate now that there is no sidebar (right?). > > Let's remove this and just use "Filter Records". Most timeline views return "Records" anyway, and it's only used in the placeholder text now. Works for me.
Created attachment 272224 [details] [Patch] Proposed Fix
Comment on attachment 272224 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=272224&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineIcons.css:87 > + opacity: 0.6); Stray ). Needs removed.
Created attachment 272225 [details] [Patch] Proposed Fix
Created attachment 272695 [details] [Patch] Proposed Fix
Comment on attachment 272695 [details] [Patch] Proposed Fix Rejecting attachment 272695 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 272695, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 01f82785f6ce0adbc491c9dce28 r197476 = 5405850fc2be6a01894c329602aee0b564fa399b r197478 = 0fff5f339ba9d4e782f7ae563b3f6c1f3457461f r197479 = 176cf13651db31cb6f644f9960e78d2f305e8bf6 Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/origin/master. Auto packing the repository in background for optimum performance. See "git help gc" for manual housekeeping. Full output: http://webkit-queues.webkit.org/results/913120
Created attachment 272709 [details] [Patch] Proposed Fix
Committed r197488: <https://trac.webkit.org/r197488>
Comment on attachment 272709 [details] [Patch] Proposed Fix Rejecting attachment 272709 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 272709, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: a/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit Parsed 11 diffs from patch file(s). patching file Source/WebInspectorUI/ChangeLog Hunk #1 succeeded at 1 with fuzz 1. Original content of Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 283. Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.webkit.org/results/913837