Introduce DeclarativeUserScriptManager.

This CL introduce DeclarativeUserScriptManager to manager a set of DeclarativeUserScriptMaster objects which were stored in the ExtensionSystemImpl.

With this refactor, it allows the DeclarativeUserScriptManager to take care of the creations of DeclarativeUserScriptMaster objects:
1) It enable the DeclarativeUserScriptManager accept all kinds of input keys and convert them as a uniform format;
2) The manager will assign a unique ID to the consumer who requests a DeclarativeUserScriptMaster object.The consumer can use this ID to do script injection.
(The above two steps will be implemented in another CLs)

This refactor is a part of issue "support DeclarativeContent script injection on <webview>", please refer to a design doc(https://goto.google.com/introduceidforscriptinjection).
BUG=437566
Committed: https://crrev.com/6d9b43a5c431b04947bdc135b179166e55199efa
Cr-Commit-Position: refs/heads/master@{#308171}

[Xi Han, 2014-12-11 19:23:09]

Patchset #2 (id:20001) has been deleted

[Xi Han, 2014-12-11 19:25:27]

Patchset #1 (id:1) has been deleted

[Xi Han, 2014-12-11 19:25:34]

Patchset #1 (id:40001) has been deleted

[Xi Han, 2014-12-11 19:29:49]

hanxi@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Xi Han, 2014-12-11 19:29:50]

Please take a look, thanks!

[Fady Samuel, 2014-12-11 19:58:27]

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

https://codereview.chromium.org/796453003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/declarative_user_script_manager.cc:23: for
(ScopedVector<DeclarativeUserScriptMaster>::iterator it =
Why not use a map?

[Xi Han, 2014-12-12 15:42:13]

PTAL, thanks!

[Fady Samuel, 2014-12-12 15:44:31]

https://codereview.chromium.org/796453003/diff/100001/extensions/browser/exte...
File extensions/browser/extension_system.h (right):

https://codereview.chromium.org/796453003/diff/100001/extensions/browser/exte...
extensions/browser/extension_system.h:81: virtual DeclarativeUserScriptManager*
declarative_user_script_manager() = 0;
This is not a trivial accessor: please call this
GetDeclarativeUserScriptManager. Thanks!

[Fady Samuel, 2014-12-12 16:17:53]

lgtm

https://codereview.chromium.org/796453003/diff/100001/extensions/browser/exte...
File extensions/browser/extension_system.h (right):

https://codereview.chromium.org/796453003/diff/100001/extensions/browser/exte...
extensions/browser/extension_system.h:81: virtual DeclarativeUserScriptManager*
declarative_user_script_manager() = 0;
On 2014/12/12 15:44:31, Fady Samuel wrote:
> This is not a trivial accessor: please call this
> GetDeclarativeUserScriptManager. Thanks!

OK, it seems like the naming in this entire file is off. I'm OK with this for
now for consistency.

[Xi Han, 2014-12-12 17:05:05]

hanxi@chromium.org changed reviewers:
+ kalman@chromium.org

[Xi Han, 2014-12-12 17:05:05]

kalman@chromium.org: Please review all changes. Thanks!

[not at google - send to devlin, 2014-12-12 17:25:26]

kalman@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[not at google - send to devlin, 2014-12-12 17:25:26]

I think Devlin knows this code?

[Xi Han, 2014-12-12 17:27:45]

Devlin: please take a look, thanks!

[Devlin, 2014-12-12 18:08:07]

High-level question that should maybe go on the doc, but are we planning on
keeping the DeclarativeUserScriptMaster/Manager in the extensions namespace?  Or
will they be moved elsewhere?

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_manager.h (right):

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:7: 
#include <map>

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:18: class
DeclarativeUserScriptManager {
Class comments needed.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:23: // Get the user
script master for declarative scripts, if any.
This "if any" is misleading - it implies this could return null.
Instead, how about "Get the user script master for declarative scripts; if one
does not exist, it will be created."?

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:25: const
ExtensionId& extension_id);
I'm slightly confused on passing in an ExtensionId here - isn't one of the goals
to make this accept an abstract "id"?  (In practice, this change will probably
just be changing "ExtensionId" to "std::string", but from a conceptual
standpoint, it makes a bigger difference).

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:30: typedef
std::map<ExtensionId, linked_ptr<DeclarativeUserScriptMaster>>
nit: we now prefer C++ 11 style for these:
using UserScriptMasterMap = std::map<ExtensionId,
linked_ptr<DeclarativeUserScriptMaster>>;

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:30: typedef
std::map<ExtensionId, linked_ptr<DeclarativeUserScriptMaster>>
nit: typedefs before class members.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:33: // Shared memory
region manager for programmatically declared scripts.
I'm not sure what the "Shared memory region" refers to for this...?  (If it's
not referring to base::SharedMemory, probably best to rephrase, since the Script
Injection System also uses that shared memory.)

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:34: // Managers are
instantiated the first time the declarative
nit: s/the first time..../lazily.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:43:
DeclarativeUserScriptManager* declarative_user_script_manager() override;
needs the "shared' comment.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:117: // Shared memory region
manager for programmatically declared scripts, one
Update this comment.

https://codereview.chromium.org/796453003/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_extension_system.cc (right):

https://codereview.chromium.org/796453003/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_extension_system.cc:126: return NULL;
nit: nullptr.

[Xi Han, 2014-12-12 19:44:25]

Patchset #4 (id:120001) has been deleted

[Xi Han, 2014-12-12 19:47:24]

Devlin: I believe eventually DeclarativeUserScriptManager will be moved out of
extensions namespace, but I am sure where it will go so far. So I simply remove
the extension namespace in its .h and .cc files. PTAL, thanks!

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_manager.h (right):

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:7: 
On 2014/12/12 18:08:07, Devlin wrote:
> #include <map>

Done.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:18: class
DeclarativeUserScriptManager {
On 2014/12/12 18:08:07, Devlin wrote:
> Class comments needed.

Done.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:23: // Get the user
script master for declarative scripts, if any.
On 2014/12/12 18:08:07, Devlin wrote:
> This "if any" is misleading - it implies this could return null.
> Instead, how about "Get the user script master for declarative scripts; if one
> does not exist, it will be created."?

Good catch, updated:)

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:25: const
ExtensionId& extension_id);
On 2014/12/12 18:08:07, Devlin wrote:
> I'm slightly confused on passing in an ExtensionId here - isn't one of the
goals
> to make this accept an abstract "id"?  (In practice, this change will probably
> just be changing "ExtensionId" to "std::string", but from a conceptual
> standpoint, it makes a bigger difference).

