Garbage Collect the Storage directory on next profile start after an extension uninstall.

Garbage Collect the Storage directory on next profile start after an extension uninstall.

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

[awong, 2012-12-05 01:01:23]

Hey Charlie, Matt,

There are still some nits in this CL (called out in 2-3 TODOs) but I think the
basic structure is correct.

Would you two be willing to take a look and let me know if it looks sane?

Matt, in particular, I would like feedback on how I'm using extension prefs and
how I'm delaying the install.

-Albert

[Charlie Reis, 2012-12-05 02:25:40]

I should probably do a closer look later, but it seems reasonable to me at a
high level, after all that we've discussed.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2289: // extension uninstall?
I'm guessing it should not be syncable.  Each instance should decide for itself
when to set this dirty bit.  If you hear about an extension uninstall from sync,
won't that set your local dirty bit as usual?

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:878: // TODO(ajwong): We must
bounce to UI thread and do correct cancelation.
Not quite sure what this means.  Also, long line below.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2313: // If installs are delayed,
then disable.
I guess I'm a bit confused by the names here.  "OnExtensionInstalled" sounds
past tense, so perhaps we're just postponing the enabling?

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:828: // Helper to search remove an
storage directories for extensions with
nit: "search remove an" typo

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:836: void
set_delay_all_installs(bool value) { delay_all_installs_ = value; }
Please add a comment to these saying why they're needed.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:837: bool delay_all_installs()
const { return delay_all_installs_; }
nit: Maybe should_delay_all_installs()?
(It sounds like a command at the moment.)

https://codereview.chromium.org/11419307/diff/1001/content/browser/browser_co...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/11419307/diff/1001/content/browser/browser_co...
content/browser/browser_context.cc:99:
GetStoragePartitionMap(browser_context)->AsyncObliterate(site, on_gc_required);
Long line.

https://codereview.chromium.org/11419307/diff/1001/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11419307/diff/1001/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:353: // The deletion is
asynchronously because after (1) completes, calling code can
nit: asynchronous?

https://codereview.chromium.org/11419307/diff/1001/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:513: // Never try to obliterate
the browser context root. Die hard.
Yippie ki-yay.  :)

