Add ChromeExtensionFunctionDetails

Add ChromeExtensionFunctionDetails

In the interest of phasing out the totally unnecessary
*Chrome*ExtensionFunction base classes, this establishes a
ChromeExtensionFunctionDetails object which can be created
by extension function implementations in //chrome who need
access to Chrome-specific details.

This object can be very easily composed into such function
implementations such that they can move away from using
e.g. ChromeAsyncExtensionFunction or
ChromeUIThreadExtensionFunction as a base class and instead
rely on more universal and generic ExtensionFunction and its
immediate derivatives.

A conversion of the tabs API ExecuteCodeInTabFunction is
included for demonstration purposes.

BUG=None
R=scheib@chromium.org
CC=lfg@chromium.org

Committed: https://crrev.com/1e1bbdec0613dedaa9e43c84a9b5346a942f1841
Cr-Commit-Position: refs/heads/master@{#293570}

[Ken Rockot(use gerrit already), 2014-09-04 22:25:41]

scheib: Ask and you shall receive! Behold, a code review.

This is motivated by some recent refactoring in the guestview code which caused
me to realized that I hate Chrome*ExtensionFunction base classes and that they
are totally unnecessary.

I will convert additional derivatives (and eventually delete
chrome_extension_function.h/cc) in future patches.

[Ken Rockot(use gerrit already), 2014-09-04 22:27:02]

Also +CC yoz who may have some feels about this.

[Yoyo Zhou, 2014-09-04 23:41:58]

yoz@chromium.org changed reviewers:
+ yoz@chromium.org

[Yoyo Zhou, 2014-09-04 23:41:59]

Cool, I like the looks of this.

https://codereview.chromium.org/543873002/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extension_function_details.h (right):

https://codereview.chromium.org/543873002/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extension_function_details.h:22: class
ChromeExtensionFunctionDetails {
class description comment?

https://codereview.chromium.org/543873002/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extension_function_details.h:64: // since such
instances should only be created and owned exclusively by
This doesn't look exactly as you've described in the example in windows_util.

Another way to look at it is that this is equivalent to a set of utility methods
for ExtensionFunction, similar to the manifest handler getters for Extension. I
think this is somewhat better than the manifest handler style (which would be
FunctionDetails::GetBlah(function) ) but the lifetime I'm not sure how to best
describe.

[Ken Rockot(use gerrit already), 2014-09-04 23:53:57]

Heh yeah, oops. I wrote that comment before windows_util reared its head.
I'll reword it. I'd like to keep the non-static style just because it's
less painful for almost all use cases.


On Thu, Sep 4, 2014 at 4:41 PM, <yoz@chromium.org> wrote:

> Cool, I like the looks of this.
>
>
> https://codereview.chromium.org/543873002/diff/1/chrome/
> browser/extensions/chrome_extension_function_details.h
> File chrome/browser/extensions/chrome_extension_function_details.h
> (right):
>
> https://codereview.chromium.org/543873002/diff/1/chrome/
> browser/extensions/chrome_extension_function_details.h#newcode22
> chrome/browser/extensions/chrome_extension_function_details.h:22: class
> ChromeExtensionFunctionDetails {
> class description comment?
>
> https://codereview.chromium.org/543873002/diff/1/chrome/
> browser/extensions/chrome_extension_function_details.h#newcode64
> chrome/browser/extensions/chrome_extension_function_details.h:64: //
> since such instances should only be created and owned exclusively by
> This doesn't look exactly as you've described in the example in
> windows_util.
>
> Another way to look at it is that this is equivalent to a set of utility
> methods for ExtensionFunction, similar to the manifest handler getters
> for Extension. I think this is somewhat better than the manifest handler
> style (which would be FunctionDetails::GetBlah(function) ) but the
> lifetime I'm not sure how to best describe.
>
> https://codereview.chromium.org/543873002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2014-09-05 00:45:03]

Also added deprecation notices to chrome_extension_function.h

https://codereview.chromium.org/543873002/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extension_function_details.h (right):

https://codereview.chromium.org/543873002/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extension_function_details.h:22: class
ChromeExtensionFunctionDetails {
On 2014/09/04 23:41:59, Yoyo Zhou wrote:
> class description comment?

Done.

https://codereview.chromium.org/543873002/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extension_function_details.h:64: // since such
instances should only be created and owned exclusively by
On 2014/09/04 23:41:58, Yoyo Zhou wrote:
> This doesn't look exactly as you've described in the example in windows_util.
> 
> Another way to look at it is that this is equivalent to a set of utility
methods
> for ExtensionFunction, similar to the manifest handler getters for Extension.
I
> think this is somewhat better than the manifest handler style (which would be
> FunctionDetails::GetBlah(function) ) but the lifetime I'm not sure how to best
> describe.

I just removed the inaccurate part.

[scheib, 2014-09-05 03:17:22]

lgtm with optional nits.

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1713: bool
ExecuteCodeInTabFunction::Init() {
Thanks for fixing other member's order to match declaration. Consider moving
Init() as well since you're here.

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/chrome_extension_function_details.h (right):

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_extension_function_details.h:22: // This can be
used to provide a a UIThreadExtensionFunction implementation with
" a a " typo.

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_extension_function_details.h:23: // some
additional Chrome-specific details.
consider shortening to: "Provides UIThreadExtensionFunction implementations with
Chrome-specific details."

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_extension_function_details.h:64: // The
function for which these details have been created. Must outlive the
Consider making this point at the class or constructor level as well?

[Ken Rockot(use gerrit already), 2014-09-05 16:47:40]

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/tabs/tabs_api.cc:1713: bool
ExecuteCodeInTabFunction::Init() {
On 2014/09/05 03:17:22, scheib wrote:
> Thanks for fixing other member's order to match declaration. Consider moving
> Init() as well since you're here.

Done.

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/chrome_extension_function_details.h (right):

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_extension_function_details.h:22: // This can be
used to provide a a UIThreadExtensionFunction implementation with
On 2014/09/05 03:17:22, scheib wrote:
> " a a " typo.

Done.

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_extension_function_details.h:23: // some
additional Chrome-specific details.
On 2014/09/05 03:17:22, scheib wrote:
> consider shortening to: "Provides UIThreadExtensionFunction implementations
with
> Chrome-specific details."

Done.

https://codereview.chromium.org/543873002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/chrome_extension_function_details.h:64: // The
function for which these details have been created. Must outlive the
On 2014/09/05 03:17:22, scheib wrote:
> Consider making this point at the class or constructor level as well?

Done.

[Ken Rockot(use gerrit already), 2014-09-05 16:48:05]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:48:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/543873002/40001

[commit-bot: I haz the power, 2014-09-05 17:10:35]

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

[commit-bot: I haz the power, 2014-09-05 17:10:36]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ken Rockot(use gerrit already), 2014-09-05 17:17:53]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-09-05 17:18:03]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-09-05 17:18:20]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-09-05 17:19:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/543873002/40001

[commit-bot: I haz the power, 2014-09-05 19:21:59]

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

[commit-bot: I haz the power, 2014-09-05 19:21:59]

Exceeded time limit waiting for builds to trigger.

[Ken Rockot(use gerrit already), 2014-09-05 19:23:58]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-09-05 19:24:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rockot@chromium.org/543873002/40001

[commit-bot: I haz the power, 2014-09-05 20:03:30]

Committed patchset #3 (id:40001) as 5bb3897ed4c37bad5181ee90798cbb5dc8c173de

[commit-bot: I haz the power, 2014-09-10 03:41:14]

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