Remove ExtensionFunction::SetResult(T*) overload.

Remove ExtensionFunction::SetResult(T*) overload.

Code should use the std::unique_ptr<T> version instead to clearly
indicate ownership.

BUG=581865
Committed: https://crrev.com/85f24dacc21477945bb8fb1a769725fb7d492b5a
Cr-Commit-Position: refs/heads/master@{#395180}

[dcheng, 2016-05-19 03:40:24]

dcheng@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, thestig@chromium.org

[dcheng, 2016-05-19 03:40:26]

https://codereview.chromium.org/1991083002/diff/1/chrome/browser/extensions/w...
File chrome/browser/extensions/window_controller.h (left):

https://codereview.chromium.org/1991083002/diff/1/chrome/browser/extensions/w...
chrome/browser/extensions/window_controller.h:83: virtual base::DictionaryValue*
CreateWindowValue() const;
Devirtualized, because none of the overloads did anything interesting.

https://codereview.chromium.org/1991083002/diff/1/extensions/browser/extensio...
File extensions/browser/extension_function.cc (right):

https://codereview.chromium.org/1991083002/diff/1/extensions/browser/extensio...
extensions/browser/extension_function.cc:259: args_ = args->CreateDeepCopy();
A bit unrelated, but I changed it since I was in the vicinity.

[dcheng, 2016-05-19 04:33:20]

(I forgot to mention this in the first email)

+thestig for //chrome
+rdevlin.cronin for //extensions

[Lei Zhang, 2016-05-19 07:21:33]

chrome/ lgtm, though I should have deferred c/b/extensions to rdevlin. ;)

https://codereview.chromium.org/1991083002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/wallpaper_api.cc (right):

https://codereview.chromium.org/1991083002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/wallpaper_api.cc:239:
SetResult(base::WrapUnique(thumbnail_result));
#include ptr_util.h and <memory> ?

https://codereview.chromium.org/1991083002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/1991083002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.cc:361: .release());
BTW, switch to the unique_ptr version at some point?

[dcheng, 2016-05-19 07:34:26]

https://codereview.chromium.org/1991083002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/extensions/wallpaper_api.cc (right):

https://codereview.chromium.org/1991083002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/wallpaper_api.cc:239:
SetResult(base::WrapUnique(thumbnail_result));
On 2016/05/19 at 07:21:33, Lei Zhang wrote:
> #include ptr_util.h and <memory> ?

Done, though <memory> is in the 'associated' header, since that uses unique_ptr
without including it.

https://codereview.chromium.org/1991083002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/1991083002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.cc:361: .release());
On 2016/05/19 at 07:21:33, Lei Zhang wrote:
> BTW, switch to the unique_ptr version at some point?

Yes, I'm intentionally limiting the scope of my changes. But this will come in a
followup when ListValue::Append(T*) is removed.

[Devlin, 2016-05-20 17:56:55]

I only just saw that Lei made the same comment I did about switching to
Append(std::unique_ptr<>), so feel free to ignore all those.  Please do fix the
file_system_api.cc comment, though.

lgtm

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/content_settings/content_settings_api.cc
(right):

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/content_settings/content_settings_api.cc:319:
base::DictionaryValue* dict = new base::DictionaryValue();
optional: make this a unique ptr so we're one step closer to removing Append(T*)

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/downloads/downloads_api.cc:1197: new
base::FundamentalValue(static_cast<int>((*it)->GetId())));
ditto

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:555: new
base::FundamentalValue(static_cast<int>(SkColorGetR(color))));
ditto

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:546: bool success =
result->GetList("entries", &entries);
nit: this is silly, since it means we repeat this look up each time we call into
AddEntryToResult.  Since you're changing the signature anyway, can you just pass
in the base::ListValue* entries?

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/extension_tab_util.cc:361: .release());
ditto

[dcheng, 2016-05-20 18:11:57]

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:546: bool success =
result->GetList("entries", &entries);
On 2016/05/20 at 17:56:54, Devlin wrote:
> nit: this is silly, since it means we repeat this look up each time we call
into AddEntryToResult.  Since you're changing the signature anyway, can you just
pass in the base::ListValue* entries?

It's a map internally, so lookup is reasonably timed. And casting from Value to
ListValue is constant time.

The alternatives are:
- copy and paste these two lines into two different places
- have CreateResult() return a tuple with an owned DictionaryValue and an
unowned ListValue

Both seem worse than the current version.

[Devlin, 2016-05-20 20:56:18]

s lgtm

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/1991083002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:546: bool success =
result->GetList("entries", &entries);
On 2016/05/20 18:11:57, dcheng wrote:
> On 2016/05/20 at 17:56:54, Devlin wrote:
> > nit: this is silly, since it means we repeat this look up each time we call
> into AddEntryToResult.  Since you're changing the signature anyway, can you
just
> pass in the base::ListValue* entries?
> 
> It's a map internally, so lookup is reasonably timed. And casting from Value
to
> ListValue is constant time.
> 
> The alternatives are:
> - copy and paste these two lines into two different places
> - have CreateResult() return a tuple with an owned DictionaryValue and an
> unowned ListValue
> 
> Both seem worse than the current version.

Ah, I didn't notice it's called from two places.  Nevermind.

[dcheng, 2016-05-20 20:59:09]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-05-20 20:59:10]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1991083002/#ps40001
(title: "IWYU")

[commit-bot: I haz the power, 2016-05-20 20:59:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1991083002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1991083002/40001

[commit-bot: I haz the power, 2016-05-20 22:20:38]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-20 22:22:45]

Description was changed from

==========
Remove ExtensionFunction::SetResult(T*) overload.

Code should use the std::unique_ptr<T> version instead to clearly
indicate ownership.

BUG=581865
==========

to

==========
Remove ExtensionFunction::SetResult(T*) overload.

Code should use the std::unique_ptr<T> version instead to clearly
indicate ownership.

BUG=581865

Committed: https://crrev.com/85f24dacc21477945bb8fb1a769725fb7d492b5a
Cr-Commit-Position: refs/heads/master@{#395180}
==========

[commit-bot: I haz the power, 2016-05-20 22:22:47]

Patchset 3 (id:??) landed as
https://crrev.com/85f24dacc21477945bb8fb1a769725fb7d492b5a
Cr-Commit-Position: refs/heads/master@{#395180}