Chromium Code Reviews
|
|
|
Created:
4 years, 10 months ago by alph Modified:
4 years, 10 months ago CC:
blink-reviews, caseq+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDevTools: Support popover on timeline overview.
Populate popover with filmstrip screenshots.
BUG=500881
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197294
Patch Set 1 #
Total comments: 32
Patch Set 2 : Addressing comments. #
Total comments: 3
Patch Set 3 : Removed elements cache #
Total comments: 1
Patch Set 4 : index-based -> frame-based #
Messages
Total messages: 26 (9 generated)
alph@chromium.org changed reviewers: + caseq@chromium.org, pfeldman@chromium.org, yurys@chromium.org
https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... File Source/devtools/front_end/components_lazy/FilmStripView.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:67: var frame = this._model.frames()[index]; Is this method guaranteed to always be called after setModel()? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:98: return Math.max(this._model.frames().upperBound(time, comparator) - 1, 0); ditto. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:109: this._label.remove(); nit: please remove label into something more descriptive. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:113: this._framesContainer.appendChild(this.frameElementByIndex(i).cloneNode(true)); So what's the purpose of the cache if we're cloning all the nodes? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:127: var element = this._framesContainer.appendChild(this.frameElementByIndex(index).cloneNode(true)); ditto. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... File Source/devtools/front_end/components_lazy/filmStripView.css (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/filmStripView.css:15: position: absolute; please consider using .fill instead. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/t... File Source/devtools/front_end/timeline/TimelinePanel.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/t... Source/devtools/front_end/timeline/TimelinePanel.js:1844: if (!this._filmStripModel.frames().length) Is this guaranteed to always be called after update()? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/t... File Source/devtools/front_end/timeline/timelinePanel.css (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/t... Source/devtools/front_end/timeline/timelinePanel.css:876: width: 200px; max-width perhaps? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui/Popover.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui/Popover.js:164: positionElement: function(anchorElement, preferredWidth, preferredHeight, arrowDirection) Exposing this does not seem to be a good style. Can we rather support repositioning popover internally within the popover? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui_lazy/OverviewGrid.js (left): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:171: this._currentPositionArea.addEventListener("mousemove", this._onMouseMove.bind(this), true); So how does this work for code that directly uses WebInspector.OverviewGrid.Window? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui_lazy/OverviewGrid.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:39: this._enabled = false; What is not enabled here? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:57: cursorElement: function() This looks like a weird interface. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:163: this._enabled = !!enabled; nit: we don't coerce to boolean parameters that are annotated as boolean. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:235: this._enabled = !!enabled; nit: drop !!, we don't coerce to boolean whatever is annotated as boolean. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js:274: WebInspector.TimelineOverviewPane.PopoverContents = function() Do you really need a class for this? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js:419: popoverElement: function(time) { }, Why is this interface in terms of time, not window position? Different overviews have different position to time mappings.
ptal https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... File Source/devtools/front_end/components_lazy/FilmStripView.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:67: var frame = this._model.frames()[index]; On 2015/06/16 14:14:49, caseq wrote: > Is this method guaranteed to always be called after setModel()? Yes. I can cover it with assert if you want. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:98: return Math.max(this._model.frames().upperBound(time, comparator) - 1, 0); On 2015/06/16 14:14:49, caseq wrote: > ditto. ditto. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:109: this._label.remove(); On 2015/06/16 14:14:49, caseq wrote: > nit: please remove label into something more descriptive. I actually didn't introduce this name, but ok renamed it to _statusLabel https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:113: this._framesContainer.appendChild(this.frameElementByIndex(i).cloneNode(true)); On 2015/06/16 14:14:49, caseq wrote: > So what's the purpose of the cache if we're cloning all the nodes? Popover uses cached versions directly. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/FilmStripView.js:127: var element = this._framesContainer.appendChild(this.frameElementByIndex(index).cloneNode(true)); On 2015/06/16 14:14:49, caseq wrote: > ditto. ditto. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... File Source/devtools/front_end/components_lazy/filmStripView.css (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... Source/devtools/front_end/components_lazy/filmStripView.css:15: position: absolute; On 2015/06/16 14:14:49, caseq wrote: > please consider using .fill instead. Nuked the div. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/t... File Source/devtools/front_end/timeline/TimelinePanel.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/t... Source/devtools/front_end/timeline/TimelinePanel.js:1844: if (!this._filmStripModel.frames().length) On 2015/06/16 14:14:49, caseq wrote: > Is this guaranteed to always be called after update()? Guarded. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/t... File Source/devtools/front_end/timeline/timelinePanel.css (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/t... Source/devtools/front_end/timeline/timelinePanel.css:876: width: 200px; On 2015/06/16 14:14:49, caseq wrote: > max-width perhaps? Done. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui/Popover.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui/Popover.js:164: positionElement: function(anchorElement, preferredWidth, preferredHeight, arrowDirection) On 2015/06/16 14:14:49, caseq wrote: > Exposing this does not seem to be a good style. Can we rather support > repositioning popover internally within the popover? Could you elaborate. Do you mean make it auto reposition when anchor moves? https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui_lazy/OverviewGrid.js (left): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:171: this._currentPositionArea.addEventListener("mousemove", this._onMouseMove.bind(this), true); On 2015/06/16 14:14:49, caseq wrote: > So how does this work for code that directly uses > WebInspector.OverviewGrid.Window? What code? Anyway moved the cursor stuff to TimelineOverviewPane. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui_lazy/OverviewGrid.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:39: this._enabled = false; On 2015/06/16 14:14:49, caseq wrote: > What is not enabled here? The cursor. Renamed. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:57: cursorElement: function() On 2015/06/16 14:14:49, caseq wrote: > This looks like a weird interface. Changed. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:163: this._enabled = !!enabled; On 2015/06/16 14:14:49, caseq wrote: > nit: we don't coerce to boolean parameters that are annotated as boolean. Done. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/OverviewGrid.js:235: this._enabled = !!enabled; On 2015/06/16 14:14:50, caseq wrote: > nit: drop !!, we don't coerce to boolean whatever is annotated as boolean. Done. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... File Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js (right): https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js:274: WebInspector.TimelineOverviewPane.PopoverContents = function() On 2015/06/16 14:14:50, caseq wrote: > Do you really need a class for this? I made it a component, so children can inject their stylesheets into it. https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js:419: popoverElement: function(time) { }, On 2015/06/16 14:14:50, caseq wrote: > Why is this interface in terms of time, not window position? Different overviews > have different position to time mappings. Done.
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1183483011/20001
On 2015/06/17 09:17:09, alph wrote: > https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... > File Source/devtools/front_end/components_lazy/FilmStripView.js (right): > > https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... > Source/devtools/front_end/components_lazy/FilmStripView.js:67: var frame = > this._model.frames()[index]; > On 2015/06/16 14:14:49, caseq wrote: > > Is this method guaranteed to always be called after setModel()? > > Yes. I can cover it with assert if you want. I don't think assert is needed -- this will crash upon pre-condition violation anyway. > https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... > Source/devtools/front_end/components_lazy/FilmStripView.js:113: > this._framesContainer.appendChild(this.frameElementByIndex(i).cloneNode(true)); > On 2015/06/16 14:14:49, caseq wrote: > > So what's the purpose of the cache if we're cloning all the nodes? > > Popover uses cached versions directly. So let me understand -- we're creating two images per frame in advance based on an assumption that one of them eventually may be needed in case popover is shown? > https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... > Source/devtools/front_end/ui/Popover.js:164: positionElement: > function(anchorElement, preferredWidth, preferredHeight, arrowDirection) > On 2015/06/16 14:14:49, caseq wrote: > > Exposing this does not seem to be a good style. Can we rather support > > repositioning popover internally within the popover? > > Could you elaborate. Do you mean make it auto reposition when anchor moves? Yes, but actually I think current version looks reasonable.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/06/17 14:03:18, caseq wrote: > On 2015/06/17 09:17:09, alph wrote: > > > > https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... > > File Source/devtools/front_end/components_lazy/FilmStripView.js (right): > > > > > https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... > > Source/devtools/front_end/components_lazy/FilmStripView.js:67: var frame = > > this._model.frames()[index]; > > On 2015/06/16 14:14:49, caseq wrote: > > > Is this method guaranteed to always be called after setModel()? > > > > Yes. I can cover it with assert if you want. > > I don't think assert is needed -- this will crash upon pre-condition violation > anyway. ok > > > > https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/c... > > Source/devtools/front_end/components_lazy/FilmStripView.js:113: > > > this._framesContainer.appendChild(this.frameElementByIndex(i).cloneNode(true)); > > On 2015/06/16 14:14:49, caseq wrote: > > > So what's the purpose of the cache if we're cloning all the nodes? > > > > Popover uses cached versions directly. > > So let me understand -- we're creating two images per frame in advance based on > an assumption that one of them eventually may be needed in case popover is > shown? Yes, the idea was to make popover frames get ready as fast as possible. But ok let's go the straighforward way first and optimize if needed. > > > > https://codereview.chromium.org/1183483011/diff/1/Source/devtools/front_end/u... > > Source/devtools/front_end/ui/Popover.js:164: positionElement: > > function(anchorElement, preferredWidth, preferredHeight, arrowDirection) > > On 2015/06/16 14:14:49, caseq wrote: > > > Exposing this does not seem to be a good style. Can we rather support > > > repositioning popover internally within the popover? > > > > Could you elaborate. Do you mean make it auto reposition when anchor moves? > > Yes, but actually I think current version looks reasonable.
https://codereview.chromium.org/1183483011/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/ui_lazy/OverviewGrid.js (right): https://codereview.chromium.org/1183483011/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui_lazy/OverviewGrid.js:39: this._enabled = false; Is this still used? https://codereview.chromium.org/1183483011/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js (right): https://codereview.chromium.org/1183483011/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js:115: var x = cursor.offsetLeft; Can we instead do pass this from outside, so we don't force layout here? https://codereview.chromium.org/1183483011/diff/20001/Source/devtools/front_e... Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js:333: positionToTime: function(position) unused?
ptal
lgtm Can we expose interface in terms of frames instead of frame indices? https://codereview.chromium.org/1183483011/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/ui_lazy/OverviewGrid.js (right): https://codereview.chromium.org/1183483011/diff/40001/Source/devtools/front_e... Source/devtools/front_end/ui_lazy/OverviewGrid.js:39: this._enabled = false; remove?
On 2015/06/17 17:36:48, caseq wrote: > lgtm > > Can we expose interface in terms of frames instead of frame indices? done.
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/1183483011/#ps60001 (title: "index-based -> frame-based")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1183483011/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1183483011/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59313)
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1183483011/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197294 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