I planed this CL only moves code around, but I can do some renaming as well to
improve readability.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:30: typedef
std::map<ExtensionId, linked_ptr<DeclarativeUserScriptMaster>>
On 2014/12/12 18:08:07, Devlin wrote:
> nit: typedefs before class members.

Moved.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:30: typedef
std::map<ExtensionId, linked_ptr<DeclarativeUserScriptMaster>>
On 2014/12/12 18:08:07, Devlin wrote:
> nit: we now prefer C++ 11 style for these:
> using UserScriptMasterMap = std::map<ExtensionId,
> linked_ptr<DeclarativeUserScriptMaster>>;

Thanks for pointing it out:)

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:33: // Shared memory
region manager for programmatically declared scripts.
On 2014/12/12 18:08:07, Devlin wrote:
> I'm not sure what the "Shared memory region" refers to for this...?  (If it's
> not referring to base::SharedMemory, probably best to rephrase, since the
Script
> Injection System also uses that shared memory.)

I think it is refer to base::SharedMemory, since the UserScriptLoader within
each master object has a pointer to the base::sharedMemory. Updated the
description to avoid confusion.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:34: // Managers are
instantiated the first time the declarative
On 2014/12/12 18:08:07, Devlin wrote:
> nit: s/the first time..../lazily.

Done.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.h (right):

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:43:
DeclarativeUserScriptManager* declarative_user_script_manager() override;
On 2014/12/12 18:08:07, Devlin wrote:
> needs the "shared' comment.

Added.

https://codereview.chromium.org/796453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.h:117: // Shared memory region
manager for programmatically declared scripts, one
On 2014/12/12 18:08:07, Devlin wrote:
> Update this comment.

Done.

https://codereview.chromium.org/796453003/diff/100001/extensions/shell/browse...
File extensions/shell/browser/shell_extension_system.cc (right):

https://codereview.chromium.org/796453003/diff/100001/extensions/shell/browse...
extensions/shell/browser/shell_extension_system.cc:126: return NULL;
On 2014/12/12 18:08:07, Devlin wrote:
> nit: nullptr.

Done.

[Devlin, 2014-12-12 20:02:45]

