Allow manifest handlers to declare keys they depend on that must be parsed before them.

Allow manifest handlers to declare keys they depend on that must be parsed before them.

Also fixes linked_ptr usage for multi-key handlers.

BUG=159265
TBR=ben@chromium.org


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

[Yoyo Zhou, 2013-02-01 22:16:42]

Note that nobody has any dependent keys yet, although some could be added right
now, given that some pending CLs (e.g. icons) make manifest handlers for things
that are currently being read out of Extension by other manifest handlers.

https://codereview.chromium.org/12091115/diff/1/chrome/common/extensions/mani...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest_handler.cc:129: // circular dependencies. Drop
them, and log a warning.
I'm not sure this is the best way to deal with this. Here are some possible
alternatives:

(0) Log the error message in some less ugly way.
(1) NOTREACHED. Not sure this is worth crashing the browser over, but
profile_dependency_manager does take this approach. Hard to unit test.
(2) Reject a manifest handler when it Registers if it tries to create a circular
dependency. This forces us to SortManifestHandlers at every registration (though
some heuristics could reduce this).

[Matt Perry, 2013-02-01 23:22:37]

https://codereview.chromium.org/12091115/diff/5002/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/5002/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.cc:97: unsorted_handlers.begin();
I think this would be easier to follow if you made it a recursive helper
function. I'm thinking something like:
- Put every handler into a set of unvisited_handlers.
- Process(string handler_name, set& unvisited_handlers, list& sorted_handlers),
defined as:
  if handler_name not in unvisited_handlers: return
  remove handler_name from unvisited_handlers
  for each dependency of handler_name:
    Process(dependency, unvisited_handler)
  append handler_name to sorted_handlers

Then you just keep calling Process() on the first entry in unvisited_handlers
until it's empty. Once you have sorted_handlers, you don't need a priority map
anymore.

I haven't thought about how to handle circular dependencies this way, but maybe
that could be a separate step.

https://codereview.chromium.org/12091115/diff/5002/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_override_unittest.cc
(right):

https://codereview.chromium.org/12091115/diff/5002/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_override_unittest.cc:17:
keys::kChromeURLOverrides,  // TODO(yoz): ???
What does this mean?

[Yoyo Zhou, 2013-02-02 00:32:22]

https://codereview.chromium.org/12091115/diff/5002/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/5002/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.cc:97: unsorted_handlers.begin();
On 2013/02/01 23:22:37, Matt Perry wrote:
> I think this would be easier to follow if you made it a recursive helper
> function. I'm thinking something like:
> - Put every handler into a set of unvisited_handlers.
> - Process(string handler_name, set& unvisited_handlers, list&
sorted_handlers),
> defined as:
>   if handler_name not in unvisited_handlers: return
>   remove handler_name from unvisited_handlers
>   for each dependency of handler_name:
>     Process(dependency, unvisited_handler)
>   append handler_name to sorted_handlers
> 
> Then you just keep calling Process() on the first entry in unvisited_handlers
> until it's empty. Once you have sorted_handlers, you don't need a priority map
> anymore.
> 
> I haven't thought about how to handle circular dependencies this way, but
maybe
> that could be a separate step.

I don't think I find it easier to follow that way.

To handle circular dependencies, you'd also need a third state (unvisited,
visited, sorted), so that if you come to something that's visited but not sorted
yet, you know you've reached your own ancestor.

The reason I didn't make a sorted_handlers list is because you iterate over the
keys in ParseExtension, so you need to get the keys for each handler (so that
you can iterate over the keys in the right order and not call a handler twice).
While you could do this, it's just another auxiliary data structure similar to
priority map, so it doesn't simplify things.

https://codereview.chromium.org/12091115/diff/5002/chrome/common/extensions/m...
File
chrome/common/extensions/manifest_tests/extension_manifests_override_unittest.cc
(right):

https://codereview.chromium.org/12091115/diff/5002/chrome/common/extensions/m...
chrome/common/extensions/manifest_tests/extension_manifests_override_unittest.cc:17:
keys::kChromeURLOverrides,  // TODO(yoz): ???
On 2013/02/01 23:22:37, Matt Perry wrote:
> What does this mean?

It means this only works if there is a namespace alias declared in one of the
included header files, which seems wrong, and I was going to find that and fix
it, which I have now done.

[Matt Perry, 2013-02-02 00:58:58]

lgtm

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.cc:107: // Nonexistent prerequisite;
ignore it.
Shouldn't this be an error?

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.cc:132: << "circular dependencies:";
This should be a fatal error, no?

[Yoyo Zhou, 2013-02-02 02:26:45]

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.cc:107: // Nonexistent prerequisite;
ignore it.
On 2013/02/02 00:58:58, Matt Perry wrote:
> Shouldn't this be an error?

A fatal error? That's probably okay. My concern is that this is more likely to
happen in a renderer/utility process, where not every key is registered, and a
developer might not notice the crash (e.g. are there browser tests that test the
utility process stuff in extension_file_util?).

But I can see that this would prevent developer errors.

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.cc:132: << "circular dependencies:";
On 2013/02/02 00:58:58, Matt Perry wrote:
> This should be a fatal error, no?

Sure.

I deleted the circular dependencies unit test, but let me know if there's a
better thing to do (e.g. leave it DISABLED with a comment, or somehow not
crash).

