Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1240703003:
Extension profile generator + benchmark for startup with profile. (Closed)
|
Created:
5 years, 5 months ago by sydli Modified:
5 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, telemetry-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtension profile generator + benchmark for startup with profile.
Extension profile extender for profile generator and a new benchmark for
startup time on a profile that loads extensions. Also for this purpose, a
script to maintain a static set of extensions in cloud storage, and small
additions to python gsutil wrapper (cloud_storage).
BUG=444230
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_perf_bisect
Committed: https://crrev.com/e8eb3a8423c7f0a44033b66c98ffa16eaa65aac5
Cr-Commit-Position: refs/heads/master@{#339807}
Patch Set 1 #
Total comments: 57
Patch Set 2 : Style, error handling fixes and zipped extension_set #
Total comments: 10
Patch Set 3 : Formatting + fix to update_remote_extensions #
Total comments: 86
Patch Set 4 : Style and whitespace fixes. #
Total comments: 15
Patch Set 5 : More style fixes. #
Total comments: 9
Patch Set 6 : Version and hash for CRX stored in CSV. Also updated OS checking for profile extenders. #Patch Set 7 : Extension profile extender properly waits for extensions to install. #
Total comments: 5
Patch Set 8 : Comments added to extension_set.csv and small style fixes. #
Total comments: 18
Patch Set 9 : Fixing docstrings and some style. #
Total comments: 3
Patch Set 10 : Removed redundant code in extensions_profile_extender.py #
Messages
Total messages: 56 (14 generated)
erikchen@chromium.org changed reviewers: + erikchen@chromium.org
https://codereview.chromium.org/1240703003/diff/1/tools/perf/benchmarks/start... File tools/perf/benchmarks/start_with_ext.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/benchmarks/start... tools/perf/benchmarks/start_with_ext.py:12: """Base Benchmark for testing startup with extensions. """ extra whitespace after "." https://codereview.chromium.org/1240703003/diff/1/tools/perf/benchmarks/start... tools/perf/benchmarks/start_with_ext.py:34: """Measure time to start Chrome cold with extensions""" period at end of sentence. ditto on line 45. https://codereview.chromium.org/1240703003/diff/1/tools/perf/page_sets/extens... File tools/perf/page_sets/extension_profile_shared_state.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/page_sets/extens... tools/perf/page_sets/extension_profile_shared_state.py:17: out_dir = generator.Run(finder_options) Does this directory end up leaking? Does it magically get cached between runs? I see that you're just copying the logic from session_restore.py, but I wouldn't blindly trust that class. https://codereview.chromium.org/1240703003/diff/1/tools/perf/page_sets/extens... tools/perf/page_sets/extension_profile_shared_state.py:18: print out_dir Did you mean to use a log statement here? https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:12: from catapult_base import cloud_storage these imports should be alphabetically ordered. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:15: # extensions from missing a period. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:16: BUCKET_NAME = "chrome-partner-telemetry" you shouldn't need to define this variable. Use the variable from cloud_storage. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:17: DEFAULT_REMOTE_DIR = "extension_set" DEFAULT implies remote_dir can be changed by someone to be something else. Can it? https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:32: self._navigation_urls = urls It might not be simpler, but it makes more sense to subclass ProfileExtender directly instead of FastNavigationProfileExtender. I guess I don't really care which way you choose. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:35: finder_options.browser_options.profile_dir = tempfile.mkdtemp() How do you prevent tempfile.mkdtemp() from leaking? https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:36: finder_options.browser_options.disable_default_apps = False I seem to recall that you also set a similar flag in benchmark.py. The flag should only be set in one location. (which is presumbably the benchmark, since it needs to affect both times the browser is started. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:40: local_extensions_dir = os.path.join(profile_dir, "External Extensions Crx") Please don't use spaces in folder names. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:41: self._DownloadRemoteExtensions( This logic should be called from ProfileExtender.Run(), not Init(). https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:49: """ Downloads set of common extensions to disk. """ What exceptions can this function throw? Please document. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:53: if not crx_file.endswith(".crx"): Should this ever happen? If it should never happen, maybe raise an exception? https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:54: continue 2 space indentation https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:59: def _GetExtensionInfoFromCRX(self, crxfile): #TODO: integrity checks? I wouldn't be too worried about integrity checks. Let the exceptions propagate. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:69: """ Loads extensions in _local_extensions_dir into finder_options """ what exceptions can this throw? https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:71: # IF MAC It's okay to start off with this benchmark only running on Mac. Just disable the benchmark on other platforms. https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:75: if not ext_file.endswith('.crx'): ditto - should this ever happen? maybe this should be an assert https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:80: 'external_version': version, formatting of this dictionary - please see python style guide for a reference https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:7: import optparse alphabetize https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:20: BUCKET_NAME = "chrome-partner-telemetry" no need for this variable https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:24: """ Download CRX (Chrome extension file) specified by extension_id from Please see python style guide for formatting of this comment. """This must be a 1-line summary. Long description goes here. Can span multiple lines""" https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:32: return None This seems like a silent failure. I would throw an exception and allow the exception to bubble to the top. Or you could add handling for the return value of this function. Either way, please document in function comment. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:49: # Delete remote directory if it exists You are deleting, and then uploading. This set of operations isn't atomic. What if all of the files are zipped first, and then you just upload the new zip to overwrite the old zip? I believe that cloud storage has guarantees about the atomicity of this operation. aiolos@ would know more. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:72: shutil.rmtree(local_extensions_dir) You probably want to wrap UpdateExtensionsInCloud() in a try/finally so that rmtree is always run afterwords. https://codereview.chromium.org/1240703003/diff/1/tools/telemetry/catapult_ba... File tools/telemetry/catapult_base/cloud_storage.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/telemetry/catapult_ba... tools/telemetry/catapult_base/cloud_storage.py:206: def Delete(bucket, remote_path, recursive=False): If you do a zip upload, you won't need to add this recursive flag.
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240703003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) (exceeded global retry quota)
https://codereview.chromium.org/1240703003/diff/1/tools/perf/benchmarks/start... File tools/perf/benchmarks/start_with_ext.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/benchmarks/start... tools/perf/benchmarks/start_with_ext.py:12: """Base Benchmark for testing startup with extensions. """ On 2015/07/14 23:36:14, erikchen wrote: > extra whitespace after "." Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/benchmarks/start... tools/perf/benchmarks/start_with_ext.py:34: """Measure time to start Chrome cold with extensions""" On 2015/07/14 23:36:14, erikchen wrote: > period at end of sentence. ditto on line 45. Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/page_sets/extens... File tools/perf/page_sets/extension_profile_shared_state.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/page_sets/extens... tools/perf/page_sets/extension_profile_shared_state.py:17: out_dir = generator.Run(finder_options) On 2015/07/14 23:36:14, erikchen wrote: > Does this directory end up leaking? Does it magically get cached between runs? I > see that you're just copying the logic from session_restore.py, but I wouldn't > blindly trust that class. Suppose it's "magically cached" since python's tempfile.tempdir uses the TMPDIR environment variable. Added code to clean up this directory using superclass TearDownState. https://codereview.chromium.org/1240703003/diff/1/tools/perf/page_sets/extens... tools/perf/page_sets/extension_profile_shared_state.py:18: print out_dir On 2015/07/14 23:36:14, erikchen wrote: > Did you mean to use a log statement here? Yeah; although I realize it's redundant since generate_profile also logs this directory. Deleted. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:12: from catapult_base import cloud_storage On 2015/07/14 23:36:14, erikchen wrote: > these imports should be alphabetically ordered. Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:15: # extensions from On 2015/07/14 23:36:14, erikchen wrote: > missing a period. Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:16: BUCKET_NAME = "chrome-partner-telemetry" On 2015/07/14 23:36:14, erikchen wrote: > you shouldn't need to define this variable. Use the variable from cloud_storage. Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:17: DEFAULT_REMOTE_DIR = "extension_set" On 2015/07/14 23:36:14, erikchen wrote: > DEFAULT implies remote_dir can be changed by someone to be something else. Can > it? No, it shouldn't be able to. Changed the name. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:32: self._navigation_urls = urls On 2015/07/14 23:36:14, erikchen wrote: > It might not be simpler, but it makes more sense to subclass ProfileExtender > directly instead of FastNavigationProfileExtender. I guess I don't really care > which way you choose. Stuck with FastNav since it creates and destroys the browser nicely. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:35: finder_options.browser_options.profile_dir = tempfile.mkdtemp() On 2015/07/14 23:36:14, erikchen wrote: > How do you prevent tempfile.mkdtemp() from leaking? Didn't realize profile generator already creates such a directory. Deleted. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:36: finder_options.browser_options.disable_default_apps = False On 2015/07/14 23:36:14, erikchen wrote: > I seem to recall that you also set a similar flag in benchmark.py. The flag > should only be set in one location. (which is presumbably the benchmark, since > it needs to affect both times the browser is started. I set this here as well so the profile generator works (and sets this flag) even when run independently of the benchmark. Not sure how the browser_options object is passed around-- Is there a way I can just set it in the profile gen so I won't have to in benchmark? https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:40: local_extensions_dir = os.path.join(profile_dir, "External Extensions Crx") On 2015/07/14 23:36:14, erikchen wrote: > Please don't use spaces in folder names. Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:41: self._DownloadRemoteExtensions( On 2015/07/14 23:36:14, erikchen wrote: > This logic should be called from ProfileExtender.Run(), not Init(). Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:49: """ Downloads set of common extensions to disk. """ On 2015/07/14 23:36:14, erikchen wrote: > What exceptions can this function throw? Please document. Should document errors that this function explicitly raises, or anything it calls that can fault? on failure of cloud_storage.Get, I'll raise exception and prompt user to run update_remote_extensions https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:53: if not crx_file.endswith(".crx"): On 2015/07/14 23:36:14, erikchen wrote: > Should this ever happen? If it should never happen, maybe raise an exception? Deleted. Check here is redundant with check in _Load https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:54: continue On 2015/07/14 23:36:14, erikchen wrote: > 2 space indentation Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:59: def _GetExtensionInfoFromCRX(self, crxfile): #TODO: integrity checks? On 2015/07/14 23:36:14, erikchen wrote: > I wouldn't be too worried about integrity checks. Let the exceptions propagate. Acknowledged. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:69: """ Loads extensions in _local_extensions_dir into finder_options """ On 2015/07/14 23:36:14, erikchen wrote: > what exceptions can this throw? Any File IO or if extension directory was malformed (i.e. contained non-crx file) https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:71: # IF MAC On 2015/07/14 23:36:14, erikchen wrote: > It's okay to start off with this benchmark only running on Mac. Just disable the > benchmark on other platforms. > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Disabled the benchmark on all platforms except mac. How should I disable the profile generator? Currently have a platform.system() check. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:75: if not ext_file.endswith('.crx'): On 2015/07/14 23:36:14, erikchen wrote: > ditto - should this ever happen? maybe this should be an assert Should definitely never happen. I'll raise an exception and prompt the user to run update_remote_extensions if there's a non-crx file. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:80: 'external_version': version, On 2015/07/14 23:36:14, erikchen wrote: > formatting of this dictionary - please see python style guide for a reference Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:7: import optparse On 2015/07/14 23:36:15, erikchen wrote: > alphabetize Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:20: BUCKET_NAME = "chrome-partner-telemetry" On 2015/07/14 23:36:15, erikchen wrote: > no need for this variable Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:24: """ Download CRX (Chrome extension file) specified by extension_id from On 2015/07/14 23:36:15, erikchen wrote: > Please see python style guide for formatting of this comment. > > """This must be a 1-line summary. > > Long description goes here. > Can span multiple lines""" Done. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:32: return None On 2015/07/14 23:36:15, erikchen wrote: > This seems like a silent failure. I would throw an exception and allow the > exception to bubble to the top. Or you could add handling for the return value > of this function. Either way, please document in function comment. Documented return value and will handle in caller (prints warning message) https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:49: # Delete remote directory if it exists On 2015/07/14 23:36:15, erikchen wrote: > You are deleting, and then uploading. This set of operations isn't atomic. > > What if all of the files are zipped first, and then you just upload the new zip > to overwrite the old zip? I believe that cloud storage has guarantees about the > atomicity of this operation. aiolos@ would know more. Oh, my bad; there's also toctou problem. Will archive files and use a single operation. https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/update_remote_extensions.py:72: shutil.rmtree(local_extensions_dir) On 2015/07/14 23:36:15, erikchen wrote: > You probably want to wrap UpdateExtensionsInCloud() in a try/finally so that > rmtree is always run afterwords. Done. https://codereview.chromium.org/1240703003/diff/1/tools/telemetry/catapult_ba... File tools/telemetry/catapult_base/cloud_storage.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/telemetry/catapult_ba... tools/telemetry/catapult_base/cloud_storage.py:206: def Delete(bucket, remote_path, recursive=False): On 2015/07/14 23:36:15, erikchen wrote: > If you do a zip upload, you won't need to add this recursive flag. Done.
https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/1/tools/perf/profile_creators... tools/perf/profile_creators/extension_profile_extender.py:36: finder_options.browser_options.disable_default_apps = False On 2015/07/15 22:56:23, sydli wrote: > On 2015/07/14 23:36:14, erikchen wrote: > > I seem to recall that you also set a similar flag in benchmark.py. The flag > > should only be set in one location. (which is presumbably the benchmark, since > > it needs to affect both times the browser is started. > > I set this here as well so the profile generator works (and sets this flag) even > when run independently of the benchmark. Not sure how the browser_options object > is passed around-- Is there a way I can just set it in the profile gen so I > won't have to in benchmark? nope, you'll need it in both places. https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:20: class InvalidExtensionArchiveError(Exception): this should inherit from exceptions.error, a telemetry class https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:63: local_zip_path) I think this is incorrectly formatted. https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:66: % (remote_bucket, remote_zip_path)) formatting https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:44: return None nit: too many spaces. https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:72: if crx_path is None: Under this code, it's possible that there's a good zip in the cloud, that the newly generated zip has some failures, but the newly generated zip replaces the clodu zip anyways. That is not good.
https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:20: class InvalidExtensionArchiveError(Exception): On 2015/07/16 00:33:28, erikchen wrote: > this should inherit from exceptions.error, a telemetry class Done. https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:63: local_zip_path) On 2015/07/16 00:33:28, erikchen wrote: > I think this is incorrectly formatted. Done. Probably did this wrong everywhere else, so fixing those too. https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:66: % (remote_bucket, remote_zip_path)) On 2015/07/16 00:33:28, erikchen wrote: > formatting Done. https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:44: return None On 2015/07/16 00:33:28, erikchen wrote: > nit: too many spaces. Done. https://codereview.chromium.org/1240703003/diff/20001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:72: if crx_path is None: On 2015/07/16 00:33:28, erikchen wrote: > Under this code, it's possible that there's a good zip in the cloud, that the > newly generated zip has some failures, but the newly generated zip replaces the > clodu zip anyways. That is not good. I'll raise an error instead. In the case that one of the extensions can no longer be downloaded from CWS and the dl fails consistently (actually-- are there cases where this happens?), the CSV must be manually updated. Will put this in error message.
erikchen@chromium.org changed reviewers: + robliao@chromium.org
+robliao lgtm
Here's a first pass on the CL. I've mostly just read it for style at this point. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/start_with_ext.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_ext.py:12: """Base Benchmark for testing startup with extensions. """ Nit: Benchmark -> benchmark https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_ext.py:30: Add extra linebreak here (2 Blank Lines at Top Level Definitions) http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank_... https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_ext.py:41: return 'start_with_ext.cold.blank_page' Tabs: 2 spaces here. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_ext.py:54: return 'start_with_ext.warm.blank_page' Tabs: 2 spaces here https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/bl... File tools/perf/page_sets/blank_page_with_extension_profile.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/bl... tools/perf/page_sets/blank_page_with_extension_profile.py:10: class BlankPageWithExtensionProfile(page_module.Page): Add a docstring to this toplevel class. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/bl... tools/perf/page_sets/blank_page_with_extension_profile.py:15: ExtensionProfileSharedState) This should line up with the extension_profile_shared_state above. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/ex... File tools/perf/page_sets/extension_profile_shared_state.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/ex... tools/perf/page_sets/extension_profile_shared_state.py:13: def __init__(self, test, finder_options, story_set): Add a docstring to this class. (See section under classes) http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank_... https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/ex... tools/perf/page_sets/extension_profile_shared_state.py:18: "extension_profile") Consistency: This delta appears to be preferring single-quotes for strings thus far. Use that here. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/ex... tools/perf/page_sets/extension_profile_shared_state.py:26: """ Clean up generated profile directory. """ Remove padding spaces in docstring. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:14: from telemetry.core import exceptions This import looks like it comes from us. This should be grouped with the previous set of imports. http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Import... https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:18: # Name of zip archive to download. Nit: Add a linebreak above here. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:23: """ Exception thrown when remote archive is invalid or malformed. Remove leading whitespace in docstring. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:26: Prompts user to update remote archive using update_remote_extensions Nit: Lowercase prompts. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:27: script. """ The """ should go on the next line for multiline docstrings. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:30: msg += "\nTry running" + \ Use single-quoted strings and ()'s to span the expression. Add the \n at the end instead so that it reads... msg += ('\nTry running\n' '\tpython update_remote..\n' ...) https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:38: """Creates a profile with many extensions. """ Removing trailing whitespace in docstring. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:45: urls = [] This can be urls = [story.url for story in self._page_set.stories] https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:52: """ Downloads archive of common extensions to disk and unzips. Remove leading whitespace. Maybe Downloads and unzips an archive of common extensions to disk? https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:56: local_extensions_dir: directory to unzip archive into. This seems to be more about creating an extensions directory rather than just unzipping a general archive. The archive here is an implementation detail. In this case, the argument is self-explanatory and could be local_extensions_dir: Destination extensions directory. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:59: InvalidExtensionArchiveError if cannot find remote archive. InvalidExtensionArchiveError if the remote archive is not found. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:65: local_zip_path) This looks like it should fit on the previous line. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:67: raise InvalidExtensionArchiveError("Can't find archive at gs://%s/%s." Change the quotes here to single quotes. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:69: with zipfile.ZipFile(local_zip_path, "r") as extensions_zip: Use single quoted 'r' https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:74: """ Retrieves version + name of extension from CRX archive. """ Remove surrounding whitespace in docstring. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:83: """ Loads extensions in _local_extensions_dir into user profile. Remove leading whitespace. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:86: https://developer.chrome.com/extensions/external_extensions.html . Optional: Arguably, you could omit the period at the end of this line. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:89: local_extensions_dir: directory containing *CRX files. Nit: *.crx or just CRX files. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:90: profile_dir: user profile directory to load extensions into. Target profile directory for the extensions. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:96: external_ext_dir = os.path.join(profile_dir, "External Extensions") Use single quotes if possible, from here and forwards :-). https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:105: 'external_crx': ext_path, Two spaces for indentation here. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:125: raise NotImplementedError('Extension profile generator on %s is not yet ' Nit optional: If this string works all on the next line, it would be more readable. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:21: # Remote directory to upload extensions to in cloud storage. Could be Remote target upload directory in cloud storage for extensions. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:23: # Name of zip archive to write to Linebreak above, add period. Could also be target zip file. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:26: def _DownloadCRXFromCWS(ext_id, dst): Should probably be _DownloadCrxFromCws since we don't capitalize HTTP in this project. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:27: """ Downloads CRX specified from Chrome Web Store. Remove leading whitespace. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:29: Retrieves CRX (Chrome extension file) sepcified by ext_id from Chrome Web specified https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:38: If download fails, return None. """ Remove trailing whitespace. Also the """ should go on the next line for multiline docstrings. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:51: """ Updates set of extensions in Cloud Storage from CSV of extension ids. Remove leading white space. Nit: From a csv https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:64: Exception if a CRX download fails. """ Move """ to the next line. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:86: def _GetCSVFromArgs(): This should probably be _GetCsvFromArgs. If you can find other evidence of CAPS use in function names, then this can stay. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:87: """ Parse options to retrieve name of CSV file. """ Remove leading and trailing whitespace in the docstring. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:105: if __name__=="__main__": Add a around the ==
Style/whitespace fixes. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... File tools/perf/benchmarks/start_with_ext.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_ext.py:12: """Base Benchmark for testing startup with extensions. """ On 2015/07/16 18:33:23, robliao wrote: > Nit: Benchmark -> benchmark Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_ext.py:30: On 2015/07/16 18:33:23, robliao wrote: > Add extra linebreak here (2 Blank Lines at Top Level Definitions) > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank_... Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_ext.py:41: return 'start_with_ext.cold.blank_page' On 2015/07/16 18:33:23, robliao wrote: > Tabs: 2 spaces here. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/benchmarks/s... tools/perf/benchmarks/start_with_ext.py:54: return 'start_with_ext.warm.blank_page' On 2015/07/16 18:33:23, robliao wrote: > Tabs: 2 spaces here Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/bl... File tools/perf/page_sets/blank_page_with_extension_profile.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/bl... tools/perf/page_sets/blank_page_with_extension_profile.py:10: class BlankPageWithExtensionProfile(page_module.Page): On 2015/07/16 18:33:23, robliao wrote: > Add a docstring to this toplevel class. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/bl... tools/perf/page_sets/blank_page_with_extension_profile.py:15: ExtensionProfileSharedState) On 2015/07/16 18:33:23, robliao wrote: > This should line up with the extension_profile_shared_state above. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/ex... File tools/perf/page_sets/extension_profile_shared_state.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/ex... tools/perf/page_sets/extension_profile_shared_state.py:13: def __init__(self, test, finder_options, story_set): On 2015/07/16 18:33:24, robliao wrote: > Add a docstring to this class. (See section under classes) > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Blank_... Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/ex... tools/perf/page_sets/extension_profile_shared_state.py:18: "extension_profile") On 2015/07/16 18:33:24, robliao wrote: > Consistency: This delta appears to be preferring single-quotes for strings thus > far. Use that here. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/page_sets/ex... tools/perf/page_sets/extension_profile_shared_state.py:26: """ Clean up generated profile directory. """ On 2015/07/16 18:33:23, robliao wrote: > Remove padding spaces in docstring. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:14: from telemetry.core import exceptions On 2015/07/16 18:33:24, robliao wrote: > This import looks like it comes from us. This should be grouped with the > previous set of imports. > http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Import... Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:18: # Name of zip archive to download. On 2015/07/16 18:33:25, robliao wrote: > Nit: Add a linebreak above here. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:23: """ Exception thrown when remote archive is invalid or malformed. On 2015/07/16 18:33:24, robliao wrote: > Remove leading whitespace in docstring. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:26: Prompts user to update remote archive using update_remote_extensions On 2015/07/16 18:33:24, robliao wrote: > Nit: Lowercase prompts. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:27: script. """ On 2015/07/16 18:33:24, robliao wrote: > The """ should go on the next line for multiline docstrings. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:30: msg += "\nTry running" + \ On 2015/07/16 18:33:24, robliao wrote: > Use single-quoted strings and ()'s to span the expression. Add the \n at the end > instead so that it reads... > msg += ('\nTry running\n' > '\tpython update_remote..\n' > ...) Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:38: """Creates a profile with many extensions. """ On 2015/07/16 18:33:24, robliao wrote: > Removing trailing whitespace in docstring. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:45: urls = [] On 2015/07/16 18:33:24, robliao wrote: > This can be > urls = [story.url for story in self._page_set.stories] Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:52: """ Downloads archive of common extensions to disk and unzips. On 2015/07/16 18:33:24, robliao wrote: > Remove leading whitespace. > Maybe Downloads and unzips an archive of common extensions to disk? Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:56: local_extensions_dir: directory to unzip archive into. On 2015/07/16 18:33:24, robliao wrote: > This seems to be more about creating an extensions directory rather than just > unzipping a general archive. > The archive here is an implementation detail. In this case, the argument is > self-explanatory and could be > local_extensions_dir: Destination extensions directory. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:59: InvalidExtensionArchiveError if cannot find remote archive. On 2015/07/16 18:33:24, robliao wrote: > InvalidExtensionArchiveError if the remote archive is not found. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:65: local_zip_path) On 2015/07/16 18:33:25, robliao wrote: > This looks like it should fit on the previous line. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:67: raise InvalidExtensionArchiveError("Can't find archive at gs://%s/%s." On 2015/07/16 18:33:24, robliao wrote: > Change the quotes here to single quotes. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:69: with zipfile.ZipFile(local_zip_path, "r") as extensions_zip: On 2015/07/16 18:33:24, robliao wrote: > Use single quoted 'r' Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:74: """ Retrieves version + name of extension from CRX archive. """ On 2015/07/16 18:33:24, robliao wrote: > Remove surrounding whitespace in docstring. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:83: """ Loads extensions in _local_extensions_dir into user profile. On 2015/07/16 18:33:25, robliao wrote: > Remove leading whitespace. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:86: https://developer.chrome.com/extensions/external_extensions.html . On 2015/07/16 18:33:24, robliao wrote: > Optional: Arguably, you could omit the period at the end of this line. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:89: local_extensions_dir: directory containing *CRX files. On 2015/07/16 18:33:24, robliao wrote: > Nit: *.crx or just CRX files. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:90: profile_dir: user profile directory to load extensions into. On 2015/07/16 18:33:24, robliao wrote: > Target profile directory for the extensions. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:96: external_ext_dir = os.path.join(profile_dir, "External Extensions") On 2015/07/16 18:33:24, robliao wrote: > Use single quotes if possible, from here and forwards :-). Yep! Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:105: 'external_crx': ext_path, On 2015/07/16 18:33:24, robliao wrote: > Two spaces for indentation here. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:125: raise NotImplementedError('Extension profile generator on %s is not yet ' On 2015/07/16 18:33:24, robliao wrote: > Nit optional: If this string works all on the next line, it would be more > readable. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:21: # Remote directory to upload extensions to in cloud storage. On 2015/07/16 18:33:25, robliao wrote: > Could be Remote target upload directory in cloud storage for extensions. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:23: # Name of zip archive to write to On 2015/07/16 18:33:25, robliao wrote: > Linebreak above, add period. Could also be target zip file. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:26: def _DownloadCRXFromCWS(ext_id, dst): On 2015/07/16 18:33:25, robliao wrote: > Should probably be _DownloadCrxFromCws since we don't capitalize HTTP in this > project. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:27: """ Downloads CRX specified from Chrome Web Store. On 2015/07/16 18:33:25, robliao wrote: > Remove leading whitespace. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:29: Retrieves CRX (Chrome extension file) sepcified by ext_id from Chrome Web On 2015/07/16 18:33:25, robliao wrote: > specified Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:38: If download fails, return None. """ On 2015/07/16 18:33:25, robliao wrote: > Remove trailing whitespace. Also the """ should go on the next line for > multiline docstrings. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:51: """ Updates set of extensions in Cloud Storage from CSV of extension ids. On 2015/07/16 18:33:25, robliao wrote: > Remove leading white space. > Nit: From a csv Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:64: Exception if a CRX download fails. """ On 2015/07/16 18:33:25, robliao wrote: > Move """ to the next line. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:86: def _GetCSVFromArgs(): On 2015/07/16 18:33:25, robliao wrote: > This should probably be _GetCsvFromArgs. > > If you can find other evidence of CAPS use in function names, then this can > stay. I'll just uncaps the function names from here on out. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:87: """ Parse options to retrieve name of CSV file. """ On 2015/07/16 18:33:25, robliao wrote: > Remove leading and trailing whitespace in the docstring. Done. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:105: if __name__=="__main__": On 2015/07/16 18:33:25, robliao wrote: > Add a around the == Done.
Looks generally good. A few comments on the second pass. https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:105: 'external_crx': ext_path, On 2015/07/16 20:01:30, sydli wrote: > On 2015/07/16 18:33:24, robliao wrote: > > Two spaces for indentation here. > > Done. I should have been clearer here. This line should be tabbed two spaces from the statement start column above. e.g. extension_info = { 'external_crx': ext_path ... } https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. 2015 :-) https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:43: maximum_batch_size = 1 Is this local necessary if it's not read in this constructor anymore? The next line could just be maximum_batch_size = 1 https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:69: extensions_zip.extractall(local_extensions_dir) If extractall throws an exception, the local_zip_path will be hanging around. This may need a try-finally. https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:72: def _GetExtensionInfoFromCRX(self, crxfile): Lowercase CRX https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:102: (version, name) = self._GetExtensionInfoFromCRX(ext_path) Do we need to be robust to failures if the CRX file contains bad contents? https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:122: # DL extensions from cloud & force-install extensions into profile. Nit DL -> Download https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:70: with open(extensions_csv, 'rb') as csvfile: csv_file
https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/40001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:105: 'external_crx': ext_path, On 2015/07/16 21:10:04, robliao wrote: > On 2015/07/16 20:01:30, sydli wrote: > > On 2015/07/16 18:33:24, robliao wrote: > > > Two spaces for indentation here. > > > > Done. > > I should have been clearer here. This line should be tabbed two spaces from the > statement start column above. e.g. > extension_info = { > 'external_crx': ext_path > ... > } Done. https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:1: # Copyright 2016 The Chromium Authors. All rights reserved. On 2015/07/16 21:10:04, robliao wrote: > 2015 :-) Done. https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:43: maximum_batch_size = 1 On 2015/07/16 21:10:04, robliao wrote: > Is this local necessary if it's not read in this constructor anymore? The next > line could just be maximum_batch_size = 1 Good catch-- done. https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:69: extensions_zip.extractall(local_extensions_dir) On 2015/07/16 21:10:04, robliao wrote: > If extractall throws an exception, the local_zip_path will be hanging around. > This may need a try-finally. Done. https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:72: def _GetExtensionInfoFromCRX(self, crxfile): On 2015/07/16 21:10:04, robliao wrote: > Lowercase CRX Done. https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:102: (version, name) = self._GetExtensionInfoFromCRX(ext_path) On 2015/07/16 21:10:04, robliao wrote: > Do we need to be robust to failures if the CRX file contains bad contents? I don't think so. Re-running the update script should be sufficient, as long as we can trust CRX given by the Webstore. Even so, is there a good way to check the integrity of a CRX archive? https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:122: # DL extensions from cloud & force-install extensions into profile. On 2015/07/16 21:10:04, robliao wrote: > Nit DL -> Download Done. https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:70: with open(extensions_csv, 'rb') as csvfile: On 2015/07/16 21:10:04, robliao wrote: > csv_file Done.
lgtm. Should be good to go for OWNER approval. https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/60001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:102: (version, name) = self._GetExtensionInfoFromCRX(ext_path) On 2015/07/17 01:00:29, sydli wrote: > On 2015/07/16 21:10:04, robliao wrote: > > Do we need to be robust to failures if the CRX file contains bad contents? > > I don't think so. Re-running the update script should be sufficient, as long as > we can trust CRX given by the Webstore. Even so, is there a good way to check > the integrity of a CRX archive? I don't know of any offhand. Rerunning the script sounds good. Might be useful to suggest that in an error message.
sydli@google.com changed reviewers: + nednguyen@google.com
nednguyen@google.com: Please review changes in extension profile generator. Also, suggestions for issues you mentioned in design doc? Currently just re-enable disable_default_apps for profile generator and benchmark.
On 2015/07/17 21:25:13, sydli wrote: > mailto:nednguyen@google.com: Please review changes in extension profile generator. > Also, suggestions for issues you mentioned in design doc? Currently just > re-enable disable_default_apps for profile generator and benchmark. We have a policy for adding new benchmarks here: https://docs.google.com/document/d/1bBKyYCW3VlUUPDpQE4xvrMFdA6tovQMZoqO9KCcmq... Can you demonstrate an example CL which shows your benchmark regress clearly?
https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:124: if platform.system() != 'Darwin': This part is not correct, the platform telemetry is hosted on & the platform the test will be run on can be two different things. +Erik: can help you with querying the right platform at the right time. https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:30: Store, into directory specified by dst. Does this CRX file have a fixed version?
On 2015/07/17 21:47:09, nednguyen wrote: > On 2015/07/17 21:25:13, sydli wrote: > > mailto:nednguyen@google.com: Please review changes in extension profile > generator. > > Also, suggestions for issues you mentioned in design doc? Currently just > > re-enable disable_default_apps for profile generator and benchmark. > > We have a policy for adding new benchmarks here: > https://docs.google.com/document/d/1bBKyYCW3VlUUPDpQE4xvrMFdA6tovQMZoqO9KCcmq... > > Can you demonstrate an example CL which shows your benchmark regress clearly? I don't know of an example CL that this benchmark shows a regression for. However, this is not a new benchmark. (A version of) this benchmark used to exist, but because it was poorly written, quickly became out-dated and broken. If you still have a problem with the addition of this benchmark, let me know and I will discuss this with Annie.
https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:124: if platform.system() != 'Darwin': On 2015/07/17 21:48:07, nednguyen wrote: > This part is not correct, the platform telemetry is hosted on & the platform the > test will be run on can be two different things. > > +Erik: can help you with querying the right platform at the right time. Would this be the correct query? https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:30: Store, into directory specified by dst. On 2015/07/17 21:48:07, nednguyen wrote: > Does this CRX file have a fixed version? No, should it be? update_remote_extensions will not be run very often, so any generated profile will have consistent extension versions.
https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:124: if platform.system() != 'Darwin': On 2015/07/17 22:29:27, sydli wrote: > On 2015/07/17 21:48:07, nednguyen wrote: > > This part is not correct, the platform telemetry is hosted on & the platform > the > > test will be run on can be two different things. > > > > +Erik: can help you with querying the right platform at the right time. > > Would this be the correct query? > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Yeah, but you need to get the pointer to platform. https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:30: Store, into directory specified by dst. On 2015/07/17 22:29:27, sydli wrote: > On 2015/07/17 21:48:07, nednguyen wrote: > > Does this CRX file have a fixed version? > > No, should it be? update_remote_extensions will not be run very often, so any > generated profile will have consistent extension versions. I see. In case someone runs "update_remote_extensions", would these binary be update silently? Your design should make sure that any changing parts in the system is done through a CL. That would allow easy rollback when anything goes wrong & the bisect bot can blame the CL if it causes regression. My suggestion is that you should also hash the extension binary and make it part of the keys in _DownloadExtensionFromCloud(key)
https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:124: if platform.system() != 'Darwin': On 2015/07/17 22:29:27, sydli wrote: > On 2015/07/17 21:48:07, nednguyen wrote: > > This part is not correct, the platform telemetry is hosted on & the platform > the > > test will be run on can be two different things. > > > > +Erik: can help you with querying the right platform at the right time. > > Would this be the correct query? > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... Yeah, you want to use the platform of the possible_browser. e.g. https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...
https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/extension_profile_extender.py:124: if platform.system() != 'Darwin': On 2015/07/17 22:50:33, erikchen wrote: > On 2015/07/17 22:29:27, sydli wrote: > > On 2015/07/17 21:48:07, nednguyen wrote: > > > This part is not correct, the platform telemetry is hosted on & the platform > > the > > > test will be run on can be two different things. > > > > > > +Erik: can help you with querying the right platform at the right time. > > > > Would this be the correct query? > > > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... > > Yeah, you want to use the platform of the possible_browser. > e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te... done; additions to profile extender to restrict the platform of possible_browser. does this look reasonable? https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/80001/tools/perf/profile_crea... tools/perf/profile_creators/update_remote_extensions.py:30: Store, into directory specified by dst. On 2015/07/17 22:49:08, nednguyen wrote: > On 2015/07/17 22:29:27, sydli wrote: > > On 2015/07/17 21:48:07, nednguyen wrote: > > > Does this CRX file have a fixed version? > > > > No, should it be? update_remote_extensions will not be run very often, so any > > generated profile will have consistent extension versions. > > I see. In case someone runs "update_remote_extensions", would these binary be > update silently? Your design should make sure that any changing parts in the > system is done through a CL. That would allow easy rollback when anything goes > wrong & the bisect bot can blame the CL if it causes regression. > > My suggestion is that you should also hash the extension binary and make it part > of the keys in _DownloadExtensionFromCloud(key) Done; not sure if this is exactly what you meant, but now stores CRX hash + version in CSV. If anything goes wrong, can inspect CL for individual extension version/hash changes in CSV.
lgtm https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_set.csv (right): https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... tools/perf/profile_creators/extension_set.csv:1: extension_name,id,hash,version Can you add add a comment in the top of this file that says this file is generated by update_remote_extensions.py and shouldn't be edit manually?
https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_set.csv (right): https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... tools/perf/profile_creators/extension_set.csv:1: extension_name,id,hash,version On 2015/07/20 23:07:54, nednguyen wrote: > Can you add add a comment in the top of this file that says this file is > generated by update_remote_extensions.py and shouldn't be edit manually? Will add instructions to manually add/remove entire extensions (in case an extension become deprecated) and warn against altering individual fields. Is there any convention for commenting CSV files?
https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_set.csv (right): https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... tools/perf/profile_creators/extension_set.csv:1: extension_name,id,hash,version On 2015/07/20 23:26:27, sydli wrote: > On 2015/07/20 23:07:54, nednguyen wrote: > > Can you add add a comment in the top of this file that says this file is > > generated by update_remote_extensions.py and shouldn't be edit manually? > > Will add instructions to manually add/remove entire extensions (in case an > extension become deprecated) and warn against altering individual fields. > Is there any convention for commenting CSV files? Not really. https://code.google.com/p/chromium/codesearch#search/&q=file:.csv$&sq=package... Use your best judgement.
https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_set.csv (right): https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... tools/perf/profile_creators/extension_set.csv:1: extension_name,id,hash,version On 2015/07/20 23:26:27, sydli wrote: > On 2015/07/20 23:07:54, nednguyen wrote: > > Can you add add a comment in the top of this file that says this file is > > generated by update_remote_extensions.py and shouldn't be edit manually? > > Will add instructions to manually add/remove entire extensions (in case an > extension become deprecated) and warn against altering individual fields. > Is there any convention for commenting CSV files? We don't. It's up to you for how to handle this. Maybe just let the parser skip the beginning lines that start with "#" ?
https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_set.csv (right): https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... tools/perf/profile_creators/extension_set.csv:1: extension_name,id,hash,version On 2015/07/20 23:30:58, nednguyen wrote: > On 2015/07/20 23:26:27, sydli wrote: > > On 2015/07/20 23:07:54, nednguyen wrote: > > > Can you add add a comment in the top of this file that says this file is > > > generated by update_remote_extensions.py and shouldn't be edit manually? > > > > Will add instructions to manually add/remove entire extensions (in case an > > extension become deprecated) and warn against altering individual fields. > > Is there any convention for commenting CSV files? > > We don't. It's up to you for how to handle this. Maybe just let the parser skip > the beginning lines that start with "#" ? Done.
On 2015/07/21 19:27:01, sydli wrote: > https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... > File tools/perf/profile_creators/extension_set.csv (right): > > https://codereview.chromium.org/1240703003/diff/120001/tools/perf/profile_cre... > tools/perf/profile_creators/extension_set.csv:1: extension_name,id,hash,version > On 2015/07/20 23:30:58, nednguyen wrote: > > On 2015/07/20 23:26:27, sydli wrote: > > > On 2015/07/20 23:07:54, nednguyen wrote: > > > > Can you add add a comment in the top of this file that says this file is > > > > generated by update_remote_extensions.py and shouldn't be edit manually? > > > > > > Will add instructions to manually add/remove entire extensions (in case an > > > extension become deprecated) and warn against altering individual fields. > > > Is there any convention for commenting CSV files? > > > > We don't. It's up to you for how to handle this. Maybe just let the parser > skip > > the beginning lines that start with "#" ? > > Done. patch set 7 lgtm
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, nednguyen@google.com Link to the patchset: https://codereview.chromium.org/1240703003/#ps140001 (title: "Comments added to extension_set.csv and small style fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240703003/140001
The CQ bit was unchecked by erikchen@chromium.org
https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:47: # Download extensions from cloud & force-install extensions into profile. Nit: & -> and https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:130: """ Stall until browser has finished installing/loading all extensions. """ Remove leading and trailing whitespace from doc. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:132: time.sleep(5) Comment why we're using a sleep instead of waiting for an event. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:133: exists_unloaded_extension = False exists_unloaded_extension looks unnecessary. You can break after self.browser.extensions.GetByExtensionId and continue on KeyError. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... File tools/perf/profile_creators/profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/profile_extender.py:71: Style: In Python classes, defs get one space. Yeah, it's fun and inconsistent, but that's the rule. https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Blank_Lines https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/update_remote_extensions.py:28: def _DownloadCrxFromCws(ext_id, dst): Two linebreaks above functions from here and forwards. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/update_remote_extensions.py:77: # skip comments and header line We're no longer skipping the comments and header line if we're keeping them around! Add that we may be using them to update the file. What happens to the column headers line? Looks like that gets put into extension_info below. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/update_remote_extensions.py:146: """Updates CSV with information in extensions_info. This docstring needs argument docs. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/update_remote_extensions.py:172: """Retrieves extension version from CRX archive.""" This docstring needs argument docs. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/update_remote_extensions.py:179: """Calculates base64 hash of item at file_path""" Arguably this satisfies... * not externally visible * very short * obvious So the docstring can be omitted. https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme...
https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:47: # Download extensions from cloud & force-install extensions into profile. On 2015/07/21 19:49:52, robliao wrote: > Nit: & -> and Done. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:130: """ Stall until browser has finished installing/loading all extensions. """ On 2015/07/21 19:49:52, robliao wrote: > Remove leading and trailing whitespace from doc. Done. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:132: time.sleep(5) On 2015/07/21 19:49:51, robliao wrote: > Comment why we're using a sleep instead of waiting for an event. Done. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:133: exists_unloaded_extension = False On 2015/07/21 19:49:51, robliao wrote: > exists_unloaded_extension looks unnecessary. > You can break after self.browser.extensions.GetByExtensionId and continue on > KeyError. Switched the loops as per your suggestion. Cleaner now! Done. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... File tools/perf/profile_creators/profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/profile_extender.py:71: On 2015/07/21 19:49:52, robliao wrote: > Style: In Python classes, defs get one space. Yeah, it's fun and inconsistent, > but that's the rule. > https://google-styleguide.googlecode.com/svn/trunk/pyguide.html#Blank_Lines Done. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/update_remote_extensions.py:28: def _DownloadCrxFromCws(ext_id, dst): On 2015/07/21 19:49:52, robliao wrote: > Two linebreaks above functions from here and forwards. Done. https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/update_remote_extensions.py:77: # skip comments and header line On 2015/07/21 19:49:52, robliao wrote: > We're no longer skipping the comments and header line if we're keeping them > around! Add that we may be using them to update the file. > > What happens to the column headers line? Looks like that gets put into > extension_info below. Updated the comment. The headers line gets skipped; once the final '#' line is added to comments, reader.next() is called again before the loop terminates.
https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... File tools/perf/profile_creators/update_remote_extensions.py (right): https://codereview.chromium.org/1240703003/diff/140001/tools/perf/profile_cre... tools/perf/profile_creators/update_remote_extensions.py:77: # skip comments and header line On 2015/07/21 20:33:04, sydli wrote: > On 2015/07/21 19:49:52, robliao wrote: > > We're no longer skipping the comments and header line if we're keeping them > > around! Add that we may be using them to update the file. > > > > What happens to the column headers line? Looks like that gets put into > > extension_info below. > > Updated the comment. The headers line gets skipped; once the final '#' line is > added to comments, reader.next() is called again before the loop terminates. Ah, I see it now. https://codereview.chromium.org/1240703003/diff/160001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/160001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:140: break This break can go into the try, no? That allows you to remove the continue in the except branch.
https://codereview.chromium.org/1240703003/diff/160001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/160001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:140: break On 2015/07/21 20:39:51, robliao wrote: > This break can go into the try, no? That allows you to remove the continue in > the except branch. Yep! Done.
lgtm https://codereview.chromium.org/1240703003/diff/160001/tools/perf/profile_cre... File tools/perf/profile_creators/extension_profile_extender.py (right): https://codereview.chromium.org/1240703003/diff/160001/tools/perf/profile_cre... tools/perf/profile_creators/extension_profile_extender.py:140: break On 2015/07/21 20:44:51, sydli wrote: > On 2015/07/21 20:39:51, robliao wrote: > > This break can go into the try, no? That allows you to remove the continue in > > the except branch. > > Yep! Done. Sweet. Much nicer than what I originally proposed.
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, erikchen@chromium.org Link to the patchset: https://codereview.chromium.org/1240703003/#ps180001 (title: "Removed redundant code in extensions_profile_extender.py")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240703003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240703003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_nexus5_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL) win_perf_bisect on tryserver.chromium.perf (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1240703003/180001
On 2015/07/21 23:17:37, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1240703003/180001 I removed android_nexus5_perf_bisect & win_perf_bisect from the CQ_EXTRA_TRYBOTS since they are just timed out to unblock this patch.
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/e8eb3a8423c7f0a44033b66c98ffa16eaa65aac5 Cr-Commit-Position: refs/heads/master@{#339807} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
