Add chrome_html gatherer, which inlines html and automatically generates image set…

Add chrome_html gatherer, which inlines html and generates image sets.

In Chromium we want to have high DPI assets in WebUI dependent on the platform on which we are compiling. To accomplish this we will specify the set of scale factors and search for matching image names in bottom-level directories.

BUG=chromium:125315
TEST=All unittests still pass, chrome_html_unittest

[flackr, 2012-05-17 13:18:52]

Hi Joi,

This is far from committing, but I was hoping you could take a look and let me
know if this is generally the right idea. A few notes:
- structure nodes did not normally generate data pack output as far as I could
tell, so I have added the means to do so but I will have to make sure that this
does not normally happen.
- I made heavy use of the code in grit/format/html_inline.py, I plan to refactor
this to share the same code allowing specifying extra modifications that happen
to HTML / CSS (in this case the image-set insertion).
- I will of course remove the scale_factors global in place of an environment or
message parameter as you suggested.

Anyways, let me know if this is roughly what you were thinking. Thanks!

[tony, 2012-05-17 17:57:25]

The only structure we use in chrome is used by chrome frame (2 dialogs).  They
don't end up in pak files because mac/linux wouldn't use them.

In this case, it looks like you just want to add an HTML file.  Can you use an
<include> for this?

https://chromiumcodereview.appspot.com/10386189/diff/1/grit/gather/chrome_htm...
File grit/gather/chrome_html.py (right):

https://chromiumcodereview.appspot.com/10386189/diff/1/grit/gather/chrome_htm...
grit/gather/chrome_html.py:6: """Prepares a Chrome HTML file by inlining
resources and adding references to high DPI resources.
Why can't we reuse html_inline.py?  Can we just add the image-set handling to
that file?

[Jói, 2012-05-17 19:39:11]

Rob:  In general this looks good to me, see a couple of comments below.  I will
want to see unit tests before it lands, but then I think you just skipped those
for now to show me a preview.

Tony:  The way grit is designed, <include> nodes aren't supposed to do any
transformation of the stuff they reference, <structure> nodes are designed for
that.  As it stands, <include> nodes have no built-in way to extend them to
multiple types (of processing) whereas <structure> nodes do, via the type
attribute.  So I actually asked Rob to clean up the inline HTML stuff and bake
it into this gatherer that handles Chrome's specific type of HTML file that does
not require extraction of translatables (although we might want that later) and
does inlining of CSS elements and possibly expands them to refer to multiple DPI
levels of resources.

Cheers,
Jói

https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/gather/chrome_...
File grit/gather/chrome_html.py (right):

https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/gather/chrome_...
grit/gather/chrome_html.py:6: """Prepares a Chrome HTML file by inlining
resources and adding references to high DPI resources.
80 char limit

https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/gather/chrome_...
grit/gather/chrome_html.py:29: 
two blank lines between top-level elements (here and elsewhere)

https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/gather/chrome_...
grit/gather/chrome_html.py:75: '''Represents an HTML document.'''
Maybe extend this to note the functionality of this type, and that you do not
extract any translateable messages (most gatherers do, and generate translated
versions of the resources they reference).

https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/node/structure.py
File grit/node/structure.py (right):

https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/node/structure...
grit/node/structure.py:92: 'scale_factors' : '',
I think we had discussed either figuring out the scale factors in the Python
implementation of chrome_html.py based on preprocessor defines, or passing in an
explicit preprocessor define that sets the scale factors.  You had mentioned a
goal of not having to specify the scale factors per item or maybe even per file,
and a preprocessor flag that gets set by GYP seems ideal for that.

[tony, 2012-05-17 20:13:50]

The new patch that reuses html_inline looks much better to me.

Just for my own understanding, can you give an example of a structure with
different 'type' values and what grit will generate?