[Matt Perry, 2013-02-02 02:29:02]

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.cc:132: << "circular dependencies:";
On 2013/02/02 02:26:45, Yoyo Zhou wrote:
> On 2013/02/02 00:58:58, Matt Perry wrote:
> > This should be a fatal error, no?
> 
> Sure.
> 
> I deleted the circular dependencies unit test, but let me know if there's a
> better thing to do (e.g. leave it DISABLED with a comment, or somehow not
> crash).

I thought there was some way to make unit tests treat CHECKs as test failures
instead of crashes. If you can't find it, I'm OK with leaving the test out.

[Yoyo Zhou, 2013-02-02 02:38:38]

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/5006/chrome/common/extensions/m...
chrome/common/extensions/manifest_handler.cc:132: << "circular dependencies:";
On 2013/02/02 02:29:03, Matt Perry wrote:
> On 2013/02/02 02:26:45, Yoyo Zhou wrote:
> > On 2013/02/02 00:58:58, Matt Perry wrote:
> > > This should be a fatal error, no?
> > 
> > Sure.
> > 
> > I deleted the circular dependencies unit test, but let me know if there's a
> > better thing to do (e.g. leave it DISABLED with a comment, or somehow not
> > crash).
> 
> I thought there was some way to make unit tests treat CHECKs as test failures
> instead of crashes. If you can't find it, I'm OK with leaving the test out.

I found this:
https://code.google.com/p/chromium/issues/detail?id=56448
which indicates that the available methods like SetLogMessageHandler are frowned
upon to some degree. I'll leave it out then.

[commit-bot: I haz the power, 2013-02-02 02:46:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12091115/46002

[commit-bot: I haz the power, 2013-02-02 02:46:47]

Presubmit check for 12091115-46002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
You might be calling functions intended only for testing from
production code.  It is OK to ignore this warning if you know what
you are doing, as the heuristics used to detect the situation are
not perfect.  The commit queue will not block on this warning.
Email joi@chromium.org if you have questions.
  chrome/common/extensions/manifest_handler.cc:25
    void ClearForTesting(); \
  chrome/common/extensions/manifest_handler.cc:172
    g_registry.Get().ClearForTesting();

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/browser/speech/extension_api
    chrome/browser/intents

Presubmit checks took 6.0s to calculate.

[Yoyo Zhou, 2013-02-02 02:52:47]

TBR=ben for uninteresting refactoring changes

[commit-bot: I haz the power, 2013-02-02 02:55:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12091115/46002

[Devlin, 2013-02-02 21:41:37]

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler.cc:37: // Manifest handlers with
circular dependencies are unregistered.
nit: comment should be updated - while it's true that the dependencies aren't
registered, we should probably mention it'll result in a CHECK(). :)

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler_unittest.cc (right):

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler_unittest.cc:24: void
Record(std::string name) {
const & string?

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler_unittest.cc:33: bool
ParsedBefore(std::string name_before, std::string name_after) {
const & strings?

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler_unittest.cc:49:
std::vector<std::string> parsed_names_;
If we only care about relative ordering, this might be easier/cleaner as a
std::map<std::string, int>:

void Record(name):
  parsed_names_[name] = parsed_names_.size();

bool ParsedBefore(before, after):
  return parsed_names_[before] < parsed_names_[after];

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler_unittest.cc:55: const
std::vector<std::string> prereqs,
const & vector?

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler_unittest.cc:69: std::string name_;
Trailing underscore and use suggests these should be private.

[Yoyo Zhou, 2013-02-04 20:24:30]

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler.cc (right):

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler.cc:37: // Manifest handlers with
circular dependencies are unregistered.
On 2013/02/02 21:41:38, D Cronin wrote:
> nit: comment should be updated - while it's true that the dependencies aren't
> registered, we should probably mention it'll result in a CHECK(). :)

Done.

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
File chrome/common/extensions/manifest_handler_unittest.cc (right):

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler_unittest.cc:49:
std::vector<std::string> parsed_names_;
On 2013/02/02 21:41:38, D Cronin wrote:
> If we only care about relative ordering, this might be easier/cleaner as a
> std::map<std::string, int>:
> 
> void Record(name):
>   parsed_names_[name] = parsed_names_.size();
> 
> bool ParsedBefore(before, after):
>   return parsed_names_[before] < parsed_names_[after];

If only STL had a sensible behavior for when the key isn't in the map, that
would work. It is possibly to rely on the default value for int not being in the
map values, but I don't find it any better.

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler_unittest.cc:55: const
std::vector<std::string> prereqs,
On 2013/02/02 21:41:38, D Cronin wrote:
> const & vector?

How did I miss that.

https://codereview.chromium.org/12091115/diff/50001/chrome/common/extensions/...
chrome/common/extensions/manifest_handler_unittest.cc:69: std::string name_;
On 2013/02/02 21:41:38, D Cronin wrote:
> Trailing underscore and use suggests these should be private.

Made protected so the subclasses can refer to them.

[commit-bot: I haz the power, 2013-02-04 20:26:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12091115/48003

[commit-bot: I haz the power, 2013-02-04 21:27:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-02-04 23:16:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/12091115/48003

[commit-bot: I haz the power, 2013-02-05 00:26:29]

Change committed as 180575