High DPI support for Themes: change chrome_html.py

High DPI support for Themes: change chrome_html.py

Modified processing of Theme URLs. We will now insert the scale factor text
before the first ? instead of just appending it. This is necessary for parsing
to work properly (see ParsePathAndScale in src/ui/webui/web_ui_util.cc).
See also recent CL which modified related *.css and *.js files:
https://codereview.chromium.org/16610002/

Example:
Old: url(chrome://theme/IDR_THEME_NTP_ATTRIBUTION?$1@2x)
New: url(chrome://theme/IDR_THEME_NTP_ATTRIBUTION?@2x$1)

BUG=248189, 133934
R=flackr@chromium.org
TEST=manual
src/tools/grit/grit.py unit
Load a theme that has 1x and 2x images for IDR_THEME_NTP_ATTRIBUTION
and IDR_THEME_NTP_BACKGROUND.

[sschmitz, 2013-06-11 21:52:20]

Rob: I just tried it. Created branch in submodule (tools/grit), edited, local
commit and 'git cl upload'. I hope it's ok.

[flackr, 2013-06-12 01:05:44]

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

https://codereview.chromium.org/15713020/diff/1/grit/gather/chrome_html.py#ne...
grit/gather/chrome_html.py:34: _THEME_SOURCE_WITH_QUESTIONMARK =
lazy_re.compile('chrome://theme/IDR_[A-Z0-9_]*\?')
You should use matching groups in the regex to get the part after the ? if it
appears. i.e.
_THEME_SOURCE =
lazy_re.compile('(?P<baseurl>chrome://theme/IDR_[A-Z0-9_]*)(?P<query>\?[^\']*)?')

Then with the match you should be able to check what (if anything) the 'query'
group matched.

[sschmitz, 2013-06-12 18:00:14]

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

https://codereview.chromium.org/15713020/diff/1/grit/gather/chrome_html.py#ne...
grit/gather/chrome_html.py:34: _THEME_SOURCE_WITH_QUESTIONMARK =
lazy_re.compile('chrome://theme/IDR_[A-Z0-9_]*\?')
On 2013/06/12 01:05:44, flackr wrote:
> You should use matching groups in the regex to get the part after the ? if it
> appears. i.e.
> _THEME_SOURCE =
>
lazy_re.compile('(?P<baseurl>chrome://theme/IDR_[A-Z0-9_]*)(?P<query>\?[^\']*)?')
> 
> Then with the match you should be able to check what (if anything) the 'query'
> group matched. 

Done.

[flackr, 2013-06-13 14:57:24]

LGTM with nits

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

https://codereview.chromium.org/15713020/diff/4001/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:33:
'(?P<baseurl>chrome://theme/IDR_[A-Z0-9_]*)(?P<query>\?[^\']*)?')
Since this is only used to search filename matches I think query can match to
the end of the line instead of looking for not a closing quotation. i.e.
(?P<query>\?.*).

https://codereview.chromium.org/15713020/diff/4001/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:77: scale_filename = theme_match.group('baseurl') +
'@' + scale_factor
nit: in python i think constructing a string with format strings like "%s@%s" %
(arg1, arg2) is preferable to string concatenation.

[flackr, 2013-06-13 15:13:33]

Can you add a test for this too?

[sschmitz, 2013-06-13 16:27:53]

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

https://codereview.chromium.org/15713020/diff/4001/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:33:
'(?P<baseurl>chrome://theme/IDR_[A-Z0-9_]*)(?P<query>\?[^\']*)?')
On 2013/06/13 14:57:24, flackr wrote:
> Since this is only used to search filename matches I think query can match to
> the end of the line instead of looking for not a closing quotation. i.e.
> (?P<query>\?.*).

Done.

https://codereview.chromium.org/15713020/diff/4001/grit/gather/chrome_html.py...
grit/gather/chrome_html.py:77: scale_filename = theme_match.group('baseurl') +
'@' + scale_factor
On 2013/06/13 14:57:24, flackr wrote:
> nit: in python i think constructing a string with format strings like "%s@%s"
%
> (arg1, arg2) is preferable to string concatenation.

Done.

[flackr, 2013-06-13 16:59:17]

Landed in grit as r127. You'll have to manually update the DEPS in the chromium
repository to pull this in.