Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8245018:
Remove race condition when installing default apps into a new profile. (Closed)
|
Created:
9 years, 2 months ago by Roger Tawa OOO till Jul 10th Modified:
6 years, 1 month ago Reviewers:
ChuckLawrence27, Finnur, Sam Kerner (Chrome), miket_OOO, commit-bot: I haz the power, Mihai Parparita -not on Chrome CC:
chromium-reviews, Aaron Boodman, Erik does not do reviews, mihaip+watch_chromium.org, Mattias Nissler (ping if slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRemove race condition when installing default apps into a new profile.
BUG=99547
TEST=Install chrome and stop it as quickly as possible when the window opens.
Or create a new profile and close all windows as soon as the new profile's
window opens. Do that as often as you like. Make sure that eventually,
after leaving the windows open, that all default apps are correctly installed.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107144
Patch Set 1 #Patch Set 2 : Adding unit tests #Patch Set 3 : Forgot to include gypi file #Patch Set 4 : Fix trybot breaks #Patch Set 5 : Fix more trybot breaks #
Total comments: 31
Patch Set 6 : Addressing review comments #
Total comments: 7
Patch Set 7 : Addressing review comments, merge after sync #Patch Set 8 : Reworked CL, much simpler #
Total comments: 12
Patch Set 9 : Addressing review comments, merge after sync #
Total comments: 8
Patch Set 10 : Addressing review comments #
Total comments: 1
Patch Set 11 : Addressing review comments #
Messages
Total messages: 33 (2 generated)
Hi Mihai, Please take another look. Fixed up as you suggested: - don't mess with extensions in the prefs_ member - detects errors during install with a new member Also added unit tests. Thanks.
Cc-ing Finnur and Sam since they know the external extension system better than I do. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:474: // We decide to install or not install default apps based on the following If you're going to be moving DefaultAppsProvider to its own file, you should also move the logic to check whether or not it needs to be registered to a DefaultAppsProvider::ShouldRegister() static method. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.h (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:55: virtual void SetPrefs(base::DictionaryValue* prefs); I'm getting uncomfortable with the increased overriding of methods from ExternalExtensionProviderImpl so that you can implement DefaultAppsProvider, since the two classes are becoming tightly coupled. The preferred design would be to either have a whole new ExternalExtensionProviderInterface implementation, or to implement a new ExternalExtensionLoader that you can plug into ExternalExtensionProviderImpl http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:113: // prefs_, but for now, there is code that depends on these invalid extensions What code are you referring to here? http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:121: class DefaultAppsProvider : public ExternalExtensionProviderImpl, Now that you're exposing this class in the header file and it's gotten pretty big, you should probably move it to its own .h/.cc file. Also, then you can exclude those files from the Chrome OS build via a Gyp condition instead of needing preprocessor wrappers. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/profiles/pro... File chrome/browser/profiles/profile.cc (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/profiles/pro... chrome/browser/profiles/profile.cc:182: prefs->RegisterIntegerPref(prefs::kDefaultAppsInstallState, 0, Pref registration should also be done in DefaultAppsProvider::RegisterUserPrefs
http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:75: if (prefs->size() == invalid_extensions().size()) { Is it safe to rely on the count? Don't you want to compare id's? http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:90: prefs::kDefaultAppsInstallState); nit: Is there room in the line above or are my eyes deceiving me? http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:97: service()->OnExternalProviderReady(); nit: Probably doesn't matter, but shouldn't you set the kInstallDone pref and then say that the provider is ready? (in other words, move this between line 102 and 103)? http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:99: // Keep track that we are done. nit: Keep track of the fact that we are done. Better? http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:110: // No break. This is a bit weird... skip the break to fall into a case statement that contains only a break? http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:114: // Continue the install process. This comment is a bit confusing. I think you want to say: // The profile was closed last time the extensions were being installed. // Resume the install process. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.h (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:121: class DefaultAppsProvider : public ExternalExtensionProviderImpl, +1 On 2011/10/19 07:31:36, Mihai Parparita wrote: > Now that you're exposing this class in the header file and it's gotten pretty > big, you should probably move it to its own .h/.cc file. Also, then you can > exclude those files from the Chrome OS build via a Gyp condition instead of > needing preprocessor wrappers. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:144: enum InstallState { Perhaps document that the values of these enums are written to the user's profile, so they must not change? http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:148: }; nit: enum should be at the top of the public section. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl_unittest.cc (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl_unittest.cc:39: // No actual loading to be done, simple use the prefs given. nit: simple -> simply?
Thanks Mihai, Finnur. Changes uploaded, please take another look. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:75: if (prefs->size() == invalid_extensions().size()) { On 2011/10/19 10:01:29, Finnur wrote: > Is it safe to rely on the count? Don't you want to compare id's? The invalid_extension set can only come from extensions listed in prefs. So if the counts are the same then all the ids must be same. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:90: prefs::kDefaultAppsInstallState); On 2011/10/19 10:01:29, Finnur wrote: > nit: Is there room in the line above or are my eyes deceiving me? yup, you need glasses :-) http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:97: service()->OnExternalProviderReady(); On 2011/10/19 10:01:29, Finnur wrote: > nit: Probably doesn't matter, but shouldn't you set the kInstallDone pref and > then say that the provider is ready? (in other words, move this between line 102 > and 103)? Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:99: // Keep track that we are done. On 2011/10/19 10:01:29, Finnur wrote: > nit: Keep track of the fact that we are done. > Better? Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:110: // No break. On 2011/10/19 10:01:29, Finnur wrote: > This is a bit weird... skip the break to fall into a case statement that > contains only a break? Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:114: // Continue the install process. On 2011/10/19 10:01:29, Finnur wrote: > This comment is a bit confusing. I think you want to say: > // The profile was closed last time the extensions were being installed. > // Resume the install process. more verbose explanation. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:474: // We decide to install or not install default apps based on the following On 2011/10/19 07:31:36, Mihai Parparita wrote: > If you're going to be moving DefaultAppsProvider to its own file, you should > also move the logic to check whether or not it needs to be registered to a > DefaultAppsProvider::ShouldRegister() static method. Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.h (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:55: virtual void SetPrefs(base::DictionaryValue* prefs); On 2011/10/19 07:31:36, Mihai Parparita wrote: > I'm getting uncomfortable with the increased overriding of methods from > ExternalExtensionProviderImpl so that you can implement DefaultAppsProvider, > since the two classes are becoming tightly coupled. > > The preferred design would be to either have a whole new > ExternalExtensionProviderInterface implementation, or to implement a new > ExternalExtensionLoader that you can plug into ExternalExtensionProviderImpl I could create a separate implementation of ExternalExtensionProviderInterface, but the implementation would have a lot of duplicate code with ExternalExtensionProviderImpl, so I would likely create a base class anyway. Not sure that makes much of a difference then. Also, the relationship between the loaders and the the provider implementation is itself tightly coupled. For example, ExternalExtensionLoader::Init() takes a ExternalExtensionProviderImpl pointer directly, and not an ExternalExtensionProviderInterface. If this interface had a SetPrefs(), then this could be cleaned up. This would of course make SetPrefs() virtual in any case. I can make this change to the interface if you like and clean up the code if this is the correct approach, let me know. The default apps provider needs to do some things that cannot be done as an extension loader only. For example, it needs to know if the extensions are valid and supported, even if they did load successfully. Furthermore, the loader could not simply say "there are no extensions to load" if the profile is detected to already have apps, since then its provider could not implement HasExtension() and GetExtensionDetails(). http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:113: // prefs_, but for now, there is code that depends on these invalid extensions On 2011/10/19 07:31:36, Mihai Parparita wrote: > What code are you referring to here? If you look at patch set #1 of this CL, I was adding extensions that were deemed invalid to the unsupported set. In a private email you sent me, you worried that because this would make those extensions unavailable to HasExtension(), this might cause problems with existing code. However, you said it would be good to clean this up eventually. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:121: class DefaultAppsProvider : public ExternalExtensionProviderImpl, On 2011/10/19 07:31:36, Mihai Parparita wrote: > Now that you're exposing this class in the header file and it's gotten pretty > big, you should probably move it to its own .h/.cc file. Also, then you can > exclude those files from the Chrome OS build via a Gyp condition instead of > needing preprocessor wrappers. Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:121: class DefaultAppsProvider : public ExternalExtensionProviderImpl, On 2011/10/19 10:01:29, Finnur wrote: > +1 > > On 2011/10/19 07:31:36, Mihai Parparita wrote: > > Now that you're exposing this class in the header file and it's gotten pretty > > big, you should probably move it to its own .h/.cc file. Also, then you can > > exclude those files from the Chrome OS build via a Gyp condition instead of > > needing preprocessor wrappers. > Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:144: enum InstallState { On 2011/10/19 10:01:29, Finnur wrote: > Perhaps document that the values of these enums are written to the user's > profile, so they must not change? Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:148: }; On 2011/10/19 10:01:29, Finnur wrote: > nit: enum should be at the top of the public section. Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl_unittest.cc (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl_unittest.cc:39: // No actual loading to be done, simple use the prefs given. On 2011/10/19 10:01:29, Finnur wrote: > nit: simple -> simply? Done. http://codereview.chromium.org/8245018/diff/13002/chrome/browser/profiles/pro... File chrome/browser/profiles/profile.cc (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/profiles/pro... chrome/browser/profiles/profile.cc:182: prefs->RegisterIntegerPref(prefs::kDefaultAppsInstallState, 0, On 2011/10/19 07:31:36, Mihai Parparita wrote: > Pref registration should also be done in DefaultAppsProvider::RegisterUserPrefs Done.
Roger, longtime fan, first-time reviewer. As of r102956, the early-out of DefaultAppsProvider:VisitRegisteredExtension() causes ExtensionService's OnExternalProviderReady() to be called without ready_ = true. On my machine this leaves OnExternalProviderReady() in a zombie state, always waiting for the last provider to finish, and many things happen such as new external extensions never being installed. As an observation (probably not your original code), that SetPrefs has the side effect of setting ready_ is part of what let this happen. It's not at all obvious that SetPrefs is necessary to lead to the terminal state. The const-ness of some of the interface methods make the fix nontrivial, so have fun with that.
http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.h (right): http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:111: // prefs_, but for now, there is code that depends on these invalid extensions I didn't mean that there was code that relies on this, just that it's a behavior change that should be documented. Maybe add it to the CL description?
LGTM http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/8245018/diff/13002/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:90: prefs::kDefaultAppsInstallState); Awww... :) On 2011/10/19 20:48:25, Roger Tawa wrote: > On 2011/10/19 10:01:29, Finnur wrote: > > nit: Is there room in the line above or are my eyes deceiving me? > > yup, you need glasses :-) http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:133: // when the profile was last closed. This may happen because the the nit: The The? Do you mean The Who? ;) http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:144: ExternalExtensionProviderImpl::VisitRegisteredExtension(); nit: I'm wondering if it makes sense to comment this line, saying that this will cause registered extensions found to be installed, since it is not clear from the code about that break means "install" and return means "not install". Just a thought. I'll leave it up to you.
Good catch Mike. Yes, the const-ness of various methods led to this code being written the way it was. For example, I wanted to put the code that registers for notifications in DefaultAppsProvider::VisitRegisteredExtension(), but then I would be calling non-const methods on a const registrar_. I could have made this a mutable member, or I could have used a scoped ptr so that the const-ness of DefaultAppsProvider does not affect the registrar, but it seemed to me the better solution was to override SetPrefs(). Mihai: you didn't comment in my questions about SetPrefs(). Any recommendations? Thanks, Roger - On Wed, Oct 19, 2011 at 18:45, <miket@chromium.org> wrote: > Roger, longtime fan, first-time reviewer. As of r102956, the early-out of > DefaultAppsProvider:VisitRegisteredExtension() causes ExtensionService's > OnExternalProviderReady() to be called without ready_ = true. On my machine > this > leaves OnExternalProviderReady() in a zombie state, always waiting for the > last > provider to finish, and many things happen such as new external extensions > never > being installed. > > As an observation (probably not your original code), that SetPrefs has the > side > effect of setting ready_ is part of what let this happen. It's not at all > obvious that SetPrefs is necessary to lead to the terminal state. > > The const-ness of some of the interface methods make the fix nontrivial, so > have > fun with that. > > http://codereview.chromium.org/8245018/ >
Thanks again guys. Some questions below. http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:133: // when the profile was last closed. This may happen because the the On 2011/10/20 10:10:43, Finnur wrote: > nit: The The? Do you mean The Who? ;) no, I really meant the the: http://en.wikipedia.org/wiki/The_The OK, fixed :-) http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:144: ExternalExtensionProviderImpl::VisitRegisteredExtension(); On 2011/10/20 10:10:43, Finnur wrote: > nit: I'm wondering if it makes sense to comment this line, saying that this will > cause registered extensions found to be installed, since it is not clear from > the code about that break means "install" and return means "not install". Just a > thought. I'll leave it up to you. I think the comment in the interface is pretty good, and in chrome we don't usually copy the comments from the interface into the classes that implement that interface. But if you really want, I can add something. The interface comments says: // Enumerate registered extensions, calling // OnExternalExtension(File|UpdateUrl)Found on the |visitor| object for each // registered extension found. http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.h (right): http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.h:111: // prefs_, but for now, there is code that depends on these invalid extensions On 2011/10/20 02:30:25, Mihai Parparita wrote: > I didn't mean that there was code that relies on this, just that it's a behavior > change that should be documented. Maybe add it to the CL description? OK. I think it would be better to have the comment in the code though and not in a CL description, since this way it will be more findable by others. You had said: ---start quote--- The change you have to by default add extension IDs unsupported_extensions will mean that a lot more extensions will be uninstalled by default (since if HasExtension returns false we uninstall the extension in ExtensionService::CheckExternalUninstall). Arguably this might be a good thing (if the path is no longer valid because the app that installed the extension was moved/uninstalled, we should remove the extension too), but I want us to make that change intentionally. ---end quote--- So it seems there is code in ExtensionService that depends on this behaviour. I thought my comment captured your sentiment pretty well but if you'd like to suggest different text I would be more than happy to replace my comment with yours.
http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/19001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:144: ExternalExtensionProviderImpl::VisitRegisteredExtension(); No, that's ok. Never mind... On 2011/10/20 14:31:15, Roger Tawa wrote: > On 2011/10/20 10:10:43, Finnur wrote: > > nit: I'm wondering if it makes sense to comment this line, saying that this > will > > cause registered extensions found to be installed, since it is not clear from > > the code about that break means "install" and return means "not install". Just > a > > thought. I'll leave it up to you. > > I think the comment in the interface is pretty good, and in chrome we don't > usually copy the comments from the interface into the classes that implement > that interface. But if you really want, I can add something. The interface > comments says: > > // Enumerate registered extensions, calling > // OnExternalExtension(File|UpdateUrl)Found on the |visitor| object for each > // registered extension found.
Hi guys, I realized that my original change for default apps was fundamentally incorrect, since it was trying to use the provider once to install the apps, and then provide no apps on subsequent runs. If it had not been for the bug Mike found, my original code would have installed the apps on the first run of a profile, and then removed them on the next run because the provider no longer provided those apps. Also, the extension service already has all the logic to keep trying all provided apps until they are installed, so need to keep this kind of state in the provider itself. So I started again, and I think this code is *much* simpler. No more need to derive from ExternalExtensionProviderImpl or make any changes to it. No need for a new provider class at all. For a profile where default apps has never been considered, I make a decision upfront to either always support default apps or not, save this decision in the profile, and then use that same decision from then on. The only exceptions are for unit tests, which use a command line flag to always disable default apps, and the the field trial, which must ensure that apps are installed or not depending on the group. So this new patch fixes the original bug as well as the issue raised by Mike. Please review again. It might be easier to compare against the base code than the previous patch sets. Also, DefaultAppsProvider has essentially become two static functions and an enum, so if you'd prefer I change this to a namespace let me know. Thanks.
http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:34: // usually set in the master_preferences file Nit: Capitalize first word of comments (personal preference). Nit: End comments with period. http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:60: NOTREACHED(); Just something to consider: If you leave out the default case here then whenever someone adds to the enum |InstallState| they get a compile assert (on Linux and Mac) if they don't add a case to handle the new enum. Maybe you need to change the type of |state| from int to InstallState (not sure). http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:67: // an API. Is there a bug on this you can reference? http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.h (right): http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.h:17: } Are ExternalExtensionLoader and DictionaryValue needed anymore? http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.h:22: // These values are persisted in the user preferences so values should not Wording nit: These values ... so values ... Maybe talk about enums and values instead? http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:345: if (DefaultAppsProvider::ShouldRegister(profile)) { This function name is a bit misleading, since it appears you are asking whether to register a profile, as opposed to apps.
Thanks again Finnur. Changes uploaded, comments addressed. Please take another look. http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:34: // usually set in the master_preferences file On 2011/10/21 09:59:32, Finnur wrote: > Nit: Capitalize first word of comments (personal preference). > Nit: End comments with period. Done. http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:60: NOTREACHED(); On 2011/10/21 09:59:32, Finnur wrote: > Just something to consider: If you leave out the default case here then whenever > someone adds to the enum |InstallState| they get a compile assert (on Linux and > Mac) if they don't add a case to handle the new enum. Maybe you need to change > the type of |state| from int to InstallState (not sure). Done. http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:67: // an API. On 2011/10/21 09:59:32, Finnur wrote: > Is there a bug on this you can reference? No, created one and added to comment. http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.h (right): http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.h:17: } On 2011/10/21 09:59:32, Finnur wrote: > Are ExternalExtensionLoader and DictionaryValue needed anymore? Done. http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.h:22: // These values are persisted in the user preferences so values should not On 2011/10/21 09:59:32, Finnur wrote: > Wording nit: These values ... so values ... > Maybe talk about enums and values instead? Done. http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (right): http://codereview.chromium.org/8245018/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:345: if (DefaultAppsProvider::ShouldRegister(profile)) { On 2011/10/21 09:59:32, Finnur wrote: > This function name is a bit misleading, since it appears you are asking whether > to register a profile, as opposed to apps. Done.
http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:72: if (EndsWith(locale, unsupported_locales[i], false)) { Is there a locale utility function that tests this?
My comments have been addressed, so LGTM.
http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:72: if (EndsWith(locale, unsupported_locales[i], false)) { On 2011/10/24 14:46:04, Sam Kerner (Chrome) wrote: > Is there a locale utility function that tests this? There does not seem to be anything in l10_util.h that tests for this. Should I look somewhere else?
On 2011/10/24 15:35:36, Roger Tawa wrote: > http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... > File chrome/browser/extensions/default_apps_provider.cc (right): > > http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... > chrome/browser/extensions/default_apps_provider.cc:72: if (EndsWith(locale, > unsupported_locales[i], false)) { > On 2011/10/24 14:46:04, Sam Kerner (Chrome) wrote: > > Is there a locale utility function that tests this? > > There does not seem to be anything in l10_util.h that tests for this. Should I > look somewhere else? That is where it should be. If it's not there, a string compare is fine. LGTM
http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:65: // Don't bother installing default apps in locales where its known that Typo (its -> it's). http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.h (right): http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.h:14: // A provider of apps that are installed by default into all new profiles. This isn't a provider anymore, seems like a new name is needed.
http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (left): http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:56: // ExternalExtensionProviderImpl overrides: Also, if you're removing this, can you also undo the changes to ExternalExtensionsProviderImpl/Interface that you made with r102956, since they're no longer needed?
Hi Mihai, changes uploaded, please take another look. http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.cc (right): http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.cc:65: // Don't bother installing default apps in locales where its known that On 2011/10/24 17:28:05, Mihai Parparita wrote: > Typo (its -> it's). Done. http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps_provider.h (right): http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/d... chrome/browser/extensions/default_apps_provider.h:14: // A provider of apps that are installed by default into all new profiles. On 2011/10/24 17:28:05, Mihai Parparita wrote: > This isn't a provider anymore, seems like a new name is needed. Done. http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/e... File chrome/browser/extensions/external_extension_provider_impl.cc (left): http://codereview.chromium.org/8245018/diff/31001/chrome/browser/extensions/e... chrome/browser/extensions/external_extension_provider_impl.cc:56: // ExternalExtensionProviderImpl overrides: On 2011/10/24 17:39:25, Mihai Parparita wrote: > Also, if you're removing this, can you also undo the changes to > ExternalExtensionsProviderImpl/Interface that you made with r102956, since > they're no longer needed? Done.
http://codereview.chromium.org/8245018/diff/34003/chrome/browser/extensions/d... File chrome/browser/extensions/default_apps.h (right): http://codereview.chromium.org/8245018/diff/34003/chrome/browser/extensions/d... chrome/browser/extensions/default_apps.h:15: class DefaultApps { A namespace seems more appropriate, since DefaultApps itself should not be instantiable.
Hi Mihai, please take another look.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerta@chromium.org/8245018/36005
Change committed as 107144
Message was sent while issue was closed.
chucklawrence27@gmail.com changed reviewers: + ChuckLawrence27@gmail.com
Message was sent while issue was closed.
chucklawrence27@gmail.com changed reviewers: + ChuckLawrence27@gmail.com
Message was sent while issue was closed.
Message was sent while issue was closed.
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
