Refactor ExtensionManagement

Refactor ExtensionManagement

This CL removes IndividualSettings and GlobalSettings from ExtensionManagment header file in order to simply header file and reduce static size of ExtensionManagement class. Linked pointer is used to prevent potential unintended use of copy constructor of these structure in the future, when more fields are added.

A new internal header file is created since these structures are also used in unit tests.

BUG=177351
Committed: https://crrev.com/81d7c55c47677582259ab1b62b92d0d7c2557d47
Cr-Commit-Position: refs/heads/master@{#297808}

[binjin, 2014-09-24 19:54:35]

[email protected] changed reviewers:
+ [email protected]

[binjin, 2014-09-24 19:56:42]

Hello Joao,

I refactored the ExtensionManagement a bit, PTAL.

-bjin

[Joao da Silva, 2014-09-25 08:53:51]

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

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:41:
IndividualSettings::IndividualSettings() {
Move this to extension_management_internal.cc

(same for everything else from the internal namespace here)

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:205: return true;
return std::find(...) != allowed_types.end();

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:50: // Will be called when then
extension management setting changed.
Called when the extension management settings change.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:103: SettingsIdMap;
I'm guessing that the linked_ptrs are used so that you can put the ownership in
this map, and so that you can point to forward declared types.

We've tried to refrain from using linked_ptrs in the past, because they are just
ref counted ptrs and there are better ways to do the same thing.

For this map, I think you can use base/containers/scoped_ptr_hash_map.h. That is
just a hash_map and should work for ExtensionId (which is just a typedef for
std::string). It will automatically own the IndividualSettings, so no need for
linked_ptrs there.

You'll probably need to include extension_management_internal.h in this header
too, because the ScopedPtrHashMap needs to "know" how to delete
IndividualSettings structs.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:116: base::Value::Type
expected_type);
#include "base/values.h"

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:123: linked_ptr<const
internal::IndividualSettings> ReadById(
Return an IndividualSettings* here

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:127: linked_ptr<const
internal::GlobalSettings> ReadGlobalSettings() const;
Same here

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:132:
linked_ptr<internal::IndividualSettings> AccessById(const ExtensionId& id);
And here

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:147:
linked_ptr<internal::IndividualSettings> default_settings_;
scoped_ptr works here. Actually, there is no need for a smart ptr if you include
extension_management_internal.h, which is fine.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:150:
linked_ptr<internal::GlobalSettings> global_settings_;
Same here.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_management_internal.h (right):

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.h:21: namespace internal
{
The names in this file are specific to the ExtensionManagement class but are
quite generic. For example:

  extensions::internal::Scope

This could easily be used by another file too. I suggest having yet another
namespace wrapping these names:

  extensions::management::internal::Scope

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.h:56: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.h:74: };
DISALLOW_COPY_AND_ASSIGN

[Joao da Silva, 2014-09-25 09:42:52]

[email protected] changed reviewers:
+ [email protected]

[Joao da Silva, 2014-09-25 09:42:53]

+pastarmovj to review while I'm away.

This CL is currently waiting for a new patch set, no nothing new to review yet
:-)

[binjin, 2014-09-25 13:19:19]

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

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:41:
IndividualSettings::IndividualSettings() {
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> Move this to extension_management_internal.cc
> 
> (same for everything else from the internal namespace here)

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.cc:205: return true;
On 2014/09/25 08:53:50, Joao da Silva (OOO until Oct6) wrote:
> return std::find(...) != allowed_types.end();

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:50: // Will be called when then
extension management setting changed.
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> Called when the extension management settings change.

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:103: SettingsIdMap;
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> I'm guessing that the linked_ptrs are used so that you can put the ownership
in
> this map, and so that you can point to forward declared types.
> 
> We've tried to refrain from using linked_ptrs in the past, because they are
just
> ref counted ptrs and there are better ways to do the same thing.
> 
> For this map, I think you can use base/containers/scoped_ptr_hash_map.h. That
is
> just a hash_map and should work for ExtensionId (which is just a typedef for
> std::string). It will automatically own the IndividualSettings, so no need for
> linked_ptrs there.
> 
> You'll probably need to include extension_management_internal.h in this header
> too, because the ScopedPtrHashMap needs to "know" how to delete
> IndividualSettings structs.
Done. But without extension_management_internal.h included in header file. It
compiles fine now and including the internal header file actually part ways from
my original goal for splitting it.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:116: base::Value::Type
expected_type);
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> #include "base/values.h"

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:123: linked_ptr<const
internal::IndividualSettings> ReadById(
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> Return an IndividualSettings* here

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:127: linked_ptr<const
internal::GlobalSettings> ReadGlobalSettings() const;
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> Same here

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:132:
linked_ptr<internal::IndividualSettings> AccessById(const ExtensionId& id);
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> And here

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:147:
linked_ptr<internal::IndividualSettings> default_settings_;
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> scoped_ptr works here. Actually, there is no need for a smart ptr if you
include
> extension_management_internal.h, which is fine.

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:150:
linked_ptr<internal::GlobalSettings> global_settings_;
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> Same here.

Done.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_management_internal.h (right):

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.h:21: namespace internal
{
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> The names in this file are specific to the ExtensionManagement class but are
> quite generic. For example:
> 
>   extensions::internal::Scope
> 
> This could easily be used by another file too. I suggest having yet another
> namespace wrapping these names:
> 
>   extensions::management::internal::Scope

I moved Scope Into IndividualSettings instead. "extension::management" was
occupied by namespace alias "management=api::management" in management api code.
Current code looks file for me since the scope of this internal header file is
limited to only two cc files.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.h:56: };
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> DISALLOW_COPY_AND_ASSIGN

Done. I actually explicitly used copy constructor of this, so added
DISALLOW_ASSIAN instead.

https://codereview.chromium.org/602803002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.h:74: };
On 2014/09/25 08:53:51, Joao da Silva (OOO until Oct6) wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Joao da Silva, 2014-09-25 14:09:41]

Looks good to me. I leave it to Julian to do a final review & give a stamp.

https://codereview.chromium.org/602803002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/602803002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:8: #include <map>
Not used

https://codereview.chromium.org/602803002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/602803002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.cc:49: if
(this->installation_mode == ExtensionManagement::INSTALLATION_FORCED ||
Remove this-> (it's not used above either)

[binjin, 2014-09-25 14:17:23]

https://codereview.chromium.org/602803002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management.h (right):

https://codereview.chromium.org/602803002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management.h:8: #include <map>
On 2014/09/25 14:09:41, Joao da Silva (OOO until Oct6) wrote:
> Not used

Done.

https://codereview.chromium.org/602803002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_management_internal.cc (right):

https://codereview.chromium.org/602803002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_management_internal.cc:49: if
(this->installation_mode == ExtensionManagement::INSTALLATION_FORCED ||
On 2014/09/25 14:09:41, Joao da Silva (OOO until Oct6) wrote:
> Remove this-> (it's not used above either)

Done.

[binjin, 2014-09-25 18:42:18]

Patchset #5 (id:80001) has been deleted

[binjin, 2014-09-25 18:51:28]

@pastarmovj: the newest patchset is ready to be reviewed.

[pastarmovj, 2014-10-02 10:16:03]

lgtm

[binjin, 2014-10-02 10:25:05]

[email protected] changed reviewers:
+ [email protected]

[binjin, 2014-10-02 10:25:06]

Finnur, could you have a look at this CL?

[Finnur, 2014-10-02 10:37:44]

LGTM from an owners perspective.

[binjin, 2014-10-02 10:50:23]

The CQ bit was checked by [email protected]

[commit-bot: I haz the power, 2014-10-02 10:50:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/602803002/100001

[commit-bot: I haz the power, 2014-10-02 11:04:56]

The CQ bit was unchecked by [email protected]

[commit-bot: I haz the power, 2014-10-02 11:04:57]

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

[binjin, 2014-10-02 11:10:55]

The CQ bit was checked by [email protected]

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/602803002/100001

[commit-bot: I haz the power, 2014-10-02 11:47:21]

Committed patchset #5 (id:100001) as 448d50af05443c537efe3d7c40d19e7d27fb3f2a

[commit-bot: I haz the power, 2014-10-02 11:48:00]

Patchset 5 (id:??) landed as
https://crrev.com/81d7c55c47677582259ab1b62b92d0d7c2557d47
Cr-Commit-Position: refs/heads/master@{#297808}