Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2308663002:
Migrate ArcBitmap to use typemapping (Closed)
DescriptionMigrate ArcBitmap to use typemapping
BUG=643050
TEST=manual test: notification small icons show in the tray correctly
TEST=Autotest cheets_NotificationTest passes
Committed: https://crrev.com/377ca77ed026cd7e281fe8d915e109411ac065da
Cr-Commit-Position: refs/heads/master@{#421712}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed comments #
Total comments: 15
Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : . #
Total comments: 1
Patch Set 5 : Addressed comment #Messages
Total messages: 91 (76 generated)
The CQ bit was checked by yoshiki@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 ========== . BUG= ========== to ========== Migrate ArcBitmap to use typemapping BUG= ==========
Description was changed from ========== Migrate ArcBitmap to use typemapping BUG= ========== to ========== Migrate ArcBitmap to use typemapping BUG=643050 ==========
yoshiki@chromium.org changed reviewers: + dcheng@chromium.org, xiyuan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 yoshiki@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_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 yoshiki@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
yoshiki@chromium.org changed reviewers: - dcheng@chromium.org, xiyuan@chromium.org
The CQ bit was checked by yoshiki@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: linux_chromium_chromeos_ozone_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 yoshiki@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by yoshiki@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...
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_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 yoshiki@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.
Patchset #1 (id:100001) has been deleted
yoshiki@chromium.org changed reviewers: + dcheng@chromium.org, xiyuan@chromium.org
dcheng@, xiyuan@, PTAL. Thanks.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by: Can you add a TEST= field indicating that you tested this works with ARC end-to-end? I'm worried that the serialization format might have changed.
https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:57: out = new SkBitmap(); This looks wrong. It would leak and created SkBitmap is not passed back. Think it should be *out = SkBitmap(); or do nothing and leave |out| untouched. https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:67: out = new SkBitmap(); ditto https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:74: out = new SkBitmap(); ditto https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:139: LOG(ERROR) << data->type; nit: remove ? https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:163: LOG(ERROR) << "0"; nit: remove this and other debugging logs ? https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.cc:91: LOG(ERROR) << data->type; nit: remove ?
https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:17: return mojo::Array<uint8_t>(); These methods are pretty complex. We should out-of-line things that aren't just simple accessors. Also, we don't need to do all these defensive checks on the sending side: they can just be DCHECKs. Hopfeully you know that you're not sending bad data. https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:38: static bool Read(arc::mojom::ArcBitmapDataView data, SkBitmap* out) { SkBitmap is an out param. In effect, Read() is called like this: // Generated mojo code. SkBitmap native_bitmap; ReadStruct(bitmap, &native_bitmap); So we should just be mutating |out| directly.
The CQ bit was checked by yoshiki@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 yoshiki@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.
Patchset #2 (id:140001) has been deleted
The CQ bit was checked by yoshiki@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 yoshiki@chromium.org to run a CQ dry run
Patchset #2 (id:160001) has been deleted
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.
Patchset #2 (id:180001) has been deleted
Description was changed from ========== Migrate ArcBitmap to use typemapping BUG=643050 ========== to ========== Migrate ArcBitmap to use typemapping BUG=643050 TEST=manual test: notification small icons show in the tray correctly TEST=Autotest cheets_NotificationTest passes ==========
Hi, I've updated the patch and added TESTs fields. PTAL again. Thanks. https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:17: return mojo::Array<uint8_t>(); On 2016/09/07 22:47:17, dcheng wrote: > These methods are pretty complex. We should out-of-line things that aren't just > simple accessors. > > Also, we don't need to do all these defensive checks on the sending side: they > can just be DCHECKs. Hopfeully you know that you're not sending bad data. Made this simpler. Is it ok? https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:38: static bool Read(arc::mojom::ArcBitmapDataView data, SkBitmap* out) { On 2016/09/07 22:47:17, dcheng wrote: > SkBitmap is an out param. In effect, Read() is called like this: > > // Generated mojo code. > SkBitmap native_bitmap; > ReadStruct(bitmap, &native_bitmap); > > So we should just be mutating |out| directly. I was misunderstood. Fixed. https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:57: out = new SkBitmap(); On 2016/09/07 22:09:00, xiyuan wrote: > This looks wrong. It would leak and created SkBitmap is not passed back. > > Think it should be > *out = SkBitmap(); > or do nothing and leave |out| untouched. Good catch. Let me use out->reset() here. https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:67: out = new SkBitmap(); On 2016/09/07 22:09:00, xiyuan wrote: > ditto Done. https://codereview.chromium.org/2308663002/diff/120001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:74: out = new SkBitmap(); On 2016/09/07 22:09:00, xiyuan wrote: > ditto Done. https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_item.cc (right): https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:139: LOG(ERROR) << data->type; On 2016/09/07 22:09:00, xiyuan wrote: > nit: remove ? Oh sorry. Done. https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... ui/arc/notification/arc_notification_item.cc:163: LOG(ERROR) << "0"; On 2016/09/07 22:09:00, xiyuan wrote: > nit: remove this and other debugging logs ? Done. https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... File ui/arc/notification/arc_notification_manager.cc (right): https://codereview.chromium.org/2308663002/diff/120001/ui/arc/notification/ar... ui/arc/notification/arc_notification_manager.cc:91: LOG(ERROR) << data->type; On 2016/09/07 22:09:00, xiyuan wrote: > nit: remove ? Done.
lgtm
https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:22: r.readPixels(info, &pixel_data[0], r.rowBytes(), 0, 0); Let's return a mojo::CArray here to avoid copying the data. We can just use getPixels() instead of readPixels() here as well. It's important to make this cheap, because it's called multiple times during serialization. I would probably out-of-line it as well, since it's not just a simple getter like width() and height(). https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:35: DCHECK_NE(out, nullptr); This should be out-of-lined, and the DCHECK is unnecessary (it means the mojo bindings are really badly broken) https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:38: if (!data.ReadPixelData(&pixel_data)) { Let's use the data view to avoid copying the pixels twice. We can use GetPixelDataView() to get a read-only view of the pixels and memcpy them directly into the allocated bitmap pixels below. https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:40: out->reset(); No need to reset here and elsewhere. https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:44: if (data.width() == 0 || data.height() == 0) { I don't think there's any particular need to make this special case: I would just remove it (it's slightly wasteful, but I imagine sending 0x0 bitmaps should be quite uncommon?) https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:50: SkImageInfo info = SkImageInfo::Make( I would recommend structuring this like https://cs.chromium.org/chromium/src/ui/gfx/ipc/skia/gfx_skia_param_traits.cc... We can just write directly into |out|, since |out| won't ever be used if we fail validation.
The CQ bit was checked by yoshiki@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.
https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:22: r.readPixels(info, &pixel_data[0], r.rowBytes(), 0, 0); On 2016/09/09 21:46:32, dcheng wrote: > Let's return a mojo::CArray here to avoid copying the data. We can just use > getPixels() instead of readPixels() here as well. It's important to make this > cheap, because it's called multiple times during serialization. I would probably > out-of-line it as well, since it's not just a simple getter like width() and > height(). Done. https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:35: DCHECK_NE(out, nullptr); On 2016/09/09 21:46:32, dcheng wrote: > This should be out-of-lined, and the DCHECK is unnecessary (it means the mojo > bindings are really badly broken) Done. https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:38: if (!data.ReadPixelData(&pixel_data)) { On 2016/09/09 21:46:33, dcheng wrote: > Let's use the data view to avoid copying the pixels twice. We can use > GetPixelDataView() to get a read-only view of the pixels and memcpy them > directly into the allocated bitmap pixels below. Done. https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:40: out->reset(); On 2016/09/09 21:46:33, dcheng wrote: > No need to reset here and elsewhere. Done. https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:44: if (data.width() == 0 || data.height() == 0) { On 2016/09/09 21:46:32, dcheng wrote: > I don't think there's any particular need to make this special case: I would > just remove it (it's slightly wasteful, but I imagine sending 0x0 bitmaps should > be quite uncommon?) Done. https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:50: SkImageInfo info = SkImageInfo::Make( On 2016/09/09 21:46:33, dcheng wrote: > I would recommend structuring this like > https://cs.chromium.org/chromium/src/ui/gfx/ipc/skia/gfx_skia_param_traits.cc... > > We can just write directly into |out|, since |out| won't ever be used if we fail > validation. That code can't be reused here, since we need to convert color type (kRGBA_8888_SkColorType to kN32_SkColorType) in copying.
https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:50: SkImageInfo info = SkImageInfo::Make( On 2016/09/12 17:35:43, yoshiki wrote: > On 2016/09/09 21:46:33, dcheng wrote: > > I would recommend structuring this like > > > https://cs.chromium.org/chromium/src/ui/gfx/ipc/skia/gfx_skia_param_traits.cc... > > > > We can just write directly into |out|, since |out| won't ever be used if we > fail > > validation. > > That code can't be reused here, since we need to convert color type > (kRGBA_8888_SkColorType to kN32_SkColorType) in copying. Can we make our input and output pixel formats match? https://codereview.chromium.org/2308663002/diff/220001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/220001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:33: static bool Read(arc::mojom::ArcBitmapDataView data, SkBitmap* out) { Please out-of-line and place the definition of this method in a .cc file. Anything that's not just calling a simple getter should probably be like that. pixel_data() should also probably be out-of-lined.
The CQ bit was checked by yoshiki@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
dcheng@, PTAL. Thanks. https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:50: SkImageInfo info = SkImageInfo::Make( On 2016/09/13 01:13:09, dcheng wrote: > On 2016/09/12 17:35:43, yoshiki wrote: > > On 2016/09/09 21:46:33, dcheng wrote: > > > I would recommend structuring this like > > > > > > https://cs.chromium.org/chromium/src/ui/gfx/ipc/skia/gfx_skia_param_traits.cc... > > > > > > We can just write directly into |out|, since |out| won't ever be used if we > > fail > > > validation. > > > > That code can't be reused here, since we need to convert color type > > (kRGBA_8888_SkColorType to kN32_SkColorType) in copying. > > Can we make our input and output pixel formats match? It's impossible. Android uses kRGBA_8888_SkColorType representative in android.graphics.Bitmap, and the standard format of Chrome's bitmap is kN32_SkColorType. https://codereview.chromium.org/2308663002/diff/220001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/220001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:33: static bool Read(arc::mojom::ArcBitmapDataView data, SkBitmap* out) { On 2016/09/13 01:13:09, dcheng wrote: > Please out-of-line and place the definition of this method in a .cc file. > Anything that's not just calling a simple getter should probably be like that. > pixel_data() should also probably be out-of-lined. Done.
LGTM with nits (and I don't understand the assertion about a standard bitmap format in Chromium) https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.h (right): https://codereview.chromium.org/2308663002/diff/200001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.h:50: SkImageInfo info = SkImageInfo::Make( On 2016/09/13 18:01:43, yoshiki wrote: > On 2016/09/13 01:13:09, dcheng wrote: > > On 2016/09/12 17:35:43, yoshiki wrote: > > > On 2016/09/09 21:46:33, dcheng wrote: > > > > I would recommend structuring this like > > > > > > > > > > https://cs.chromium.org/chromium/src/ui/gfx/ipc/skia/gfx_skia_param_traits.cc... > > > > > > > > We can just write directly into |out|, since |out| won't ever be used if > we > > > fail > > > > validation. > > > > > > That code can't be reused here, since we need to convert color type > > > (kRGBA_8888_SkColorType to kN32_SkColorType) in copying. > > > > Can we make our input and output pixel formats match? > > It's impossible. Android uses kRGBA_8888_SkColorType representative in > android.graphics.Bitmap, and the standard format of Chrome's bitmap is > kN32_SkColorType. What does that mean "standard format"? I don't think there's any standard format... and I would expect the right conversion to happen when you end up drawing the bitmap into a canvas. https://codereview.chromium.org/2308663002/diff/240001/components/arc/bitmap/... File components/arc/bitmap/bitmap_struct_traits.cc (right): https://codereview.chromium.org/2308663002/diff/240001/components/arc/bitmap/... components/arc/bitmap/bitmap_struct_traits.cc:33: if (!bitmap.copyTo(out, kN32_SkColorType)) { return bitmap.CopyTo(...);
The CQ bit was checked by yoshiki@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.
components/arc lgtm
The CQ bit was checked by yoshiki@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2308663002/#ps260001 (title: "Addressed comment")
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 ========== Migrate ArcBitmap to use typemapping BUG=643050 TEST=manual test: notification small icons show in the tray correctly TEST=Autotest cheets_NotificationTest passes ========== to ========== Migrate ArcBitmap to use typemapping BUG=643050 TEST=manual test: notification small icons show in the tray correctly TEST=Autotest cheets_NotificationTest passes ==========
Message was sent while issue was closed.
Committed patchset #5 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Migrate ArcBitmap to use typemapping BUG=643050 TEST=manual test: notification small icons show in the tray correctly TEST=Autotest cheets_NotificationTest passes ========== to ========== Migrate ArcBitmap to use typemapping BUG=643050 TEST=manual test: notification small icons show in the tray correctly TEST=Autotest cheets_NotificationTest passes Committed: https://crrev.com/377ca77ed026cd7e281fe8d915e109411ac065da Cr-Commit-Position: refs/heads/master@{#421712} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/377ca77ed026cd7e281fe8d915e109411ac065da Cr-Commit-Position: refs/heads/master@{#421712} |
