Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1184383002:
DevTools: adopt FilmStripModel to new screenshot recorder trace format (Closed)
|
Created:
5 years, 6 months ago by caseq Modified:
5 years, 6 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: adopt FilmStripModel to new screenshot recorder trace format
Screenshots are about to be emitted by browser as object snapshots
rather than instant trace events, so prepare to consuming both these
and old events. Since snapshot contents may reside in the backing
sotrage and should be requested asynchronously, adopt FilmStripModel
usage by clients as well.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197580
Patch Set 1 #Patch Set 2 : make imageDataForFrame map a parameter, not a member #
Total comments: 5
Patch Set 3 : promisified #
Total comments: 4
Patch Set 4 : removed accidental change #
Total comments: 3
Patch Set 5 : review comments addressed #Patch Set 6 : rebased #
Total comments: 1
Patch Set 7 : made popover generation async #
Total comments: 5
Patch Set 8 : don't use map() with createFrameElement #Patch Set 9 : get rid of callback in createFrameElement() #
Total comments: 3
Patch Set 10 : use Promise.all() before appending children #Patch Set 11 : #
Messages
Total messages: 35 (10 generated)
caseq@chromium.org changed reviewers: + alph@chromium.org, pfeldman@chromium.org
https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/components_lazy/FilmStripModel.js (right): https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripModel.js:31: var data = events[i].args.data; nit: we usually write args["data"] https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripModel.js:43: } nit: add a line https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripModel.js:137: callback(this._imageData); Consider setting up frame.imageData rather than provide an async interface. So you could get rid of the map in the FilmStripView. https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/components_lazy/FilmStripView.js (right): https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripView.js:69: frame.requestImageData(onImageData.bind(this, frame)); If you turn them into a promises, the code should become much simpler with use of Promise.all https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripView.js:216: WebInspector.FilmStripView._loadImageForFrame = function(image, frame) Can't find where is it used.
On 2015/06/17 09:41:59, alph wrote: > https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... > File Source/devtools/front_end/components_lazy/FilmStripModel.js (right): > > https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/components_lazy/FilmStripModel.js:31: var data = > events[i].args.data; > nit: we usually write args["data"] Fixed. > > https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/components_lazy/FilmStripModel.js:43: } > nit: add a line Done. > https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/components_lazy/FilmStripModel.js:137: > callback(this._imageData); > Consider setting up frame.imageData rather than provide an async interface. So > you could get rid of the map in the FilmStripView. Nope -- we do caching as required in ObjectSnapshot, and I don't want to expose a field on frame that is either present or not depending on some third-party action. > https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... > File Source/devtools/front_end/components_lazy/FilmStripView.js (right): > > https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/components_lazy/FilmStripView.js:69: > frame.requestImageData(onImageData.bind(this, frame)); > If you turn them into a promises, the code should become much simpler with use > of Promise.all Done. > https://codereview.chromium.org/1184383002/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/components_lazy/FilmStripView.js:216: > WebInspector.FilmStripView._loadImageForFrame = function(image, frame) > Can't find where is it used. Nuked it, thanks!
https://codereview.chromium.org/1184383002/diff/40001/Source/devtools/front_e... File Source/devtools/front_end/main/Tests.js (right): https://codereview.chromium.org/1184383002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/main/Tests.js:531: test.assertTrue(frames.length > 2 && typeof frames.length === "number"); We tend to not use assert in tests, but rather log results to the output. https://codereview.chromium.org/1184383002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/main/Tests.js:541: image.src = "data:image/jpg;base64," + frame.imageData; Ain't you just nuked imageData? https://codereview.chromium.org/1184383002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/main/Tests.js:557: test.assertTrue(image.naturalWidth > 10); ditto for assertTrue https://codereview.chromium.org/1184383002/diff/40001/Source/devtools/front_e... Source/devtools/front_end/main/Tests.js:645: WebInspector.multitargetConsoleModel.addEventListener(WebInspector.ConsoleModel.Events.MessageAdded, onConsoleMessage, this); Not sure, but does it make sense to addEventListener before evaluateInConsole?
Ouch, please disregard Tests.js, it's an accidentally uploaded part of another (already reviewed) CL. On 2015/06/17 13:47:51, alph wrote: > https://codereview.chromium.org/1184383002/diff/40001/Source/devtools/front_e... > File Source/devtools/front_end/main/Tests.js (right): > > https://codereview.chromium.org/1184383002/diff/40001/Source/devtools/front_e... > Source/devtools/front_end/main/Tests.js:531: test.assertTrue(frames.length > 2 > && typeof frames.length === "number"); > We tend to not use assert in tests, but rather log results to the output. FYI, this is not a layout test. It does not have its output validated against the golden file.
(re-uploaded w/o Tests.js)
lgtm https://codereview.chromium.org/1184383002/diff/60001/Source/devtools/front_e... File Source/devtools/front_end/components_lazy/FilmStripModel.js (right): https://codereview.chromium.org/1184383002/diff/60001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripModel.js:102: * @param {number} index @return https://codereview.chromium.org/1184383002/diff/60001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripModel.js:107: frame._imageData = event.args.data; nit: args["data"] https://codereview.chromium.org/1184383002/diff/60001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripModel.js:114: * @param {number} index @return
On 2015/06/17 16:59:50, alph wrote: > lgtm > > https://codereview.chromium.org/1184383002/diff/60001/Source/devtools/front_e... > File Source/devtools/front_end/components_lazy/FilmStripModel.js (right): > > https://codereview.chromium.org/1184383002/diff/60001/Source/devtools/front_e... > Source/devtools/front_end/components_lazy/FilmStripModel.js:102: * @param > {number} index > @return > > https://codereview.chromium.org/1184383002/diff/60001/Source/devtools/front_e... > Source/devtools/front_end/components_lazy/FilmStripModel.js:107: > frame._imageData = event.args.data; > nit: args["data"] > > https://codereview.chromium.org/1184383002/diff/60001/Source/devtools/front_e... > Source/devtools/front_end/components_lazy/FilmStripModel.js:114: * @param > {number} index > @return Thanks, all done.
The CQ bit was checked by caseq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/1184383002/#ps80001 (title: "review comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1184383002/80001
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/59282)
The CQ bit was checked by caseq@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org Link to the patchset: https://codereview.chromium.org/1184383002/#ps90001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1184383002/90001
not lgtm https://codereview.chromium.org/1184383002/diff/90001/Source/devtools/front_e... File Source/devtools/front_end/components_lazy/FilmStripView.js (right): https://codereview.chromium.org/1184383002/diff/90001/Source/devtools/front_e... Source/devtools/front_end/components_lazy/FilmStripView.js:105: this._imageDataForFrame = new Map(); What if another update is fired while promises are in flight?
The CQ bit was unchecked by alph@chromium.org
I can see you guys are having fun :)
ptal, I had to make popoverElement() return a promise, since we need to show popover when image data is available in order to properly estimate popover size.
https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... File Source/devtools/front_end/components_lazy/FilmStripView.js (right): https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... Source/devtools/front_end/components_lazy/FilmStripView.js:114: frames.map(this.createFrameElement.bind(this)).forEach(this.contentElement.appendChild.bind(this.contentElement)); Hmm, how does it work? Array.map passes index as the second argument to the callback, which will interpret it as imageLoadedCallback. https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... Source/devtools/front_end/components_lazy/FilmStripView.js:120: var element0 = this.createFrameElement(frames[0], continueWhenFrameImageLoaded.bind(this)); // Calculate frame width basing on the first frame. Why don't stay with your previous schema, but use Promise.race instead, so once you get one of the images you can proceed with building the filmstrip. So you can get rid of that callback. wdyt? https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... File Source/devtools/front_end/timeline/TimelinePanel.js (right): https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... Source/devtools/front_end/timeline/TimelinePanel.js:1841: * @return {!Promise<?Element>} Maybe ?Promise<!Element> instead. By the time of invocation this function already knows if it has something to show in popover or not. https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... Source/devtools/front_end/timeline/TimelinePanel.js:1869: resolve(this._lastElement); Can the this._lastElement change by the time image is loaded? https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... File Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js (right): https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js:463: * @return {!Promise<?Element>} Looks like over complication. Can it just return an element that will update itself when it's ready? To this dynamic update logic is hidden from the client.
On 2015/06/19 14:22:38, alph wrote: > https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... > File Source/devtools/front_end/components_lazy/FilmStripView.js (right): > > https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... > Source/devtools/front_end/components_lazy/FilmStripView.js:114: > frames.map(this.createFrameElement.bind(this)).forEach(this.contentElement.appendChild.bind(this.contentElement)); > Hmm, how does it work? > Array.map passes index as the second argument to the callback, which will > interpret it as imageLoadedCallback. Hmm, indeed. Anyway, this is rewritten now, as createFrameElement() now returns an element promise instead of accepting callback. > > https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... > Source/devtools/front_end/components_lazy/FilmStripView.js:120: var element0 = > this.createFrameElement(frames[0], continueWhenFrameImageLoaded.bind(this)); // > Calculate frame width basing on the first frame. > Why don't stay with your previous schema, but use Promise.race instead, so once > you get one of the images you can proceed with building the filmstrip. So you > can get rid of that callback. wdyt? We still need some sort of callback for the popover. Rewritten createFrameElement() to return promise, this simplifies things a bit. > https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... > File Source/devtools/front_end/timeline/TimelinePanel.js (right): > > https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... > Source/devtools/front_end/timeline/TimelinePanel.js:1841: * @return > {!Promise<?Element>} > Maybe ?Promise<!Element> instead. By the time of invocation this function > already knows if it has something to show in popover or not. Nope. In general, we may fail at later stage, so !Promise<?Element> is more flexible than ?Promise<!Element> and does not really add any complexity given that we still need to get results from all panes. > https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... > Source/devtools/front_end/timeline/TimelinePanel.js:1869: > resolve(this._lastElement); > Can the this._lastElement change by the time image is loaded? > > https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... > File Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js (right): > > https://codereview.chromium.org/1184383002/diff/110001/Source/devtools/front_... > Source/devtools/front_end/ui_lazy/TimelineOverviewPane.js:463: * @return > {!Promise<?Element>} > Looks like over complication. > Can it just return an element that will update itself when it's ready? To this > dynamic update logic is hidden from the client. That was the whole point of this change -- we need image loaded by the time we show popover, so that it is shown in proper size.
https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... File Source/devtools/front_end/components_lazy/FilmStripView.js (right): https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... Source/devtools/front_end/components_lazy/FilmStripView.js:67: frame.imageDataPromise().then(WebInspector.FilmStripView._setImageData.bind(null, imageElement)); you can remove this line. https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... Source/devtools/front_end/components_lazy/FilmStripView.js:75: function returnElement() nit: annotate? honestly, I'd just inline it into then() :-) https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... Source/devtools/front_end/components_lazy/FilmStripView.js:142: function appendElement(element) Are these called in the proper order?
Patchset #10 (id:170001) has been deleted
On 2015/06/22 12:36:22, alph wrote: > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > File Source/devtools/front_end/components_lazy/FilmStripView.js (right): > > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > Source/devtools/front_end/components_lazy/FilmStripView.js:67: > frame.imageDataPromise().then(WebInspector.FilmStripView._setImageData.bind(null, > imageElement)); > you can remove this line. done. > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > Source/devtools/front_end/components_lazy/FilmStripView.js:75: function > returnElement() > nit: annotate? > honestly, I'd just inline it into then() :-) We don't inline functions (officially). > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > Source/devtools/front_end/components_lazy/FilmStripView.js:142: function > appendElement(element) > Are these called in the proper order? Yes.
On 2015/06/22 13:27:05, caseq wrote: > On 2015/06/22 12:36:22, alph wrote: > > > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > > File Source/devtools/front_end/components_lazy/FilmStripView.js (right): > > > > > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > > Source/devtools/front_end/components_lazy/FilmStripView.js:67: > > > frame.imageDataPromise().then(WebInspector.FilmStripView._setImageData.bind(null, > > imageElement)); > > you can remove this line. > > done. > > > > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > > Source/devtools/front_end/components_lazy/FilmStripView.js:75: function > > returnElement() > > nit: annotate? > > honestly, I'd just inline it into then() :-) > > We don't inline functions (officially). > > > > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > > Source/devtools/front_end/components_lazy/FilmStripView.js:142: function > > appendElement(element) > > Are these called in the proper order? > > Yes. Sorry, I don't see why. You fire N promises, they will resolve in an arbitrary order. No?
> https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > > > Source/devtools/front_end/components_lazy/FilmStripView.js:142: function > > > appendElement(element) > > > Are these called in the proper order? > > > > Yes. > > Sorry, I don't see why. You fire N promises, they will resolve in an arbitrary > order. No? The order in which promises resolve is not that of promises in the array supplied by Promise.all. The behavior of Promise.all is defined by the spec (http://www.ecma-international.org/ecma-262/6.0/#sec-promise.all)
On 2015/06/22 13:41:31, caseq wrote: > > > https://codereview.chromium.org/1184383002/diff/150001/Source/devtools/front_... > > > > Source/devtools/front_end/components_lazy/FilmStripView.js:142: function > > > > appendElement(element) > > > > Are these called in the proper order? > > > > > > Yes. > > > > Sorry, I don't see why. You fire N promises, they will resolve in an arbitrary > > order. No? > > The order in which promises resolve is not that of promises in the array > supplied by Promise.all. The behavior of Promise.all is defined by the spec > (http://www.ecma-international.org/ecma-262/6.0/#sec-promise.all) I don't see Promise.all on line 136. You fire a bunch of promises in a loop.
On 2015/06/22 13:52:54, alph wrote: > On 2015/06/22 13:41:31, caseq wrote: > > I don't see Promise.all on line 136. You fire a bunch of promises in a loop. As, sorry, I was mistakenly thinking you're referring to a different place. Changed that one to use Promise.all() before appending elements, too.
Patchset #10 (id:190001) has been deleted
thanks. lgtm.
The CQ bit was checked by caseq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1184383002/230001
Message was sent while issue was closed.
Committed patchset #11 (id:230001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197580 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