On 2012/05/17 19:39:11, Jói wrote:
> Rob:  In general this looks good to me, see a couple of comments below.  I
will
> want to see unit tests before it lands, but then I think you just skipped
those
> for now to show me a preview.
> 
> Tony:  The way grit is designed, <include> nodes aren't supposed to do any
> transformation of the stuff they reference, <structure> nodes are designed for
> that.  As it stands, <include> nodes have no built-in way to extend them to
> multiple types (of processing) whereas <structure> nodes do, via the type
> attribute.  So I actually asked Rob to clean up the inline HTML stuff and bake
> it into this gatherer that handles Chrome's specific type of HTML file that
does
> not require extraction of translatables (although we might want that later)
and
> does inlining of CSS elements and possibly expands them to refer to multiple
DPI
> levels of resources.
> 
> Cheers,
> Jói
> 
>
https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/gather/chrome_...
> File grit/gather/chrome_html.py (right):
> 
>
https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/gather/chrome_...
> grit/gather/chrome_html.py:6: """Prepares a Chrome HTML file by inlining
> resources and adding references to high DPI resources.
> 80 char limit
> 
>
https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/gather/chrome_...
> grit/gather/chrome_html.py:29: 
> two blank lines between top-level elements (here and elsewhere)
> 
>
https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/gather/chrome_...
> grit/gather/chrome_html.py:75: '''Represents an HTML document.'''
> Maybe extend this to note the functionality of this type, and that you do not
> extract any translateable messages (most gatherers do, and generate translated
> versions of the resources they reference).
> 
>
https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/node/structure.py
> File grit/node/structure.py (right):
> 
>
https://chromiumcodereview.appspot.com/10386189/diff/5001/grit/node/structure...
> grit/node/structure.py:92: 'scale_factors' : '',
> I think we had discussed either figuring out the scale factors in the Python
> implementation of chrome_html.py based on preprocessor defines, or passing in
an
> explicit preprocessor define that sets the scale factors.  You had mentioned a
> goal of not having to specify the scale factors per item or maybe even per
file,
> and a preprocessor flag that gets set by GYP seems ideal for that.

[flackr, 2012-05-22 21:25:02]

http://codereview.chromium.org/10386189/diff/5001/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

http://codereview.chromium.org/10386189/diff/5001/grit/gather/chrome_html.py#...
grit/gather/chrome_html.py:6: """Prepares a Chrome HTML file by inlining
resources and adding references to high DPI resources.
On 2012/05/17 19:39:12, Jói wrote:
> 80 char limit

Done.

http://codereview.chromium.org/10386189/diff/5001/grit/gather/chrome_html.py#...
grit/gather/chrome_html.py:29: 
On 2012/05/17 19:39:12, Jói wrote:
> two blank lines between top-level elements (here and elsewhere)

Done.

http://codereview.chromium.org/10386189/diff/5001/grit/gather/chrome_html.py#...
grit/gather/chrome_html.py:75: '''Represents an HTML document.'''
On 2012/05/17 19:39:12, Jói wrote:
> Maybe extend this to note the functionality of this type, and that you do not
> extract any translateable messages (most gatherers do, and generate translated
> versions of the resources they reference).

Done.

http://codereview.chromium.org/10386189/diff/5001/grit/node/structure.py
File grit/node/structure.py (right):

http://codereview.chromium.org/10386189/diff/5001/grit/node/structure.py#newc...
grit/node/structure.py:92: 'scale_factors' : '',
On 2012/05/17 19:39:12, Jói wrote:
> I think we had discussed either figuring out the scale factors in the Python
> implementation of chrome_html.py based on preprocessor defines, or passing in
an
> explicit preprocessor define that sets the scale factors.  You had mentioned a
> goal of not having to specify the scale factors per item or maybe even per
file,
> and a preprocessor flag that gets set by GYP seems ideal for that.

Yes, I hadn't gotten that far yet. Done now :-).

