Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10834191:
new implementation of default apps (Closed)
|
Created:
8 years, 4 months ago by Gaurav Modified:
8 years, 4 months ago Reviewers:
Mihai Parparita -not on Chrome CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org Visibility:
Public. |
DescriptionNew implementation of default apps.
The default apps are now installed using the external extension install
flow but are considered as internal thereafter. Thus, they should behave
exactly the other apps post installation. Also, they are only installed
during new profile creation or for new Chrome users.
BUG=138614
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153137
Patch Set 1 #
Total comments: 9
Patch Set 2 : fixing comments #Patch Set 3 : Added unit_test #Patch Set 4 : fixed broken test #
Total comments: 8
Patch Set 5 : addressed comments #
Total comments: 5
Patch Set 6 : comments #
Total comments: 3
Patch Set 7 : comments #Patch Set 8 : disable default_app unit_test for chromeos #
Total comments: 1
Messages
Total messages: 18 (0 generated)
This implementation satisfies most of the points in the requirement docs. I have tested few of them manually(have to test more thoroughly with different combinations) and writing unit tests for all of them. Also, yet to test the sync behavior. Can you please have a initial look. Thanks, -- Gaurav
As far as testing this, doing it via unit tests and checking the installed state of extensions seems reasonable. That's what was done for the one (and only) default apps-related test that was added: http://codereview.chromium.org/9133023/diff/10001/chrome/browser/extensions/e... https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... File chrome/browser/extensions/default_apps.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... chrome/browser/extensions/default_apps.cc:47: case default_apps::kAlwaysProvideDefaultApps: It seems like in this case you'll want to migrate from the old default apps mechanism (whcih used the EXTERNAL location) to the new one, otherwise users who had their default apps before your CL will lose them (since they'll no longer be provided). https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... chrome/browser/extensions/default_apps.cc:77: install_apps = base::FieldTrialList::Find( Is this how we end up with install_apps being true most of the time (otherwise, it seems like we set it to false nearly everywhere else)? https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... File chrome/browser/extensions/external_provider_impl.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... chrome/browser/extensions/external_provider_impl.cc:397: // The default apps are installed as INTERNAL but use the external This is clever, since it means that you don't have to reinvent a mechanism for installing a list of extensions. However, I can see it being a bit confusing (since it uses the "external" name). Aaron, does this seem OK to you?
Also, I'm not sure if you realized this already, but the default apps list is only included for branded "Chrome" builds: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/chrome_resources.gy... This means that "Chromium" builds (what most developers build on a regular basis, and what is tested on build.chromium.org) won't have them. For testing, you may want a way of overriding that. Mihai On Tue, Aug 7, 2012 at 5:22 PM, <mihaip@chromium.org> wrote: > As far as testing this, doing it via unit tests and checking the installed > state > of extensions seems reasonable. That's what was done for the one (and only) > default apps-related test that was added: > > http://codereview.chromium.**org/9133023/diff/10001/chrome/** > browser/extensions/extension_**service_unittest.cc<http://codereview.chromium.org/9133023/diff/10001/chrome/browser/extensions/extension_service_unittest.cc> > > > https://chromiumcodereview.**appspot.com/10834191/diff/1/** > chrome/browser/extensions/**default_apps.cc<https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensions/default_apps.cc> > File chrome/browser/extensions/**default_apps.cc (right): > > https://chromiumcodereview.**appspot.com/10834191/diff/1/** > chrome/browser/extensions/**default_apps.cc#newcode47<https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensions/default_apps.cc#newcode47> > chrome/browser/extensions/**default_apps.cc:47: case > default_apps::**kAlwaysProvideDefaultApps: > It seems like in this case you'll want to migrate from the old default > apps mechanism (whcih used the EXTERNAL location) to the new one, > otherwise users who had their default apps before your CL will lose them > (since they'll no longer be provided). > > https://chromiumcodereview.**appspot.com/10834191/diff/1/** > chrome/browser/extensions/**default_apps.cc#newcode77<https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensions/default_apps.cc#newcode77> > chrome/browser/extensions/**default_apps.cc:77: install_apps = > base::FieldTrialList::Find( > Is this how we end up with install_apps being true most of the time > (otherwise, it seems like we set it to false nearly everywhere else)? > > https://chromiumcodereview.**appspot.com/10834191/diff/1/** > chrome/browser/extensions/**external_provider_impl.cc<https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensions/external_provider_impl.cc> > File chrome/browser/extensions/**external_provider_impl.cc (right): > > https://chromiumcodereview.**appspot.com/10834191/diff/1/** > chrome/browser/extensions/**external_provider_impl.cc#**newcode397<https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensions/external_provider_impl.cc#newcode397> > chrome/browser/extensions/**external_provider_impl.cc:397: // The default > > apps are installed as INTERNAL but use the external > This is clever, since it means that you don't have to reinvent a > mechanism for installing a list of extensions. However, I can see it > being a bit confusing (since it uses the "external" name). > > Aaron, does this seem OK to you? > > https://chromiumcodereview.**appspot.com/10834191/<https://chromiumcodereview... >
http://codereview.chromium.org/10834191/diff/1/chrome/browser/extensions/defa... File chrome/browser/extensions/default_apps.h (right): http://codereview.chromium.org/10834191/diff/1/chrome/browser/extensions/defa... chrome/browser/extensions/default_apps.h:5: #ifndef CHROME_BROWSER_EXTENSIONS_DEFAULT_APPS_H_ Nit: Since this is not really apps-specific, it should be called default_extensions or default_items. http://codereview.chromium.org/10834191/diff/1/chrome/browser/extensions/defa... chrome/browser/extensions/default_apps.h:25: kNeverProvideDefaultApps, Usually when enums have a warning like this, the order should not be changed (because the thing persisted is the integer value). http://codereview.chromium.org/10834191/diff/1/chrome/browser/extensions/exte... File chrome/browser/extensions/external_provider_impl.cc (right): http://codereview.chromium.org/10834191/diff/1/chrome/browser/extensions/exte... chrome/browser/extensions/external_provider_impl.cc:397: // The default apps are installed as INTERNAL but use the external On 2012/08/08 00:22:43, Mihai Parparita wrote: > This is clever, since it means that you don't have to reinvent a mechanism for > installing a list of extensions. However, I can see it being a bit confusing > (since it uses the "external" name). > > Aaron, does this seem OK to you? It seems reasonable to me that providers can specify what location they want to be installed as, and that they only participate in the rest of the protocol if they pick one of external_*. The naming is a bit confusing due to legacy... maybe these are really just ExtensionProviders, or ExtensionInstallSources, or something. It would be nice to rename it so that its behavior is better described. But that could be a separate CL.
<moving myself to cc>
Added a unit_test and added code to migrate old default apps. Will add a preference remembering default apps, and disabling sync in the follow up CL. https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... File chrome/browser/extensions/default_apps.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... chrome/browser/extensions/default_apps.cc:47: case default_apps::kAlwaysProvideDefaultApps: install_apps becomes true in most cases from the kDefaultApps == "install" check above. On 2012/08/08 00:22:42, Mihai Parparita wrote: > It seems like in this case you'll want to migrate from the old default apps > mechanism (whcih used the EXTERNAL location) to the new one, otherwise users who > had their default apps before your CL will lose them (since they'll no longer be > provided). https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... File chrome/browser/extensions/default_apps.h (right): https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... chrome/browser/extensions/default_apps.h:25: kNeverProvideDefaultApps, Ya I missed it. Fixed it now. On 2012/08/08 22:23:29, Aaron Boodman wrote: > Usually when enums have a warning like this, the order should not be changed > (because the thing persisted is the integer value). https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... File chrome/browser/extensions/external_provider_impl.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/1/chrome/browser/extensi... chrome/browser/extensions/external_provider_impl.cc:397: // The default apps are installed as INTERNAL but use the external It may require more re-factoring and isolation. The kind at some places assumes that all extensions provided are external and uninstalls them if they are not provided anymore. Will do in a followup CL. On 2012/08/08 22:23:29, Aaron Boodman wrote: > On 2012/08/08 00:22:43, Mihai Parparita wrote: > > This is clever, since it means that you don't have to reinvent a mechanism for > > installing a list of extensions. However, I can see it being a bit confusing > > (since it uses the "external" name). > > > > Aaron, does this seem OK to you? > > It seems reasonable to me that providers can specify what location they want to > be installed as, and that they only participate in the rest of the protocol if > they pick one of external_*. > > The naming is a bit confusing due to legacy... maybe these are really just > ExtensionProviders, or ExtensionInstallSources, or something. It would be nice > to rename it so that its behavior is better described. But that could be a > separate CL.
https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... File chrome/browser/extensions/default_apps.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... chrome/browser/extensions/default_apps.cc:40: // migrate old default_apps. This will install them once changing the location - Capitalize "Migrate" - Remove the underscore in default_apps. - The comment should have a sentence or two about the old default apps mechanism (mention that it used the external location, etc.). - There should be a TODO to remove the migration code in ~6 months. https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... chrome/browser/extensions/default_apps.cc:43: state = default_apps::kUnknown; I think this will mean that users will lose their default apps. If I had default apps with Chrome 22 (value was kAlwaysProvideDefaultApps), then state gets reset of kUnknown here. Then on line 51, we check if this is a new profile (assuming we're now running Chrome 23), and since it's not, install_apps is now false. Your test doesn't catch this, since it's testing with a profile created by the same version as the one that's currently running. https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... chrome/browser/extensions/default_apps.cc:123: // Don't bother installing default apps in locales where it is known that Intended one too many times. https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... File chrome/browser/extensions/default_apps.h (right): https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... chrome/browser/extensions/default_apps.h:26: kAlwaysProvideDefaultApps, Can you rename the enum value to kProvidedLegacyDefaultApps or something else more descriptive (even when the comment isn't visible)? https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... chrome/browser/extensions/default_apps.h:27: kNeverProvideDefaultApps, Rename this to kNeverInstallDefaultApps, since we no longe really "provide" install apps. https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... chrome/browser/extensions/extension_service.cc:2272: if (existing && (location != Extension::INTERNAL || I found this boolean expression hard to read because of all the negations. Can you instead have a helper variable like: bool is_default_apps_migration = location == Extension::INTERNAL && Extension::IsExternalLocation(existing->location()); if (existing && !is_default_apps_migration) { ... https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... File chrome/browser/extensions/extension_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... chrome/browser/extensions/extension_service_unittest.cc:3726: Nit: extra newline. https://chromiumcodereview.appspot.com/10834191/diff/6003/chrome/browser/exte... chrome/browser/extensions/extension_service_unittest.cc:3729: // The default apps should be installed if kDefaultAppsInstallState Extra space after if.
Addressed comments
https://chromiumcodereview.appspot.com/10834191/diff/1007/chrome/browser/exte... File chrome/browser/extensions/default_apps.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/1007/chrome/browser/exte... chrome/browser/extensions/default_apps.cc:51: // installed everytime chrome was run. Thus, changing the list of default Nit: capitalize Chrome. https://chromiumcodereview.appspot.com/10834191/diff/1007/chrome/browser/exte... chrome/browser/extensions/default_apps.cc:52: // apps affected all users. Migrate old default apps to new mechanism where BTW, migration is going to be get a bit more complicated once we add the new app (Google Docs/Drive) to the default apps set. We're going to want to migrate the older apps, but not the new one. We can worry about that in a follow-up CL though. https://chromiumcodereview.appspot.com/10834191/diff/1007/chrome/browser/exte... chrome/browser/extensions/default_apps.cc:54: // TODO (grv) : remove after migration (~6 months). Instead of a relative timestamp, make this absolute. Q1 2013 perhaps? https://chromiumcodereview.appspot.com/10834191/diff/1007/chrome/browser/exte... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/1007/chrome/browser/exte... chrome/browser/extensions/extension_service.cc:2295: Nit: extra newline. https://chromiumcodereview.appspot.com/10834191/diff/1007/chrome/browser/exte... File chrome/browser/extensions/extension_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/1007/chrome/browser/exte... chrome/browser/extensions/extension_service_unittest.cc:3725: TEST_F(ExtensionServiceTest, DefaultAppsInstall) { Can you also add a test for the migration case where the chrome version changes (the bug you fixed in this most recent patch set)?
LGTM https://chromiumcodereview.appspot.com/10834191/diff/4004/chrome/browser/exte... File chrome/browser/extensions/default_apps.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/4004/chrome/browser/exte... chrome/browser/extensions/default_apps.cc:54: // TODO (grv) : remove after Q1-2013. Nit: no space between TODO and parenthesis (see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C...) https://chromiumcodereview.appspot.com/10834191/diff/4004/chrome/browser/exte... File chrome/browser/extensions/default_apps.h (right): https://chromiumcodereview.appspot.com/10834191/diff/4004/chrome/browser/exte... chrome/browser/extensions/default_apps.h:25: // Now unused, leave it for backward compatibility. Nit: "left for backward compatibility/migration" is more correct. https://chromiumcodereview.appspot.com/10834191/diff/4004/chrome/browser/exte... File chrome/browser/extensions/extension_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/4004/chrome/browser/exte... chrome/browser/extensions/extension_service_unittest.cc:3755: bool WasCreatedByVersionOrLater(const std::string& version) { Seems like this needs virtual and OVERRIDE annotations.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grv@chromium.org/10834191/15001
Try job failure for 10834191-15001 (retry) on linux_chromeos for step "unit_tests". It's a second try, previously, step "unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Looks like you need to exclude the test on ChromeOS, since default apps are not enabled there: http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/... Mihai On Thu, Aug 23, 2012 at 3:05 PM, <commit-bot@chromium.org> wrote: > Try job failure for 10834191-15001 (retry) on linux_chromeos for step > "unit_tests". > It's a second try, previously, step "unit_tests" failed. > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=linux_**chromeos&number=34899<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=34899> > > > https://chromiumcodereview.**appspot.com/10834191/<https://chromiumcodereview... >
Thanks Mihai On 2012/08/23 23:08:01, Mihai Parparita wrote: > Looks like you need to exclude the test on ChromeOS, since default apps are > not enabled there: > > http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/... > > Mihai > > On Thu, Aug 23, 2012 at 3:05 PM, <mailto:commit-bot@chromium.org> wrote: > > > Try job failure for 10834191-15001 (retry) on linux_chromeos for step > > "unit_tests". > > It's a second try, previously, step "unit_tests" failed. > > http://build.chromium.org/p/**tryserver.chromium/** > > > buildstatus?builder=linux_**chromeos&number=34899<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chromeos&number=34899> > > > > > > > https://chromiumcodereview.**appspot.com/10834191/%3Chttps://chromiumcoderevi...> > >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grv@chromium.org/10834191/23001
https://chromiumcodereview.appspot.com/10834191/diff/23001/chrome/browser/ext... File chrome/browser/extensions/extension_service_unittest.cc (right): https://chromiumcodereview.appspot.com/10834191/diff/23001/chrome/browser/ext... chrome/browser/extensions/extension_service_unittest.cc:3831: #if !defined(OS_CHROMEOS) Just realized that this might make sense in a separate default_apps_unittest.cc file (this file is long enough as it is). Feel free to do that in a followup CL.
Change committed as 153137 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
