Compile Android using GRIT's new --no-output-all-resource-defines flag.

Compile Android using GRIT's new --no-output-all-resource-defines flag.

This flag causes GRIT to include the same resources in generated .h
files as are included in the PAK files. This seems like an obvious thing
to do, right? Well, before this CL, *every* resource from the GRD file
was listed in the .h file, even resources inside of <if> statements that
evaluated to false. As a result, it was possible to reference a resource
(e.g. IDS_FOO) in a .cc file, even if IDS_FOO was excluded from the PAK
file. What happened at runtime? Crash!

This change brings compile-time safety to resource loading: the crash
mentioned above would now be a compile failure since IDS_FOO is no
longer in the generated .h file.

BUG=428947
Committed: https://crrev.com/0548dfae799a5423abd007f2f9d8ea0a1d5d571b
Cr-Commit-Position: refs/heads/master@{#303180}

[lliabraa, 2014-10-30 13:45:12]

lliabraa@chromium.org changed reviewers:
+ lliabraa@chromium.org

[lliabraa, 2014-10-30 13:45:13]

Thanks for putting this together. I've launches some tryjobs to see if this
affects other platforms.

[newt (away), 2014-10-30 16:29:58]

On 2014/10/30 13:45:13, lliabraa wrote:
> Thanks for putting this together. I've launches some tryjobs to see if this
> affects other platforms.

On Android, resources_util_unittest.cc needs to be updated so it doesn't
reference excluded resources.

On the desktop platforms, looks like there are more issues that need
investigating.

[lliabraa, 2014-11-03 13:16:50]

On 2014/10/30 16:29:58, newt wrote:
> On 2014/10/30 13:45:13, lliabraa wrote:
> > Thanks for putting this together. I've launches some tryjobs to see if this
> > affects other platforms.
> 
> On Android, resources_util_unittest.cc needs to be updated so it doesn't
> reference excluded resources.
> 
> On the desktop platforms, looks like there are more issues that need
> investigating.

Once support for --no-output-all-resource-defines lands
(https://chromiumcodereview.appspot.com/691873002/) you'll want to set that for
android in the GRIT_DEFINES in gyp instead of modifying the .grd files.

[newt (away), 2014-11-06 09:13:03]

newt@chromium.org changed reviewers:
+ vitalybuka@chromium.org

[newt (away), 2014-11-06 09:13:03]

vitalybuka: Could you look at my question in
chrome/renderer/printing/print_web_view_helper.cc ?

https://codereview.chromium.org/692733003/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/692733003/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:474:
0));//IDR_PRINT_PREVIEW_PAGE));
IDR_PRINT_PREVIEW_PAGE is defined in chrome/browser/browser_resources.grd inside
an if tag:

    <if expr="enable_print_preview">
      ...
      <include name="IDR_PRINT_PREVIEW_PAGE" ...>
    </if>

This implies that IDR_PRINT_PREVIEW_PAGE is only needed if enable_print_preview
is true. Yet this code is compiled on Android (where enable_print_preview is
false), and it references IDR_PRINT_PREVIEW_PAGE.

vitalybuka: Can you explain this?

[Vitaly Buka (NO REVIEWS), 2014-11-06 09:38:02]

https://codereview.chromium.org/692733003/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/692733003/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:474:
0));//IDR_PRINT_PREVIEW_PAGE));
PrintWebViewHelper::PrintHeaderAndFooter should not be called on android.

You can wrap entire PrintWebViewHelper::PrintHeaderAndFooter here and in *h file
in #if defined(ENABLE_PRINT_PREVIEW)

On 2014/11/06 09:13:03, newt wrote:
> IDR_PRINT_PREVIEW_PAGE is defined in chrome/browser/browser_resources.grd
inside
> an if tag:
> 
>     <if expr="enable_print_preview">
>       ...
>       <include name="IDR_PRINT_PREVIEW_PAGE" ...>
>     </if>
> 
> This implies that IDR_PRINT_PREVIEW_PAGE is only needed if
enable_print_preview
> is true. Yet this code is compiled on Android (where enable_print_preview is
> false), and it references IDR_PRINT_PREVIEW_PAGE.
> 
> vitalybuka: Can you explain this?

[Vitaly Buka (NO REVIEWS), 2014-11-06 09:41:50]

