Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2767803003:
Replace Canvas::ExtractImageRep in render_view_context_menu.cc (Closed)
DescriptionReplace Canvas::ExtractImageRep in render_view_context_menu.cc
Instead of using ExtractImageRep to raster a bitmap that is passed
to an ImageSkiaSource for later scaling, we just tell that
ImageSkiaSource to do the circle mask also, avoiding the first
bitmap altogether.
R=pkasting@chromium.org
BUG=671433
Review-Url: https://codereview.chromium.org/2767803003
Cr-Commit-Position: refs/heads/master@{#458885}
Committed: https://chromium.googlesource.com/chromium/src/+/d0d5714663b6a19befac1ec77ebdb1b4585ae953
Patch Set 1 #Patch Set 2 : menu-bitmap: . #
Total comments: 2
Patch Set 3 : menu-bitmap: headers #Patch Set 4 : menu-bitmap: just-use-shape-circle #Messages
Total messages: 27 (19 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 ========== Replace Canvas::ExtractBitmap in render_view_context_menu.cc This replaces calls to Canvas::ExtractBitmap which copies from the backing bitmap of a gfx::Canvas with instead directly rastering into an SkBitmap. R=pkasting@chromium.org BUG=671433 ========== to ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc This replaces calls to Canvas::ExtractBitmap which copies from the backing bitmap of a gfx::Canvas with instead directly rastering into an SkBitmap. R=pkasting@chromium.org BUG=671433 ==========
Description was changed from ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc This replaces calls to Canvas::ExtractBitmap which copies from the backing bitmap of a gfx::Canvas with instead directly rastering into an SkBitmap. R=pkasting@chromium.org BUG=671433 ========== to ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc This replaces calls to Canvas::ExtractImageRep which copies from the backing bitmap of a gfx::Canvas with instead directly rastering into an SkBitmap. R=pkasting@chromium.org BUG=671433 ==========
Description was changed from ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc This replaces calls to Canvas::ExtractImageRep which copies from the backing bitmap of a gfx::Canvas with instead directly rastering into an SkBitmap. R=pkasting@chromium.org BUG=671433 ========== to ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc This replaces calls to Canvas::ExtractImageRep which copies from the backing bitmap of a gfx::Canvas with instead directly rastering into an SkBitmap. R=pkasting@chromium.org BUG=671433 ==========
https://codereview.chromium.org/2767803003/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2767803003/diff/20001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:465: icon.ToImageSkia()->GetRepresentation(raster_scale); Rather than used this fixed-scale representation, we should set the icon using a custom ImageSkiaSource that can draw the correct representation at the desired scale factor. I don't know if you're familiar with those. Basically, we need to implement a class here that overrides ImageSkiaSource::GetImageForScale() to do effectively what this code is currently doing, except that the scale factor will be passed in and not fixed at 1. This class can then be used to construct an ImageSkia that will pull the correct scaled representation as needed, and cache it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
https://codereview.chromium.org/2767803003/diff/20001/chrome/browser/renderer... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2767803003/diff/20001/chrome/browser/renderer... chrome/browser/renderer_context_menu/render_view_context_menu.cc:471: gfx::CalculateFaviconTargetSize(&width, &height); I had some trouble understanding if the width/height here and below are supposed to be the bitmap px or dip. I will try some more.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. This code was making a bitmap with the circle, then giving that to another ImageSkiaSource inside profiles:: that would scale it. Instead of having two ImageSkiaSources which I would have ended up with here, I discovered that we can tell profiles:: that we want the circle mask, and do the scaling and the masking at the same time. So I did that.
Description was changed from ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc This replaces calls to Canvas::ExtractImageRep which copies from the backing bitmap of a gfx::Canvas with instead directly rastering into an SkBitmap. R=pkasting@chromium.org BUG=671433 ========== to ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc Instead of using ExtractImageRep to raster a bitmap that is passed to an ImageSkiaSource for later scaling, we just tell that ImageSkiaSource to do the circle mask also, avoiding the first bitmap altogether. R=pkasting@chromium.org BUG=671433 ==========
danakj@chromium.org changed reviewers: + avi@chromium.org
Oh, I mistakenly thought you're an owner here, +avi for OWNERS. (Feel free to review appreciate your feedback still)
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...
lgtm
The CQ bit was unchecked by danakj@chromium.org
The CQ bit was checked by danakj@chromium.org
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": 1490217320412450,
"parent_rev": "2ec06ab81aae16ab444938342c2bf0a6fc712374", "commit_rev":
"d0d5714663b6a19befac1ec77ebdb1b4585ae953"}
Message was sent while issue was closed.
Description was changed from ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc Instead of using ExtractImageRep to raster a bitmap that is passed to an ImageSkiaSource for later scaling, we just tell that ImageSkiaSource to do the circle mask also, avoiding the first bitmap altogether. R=pkasting@chromium.org BUG=671433 ========== to ========== Replace Canvas::ExtractImageRep in render_view_context_menu.cc Instead of using ExtractImageRep to raster a bitmap that is passed to an ImageSkiaSource for later scaling, we just tell that ImageSkiaSource to do the circle mask also, avoiding the first bitmap altogether. R=pkasting@chromium.org BUG=671433 Review-Url: https://codereview.chromium.org/2767803003 Cr-Commit-Position: refs/heads/master@{#458885} Committed: https://chromium.googlesource.com/chromium/src/+/d0d5714663b6a19befac1ec77ebd... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d0d5714663b6a19befac1ec77ebd... |
