GRIT: Enable variable expansion in filenames during HTML inlining.

GRIT: Enable variable expansion in filenames during HTML inlining.

BUG=232048
R=flackr@chromium.org

Committed: https://code.google.com/p/grit-i18n/source/detail?r=125

[dconnelly, 2013-06-06 10:28:44]

Hey guys, sorry this took so long.

Additionally I'm not at all sure of the approach I've taken here.  Any advice
would be greatly appreciated!

[Jói, 2013-06-06 13:21:37]

This looks fine to me, didn't notice anything off, but flackr@ should give the
approval, I think he knows this part of the code much better than I do.

Cheers,
Jói

[flackr, 2013-06-06 14:07:19]

Looks good, just a few comments.

https://codereview.chromium.org/16539002/diff/1/grit/format/html_inline.py
File grit/format/html_inline.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/format/html_inline.py#ne...
grit/format/html_inline.py:92: filename = filename.replace(DIST_SUBSTR ,
distribution)
It looks to me like this won't do the filename replacement. You probably need to
pass the filename replacement function to this function or call GetFilePath for
each file result (probably better since it would use the same code).

Can you add an image to the test to verify?

https://codereview.chromium.org/16539002/diff/1/grit/format/html_inline_unitt...
File grit/format/html_inline_unittest.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/format/html_inline_unitt...
grit/format/html_inline_unittest.py:244: self.failUnlessEqual(resources,
source_resources)
It might be worth adding a test which calls DoInline with names_only=True and
testing this as well since the behavior when not actually inlining the data is
subtly different.

https://codereview.chromium.org/16539002/diff/1/grit/node/structure.py
File grit/node/structure.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/node/structure.py#newcod...
grit/node/structure.py:115: skel.SetFilenameExpansionFunction(self._Substitute)
Hmm, we should be calling SetDefines here as well I think.

[flackr, 2013-06-06 14:10:26]

https://codereview.chromium.org/16539002/diff/1/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/gather/chrome_html.py#ne...
grit/gather/chrome_html.py:81: filename = filename.replace(DIST_SUBSTR,
distribution)
Should do filename substitution here as well so that it works in conjunction
with multiple scale factors.

[dconnelly, 2013-06-07 09:10:38]

https://codereview.chromium.org/16539002/diff/1/grit/format/html_inline.py
File grit/format/html_inline.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/format/html_inline.py#ne...
grit/format/html_inline.py:92: filename = filename.replace(DIST_SUBSTR ,
distribution)
On 2013/06/06 14:07:19, flackr wrote:
> It looks to me like this won't do the filename replacement. You probably need
to
> pass the filename replacement function to this function or call GetFilePath
for
> each file result (probably better since it would use the same code).
> 
> Can you add an image to the test to verify?

Good catch, thanks.  I added the function parameter because src_match is an re
match object and not a string, so it can't be expanded before it's passed in.

https://codereview.chromium.org/16539002/diff/1/grit/format/html_inline_unitt...
File grit/format/html_inline_unittest.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/format/html_inline_unitt...
grit/format/html_inline_unittest.py:244: self.failUnlessEqual(resources,
source_resources)
On 2013/06/06 14:07:19, flackr wrote:
> It might be worth adding a test which calls DoInline with names_only=True and
> testing this as well since the behavior when not actually inlining the data is
> subtly different.

Done.

https://codereview.chromium.org/16539002/diff/1/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/gather/chrome_html.py#ne...
grit/gather/chrome_html.py:81: filename = filename.replace(DIST_SUBSTR,
distribution)
On 2013/06/06 14:10:26, flackr wrote:
> Should do filename substitution here as well so that it works in conjunction
> with multiple scale factors.

Done.

https://codereview.chromium.org/16539002/diff/1/grit/node/structure.py
File grit/node/structure.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/node/structure.py#newcod...
grit/node/structure.py:115: skel.SetFilenameExpansionFunction(self._Substitute)
On 2013/06/06 14:07:19, flackr wrote:
> Hmm, we should be calling SetDefines here as well I think.

Done.

[flackr, 2013-06-07 14:50:40]

Looks good, just one last request, thanks!

https://codereview.chromium.org/16539002/diff/1/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/gather/chrome_html.py#ne...
grit/gather/chrome_html.py:81: filename = filename.replace(DIST_SUBSTR,
distribution)
On 2013/06/07 09:10:38, dconnelly wrote:
> On 2013/06/06 14:10:26, flackr wrote:
> > Should do filename substitution here as well so that it works in conjunction
> > with multiple scale factors.
> 
> Done.

Can you add a test case to chrome_html_unittest.py?

[dconnelly, 2013-06-07 16:00:40]

https://codereview.chromium.org/16539002/diff/1/grit/gather/chrome_html.py
File grit/gather/chrome_html.py (right):

https://codereview.chromium.org/16539002/diff/1/grit/gather/chrome_html.py#ne...
grit/gather/chrome_html.py:81: filename = filename.replace(DIST_SUBSTR,
distribution)
On 2013/06/07 14:50:40, flackr wrote:
> On 2013/06/07 09:10:38, dconnelly wrote:
> > On 2013/06/06 14:10:26, flackr wrote:
> > > Should do filename substitution here as well so that it works in
conjunction
> > > with multiple scale factors.
> > 
> > Done.
> 
> Can you add a test case to chrome_html_unittest.py?

Done, and found two more cases.

[flackr, 2013-06-07 16:54:29]

LGTM

[Joao da Silva, 2013-06-10 08:19:22]

GRIT: Enable variable expansion in filenames during HTML inlining.

BUG=232048

[Joao da Silva, 2013-06-10 08:22:44]

Committed patchset #5 manually as r125 (presubmit successful).