Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2943363003:
Fixing transparent pixels appearing black when rendered for the context menu. (Closed)
|
Created:
3 years, 6 months ago by injae Modified:
3 years, 5 months ago Reviewers:
Daniel Park, Avi (use Gerrit), jochen (gone - plz use gerrit), msarett1, Yaron, Tom Sepez, gone CC:
Aaron Boodman, abarth-chromium, ajwong+watch_chromium.org, chromium-reviews, creis+watch_chromium.org, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing transparent pixels appearing black when rendered for the context menu.
JPEG Codec does not support transparent pixels, so PNG Codec was added.
Added a new parameter to chrome_render_frame_observer#RequestThumbnailForContextNode which controls the codec used to encode the image.
Kept current implementations of RequestThumbnailForContextNode using JPEG Codec.
Currently, the only use for PNG Codec is for the context menu
BUG=730327
Review-Url: https://codereview.chromium.org/2943363003
Cr-Commit-Position: refs/heads/master@{#483219}
Committed: https://chromium.googlesource.com/chromium/src/+/4b282d1865ab5c2273f7cb03be89eb0e55a58768
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fixed spacing for mojom file #
Total comments: 2
Patch Set 3 : comment length #Patch Set 4 : fixed long comments #Patch Set 5 : git rebase #Patch Set 6 : rebase #
Total comments: 2
Patch Set 7 : creating and implementing ImageFormat enum #
Messages
Total messages: 62 (32 generated)
The CQ bit was checked by danielpark@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...
danielpark@chromium.org changed reviewers: + dfalcantara@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm not the right person to be reviewing this because it's native code. I'd suggest trying msarett@; he seems to have edited these lines on June here: https://codereview.chromium.org/2895953003 If anything, he might be able to redirect you. https://codereview.chromium.org/2943363003/diff/1/chrome/common/thumbnail_cap... File chrome/common/thumbnail_capturer.mojom (right): https://codereview.chromium.org/2943363003/diff/1/chrome/common/thumbnail_cap... chrome/common/thumbnail_capturer.mojom:10: // Requests an encoded thumbnail of the image selected by the most recently opened context menu. The encoding format is specified as a parameter. If no image is selected or there's an error capturing a thumbnail, |thumbnail_data| will be empty. If the image area is larger than |thumbnail_min_area_pixels| it will be downscaled to fit within |thumbnail_max_size_pixels|. Not sure what's going on here.
danielpark@chromium.org changed reviewers: + msarett@chromium.org - dfalcantara@chromium.org
Description was changed from ========== Fixing transparent pixels appearing black when rendered for the context menu. Added a new parameter to chrome_render_frame_observer#RequestThumbnailForContextNode which controls the codec used to encode the image. Kept current implementations of RequestThumbnailForContextNode using JPEG Codec. Currently, the only use for PNG Codec is for the context menu BUG=730327 ========== to ========== Fixing transparent pixels appearing black when rendered for the context menu. JPEG Codec does not support transparent pixels, so PNG Codec was added. Added a new parameter to chrome_render_frame_observer#RequestThumbnailForContextNode which controls the codec used to encode the image. Kept current implementations of RequestThumbnailForContextNode using JPEG Codec. Currently, the only use for PNG Codec is for the context menu BUG=730327 ==========
lgtm on the new call into gfx::PngCodec. Without understanding much about the use case, I'm wondering if use_png_codec is the right name for the flag. If all we care about is encoding the alpha values, maybe has_alpha? The jpeg vs png tradeoff encompasses more than alpha. Ex: jpeg - lossy, maybe faster, maybe smaller, opaque png - lossless, with alpha or opaque https://codereview.chromium.org/2943363003/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/2943363003/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_frame_observer.cc:76: static const bool kDiscardTransparencyForContextMenu = false; I'm glad this is false. "true" is fairly awkward because the png encoder must (1) unpremultiply by alpha then (2) discard the alpha channel. I'd like to remove that capability if possible.
On 2017/06/21 04:18:15, msarett1 wrote: > lgtm on the new call into gfx::PngCodec. > > Without understanding much about the use case, I'm wondering if use_png_codec is > the right name for the flag. If all we care about is encoding the alpha values, > maybe has_alpha? The jpeg vs png tradeoff encompasses more than alpha. Ex: > jpeg - lossy, maybe faster, maybe smaller, opaque > png - lossless, with alpha or opaque > > https://codereview.chromium.org/2943363003/diff/1/chrome/renderer/chrome_rend... > File chrome/renderer/chrome_render_frame_observer.cc (right): > I'm not sure I get what you're saying; you suggest renaming the variable to has_alpha, but you also mention that the difference between png and jpeg includes more than the alpha. I believe use_png_codec makes sense here simply because if it's set to true, we use the png codec. > https://codereview.chromium.org/2943363003/diff/1/chrome/renderer/chrome_rend... > chrome/renderer/chrome_render_frame_observer.cc:76: static const bool > kDiscardTransparencyForContextMenu = false; > I'm glad this is false. > > "true" is fairly awkward because the png encoder must (1) unpremultiply by alpha > then (2) discard the alpha channel. I'd like to remove that capability if > possible.
https://codereview.chromium.org/2943363003/diff/1/chrome/common/thumbnail_cap... File chrome/common/thumbnail_capturer.mojom (right): https://codereview.chromium.org/2943363003/diff/1/chrome/common/thumbnail_cap... chrome/common/thumbnail_capturer.mojom:10: // Requests an encoded thumbnail of the image selected by the most recently opened context menu. The encoding format is specified as a parameter. If no image is selected or there's an error capturing a thumbnail, |thumbnail_data| will be empty. If the image area is larger than |thumbnail_min_area_pixels| it will be downscaled to fit within |thumbnail_max_size_pixels|. On 2017/06/20 18:30:36, dfalcantara wrote: > Not sure what's going on here. i updated the documentation to include the use of PNG Codecs. https://codereview.chromium.org/2943363003/diff/1/chrome/renderer/chrome_rend... File chrome/renderer/chrome_render_frame_observer.cc (right): https://codereview.chromium.org/2943363003/diff/1/chrome/renderer/chrome_rend... chrome/renderer/chrome_render_frame_observer.cc:76: static const bool kDiscardTransparencyForContextMenu = false; On 2017/06/21 04:18:15, msarett1 wrote: > I'm glad this is false. > > "true" is fairly awkward because the png encoder must (1) unpremultiply by alpha > then (2) discard the alpha channel. I'd like to remove that capability if > possible. I'm a little unsure of how to proceed with regards to this comment. I made this constant a local variable in order to prevent confusion.
danielpark@chromium.org changed reviewers: + dfalcantara@chromium.org - msarett@chromium.org
https://codereview.chromium.org/2943363003/diff/1/chrome/common/thumbnail_cap... File chrome/common/thumbnail_capturer.mojom (right): https://codereview.chromium.org/2943363003/diff/1/chrome/common/thumbnail_cap... chrome/common/thumbnail_capturer.mojom:10: // Requests an encoded thumbnail of the image selected by the most recently opened context menu. The encoding format is specified as a parameter. If no image is selected or there's an error capturing a thumbnail, |thumbnail_data| will be empty. If the image area is larger than |thumbnail_min_area_pixels| it will be downscaled to fit within |thumbnail_max_size_pixels|. On 2017/06/22 01:11:35, Daniel Park wrote: > On 2017/06/20 18:30:36, dfalcantara wrote: > > Not sure what's going on here. > > i updated the documentation to include the use of PNG Codecs. woops. you weren't referring to that. apparently 'git cl format' doesn't work on mojom files. it's fixed now.
msarett@chromium.org changed reviewers: + msarett@chromium.org
Yes, looks good to me as is. Just wanted to provide some extra context.
OWNERs lgtm
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@chromium.org Link to the patchset: https://codereview.chromium.org/2943363003/#ps20001 (title: "Fixed spacing for mojom file")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
danielpark@chromium.org changed reviewers: + avi@chromium.org, tsepez@chromium.org, yfriedman@chromium.org - dfalcantara@chromium.org, msarett@chromium.org
avi: please review render_view_context_menu_browsertest.cc and core_tab_helper.cc. yfriedman: please review context_menu_helper.cc tsepez: please review thumbnail_capturer.mojom, chrome_render_frame_observer.h, and chrome_render_frame_observer.cc thank you!
lgtm https://codereview.chromium.org/2943363003/diff/20001/chrome/common/thumbnail... File chrome/common/thumbnail_capturer.mojom (right): https://codereview.chromium.org/2943363003/diff/20001/chrome/common/thumbnail... chrome/common/thumbnail_capturer.mojom:10: // Requests an encoded thumbnail of the image selected by the most recently opened context menu. nit: 80 cols.
On 2017/06/23 20:21:42, Daniel Park wrote: > avi: please review render_view_context_menu_browsertest.cc and > core_tab_helper.cc. LGTM; please wrap the comment to 80 columns as Tom notes.
chrome/browser/ui/android/context_menu_helper.cc lgtm
https://codereview.chromium.org/2943363003/diff/20001/chrome/common/thumbnail... File chrome/common/thumbnail_capturer.mojom (right): https://codereview.chromium.org/2943363003/diff/20001/chrome/common/thumbnail... chrome/common/thumbnail_capturer.mojom:10: // Requests an encoded thumbnail of the image selected by the most recently opened context menu. On 2017/06/23 20:25:27, Tom Sepez wrote: > nit: 80 cols. Done.
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, msarett@chromium.org, yfriedman@chromium.org, tsepez@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2943363003/#ps40001 (title: "comment length")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, msarett@chromium.org, yfriedman@chromium.org, tsepez@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2943363003/#ps60001 (title: "fixed long comments")
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
Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, msarett@chromium.org, yfriedman@chromium.org, tsepez@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2943363003/#ps80001 (title: "git rebase")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by danielpark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, msarett@chromium.org, yfriedman@chromium.org, tsepez@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2943363003/#ps100001 (title: "rebase")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/06/24 00:45:00, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Your presubmit is failing because you're missing a reviewer for chrome/renderer/chrome_render_frame_observer.*. You asked tsepez@ to review them but he's not an owner of them and can't grant you an LG. You need to find an owner.
danielpark@chromium.org changed reviewers: + jochen@chromium.org - avi@chromium.org, tsepez@chromium.org, yfriedman@chromium.org
jochen: please review chrome/renderer/chrome_render_frame_observer.*. thank you!
danielpark@chromium.org changed reviewers: + avi@chromium.org, dfalcantara@chromium.org, yfriedman@chromium.org
jochen@chromium.org changed reviewers: + msarett@chromium.org, tsepez@chromium.org
lgtm with comment https://codereview.chromium.org/2943363003/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_render_frame_observer.h (right): https://codereview.chromium.org/2943363003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_render_frame_observer.h:58: bool use_png_codec, can you make this an enum class with PNG and JPEG as values?
The CQ bit was checked by injae@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, jochen@chromium.org, msarett@chromium.org, yfriedman@chromium.org, tsepez@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2943363003/#ps120001 (title: "creating and implementing ImageFormat enum")
https://codereview.chromium.org/2943363003/diff/100001/chrome/renderer/chrome... File chrome/renderer/chrome_render_frame_observer.h (right): https://codereview.chromium.org/2943363003/diff/100001/chrome/renderer/chrome... chrome/renderer/chrome_render_frame_observer.h:58: bool use_png_codec, On 2017/06/28 18:15:17, jochen wrote: > can you make this an enum class with PNG and JPEG as values? Done.
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": 120001, "attempt_start_ts": 1498690180432040,
"parent_rev": "214d8372995dfe8ff136ee05209945ff4f09ed79", "commit_rev":
"4b282d1865ab5c2273f7cb03be89eb0e55a58768"}
Message was sent while issue was closed.
Description was changed from ========== Fixing transparent pixels appearing black when rendered for the context menu. JPEG Codec does not support transparent pixels, so PNG Codec was added. Added a new parameter to chrome_render_frame_observer#RequestThumbnailForContextNode which controls the codec used to encode the image. Kept current implementations of RequestThumbnailForContextNode using JPEG Codec. Currently, the only use for PNG Codec is for the context menu BUG=730327 ========== to ========== Fixing transparent pixels appearing black when rendered for the context menu. JPEG Codec does not support transparent pixels, so PNG Codec was added. Added a new parameter to chrome_render_frame_observer#RequestThumbnailForContextNode which controls the codec used to encode the image. Kept current implementations of RequestThumbnailForContextNode using JPEG Codec. Currently, the only use for PNG Codec is for the context menu BUG=730327 Review-Url: https://codereview.chromium.org/2943363003 Cr-Commit-Position: refs/heads/master@{#483219} Committed: https://chromium.googlesource.com/chromium/src/+/4b282d1865ab5c2273f7cb03be89... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/4b282d1865ab5c2273f7cb03be89... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