[Jói, 2012-05-23 10:22:46]

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:6: '''Prepares a Chrome HTML file by inlining
resources and adding references to
For triple quotes, we prefer """ (in grit at least, and I think this is Google
style as well).  For regular quotes, prefer ' but can use " if it helps you
avoid escaping.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:9: This is a small script that takes a HTML file,
looks for src attributes
small script -> small gatherer (?)

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:45: src_match: regex match object with 'filename'
named capturing group
I would suggest making the regex a lazy_re.compile'd regex at the top level, so
that you can refer to it here, i.e. that the matches come from that regex or one
with the same groups.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:65: if filename.find(':') != -1:
why not do 
if filename.find('http://') != -1 and filename.find('https://') != -1:
  ...

(I presume we're not worried about Gopher URLs ;-) although if we do need to
match e.g. ftp URLs we could use a regex to check)

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:76: scale_image_path = "%s/%s/%s" % (scale_path[0],
sc, scale_path[1])
Have you tested this on Windows?  A more platform-agnostic way to do it would be

scale_image_path = os.path.join(scale_path[0], sc, scale_path[1])

I guess in CSS we probably always use / but just a thought.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:78: scale_image_name =
re.sub('(?P<path>(.*/)?)(?P<file>[^/]*)',
Again here you may need to be careful with / vs. \ and might want to use
os.path.join.  If this is not a practical concern, a line of documentation
somewhere above like 'CSS always uses forward slashed paths' might make sense.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:86: '''Helper function that inlines external images
in CSS backgrounds.'''
I think this docstring is copy-pasted and needs to be updated.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:102: translateable messages and instead generates a
single DataPack resource.'''
For multi-line docstrings, prefer to put the closing """ on a separate line.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:112: self.scale_factors_ = []
Should this include 100 right off the bat? I guess for that case we don't add an
image set so maybe not?

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html_un...
File grit/gather/chrome_html_unittest.py (right):

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html_un...
grit/gather/chrome_html_unittest.py:27: dir_path = file_path.rsplit('/', 1)[0]
I think you want os.path.split

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html_un...
grit/gather/chrome_html_unittest.py:41: os.removedirs(name)
This could fail depending on the order of directories named in your test data,
e.g. you might have the deepest path first (so it gets deleted last but at that
point no longer exists).  Not a big deal, just pointing it out in case it wasn't
intended.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html_un...
grit/gather/chrome_html_unittest.py:194: 
two blank lines between top-level elements

http://codereview.chromium.org/10386189/diff/14002/grit/node/structure.py
File grit/node/structure.py (right):

http://codereview.chromium.org/10386189/diff/14002/grit/node/structure.py#new...
grit/node/structure.py:124: def GetDataPackPair(self, lang, encoding):
Should document that the second item of the pair may be None.

[flackr, 2012-05-23 19:34:14]

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:6: '''Prepares a Chrome HTML file by inlining
resources and adding references to
On 2012/05/23 10:22:46, Jói wrote:
> For triple quotes, we prefer """ (in grit at least, and I think this is Google
> style as well).  For regular quotes, prefer ' but can use " if it helps you
> avoid escaping.

Done, thanks.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:9: This is a small script that takes a HTML file,
looks for src attributes
On 2012/05/23 10:22:46, Jói wrote:
> small script -> small gatherer (?)

Done.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:45: src_match: regex match object with 'filename'
named capturing group
On 2012/05/23 10:22:46, Jói wrote:
> I would suggest making the regex a lazy_re.compile'd regex at the top level,
so
> that you can refer to it here, i.e. that the matches come from that regex or
one
> with the same groups.

Done.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:65: if filename.find(':') != -1:
On 2012/05/23 10:22:46, Jói wrote:
> why not do 
> if filename.find('http://') != -1 and filename.find('https://') != -1:
>   ...
> 
> (I presume we're not worried about Gopher URLs ;-) although if we do need to
> match e.g. ftp URLs we could use a regex to check)

This is the same condition as used in html_inline, the code that follows expects
that the filename is a relative file path. We weed to at least worry about
chrome:// as well.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:76: scale_image_path = "%s/%s/%s" % (scale_path[0],
sc, scale_path[1])
On 2012/05/23 10:22:46, Jói wrote:
> Have you tested this on Windows?  A more platform-agnostic way to do it would
be
> 
> scale_image_path = os.path.join(scale_path[0], sc, scale_path[1])
> 
> I guess in CSS we probably always use / but just a thought.

