Fully merge the implementations of the browserAction and pageAction APIs

Fully merge the implementations of the browserAction and pageAction APIs

BUG=124605
TEST=browser_tests --gtest_filter=*BrowserAction* and --gtest_filter=*PageAction*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134080

[not at google - send to devlin, 2012-04-26 04:26:57]

Yay!

8 files changed, 298 insertions(+), 403 deletions(-)

Aaron this is the browser side of that other patch pulled out. Hopefully it's
more to your taste. It's more to my taste too.

(note that koz is on leave so probably won't get to this next week if at all).

[Aaron Boodman, 2012-04-26 05:30:53]

Seems cool to me.

https://chromiumcodereview.appspot.com/10231002/diff/2001/chrome/browser/exte...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://chromiumcodereview.appspot.com/10231002/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:11:
#include
"chrome/browser/extensions/api/extension_action/extension_page_actions_api_constants.h"
There's no reason for these separate constants files anymore. Can you merge them
together in a separate change?

https://chromiumcodereview.appspot.com/10231002/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:57: if
(arg->GetType() == Value::TYPE_INTEGER) {
Nit: There's IsType().

https://chromiumcodereview.appspot.com/10231002/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:64:
EXTENSION_FUNCTION_VALIDATE(false);
You could simplify this slightly by making the previous branch an else and then
making this EXTENSION_FUNCTION_VALIDATE(IsType(DICTIONARY)).

https://chromiumcodereview.appspot.com/10231002/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:84: void
ExtensionActionFunction::NotifyChange() {
Nit: I would make this abstract and have the subclasses implement it, since they
are different.

https://chromiumcodereview.appspot.com/10231002/diff/2001/chrome/browser/exte...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:142:
EXTENSION_FUNCTION_VALIDATE(GetExtension()->page_action());
If this has to always be page_action, why put it here?

[not at google - send to devlin, 2012-04-26 06:01:56]

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:11:
#include
"chrome/browser/extensions/api/extension_action/extension_page_actions_api_constants.h"
On 2012/04/26 05:30:54, Aaron Boodman wrote:
> There's no reason for these separate constants files anymore. Can you merge
them
> together in a separate change?

Merge which files?

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:57: if
(arg->GetType() == Value::TYPE_INTEGER) {
On 2012/04/26 05:30:54, Aaron Boodman wrote:
> Nit: There's IsType().

I'm innocent!

I'm going to switch. I like switching.

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:64:
EXTENSION_FUNCTION_VALIDATE(false);
On 2012/04/26 05:30:54, Aaron Boodman wrote:
> You could simplify this slightly by making the previous branch an else and
then
> making this EXTENSION_FUNCTION_VALIDATE(IsType(DICTIONARY)).

Oh, I made it a switch. I think it looks nice now.

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:84: void
ExtensionActionFunction::NotifyChange() {
On 2012/04/26 05:30:54, Aaron Boodman wrote:
> Nit: I would make this abstract and have the subclasses implement it, since
they
> are different.

Yeah that's the thing, it's what I'm trying to avoid.

When the pageAction API can be called despite the extension having a
browser_action rather than a page_action (as in my next patch) it's this code
that prevents us needing a CommandLine check. I like that.

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:142:
EXTENSION_FUNCTION_VALIDATE(GetExtension()->page_action());
On 2012/04/26 05:30:54, Aaron Boodman wrote:
> If this has to always be page_action, why put it here?

Yeah, good point. I was trying to make this API work independently of what the
actual API bindings were (in extension_function_registry) since that's the
situation we are probably going to end up with, sort of.

Anyway in my next patch this changes to

if (!GetExtension()->page_action())
  return true;

since when the pageAction API gets called into a converted browser_action we'll
end up with this.

Eventually we'll add whatever the UX is for a page action that wants to show.

Anyway, I'll remove it for now.

[Aaron Boodman, 2012-04-26 06:25:36]

LGTM

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:11:
#include
"chrome/browser/extensions/api/extension_action/extension_page_actions_api_constants.h"
On 2012/04/26 06:01:57, kalman wrote:
> On 2012/04/26 05:30:54, Aaron Boodman wrote:
> > There's no reason for these separate constants files anymore. Can you merge
> them
> > together in a separate change?
> 
> Merge which files?

Nevermind, I didn't read this carefully.

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:84: void
ExtensionActionFunction::NotifyChange() {
On 2012/04/26 06:01:57, kalman wrote:
> On 2012/04/26 05:30:54, Aaron Boodman wrote:
> > Nit: I would make this abstract and have the subclasses implement it, since
> they
> > are different.
> 
> Yeah that's the thing, it's what I'm trying to avoid.
> 
> When the pageAction API can be called despite the extension having a
> browser_action rather than a page_action (as in my next patch) it's this code
> that prevents us needing a CommandLine check. I like that.

I see! OK.

http://codereview.chromium.org/10231002/diff/2001/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:142:
EXTENSION_FUNCTION_VALIDATE(GetExtension()->page_action());
On 2012/04/26 06:01:57, kalman wrote:
> On 2012/04/26 05:30:54, Aaron Boodman wrote:
> > If this has to always be page_action, why put it here?
> 
> Yeah, good point. I was trying to make this API work independently of what the
> actual API bindings were (in extension_function_registry) since that's the
> situation we are probably going to end up with, sort of.
> 
> Anyway in my next patch this changes to
> 
> if (!GetExtension()->page_action())
>   return true;
> 
> since when the pageAction API gets called into a converted browser_action
we'll
> end up with this.
> 
> Eventually we'll add whatever the UX is for a page action that wants to show.
> 
> Anyway, I'll remove it for now.

It's fine, after seeing the other patch, it makes sense.

[commit-bot: I haz the power, 2012-04-26 07:36:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10231002/4003

[commit-bot: I haz the power, 2012-04-26 07:36:58]

Can't process patch for file
chrome/browser/extensions/api/extension_action/extension_page_actions_api.h.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2012-04-26 07:37:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/10231002/13001

[commit-bot: I haz the power, 2012-04-26 09:07:53]

Change committed as 134080