Restore <img> src values now that grit converts these.

Restore <img> src values now that grit converts these.

BUG=135179
TEST=Run chrome with --force-device-scale-factor=2 and ensure that high DPI images are used on these pages (i.e. the logo on chrome://chrome/help/)


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148804

[flackr, 2012-07-27 16:20:54]

Hey, I did a search for the bug number, were there any instances you didn't
label? I verified the <img> conversion is working correctly. PTAL, thanks.

[Nico, 2012-07-27 16:29:21]

LGTM

These are all that I changed.

[Dan Beam, 2012-07-27 16:50:04]

drive-by nit for Nico: please don't bypass presubmit hooks (that make others fix
your issue later) and add TODOs with no author (we have checks specifically for
this case -
http://src.chromium.org/viewvc/chrome/trunk/tools/depot_tools/presubmit_canne...,
CheckChangeTodoHasOwner, I guess they're just not running right now...)

[flackr, 2012-07-27 16:57:13]

James, PTAL, thanks!

[Dan Beam, 2012-07-27 17:02:34]

lgtm for OWNERS

[commit-bot: I haz the power, 2012-07-27 17:04:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10830051/1

[commit-bot: I haz the power, 2012-07-27 17:05:00]

Failed to apply patch for chrome/browser/resources/about_version.html:
While running patch -p1 --forward --force;
patching file chrome/browser/resources/about_version.html
Hunk #1 FAILED at 40.
Hunk #2 succeeded at 21 (offset -49 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/resources/about_version.html.rej

[James Hawkins, 2012-07-27 17:16:21]

LGTM

[flackr, 2012-07-27 18:03:45]

Ted, I can't test on android but patchset 3 may fix the issue you mentioned.

Dan, please take another quick look. Thanks!

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
File chrome/browser/resources/about_version.html (right):

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
chrome/browser/resources/about_version.html:26: <img
src="../../app/theme/default_100_percent/%DISTRIBUTION%/product_logo.png">
I'm not sure this is necessary (moved from about_version_android.css), it may
have been because of the error that I fixed in patchset 3.

[Ted C, 2012-07-27 18:18:45]

lgtm w/ the if def for themes

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
File chrome/browser/resources/about_version.html (right):

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
chrome/browser/resources/about_version.html:26: <img
src="../../app/theme/default_100_percent/%DISTRIBUTION%/product_logo.png">
On 2012/07/27 18:03:45, flackr wrote:
> I'm not sure this is necessary (moved from about_version_android.css), it may
> have been because of the error that I fixed in patchset 3.

Sadly it is, android excludes all the theme source in chrome_browser.gypi:4839
or there abouts.

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/ui/w...
File chrome/browser/ui/webui/about_ui.cc (right):

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/ui/w...
chrome/browser/ui/webui/about_ui.cc:1386: // Set up the chrome://theme/ source.
Android doesn't support themes, so can you wrap this in:

#if defined(ENABLE_THEMES)
...

[Dan Beam, 2012-07-27 18:20:59]

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
File chrome/browser/resources/about_version.html (right):

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
chrome/browser/resources/about_version.html:26: <img
src="../../app/theme/default_100_percent/%DISTRIBUTION%/product_logo.png">
On 2012/07/27 18:18:45, Ted C wrote:
> On 2012/07/27 18:03:45, flackr wrote:
> > I'm not sure this is necessary (moved from about_version_android.css), it
may
> > have been because of the error that I fixed in patchset 3.
> 
> Sadly it is, android excludes all the theme source in chrome_browser.gypi:4839
> or there abouts.

Is it possible to use ENABLE_THEMES instead of android here as well, by chance?

[flackr, 2012-07-27 18:24:24]

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
File chrome/browser/resources/about_version.html (right):

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
chrome/browser/resources/about_version.html:26: <img
src="../../app/theme/default_100_percent/%DISTRIBUTION%/product_logo.png">
On 2012/07/27 18:20:59, Dan Beam wrote:
> On 2012/07/27 18:18:45, Ted C wrote:
> > On 2012/07/27 18:03:45, flackr wrote:
> > > I'm not sure this is necessary (moved from about_version_android.css), it
> may
> > > have been because of the error that I fixed in patchset 3.
> > 
> > Sadly it is, android excludes all the theme source in
chrome_browser.gypi:4839
> > or there abouts.
> 
> Is it possible to use ENABLE_THEMES instead of android here as well, by
chance?

I believe grit only knows what we pass it in grit_defines from
build/common.gypi:
http://code.google.com/p/chromium/source/search?q=grit_defines.*android&origq...
but I can verify.

[flackr, 2012-07-27 18:26:28]

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
File chrome/browser/resources/about_version.html (right):

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/reso...
chrome/browser/resources/about_version.html:26: <img
src="../../app/theme/default_100_percent/%DISTRIBUTION%/product_logo.png">
On 2012/07/27 18:24:24, flackr wrote:
> On 2012/07/27 18:20:59, Dan Beam wrote:
> > On 2012/07/27 18:18:45, Ted C wrote:
> > > On 2012/07/27 18:03:45, flackr wrote:
> > > > I'm not sure this is necessary (moved from about_version_android.css),
it
> > may
> > > > have been because of the error that I fixed in patchset 3.
> > > 
> > > Sadly it is, android excludes all the theme source in
> chrome_browser.gypi:4839
> > > or there abouts.
> > 
> > Is it possible to use ENABLE_THEMES instead of android here as well, by
> chance?
> 
> I believe grit only knows what we pass it in grit_defines from
> build/common.gypi:
>
http://code.google.com/p/chromium/source/search?q=grit_defines.*android&origq...
> but I can verify.

In fact I'll just pass enable_themes to grit as well so we can rely on this
instead of checking for android.

[Dan Beam, 2012-07-27 18:31:32]

slgtm w/bug to pass enabled_themes to grit

[flackr, 2012-07-27 18:38:45]

Filed crbug.com/139369 to track passing enable_themes to grit. Thanks.

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/ui/w...
File chrome/browser/ui/webui/about_ui.cc (right):

https://chromiumcodereview.appspot.com/10830051/diff/7002/chrome/browser/ui/w...
chrome/browser/ui/webui/about_ui.cc:1386: // Set up the chrome://theme/ source.
On 2012/07/27 18:18:45, Ted C wrote:
> Android doesn't support themes, so can you wrap this in:
> 
> #if defined(ENABLE_THEMES)
> ...

Done.

[commit-bot: I haz the power, 2012-07-27 18:40:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/flackr@chromium.org/10830051/9010

[commit-bot: I haz the power, 2012-07-27 21:04:33]

Change committed as 148804