Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2796153002:
ui: Replace a use of ExtractImageRep with a canvas SaveLayer (Closed)
Descriptionui: Replace a use of ExtractImageRep with a canvas SaveLayer
This changes the code for drawing the tab changed indicator to use
save layer in order to achieve its effect instead of making a CPU
bitmap and drawing from that into the recording canvas. This defers
the work until raster time, so it can be executed more efficiently
(and on the GPU), and removes a use of ExtractImageRep so that it
can go away soon.
BUG=671433
Review-Url: https://codereview.chromium.org/2796153002
Cr-Commit-Position: refs/heads/master@{#462102}
Committed: https://chromium.googlesource.com/chromium/src/+/073e4d7067e7af90e5faa0ef4adb19f3e8e4ded4
Patch Set 1 #
Total comments: 2
Patch Set 2 : tabindicator: constants-back #Messages
Total messages: 21 (13 generated)
The CQ bit was checked by danakj@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...
Description was changed from ========== ui: Replace a use of ExtractImageRep with a canvas SaveLayer This changes the code for drawing the tab changed indicator to use save layer in order to achieve its effect instead of making a CPU bitmap and drawing from that into the recording canvas. This defers the work until raster time, so it can be executed more efficiently (and on the GPU), and removes a use of ExtractImageRep so that it can go away soon. R=pkasting@chromium.org BUG=671433 ========== to ========== ui: Replace a use of ExtractImageRep with a canvas SaveLayer This changes the code for drawing the tab changed indicator to use save layer in order to achieve its effect instead of making a CPU bitmap and drawing from that into the recording canvas. This defers the work until raster time, so it can be executed more efficiently (and on the GPU), and removes a use of ExtractImageRep so that it can go away soon. BUG=671433 ==========
danakj@chromium.org changed reviewers: + sky@chromium.org - pkasting@chromium.org
Before: http://imgur.com/eosJ94P After: http://imgur.com/Dn0JSuC No indicator just for reference: http://imgur.com/wkPpiCn
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Assuming this produces the right results, and you tried RTL LGTM https://codereview.chromium.org/2796153002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2796153002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1278: const float kIndicatorCropRadius = 4.5f; Is there a reason to move these from where they were? I prefer keeping the variables close to where they are used.
On 2017/04/04 22:02:41, sky wrote: > Assuming this produces the right results, and you tried RTL LGTM > > https://codereview.chromium.org/2796153002/diff/1/chrome/browser/ui/views/tab... > File chrome/browser/ui/views/tabs/tab.cc (right): > > https://codereview.chromium.org/2796153002/diff/1/chrome/browser/ui/views/tab... > chrome/browser/ui/views/tabs/tab.cc:1278: const float kIndicatorCropRadius = > 4.5f; > Is there a reason to move these from where they were? I prefer keeping the > variables close to where they are used. By right results, I mean looks the same.
https://codereview.chromium.org/2796153002/diff/1/chrome/browser/ui/views/tab... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2796153002/diff/1/chrome/browser/ui/views/tab... chrome/browser/ui/views/tabs/tab.cc:1278: const float kIndicatorCropRadius = 4.5f; On 2017/04/04 22:02:41, sky wrote: > Is there a reason to move these from where they were? I prefer keeping the > variables close to where they are used. Oh, putting them together shows their relationship clearly, I can put them back then. I did the math for RTL but didn't run it, I'll give it a try.
The CQ bit was checked by danakj@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...
On 2017/04/04 22:10:44, danakj wrote: > https://codereview.chromium.org/2796153002/diff/1/chrome/browser/ui/views/tab... > File chrome/browser/ui/views/tabs/tab.cc (right): > > https://codereview.chromium.org/2796153002/diff/1/chrome/browser/ui/views/tab... > chrome/browser/ui/views/tabs/tab.cc:1278: const float kIndicatorCropRadius = > 4.5f; > On 2017/04/04 22:02:41, sky wrote: > > Is there a reason to move these from where they were? I prefer keeping the > > variables close to where they are used. > > Oh, putting them together shows their relationship clearly, I can put them back > then. Separated and moved the constants back down. > I did the math for RTL but didn't run it, I'll give it a try. Confirmed RTL looks the same before and after this CL with --force-ui-direction=rtl.
The CQ bit was unchecked by danakj@chromium.org
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2796153002/#ps20001 (title: "tabindicator: constants-back")
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": 20001, "attempt_start_ts": 1491407333126430,
"parent_rev": "6d0446e51df56fa7b2b3a0606fa549217ccf75b6", "commit_rev":
"073e4d7067e7af90e5faa0ef4adb19f3e8e4ded4"}
Message was sent while issue was closed.
Description was changed from ========== ui: Replace a use of ExtractImageRep with a canvas SaveLayer This changes the code for drawing the tab changed indicator to use save layer in order to achieve its effect instead of making a CPU bitmap and drawing from that into the recording canvas. This defers the work until raster time, so it can be executed more efficiently (and on the GPU), and removes a use of ExtractImageRep so that it can go away soon. BUG=671433 ========== to ========== ui: Replace a use of ExtractImageRep with a canvas SaveLayer This changes the code for drawing the tab changed indicator to use save layer in order to achieve its effect instead of making a CPU bitmap and drawing from that into the recording canvas. This defers the work until raster time, so it can be executed more efficiently (and on the GPU), and removes a use of ExtractImageRep so that it can go away soon. BUG=671433 Review-Url: https://codereview.chromium.org/2796153002 Cr-Commit-Position: refs/heads/master@{#462102} Committed: https://chromium.googlesource.com/chromium/src/+/073e4d7067e7af90e5faa0ef4adb... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/073e4d7067e7af90e5faa0ef4adb... |
