Refactor PermissionsData pt1

Refactor PermissionsData pt1

In this CL, we:
- Make PermissionsData not-so-static. Only methods which either don't rely on
  knowledge of specific permissions or methods to get/create the object are
  static.
- Separate runtime permissions knowledge from manifest parsing (PermissionsData
  vs PermissionsParser)
- Move required/optional permissions access into PermissionsParser.
- Handle the locking issues.

BUG=379356
TBR=sky@chromium.org (for c/b/ui/, no functional changes)

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

[Devlin, 2014-06-02 22:33:49]

Yes, this looks really, really scary.  But's it's not as bad as it seems.  The
only files with real changes are PermissionsData and the new PermissionsParser.

[Lei Zhang, 2014-06-02 22:40:30]

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:99: if
(extensions::PermissionsData::ForExtension(*i)
I'm only looking at the media galleries bits.

Here, you can save the returned pointer from
extensions::PermissionsData::ForExtension(*i) and use it again on the next line
to avoid having to write "->" on a separate line.

On the topic of these very long lines of code, I think the aussies have been
writing:

very_long_line->
    method()

rather than:

very_long_line
    ->method()

We should try to be consistent there.

[not at google - send to devlin, 2014-06-02 22:43:00]

On 2014/06/02 22:40:30, Lei Zhang wrote:
>
https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
> File chrome/browser/media_galleries/media_galleries_preferences.cc (right):
> 
>
https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
> chrome/browser/media_galleries/media_galleries_preferences.cc:99: if
> (extensions::PermissionsData::ForExtension(*i)
> I'm only looking at the media galleries bits.
> 
> Here, you can save the returned pointer from
> extensions::PermissionsData::ForExtension(*i) and use it again on the next
line
> to avoid having to write "->" on a separate line.
> 
> On the topic of these very long lines of code, I think the aussies have been
> writing:
> 
> very_long_line->
>     method()
> 
> rather than:
> 
> very_long_line
>     ->method()
> 
> We should try to be consistent there.

blame the aussies!

I'm happy with whatever git cl format decides to write.

[not at google - send to devlin, 2014-06-02 23:20:05]

looking forward to that ActivePermissions change :)

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2519:
->HasAPIPermission(extensions::APIPermission::kSocket)) {
it's true though; one of the advantages of
extensions::PermissionsData::ForExtension(..) is that you can actually hold onto
that PermissionsData. I'll point out places. here is one.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:76:
PermissionsData::ForExtension(extension)
and here

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_tab_unittest.cc:114:
PermissionsData::ForExtension(extension)
here

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:135: bool
has_read_permission =
here

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_permissions.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:136:
extensions::PermissionsData::ForExtension(extension)
here

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/browser_permissions_policy_delegate_unittest.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/browser_permissions_policy_delegate_unittest.cc:77:
EXPECT_TRUE(PermissionsData::ForExtension(extension)
here

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.cc:299: if
(SharedModuleInfo::IsSharedModule(extension) &&
here

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1049: if
(extensions::PermissionsData::ForExtension(extension)
here

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:124:
extensions::PermissionsData::ForExtension(extension_)
here

https://codereview.chromium.org/309533007/diff/40001/chrome/common/extensions...
File chrome/common/extensions/extension_unittest.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/common/extensions...
chrome/common/extensions/extension_unittest.cc:239:
EXPECT_FALSE(PermissionsData::ForExtension(extension)->CanExecuteScriptOnPage(
here!

https://codereview.chromium.org/309533007/diff/40001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:284:
PermissionsParser::AddAPIPermission(
here

https://codereview.chromium.org/309533007/diff/40001/extensions/browser/api/s...
File extensions/browser/api/socket/socket_api.cc (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/browser/api/s...
extensions/browser/api/socket/socket_api.cc:878: if
(!PermissionsData::ForExtension(GetExtension())
so much of this. almost seems like there should be
ExtensionFunction::GetPermissionsData() but that sounds confusing and ultimately
unnecessary.

https://codereview.chromium.org/309533007/diff/40001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/extens...
extensions/common/extension.h:316: }
neither PermissionsParser nor PermissionsData belong here either. you might want
to mention that.

https://codereview.chromium.org/309533007/diff/40001/extensions/common/manife...
File extensions/common/manifest_handlers/permissions_parser.h (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/manife...
extensions/common/manifest_handlers/permissions_parser.h:22: // name).
not too keen on the name "PermissionsParser", even though it's strictly not a
ManifestHandler I do think there's not much harm to come from pretending that it
is; PermissionsHandler vs PermissionsInfo (though I'd hold off on the
"permissions info" since it's rather confusing given the presence of
"permissions data").

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (left):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:321: scoped_refptr<const
PermissionSet> PermissionsData::GetActivePermissions(
i actually liked this method, it means you don't need to lock everywhere :)

how about either adding it back, or having a method like
active_permissions_safe() accessor and renaming active_permissions_ to
active_permissions_unsafe_.

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:10: #include
"extensions/common/error_utils.h"
changes to this file is pretty crazy. effectively it's just removing a few
things and locking a few things right?

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:34: // PermissionInfo, and
PermissionsData. No one will be able to keep them
ah. yes, well, there you go :)

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data_unittest.cc (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data_unittest.cc:98:
PermissionsData::ForExtension(extension)
here

at this point I feel pretty bad asking you to change this, and the other place
there were like 50 of these in the 1 place. it should only be a single vim
command though.

[not at google - send to devlin, 2014-06-03 14:39:35]

https://codereview.chromium.org/309533007/diff/40001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/extens...
extensions/common/extension.h:316: }
On 2014/06/02 23:20:06, kalman wrote:
> neither PermissionsParser nor PermissionsData belong here either. you might
want
> to mention that.

Thinking about this a bit more, PermissionsData *does* belong here,
PermissionsParser *does not*.
1- [conceptual] manifest handlers don't live on Extension
2- OTOH "active permissions" are fundamental to Extensions and we should be
encouraging people to use them not-awkwardly; so leave the permissions_data()
accessor.

(we should still be looking to remove the HasAPIPermission methods etc though)

If you agree with (2) then that's another avenue you could pursue for cleanups
in the CL, but go ahead with whatever is minimal churn.

[Devlin, 2014-06-03 15:28:21]

All changes were originally made via script, which ensured that it was no better
nor worse than original performance (the bad stuff was just more obvious).  But,
since you guys took the trouble to comment on it, they've all been fixed.

Cheers!

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:2519:
->HasAPIPermission(extensions::APIPermission::kSocket)) {
On 2014/06/02 23:20:06, kalman wrote:
> it's true though; one of the advantages of
> extensions::PermissionsData::ForExtension(..) is that you can actually hold
onto
> that PermissionsData. I'll point out places. here is one.

Done.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_tab_permission_granter.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_tab_permission_granter.cc:76:
PermissionsData::ForExtension(extension)
On 2014/06/02 23:20:06, kalman wrote:
> and here

Done.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_tab_unittest.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_tab_unittest.cc:114:
PermissionsData::ForExtension(extension)
On 2014/06/02 23:20:06, kalman wrote:
> here

Done.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:135: bool
has_read_permission =
On 2014/06/02 23:20:06, kalman wrote:
> here

Done.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_permissions.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:136:
extensions::PermissionsData::ForExtension(extension)
On 2014/06/02 23:20:06, kalman wrote:
> here

Not done. Reusing pointer makes no cases faster and DO_NOT_CHECK_HOST case
slower.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/browser_permissions_policy_delegate_unittest.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/browser_permissions_policy_delegate_unittest.cc:77:
EXPECT_TRUE(PermissionsData::ForExtension(extension)
On 2014/06/02 23:20:06, kalman wrote:
> here

Done.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/crx_installer.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/crx_installer.cc:299: if
(SharedModuleInfo::IsSharedModule(extension) &&
On 2014/06/02 23:20:06, kalman wrote:
> here

Not done. No speed advantage and given that we refer to multiple
permissionsdatas here, I think it would make it more confusing.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1049: if
(extensions::PermissionsData::ForExtension(extension)
On 2014/06/02 23:20:06, kalman wrote:
> here

Done.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
File chrome/browser/media_galleries/media_galleries_preferences.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_preferences.cc:99: if
(extensions::PermissionsData::ForExtension(*i)
On 2014/06/02 22:40:31, Lei Zhang wrote:
> I'm only looking at the media galleries bits.
> 
> Here, you can save the returned pointer from
> extensions::PermissionsData::ForExtension(*i) and use it again on the next
line
> to avoid having to write "->" on a separate line.
> 
> On the topic of these very long lines of code, I think the aussies have been
> writing:
> 
> very_long_line->
>     method()
> 
> rather than:
> 
> very_long_line
>     ->method()
> 
> We should try to be consistent there.

Happy to do both.  As Kalman points out, git cl format prefers newline ->, but
OWNERs preference takes precedence.

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
File
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/media_gal...
chrome/browser/media_galleries/media_galleries_scan_result_dialog_controller.cc:124:
extensions::PermissionsData::ForExtension(extension_)
On 2014/06/02 23:20:06, kalman wrote:
> here

Done.

https://codereview.chromium.org/309533007/diff/40001/chrome/common/extensions...
File chrome/common/extensions/extension_unittest.cc (right):

https://codereview.chromium.org/309533007/diff/40001/chrome/common/extensions...
chrome/common/extensions/extension_unittest.cc:239:
EXPECT_FALSE(PermissionsData::ForExtension(extension)->CanExecuteScriptOnPage(
On 2014/06/02 23:20:06, kalman wrote:
> here!

No! ;)

Look closer.  We reassign extension, like, every two lines.

https://codereview.chromium.org/309533007/diff/40001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:284:
PermissionsParser::AddAPIPermission(
On 2014/06/02 23:20:06, kalman wrote:
> here

This is PermissionsParser, not PermissionsData.

https://codereview.chromium.org/309533007/diff/40001/extensions/browser/api/s...
File extensions/browser/api/socket/socket_api.cc (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/browser/api/s...
extensions/browser/api/socket/socket_api.cc:878: if
(!PermissionsData::ForExtension(GetExtension())
On 2014/06/02 23:20:06, kalman wrote:
> so much of this. almost seems like there should be
> ExtensionFunction::GetPermissionsData() but that sounds confusing and
ultimately
> unnecessary.

Yeah.... let's hold off on that for now.

https://codereview.chromium.org/309533007/diff/40001/extensions/common/extens...
File extensions/common/extension.h (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/extens...
extensions/common/extension.h:316: }
On 2014/06/03 14:39:36, kalman wrote:
> On 2014/06/02 23:20:06, kalman wrote:
> > neither PermissionsParser nor PermissionsData belong here either. you might
> want
> > to mention that.
> 
> Thinking about this a bit more, PermissionsData *does* belong here,
> PermissionsParser *does not*.
> 1- [conceptual] manifest handlers don't live on Extension
> 2- OTOH "active permissions" are fundamental to Extensions and we should be
> encouraging people to use them not-awkwardly; so leave the permissions_data()
> accessor.
> 
> (we should still be looking to remove the HasAPIPermission methods etc though)
> 
> If you agree with (2) then that's another avenue you could pursue for cleanups
> in the CL, but go ahead with whatever is minimal churn.

Yeah, I came to the same conclusion (hence the todo on permissions data).  In a
cleanup CL, I'll see if I can't take out ForExtension().

https://codereview.chromium.org/309533007/diff/40001/extensions/common/manife...
File extensions/common/manifest_handlers/permissions_parser.h (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/manife...
extensions/common/manifest_handlers/permissions_parser.h:22: // name).
On 2014/06/02 23:20:06, kalman wrote:
> not too keen on the name "PermissionsParser", even though it's strictly not a
> ManifestHandler I do think there's not much harm to come from pretending that
it
> is; PermissionsHandler vs PermissionsInfo (though I'd hold off on the
> "permissions info" since it's rather confusing given the presence of
> "permissions data").

IIRC, we decided on [X]Info for handlers.  Since PermissionsInfo is taken, that
doesn't make sense.  And if we add in PermissionsHandler, we're adding to the
confusion between [X]Info and [X]Handler, which we're just now trying to
regulate.  I think I'd like to make it clearly separate, if that's okay?

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (left):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:321: scoped_refptr<const
PermissionSet> PermissionsData::GetActivePermissions(
On 2014/06/02 23:20:06, kalman wrote:
> i actually liked this method, it means you don't need to lock everywhere :)
> 
> how about either adding it back, or having a method like
> active_permissions_safe() accessor and renaming active_permissions_ to
> active_permissions_unsafe_.

Since active_permissions() is public, it seems odd to expose that part (it's
just an implementation detail - we generally assume public functions are safe
unless otherwise indicated).  But we can definitely rename to unsafe_ and
replace most of these locks and accesses with just active_permissions().

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:10: #include
"extensions/common/error_utils.h"
On 2014/06/02 23:20:06, kalman wrote:
> changes to this file is pretty crazy. effectively it's just removing a few
> things and locking a few things right?

Effectively... yes.

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:34: // PermissionInfo, and
PermissionsData. No one will be able to keep them
On 2014/06/02 23:20:06, kalman wrote:
> ah. yes, well, there you go :)

haha yep.

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data_unittest.cc (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data_unittest.cc:98:
PermissionsData::ForExtension(extension)
On 2014/06/02 23:20:06, kalman wrote:
> here
> 
> at this point I feel pretty bad asking you to change this, and the other place
> there were like 50 of these in the 1 place. it should only be a single vim
> command though.

Again, we reassign extension every 5 lines. :)

[not at google - send to devlin, 2014-06-03 15:45:16]

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_permissions.cc
(right):

https://codereview.chromium.org/309533007/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:136:
extensions::PermissionsData::ForExtension(extension)
On 2014/06/03 15:28:21, Devlin wrote:
> On 2014/06/02 23:20:06, kalman wrote:
> > here
> 
> Not done. Reusing pointer makes no cases faster and DO_NOT_CHECK_HOST case
> slower.

fine, but note that I'm not worried about the performance, but rather the
brain-load of seeing the same code repeated.

https://codereview.chromium.org/309533007/diff/40001/extensions/common/manife...
File extensions/common/manifest_handlers/permissions_parser.h (right):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/manife...
extensions/common/manifest_handlers/permissions_parser.h:22: // name).
On 2014/06/03 15:28:21, Devlin wrote:
> On 2014/06/02 23:20:06, kalman wrote:
> > not too keen on the name "PermissionsParser", even though it's strictly not
a
> > ManifestHandler I do think there's not much harm to come from pretending
that
> it
> > is; PermissionsHandler vs PermissionsInfo (though I'd hold off on the
> > "permissions info" since it's rather confusing given the presence of
> > "permissions data").
> 
> IIRC, we decided on [X]Info for handlers.  Since PermissionsInfo is taken,
that
> doesn't make sense.  And if we add in PermissionsHandler, we're adding to the
> confusion between [X]Info and [X]Handler, which we're just now trying to
> regulate.  I think I'd like to make it clearly separate, if that's okay?

alright :)

aside: PermissionsInfo is bizarre.

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
File extensions/common/permissions/permissions_data.cc (left):

https://codereview.chromium.org/309533007/diff/40001/extensions/common/permis...
extensions/common/permissions/permissions_data.cc:321: scoped_refptr<const
PermissionSet> PermissionsData::GetActivePermissions(
On 2014/06/03 15:28:21, Devlin wrote:
> On 2014/06/02 23:20:06, kalman wrote:
> > i actually liked this method, it means you don't need to lock everywhere :)
> > 
> > how about either adding it back, or having a method like
> > active_permissions_safe() accessor and renaming active_permissions_ to
> > active_permissions_unsafe_.
> 
> Since active_permissions() is public, it seems odd to expose that part (it's
> just an implementation detail - we generally assume public functions are safe
> unless otherwise indicated).  But we can definitely rename to unsafe_ and
> replace most of these locks and accesses with just active_permissions().

Hm I didn't see that active_permissions() got added and was public.

But, yes to your second sentence, that is what I was getting at.

[Devlin, 2014-06-03 15:47:59]

https://chromiumcodereview.appspot.com/309533007/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/api/web_request/web_request_permissions.cc
(right):

https://chromiumcodereview.appspot.com/309533007/diff/40001/chrome/browser/ex...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:136:
extensions::PermissionsData::ForExtension(extension)
On 2014/06/03 15:45:16, kalman wrote:
> On 2014/06/03 15:28:21, Devlin wrote:
> > On 2014/06/02 23:20:06, kalman wrote:
> > > here
> > 
> > Not done. Reusing pointer makes no cases faster and DO_NOT_CHECK_HOST case
> > slower.
> 
> fine, but note that I'm not worried about the performance, but rather the
> brain-load of seeing the same code repeated.

Agreed, and if it's a wash in speed, no problem, but I'd prefer not to slow it
down (especially when it can be inlined like this).

Also, this will be even better when we just do
extension->active_permissions()->Foo().

[not at google - send to devlin, 2014-06-03 15:50:25]

fair enough.

ok, lgtm once you do the unsafe_active_permissions_ change and make callers use
active_permissions() without locking.

[not at google - send to devlin, 2014-06-03 15:51:43]

lgtm

https://chromiumcodereview.appspot.com/309533007/diff/50001/extensions/common...
File extensions/common/permissions/permissions_data.cc (right):

https://chromiumcodereview.appspot.com/309533007/diff/50001/extensions/common...
extensions/common/permissions/permissions_data.cc:37: base::AutoLock
aut_lock(runtime_lock_);
auto_lock

https://chromiumcodereview.appspot.com/309533007/diff/50001/extensions/common...
File extensions/common/permissions/permissions_data.h (right):

https://chromiumcodereview.appspot.com/309533007/diff/50001/extensions/common...
extensions/common/permissions/permissions_data.h:197: // Unsafe indicates that
we must lock anytime this is directly accessed.
Say "use the active_permissions() accessor instead"

[Devlin, 2014-06-03 15:56:15]

On 2014/06/03 15:50:25, kalman wrote:
> fair enough.
> 
> ok, lgtm once you do the unsafe_active_permissions_ change and make callers
use
> active_permissions() without locking.

Already done in patch set 2.

https://chromiumcodereview.appspot.com/309533007/diff/50001/extensions/common...
File extensions/common/permissions/permissions_data.cc (right):

https://chromiumcodereview.appspot.com/309533007/diff/50001/extensions/common...
extensions/common/permissions/permissions_data.cc:37: base::AutoLock
aut_lock(runtime_lock_);
On 2014/06/03 15:51:43, kalman wrote:
> auto_lock

Done.

https://chromiumcodereview.appspot.com/309533007/diff/50001/extensions/common...
File extensions/common/permissions/permissions_data.h (right):

https://chromiumcodereview.appspot.com/309533007/diff/50001/extensions/common...
extensions/common/permissions/permissions_data.h:197: // Unsafe indicates that
we must lock anytime this is directly accessed.
On 2014/06/03 15:51:43, kalman wrote:
> Say "use the active_permissions() accessor instead"

Done.

[Devlin, 2014-06-03 20:07:13]

thestig@, friendly ping?

[Lei Zhang, 2014-06-03 20:26:04]

On 2014/06/03 20:07:13, Devlin wrote:
> thestig@, friendly ping?

Which part do you want me to review in particular?

[Devlin, 2014-06-03 20:58:16]

On 2014/06/03 20:26:04, Lei Zhang wrote:
> On 2014/06/03 20:07:13, Devlin wrote:
> > thestig@, friendly ping?
> 
> Which part do you want me to review in particular?

Since there are no functional changes anywhere here, it should all be TBR-able. 
But since you had made requests for changes in media_galleries, I wanted to make
sure you're happy with it.

[Lei Zhang, 2014-06-03 21:22:43]

On 2014/06/03 20:58:16, Devlin wrote:
> On 2014/06/03 20:26:04, Lei Zhang wrote:
> > On 2014/06/03 20:07:13, Devlin wrote:
> > > thestig@, friendly ping?
> > 
> > Which part do you want me to review in particular?
> 
> Since there are no functional changes anywhere here, it should all be
TBR-able. 
> But since you had made requests for changes in media_galleries, I wanted to
make
> sure you're happy with it.

Yep. lgtm for chrome/ bits, but I only spot checked.

[Devlin, 2014-06-04 21:52:32]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-06-04 21:54:58]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/309533007/...

[commit-bot: I haz the power, 2014-06-05 02:10:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-05 02:25:31]

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

[commit-bot: I haz the power, 2014-06-05 02:25:32]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)

[Devlin, 2014-06-05 16:03:26]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-06-05 16:04:26]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/309533007/...

[commit-bot: I haz the power, 2014-06-05 16:16:34]

Change committed as 275142