On 2014/11/06 09:38:02, Vitaly Buka wrote:
>
https://codereview.chromium.org/692733003/diff/40001/chrome/renderer/printing...
> File chrome/renderer/printing/print_web_view_helper.cc (right):
> 
>
https://codereview.chromium.org/692733003/diff/40001/chrome/renderer/printing...
> chrome/renderer/printing/print_web_view_helper.cc:474:
> 0));//IDR_PRINT_PREVIEW_PAGE));
> PrintWebViewHelper::PrintHeaderAndFooter should not be called on android.
> 
> You can wrap entire PrintWebViewHelper::PrintHeaderAndFooter here and in *h
file
> in #if defined(ENABLE_PRINT_PREVIEW)
> 
> On 2014/11/06 09:13:03, newt wrote:
> > IDR_PRINT_PREVIEW_PAGE is defined in chrome/browser/browser_resources.grd
> inside
> > an if tag:
> > 
> >     <if expr="enable_print_preview">
> >       ...
> >       <include name="IDR_PRINT_PREVIEW_PAGE" ...>
> >     </if>
> > 
> > This implies that IDR_PRINT_PREVIEW_PAGE is only needed if
> enable_print_preview
> > is true. Yet this code is compiled on Android (where enable_print_preview is
> > false), and it references IDR_PRINT_PREVIEW_PAGE.
> > 
> > vitalybuka: Can you explain this?

also call itself in print_web_view_helper_linux.cc
only print preview sets params.params.display_header_footer

[Vitaly Buka (NO REVIEWS), 2014-11-06 09:48:19]

https://codereview.chromium.org/692733003/diff/40001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/692733003/diff/40001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:474:
0));//IDR_PRINT_PREVIEW_PAGE));
On 2014/11/06 09:38:02, Vitaly Buka wrote:
> PrintWebViewHelper::PrintHeaderAndFooter should not be called on android.
> 
> You can wrap entire PrintWebViewHelper::PrintHeaderAndFooter here and in *h
file
> in #if defined(ENABLE_PRINT_PREVIEW)
> 
> On 2014/11/06 09:13:03, newt wrote:
> > IDR_PRINT_PREVIEW_PAGE is defined in chrome/browser/browser_resources.grd
> inside
> > an if tag:
> > 
> >     <if expr="enable_print_preview">
> >       ...
> >       <include name="IDR_PRINT_PREVIEW_PAGE" ...>
> >     </if>
> > 
> > This implies that IDR_PRINT_PREVIEW_PAGE is only needed if
> enable_print_preview
> > is true. Yet this code is compiled on Android (where enable_print_preview is
> > false), and it references IDR_PRINT_PREVIEW_PAGE.
> > 
> > vitalybuka: Can you explain this?
> 

or just disable PrintWebViewHelper::PrintHeaderAndFooter body

[lliabraa, 2014-11-06 19:08:37]

LGTM pending fix in print_web_view_helper.cc

[newt (away), 2014-11-06 19:14:42]

vitalybuka: Thanks for the suggestions. PTAL.

[Vitaly Buka (NO REVIEWS), 2014-11-06 19:34:24]

lgtm

https://codereview.chromium.org/692733003/diff/60001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/692733003/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:75: 
why not to combine both blocks?

[newt (away), 2014-11-06 19:50:38]

https://codereview.chromium.org/692733003/diff/60001/chrome/renderer/printing...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/692733003/diff/60001/chrome/renderer/printing...
chrome/renderer/printing/print_web_view_helper.cc:75: 
On 2014/11/06 19:34:24, Vitaly Buka wrote:
> why not to combine both blocks?

Done.

[newt (away), 2014-11-06 19:56:47]

newt@chromium.org changed reviewers:
+ oshima@chromium.org, samarth@chromium.org, thakis@chromium.org

[newt (away), 2014-11-06 19:56:47]

samarth: chrome/browser/search/instant_service.cc
oshima: ui/resources/ui_resources.grd
thakis: chrome/browser/resources_util_unittest.cc

PTAL. Thanks!

[Nico, 2014-11-06 19:59:53]

chrome/browser/resources_util_unittest.cc lgtm

[oshima, 2014-11-06 20:00:16]

ui/resources lgtm

[newt (away), 2014-11-07 02:34:30]

The CQ bit was checked by newt@chromium.org

[commit-bot: I haz the power, 2014-11-07 02:35:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/692733003/80001

[commit-bot: I haz the power, 2014-11-07 03:39:18]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-07 03:40:44]

Patchset 5 (id:??) landed as
https://crrev.com/0548dfae799a5423abd007f2f9d8ea0a1d5d571b
Cr-Commit-Position: refs/heads/master@{#303180}