Add baked-in favicons for default popular sites on NTP

components/ntp_tiles/update_default_sites_resources.py

Index: components/ntp_tiles/update_default_sites_resources.py
diff --git a/components/ntp_tiles/update_default_sites_resources.py b/components/ntp_tiles/update_default_sites_resources.py
new file mode 100755
index 0000000000000000000000000000000000000000..a000a3b3aa5827626460512ed3face6c3ab4ad42
--- /dev/null
+++ b/components/ntp_tiles/update_default_sites_resources.py
@@ -0,0 +1,74 @@
+#!/usr/bin/env python2
+# Copyright 2017 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.
+

[sfiera, 2017/02/16 11:14:23]

py-style: please add __future__ imports:

from __future__ import absolute_import
from __future__ import division
from __future__ import print_function

(you may add additional ones, such as division)

[fhorschig, 2017/02/16 17:06:42]

Done.

+import glob
+import json
+import os
+import sys
+import urllib
+import Image
+from io import BytesIO

[sfiera, 2017/02/16 11:14:23]

py-style: from x import y is not allowed unless y is a package.

[fhorschig, 2017/02/16 17:06:42]

Done.

+
+# This script downloads the default popular sites and large icons associated
+# with it. If an icon is too large, it will get resized in the process.
+
+DEFAULT_POPULAR_SITES = 'https://www.gstatic.com/chrome/ntp/'\

[sfiera, 2017/02/16 11:14:23]

py-style: use parens, not backslashes.

[fhorschig, 2017/02/16 17:06:42]

Done.

+                        'suggested_sites_DEFAULT_5.json'
+LARGE_ICON_KEY = 'large_icon_url'
+SITE_TITLE_KEY = 'title'
+MAXIMAL_SIZE = (196, 196)
+SITE_ICON_NAME = 'icon'

[sfiera, 2017/02/16 11:14:23]

You might prefer `SITE_ICON_FORMAT = "icon%d.png"` so you can use
`SITE_ICON_FORMAT % i` below.

[fhorschig, 2017/02/16 17:06:41]

I like this but now I need a second one for deleting icons.

+NTP_TILES_RESOURCE_PATH = os.path.join(
+    os.path.dirname(os.path.realpath(__file__)), 'resources')
+DEFAULT_POPULAR_SITES_PATH = os.path.join(NTP_TILES_RESOURCE_PATH,
+                                        'default_popular_sites.json')

[sfiera, 2017/02/16 11:14:23]

py-style: alignment

[fhorschig, 2017/02/16 17:06:42]

Done.

+
+def download_popular_sites():

[sfiera, 2017/02/16 11:14:23]

Docstrings, please. For example, it's useful here to know that this function
both writes the file and returns the dict.

[fhorschig, 2017/02/16 17:06:42]

Split the function. (For multiple reasons)

+    print("Downloading popular sites... (" + DEFAULT_POPULAR_SITES + ")")

[sfiera, 2017/02/16 11:14:22]

py-style: Proper indentation is 2 spaces.

[sfiera, 2017/02/16 11:14:23]

py-style: You're switching between ' and " quotes. Either is OK, but please pick
one.

[fhorschig, 2017/02/16 17:06:42]

Done although your guidelines say 4 [1].
Is there a Chromium-specific style?

[1]
http://google.github.io/styleguide/pyguide.html?showone=Indentation#Indentation

[fhorschig, 2017/02/16 17:06:42]

Done.

[sfiera, 2017/02/16 18:54:16]

Oh, apparently 2 space is Google internal but 4 is Google external. We should
use 4 here, then.

[fhorschig, 2017/02/17 16:24:03]

Done.

+    data = json.load(urllib.urlopen(url=DEFAULT_POPULAR_SITES))
+    print("... done. (%d sites found)" % len(data))
+    with open(DEFAULT_POPULAR_SITES_PATH, 'w') as f:
+        json.dump(data, f)
+    print("JSON was written to " + DEFAULT_POPULAR_SITES_PATH)
+    return data
+
+def delete_old_icons():
+    print("Deleting old icons..")
+    for f in glob.glob(os.path.join(NTP_TILES_RESOURCE_PATH,
+                                    SITE_ICON_NAME + '[0-9].png')):
+        os.remove(os.path.join(f))
+    print("... done.")
+
+def download_image_for_popular_site(site):
+    image_response_data = urllib.urlopen(url=site[LARGE_ICON_KEY]).read()

[sfiera, 2017/02/16 11:14:23]

Please use "with …urlopen() as …" to ensure that the file object is closed after
use.

[fhorschig, 2017/02/16 17:06:42]

Done with hack:
https://docs.python.org/2/library/contextlib.html#contextlib.closing

+    return Image.open(BytesIO(image_response_data))
+
+def resize_if_too_large(image):

[sfiera, 2017/02/16 11:14:23]

Have we determined yet if resizing is OK? I would hold off for now. Besides,
resizing a PNG without subsequently optimizing might result in a growth in file
size.

[fhorschig, 2017/02/16 17:06:42]

We have not yet decided. I added a flag to prevent resizing.

+    if image.size > MAXIMAL_SIZE:

[sfiera, 2017/02/16 11:14:22]

This does not mean what you think it means. Tuple comparison is element-wise,
i.e.:

(0, 0) < (0, 1) < (0, 2) < (0, 196) < (1, 0) < (196, 0) < (196, 196)

(however, "x < y < z" actually means what it looks like, unlike C++!)

[fhorschig, 2017/02/16 17:06:42]

I am actually fine with since sizes must be quadratic but made my intention
more clear by comparing only widths.

+        print("... and resizing image from %s to %s ..." %
+              (image.size, MAXIMAL_SIZE));
+        image.thumbnail(MAXIMAL_SIZE, Image.ANTIALIAS)
+
+def malformed_json(site):
+    return not SITE_TITLE_KEY in site or not LARGE_ICON_KEY in site
+
+def main():
+    delete_old_icons()
+    popular_sites = download_popular_sites()
+    for i in range(len(popular_sites)):

[sfiera, 2017/02/16 11:14:23]

for i, site in enumerate(popular_sites):

[fhorschig, 2017/02/16 17:06:42]

Uuuh, nice.

+        site = popular_sites[i]
+        if malformed_json(site):
+          print("Could not download image for malformed entry: " + str(site))

[sfiera, 2017/02/16 11:14:23]

"malformed" is not quote the right term here. Some countries have sites with a
favicon_url instead of a large_icon_url.

Use repr() instead of str() here (or "…: %r" % site)

[fhorschig, 2017/02/16 17:06:42]

Done.

+          continue
+        print("Downloading icon for \"" + site[SITE_TITLE_KEY]) +"\"..."

[sfiera, 2017/02/16 11:14:23]

"Downloading icon for %r..." % site[SITE_TITLE_KEY]

[fhorschig, 2017/02/16 17:06:42]

Done.

+        image = download_image_for_popular_site(site)
+        resize_if_too_large(image)
+        image_name = SITE_ICON_NAME + str(i) + ".png"

[sfiera, 2017/02/16 11:14:22]

In general, using str() looks not very python-y. If you want to put a number in
a string, it's better to use format strings.

[fhorschig, 2017/02/16 17:06:42]

Gone.

+        image.save(os.path.join(NTP_TILES_RESOURCE_PATH, image_name), 'PNG')
+        print("... done.(Stored as " + image_name + ")");

[sfiera, 2017/02/16 11:14:22]

Nit: Space between . and (

[fhorschig, 2017/02/16 17:06:42]

Done.

+
+
+if __name__ == '__main__':
+    main()