Take 2 at implementing activeTab.

Implement the activeTab permission, so that extensions with the permission
that get tab-specific permissions granted (currently, only via clicking a browser
action, but this will expand) can executeScript and captureVisibleTab on that tab
only, until the next page navigation or refresh.


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

[Aaron Boodman, 2012-06-06 01:03:13]

Here are some comments, but...

I think we should remove the cross-origin XHR bit of this feature. It seems to
add lots of complexity, and it is not that valuable. Sorry :/

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_manager.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:31: void
SendToProfile(Profile* profile, const E& message) {
Nit: this would make more sense me if it were named SendToRenderers.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:38: host->Send(new
E(message));
Is it possible to pass a heap-allocated message to SendToProfile to avoid this
copy?

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:66:
granted_.Insert(extension);
What happens if the extension is unloaded?

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:70:
ExtensionMsg_SetTabSpecificPermissions(GetPageID() + 1, // mauahaha
also, i believe the correct spelling is 'muah'. It's supposed to sound like
'mwuah', not 'mouh'.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:70:
ExtensionMsg_SetTabSpecificPermissions(GetPageID() + 1, // mauahaha
I don't follow... why +1?

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:116: URLPattern
pattern(URLPattern::SCHEME_FILE |
UserScript::kValidUserScriptSchemes? (ftp is a bit dubious, but at least it's
consistent).

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_manager.h (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.h:27: virtual
~ActiveTabPermissionManager();
I think the destructor should be private; we only want to be deleted via the
WebContentsObserver interface.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/execute_code_in_tab_function.cc (left):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/execute_code_in_tab_function.cc:231:
SendResponse(false);
This seems like it would be a behavior change.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/execute_code_in_tab_function.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/execute_code_in_tab_function.cc:124: return
Execute(code_string);
heh.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_helper.cc:137:
ExtensionTabUtil::GetTabId(web_contents())));
You can't call this->GetTabId() here?

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/extension_toolbar_model.cc:99:
tab_contents->extension_tab_helper()->active_tab_permission_manager()->Grant(
I hate these kind of method names, but it might be worth it here: MaybeGrant?

http://codereview.chromium.org/10443105/diff/16001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3348: const URLPatternSet* tab =
nit: tab_permissions?

http://codereview.chromium.org/10443105/diff/16001/chrome/renderer/extensions...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/renderer/extensions...
chrome/renderer/extensions/user_script_slave.cc:282: // Don't consider the tab
ID when deciding whether to run content scripts;
I think this comment can just be:

// Content scripts are not tab-specific.

[not at google - send to devlin, 2012-06-06 07:38:40]

Hi Aaron,

I went through your comments, fixed up those unit tests, and added a basic test
for ActiveTabPermissionsManager in active_tab_unittest.cc (more to come there
tomorrow).  I had actually planned on starting a new review off before anybody
reviewed it, cos there is a lot of noise in the patch versions.

Regarding reverting those changes to XHR -- as discussed, I did take a while
getting that right, but I do agree that it's complicated.  If you're ok with not
submitting that then it's fine with me (means I don't have to write the tests).

However I think I'll finish off this change without reverting it, then when it's
good to go revert the XHR stuff, submit it, submit another patch with the XHR
stuff over the top, then revert that one.  So that we have it in the commit
history if we decide to go ahead with it.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_manager.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:31: void
SendToProfile(Profile* profile, const E& message) {
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> Nit: this would make more sense me if it were named SendToRenderers.

Done.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:38: host->Send(new
E(message));
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> Is it possible to pass a heap-allocated message to SendToProfile to avoid this
> copy?

Everybody always does "Send(new BlahMessage)" so I figured Send must take
ownership, so copying would be kind of unavoidable. Looking at ipc/ipc_message.h
there's no other way to do it.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:66:
granted_.Insert(extension);
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> What happens if the extension is unloaded?

The extension stays around cos it's refcounted, and when it's
re-enabled/installed it'll have the same ID so still have access to the tab.

When I wrote the code I thought about it and was like "eh yeah that sounds
right" but I don't know anymore. Listening to unloaded events now.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:70:
ExtensionMsg_SetTabSpecificPermissions(GetPageID() + 1, // mauahaha
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> I don't follow... why +1?

Oh, I was just testing that the page ID blocking logic worked.  It does! 
Just... forgot to take it out.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:116: URLPattern
pattern(URLPattern::SCHEME_FILE |
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> UserScript::kValidUserScriptSchemes? (ftp is a bit dubious, but at least it's
> consistent).

Didn't know about that constant. Done.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/execute_code_in_tab_function.cc (left):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/execute_code_in_tab_function.cc:231:
SendResponse(false);
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> This seems like it would be a behavior change.

Don't think so*.  It think it's an existing bug in this code that causes
SendResponse(false) to be called twice (in extension_function.cc, if RunImpl()
returns false, which happens when this method returns false, SendResponse(false)
gets called).

* it actually was, I needed to call it from the other place that Execute() is
called from (DidLoadAndLocalizeFile).  Fixed that.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_helper.cc:137:
ExtensionTabUtil::GetTabId(web_contents())));
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> You can't call this->GetTabId() here?

I can.  Must have wrote that code before adding GetTabId().  Or maybe, I wrote
GetTabId after seeing the silliness of this code.

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
chrome/browser/extensions/extension_toolbar_model.cc:99:
tab_contents->extension_tab_helper()->active_tab_permission_manager()->Grant(
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> I hate these kind of method names, but it might be worth it here: MaybeGrant?

yeah I hate Maybe too. So vague, and always requires looking up the comment on
the method to understand.  Maybe... GrantIfPossible?  GrantIfPermissible
(ewwwww)? GrantIfHasPermission? GrantIfHasActiveTab? 
GrantIfHasActiveTabPermission?

bleh, MaybeGrant it is.

http://codereview.chromium.org/10443105/diff/16001/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3348: const URLPatternSet* tab =
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> nit: tab_permissions?

But then it won't fit on 1 line!

http://codereview.chromium.org/10443105/diff/16001/chrome/renderer/extensions...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/10443105/diff/16001/chrome/renderer/extensions...
chrome/renderer/extensions/user_script_slave.cc:282: // Don't consider the tab
ID when deciding whether to run content scripts;
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> I think this comment can just be:
> 
> // Content scripts are not tab-specific.

Done.

http://codereview.chromium.org/10443105/diff/16001/chrome/renderer/extensions...
chrome/renderer/extensions/user_script_slave.cc:282: // Don't consider the tab
ID when deciding whether to run content scripts;
On 2012/06/06 01:03:13, Aaron Boodman wrote:
> I think this comment can just be:
> 
> // Content scripts are not tab-specific.

Done.

http://codereview.chromium.org/10443105/diff/26001/chrome/common/extensions/e...
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/10443105/diff/26001/chrome/common/extensions/e...
chrome/common/extensions/extension_unittest.cc:865:
EXPECT_FALSE(extension->HasHostPermission(settings_url()));
Looking at the damage I made here, I should revert that accessor change and just
call the member variables http_url and https_url etc.

[Aaron Boodman, 2012-06-06 15:50:23]

OK, your plan for the xhr code sounds good.

Sorry for reviewing early, I didn't understand that you weren't ready
to have this reviewed yet (in that case, I guess it's lucky I did look
at it early!)

- a

On Wed, Jun 6, 2012 at 12:38 AM,  <kalman@chromium.org> wrote:
> Reviewers: kalman, Aaron Boodman,
>
> Message:
> Hi Aaron,
>
> I went through your comments, fixed up those unit tests, and added a basic
> test
> for ActiveTabPermissionsManager in active_tab_unittest.cc (more to come
> there
> tomorrow).  I had actually planned on starting a new review off before
> anybody
> reviewed it, cos there is a lot of noise in the patch versions.
>
> Regarding reverting those changes to XHR -- as discussed, I did take a while
> getting that right, but I do agree that it's complicated.  If you're ok with
> not
> submitting that then it's fine with me (means I don't have to write the
> tests).
>
> However I think I'll finish off this change without reverting it, then when
> it's
> good to go revert the XHR stuff, submit it, submit another patch with the
> XHR
> stuff over the top, then revert that one.  So that we have it in the commit
> history if we decide to go ahead with it.
>
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> File chrome/browser/extensions/active_tab_permission_manager.cc (right):
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/active_tab_permission_manager.cc:31: void
> SendToProfile(Profile* profile, const E& message) {
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> Nit: this would make more sense me if it were named SendToRenderers.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/active_tab_permission_manager.cc:38:
> host->Send(new E(message));
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> Is it possible to pass a heap-allocated message to SendToProfile to
>
> avoid this
>>
>> copy?
>
>
> Everybody always does "Send(new BlahMessage)" so I figured Send must
> take ownership, so copying would be kind of unavoidable. Looking at
> ipc/ipc_message.h there's no other way to do it.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/active_tab_permission_manager.cc:66:
> granted_.Insert(extension);
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> What happens if the extension is unloaded?
>
>
> The extension stays around cos it's refcounted, and when it's
> re-enabled/installed it'll have the same ID so still have access to the
> tab.
>
> When I wrote the code I thought about it and was like "eh yeah that
> sounds right" but I don't know anymore. Listening to unloaded events
> now.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/active_tab_permission_manager.cc:70:
> ExtensionMsg_SetTabSpecificPermissions(GetPageID() + 1, // mauahaha
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> I don't follow... why +1?
>
>
> Oh, I was just testing that the page ID blocking logic worked.  It does!
>  Just... forgot to take it out.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/active_tab_permission_manager.cc:116:
> URLPattern pattern(URLPattern::SCHEME_FILE |
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> UserScript::kValidUserScriptSchemes? (ftp is a bit dubious, but at
>
> least it's
>>
>> consistent).
>
>
> Didn't know about that constant. Done.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> File chrome/browser/extensions/execute_code_in_tab_function.cc (left):
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/execute_code_in_tab_function.cc:231:
> SendResponse(false);
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> This seems like it would be a behavior change.
>
>
> Don't think so*.  It think it's an existing bug in this code that causes
> SendResponse(false) to be called twice (in extension_function.cc, if
> RunImpl() returns false, which happens when this method returns false,
> SendResponse(false) gets called).
>
> * it actually was, I needed to call it from the other place that
> Execute() is called from (DidLoadAndLocalizeFile).  Fixed that.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_tab_helper.cc (right):
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_tab_helper.cc:137:
> ExtensionTabUtil::GetTabId(web_contents())));
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> You can't call this->GetTabId() here?
>
>
> I can.  Must have wrote that code before adding GetTabId().  Or maybe, I
> wrote GetTabId after seeing the silliness of this code.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_toolbar_model.cc (right):
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/browser/extensions/...
> chrome/browser/extensions/extension_toolbar_model.cc:99:
> tab_contents->extension_tab_helper()->active_tab_permission_manager()->Grant(
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> I hate these kind of method names, but it might be worth it here:
>
> MaybeGrant?
>
> yeah I hate Maybe too. So vague, and always requires looking up the
> comment on the method to understand.  Maybe... GrantIfPossible?
> GrantIfPermissible (ewwwww)? GrantIfHasPermission? GrantIfHasActiveTab?
> GrantIfHasActiveTabPermission?
>
> bleh, MaybeGrant it is.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/common/extensions/e...
> File chrome/common/extensions/extension.cc (right):
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/common/extensions/e...
> chrome/common/extensions/extension.cc:3348: const URLPatternSet* tab =
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> nit: tab_permissions?
>
>
> But then it won't fit on 1 line!
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/renderer/extensions...
> File chrome/renderer/extensions/user_script_slave.cc (right):
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/renderer/extensions...
> chrome/renderer/extensions/user_script_slave.cc:282: // Don't consider
> the tab ID when deciding whether to run content scripts;
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> I think this comment can just be:
>
>
>> // Content scripts are not tab-specific.
>
>
> Done.
>
>
>
http://codereview.chromium.org/10443105/diff/16001/chrome/renderer/extensions...
> chrome/renderer/extensions/user_script_slave.cc:282: // Don't consider
> the tab ID when deciding whether to run content scripts;
> On 2012/06/06 01:03:13, Aaron Boodman wrote:
>>
>> I think this comment can just be:
>
>
>> // Content scripts are not tab-specific.
>
>
> Done.
>
>
http://codereview.chromium.org/10443105/diff/26001/chrome/common/extensions/e...
> File chrome/common/extensions/extension_unittest.cc (right):
>
>
http://codereview.chromium.org/10443105/diff/26001/chrome/common/extensions/e...
> chrome/common/extensions/extension_unittest.cc:865:
> EXPECT_FALSE(extension->HasHostPermission(settings_url()));
> Looking at the damage I made here, I should revert that accessor change
> and just call the member variables http_url and https_url etc.
>
> Description:
> Take 2 at implementing activeTab.
>
> Don't review obviously.
>
>
> BUG=
> TEST=
>
>
> Please review this at http://codereview.chromium.org/10443105/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  A chrome/browser/extensions/active_tab_permission_manager.h
>  A chrome/browser/extensions/active_tab_permission_manager.cc
>  A chrome/browser/extensions/active_tab_unittest.cc
>  M chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc
>  M chrome/browser/extensions/execute_code_in_tab_function.cc
>  M chrome/browser/extensions/extension_tab_helper.h
>  M chrome/browser/extensions/extension_tab_helper.cc
>  M chrome/browser/extensions/extension_tab_util.cc
>  M chrome/browser/extensions/extension_tabs_module.h
>  M chrome/browser/extensions/extension_tabs_module.cc
>  M chrome/browser/extensions/extension_toolbar_model.cc
>  M chrome/browser/extensions/script_badge_controller.cc
>  M chrome/chrome_browser_extensions.gypi
>  M chrome/chrome_renderer.gypi
>  M chrome/chrome_tests.gypi
>  M chrome/common/extensions/extension.h
>  M chrome/common/extensions/extension.cc
>  M chrome/common/extensions/extension_messages.h
>  M chrome/common/extensions/extension_messages.cc
>  M chrome/common/extensions/extension_unittest.cc
>  M chrome/common/extensions/url_pattern_set.h
>  M chrome/common/extensions/url_pattern_set.cc
>  M chrome/renderer/extensions/extension_dispatcher.h
>  M chrome/renderer/extensions/extension_dispatcher.cc
>  M chrome/renderer/extensions/extension_helper.h
>  M chrome/renderer/extensions/extension_helper.cc
>  A chrome/renderer/extensions/tab_finder.h
>  A chrome/renderer/extensions/tab_finder.cc
>  M chrome/renderer/extensions/user_script_scheduler.cc
>  M chrome/renderer/extensions/user_script_slave.cc
>  A chrome/test/data/extensions/script_and_capture/tab_specific.json
>
>

[not at google - send to devlin, 2012-06-06 22:35:16]

On 2012/06/06 15:50:23, Aaron Boodman wrote:
> OK, your plan for the xhr code sounds good.
> 
> Sorry for reviewing early, I didn't understand that you weren't ready
> to have this reviewed yet (in that case, I guess it's lucky I did look
> at it early!)
> 
> - a

The non-test code was ready for review, I just had tests to go (plus any other
changes as a result of testing difficulties).

[not at google - send to devlin, 2012-06-06 22:36:18]

On 2012/06/06 22:35:16, kalman wrote:
> On 2012/06/06 15:50:23, Aaron Boodman wrote:
> > OK, your plan for the xhr code sounds good.
> > 
> > Sorry for reviewing early, I didn't understand that you weren't ready
> > to have this reviewed yet (in that case, I guess it's lucky I did look
> > at it early!)
> > 
> > - a
> 
> The non-test code was ready for review, I just had tests to go (plus any other
> changes as a result of testing difficulties).

Though yes, very lucky you got to it early.

Also the description does say "don't review obviously". Not obvious enough, it
seems.

[Aaron Boodman, 2012-06-06 23:45:34]

On Wed, Jun 6, 2012 at 3:36 PM,  <kalman@chromium.org> wrote:
> On 2012/06/06 22:35:16, kalman wrote:
>>
>> On 2012/06/06 15:50:23, Aaron Boodman wrote:
>> > OK, your plan for the xhr code sounds good.
>> >
>> > Sorry for reviewing early, I didn't understand that you weren't ready
>> > to have this reviewed yet (in that case, I guess it's lucky I did look
>> > at it early!)
>> >
>> > - a
>
>
>> The non-test code was ready for review, I just had tests to go (plus any
>> other
>> changes as a result of testing difficulties).
>
>
> Though yes, very lucky you got to it early.
>
> Also the description does say "don't review obviously". Not obvious enough,
> it
> seems.

Weeeeell you sent an email to me saying "done, finally!". I thought I
was supposed to review after that.

- a

[not at google - send to devlin, 2012-06-07 08:49:21]

Alright, done.

I've written a proper bug description, and added a more complete set of tests.

I would also like to add some kind of API test for this stuff, because the
renderer side isn't being exercised.  It uses the same logic as the browser for
most of it (most of the non-XHR stuff anyway) so I'm fairly comfortable with it;
I was thinking about waiting until we have more of an API (like the permissions
stuff we were talking about) around this before writing them.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
File chrome/common/extensions/extension_unittest.cc (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
chrome/common/extensions/extension_unittest.cc:929: // - lots of tab specific
permissions (test merge algorithm).
I've deleted this, just didn't upload yet so as to reduce noise. It only makes
sense with that new ActiveHostPermissions stuff, which is only applicable with
the XHR, and no point testing that if we're not going to submit it anyway.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/renderer/ex...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/renderer/ex...
chrome/renderer/extensions/extension_dispatcher.cc:885: "Wrong page ID, wanted "
<< page_id << " but was " << view->GetPageId();
(Also removed this logging).

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/test/data/e...
File chrome/test/data/extensions/script_and_capture/tab_specific.json (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/test/data/e...
chrome/test/data/extensions/script_and_capture/tab_specific.json:2: "name":
"Extension with no host permissions permissions.",
(and this)

[Aaron Boodman, 2012-06-08 05:31:30]

Nice. Nothing major.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_manager.cc (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:7: #include
"base/memory/ref_counted.h"
Is this used?

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:9: #include
"chrome/browser/extensions/extension_tab_util.h"
Is this used?

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:31: template <class
E>
Why 'E'? How about 'Message' or 'T' (traditional template param name).

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:57: void
ActiveTabPermissionManager::MaybeGrant(const Extension* extension) {
GrantIfRequested? GrantIfWanted?

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:66: if
(old_permissions && old_permissions->size() == active_urls_.size())
Add parens to made desired order of operation clear.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:116: if
(granted_.is_empty())
Nit: I think this is clear w/o the comment.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:135: if
(pattern.Parse(url.spec()) == URLPattern::PARSE_SUCCESS)
If this doesn't parse something has gone very wrong. DCHECK?

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.cc:139: int32
ActiveTabPermissionManager::tab_id() {
This method should be PascalCase because GetTabId is.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_permission_manager.h (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_permission_manager.h:43: virtual void
WebContentsDestroyed(content::WebContents* web_contents)
Add a blank line here. Also, can this be private?

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
File chrome/browser/extensions/active_tab_unittest.cc (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:37:
permissions->Append(Value::CreateStringValue("activeTab"));
indent -= 2;

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:45: FilePath(),
Nit: these params can be start on the same line as Create(, and you can put
multiple on one line.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:46: Extension::EXTERNAL_PREF,
Is there any particular reason for this location? Otherwise I would just use
INTERNAL since it is 'standard'. Using other than standard makes it seem
important somehow.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:48: 0,
Document or constify magic number. Same with string below.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:53: {
These blocks are a little unusual. Consider moving these initializations into
helper functions. You might be able to share a little code, too.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:55:
permissions->Append(Value::CreateStringValue("activeTab"));
indent -= 2;

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:92: return extension_.get();
Can you just make the members protected? The getters seem overkill when
everything is used within this one file.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:103: int tab_id() {
Nit: GetTabId().

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:107:
ActiveTabPermissionManager* manager() {
'manager' is a little generic. Maybe active_tab_manager, or just
active_tab_permission_manager.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:112: bool Allowed(const
Extension* extension, const GURL& url) {
IsAllowed

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:121: bool Blocked(const
Extension* extension, const GURL& url) {
IsBlocked

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:339:
EXPECT_TRUE(Allowed(extension(), chromium_blank));
Maybe comment that this works because the comparison is by origin?

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/active_tab_unittest.cc:444: }
Very nice tests.

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_helper.cc:8: #include
"chrome/browser/extensions/extension_tab_util.h"
Used?

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
File chrome/browser/extensions/extension_tab_helper.h (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/browser/extensions/...
chrome/browser/extensions/extension_tab_helper.h:123: virtual void
RenderViewCreated(content::RenderViewHost* render_view_host)
Weird to have OVERRIDE on line by itself. Consider wrapping at first paren. Not
sure what the rule is.

http://codereview.chromium.org/10443105/diff/27032/chrome/common/extensions/e...
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3792: std::vector<linked_ptr<const
URLPatternSet> > all;
Consider moving this into a method of URLPatternSet.

http://codereview.chromium.org/10443105/diff/27032/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3800: for (size_t i = 0; i < all.size() -
skip; i += skip) {
(all.size() - skip) for clarity.

http://codereview.chromium.org/10443105/diff/27032/chrome/common/extensions/e...
chrome/common/extensions/extension.cc:3820: (*out)[it->first] = *it->second;
You aren't saving much by passing an out param because the URLPatternSets are
still copied by value. I would say just return by value. Many compilers optimize
it away, anyway.

http://codereview.chromium.org/10443105/diff/27032/chrome/common/extensions/e...
File chrome/common/extensions/extension_messages.h (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/common/extensions/e...
chrome/common/extensions/extension_messages.h:129: std::map<int, URLPatternSet>
tab_specific_host_permissions;
I'm curious how this part is going to work when you take out the cross-origin
XHR support. Only two renderers are going to care about this information then.
Are you just going to leave the member in the message, but only populate it for
certain renderers?

http://codereview.chromium.org/10443105/diff/27032/chrome/common/extensions/e...
chrome/common/extensions/extension_messages.h:244: // Tell the renderer what its
tab ID is.
Technically its the render *view* that has a tab ID. The comment for the message
above is also wrong.

http://codereview.chromium.org/10443105/diff/27032/chrome/common/extensions/e...
chrome/common/extensions/extension_messages.h:256: // Tell the renderer about
new tab-specific permissions for an extension.
It seems like this duplicates the functionality of the new member in
ExtensionMsg_ExtensionsLoaded. Can you use this for both cases? I don't think an
extra IPC at renderer startup is a big deal.

http://codereview.chromium.org/10443105/diff/27032/chrome/renderer/extensions...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:841: case
UpdatedExtensionPermissionsInfo::REMOVED:
default: NOTREACHED() or CHECK(false)

http://codereview.chromium.org/10443105/diff/27032/chrome/renderer/extensions...
chrome/renderer/extensions/extension_dispatcher.cc:878: // If this renderer
contains the target tab then check against page_id to
If we were keeping this code, probably the cleaner way to do it would be
translate the XHR-related temporary permissions into 'normal' permissions in the
browser and communicate them to the renderer that way. The renderer doesn't need
to know that a host permission update was because of activeTab or not.

The tab-specific permissions would only be communicated to the one renderer that
actually contains that tab, and this caveat would be unnecessary.

http://codereview.chromium.org/10443105/diff/27032/chrome/renderer/extensions...
File chrome/renderer/extensions/user_script_slave.cc (right):

http://codereview.chromium.org/10443105/diff/27032/chrome/renderer/extensions...
chrome/renderer/extensions/user_script_slave.cc:283: int no_tab_id = -1;
Nit: const int kNoTabId.

http://codereview.chromium.org/10443105/diff/27032/chrome/renderer/extensions...
chrome/renderer/extensions/user_script_slave.cc:284: 
Nit: you can delete this blank line.

[not at google - send to devlin, 2012-06-12 20:40:51]

So I ended up removing the XHR stuff in this patch.

However, I kept the n-way union stuff even though it's not used.  Might be
useful.  Added tests for it.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
File chrome/browser/extensions/active_tab_permission_manager.cc (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.cc:7: #include
"base/memory/ref_counted.h"
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Is this used?

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.cc:9: #include
"chrome/browser/extensions/extension_tab_util.h"
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Is this used?

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.cc:31: template <class
E>
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Why 'E'? How about 'Message' or 'T' (traditional template param name).

Done. Dunno why E, weird.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.cc:57: void
ActiveTabPermissionManager::MaybeGrant(const Extension* extension) {
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> GrantIfRequested? GrantIfWanted?

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.cc:66: if
(old_permissions && old_permissions->size() == active_urls_.size())
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Add parens to made desired order of operation clear.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.cc:116: if
(granted_.is_empty())
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Nit: I think this is clear w/o the comment.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.cc:135: if
(pattern.Parse(url.spec()) == URLPattern::PARSE_SUCCESS)
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> If this doesn't parse something has gone very wrong. DCHECK?

Some legitimate reasons it won't parse is for things like chrome:// URLs.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.cc:139: int32
ActiveTabPermissionManager::tab_id() {
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> This method should be PascalCase because GetTabId is.

I think I'll change the one on ExtensionTabHelper to be tab_id. It's only
calling unix_style methods.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
File chrome/browser/extensions/active_tab_permission_manager.h (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_permission_manager.h:43: virtual void
WebContentsDestroyed(content::WebContents* web_contents)
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Add a blank line here. Also, can this be private?

I usually see methods bunched together like this when they're just a bunch of
overrides to an interface. But ok.

This one can be private but the other can't, I'm calling it from a test
(unfortunately, but I don't see any other way) to fake creating frames on this
tab.

Anyway, updated.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:37:
permissions->Append(Value::CreateStringValue("activeTab"));
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> indent -= 2;

Aww I like it like that.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:46: Extension::EXTERNAL_PREF,
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Is there any particular reason for this location? Otherwise I would just use
> INTERNAL since it is 'standard'. Using other than standard makes it seem
> important somehow.

Oh ok. INTERNAL it is.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:48: 0,
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Document or constify magic number. Same with string below.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:53: {
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> These blocks are a little unusual. Consider moving these initializations into
> helper functions. You might be able to share a little code, too.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:55:
permissions->Append(Value::CreateStringValue("activeTab"));
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> indent -= 2;

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:92: return extension_.get();
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Can you just make the members protected? The getters seem overkill when
> everything is used within this one file.

It's so I could type () instead of .get() everywhere. If I just make these
methods accept scoped_refptrs though, won't need either.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:103: int tab_id() {
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Nit: GetTabId().

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:107:
ActiveTabPermissionManager* manager() {
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> 'manager' is a little generic. Maybe active_tab_manager, or just
> active_tab_permission_manager.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:112: bool Allowed(const
Extension* extension, const GURL& url) {
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> IsAllowed

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:121: bool Blocked(const
Extension* extension, const GURL& url) {
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> IsBlocked

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:339:
EXPECT_TRUE(Allowed(extension(), chromium_blank));
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Maybe comment that this works because the comparison is by origin?

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/active_tab_unittest.cc:444: }
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Very nice tests.

Thank you.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
File chrome/browser/extensions/extension_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/extension_tab_helper.cc:8: #include
"chrome/browser/extensions/extension_tab_util.h"
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Used?

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
File chrome/browser/extensions/extension_tab_helper.h (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/browser/ext...
chrome/browser/extensions/extension_tab_helper.h:123: virtual void
RenderViewCreated(content::RenderViewHost* render_view_host)
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Weird to have OVERRIDE on line by itself. Consider wrapping at first paren.
Not
> sure what the rule is.

ok.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
File chrome/common/extensions/extension.cc (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
chrome/common/extensions/extension.cc:3792: std::vector<linked_ptr<const
URLPatternSet> > all;
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Consider moving this into a method of URLPatternSet.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
chrome/common/extensions/extension.cc:3800: for (size_t i = 0; i < all.size() -
skip; i += skip) {
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> (all.size() - skip) for clarity.

ok.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
chrome/common/extensions/extension.cc:3820: (*out)[it->first] = *it->second;
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> You aren't saving much by passing an out param because the URLPatternSets are
> still copied by value. I would say just return by value. Many compilers
optimize
> it away, anyway.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
File chrome/common/extensions/extension_messages.h (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
chrome/common/extensions/extension_messages.h:129: std::map<int, URLPatternSet>
tab_specific_host_permissions;
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> I'm curious how this part is going to work when you take out the cross-origin
> XHR support. Only two renderers are going to care about this information then.
> Are you just going to leave the member in the message, but only populate it
for
> certain renderers?

Yeah, good point.

Actually, in the world of not caring about XHR, I'm not sure that anybody cares
about it.  As in, the tab cares about it, but if this is the tab being created
then presumably nothing has clicked on it yet, so.... yeah.

And the background page cares as much as any other renderer that isn't the
tab's, which is to say, not at all.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
chrome/common/extensions/extension_messages.h:244: // Tell the renderer what its
tab ID is.
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Technically its the render *view* that has a tab ID. The comment for the
message
> above is also wrong.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/common/exte...
chrome/common/extensions/extension_messages.h:256: // Tell the renderer about
new tab-specific permissions for an extension.
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> It seems like this duplicates the functionality of the new member in
> ExtensionMsg_ExtensionsLoaded. Can you use this for both cases? I don't think
an
> extra IPC at renderer startup is a big deal.

It would be a bit awkward to re-use it, but it's moot anyway because I removed
it from Loading.

I'm going to rename this "Update" by the way, to match the other IPC.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/renderer/ex...
File chrome/renderer/extensions/extension_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/renderer/ex...
chrome/renderer/extensions/extension_dispatcher.cc:841: case
UpdatedExtensionPermissionsInfo::REMOVED:
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> default: NOTREACHED() or CHECK(false)

clang catches missing enum branches as a compile error; better to leave
"default" out.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/renderer/ex...
chrome/renderer/extensions/extension_dispatcher.cc:878: // If this renderer
contains the target tab then check against page_id to
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> If we were keeping this code, probably the cleaner way to do it would be
> translate the XHR-related temporary permissions into 'normal' permissions in
the
> browser and communicate them to the renderer that way. The renderer doesn't
need
> to know that a host permission update was because of activeTab or not.
> 
> The tab-specific permissions would only be communicated to the one renderer
that
> actually contains that tab, and this caveat would be unnecessary.

Yeah, we discussed this, but I'll write some of it down anyway.

It would definitely work to put a lot of the smarts in the browser to figure out
what to send each renderer. But it would be complicated.  I think it's easier to
keep the logic the same everywhere by having that stuff in Extension.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/renderer/ex...
File chrome/renderer/extensions/user_script_slave.cc (right):

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/renderer/ex...
chrome/renderer/extensions/user_script_slave.cc:283: int no_tab_id = -1;
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Nit: const int kNoTabId.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/27032/chrome/renderer/ex...
chrome/renderer/extensions/user_script_slave.cc:284: 
On 2012/06/08 05:31:30, Aaron Boodman wrote:
> Nit: you can delete this blank line.

Done.

https://chromiumcodereview.appspot.com/10443105/diff/20004/chrome/common/exte...
File chrome/common/extensions/url_pattern_set_unittest.cc (left):

https://chromiumcodereview.appspot.com/10443105/diff/20004/chrome/common/exte...
chrome/common/extensions/url_pattern_set_unittest.cc:129:
TEST(URLPatternSetTest, CreateUnion) {
oops

[Aaron Boodman, 2012-06-12 21:21:38]

lgtm

https://chromiumcodereview.appspot.com/10443105/diff/20004/chrome/common/exte...
File chrome/common/extensions/extension_messages.h (right):

https://chromiumcodereview.appspot.com/10443105/diff/20004/chrome/common/exte...
chrome/common/extensions/extension_messages.h:241: // Tell the render review
what its tab ID is.
heh.

[not at google - send to devlin, 2012-06-12 21:24:25]

https://chromiumcodereview.appspot.com/10443105/diff/20004/chrome/common/exte...
File chrome/common/extensions/extension_messages.h (right):

https://chromiumcodereview.appspot.com/10443105/diff/20004/chrome/common/exte...
chrome/common/extensions/extension_messages.h:241: // Tell the render review
what its tab ID is.
On 2012/06/12 21:21:38, Aaron Boodman wrote:
> heh.

oops

[commit-bot: I haz the power, 2012-06-12 21:25:19]

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

[commit-bot: I haz the power, 2012-06-12 21:25:36]

Failed to apply patch for chrome/browser/extensions/extension_tab_helper.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_tab_helper.cc
Hunk #2 FAILED at 49.
Hunk #3 succeeded at 76 (offset 1 line).
Hunk #4 succeeded at 130 (offset 1 line).
1 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_tab_helper.cc.rej

[commit-bot: I haz the power, 2012-06-12 21:51:57]

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

[commit-bot: I haz the power, 2012-06-12 22:29:22]

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

[commit-bot: I haz the power, 2012-06-12 22:48:02]

Try job failure for 10443105-21002 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-12 23:03:55]

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

[commit-bot: I haz the power, 2012-06-13 00:22:59]

Change committed as 141801

[jstritar, 2012-06-15 15:28:23]

Nice work, look forward to trying it out!