Always send the full tab object in ExtensionAction click event.

Always send the full tab object in ExtensionAction click event.

Along the way, decompose a few swiss army knife functions to
simplify and generalize code.

BUG=149020
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157082

[Aaron Boodman, 2012-09-16 07:30:10]

+hebert.christopherj fyi

[Aaron Boodman, 2012-09-16 08:54:08]

https://codereview.chromium.org/10909256/diff/5002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_util.cc (left):

https://codereview.chromium.org/10909256/diff/5002/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_util.cc:125: tab_index,
APIPermission::kTab) ||
bug here! should be tab_id!

[not at google - send to devlin, 2012-09-17 01:46:53]

lgtm

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/message_service.cc:165: source_contents,
ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
so I guess this is safe because by this point the extension presumably has
access to the tab, and we know it's safe?

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_event_router.cc (right):

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/b...
chrome/browser/extensions/browser_event_router.cc:418:
args_base->Append(Value::CreateIntegerValue(
can use AppendInteger now.

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/b...
chrome/browser/extensions/browser_event_router.cc:594:
tab_contents->web_contents(), NULL, -1,
It looks like you've added a convenience form of this which leaves out the NULL
and -1 arguments? Use that?

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_util.cc (left):

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_util.cc:127: tab_index,
APIPermission::kWebNavigation);
so web navigation no longer grants it?

[Aaron Boodman, 2012-09-17 03:22:17]

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/messaging/message_service.cc (right):

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/message_service.cc:165: source_contents,
ExtensionTabUtil::INCLUDE_PRIVACY_SENSITIVE_FIELDS));
On 2012/09/17 01:46:53, kalman wrote:
> so I guess this is safe because by this point the extension presumably has
> access to the tab, and we know it's safe?

Yep.

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_util.cc (left):

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_util.cc:127: tab_index,
APIPermission::kWebNavigation);
On 2012/09/17 01:46:53, kalman wrote:
> so web navigation no longer grants it?

Yeah, it's a bit confusing, and I'm not sure I've decided how this should work.

Today, the tabs and webNavigation permission are essentially completely
separate. The fact that they grant access to the same sort of data is a
coincidence, and only reflected in the fact that they happen to display the same
warning message.

We could either:

1) Keep them totally separate. You need to specify 'tabs' to get access to
privacy-sensitive bits of the tabs API, and 'webNavigation' to get access to the
privacy-sensitive bits of the webNavigation API.

2) Make them interchangeable. You can specify either 'tabs' or 'webNavigation'
and get access to the privacy sensitive bits of either the tabs or webNavigation
APIs.

3) Either (1) or (2), but also deprecate both permissions and add a new one that
means 'access to privacy-sensitive data throughout the system'. Something like
'browsingActivity'.

I am currently leaning toward to 1+3. 2 feels confusing - it's weird that
specifying 'tabs' - a permission that used to correspond to a very particular
API now affects other APIs. Same with webNavigation.

Do you have any opinion on this? Feedback appreciated.

[chebert, 2012-09-17 05:20:01]

https://chromiumcodereview.appspot.com/10909256/diff/5002/chrome/browser/exte...
File chrome/browser/extensions/extension_tab_util.cc (left):

https://chromiumcodereview.appspot.com/10909256/diff/5002/chrome/browser/exte...
chrome/browser/extensions/extension_tab_util.cc:125: tab_index,
APIPermission::kTab) ||
ahh dang. I think this is mine. Good catch and thanks, aaron.
On 2012/09/16 08:54:08, Aaron Boodman wrote:
> bug here! should be tab_id!

[not at google - send to devlin, 2012-09-17 05:28:20]

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_util.cc (left):

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_util.cc:127: tab_index,
APIPermission::kWebNavigation);
On 2012/09/17 03:22:17, Aaron Boodman wrote:
> On 2012/09/17 01:46:53, kalman wrote:
> > so web navigation no longer grants it?
> 
> Yeah, it's a bit confusing, and I'm not sure I've decided how this should
work.
> 
> Today, the tabs and webNavigation permission are essentially completely
> separate. The fact that they grant access to the same sort of data is a
> coincidence, and only reflected in the fact that they happen to display the
same
> warning message.
> 
> We could either:
> 
> 1) Keep them totally separate. You need to specify 'tabs' to get access to
> privacy-sensitive bits of the tabs API, and 'webNavigation' to get access to
the
> privacy-sensitive bits of the webNavigation API.
> 
> 2) Make them interchangeable. You can specify either 'tabs' or 'webNavigation'
> and get access to the privacy sensitive bits of either the tabs or
webNavigation
> APIs.
> 
> 3) Either (1) or (2), but also deprecate both permissions and add a new one
that
> means 'access to privacy-sensitive data throughout the system'. Something like
> 'browsingActivity'.
> 
> I am currently leaning toward to 1+3. 2 feels confusing - it's weird that
> specifying 'tabs' - a permission that used to correspond to a very particular
> API now affects other APIs. Same with webNavigation.
> 
> Do you have any opinion on this? Feedback appreciated.

It almost seems as though the tabs and webNavigation APIs could share the same
API namespace. But too late to consider that, and that isn't the point anyway.

TBH I don't understand why the tabs/webNavigation permissions play into this
privacy stuff anyway. Shouldn't that be entirely determined via host permissions
(+activeTab)?

Am I confused?

[Aaron Boodman, 2012-09-17 05:32:04]

On Sun, Sep 16, 2012 at 10:28 PM,  <kalman@chromium.org> wrote:
> Am I confused?

Maybe a little. We can sort it out on IM.

- a

[Aaron Boodman, 2012-09-17 05:51:12]

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/b...
File chrome/browser/extensions/browser_event_router.cc (right):

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/b...
chrome/browser/extensions/browser_event_router.cc:418:
args_base->Append(Value::CreateIntegerValue(
On 2012/09/17 01:46:53, kalman wrote:
> can use AppendInteger now.

Done.

http://codereview.chromium.org/10909256/diff/4003/chrome/browser/extensions/b...
chrome/browser/extensions/browser_event_router.cc:594:
tab_contents->web_contents(), NULL, -1,
On 2012/09/17 01:46:53, kalman wrote:
> It looks like you've added a convenience form of this which leaves out the
NULL
> and -1 arguments? Use that?

Done.