Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
671873004:
Migrates legacy packaged app data when it's upgraded to a platform app (Closed)
|
Created:
6 years, 1 month ago by ryanackley Modified:
5 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, dgrogan, jam, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, jsbell+idb_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrates legacy packaged app data when it's upgraded to a platform app
Currently, data stored by a legacy packaged app in the HTML5 filesystem
and IndexedDB is lost when it gets migrated to the new type of Chrome App
(internally referred to as "platform apps"). This is because Chrome stores
extension and legacy packaged app data in the general storage partition and
it stores platform app data in a it's own isolated storage partition.
When a legacy packaged app is upgraded to the new type of app, a new
isolated storage partition is created but the data gets left behind
in the general storage partition.
This change fixes this by performing a migration of the data to the new
storage partition at the time of the upgrade. It only migrates IndexedDB
and HTML5 filesystem because these are the only permanent data storage
options available to platform apps. Platform apps are prohibited from
using DOMStorage and WebSQL. Data stored by chrome.storage APIs is
separate from the storage partitions.
BUG=302577
Committed: https://crrev.com/48bedbd2773df847e5dbf1c01fb409da4636d542
Cr-Commit-Position: refs/heads/master@{#313394}
Patch Set 1 #Patch Set 2 : Fixes unit tests #Patch Set 3 : Rebase on ToT #Patch Set 4 : Empty patch trying to unwedge try server #
Total comments: 12
Patch Set 5 : Changes based on review comments #
Total comments: 9
Patch Set 6 : More changes for review #
Total comments: 1
Patch Set 7 : Address review comments and adds tests #
Total comments: 7
Patch Set 8 : Changes based on review comments #Patch Set 9 : Rebase #
Total comments: 7
Patch Set 10 : Addresses review comments #
Total comments: 16
Patch Set 11 : Addresses cmumford comments #
Total comments: 7
Patch Set 12 : Minor changes #Patch Set 13 : incorporate tzik's feedback #Patch Set 14 : Puts back some code that was mistakenly deleted in a merge #Patch Set 15 : rebase #Patch Set 16 : trybot complaints #Messages
Total messages: 67 (12 generated)
dgrogan@chromium.org changed reviewers: + dgrogan@chromium.org
Drive by comments on patchset 1: 1. There are some trivial conflicts applying your patch to ToT. 2. indexed_db_quota_client_unittest.cc depends on GetFilePathForTesting, which your patch removes. Build the target content_unittests to see the problem.
dgrogan@chromium.org changed reviewers: - dgrogan@chromium.org
ryanackley@gmail.com changed reviewers: + benwells@chromium.org
Before looking I started a bunch of try jobs. I don't think you can run these yourself yet, but you might be able to. You should be able to see the results as they appear under your patch set. Based on the comments of dgrogan@ you have some problems which the try jobs should show.
On 2014/10/30 06:09:13, benwells wrote: > Before looking I started a bunch of try jobs. I don't think you can run these > yourself yet, but you might be able to. You should be able to see the results as > they appear under your patch set. > > Based on the comments of dgrogan@ you have some problems which the try jobs > should show. There you go, patch failure. You might need to rebase your patch on ToT and upload again. Might as well try building content_unittests as well. Once you got all that working, try to start some more try jobs (if you can see the Try more bots link, click on that).
On 2014/10/30 06:10:28, benwells wrote: > On 2014/10/30 06:09:13, benwells wrote: > > Before looking I started a bunch of try jobs. I don't think you can run these > > yourself yet, but you might be able to. You should be able to see the results > as > > they appear under your patch set. > > > > Based on the comments of dgrogan@ you have some problems which the try jobs > > should show. > > There you go, patch failure. You might need to rebase your patch on ToT and > upload again. Might as well try building content_unittests as well. Once you got > all that working, try to start some more try jobs (if you can see the Try more > bots link, click on that). I was able to run Trybot once but I get a purple lozenge indicating an exception with no other info when I try now. I've verified that everything builds locally so I'm thinking It's related to permissions/authentication.
Overall looks pretty good, but I don't know much about the storage stuff so we'll need to get someone who knows about that to review. Please run git cl format on your code before uploading it, if you haven't already. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:148: GURL extension_url = Extension::GetBaseURLFromExtensionId(extension->id()); Could you add a check here that this is happening on the correct thread? https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:178: while (extension_url != (origin = enumerator->Next()) && !origin.is_empty()){ Nit: you need a space before the curly (here and elsewhere). But you also don't need these curies at all. Nit: having an assignment in a conditional is generally yuck. I'd redo this to avoid that. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1799: bool ExtensionService::NeedsMigration(const Extension* extension){ Could all the new logic you're adding go in a new file / class - Extension? It would be on ExtensionService as a field. This file is kind of big and ugly and this is adding more dependencies out onto other bits of code. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1799: bool ExtensionService::NeedsMigration(const Extension* extension){ Can you add a check here that this is happening on the UI thread? https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1828: BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE, Could you use the blocking pool instead? See https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... and callers for how to use it. The blocking pool is a replacement of the FILE thread. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1855: if (NeedsMigration(extension)){ The patter chrome tends to use here is more like: if (blah) { blab(); return; } otherblab(); }
ryanackley@gmail.com changed reviewers: + tzik@chromium.org
https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:148: GURL extension_url = Extension::GetBaseURLFromExtensionId(extension->id()); On 2014/11/03 00:08:41, benwells wrote: > Could you add a check here that this is happening on the correct thread? Acknowledged. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:178: while (extension_url != (origin = enumerator->Next()) && !origin.is_empty()){ On 2014/11/03 00:08:41, benwells wrote: > Nit: you need a space before the curly (here and elsewhere). But you also don't > need these curies at all. > > Nit: having an assignment in a conditional is generally yuck. I'd redo this to > avoid that. Acknowledged. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1799: bool ExtensionService::NeedsMigration(const Extension* extension){ On 2014/11/03 00:08:42, benwells wrote: > Can you add a check here that this is happening on the UI thread? Acknowledged. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1799: bool ExtensionService::NeedsMigration(const Extension* extension){ On 2014/11/03 00:08:41, benwells wrote: > Could all the new logic you're adding go in a new file / class - Extension? It > would be on ExtensionService as a field. This file is kind of big and ugly and > this is adding more dependencies out onto other bits of code. Acknowledged. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1828: BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE, On 2014/11/03 00:08:41, benwells wrote: > Could you use the blocking pool instead? See > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > and callers for how to use it. > > The blocking pool is a replacement of the FILE thread. Acknowledged. https://codereview.chromium.org/671873004/diff/60001/chrome/browser/extension... chrome/browser/extensions/extension_service.cc:1855: if (NeedsMigration(extension)){ On 2014/11/03 00:08:42, benwells wrote: > The patter chrome tends to use here is more like: > > if (blah) { > blab(); > return; > } > > otherblab(); > } Acknowledged.
I just added tzik as a reviewer since he is in the OWNERS file in the fileapi directory and based on recent commits seems to be active in the areas I am using for the migration. Should I ping him first or is it simply enough to add him as a reviewer on the issue?
On 2014/11/04 22:55:13, ryanackley wrote: > I just added tzik as a reviewer since he is in the OWNERS file in the fileapi > directory and based on recent commits seems to be active in the areas I am using > for the migration. Should I ping him first or is it simply enough to add him as > a reviewer on the issue? He would have just got that question as an email :)
Data migration is nice to have. Though I'm afraid it's hard to do it safely. If you want to copy whole directory. We should ensure no in-flight file operation exists on the storage. And all backing LevelDBs should be closed during the migration.
On 2014/11/06 14:18:25, tzik wrote: > Data migration is nice to have. Though I'm afraid it's hard to do it safely. > > If you want to copy whole directory. We should ensure no in-flight file > operation exists on the storage. And all backing LevelDBs should be closed > during the migration. @tzik, thanks for reviewing. As part of the extension upgrade process, the old extension's background page and tabs are shutdown. Because of domain restrictions, is it safe to assume that the extension would be the only client of its respective storage? If so, would it be enough to make sure the copy is happening after all processes belonging to the old extension are completely closed? My only concern with trying to determine the in-flight status of the file system and/or the connection status of the indexeddb is that there doesn't appear to be an API accessible from the current entry point I'm using (StoragePartition). Do you have any pointers or suggestions on how to access this info from the ExtensionService without adding a lot of plumbing?
Some more nits. Like tzik@ said, migration is good so thanks for the doing this! I'll let tzik@ look at the logic of the migration. Could we get some tests? https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/app_data_migrator.cc (right): https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/app_data_migrator.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Nit: "Copyright 2014..." https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/app_data_migrator.cc:115: // juggle the enabled status of the new and old extensions so we can get Uber nit: Capital J and a full stop at the end of the comment. https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/app_data_migrator.cc:121: bool oldWasDisabled = registry_->AddEnabled(extension); Could you add a comment explaining what is happening here? It is strange that we're enabling / disabling extensions here. https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/app_data_migrator.h (right): https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/app_data_migrator.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Nit: "Copyright 2014..." https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/app_data_migrator.h:16: class AppDataMigrator { Nit: class level comment please. https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/app_data_migrator.h:29: Profile* profile_; DISALLOW_COPY_AND_ASSIGN(AppDataMigrator) https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/app_data_migrator.h:32: } // namespace extensions Nit: two spaces before "//" https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/app_data_migrator.h:33: #endif // CHROME_BROWSER_EXTENSIONS_APP_DATA_MIGRATOR_H_ Nit: blank line here. https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_service.h (right): https://codereview.chromium.org/671873004/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_service.h:722: scoped_ptr<extensions::AppDataMigrator> app_data_migrator_; Nit: add a comment explaining briefly what it is for (in particular what cases it is used in).
@tzik, the latest patch set should address your concerns. I moved the indexeddb migration logic to the IndexedDBContextImpl. It now closes all open connections and performs the copy on the indexed db context's task runner. For the file system copy, it performs the copy on the sandbox delegate's sequenced task runner. This should ensure that it will happen after any in-flight file system operations since these should happen in FIFO order on the same task runner. @benwells, the latest patch should address all your comments except tests. I was waiting for more definitive approval on the approach I took before I start writing tests. Looking forward to your next set of comments. Thanks for your patience :)
On 2014/11/13 02:05:41, ryanackley wrote: > @tzik, the latest patch set should address your concerns. I moved the indexeddb > migration logic to the IndexedDBContextImpl. It now closes all open connections > and performs the copy on the indexed db context's task runner. For the file > system copy, it performs the copy on the sandbox delegate's sequenced task > runner. This should ensure that it will happen after any in-flight file system > operations since these should happen in FIFO order on the same task runner. > > @benwells, the latest patch should address all your comments except tests. I was > waiting for more definitive approval on the approach I took before I start > writing tests. > > Looking forward to your next set of comments. Thanks for your patience :) Sorry for slow response. There are still a few things to do it safely. Let me respond about them in the crbug.com.
https://codereview.chromium.org/671873004/diff/100001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.cc (right): https://codereview.chromium.org/671873004/diff/100001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:36: origin, type, false); The backing database may be left open here. Could you add a method and call it to close the database? That should be on SandboxFileSystemBackendDelegate and relayed to ObfuscatedFileUtil to erase corresponding |directories_| entry.
On 2014/11/20 06:56:21, tzik wrote: > https://codereview.chromium.org/671873004/diff/100001/chrome/browser/extensio... > File chrome/browser/extensions/app_data_migrator.cc (right): > > https://codereview.chromium.org/671873004/diff/100001/chrome/browser/extensio... > chrome/browser/extensions/app_data_migrator.cc:36: origin, type, false); > The backing database may be left open here. > Could you add a method and call it to close the database? > That should be on SandboxFileSystemBackendDelegate and relayed to > ObfuscatedFileUtil to erase corresponding |directories_| entry. Ok done. Also added some tests. Right now, the tests only address file system migration. Unfortunately, I wrote an elaborate test for the indexeddb migration but the dependencies I needed violated checkdeps and I couldn't add them to the changeset. Therefore, I removed that test. Not sure how to get around that problem.
tzik@chromium.org changed reviewers: + jsbell@chromium.org
Looks good overall. Adding jsbell@ for IDB part. https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.cc (right): https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:69: } nit: insert empty line here? https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:79: FileSystemContext* fs_context = current_partition->GetFileSystemContext(); It is not clear that these FileSystemContexts outlive the task posted below. Could you pass them as scoped_refptr and pick the delegate in the posted task? https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:97: } ditto https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:112: base::Closure migrateFs = base::Bind(&MigrateFileSystem, s/migrateFs/migrate_fs/? https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:153: bool oldWasDisabled = registry_->AddEnabled(extension); s/oldWasDisabled/old_was_disabled/? https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.h (right): https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: newer code doesn't need "(c)" here. https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator_unittest.cc (right): https://codereview.chromium.org/671873004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator_unittest.cc:3: // found in the LICENSE file. insert an empty line here?
Sorry for taking so long. I'll go through it again in detail but on first glance it looks like it's missing the ".blob" directories which may not exist, but would be siblings to the ".leveldb" directories. These are created/maintained by the indexed_db_backing_store.cc file. The code was recently added, and the context doesn't account for them (related bugs: crbug.com/435631 crbug.com/435627 crbug.com/409436)
On 2014/12/05 01:12:57, jsbell wrote: > Sorry for taking so long. I'll go through it again in detail but on first glance > it looks like it's missing the ".blob" directories which may not exist, but > would be siblings to the ".leveldb" directories. > > These are created/maintained by the indexed_db_backing_store.cc file. The code > was recently added, and the context doesn't account for them (related bugs: > crbug.com/435631 crbug.com/435627 crbug.com/409436) FYI, crbug.com/435627 is being tackled over in crrev.com/779303003 which depends on crrev.com/779433006 - the latter will expose a list of directories, so basing this CL on that one is probably the best approach.
On 2014/12/08 21:49:00, jsbell wrote: > FYI, crbug.com/435627 is being tackled over in crrev.com/779303003 which depends > on crrev.com/779433006 - the latter will expose a list of directories, so basing > this CL on that one is probably the best approach. The important bit landed - the indexed_db_context_impl now implements a GetStoragePaths() method that will give you all the directories that you need to (recursively) copy for an origin.
https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... File content/browser/indexed_db/indexed_db_context_impl.cc (right): https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:333: GetFilePathForTesting(origin_url.possibly_invalid_spec()); The "ForTesting" methods should only be called from unit tests. This should instead call GetStoragePaths() and iterate over the vector of returned paths, copying each so that both the .leveldb and .blob directories are copied. https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:334: base::FilePath dest_directory = This is basically just computing <context>/IndexedDB/<origin>.indexeddb.leveldb then throwing away everything but <context>/IndexedDB - I think it would be cleaner to expose that as a method on the context. Then this whole thing basically becomes: base::FilePath dest_dir = dest_context->GetContextPath(); // name? if (!base::PathExists(dest_dir)) base::CreateDirectory(dest_dir); for (const auto& path : GetFilePaths()) base::CopyDirectory(path, dest_dir); ... although you may want to do an early loop over the paths and exit early if one exists, which is what this CL's line 337 does (although it doesn't return a failure code?)
On 2014/12/12 17:46:09, jsbell wrote: > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > File content/browser/indexed_db/indexed_db_context_impl.cc (right): > > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > content/browser/indexed_db/indexed_db_context_impl.cc:333: > GetFilePathForTesting(origin_url.possibly_invalid_spec()); > The "ForTesting" methods should only be called from unit tests. > > This should instead call GetStoragePaths() and iterate over the vector of > returned paths, copying each so that both the .leveldb and .blob directories are > copied. > > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > content/browser/indexed_db/indexed_db_context_impl.cc:334: base::FilePath > dest_directory = > This is basically just computing <context>/IndexedDB/<origin>.indexeddb.leveldb > then throwing away everything but <context>/IndexedDB - I think it would be > cleaner to expose that as a method on the context. Then this whole thing > basically becomes: > > base::FilePath dest_dir = dest_context->GetContextPath(); // name? > if (!base::PathExists(dest_dir)) > base::CreateDirectory(dest_dir); > for (const auto& path : GetFilePaths()) > base::CopyDirectory(path, dest_dir); > > ... although you may want to do an early loop over the paths and exit early if > one exists, which is what this CL's line 337 does (although it doesn't return a > failure code?) Also, GetBlobPath() returns paths that *might* exist. The Blob directory is only lazily created when a Blob is added, and probably won't exist.
cmumford@chromium.org changed reviewers: + cmumford@chromium.org
https://codereview.chromium.org/671873004/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.cc (right): https://codereview.chromium.org/671873004/diff/160001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: no "(c)" https://codereview.chromium.org/671873004/diff/160001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:67: CHECK(old_indexed_db_context->TaskRunner()->RunsTasksOnCurrentThread()); Question: In Chrome there are 640 DCHECK's for RunsTasksOnCurrentThread, but only 8 which are CHECK's. Should this be a debug check like the rest? https://codereview.chromium.org/671873004/diff/160001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator_unittest.cc (right): https://codereview.chromium.org/671873004/diff/160001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator_unittest.cc:178: std::string actual = buffer.get(); Need to add: #include <string> https://codereview.chromium.org/671873004/diff/160001/chrome/chrome_browser_e... File chrome/chrome_browser_extensions.gypi (right): https://codereview.chromium.org/671873004/diff/160001/chrome/chrome_browser_e... chrome/chrome_browser_extensions.gypi:504: 'browser/extensions/app_data_migrator.h', nit: .h after the .c?
https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... File content/browser/indexed_db/indexed_db_context_impl.cc (right): https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:334: base::FilePath dest_directory = On 2014/12/12 17:46:09, jsbell wrote: > This is basically just computing <context>/IndexedDB/<origin>.indexeddb.leveldb > then throwing away everything but <context>/IndexedDB - I think it would be > cleaner to expose that as a method on the context. Then this whole thing > basically becomes: > > base::FilePath dest_dir = dest_context->GetContextPath(); // name? > if (!base::PathExists(dest_dir)) > base::CreateDirectory(dest_dir); > for (const auto& path : GetFilePaths()) > base::CopyDirectory(path, dest_dir); > > ... although you may want to do an early loop over the paths and exit early if > one exists, which is what this CL's line 337 does (although it doesn't return a > failure code?) Should I expose the methods I need on IndexedDBContext or just do a static cast to IndexedDBContextImpl on dest_context? If I'm using IndexedDBContextImpl, there is already a data_path() method that will give me the <context>/IndexedDB path. With regard to the failure condition, I was thinking of just changing it to a DCHECK instead of a silent return. For my purposes, there isn't much I can do with an error. Most of the time extension upgrades happen automatically in the background. If the migration were to fail, I couldn't easily bubble this up to the user to action it. i.e. "To continue please delete your c:\users\bob\AppData\Local\Chrome\Storage\ext\sasdjkasdhkjhasdjkd\IndexedDB directory. Oh and btw this folder may be hidden depending on your shell settings." It's more along the lines that if this fails, something is broken with the state of our data model which would signify a bug or extraordinary condition.
On 2014/12/12 19:03:05, ryanackley wrote: > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > File content/browser/indexed_db/indexed_db_context_impl.cc (right): > > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > content/browser/indexed_db/indexed_db_context_impl.cc:334: base::FilePath > dest_directory = > On 2014/12/12 17:46:09, jsbell wrote: > > This is basically just computing > <context>/IndexedDB/<origin>.indexeddb.leveldb > > then throwing away everything but <context>/IndexedDB - I think it would be > > cleaner to expose that as a method on the context. Then this whole thing > > basically becomes: > > > > base::FilePath dest_dir = dest_context->GetContextPath(); // name? > > if (!base::PathExists(dest_dir)) > > base::CreateDirectory(dest_dir); > > for (const auto& path : GetFilePaths()) > > base::CopyDirectory(path, dest_dir); > > > > ... although you may want to do an early loop over the paths and exit early if > > one exists, which is what this CL's line 337 does (although it doesn't return > a > > failure code?) > > Should I expose the methods I need on IndexedDBContext or just do a static cast > to IndexedDBContextImpl on dest_context? If I'm using IndexedDBContextImpl, > there is already a data_path() method that will give me the <context>/IndexedDB > path. > > With regard to the failure condition, I was thinking of just changing it to a > DCHECK instead of a silent return. For my purposes, there isn't much I can do > with an error. Most of the time extension upgrades happen automatically in the > background. If the migration were to fail, I couldn't easily bubble this up to > the user to action it. i.e. "To continue please delete your > c:\users\bob\AppData\Local\Chrome\Storage\ext\sasdjkasdhkjhasdjkd\IndexedDB > directory. Oh and btw this folder may be hidden depending on your shell > settings." It's more along the lines that if this fails, something is broken > with the state of our data model which would signify a bug or extraordinary > condition. A crash or forced quit can occur while a migration is in progress right? If we can't continue/restart then we should at least not get stuck in a bad state.
On 2014/12/12 19:06:34, cmumford wrote: > On 2014/12/12 19:03:05, ryanackley wrote: > > > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > > File content/browser/indexed_db/indexed_db_context_impl.cc (right): > > > > > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > > content/browser/indexed_db/indexed_db_context_impl.cc:334: base::FilePath > > dest_directory = > > On 2014/12/12 17:46:09, jsbell wrote: > > > This is basically just computing > > <context>/IndexedDB/<origin>.indexeddb.leveldb > > > then throwing away everything but <context>/IndexedDB - I think it would be > > > cleaner to expose that as a method on the context. Then this whole thing > > > basically becomes: > > > > > > base::FilePath dest_dir = dest_context->GetContextPath(); // name? > > > if (!base::PathExists(dest_dir)) > > > base::CreateDirectory(dest_dir); > > > for (const auto& path : GetFilePaths()) > > > base::CopyDirectory(path, dest_dir); > > > > > > ... although you may want to do an early loop over the paths and exit early > if > > > one exists, which is what this CL's line 337 does (although it doesn't > return > > a > > > failure code?) > > > > Should I expose the methods I need on IndexedDBContext or just do a static > cast > > to IndexedDBContextImpl on dest_context? If I'm using IndexedDBContextImpl, > > there is already a data_path() method that will give me the > <context>/IndexedDB > > path. > > > > With regard to the failure condition, I was thinking of just changing it to a > > DCHECK instead of a silent return. For my purposes, there isn't much I can do > > with an error. Most of the time extension upgrades happen automatically in the > > background. If the migration were to fail, I couldn't easily bubble this up to > > the user to action it. i.e. "To continue please delete your > > c:\users\bob\AppData\Local\Chrome\Storage\ext\sasdjkasdhkjhasdjkd\IndexedDB > > directory. Oh and btw this folder may be hidden depending on your shell > > settings." It's more along the lines that if this fails, something is broken > > with the state of our data model which would signify a bug or extraordinary > > condition. > > A crash or forced quit can occur while a migration is in progress right? If we > can't continue/restart then we should at least not get stuck in a bad state. Does it seem reasonable to delete any existing directories then?
On 2014/12/12 19:14:36, ryanackley wrote: > On 2014/12/12 19:06:34, cmumford wrote: > > On 2014/12/12 19:03:05, ryanackley wrote: > > > > > > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > > > File content/browser/indexed_db/indexed_db_context_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/671873004/diff/160001/content/browser/indexed... > > > content/browser/indexed_db/indexed_db_context_impl.cc:334: base::FilePath > > > dest_directory = > > > On 2014/12/12 17:46:09, jsbell wrote: > > > > This is basically just computing > > > <context>/IndexedDB/<origin>.indexeddb.leveldb > > > > then throwing away everything but <context>/IndexedDB - I think it would > be > > > > cleaner to expose that as a method on the context. Then this whole thing > > > > basically becomes: > > > > > > > > base::FilePath dest_dir = dest_context->GetContextPath(); // name? > > > > if (!base::PathExists(dest_dir)) > > > > base::CreateDirectory(dest_dir); > > > > for (const auto& path : GetFilePaths()) > > > > base::CopyDirectory(path, dest_dir); > > > > > > > > ... although you may want to do an early loop over the paths and exit > early > > if > > > > one exists, which is what this CL's line 337 does (although it doesn't > > return > > > a > > > > failure code?) > > > > > > Should I expose the methods I need on IndexedDBContext or just do a static > > cast > > > to IndexedDBContextImpl on dest_context? If I'm using IndexedDBContextImpl, > > > there is already a data_path() method that will give me the > > <context>/IndexedDB > > > path. > > > > > > With regard to the failure condition, I was thinking of just changing it to > a > > > DCHECK instead of a silent return. For my purposes, there isn't much I can > do > > > with an error. Most of the time extension upgrades happen automatically in > the > > > background. If the migration were to fail, I couldn't easily bubble this up > to > > > the user to action it. i.e. "To continue please delete your > > > c:\users\bob\AppData\Local\Chrome\Storage\ext\sasdjkasdhkjhasdjkd\IndexedDB > > > directory. Oh and btw this folder may be hidden depending on your shell > > > settings." It's more along the lines that if this fails, something is > broken > > > with the state of our data model which would signify a bug or extraordinary > > > condition. > > > > A crash or forced quit can occur while a migration is in progress right? If we > > can't continue/restart then we should at least not get stuck in a bad state. > > Does it seem reasonable to delete any existing directories then? I think deleting existing destination directories and restarting the migration sounds like a reasonable approach.
bump. In case nobody noticed, I uploaded a new changeset prior to the holidays. I haven't gotten any feedback. Hopefully, you guys have some time to look at it now that the holidays have passed.
One question and the rest are just nit's on my part. Looking pretty good. Do you think it's worth the effort to write a test to make sure you can recover from a failed migration? With billion Chrome users we know it's gonna happen. https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.cc (right): https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:103: // Retrieve both IndexedDB contexts in preparation for migration. Nit: I'm not sure this context (and the one above) adds much. https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:158: if (!old_was_disabled) { No need for braces when scoping a single line of code. Nit: Consider removing the "!" - maybe the code will be a bit more readable? https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:164: // Begin the actual migration flow. Nit: does this comment add much? https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.h (right): https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.h:24: bool NeedsMigration(const Extension* old, const Extension* extension); The name suggests this can be a const method - can it? Actually - can't it just be a static method? https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator_unittest.cc (right): https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator_unittest.cc:65: content::StoragePartition* default_partition_; Just to be safe you might want to initialize the raw pointers in the constructor - granted SetUp initializes them. Your call. https://codereview.chromium.org/671873004/diff/180001/content/browser/indexed... File content/browser/indexed_db/indexed_db_context_impl.cc (right): https://codereview.chromium.org/671873004/diff/180001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:339: CHECK(dest_context_impl->data_path() != data_path()); CHECK_NE https://codereview.chromium.org/671873004/diff/180001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:346: if (base::PathExists(dest_path)) { Nit: consider just calling DeleteFile(), and ignoring the return value. Also, no need for braces around DeleteFile. https://codereview.chromium.org/671873004/diff/180001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:351: // Creates the IndexedDB directory in the destination context if it doesn't Comment adds no value - code is obvious. https://codereview.chromium.org/671873004/diff/180001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:355: base::CreateDirectory(dest_data_path); Consider just calling CreateDirectory and ignoring return value. https://codereview.chromium.org/671873004/diff/180001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:358: // Copies the storage paths to the destination context. Nit: consider dropping obvious comment. https://codereview.chromium.org/671873004/diff/180001/content/browser/indexed... content/browser/indexed_db/indexed_db_context_impl.cc:361: base::CopyDirectory(src_path, dest_data_path, true); Nit: Can you make the names congruent? src_data_path/dest_data_path? https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... File storage/browser/fileapi/obfuscated_file_util.cc (right): https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... storage/browser/fileapi/obfuscated_file_util.cc:907: DirectoryMap::iterator iter = directories_.find(key); I don't know this class very well, but looking at ObfuscatedFileUtil::DestroyDirectoryDatabase() it appears that multiple keys may match. It does a loop and this function does not. Is that required? https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... File storage/browser/fileapi/sandbox_file_system_backend_delegate.cc (right): https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... storage/browser/fileapi/sandbox_file_system_backend_delegate.cc:665: if (base::PathExists(dest_path)) { Consider disposing of the exists check and just call DeleteFile ignoring the result. https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... storage/browser/fileapi/sandbox_file_system_backend_delegate.cc:667: CHECK(base_path != dest_path); CHECK_NE
@cmumford Thanks for getting back to me. I really appreciate it. I'm trying to get a new patch up asap. I thought I would just touch base quickly since it may not be this week. With regard to migration failure, I don't see it as a priority. From my perspective, the current state of affairs in Chromium is a migration failure and this change will be a drastic improvement. That being said it's really up to you guys if testing that condition is a necessity. Along those lines, I would love to nail down a definitive list of requirements for getting this patch accepted. I'm not sure what is a typical turn-around for you guys to accept outside contributions but I'm starting to get a little discouraged since this has been dragging on for a couple of months now.
On 2015/01/09 17:48:36, ryanackley wrote: > @cmumford Thanks for getting back to me. I really appreciate it. I'm trying to > get a new patch up asap. I thought I would just touch base quickly since it may > not be this week. > > With regard to migration failure, I don't see it as a priority. From my > perspective, the current state of affairs in Chromium is a migration failure and > this change will be a drastic improvement. That being said it's really up to you > guys if testing that condition is a necessity. > > Along those lines, I would love to nail down a definitive list of requirements > for getting this patch accepted. I'm not sure what is a typical turn-around for > you guys to accept outside contributions but I'm starting to get a little > discouraged since this has been dragging on for a couple of months now. ryanackley@ I noticed the number of patches and comments which is why I mentioned that it was looking good - I didn't want you to get discouraged. We're mostly checking for good design and conformance with http://www.chromium.org/developers/coding-style, but there's still plenty of room for subjective opinion. If you look at my earlier reviews (and even some newer ones) you'll see several with 10 (or more) patches and 30-80 comments! It just takes time to internalize the full style guide - I'll let you know when I think I've done it. From my perspective you're basically done. Regarding the test that's often a judgement call. I don't feel strongly that one is needed which is why I phrased it as a question instead of a request. If you think this will be a test that is just busy work and never fails then I wouldn't bother. So I expect to +1 your next submission, and you will need to add an owner for content/public/browser/indexed_db_context.h. When you add them make sure to point out that it's just for that one file.
On 2015/01/06 00:20:43, ryanackley wrote: > bump. > > In case nobody noticed, I uploaded a new changeset prior to the holidays. I > haven't gotten any feedback. Hopefully, you guys have some time to look at it > now that the holidays have passed. Thanks for bumping, and sorry the process has been so slow going. Once the storage folks are happy with the approach I'll be taking a look at the integration with extensions stuff. Hopefully that will be quick as I looked at it before and it looked good. Something to note - when you upload a patch we don't get notified so make sure you hit publish and say there is a new patch set. That way we'll get an email and we'll take a look. The code review system is basically email based, if you don't publish we won't notice. That is, we don't come back and look through our reviews for things that have changed, we rely on email. Also, sometimes we just forget or things get lost. Please don't be shy of pinging and bumping (as you just did!), we won't be offended at all. Lastly, thanks again for your contributions, they really are appreciated. This is a great bug to be fixing.
https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... File storage/browser/fileapi/obfuscated_file_util.cc (right): https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... storage/browser/fileapi/obfuscated_file_util.cc:907: DirectoryMap::iterator iter = directories_.find(key); On 2015/01/06 15:30:27, cmumford wrote: > I don't know this class very well, but looking at > ObfuscatedFileUtil::DestroyDirectoryDatabase() it appears that multiple keys may > match. It does a loop and this function does not. Is that required? I think there was a recent change because this code was originally taken from that method.
On 2015/01/12 21:18:17, ryanackley wrote: > https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... > File storage/browser/fileapi/obfuscated_file_util.cc (right): > > https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... > storage/browser/fileapi/obfuscated_file_util.cc:907: DirectoryMap::iterator iter > = directories_.find(key); > On 2015/01/06 15:30:27, cmumford wrote: > > I don't know this class very well, but looking at > > ObfuscatedFileUtil::DestroyDirectoryDatabase() it appears that multiple keys > may > > match. It does a loop and this function does not. Is that required? > > I think there was a recent change because this code was originally taken from > that method. Yes, looks like it is http://crrev.com/579083004. I'm not familiar with this and think that you *may* need to do a similar thing in your function. You may want to ask nhiroki@ if this is relevant to your change.
On 2015/01/12 21:55:53, cmumford wrote: > On 2015/01/12 21:18:17, ryanackley wrote: > > > https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... > > File storage/browser/fileapi/obfuscated_file_util.cc (right): > > > > > https://codereview.chromium.org/671873004/diff/180001/storage/browser/fileapi... > > storage/browser/fileapi/obfuscated_file_util.cc:907: DirectoryMap::iterator > iter > > = directories_.find(key); > > On 2015/01/06 15:30:27, cmumford wrote: > > > I don't know this class very well, but looking at > > > ObfuscatedFileUtil::DestroyDirectoryDatabase() it appears that multiple keys > > may > > > match. It does a loop and this function does not. Is that required? > > > > I think there was a recent change because this code was originally taken from > > that method. > > Yes, looks like it is http://crrev.com/579083004. I'm not familiar with this and > think that you *may* need to do a similar thing in your function. You may want > to ask nhiroki@ if this is relevant to your change. I updated that code block in the changeset I just uploaded
lgtm
c/b/extensions lgtm. Thanks! https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.cc (right): https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Ditto with the year. https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.h (right): https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Nit: now it is 2015... https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.h:31: ExtensionRegistry* registry_; Nit: you can remove the blank line between the variable declarations. https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.h:38: } // namespace extensions Nit: blank line after namespace. https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator_unittest.cc (right): https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Ditto with the year https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator_unittest.cc:33: namespace extensions { Nit: blank line before namespace https://codereview.chromium.org/671873004/diff/200001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator_unittest.cc:271: } // namespace extensions Nit: blank line before namespace end
I defer to cmumford for indexeddb/ tzik@ - do you want to take another look?
On 2015/01/14 18:21:01, jsbell wrote: > I defer to cmumford for indexeddb/ > > tzik@ - do you want to take another look? ryanackley: benwells comments are simple or nits, so once done this change slgtm.
On 2015/01/14 23:45:04, cmumford wrote: > On 2015/01/14 18:21:01, jsbell wrote: > > I defer to cmumford for indexeddb/ > > > > tzik@ - do you want to take another look? > > ryanackley: benwells comments are simple or nits, so once done this change > slgtm. Ok, updated based on benwells feedback. Should I do a rebase on master before it's merged?
On 2015/01/16 00:20:43, ryanackley wrote: > On 2015/01/14 23:45:04, cmumford wrote: > > On 2015/01/14 18:21:01, jsbell wrote: > > > I defer to cmumford for indexeddb/ > > > > > > tzik@ - do you want to take another look? > > > > ryanackley: benwells comments are simple or nits, so once done this change > > slgtm. > > Ok, updated based on benwells feedback. Should I do a rebase on master before > it's merged? Technically a rebase is not required and if the commit queue can cherry pick w/o conflict it will land OK. However this CL has been in review so long you might want to do a rebase and run of some/all unit tests to ensure it's still valid. I'd say that if you rebase w/o conflict and tests pass then just press the Commit button and monitor results. Since it's your first (I think) I'd suggest doing this when you have a few hours to track the progress to make sure it doesn't break anything. Some of the bots run more (and slower) tests so you don't always know right away if you caused any problems.
On 2015/01/16 01:02:51, cmumford wrote: > On 2015/01/16 00:20:43, ryanackley wrote: > > On 2015/01/14 23:45:04, cmumford wrote: > > > On 2015/01/14 18:21:01, jsbell wrote: > > > > I defer to cmumford for indexeddb/ > > > > > > > > tzik@ - do you want to take another look? > > > > > > ryanackley: benwells comments are simple or nits, so once done this change > > > slgtm. > > > > Ok, updated based on benwells feedback. Should I do a rebase on master before > > it's merged? > > Technically a rebase is not required and if the commit queue can cherry pick w/o > conflict it will land OK. However this CL has been in review so long you might > want to do a rebase and run of some/all unit tests to ensure it's still valid. > I'd say that if you rebase w/o conflict and tests pass then just press the > Commit button and monitor results. > > Since it's your first (I think) I'd suggest doing this when you have a few hours > to track the progress to make sure it doesn't break anything. Some of the bots > run more (and slower) tests so you don't always know right away if you caused > any problems. I think you still need more owners - for content/ and for storage/
https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... File chrome/browser/extensions/app_data_migrator.cc (right): https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... chrome/browser/extensions/app_data_migrator.cc:112: base::Bind(&MigrateFileSystem, old_partition, current_partition, This closure may run after the profile has gone. How about: - Convert this function to AppDataMigration's method, - Add an WeakPtrFactory to AppDataMigration. - Pass its WeakPtr as this pointer, so that the task will be canceled when the instance is destroyed.
On 2015/01/16 03:22:25, benwells wrote: > On 2015/01/16 01:02:51, cmumford wrote: > > On 2015/01/16 00:20:43, ryanackley wrote: > > > On 2015/01/14 23:45:04, cmumford wrote: > > > > On 2015/01/14 18:21:01, jsbell wrote: > > > > > I defer to cmumford for indexeddb/ > > > > > > > > > > tzik@ - do you want to take another look? > > > > > > > > ryanackley: benwells comments are simple or nits, so once done this change > > > > slgtm. > > > > > > Ok, updated based on benwells feedback. Should I do a rebase on master > before > > > it's merged? > > > > Technically a rebase is not required and if the commit queue can cherry pick > w/o > > conflict it will land OK. However this CL has been in review so long you might > > want to do a rebase and run of some/all unit tests to ensure it's still valid. > > I'd say that if you rebase w/o conflict and tests pass then just press the > > Commit button and monitor results. > > > > Since it's your first (I think) I'd suggest doing this when you have a few > hours > > to track the progress to make sure it doesn't break anything. Some of the bots > > run more (and slower) tests so you don't always know right away if you caused > > any problems. > > I think you still need more owners - for content/ and for storage/ By content/, you're referring to the changes for indexeddb? I thought jsbell deferred to cmumford for that? Am I missing something?
On 2015/01/20 15:00:22, ryanackley wrote: > On 2015/01/16 03:22:25, benwells wrote: > > On 2015/01/16 01:02:51, cmumford wrote: > > > On 2015/01/16 00:20:43, ryanackley wrote: > > > > On 2015/01/14 23:45:04, cmumford wrote: > > > > > On 2015/01/14 18:21:01, jsbell wrote: > > > > > > I defer to cmumford for indexeddb/ > > > > > > > > > > > > tzik@ - do you want to take another look? > > > > > > > > > > ryanackley: benwells comments are simple or nits, so once done this > change > > > > > slgtm. > > > > > > > > Ok, updated based on benwells feedback. Should I do a rebase on master > > before > > > > it's merged? > > > > > > Technically a rebase is not required and if the commit queue can cherry pick > > w/o > > > conflict it will land OK. However this CL has been in review so long you > might > > > want to do a rebase and run of some/all unit tests to ensure it's still > valid. > > > I'd say that if you rebase w/o conflict and tests pass then just press the > > > Commit button and monitor results. > > > > > > Since it's your first (I think) I'd suggest doing this when you have a few > > hours > > > to track the progress to make sure it doesn't break anything. Some of the > bots > > > run more (and slower) tests so you don't always know right away if you > caused > > > any problems. > > > > I think you still need more owners - for content/ and for storage/ > > By content/, you're referring to the changes for indexeddb? I thought jsbell > deferred to cmumford for that? Am I missing something? content/public will require a content/ owner - maybe jam@ ?
On 2015/01/16 05:56:36, tzik wrote: > https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... > File chrome/browser/extensions/app_data_migrator.cc (right): > > https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... > chrome/browser/extensions/app_data_migrator.cc:112: > base::Bind(&MigrateFileSystem, old_partition, current_partition, > This closure may run after the profile has gone. > > How about: > - Convert this function to AppDataMigration's method, > - Add an WeakPtrFactory to AppDataMigration. > - Pass its WeakPtr as this pointer, so that the task will be canceled when the > instance is destroyed. @tzik, looking through the code, I can't find a code path where the profile would go away without cleaning up the FileSystem on it's respective task runner. Since I'm using that task runner, shouldn't this be enough?
On 2015/01/20 16:37:22, jsbell wrote: > On 2015/01/20 15:00:22, ryanackley wrote: > > On 2015/01/16 03:22:25, benwells wrote: > > > On 2015/01/16 01:02:51, cmumford wrote: > > > > On 2015/01/16 00:20:43, ryanackley wrote: > > > > > On 2015/01/14 23:45:04, cmumford wrote: > > > > > > On 2015/01/14 18:21:01, jsbell wrote: > > > > > > > I defer to cmumford for indexeddb/ > > > > > > > > > > > > > > tzik@ - do you want to take another look? > > > > > > > > > > > > ryanackley: benwells comments are simple or nits, so once done this > > change > > > > > > slgtm. > > > > > > > > > > Ok, updated based on benwells feedback. Should I do a rebase on master > > > before > > > > > it's merged? > > > > > > > > Technically a rebase is not required and if the commit queue can cherry > pick > > > w/o > > > > conflict it will land OK. However this CL has been in review so long you > > might > > > > want to do a rebase and run of some/all unit tests to ensure it's still > > valid. > > > > I'd say that if you rebase w/o conflict and tests pass then just press the > > > > Commit button and monitor results. > > > > > > > > Since it's your first (I think) I'd suggest doing this when you have a few > > > hours > > > > to track the progress to make sure it doesn't break anything. Some of the > > bots > > > > run more (and slower) tests so you don't always know right away if you > > caused > > > > any problems. > > > > > > I think you still need more owners - for content/ and for storage/ > > > > By content/, you're referring to the changes for indexeddb? I thought jsbell > > deferred to cmumford for that? Am I missing something? > > content/public will require a content/ owner - maybe jam@ ? Ryan: for each file in your patch, you need to get an lg from an owner of that file. If you don't have this, you won't be able to land your patch. To find owners, look for an OWNERS file in the file's directory, or a parent. The OWNERS are looked for recursively. You should prefer owners in OWNERS files closer to the files your changing. This probably explains it better than I can: http://www.chromium.org/developers/owners-files
ryanackley@gmail.com changed reviewers: + jam@chromium.org
On 2015/01/16 05:56:36, tzik wrote: > https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... > File chrome/browser/extensions/app_data_migrator.cc (right): > > https://codereview.chromium.org/671873004/diff/180001/chrome/browser/extensio... > chrome/browser/extensions/app_data_migrator.cc:112: > base::Bind(&MigrateFileSystem, old_partition, current_partition, > This closure may run after the profile has gone. > > How about: > - Convert this function to AppDataMigration's method, > - Add an WeakPtrFactory to AppDataMigration. > - Pass its WeakPtr as this pointer, so that the task will be canceled when the > instance is destroyed. @tzik, I didn't do exactly what you suggested because there are restrictions on unwrapping a WeakPtr on different threads. I think the way I ended up doing it accomplishes the same goal. Please review and let me know.
content/public lgtm
lgtm
The CQ bit was checked by ryanackley@gmail.com
The CQ bit was unchecked by ryanackley@gmail.com
The CQ bit was checked by ryanackley@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/671873004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ryanackley@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/671873004/300001
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/48bedbd2773df847e5dbf1c01fb409da4636d542 Cr-Commit-Position: refs/heads/master@{#313394} |
