Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2199103002:
DevTools: fix animations panel after image loading became async (Closed)
|
Created:
4 years, 4 months ago by luoe Modified:
4 years, 4 months ago Reviewers:
lushnikov CC:
chromium-reviews, caseq+blink_chromium.org, shans, rjwright, blink-reviews-animation_chromium.org, darktears, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, Eric Willigers, pfeldman, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: fix animations panel after image loading became async
The loading of data URIs is now async, which affected the visibility of
animation screenshot popovers. Popover measurement is taken right after being
shown, so this CL waits for images to load before taking the measurement.
See https://codereview.chromium.org/2173003002
BUG=632529
Committed: https://crrev.com/3b90447cdf5dcc6ea76146ebf3b63881cd7a3e0f
Cr-Commit-Position: refs/heads/master@{#409330}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #Patch Set 3 : jsdoc+1 #Patch Set 4 : Switched back to using first image #Patch Set 5 : put into one file #Messages
Total messages: 20 (9 generated)
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
Ptal
https://codereview.chromium.org/2199103002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js (right): https://codereview.chromium.org/2199103002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:823: * @param {function(?)=} onImageLoad Let's not pass onImageLoad here https://codereview.chromium.org/2199103002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js (right): https://codereview.chromium.org/2199103002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:197: if (screenshots[0].complete) why do we care only about the 0's screenshot?
https://codereview.chromium.org/2199103002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js (right): https://codereview.chromium.org/2199103002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/animation/AnimationModel.js:823: * @param {function(?)=} onImageLoad On 2016/08/01 23:16:11, lushnikov wrote: > Let's not pass onImageLoad here Yeah, I should have realized this was awkward. Moved to a public setOnScreenshotsLoaded() function. https://codereview.chromium.org/2199103002/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js (right): https://codereview.chromium.org/2199103002/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/animation/AnimationTimeline.js:197: if (screenshots[0].complete) On 2016/08/01 23:16:11, lushnikov wrote: > why do we care only about the 0's screenshot? We shouldn't :) Changed to keep a counter for remaining loading images. When it hits 0, we call onImageLoad().
I don't follow. Why would you postpone adding images to DOM until they load?
Prior to this CL, images are requested and then we call popover.showView() before the images have loaded yet. The logic in Popover's _innerShow calls view.measurePreferredSize(), which calculates and saves the dimensions of the element. Since the images haven't loaded, the offsetWidth/Height is 0/0, so the popover is immediately inserted into the page but can't be seen. Therefore, we need some visible image to load before we take measurements. AnimationScreenshotPopover's constructor makes every image display: none except for the first, so we actually don't need to wait for the rest.
Description was changed from ========== DevTools: fix animations panel after image loading became async BUG=632529 ========== to ========== DevTools: fix animations panel after image loading became async The loading of data URIs is now async, which affected the visibility of animation screenshot popovers. Popover measurement is taken right after being shown, so this CL waits for images to load before taking the measurement. See https://codereview.chromium.org/2173003002 BUG=632529 ==========
let's just localize everything into a single AnimationTimeline file.
Done.
lgtm, thanks
The CQ bit was checked by luoe@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.
The CQ bit was checked by luoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: fix animations panel after image loading became async The loading of data URIs is now async, which affected the visibility of animation screenshot popovers. Popover measurement is taken right after being shown, so this CL waits for images to load before taking the measurement. See https://codereview.chromium.org/2173003002 BUG=632529 ========== to ========== DevTools: fix animations panel after image loading became async The loading of data URIs is now async, which affected the visibility of animation screenshot popovers. Popover measurement is taken right after being shown, so this CL waits for images to load before taking the measurement. See https://codereview.chromium.org/2173003002 BUG=632529 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: fix animations panel after image loading became async The loading of data URIs is now async, which affected the visibility of animation screenshot popovers. Popover measurement is taken right after being shown, so this CL waits for images to load before taking the measurement. See https://codereview.chromium.org/2173003002 BUG=632529 ========== to ========== DevTools: fix animations panel after image loading became async The loading of data URIs is now async, which affected the visibility of animation screenshot popovers. Popover measurement is taken right after being shown, so this CL waits for images to load before taking the measurement. See https://codereview.chromium.org/2173003002 BUG=632529 Committed: https://crrev.com/3b90447cdf5dcc6ea76146ebf3b63881cd7a3e0f Cr-Commit-Position: refs/heads/master@{#409330} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/3b90447cdf5dcc6ea76146ebf3b63881cd7a3e0f Cr-Commit-Position: refs/heads/master@{#409330} |
