app_shell: Extract extension runtime data from ExtensionService

app_shell: Extract extension runtime data from ExtensionService

This allows ExtensionHost to use the data when there is no ExtensionService, such as when running app_shell.

BUG=332982
TEST=Added unit test, existing extensions browser tests
TBR=msw@chromium.org for mechanical change to chrome/browser/ui/views/

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

[James Cook, 2014-01-17 18:14:43]

kalman, PTAL

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/extension...
File chrome/browser/extensions/extension_host.cc (right):

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/extension...
chrome/browser/extensions/extension_host.cc:327:
ExtensionSystem::GetForBrowserContext(browser_context_)
This block is the primary motivation for this change. With one more change to
ExtensionHost I'll be able to eliminate its dependence on ExtensionService.

[not at google - send to devlin, 2014-01-18 01:09:04]

https://codereview.chromium.org/131743021/diff/30001/apps/shell/shell_extensi...
File apps/shell/shell_extension_system.h (right):

https://codereview.chromium.org/131743021/diff/30001/apps/shell/shell_extensi...
apps/shell/shell_extension_system.h:44: virtual RuntimeData* runtime_data()
OVERRIDE;
I'm curious; what is the general rationale for items on ExtensionSystem vs
BrowserContextKeyedServices?

My personal preference is to have dependencies be explicit, but the Chrome
pattern seems to be to pull everything into BCKS - which is fine, but... this
file implies otherwise. At least, it seems arbitrary. Why is ExtensionRegistry a
BCKS while RuntimeData on ExtensionSystem?

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/automatio...
File chrome/browser/automation/automation_provider_observers.cc (right):

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/automatio...
chrome/browser/automation/automation_provider_observers.cc:576:
extensions::ExtensionSystem* system,
given this file isn't extensions related I'd expect this to be called
extension_system... sorry :(

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

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1585:
system_->runtime_data()->Erase(extension);
I would expect RuntimeData to be able to manage cleanup itself by listening to
the UNLOAD notification.

https://codereview.chromium.org/131743021/diff/30001/extensions/browser/runti...
File extensions/browser/runtime_data.h (right):

https://codereview.chromium.org/131743021/diff/30001/extensions/browser/runti...
extensions/browser/runtime_data.h:24: void Erase(const Extension* extension);
see comment in extension service. RuntimeData should be able to manage this
itself right?

https://codereview.chromium.org/131743021/diff/30001/extensions/browser/runti...
extensions/browser/runtime_data.h:63: typedef std::map<std::string,
ExtensionData> ExtensionDataMap;
As it currently is a struct is a bit overkill for this. You could use a bitmask
(or... a std::bitset might work nicely?)

Of course, if this needs to be augmented later for non-bool data then it'd need
to be a struct again; but then, you'd probably not want to be copying it all
over the place either.

[James Cook, 2014-01-18 02:01:18]

kalman, please take another look

https://codereview.chromium.org/131743021/diff/30001/apps/shell/shell_extensi...
File apps/shell/shell_extension_system.h (right):

https://codereview.chromium.org/131743021/diff/30001/apps/shell/shell_extensi...
apps/shell/shell_extension_system.h:44: virtual RuntimeData* runtime_data()
OVERRIDE;
On 2014/01/18 01:09:04, kalman wrote:
> I'm curious; what is the general rationale for items on ExtensionSystem vs
> BrowserContextKeyedServices?
> 
> My personal preference is to have dependencies be explicit, but the Chrome
> pattern seems to be to pull everything into BCKS - which is fine, but... this
> file implies otherwise. At least, it seems arbitrary. Why is ExtensionRegistry
a
> BCKS while RuntimeData on ExtensionSystem?

I don't think there is a rationale. I like BCKS because it makes the services
feel more independent. In particular, I find it forces the services to be more
relaxed about their creation order -- it's hard for them to get tied into "this
object must be constructed right here, not a line before or a line after".  For
example, ExtensionSystem initialization is a mess.  But Yoyo felt that
RuntimeData was too simple to merit a BCKS and it seemed reasonable to us to
hang it here.

Do you want me to change it to a BCKS?

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/automatio...
File chrome/browser/automation/automation_provider_observers.cc (right):

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/automatio...
chrome/browser/automation/automation_provider_observers.cc:576:
extensions::ExtensionSystem* system,
On 2014/01/18 01:09:04, kalman wrote:
> given this file isn't extensions related I'd expect this to be called
> extension_system... sorry :(

No worries. Done.

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

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1585:
system_->runtime_data()->Erase(extension);
On 2014/01/18 01:09:04, kalman wrote:
> I would expect RuntimeData to be able to manage cleanup itself by listening to
> the UNLOAD notification.

I didn't do that because NOTIFICATION_EXTENSION_UNLOADED is a chrome concept
that hasn't yet migrated to src/extensions. It's also not clear to me that doing
it on UNLOAD is equivalent to what's happening here -- for example, the
notification is not always sent below.  And it would change RuntimeData from a
simple wrapper around maps into something that needs a NotificationObserver,
NotificationRegistrar, etc.  And I have trouble reasoning about when things need
to get cleaned up relative to other listeners to the UNLOADED notification.

Should I change it to listen to the notification?

https://codereview.chromium.org/131743021/diff/30001/extensions/browser/runti...
File extensions/browser/runtime_data.h (right):

https://codereview.chromium.org/131743021/diff/30001/extensions/browser/runti...
extensions/browser/runtime_data.h:63: typedef std::map<std::string,
ExtensionData> ExtensionDataMap;
Changed to a bitmask.

[not at google - send to devlin, 2014-01-21 18:47:46]

https://codereview.chromium.org/131743021/diff/30001/apps/shell/shell_extensi...
File apps/shell/shell_extension_system.h (right):

https://codereview.chromium.org/131743021/diff/30001/apps/shell/shell_extensi...
apps/shell/shell_extension_system.h:44: virtual RuntimeData* runtime_data()
OVERRIDE;
On 2014/01/18 02:01:18, James Cook wrote:
> On 2014/01/18 01:09:04, kalman wrote:
> > I'm curious; what is the general rationale for items on ExtensionSystem vs
> > BrowserContextKeyedServices?
> > 
> > My personal preference is to have dependencies be explicit, but the Chrome
> > pattern seems to be to pull everything into BCKS - which is fine, but...
this
> > file implies otherwise. At least, it seems arbitrary. Why is
ExtensionRegistry
> a
> > BCKS while RuntimeData on ExtensionSystem?
> 
> I don't think there is a rationale. I like BCKS because it makes the services
> feel more independent. In particular, I find it forces the services to be more
> relaxed about their creation order -- it's hard for them to get tied into
"this
> object must be constructed right here, not a line before or a line after". 
For
> example, ExtensionSystem initialization is a mess.  But Yoyo felt that
> RuntimeData was too simple to merit a BCKS and it seemed reasonable to us to
> hang it here.
> 
> Do you want me to change it to a BCKS?

Hanging off ExtensionSystem SGTM.

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

https://codereview.chromium.org/131743021/diff/30001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1585:
system_->runtime_data()->Erase(extension);
On 2014/01/18 02:01:18, James Cook wrote:
> On 2014/01/18 01:09:04, kalman wrote:
> > I would expect RuntimeData to be able to manage cleanup itself by listening
to
> > the UNLOAD notification.
> 
> I didn't do that because NOTIFICATION_EXTENSION_UNLOADED is a chrome concept
> that hasn't yet migrated to src/extensions. It's also not clear to me that
doing
> it on UNLOAD is equivalent to what's happening here -- for example, the
> notification is not always sent below.  And it would change RuntimeData from a
> simple wrapper around maps into something that needs a NotificationObserver,
> NotificationRegistrar, etc.  And I have trouble reasoning about when things
need
> to get cleaned up relative to other listeners to the UNLOADED notification.
> 
> Should I change it to listen to the notification?

Ah right. I think the solution to this would be to add some sensible way to
observe changes to ExtensionRegistry. It's a bit tricky since I think there's
too much subtlety in the way that ExtensionService to, say, have
ExtensionRegistry implement "AddExtensionUnloadedObserver" itself. However I
could at least see an interim solution where ExtensionService fires the event.

I.e. when ExtensionService fires the EXTENSION_UNLOADED event it also calls
registry->OnUnloaded(Extension*), then add a method like
registry->{Add,Remove}UnloadObserver(Callback).

I feel kinda bad suggesting you should shave this yak now, but it's better than
trying to manually maintain this state across ExtensionService and RuntimeData.
Better to just bite the bullet and implement a more generic observation
mechanism now (and start using it).

You should probably check with appshell-team before actually following up on
this :)

https://codereview.chromium.org/131743021/diff/140001/extensions/browser/runt...
File extensions/browser/runtime_data.h (right):

https://codereview.chromium.org/131743021/diff/140001/extensions/browser/runt...
extensions/browser/runtime_data.h:58: bool GetFlag(const Extension* extension,
RuntimeFlag flag) const;
nit: HasFlag?

[James Cook, 2014-01-21 23:20:39]

kalman, please take another look.  I think this approach works OK.

https://codereview.chromium.org/131743021/diff/210001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/131743021/diff/210001/extensions/browser/exte...
extensions/browser/extension_registry.h:79: void EnableExtension(const
scoped_refptr<const Extension>& extension);
By adding this function and the one below I was able to get rid of
RemoveEnabled() and RemovedDisabled(). This gets us closer to having each
extension only appear in one set at a time.

[not at google - send to devlin, 2014-01-21 23:58:58]

responding to comments. feel free to ping me instead.

https://codereview.chromium.org/131743021/diff/210001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/131743021/diff/210001/extensions/browser/exte...
extensions/browser/extension_registry.h:75: void UnloadExtension(const
scoped_refptr<const Extension>& extension);
I kind of preferred the way it was... making this class really dumb and not
assuming intent of the caller. So like, this method in particular, the fact that
it "removes from enabled and disabled sets" seems a bit magical (same below with
Enable/DisableExtension though to a lesser extent).

I had imagined something like AddObserver/RemoveObserver then some number of
Trigger methods... TriggerOnUnloaded, etc.

[James Cook, 2014-01-22 01:00:47]

kalman, please take another look

[not at google - send to devlin, 2014-01-22 16:17:42]

I guess this change is more subtle than expected! See my next huge collection of
comments. It's basically some superficial changes + one giant question which has
nothing to do with you, but, I guess you're used to such things working on app
shell...

(sorry sorry sorry :( :( )

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/extension_host.cc (right):

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/extension_host.cc:329:
->SetBackgroundPageReady(extension_, true);
(I would just put this on the same line as runtime_data() but whatever you
prefer)

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1594:
registry_->TriggerOnUnloaded(extension);
Should this go in NotifyExtensionUnloaded? That would actually change the
behaviour here, since it would only trigger the unloaded event on !was_disabled,
which given the old code is probably the right thing to do.

Hm, maybe I'm missing something here because it looks like you did explicitly
make that change; reverting it would also mean going basically back to the old
code just without that runtime_data reference.

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry.h:60: // The usual observer interface.
[musing]

One nice thing about explicit dependencies is that it would let you control the
const-ness of this object. Namely, you could make ExtensionSystem only expose a
const version of ExtensionRegistry through a getter, such that nothing could
actually modify it (neither add/remove extensions nor fire events). Then,
ExtensionSystem would pass the only non-const registry to ExtensionService.

Of course... you'd need to make the observer list mutable then. Hrm.

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry.h:66: void TriggerOnUnloaded(const
Extension* extension);
It looks like the notification also has an "unload reason". Eventually that will
need to be here too, I presume (might as well follow up with it).

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry.h:70: void TriggerOnDisabled(const
Extension* extension);
It's interesting; there's no disabled notification, and yet we need a disabled
event here. That's notable because in the entire life of the extensions
platform... nobody has needed it. "Unloading" is the idiom. And yet
ExtensionService does do something special on disabled; it clears that
background page bit.

I wonder why it does it on disabled rather than unloaded? It seems kinda fishy
to me.

In fact, looking at this a bit more (reading the code; it may be more subtle
than this) ExtensionService (and thus now ExtensionRegistry) on disable:
- clears the "has background page" bit
- fires the unloaded event if it hasn't already

The odd part that "clear background page" stuff. That happens right before the
notification (NotifyExtensionUnloaded). The only other place that is called from
is UnloadExtension itself, which clears the whole of extension runtime data.

Basically: I find the behaviour of ExtensionService fishy. I can't imagine why
it doesn't just clear all of the runtime data. That would let you simplify this
code.

Perhaps there are answers in the revision history?

I would suggest that you just submit this change as-is and worry about
simplifying it later, but I'm not sure; it seems dangerous having the disabled
event in the codebase at all (see other comments about big warnings).

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
File extensions/browser/extension_registry_observer.h (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry_observer.h:20: virtual void
OnExtensionUnloaded(const Extension* extension) = 0;
I can imagine this interface getting a few times bigger, once all of the
extension lifetime events are also present here. Point being: should they be

virtual void OED(e) {};

rather than

virtual void OED(e) = 0;

? Chances are classes only care about unloading anyway.

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry_observer.h:24: virtual void
OnExtensionDisabled(const Extension* extension) = 0;
see comment in ExtensionRegistry about a giant warning :)

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/runt...
File extensions/browser/runtime_data.cc (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/runt...
extensions/browser/runtime_data.cc:14: if (registry_)
when is there no registry? tests? can we fix that?

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/runt...
File extensions/browser/runtime_data.h (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/runt...
extensions/browser/runtime_data.h:49: virtual void OnExtensionDisabled(const
Extension* extension) OVERRIDE;
make these private

[James Cook, 2014-01-22 17:41:52]

Some comments from the bus. I'm getting kind of frustrated with this patch. I'm
at +450 -150 lines to eliminate 1 line from ExtensionHost and I feel like I'm
going in circles.

I'm in SF today. Let me know about my questions below and I'll update the patch
- maybe we can chat in person then?

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/extension_host.cc (right):

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/extension_host.cc:329:
->SetBackgroundPageReady(extension_, true);
On 2014/01/22 16:17:43, kalman wrote:
> (I would just put this on the same line as runtime_data() but whatever you
> prefer)

This is the output of clang-format, so I prefer it.

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1594:
registry_->TriggerOnUnloaded(extension);
On 2014/01/22 16:17:43, kalman wrote:
> Should this go in NotifyExtensionUnloaded? That would actually change the
> behaviour here, since it would only trigger the unloaded event on
!was_disabled,
> which given the old code is probably the right thing to do.
> 
> Hm, maybe I'm missing something here because it looks like you did explicitly
> make that change; reverting it would also mean going basically back to the old
> code just without that runtime_data reference.

The code I wrote has identical behavior to the old code with respect to clearing
the runtime data and calling NotifyExtensionUnloaded. Are you saying I should
change when runtime data is cleared?

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry.h:66: void TriggerOnUnloaded(const
Extension* extension);
On 2014/01/22 16:17:43, kalman wrote:
> It looks like the notification also has an "unload reason". Eventually that
will
> need to be here too, I presume (might as well follow up with it).

I don't understand what you mean by "might as well follow up with it".  Does
that mean I can do it in a later CL if I need the reason code, or does that mean
I should follow up and add it in this patch?

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry.h:70: void TriggerOnDisabled(const
Extension* extension);
> I wonder why it does it on disabled rather than unloaded? It seems kinda fishy
> to me.

I agree it looks odd.

> The odd part that "clear background page" stuff. That happens right before the
> notification (NotifyExtensionUnloaded). The only other place that is called
from
> is UnloadExtension itself, which clears the whole of extension runtime data.
> 
> Basically: I find the behaviour of ExtensionService fishy. I can't imagine why
> it doesn't just clear all of the runtime data. That would let you simplify
this
> code.
> 
> Perhaps there are answers in the revision history?
> 
> I would suggest that you just submit this change as-is and worry about
> simplifying it later, but I'm not sure; it seems dangerous having the disabled
> event in the codebase at all (see other comments about big warnings).

I looked at the revision history before writing this. Clearing the background
ready flag was apparently done for purely theoretical reasons, see
https://code.google.com/p/chromium/issues/detail?id=232435

I have no idea what happens if we don't do it.

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
File extensions/browser/extension_registry_observer.h (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry_observer.h:20: virtual void
OnExtensionUnloaded(const Extension* extension) = 0;
On 2014/01/22 16:17:43, kalman wrote:
> I can imagine this interface getting a few times bigger, once all of the
> extension lifetime events are also present here. Point being: should they be
> 
> virtual void OED(e) {};
> 
> rather than
> 
> virtual void OED(e) = 0;
> 
> ? Chances are classes only care about unloading anyway.

Sure, I'll do that.  I've seen both patterns for interfaces in Chrome.

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/runt...
File extensions/browser/runtime_data.cc (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/runt...
extensions/browser/runtime_data.cc:14: if (registry_)
On 2014/01/22 16:17:43, kalman wrote:
> when is there no registry? tests? can we fix that?

See the comment in the header - it was for tests. I will change it.

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/runt...
File extensions/browser/runtime_data.h (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/runt...
extensions/browser/runtime_data.h:49: virtual void OnExtensionDisabled(const
Extension* extension) OVERRIDE;
On 2014/01/22 16:17:43, kalman wrote:
> make these private

OK.

[not at google - send to devlin, 2014-01-22 17:49:06]

Yes let's chat. I think this CL is fine but it has exposed potential bugs in
extension service (namely inconsitencies between disable and unload, when it
seems like mostly they should be the same). How about - lgtm once that last
round of minor comments are addressed - and up to you whether we address the
disablement issue now or later.

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/131743021/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:1594:
registry_->TriggerOnUnloaded(extension);
On 2014/01/22 17:41:52, James Cook wrote:
> On 2014/01/22 16:17:43, kalman wrote:
> > Should this go in NotifyExtensionUnloaded? That would actually change the
> > behaviour here, since it would only trigger the unloaded event on
> !was_disabled,
> > which given the old code is probably the right thing to do.
> > 
> > Hm, maybe I'm missing something here because it looks like you did
explicitly
> > make that change; reverting it would also mean going basically back to the
old
> > code just without that runtime_data reference.
> 
> The code I wrote has identical behavior to the old code with respect to
clearing
> the runtime data and calling NotifyExtensionUnloaded. Are you saying I should
> change when runtime data is cleared?

Yeah I'm questioning whether the old code is correct.

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
File extensions/browser/extension_registry.h (right):

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry.h:66: void TriggerOnUnloaded(const
Extension* extension);
On 2014/01/22 17:41:52, James Cook wrote:
> On 2014/01/22 16:17:43, kalman wrote:
> > It looks like the notification also has an "unload reason". Eventually that
> will
> > need to be here too, I presume (might as well follow up with it).
> 
> I don't understand what you mean by "might as well follow up with it".  Does
> that mean I can do it in a later CL if I need the reason code, or does that
mean
> I should follow up and add it in this patch?

Up to you, I don't feel one way or the other, just pointing it out for either
now or the future.

https://codereview.chromium.org/131743021/diff/270001/extensions/browser/exte...
extensions/browser/extension_registry.h:70: void TriggerOnDisabled(const
Extension* extension);
On 2014/01/22 17:41:52, James Cook wrote:
> > I wonder why it does it on disabled rather than unloaded? It seems kinda
fishy
> > to me.
> 
> I agree it looks odd.
> 
> > The odd part that "clear background page" stuff. That happens right before
the
> > notification (NotifyExtensionUnloaded). The only other place that is called
> from
> > is UnloadExtension itself, which clears the whole of extension runtime data.
> > 
> > Basically: I find the behaviour of ExtensionService fishy. I can't imagine
why
> > it doesn't just clear all of the runtime data. That would let you simplify
> this
> > code.
> > 
> > Perhaps there are answers in the revision history?
> > 
> > I would suggest that you just submit this change as-is and worry about
> > simplifying it later, but I'm not sure; it seems dangerous having the
disabled
> > event in the codebase at all (see other comments about big warnings).
> 
> I looked at the revision history before writing this. Clearing the background
> ready flag was apparently done for purely theoretical reasons, see
> https://code.google.com/p/chromium/issues/detail?id=232435
> 
> I have no idea what happens if we don't do it.

I meant that it seems like we should clear all of the runtime data, not just the
background ready flag.

[James Cook, 2014-01-22 21:03:57]

kalman, does ExtensionService look OK with this patch?

[not at google - send to devlin, 2014-01-22 21:08:15]

lgtm

https://codereview.chromium.org/131743021/diff/440002/extensions/browser/runt...
File extensions/browser/runtime_data_unittest.cc (right):

https://codereview.chromium.org/131743021/diff/440002/extensions/browser/runt...
extensions/browser/runtime_data_unittest.cc:44: RuntimeData
runtime_data(&registry);
you could do this in a SetUp method if you wanted...

[commit-bot: I haz the power, 2014-01-22 21:43:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/131743021/830001

[commit-bot: I haz the power, 2014-01-22 23:47:26]

Change committed as 246449