Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629133002:
Avoid UI unresponsiveness when switching between large images. (Closed)
|
Created:
3 years, 11 months ago by yamaguchi Modified:
3 years, 11 months ago CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid UI unresponsiveness when switching between large images.
A new rendering of a big image can cause visible UI freeze even when the
image binary were already prefetched. This change forces to use thumbnail
image to begin slide-in animation quicker.
Downside of this change is that the thumbnail image will always be used
for the slide animation for larger images than the threshold, even when
flipping between 2 adjacent images back and forth. Before this change,
the full image could have been cached in the renderer by chance and might
have been working smoother on some platforms.
BUG=656430
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2629133002
Cr-Commit-Position: refs/heads/master@{#443880}
Committed: https://chromium.googlesource.com/chromium/src/+/9eb76629f83b6d9285876f88ce64e9fbe3ab373c
Patch Set 1 #
Total comments: 4
Patch Set 2 : Early return when metadataItem or any field is undefined. #Patch Set 3 : Fix errors. #
Total comments: 3
Patch Set 4 : Remove @private from the constant to be consistent with others. #
Messages
Total messages: 25 (15 generated)
Description was changed from ========== Avoid UI unresponsiveness when switching between large images. A new rendering of a big image can cause visible UI freeze even when the image binary were already prefetched. This change forces to use thumbnail image to begin slide-in animation quicker. Downside of this change is that the thumbnail image will always be used for the slide animation for larger images than the threshold, even when flipping between 2 adjacent images back and forth. Before this change, the full image could have been cached in the renderer by chance and might have been working smoother on some platforms. BUG=656430 ========== to ========== Avoid UI unresponsiveness when switching between large images. A new rendering of a big image can cause visible UI freeze even when the image binary were already prefetched. This change forces to use thumbnail image to begin slide-in animation quicker. Downside of this change is that the thumbnail image will always be used for the slide animation for larger images than the threshold, even when flipping between 2 adjacent images back and forth. Before this change, the full image could have been cached in the renderer by chance and might have been working smoother on some platforms. BUG=656430 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@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.
yamaguchi@chromium.org changed reviewers: + fukino@chromium.org
ptal
lgtm with a nit https://codereview.chromium.org/2629133002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/gallery_item.js (right): https://codereview.chromium.org/2629133002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/gallery_item.js:451: GalleryItem.HEAVY_RENDERING_THRESHOLD_PIXELS = 4e3 * 3e3; nit: prefer 4000 * 3000 for easier read.
oka@chromium.org changed reviewers: + oka@chromium.org
https://codereview.chromium.org/2629133002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/gallery_item.js (right): https://codereview.chromium.org/2629133002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/gallery_item.js:459: var numPixels = this.metadataItem_.imageHeight * I think imageHeight can be undefined. Maybe it's better to early-return if imageHeight/width is undefined for readability.
The CQ bit was checked by yamaguchi@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: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
https://codereview.chromium.org/2629133002/diff/1/ui/file_manager/gallery/js/... File ui/file_manager/gallery/js/gallery_item.js (right): https://codereview.chromium.org/2629133002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/gallery_item.js:451: GalleryItem.HEAVY_RENDERING_THRESHOLD_PIXELS = 4e3 * 3e3; On 2017/01/13 02:50:33, fukino wrote: > nit: prefer 4000 * 3000 for easier read. Done. https://codereview.chromium.org/2629133002/diff/1/ui/file_manager/gallery/js/... ui/file_manager/gallery/js/gallery_item.js:459: var numPixels = this.metadataItem_.imageHeight * On 2017/01/13 04:23:19, oka wrote: > I think imageHeight can be undefined. Maybe it's better to early-return if > imageHeight/width is undefined for readability. Done. https://codereview.chromium.org/2629133002/diff/40001/ui/file_manager/gallery... File ui/file_manager/gallery/js/gallery_item.js (right): https://codereview.chromium.org/2629133002/diff/40001/ui/file_manager/gallery... ui/file_manager/gallery/js/gallery_item.js:450: * @private Changed to private & const
https://codereview.chromium.org/2629133002/diff/40001/ui/file_manager/gallery... File ui/file_manager/gallery/js/gallery_item.js (right): https://codereview.chromium.org/2629133002/diff/40001/ui/file_manager/gallery... ui/file_manager/gallery/js/gallery_item.js:450: * @private On 2017/01/13 12:25:39, yamaguchi wrote: > Changed to private & const Nit: add a trailing _ if it's private?
(I just drove by. Feel free to commit it without my lgtm.) On Mon, Jan 16, 2017 at 11:23 AM <oka@chromium.org> wrote: > > > https://codereview.chromium.org/2629133002/diff/40001/ui/file_manager/gallery... > File ui/file_manager/gallery/js/gallery_item.js (right): > > > https://codereview.chromium.org/2629133002/diff/40001/ui/file_manager/gallery... > ui/file_manager/gallery/js/gallery_item.js:450: * @private > On 2017/01/13 12:25:39, yamaguchi wrote: > > Changed to private & const > > Nit: add a trailing _ if it's private? > > https://codereview.chromium.org/2629133002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2629133002/diff/40001/ui/file_manager/gallery... File ui/file_manager/gallery/js/gallery_item.js (right): https://codereview.chromium.org/2629133002/diff/40001/ui/file_manager/gallery... ui/file_manager/gallery/js/gallery_item.js:450: * @private On 2017/01/16 02:23:39, oka wrote: > On 2017/01/13 12:25:39, yamaguchi wrote: > > Changed to private & const > > Nit: add a trailing _ if it's private? Now I found we don't use @private for this kind of class constants.
This change will make Gallery app show thumbnail image longer, but it is sometimes rotated wrongly at current head. By this reason, I will merge this changelist after https://codereview.chromium.org/2630283002/ .
The CQ bit was checked by yamaguchi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fukino@chromium.org Link to the patchset: https://codereview.chromium.org/2629133002/#ps60001 (title: "Remove @private from the constant to be consistent with others.")
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": 60001, "attempt_start_ts": 1484570880831190,
"parent_rev": "d28adf3696a330580b5fb705531322ca4f9f3297", "commit_rev":
"9eb76629f83b6d9285876f88ce64e9fbe3ab373c"}
Message was sent while issue was closed.
Description was changed from ========== Avoid UI unresponsiveness when switching between large images. A new rendering of a big image can cause visible UI freeze even when the image binary were already prefetched. This change forces to use thumbnail image to begin slide-in animation quicker. Downside of this change is that the thumbnail image will always be used for the slide animation for larger images than the threshold, even when flipping between 2 adjacent images back and forth. Before this change, the full image could have been cached in the renderer by chance and might have been working smoother on some platforms. BUG=656430 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Avoid UI unresponsiveness when switching between large images. A new rendering of a big image can cause visible UI freeze even when the image binary were already prefetched. This change forces to use thumbnail image to begin slide-in animation quicker. Downside of this change is that the thumbnail image will always be used for the slide animation for larger images than the threshold, even when flipping between 2 adjacent images back and forth. Before this change, the full image could have been cached in the renderer by chance and might have been working smoother on some platforms. BUG=656430 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2629133002 Cr-Commit-Position: refs/heads/master@{#443880} Committed: https://chromium.googlesource.com/chromium/src/+/9eb76629f83b6d9285876f88ce64... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/9eb76629f83b6d9285876f88ce64... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
