Adds a new constructor for gfx::Image on iOS.

Adds a new constructor for gfx::Image on iOS.

Chrome iOS is moving to Objective-C Automatic Reference Counting. Under
it, arguments are supposed to be autoreleased. This CL adds a new
constructor that can take both a retained and an autoreleased object.
This is a part of a multiple CL setup that will change the Image(UIImage*) constructor from consuming +1 refcount to retaining the argument.

TEST=none
BUG=645125
Committed: https://crrev.com/90031d9584452e5018e68a3ad2ff47634aab70b6
Cr-Commit-Position: refs/heads/master@{#437513}

[stkhapugin, 2016-11-22 14:07:17]

stkhapugin@chromium.org changed reviewers:
+ sdefresne@chromium.org, thakis@chromium.org

[stkhapugin, 2016-11-22 14:07:18]

PTAL

[sdefresne, 2016-11-22 14:23:16]

I would change the description to not list explicitly each CL but instead to say
what is the goal, i.e. moving from code that sinks the reference of the object
passed to the constructor to code that explicitly retain the object in the
class.

I would also add major warning on top of the constructors to highlight that the
behaviour will be different on iOS and macOS for this class (macOS will sink the
reference while iOS will retain the object).

https://codereview.chromium.org/2504683002/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

https://codereview.chromium.org/2504683002/diff/1/ui/gfx/image/image.h#newcode77
ui/gfx/image/image.h:77: explicit Image(UIImage* image, bool retain_argument);
"explicit" is only for constructor that can be invoked with exactly one
argument. This is not the case here, so remove "explicit".

I don't really like using a bool parameter as it is always hard to understand
the call-sites. Since base::scoped_policy::OwnershipPolicy exists, can you use
that instead of a bool?

[stkhapugin, 2016-12-06 10:41:35]

Description was changed from

==========
Adds a new constructor for gfx::Image on iOS.

Chrome iOS is moving to Objective-C Automatic Reference Counting. Under
it, arguments are supposed to be autoreleased. This CL adds a new
constructor that can take both a retained and an autoreleased object.

This is a part of several CLs:
1. This CL. Adds a new constructor.
2. A downstream CL that changes all usage of old constructor to the new
one.
3. An upstream CL changing the default behavior of the old constructor
to not assume the argument to be retained.
4. A downstream CL that migrates all usage of new constructor back to
the old one.
5. An upstream CL that removes the new constructor.

TEST=none
BUG=645125
==========

to

==========
Adds a new constructor for gfx::Image on iOS.

Chrome iOS is moving to Objective-C Automatic Reference Counting. Under
it, arguments are supposed to be autoreleased. This CL adds a new
constructor that can take both a retained and an autoreleased object.
This is a part of a multiple CL setup that will change the Image(UIImage*)
constructor from consuming +1 refcount to retaining the argument.  

TEST=none
BUG=645125
==========

[stkhapugin, 2016-12-06 10:41:57]

PTAL

> I would change the description to not list explicitly each CL but instead to
say
> what is the goal
Done

> I would also add major warning on top of the constructors to highlight that
the
> behaviour will be different on iOS and macOS for this class (macOS will sink
the
> reference while iOS will retain the object).
It makes sense to do this in a future CL, since in this one the constructors
still 
behave the same.

https://codereview.chromium.org/2504683002/diff/1/ui/gfx/image/image.h
File ui/gfx/image/image.h (right):

https://codereview.chromium.org/2504683002/diff/1/ui/gfx/image/image.h#newcode77
ui/gfx/image/image.h:77: explicit Image(UIImage* image, bool retain_argument);
On 2016/11/22 14:23:16, sdefresne wrote:
> "explicit" is only for constructor that can be invoked with exactly one
> argument. This is not the case here, so remove "explicit".
> 
> I don't really like using a bool parameter as it is always hard to understand
> the call-sites. Since base::scoped_policy::OwnershipPolicy exists, can you use
> that instead of a bool?

Done

[sdefresne, 2016-12-06 10:54:11]

https://codereview.chromium.org/2504683002/diff/20001/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

https://codereview.chromium.org/2504683002/diff/20001/ui/gfx/image/image.cc#n...
ui/gfx/image/image.cc:423: Image::Image(UIImage* image) : Image(image, false) {}
Image::Image(UIImage* image) : Image(image, base::scoped_policy::ASSUME) {}

[stkhapugin, 2016-12-06 12:10:56]

PTAL

https://codereview.chromium.org/2504683002/diff/20001/ui/gfx/image/image.cc
File ui/gfx/image/image.cc (right):

https://codereview.chromium.org/2504683002/diff/20001/ui/gfx/image/image.cc#n...
ui/gfx/image/image.cc:423: Image::Image(UIImage* image) : Image(image, false) {}
On 2016/12/06 10:54:11, sdefresne wrote:
> Image::Image(UIImage* image) : Image(image, base::scoped_policy::ASSUME) {}

Done.

[sdefresne, 2016-12-06 12:15:33]

lgtm

[Nico, 2016-12-06 14:24:24]

lgtm

(I find the order of CLs here fairly confusing. I think I got it now, but it
took me a bit to figure it out)

[stkhapugin, 2016-12-07 17:30:13]

The CQ bit was checked by stkhapugin@chromium.org

[commit-bot: I haz the power, 2016-12-07 17:30:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 17:33:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 17:33:10]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[stkhapugin, 2016-12-08 12:20:23]

The CQ bit was checked by stkhapugin@chromium.org

[stkhapugin, 2016-12-08 12:20:24]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2504683002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-08 12:20:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 13:36:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 13:36:11]

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_...)