Sorry for the back-and-forth on extensions namespace.

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_manager.cc (right):

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.cc:22: return
(it->second).get();
nit: don't need parens around (it->second).

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_manager.h (right):

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:15: namespace
extensions {
hmm... actually, for as long as this class is in c/b/extensions, let's leave it
in the extensions namespace for consistency.

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:19: using
UserScriptMasterMap =
nit: When I mentioned this should go above class members, I just meant it should
be above the member variables (i.e., it should be above
declarative_user_script_masters_ and profile_).  You were right with putting it
in the private: section of the class. :)

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:28: // Get the user
script master for declarative scripts; if does not exist, a
nit: "if one does not exist" instead of "if does not exist".

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:34: // Manages a set
of DeclarativeUserScript objects for programmatically
This seems more like a comment of the full class (DeclarativeUserScriptManager),
rather than the map.  Maybe just "A map of DeclarativeUserScriptMasters for ids;
each master is lazily initialized."?

[Xi Han, 2014-12-12 20:32:31]

PTAL, thanks!

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_manager.cc (right):

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.cc:22: return
(it->second).get();
On 2014/12/12 20:02:45, Devlin wrote:
> nit: don't need parens around (it->second).

Done.

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_manager.h (right):

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:15: namespace
extensions {
On 2014/12/12 20:02:45, Devlin wrote:
> hmm... actually, for as long as this class is in c/b/extensions, let's leave
it
> in the extensions namespace for consistency.

That is ok, add the namespace back.

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:19: using
UserScriptMasterMap =
On 2014/12/12 20:02:45, Devlin wrote:
> nit: When I mentioned this should go above class members, I just meant it
should
> be above the member variables (i.e., it should be above
> declarative_user_script_masters_ and profile_).  You were right with putting
it
> in the private: section of the class. :)

Done.

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:28: // Get the user
script master for declarative scripts; if does not exist, a
On 2014/12/12 20:02:45, Devlin wrote:
> nit: "if one does not exist" instead of "if does not exist".

Done.

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:28: // Get the user
script master for declarative scripts; if does not exist, a
On 2014/12/12 20:02:45, Devlin wrote:
> nit: "if one does not exist" instead of "if does not exist".

Done.

https://codereview.chromium.org/796453003/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:34: // Manages a set
of DeclarativeUserScript objects for programmatically
On 2014/12/12 20:02:45, Devlin wrote:
> This seems more like a comment of the full class
(DeclarativeUserScriptManager),
> rather than the map.  Maybe just "A map of DeclarativeUserScriptMasters for
ids;
> each master is lazily initialized."?

Sounds better. Updated.

[Devlin, 2014-12-12 20:36:10]

lgtm

https://codereview.chromium.org/796453003/diff/160001/chrome/browser/extensio...
File chrome/browser/extensions/declarative_user_script_manager.h (right):

https://codereview.chromium.org/796453003/diff/160001/chrome/browser/extensio...
chrome/browser/extensions/declarative_user_script_manager.h:31:
std::map<std::string, linked_ptr<DeclarativeUserScriptMaster>>;
I'm assuming this weird indentation is the work of git cl format?  If not, I
think it should be
using UserScriptMasterMap =
    std::map<std::string, linked_ptr<...>>;

If so, then I guess it's fine (<<grumble about clang format's
inconsistencies...>>)

[Xi Han, 2014-12-12 20:41:18]

On 2014/12/12 20:36:10, Devlin wrote:
> lgtm
> 
>
https://codereview.chromium.org/796453003/diff/160001/chrome/browser/extensio...
> File chrome/browser/extensions/declarative_user_script_manager.h (right):
> 
>
https://codereview.chromium.org/796453003/diff/160001/chrome/browser/extensio...
> chrome/browser/extensions/declarative_user_script_manager.h:31:
> std::map<std::string, linked_ptr<DeclarativeUserScriptMaster>>;
> I'm assuming this weird indentation is the work of git cl format?  If not, I
> think it should be
> using UserScriptMasterMap =
>     std::map<std::string, linked_ptr<...>>;
> 
> If so, then I guess it's fine (<<grumble about clang format's
> inconsistencies...>>)

Yes, it looks weird, updated.

[Xi Han, 2014-12-12 20:41:26]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-12-12 20:42:02]

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

[commit-bot: I haz the power, 2014-12-12 21:53:54]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2014-12-12 22:01:03]

Patchset 6 (id:??) landed as
https://crrev.com/6d9b43a5c431b04947bdc135b179166e55199efa
Cr-Commit-Position: refs/heads/master@{#308171}