Extension profile generator + benchmark for startup with profile.

tools/perf/profile_creators/extension_profile_extender.py

+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import json
+import os
+import platform
+import zipfile
+
+from catapult_base import cloud_storage
+from profile_creators import fast_navigation_profile_extender
+import page_sets
+
+from telemetry.core import exceptions

[robliao, 2015/07/16 18:33:24]

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...

[sydli, 2015/07/16 20:01:29]

Done.

+
+# Remote directory to download extensions from in cloud storage.
+REMOTE_DIR = "extension_set"
+# Name of zip archive to download.

[robliao, 2015/07/16 18:33:25]

Nit: Add a linebreak above here.

[sydli, 2015/07/16 20:01:30]

Done.

+ZIP_NAME = "extensions.zip"
+
+
+class InvalidExtensionArchiveError(exceptions.Error):
+  """ Exception thrown when remote archive is invalid or malformed.

[robliao, 2015/07/16 18:33:24]

Remove leading whitespace in docstring.

[sydli, 2015/07/16 20:01:30]

Done.

+
+  Remote archive should be located at REMOTE_DIR/ZIP_NAME. Upon failure,
+  Prompts user to update remote archive using update_remote_extensions

[robliao, 2015/07/16 18:33:24]

Nit: Lowercase prompts.

[sydli, 2015/07/16 20:01:29]

Done.

+  script. """

[robliao, 2015/07/16 18:33:24]

The """ should go on the next line for multiline docstrings.

[sydli, 2015/07/16 20:01:30]

Done.

+
+  def __init__(self, msg=''):
+    msg += "\nTry running" + \

[robliao, 2015/07/16 18:33:24]

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'
        ...)

[sydli, 2015/07/16 20:01:29]

Done.

+           "\n\tpython update_remote_extensions.py -e extension_set.csv\n" + \
+           "in src/tools/perf/profile_creator subdirectory."
+    super(InvalidExtensionArchiveError, self).__init__(msg)
+
+
+class ExtensionProfileExtender(
+      fast_navigation_profile_extender.FastNavigationProfileExtender):
+  """Creates a profile with many extensions. """

[robliao, 2015/07/16 18:33:24]

Removing trailing whitespace in docstring.

[sydli, 2015/07/16 20:01:29]

Done.

+
+  def __init__(self, finder_options):
+    maximum_batch_size = 1
+    super(ExtensionProfileExtender, self).__init__(finder_options,
+                                                   maximum_batch_size)
+    self._page_set = page_sets.BlankPageSet()
+    urls = []

[robliao, 2015/07/16 18:33:24]

This can be
urls = [story.url for story in self._page_set.stories]

[sydli, 2015/07/16 20:01:30]

Done.

+    for story in self._page_set.stories:
+      urls.append(story.url)
+    self._navigation_urls = urls
+    finder_options.browser_options.disable_default_apps = False
+
+  def _DownloadRemoteExtensions(self, remote_bucket, local_extensions_dir):
+    """ Downloads archive of common extensions to disk and unzips.

[robliao, 2015/07/16 18:33:24]

Remove leading whitespace.
Maybe Downloads and unzips an archive of common extensions to disk?

[sydli, 2015/07/16 20:01:30]

Done.

+
+    Args:
+        remote_bucket: bucket to download remote archive from.
+        local_extensions_dir: directory to unzip archive into.

[robliao, 2015/07/16 18:33:24]

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.

[sydli, 2015/07/16 20:01:30]

Done.

+
+    Raises:
+        InvalidExtensionArchiveError if cannot find remote archive.

[robliao, 2015/07/16 18:33:24]

InvalidExtensionArchiveError if the remote archive is not found.

[sydli, 2015/07/16 20:01:29]

Done.