[stkhapugin, 2016-12-09 10:43:53]

The CQ bit was checked by stkhapugin@chromium.org

[commit-bot: I haz the power, 2016-12-09 10:44:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 10:48:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481280233228380,
"parent_rev": "a97a7e30746655316b02d39b6a855faedf9f9591", "commit_rev":
"4b7c308b1b7a4e880f9df00f442025d24c01131b"}

[commit-bot: I haz the power, 2016-12-09 10:48:40]

Description was changed from

==========
Adds a new constructor for gfx::Image on iOS.

Chrome iOS is moving to Objective-C Automatic Reference Counting. Under
it, arguments are supposed to be autoreleased. This CL adds a new
constructor that can take both a retained and an autoreleased object.
This is a part of a multiple CL setup that will change the Image(UIImage*)
constructor from consuming +1 refcount to retaining the argument.  

TEST=none
BUG=645125
==========

to

==========
Adds a new constructor for gfx::Image on iOS.

Chrome iOS is moving to Objective-C Automatic Reference Counting. Under
it, arguments are supposed to be autoreleased. This CL adds a new
constructor that can take both a retained and an autoreleased object.
This is a part of a multiple CL setup that will change the Image(UIImage*)
constructor from consuming +1 refcount to retaining the argument.  

TEST=none
BUG=645125

Review-Url: https://codereview.chromium.org/2504683002
==========

[commit-bot: I haz the power, 2016-12-09 10:48:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-09 10:51:14]

Description was changed from

==========
Adds a new constructor for gfx::Image on iOS.

Chrome iOS is moving to Objective-C Automatic Reference Counting. Under
it, arguments are supposed to be autoreleased. This CL adds a new
constructor that can take both a retained and an autoreleased object.
This is a part of a multiple CL setup that will change the Image(UIImage*)
constructor from consuming +1 refcount to retaining the argument.  

TEST=none
BUG=645125

Review-Url: https://codereview.chromium.org/2504683002
==========

to

==========
Adds a new constructor for gfx::Image on iOS.

Chrome iOS is moving to Objective-C Automatic Reference Counting. Under
it, arguments are supposed to be autoreleased. This CL adds a new
constructor that can take both a retained and an autoreleased object.
This is a part of a multiple CL setup that will change the Image(UIImage*)
constructor from consuming +1 refcount to retaining the argument.

TEST=none
BUG=645125

Committed: https://crrev.com/90031d9584452e5018e68a3ad2ff47634aab70b6
Cr-Commit-Position: refs/heads/master@{#437513}
==========

[commit-bot: I haz the power, 2016-12-09 10:51:15]

Patchset 4 (id:??) landed as
https://crrev.com/90031d9584452e5018e68a3ad2ff47634aab70b6
Cr-Commit-Position: refs/heads/master@{#437513}