Done. Good point, here we want an OS path but below we want a CSS path. I have
commented the following case as such. I will test on Windows to make sure
everything works as expected.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:78: scale_image_name =
re.sub('(?P<path>(.*/)?)(?P<file>[^/]*)',
On 2012/05/23 10:22:46, Jói wrote:
> Again here you may need to be careful with / vs. \ and might want to use
> os.path.join.  If this is not a practical concern, a line of documentation
> somewhere above like 'CSS always uses forward slashed paths' might make sense.

Done.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:86: '''Helper function that inlines external images
in CSS backgrounds.'''
On 2012/05/23 10:22:46, Jói wrote:
> I think this docstring is copy-pasted and needs to be updated.

Done.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:102: translateable messages and instead generates a
single DataPack resource.'''
On 2012/05/23 10:22:46, Jói wrote:
> For multi-line docstrings, prefer to put the closing """ on a separate line.

Done.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:112: self.scale_factors_ = []
On 2012/05/23 10:22:46, Jói wrote:
> Should this include 100 right off the bat? I guess for that case we don't add
an
> image set so maybe not?

Yes, for 1x/100 we don't add an image set.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html_un...
File grit/gather/chrome_html_unittest.py (right):

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html_un...
grit/gather/chrome_html_unittest.py:27: dir_path = file_path.rsplit('/', 1)[0]
On 2012/05/23 10:22:46, Jói wrote:
> I think you want os.path.split

Done.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html_un...
grit/gather/chrome_html_unittest.py:41: os.removedirs(name)
On 2012/05/23 10:22:46, Jói wrote:
> This could fail depending on the order of directories named in your test data,
> e.g. you might have the deepest path first (so it gets deleted last but at
that
> point no longer exists).  Not a big deal, just pointing it out in case it
wasn't
> intended.

That should be fine. I just didn't want it to leave temporary directories
around.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html_un...
grit/gather/chrome_html_unittest.py:194: 
On 2012/05/23 10:22:46, Jói wrote:
> two blank lines between top-level elements

Done.

http://codereview.chromium.org/10386189/diff/14002/grit/node/structure.py
File grit/node/structure.py (right):

http://codereview.chromium.org/10386189/diff/14002/grit/node/structure.py#new...
grit/node/structure.py:124: def GetDataPackPair(self, lang, encoding):
On 2012/05/23 10:22:46, Jói wrote:
> Should document that the second item of the pair may be None.

Done.

[Jói, 2012-05-23 22:59:54]

LGTM, couple of small nits.

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

http://codereview.chromium.org/10386189/diff/14002/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:65: if filename.find(':') != -1:
On 2012/05/23 19:34:14, flackr wrote:
> On 2012/05/23 10:22:46, Jói wrote:
> > why not do 
> > if filename.find('http://') != -1 and filename.find('https://') != -1:
> >   ...
> > 
> > (I presume we're not worried about Gopher URLs ;-) although if we do need to
> > match e.g. ftp URLs we could use a regex to check)
> 
> This is the same condition as used in html_inline, the code that follows
expects
> that the filename is a relative file path. We weed to at least worry about
> chrome:// as well.

OK.  Fine by me to leave as is.

http://codereview.chromium.org/10386189/diff/19001/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

http://codereview.chromium.org/10386189/diff/19001/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:126: """Returns inlined text of the HTML document"""
End this sentence with a period.

http://codereview.chromium.org/10386189/diff/19001/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:130: """Returns inlined text of the HTML document"""
End this sentence with a period.

[flackr, 2012-05-24 17:52:06]

Also fixed the platform dependent issues after testing on Windows. chrome_html
worked fine but the expected output of chrome_html_unittest did not match due to
newline and reported mime type differences.

http://codereview.chromium.org/10386189/diff/19001/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

http://codereview.chromium.org/10386189/diff/19001/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:126: """Returns inlined text of the HTML document"""
On 2012/05/23 22:59:54, Jói wrote:
> End this sentence with a period.

Done.

http://codereview.chromium.org/10386189/diff/19001/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:130: """Returns inlined text of the HTML document"""
On 2012/05/23 22:59:54, Jói wrote:
> End this sentence with a period.

Done.

[Jói, 2012-05-24 22:13:13]

LGTM