+    """
+    remote_zip_path = os.path.join(REMOTE_DIR, ZIP_NAME)
+    local_zip_path = os.path.join(local_extensions_dir, ZIP_NAME)
+    try:
+      cloud_storage.Get(remote_bucket, remote_zip_path,
+                        local_zip_path)

[robliao, 2015/07/16 18:33:25]

This looks like it should fit on the previous line.

[sydli, 2015/07/16 20:01:30]

Done.

+    except:
+      raise InvalidExtensionArchiveError("Can't find archive at gs://%s/%s."

[robliao, 2015/07/16 18:33:24]

Change the quotes here to single quotes.

[sydli, 2015/07/16 20:01:30]

Done.

+                                         % (remote_bucket, remote_zip_path))
+    with zipfile.ZipFile(local_zip_path, "r") as extensions_zip:

[robliao, 2015/07/16 18:33:24]

Use single quoted 'r'

[sydli, 2015/07/16 20:01:29]

Done.

+      extensions_zip.extractall(local_extensions_dir)
+    os.remove(local_zip_path)
+
+  def _GetExtensionInfoFromCRX(self, crxfile):
+    """ Retrieves version + name of extension from CRX archive. """

[robliao, 2015/07/16 18:33:24]

Remove surrounding whitespace in docstring.

[sydli, 2015/07/16 20:01:30]

Done.

+    crx_zip = zipfile.ZipFile(crxfile)
+    manifest_contents = crx_zip.read('manifest.json')
+    decoded_manifest = json.loads(manifest_contents)
+    crx_version = decoded_manifest['version']
+    extension_name = decoded_manifest['name']
+    return (crx_version, extension_name)
+
+  def _LoadExtensions(self, local_extensions_dir, profile_dir):
+    """ Loads extensions in _local_extensions_dir into user profile.

[robliao, 2015/07/16 18:33:25]

Remove leading whitespace.

[sydli, 2015/07/16 20:01:30]

Done.

+
+    Extensions are loaded according to platform specifications at
+    https://developer.chrome.com/extensions/external_extensions.html .

[robliao, 2015/07/16 18:33:24]

Optional: Arguably, you could omit the period at the end of this line.

[sydli, 2015/07/16 20:01:29]

Done.

+
+    Args:
+        local_extensions_dir: directory containing *CRX files.

[robliao, 2015/07/16 18:33:24]

Nit: *.crx or just CRX files.

[sydli, 2015/07/16 20:01:30]

Done.

+        profile_dir: user profile directory to load extensions into.

[robliao, 2015/07/16 18:33:24]

Target profile directory for the extensions.

[sydli, 2015/07/16 20:01:29]

Done.

+
+    Raises:
+        InvalidExtensionArchiveError if archive contains a non-CRX file.
+    """
+    ext_files = os.listdir(local_extensions_dir)
+    external_ext_dir = os.path.join(profile_dir, "External Extensions")

[robliao, 2015/07/16 18:33:24]

Use single quotes if possible, from here and forwards :-).

[sydli, 2015/07/16 20:01:29]

Yep! Done.

+    os.makedirs(external_ext_dir)
+    for ext_file in ext_files:
+      ext_path = os.path.join(local_extensions_dir, ext_file)
+      if not ext_file.endswith(".crx"):
+        raise InvalidExtensionArchiveError("Archive contains non-crx file %s."
+                                           % ext_file)
+      (version, name) = self._GetExtensionInfoFromCRX(ext_path)
+      extension_info = {
+              'external_crx': ext_path,

[robliao, 2015/07/16 18:33:24]

Two spaces for indentation here.

[sydli, 2015/07/16 20:01:30]

Done.

[robliao, 2015/07/16 21:10:04]

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
      ...
    }

[sydli, 2015/07/17 01:00:28]

Done.

+              'external_version': version,
+              '_comment': name
+          }
+      ext_id = os.path.splitext(os.path.basename(ext_path))[0]
+      extension_json_path = os.path.join(external_ext_dir, "%s.json" % ext_id)
+      with open(extension_json_path, 'w') as f:
+        f.write(json.dumps(extension_info))
+
+  def GetUrlIterator(self):
+    """Superclass override."""
+    return iter(self._navigation_urls)
+
+  def ShouldExitAfterBatchNavigation(self):
+    """Superclass override."""
+    return False
+
+  def Run(self):
+    # DL extensions from cloud & force-install extensions into profile.
+    if platform.system() != 'Darwin':
+      raise NotImplementedError('Extension profile generator on %s is not yet '

[robliao, 2015/07/16 18:33:24]

Nit optional: If this string works all on the next line, it would be more
readable.

[sydli, 2015/07/16 20:01:29]

Done.

+        'supported' % platform.system())
+    local_extensions_dir = os.path.join(self.profile_path,
+                                        "external_extensions_crx")
+    self._DownloadRemoteExtensions(cloud_storage.PARTNER_BUCKET,
+                                   local_extensions_dir)
+    self._LoadExtensions(local_extensions_dir, self.profile_path)
+    super(ExtensionProfileExtender, self).Run()
+