[Matt Perry, 2012-12-05 19:55:33]

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2315:
pending_extension_installs_.push_back(
Can you explain why reusing the FinishDelayedInstallation bits doesn't work?
It's going to be very confusing that we have delayed installs and pending
installs, and both features are disjoint.

[awong, 2012-12-05 21:01:57]

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2315:
pending_extension_installs_.push_back(
On 2012/12/05 19:55:33, Matt Perry wrote:
> Can you explain why reusing the FinishDelayedInstallation bits doesn't work?
> It's going to be very confusing that we have delayed installs and pending
> installs, and both features are disjoint.

I probably misunderstood the code.  What I saw was
pending_extension_updates_.Insert() is gated by ShouldDelayExtensionUpdate(id,
wait_for_idle).

If I were to modify the ShouldDelayExtensionUpdate() call so it returned true
and added the extension id to the pending_extension_updates_ when
delay_all_installs_ is set, I run into these issues:

  (1) I don't know which elements in pending_extension_updates_ needs to passed
to FinishDelayedInstallation when the GC is finished.
  (2) I've lost the wait_for_idle flag information so I can't just blindly retry
them all.
  (3) The code calling extension_prefs_->OnExtensionInstalled() and
extension_prefs_->SetAllowFileAccess() is skipped.


The crux is FinishDelayedInstall() didn't look like a delayed execution of
OnExtensionInstalled()'s code.  Instead, it looks like something handling
extension updates with a variety of attributes I couldn't understand...

[Matt Perry, 2012-12-05 21:22:08]

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2315:
pending_extension_installs_.push_back(
On 2012/12/05 21:01:57, awong wrote:
> On 2012/12/05 19:55:33, Matt Perry wrote:
> > Can you explain why reusing the FinishDelayedInstallation bits doesn't work?
> > It's going to be very confusing that we have delayed installs and pending
> > installs, and both features are disjoint.
> 
> I probably misunderstood the code.  What I saw was
> pending_extension_updates_.Insert() is gated by ShouldDelayExtensionUpdate(id,
> wait_for_idle).
> 
> If I were to modify the ShouldDelayExtensionUpdate() call so it returned true
> and added the extension id to the pending_extension_updates_ when
> delay_all_installs_ is set, I run into these issues:
> 
>   (1) I don't know which elements in pending_extension_updates_ needs to
passed
> to FinishDelayedInstallation when the GC is finished.
>   (2) I've lost the wait_for_idle flag information so I can't just blindly
retry
> them all.
>   (3) The code calling extension_prefs_->OnExtensionInstalled() and
> extension_prefs_->SetAllowFileAccess() is skipped.
> 
> 
> The crux is FinishDelayedInstall() didn't look like a delayed execution of
> OnExtensionInstalled()'s code.  Instead, it looks like something handling
> extension updates with a variety of attributes I couldn't understand...

I think the main question is, exactly what do you need to defer? Currently
you're deferring all of OnExtensionInstalled. Maybe you only need to defer the
final call to FinishInstallation at the end of this method.

If it's the latter, then you should be able to share some code with
pending_extension_updates_. Of course, you'll need to change it a bit since that
code assumes it's only dealing with installs delayed while waiting for extension
idle.

[awong, 2012-12-08 01:45:23]

I think this is ready for a real review now.  PTAL

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2289: // extension uninstall?
On 2012/12/05 02:25:40, creis wrote:
> I'm guessing it should not be syncable.  Each instance should decide for
itself
> when to set this dirty bit.  If you hear about an extension uninstall from
sync,
> won't that set your local dirty bit as usual?

Agreed. Removed comment.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:878: // TODO(ajwong): We must
bounce to UI thread and do correct cancelation.
On 2012/12/05 02:25:40, creis wrote:
> Not quite sure what this means.  Also, long line below.

Resolved it. I hadn't fixed the API to make sure the callback was run back on
the UI thread, and to cancel if the ExtensionService gets deleted.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2313: // If installs are delayed,
then disable.
On 2012/12/05 02:25:40, creis wrote:
> I guess I'm a bit confused by the names here.  "OnExtensionInstalled" sounds
> past tense, so perhaps we're just postponing the enabling?

Yeah, it confused me too.  OnExtensionInstalled() isn't really enabling. 
There's another state for that. This is where things get inserted into the
actual recording keeping structures (prefs, internal lists, etc) so until this
is finished the extension really isn't "installed."  But given Matt's comments
below, I had to change this logic anyways.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2315:
pending_extension_installs_.push_back(
On 2012/12/05 21:22:08, Matt Perry wrote:
> On 2012/12/05 21:01:57, awong wrote:
> > On 2012/12/05 19:55:33, Matt Perry wrote:
> > > Can you explain why reusing the FinishDelayedInstallation bits doesn't
work?
> > > It's going to be very confusing that we have delayed installs and pending
> > > installs, and both features are disjoint.
> > 
> > I probably misunderstood the code.  What I saw was
> > pending_extension_updates_.Insert() is gated by
ShouldDelayExtensionUpdate(id,
> > wait_for_idle).
> > 
> > If I were to modify the ShouldDelayExtensionUpdate() call so it returned
true
> > and added the extension id to the pending_extension_updates_ when
> > delay_all_installs_ is set, I run into these issues:
> > 
> >   (1) I don't know which elements in pending_extension_updates_ needs to
> passed
> > to FinishDelayedInstallation when the GC is finished.
> >   (2) I've lost the wait_for_idle flag information so I can't just blindly
> retry
> > them all.
> >   (3) The code calling extension_prefs_->OnExtensionInstalled() and
> > extension_prefs_->SetAllowFileAccess() is skipped.
> > 
> > 
> > The crux is FinishDelayedInstall() didn't look like a delayed execution of
> > OnExtensionInstalled()'s code.  Instead, it looks like something handling
> > extension updates with a variety of attributes I couldn't understand...
> 
> I think the main question is, exactly what do you need to defer? Currently
> you're deferring all of OnExtensionInstalled. Maybe you only need to defer the
> final call to FinishInstallation at the end of this method.
> 
> If it's the latter, then you should be able to share some code with
> pending_extension_updates_. Of course, you'll need to change it a bit since
that
> code assumes it's only dealing with installs delayed while waiting for
extension
> idle.

I've redone this a bit. Let me know what you think.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:828: // Helper to search remove an
storage directories for extensions with
On 2012/12/05 02:25:40, creis wrote:
> nit: "search remove an" typo

Done.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:836: void
set_delay_all_installs(bool value) { delay_all_installs_ = value; }
On 2012/12/05 02:25:40, creis wrote:
> Please add a comment to these saying why they're needed.

Done.

https://codereview.chromium.org/11419307/diff/1001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.h:837: bool delay_all_installs()
const { return delay_all_installs_; }
On 2012/12/05 02:25:40, creis wrote:
> nit: Maybe should_delay_all_installs()?
> (It sounds like a command at the moment.)

How about installs_delayed()?  should_delay_all_installs is very long...

[Matt Perry, 2012-12-08 01:55:33]

Thanks for taking the time to untangle everything and do this the right way. It
looks much better now.

LGTM with nits for extensions/ stuff.

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2498:
extension_prefs_->SetIdleInstallInfo(extension, initial_state,
naming nit: this should probably be SetDelayedInstallInfo now

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:3145: void
ExtensionService::OnNeedsToGarbageCollectedIsolatedStorage() {
Collected -> Collect

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:901: ExtensionSet
delayed_for_idle_updates_;
nit: delayed_updates_for_idle_
(otherwise it sounds like you're referring to "idle updates")

[awong, 2012-12-08 02:11:09]

Comments addressed...causing a moderate amount of rename churn. :P

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2498:
extension_prefs_->SetIdleInstallInfo(extension, initial_state,
On 2012/12/08 01:55:33, Matt Perry wrote:
> naming nit: this should probably be SetDelayedInstallInfo now

Right...meant to do that. Done now.  Also renamed related functions/tests/etc.

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:3145: void
ExtensionService::OnNeedsToGarbageCollectedIsolatedStorage() {
On 2012/12/08 01:55:33, Matt Perry wrote:
> Collected -> Collect

Done.

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11419307/diff/19017/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:901: ExtensionSet
delayed_for_idle_updates_;
On 2012/12/08 01:55:33, Matt Perry wrote:
> nit: delayed_updates_for_idle_
> (otherwise it sounds like you're referring to "idle updates")

Done.

[Matt Perry, 2012-12-10 18:33:27]

lgtm

[Charlie Reis, 2012-12-10 23:22:28]

Looks pretty good.  Just a few nits and minor questions below.

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:391: // Removes any idle install
information we have for the given |extension_id|.
nit: s/idle/delayed/, here and below

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (left):

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2475: // Transfer ownership of
|extension|.
Is this comment worth keeping?

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2554: // overridden, don't reset
the value.
This seems somewhat unrelated to the rest of the CL.  Is it intentional or maybe
the result of a merge conflict?

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:3118:
GetSiteForExtensionId((*it)->id()))->GetPath());
Why is GetPath() needed?  GetSiteForURL doesn't include a path, does it?

(If there's a corner case, just leave it for now and file a bug on me.)

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:331: //
BrowserContext::GetStoragePartitionForSite()
nit: End with period.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:334: } else {
nit: Don't need the else, since we return early above.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:355: // The deletion is
asynchronously because after (1) completes, calling code can
nit: asynchronous
Also, "calling code can safely continue..."

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:363: void BlockingGarbageCollect(
What does the "Blocking" part of the name refer to?  Just the moving of
directories into the trash directory?  Unclear, given how much emphasis there is
on the asynchronous behavior.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:521: CHECK(domain_root !=
browser_context_->GetPath()) << site;
Can we easily check that domain_root is within browser_context_->GetPath() as
well?  Maybe that's overkill, but I wouldn't want something like "../" to be
able to cause larger parts of the user's filesystem to be deleted.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:48: // Examines the on disk storage
and removes any entires that are not listed in
nit: on-disk

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:49: // the active_paths, or in use
by current entires in the storage partition.
nit: entries

Can you also mention that the done closure is called when it finishes?

[awong, 2012-12-11 01:18:31]

comments addressed. PTAL

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:391: // Removes any idle install
information we have for the given |extension_id|.
On 2012/12/10 23:22:28, creis wrote:
> nit: s/idle/delayed/, here and below

Done.

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (left):

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2475: // Transfer ownership of
|extension|.
On 2012/12/10 23:22:28, creis wrote:
> Is this comment worth keeping?

I don't believe so.

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2554: // overridden, don't reset
the value.
On 2012/12/10 23:22:28, creis wrote:
> This seems somewhat unrelated to the rest of the CL.  Is it intentional or
maybe
> the result of a merge conflict?

This is actually intentional. Without this change, delayed installs for unpacked
extensions would not be granted file access.

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:3118:
GetSiteForExtensionId((*it)->id()))->GetPath());
On 2012/12/10 23:22:28, creis wrote:
> Why is GetPath() needed?  GetSiteForURL doesn't include a path, does it?
> 
> (If there's a corner case, just leave it for now and file a bug on me.)

It's actually getting the path on the StoragePartition.  The parenthesis are
getting pretty ugly.  Do you think it'd be worth splitting into a local
variable?

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/11419307/diff/14013/chrome/browser/extensions...
chrome/browser/extensions/extension_service.h:331: //
BrowserContext::GetStoragePartitionForSite()
On 2012/12/10 23:22:28, creis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:334: } else {
On 2012/12/10 23:22:28, creis wrote:
> nit: Don't need the else, since we return early above.

Done.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:355: // The deletion is
asynchronously because after (1) completes, calling code can
On 2012/12/10 23:22:28, creis wrote:
> nit: asynchronous
> Also, "calling code can safely continue..."

Done.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:363: void BlockingGarbageCollect(
On 2012/12/10 23:22:28, creis wrote:
> What does the "Blocking" part of the name refer to?  Just the moving of
> directories into the trash directory?  Unclear, given how much emphasis there
is
> on the asynchronous behavior.

Yeah, it's referring to the move to trash.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:363: void BlockingGarbageCollect(
On 2012/12/10 23:22:28, creis wrote:
> What does the "Blocking" part of the name refer to?  Just the moving of
> directories into the trash directory?  Unclear, given how much emphasis there
is
> on the asynchronous behavior.

Yep. It's referring to the moves and the enumeration. I added a comment.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:521: CHECK(domain_root !=
browser_context_->GetPath()) << site;
On 2012/12/10 23:22:28, creis wrote:
> Can we easily check that domain_root is within browser_context_->GetPath() as
> well?  Maybe that's overkill, but I wouldn't want something like "../" to be
> able to cause larger parts of the user's filesystem to be deleted.

Made this check more robust.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:48: // Examines the on disk storage
and removes any entires that are not listed in
On 2012/12/10 23:22:28, creis wrote:
> nit: on-disk

Done.

https://codereview.chromium.org/11419307/diff/14013/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:49: // the active_paths, or in use
by current entires in the storage partition.
On 2012/12/10 23:22:28, creis wrote:
> nit: entries
> 
> Can you also mention that the done closure is called when it finishes?

Done.

[Charlie Reis, 2012-12-11 02:15:56]

Thanks for the extra FS checks!  LGTM with nits.

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.cc:128: const char
kPrefSuggestedPageOrdinal[] = "suggested_page_ordinal";
Merge conflict here.

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:391: // Removes any delay install
information we have for the given
nit: delayed

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:398: // Returns the ExtensionInfo
from the prefs for idle install information for
nit: idle -> delayed

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:403: // Returns information about
all the extensions that have pending idle
pending idle -> delayed

https://codereview.chromium.org/11419307/diff/39002/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11419307/diff/39002/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:52: // The |done| closure is
executing on the calling thread when garbage
nit: executed

https://codereview.chromium.org/11419307/diff/39002/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:53: // collecition is complete.
nit: collection

[awong, 2012-12-11 03:34:23]

comments addressed.  CQing.

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.cc:128: const char
kPrefSuggestedPageOrdinal[] = "suggested_page_ordinal";
On 2012/12/11 02:15:56, creis wrote:
> Merge conflict here.

Done.

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.h (right):

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:391: // Removes any delay install
information we have for the given
On 2012/12/11 02:15:56, creis wrote:
> nit: delayed

Done.

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:398: // Returns the ExtensionInfo
from the prefs for idle install information for
On 2012/12/11 02:15:56, creis wrote:
> nit: idle -> delayed

Done.

https://codereview.chromium.org/11419307/diff/39002/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.h:403: // Returns information about
all the extensions that have pending idle
On 2012/12/11 02:15:56, creis wrote:
> pending idle -> delayed

Done.

https://codereview.chromium.org/11419307/diff/39002/content/browser/storage_p...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11419307/diff/39002/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:52: // The |done| closure is
executing on the calling thread when garbage
On 2012/12/11 02:15:56, creis wrote:
> nit: executed

Done.

https://codereview.chromium.org/11419307/diff/39002/content/browser/storage_p...
content/browser/storage_partition_impl_map.h:53: // collecition is complete.
On 2012/12/11 02:15:56, creis wrote:
> nit: collection

Done.

[commit-bot: I haz the power, 2012-12-11 03:36:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/11419307/38004

[commit-bot: I haz the power, 2012-12-11 09:33:45]

Change committed as 172278