New issue
Advanced search Search tips

Issue 904645 link

Starred by 2 users
Status: Untriaged
Owner: ----
Cc:
a...@chromium.org
rsesek@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

gfx::Image has too many conversion functions

Project Member Reported by a...@chromium.org, Nov 13 
gfx::Image has
ToNSImage()
AsNSImage()
CopyNSImage()

and corresponding versions for other platform images. Their differences are subtle.

ToXXXImage() has undefined behavior if the image is null.

AsXXXImage() returns null if the image is null.

CopyXXXImage() has undefined behavior if the image is null *and* returns an owning raw pointer to the image.

Having these three different versions is confusing. We should keep only one, AsXXXImage, that has no undefined behavior.

Comment 1 by rsesek@chromium.org, Nov 13 

ToX functions CHECK if the image is null
AsX returns null if the image is null
CopyX is ToX that returns a strong ref

Removing Copy sounds like a good first step.

Comment 2 by rsesek@chromium.org, Nov 13

Cc: rsesek@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8

commit c58186cc608c79f65998f4ac7e173a9c5ddf7dd8
Author: Avi Drissman <avi@chromium.org>
Date: Thu Nov 15 23:09:28 2018

Make gfx::Image's NSImage constructor retain the incoming image.

The Mac convention for ObjC objects is that if a callee needs to
have a reference, it should take the reference itself. The
current behavior of gfx::Image's NSImage constructor is surprising
and relying on explaining it in the comment is inadequate.

Note that gfx::Image's UIImage constructor had a similar change
made to it in 2a1f622ddc372a0bd4206fb01c45f4ebbe37258b.

BUG=904645

Change-Id: I4dab00c3c7f3ff2f73e17520562d728b3a2e52aa
Reviewed-on: https://chromium-review.googlesource.com/c/1335957
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608566}
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/chrome/browser/icon_loader_mac.mm
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/ui/base/resource/resource_bundle_mac.mm
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/ui/gfx/image/image.cc
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/ui/gfx/image/image.h
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/ui/gfx/image/image_mac_unittest.mm
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/ui/gfx/image/image_unittest_util.cc
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/ui/message_center/cocoa/notification_controller_unittest.mm
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/ui/message_center/cocoa/popup_collection_unittest.mm
[modify] https://crrev.com/c58186cc608c79f65998f4ac7e173a9c5ddf7dd8/ui/snapshot/snapshot_mac.mm
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f0bdf1c77e11b3045804543db8600a4a7a884564

commit f0bdf1c77e11b3045804543db8600a4a7a884564
Author: Avi Drissman <avi@chromium.org>
Date: Fri Nov 16 00:39:48 2018

Remove gfx::Image's Copy* methods.

They return ownership via raw pointer, and they're unneeded.

Also remove AsUIImage as it has no callers.

BUG=904645

Change-Id: I18497ae1e32ad36ec78cdcf3c5df07d0faf8af39
Reviewed-on: https://chromium-review.googlesource.com/c/1333876
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608601}
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/chrome/browser/background/background_application_list_model.cc
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/chrome/browser/ui/bookmarks/bookmark_utils.cc
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/chrome/browser/ui/cocoa/history_menu_bridge.mm
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/chrome/browser/ui/cocoa/share_menu_controller.mm
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/ui/gfx/image/image.cc
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/ui/gfx/image/image.h
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/ui/gfx/image/image_family.cc
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/ui/gfx/image/image_unittest.cc
[modify] https://crrev.com/f0bdf1c77e11b3045804543db8600a4a7a884564/ui/message_center/public/cpp/notification.cc
►

Sign in to add a comment