Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
46583007:
Download webstore CRXs into separate directory. (Closed)
|
Created:
7 years, 1 month ago by jackhou1 Modified:
7 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, benwells, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDownload webstore CRXs into separate directory.
Currently CRXs downloaded from the webstore (i.e. during extension/app
install) go to the default download directory, but are deleted once
installed. This is particularly obvious on Mac where the downloads
folder will bounce in the dock to indicate a new file.
This CL adds a field trial to make all CRXs downloaded from the webstore
go into their own directory under the user data dir. It also adds UMA for
successful and failed installs from the webstore.
BUG=309536, 313992
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236167
Patch Set 1 #Patch Set 2 : Use FILE_PATH_LITERAL #Patch Set 3 : Control via field trial. Add UMA for webstore installs. #
Total comments: 2
Patch Set 4 : Link field trial with histogram. #
Total comments: 5
Patch Set 5 : Add enum="BooleanSuccess" #Patch Set 6 : Remove field trial block #
Total comments: 14
Patch Set 7 : Sync and rebase #Patch Set 8 : Address comments #
Messages
Total messages: 23 (0 generated)
asargent, not sure if you're the right person to review this, feel free to redirect/add more reviewers.
This is a good start. A couple of questions: -Does this properly handle cleanup in the case of interrupted transfers? I think we might get that for free currently, but I don't know off the top of my head if the implementation is tied to doing something like scanning the Downloads directory on startup for files with a special prefix indicating they were in-progress downloads. -Given the history of problems we've had with extension/app downloads and installs, I wonder if we want to try rolling this out slowly using either a Field Trial (base/metrics/field_trial.h) or even getting fancy and using Finch (go/finch). In either case we'd want to include an UMA_HISTOGRAM so we can measure any difference in success rates from the current code.
On 2013/10/31 05:16:05, Antony Sargent wrote: > This is a good start. A couple of questions: > > -Does this properly handle cleanup in the case of interrupted transfers? I think > we might get that for free currently, but I don't know off the top of my head if > the implementation is tied to doing something like scanning the Downloads > directory on startup for files with a special prefix indicating they were > in-progress downloads. At the moment, webstore CRX downloads don't get a prefix to indicate an in-progress download. However, if the download is cancelled or fails, the file is deleted, regardless of where the file is. If Chrome is killed during the download, the partial CRX file is not deleted, also regardless of where the file is. > -Given the history of problems we've had with extension/app downloads and > installs, I wonder if we want to try rolling this out slowly using either a > Field Trial (base/metrics/field_trial.h) or even getting fancy and using Finch > (go/finch). In either case we'd want to include an UMA_HISTOGRAM so we can > measure any difference in success rates from the current code. Done.
asargent, PTAL
lgtm w/ one possible change you might want to make https://codereview.chromium.org/46583007/diff/190001/chrome/browser/extension... File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc (right): https://codereview.chromium.org/46583007/diff/190001/chrome/browser/extension... chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:188: UMA_HISTOGRAM_BOOLEAN("Webstore.ExtensionInstallResult", success); Does the histograms dashboard have an easy way for you to see the aggregate values for this broken out by the field trial value? If not you might want to switch to having two different histogram names, one for each field trial bucket.
motek, could you please review to see if I've done the histogram/field trial stuff right? We want be able to track the success rate with and without the field trial enabled. https://codereview.chromium.org/46583007/diff/190001/chrome/browser/extension... File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc (right): https://codereview.chromium.org/46583007/diff/190001/chrome/browser/extension... chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:188: UMA_HISTOGRAM_BOOLEAN("Webstore.ExtensionInstallResult", success); On 2013/11/07 00:18:47, Antony Sargent wrote: > Does the histograms dashboard have an easy way for you to see the aggregate > values for this broken out by the field trial value? If not you might want to > switch to having two different histogram names, one for each field trial bucket. > Ah, thanks for pointing this out. The dashboard doesn't, but I think you can generate histograms prefixed with the field trial using histograms.xml.
https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... tools/metrics/histograms/histograms.xml:19676: +<histogram name="Webstore.ExtensionInstallResult"> The result is either 'successful or not', right? If so, you should declare histogram's enum tag as either BooleanSuccess or simply Boolean. This will make results easier to interpret. https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... tools/metrics/histograms/histograms.xml:30740: + <group name="SeparateDirectoryUnderUDD"/> It seems that for your purpose this block is not necessary. You will be able to declare your field trial server-side. Please remove unless you think you need suffixed version of histograms for some reason.
https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... tools/metrics/histograms/histograms.xml:19676: +<histogram name="Webstore.ExtensionInstallResult"> On 2013/11/12 16:41:57, motek. wrote: > The result is either 'successful or not', right? If so, you should declare > histogram's enum tag as either BooleanSuccess or simply Boolean. This will make > results easier to interpret. Done. https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... tools/metrics/histograms/histograms.xml:30740: + <group name="SeparateDirectoryUnderUDD"/> On 2013/11/12 16:41:57, motek. wrote: > It seems that for your purpose this block is not necessary. You will be able to > declare your field trial server-side. Please remove unless you think you need > suffixed version of histograms for some reason. If I remove this block, will I be able to see the results of Webstore.ExtensionInstallResult separately (i.e. with and without the field trial)?
On 2013/11/12 23:39:07, jackhou1 wrote: > https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... > File tools/metrics/histograms/histograms.xml (right): > > https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... > tools/metrics/histograms/histograms.xml:19676: +<histogram > name="Webstore.ExtensionInstallResult"> > On 2013/11/12 16:41:57, motek. wrote: > > The result is either 'successful or not', right? If so, you should declare > > histogram's enum tag as either BooleanSuccess or simply Boolean. This will > make > > results easier to interpret. > > Done. I don't see it. Make sure to upload, pls. > > https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... > tools/metrics/histograms/histograms.xml:30740: + <group > name="SeparateDirectoryUnderUDD"/> > On 2013/11/12 16:41:57, motek. wrote: > > It seems that for your purpose this block is not necessary. You will be able > to > > declare your field trial server-side. Please remove unless you think you need > > suffixed version of histograms for some reason. > > If I remove this block, will I be able to see the results of > Webstore.ExtensionInstallResult separately (i.e. with and without the field > trial)? Yes.
>I don't see it. Make sure to upload, pls. It should be up now in Patch 6. Have I done this right? The histogram line now reads: <histogram name="Webstore.ExtensionInstallResult" enum="BooleanSuccess"> https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/46583007/diff/260001/tools/metrics/his... tools/metrics/histograms/histograms.xml:30740: + <group name="SeparateDirectoryUnderUDD"/> On 2013/11/12 23:39:08, jackhou1 wrote: > On 2013/11/12 16:41:57, motek. wrote: > > It seems that for your purpose this block is not necessary. You will be able > to > > declare your field trial server-side. Please remove unless you think you need > > suffixed version of histograms for some reason. > > If I remove this block, will I be able to see the results of > Webstore.ExtensionInstallResult separately (i.e. with and without the field > trial)? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/46583007/350001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
jar, please review for OWNERS in tools/metrics
Ping, also +mpearson in case jar@ is not available.
Your best bet for OWNER's on finch-related things might be asvitkine@. On Mon, Nov 18, 2013 at 6:06 PM, <jackhou@chromium.org> wrote: > Ping, also +mpearson in case jar@ is not available. > > https://codereview.chromium.org/46583007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
histograms.xml lgtm with one revision below https://codereview.chromium.org/46583007/diff/350001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/46583007/diff/350001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:19678: + The success or failure of extension installs from the webstore. Is this only user-initiated installs or all installs of an extension hosted on the webstore (e.g., an install that can be initiated by sync)? Please clarify.
histograms.xml LGTM... but please address the nits listed below. (yeah... I know some is not your code... but since you're in the file near these lines, you should probably fix 'em. thanks!) https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... File chrome/browser/extensions/webstore_installer.cc (right): https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:80: "Dependency is not shared module"; nit: indent 4 https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:94: const base::FilePath& download_directory, const std::string& id, nit: one arg per line in definitions and declarations when you have to wrap. https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:97: *g_download_directory_for_tests : download_directory); nit: indent only 4 https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:137: const char kWebstoreDownloadDirectoryFieldTrialName[] = nit: these super long names reduce readability IMO. Consider: const char kFieldTrial = ... Alternatively... it would be nice to reference something in a header that is shared. https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:152: const std::string& extension_id, InstallSource source) { nit: one arg per line. https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:506: const std::string& extension_id, InstallSource source) { nit: one arg per line.
https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... File chrome/browser/extensions/webstore_installer.cc (right): https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:80: "Dependency is not shared module"; On 2013/11/18 23:49:44, jar wrote: > nit: indent 4 Done. https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:94: const base::FilePath& download_directory, const std::string& id, On 2013/11/18 23:49:44, jar wrote: > nit: one arg per line in definitions and declarations when you have to wrap. Done. https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:97: *g_download_directory_for_tests : download_directory); On 2013/11/18 23:49:44, jar wrote: > nit: indent only 4 Done. https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:137: const char kWebstoreDownloadDirectoryFieldTrialName[] = On 2013/11/18 23:49:44, jar wrote: > nit: these super long names reduce readability IMO. Consider: > > const char kFieldTrial = ... > > Alternatively... it would be nice to reference something in a header that is > shared. > Done. https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:152: const std::string& extension_id, InstallSource source) { On 2013/11/18 23:49:44, jar wrote: > nit: one arg per line. Done. https://codereview.chromium.org/46583007/diff/350001/chrome/browser/extension... chrome/browser/extensions/webstore_installer.cc:506: const std::string& extension_id, InstallSource source) { On 2013/11/18 23:49:44, jar wrote: > nit: one arg per line. Done. https://codereview.chromium.org/46583007/diff/350001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/46583007/diff/350001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:19678: + The success or failure of extension installs from the webstore. On 2013/11/18 23:48:52, Mark P wrote: > Is this only user-initiated installs or all installs of an extension hosted on > the webstore (e.g., an install that can be initiated by sync)? Please clarify. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/46583007/550001
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests, chromedriver2_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, gpu_unittests, installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jackhou@chromium.org/46583007/550001
Message was sent while issue was closed.
Change committed as 236167 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
