Reland of Deduplicate Monochrome locale .paks

Reland of Deduplicate Monochrome locale .paks

Instead of using system webview's resource whitelist, now uses a
generated list of resource IDs that are actually packed into
Webview's locale paks. This fixes the missing strings issue.

Original issue:
https://codereview.chromium.org/2980773002/

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days
ago.
BUG=724110, 742388
Review-Url: https://codereview.chromium.org/2977993002
Cr-Commit-Position: refs/heads/master@{#487176}
Committed: https://chromium.googlesource.com/chromium/src/+/368afac85d2ca5e22efcbb2271c0d379b2fc22ab

[F, 2017-07-14 18:26:06]

Created Reland of Deduplicate Monochrome locale .paks

[F, 2017-07-14 18:26:46]

Not ready for review or relanding. Will update when the fix is in.

[F, 2017-07-14 21:05:18]

Description was changed from

==========
Reland of Deduplicate Monochrome locale .paks (patchset #1 id:1 of
https://codereview.chromium.org/2980773002/ )

Reason for revert:
Will reland with fix

Original issue's description:
> Revert of Deduplicate Monochrome locale .paks (patchset #4 id:180001 of
https://codereview.chromium.org/2933343002/ )
> 
> Reason for revert:
> Chrome is now missing 5 locale strings. The reason is that these strings are
believed to be in WebView locales because they're whitelisted, but since WebView
uses multiple whitelists to pack its locale strings, these strings are actually
not contained by WebView
> 
> Original issue's description:
> > Deduplicate Monochrome locale .paks
> > 
> > This CL removes WebView strings from Chrome locale .paks in Monochrome
> > using a generated resource whitelist. While WebView's locale resource
> > logic remains the same, Chrome in Monochrome now loads a secondary
> > locale pack file as a fallback when a string cannot be found in the
> > primary locale pack file.
> > 
> > This CL reduces binary size by over 400KB.
> > 
> > BUG=724110
> > 
> > Review-Url: https://codereview.chromium.org/2933343002
> > Cr-Commit-Position: refs/heads/master@{#485635}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc...
> 
>
TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=724110, 741798
> 
> Review-Url: https://codereview.chromium.org/2980773002
> Cr-Commit-Position: refs/heads/master@{#486397}
> Committed:
https://chromium.googlesource.com/chromium/src/+/df9f33ffee10b4ffbd6469697bc9...

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=724110, 741798
==========

to

==========
Reland of Deduplicate Monochrome locale .paks (patchset #1 id:1 of
https://codereview.chromium.org/2980773002/ )

Reason for revert:
Instead of using system webview's resource whitelist, now uses a
generated list of resource IDs that are actually packed into
Webview's locale paks. This fixes the missing strings issue.

Original issue's description:
> Revert of Deduplicate Monochrome locale .paks (patchset #4 id:180001
of https://codereview.chromium.org/2933343002/ )
> 
> Reason for revert:
> Chrome is now missing 5 locale strings. The reason is that these
strings are believed to be in WebView locales because they're
whitelisted, but since WebView uses multiple whitelists to pack its
locale strings, these strings are actually not contained by WebView
> 
> Original issue's description:
> > Deduplicate Monochrome locale .paks
> > 
> > This CL removes WebView strings from Chrome locale .paks in
Monochrome
> > using a generated resource whitelist. While WebView's locale
resource
> > logic remains the same, Chrome in Monochrome now loads a secondary
> > locale pack file as a fallback when a string cannot be found in the
> > primary locale pack file.
> > 
> > This CL reduces binary size by over 400KB.
> > 
> > BUG=724110
> > 
> > Review-Url: https://codereview.chromium.org/2933343002
> > Cr-Commit-Position: refs/heads/master@{#485635}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc...
> 
>
TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
> # Not skipping CQ checks because original CL landed more than 1 days
ago.
> BUG=724110, 741798
> 
> Review-Url: https://codereview.chromium.org/2980773002
> Cr-Commit-Position: refs/heads/master@{#486397}
> Committed:
https://chromium.googlesource.com/chromium/src/+/df9f33ffee10b4ffbd6469697bc9...

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
# Not skipping CQ checks because original CL landed more than 1 days
ago.
BUG=724110, 741798
==========

[F, 2017-07-14 21:05:26]

Description was changed from

==========
Reland of Deduplicate Monochrome locale .paks (patchset #1 id:1 of
https://codereview.chromium.org/2980773002/ )

Reason for revert:
Instead of using system webview's resource whitelist, now uses a
generated list of resource IDs that are actually packed into
Webview's locale paks. This fixes the missing strings issue.

Original issue's description:
> Revert of Deduplicate Monochrome locale .paks (patchset #4 id:180001
of https://codereview.chromium.org/2933343002/ )
> 
> Reason for revert:
> Chrome is now missing 5 locale strings. The reason is that these
strings are believed to be in WebView locales because they're
whitelisted, but since WebView uses multiple whitelists to pack its
locale strings, these strings are actually not contained by WebView
> 
> Original issue's description:
> > Deduplicate Monochrome locale .paks
> > 
> > This CL removes WebView strings from Chrome locale .paks in
Monochrome
> > using a generated resource whitelist. While WebView's locale
resource
> > logic remains the same, Chrome in Monochrome now loads a secondary
> > locale pack file as a fallback when a string cannot be found in the
> > primary locale pack file.
> > 
> > This CL reduces binary size by over 400KB.
> > 
> > BUG=724110
> > 
> > Review-Url: https://codereview.chromium.org/2933343002
> > Cr-Commit-Position: refs/heads/master@{#485635}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc...
> 
>
TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
> # Not skipping CQ checks because original CL landed more than 1 days
ago.
> BUG=724110, 741798
> 
> Review-Url: https://codereview.chromium.org/2980773002
> Cr-Commit-Position: refs/heads/master@{#486397}
> Committed:
https://chromium.googlesource.com/chromium/src/+/df9f33ffee10b4ffbd6469697bc9...

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
# Not skipping CQ checks because original CL landed more than 1 days
ago.
BUG=724110, 741798
==========

to

==========
Reland of Deduplicate Monochrome locale .paks (patchset #1 id:1 of
https://codereview.chromium.org/2980773002/ )

Instead of using system webview's resource whitelist, now uses a
generated list of resource IDs that are actually packed into
Webview's locale paks. This fixes the missing strings issue.

Original issue's description:
> Revert of Deduplicate Monochrome locale .paks (patchset #4 id:180001
of https://codereview.chromium.org/2933343002/ )
> 
> Reason for revert:
> Chrome is now missing 5 locale strings. The reason is that these
strings are believed to be in WebView locales because they're
whitelisted, but since WebView uses multiple whitelists to pack its
locale strings, these strings are actually not contained by WebView
> 
> Original issue's description:
> > Deduplicate Monochrome locale .paks
> > 
> > This CL removes WebView strings from Chrome locale .paks in
Monochrome
> > using a generated resource whitelist. While WebView's locale
resource
> > logic remains the same, Chrome in Monochrome now loads a secondary
> > locale pack file as a fallback when a string cannot be found in the
> > primary locale pack file.
> > 
> > This CL reduces binary size by over 400KB.
> > 
> > BUG=724110
> > 
> > Review-Url: https://codereview.chromium.org/2933343002
> > Cr-Commit-Position: refs/heads/master@{#485635}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc...
> 
>
TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
> # Not skipping CQ checks because original CL landed more than 1 days
ago.
> BUG=724110, 741798
> 
> Review-Url: https://codereview.chromium.org/2980773002
> Cr-Commit-Position: refs/heads/master@{#486397}
> Committed:
https://chromium.googlesource.com/chromium/src/+/df9f33ffee10b4ffbd6469697bc9...

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
# Not skipping CQ checks because original CL landed more than 1 days
ago.
BUG=724110, 741798
==========

[F, 2017-07-14 21:06:42]

Description was changed from

==========
Reland of Deduplicate Monochrome locale .paks (patchset #1 id:1 of
https://codereview.chromium.org/2980773002/ )

Instead of using system webview's resource whitelist, now uses a
generated list of resource IDs that are actually packed into
Webview's locale paks. This fixes the missing strings issue.

Original issue's description:
> Revert of Deduplicate Monochrome locale .paks (patchset #4 id:180001
of https://codereview.chromium.org/2933343002/ )
> 
> Reason for revert:
> Chrome is now missing 5 locale strings. The reason is that these
strings are believed to be in WebView locales because they're
whitelisted, but since WebView uses multiple whitelists to pack its
locale strings, these strings are actually not contained by WebView
> 
> Original issue's description:
> > Deduplicate Monochrome locale .paks
> > 
> > This CL removes WebView strings from Chrome locale .paks in
Monochrome
> > using a generated resource whitelist. While WebView's locale
resource
> > logic remains the same, Chrome in Monochrome now loads a secondary
> > locale pack file as a fallback when a string cannot be found in the
> > primary locale pack file.
> > 
> > This CL reduces binary size by over 400KB.
> > 
> > BUG=724110
> > 
> > Review-Url: https://codereview.chromium.org/2933343002
> > Cr-Commit-Position: refs/heads/master@{#485635}
> > Committed:
https://chromium.googlesource.com/chromium/src/+/08ba6b7c7e7edb3f7c8851eb7ddc...
> 
>
TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
> # Not skipping CQ checks because original CL landed more than 1 days
ago.
> BUG=724110, 741798
> 
> Review-Url: https://codereview.chromium.org/2980773002
> Cr-Commit-Position: refs/heads/master@{#486397}
> Committed:
https://chromium.googlesource.com/chromium/src/+/df9f33ffee10b4ffbd6469697bc9...

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
# Not skipping CQ checks because original CL landed more than 1 days
ago.
BUG=724110, 741798
==========

to

==========
Reland of Deduplicate Monochrome locale .paks

Instead of using system webview's resource whitelist, now uses a
generated list of resource IDs that are actually packed into
Webview's locale paks. This fixes the missing strings issue.

Original issue:
https://codereview.chromium.org/2980773002/

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
# Not skipping CQ checks because original CL landed more than 1 days
ago.
BUG=724110, 742388
==========

[F, 2017-07-14 21:07:02]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-14 21:07:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-07-14 21:10:45]

Hi Andrew, Dirk, Sadrul, and Lei,

The fixes are in BUILD.gn & pak_util.py only. Now Monochrome uses a generated
exact resource ID list for de-duplication instead of resource whitelist.

PTAL. Thanks!

[commit-bot: I haz the power, 2017-07-14 22:29:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-14 22:29:26]

Dry run: This issue passed the CQ dry run.

[Dirk Pranke, 2017-07-14 23:01:51]

I defer to the others here. lgtm if/when they're happy.

https://codereview.chromium.org/2977993002/diff/90002/tools/resources/filter_...
File tools/resources/filter_resource_whitelist.py (right):

https://codereview.chromium.org/2977993002/diff/90002/tools/resources/filter_...
tools/resources/filter_resource_whitelist.py:6: __doc__ =
"""filter_resource_whitelist.py [-h] [--input INPUT]
Nit: you don't need to (and shouldn't) assign to __doc__ here; it's set
automatically.

[agrieve, 2017-07-17 13:50:52]

https://codereview.chromium.org/2977993002/diff/90002/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2977993002/diff/90002/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:739: script = "//tools/grit/pak_util.py"
Would be good to add a comment to this explicitly stating that you can't just
use android_webview's compiler whitelist because generate_webui_resources and
generate_components_resources use hand-written whitelists

https://codereview.chromium.org/2977993002/diff/90002/tools/grit/pak_util.py
File tools/grit/pak_util.py (right):

https://codereview.chromium.org/2977993002/diff/90002/tools/grit/pak_util.py#...
tools/grit/pak_util.py:98: sub_parser = sub_parsers.add_parser('list',
nit: Might be nice to just make "print" more capable rather than adding another
almost-the-same-sounding command.

E.g.: --output would be useful for "print".

How about doing:

pak_util.py print --output=FOO --format='{id}'

Have the default for format be: 'Entry(id={id}, len={len}, sha1={sha1}):
{value:.60}'

Then print lines via fmt.format(id=resource_id, len=len(data), sha1=sha1,
value=desc) 

Having the default like this should be enough to document what keys are allowed
:)

[F, 2017-07-17 14:34:50]

The CQ bit was checked by zpeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-17 14:34:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[F, 2017-07-17 14:35:14]

Thanks Andrew & Dirk! PTAL

https://codereview.chromium.org/2977993002/diff/90002/chrome/android/BUILD.gn
File chrome/android/BUILD.gn (right):

https://codereview.chromium.org/2977993002/diff/90002/chrome/android/BUILD.gn...
chrome/android/BUILD.gn:739: script = "//tools/grit/pak_util.py"
On 2017/07/17 13:50:52, agrieve wrote:
> Would be good to add a comment to this explicitly stating that you can't just
> use android_webview's compiler whitelist because generate_webui_resources and
> generate_components_resources use hand-written whitelists

Done.

https://codereview.chromium.org/2977993002/diff/90002/tools/grit/pak_util.py
File tools/grit/pak_util.py (right):

https://codereview.chromium.org/2977993002/diff/90002/tools/grit/pak_util.py#...
tools/grit/pak_util.py:98: sub_parser = sub_parsers.add_parser('list',
On 2017/07/17 13:50:52, agrieve wrote:
> nit: Might be nice to just make "print" more capable rather than adding
another
> almost-the-same-sounding command.
> 
> E.g.: --output would be useful for "print".
> 
> How about doing:
> 
> pak_util.py print --output=FOO --format='{id}'
> 
> Have the default for format be: 'Entry(id={id}, len={len}, sha1={sha1}):
> {value:.60}'
> 
> Then print lines via fmt.format(id=resource_id, len=len(data), sha1=sha1,
> value=desc) 
> 
> Having the default like this should be enough to document what keys are
allowed
> :)

Resolved offline. Name changed to "list-id"

https://codereview.chromium.org/2977993002/diff/90002/tools/resources/filter_...
File tools/resources/filter_resource_whitelist.py (right):

https://codereview.chromium.org/2977993002/diff/90002/tools/resources/filter_...
tools/resources/filter_resource_whitelist.py:6: __doc__ =
"""filter_resource_whitelist.py [-h] [--input INPUT]
On 2017/07/14 23:01:51, Dirk Pranke wrote:
> Nit: you don't need to (and shouldn't) assign to __doc__ here; it's set
> automatically.

Done.

[agrieve, 2017-07-17 14:43:59]

On 2017/07/17 14:35:14, F wrote:
> Thanks Andrew & Dirk! PTAL
> 
> https://codereview.chromium.org/2977993002/diff/90002/chrome/android/BUILD.gn
> File chrome/android/BUILD.gn (right):
> 
>
https://codereview.chromium.org/2977993002/diff/90002/chrome/android/BUILD.gn...
> chrome/android/BUILD.gn:739: script = "//tools/grit/pak_util.py"
> On 2017/07/17 13:50:52, agrieve wrote:
> > Would be good to add a comment to this explicitly stating that you can't
just
> > use android_webview's compiler whitelist because generate_webui_resources
and
> > generate_components_resources use hand-written whitelists
> 
> Done.
> 
> https://codereview.chromium.org/2977993002/diff/90002/tools/grit/pak_util.py
> File tools/grit/pak_util.py (right):
> 
>
https://codereview.chromium.org/2977993002/diff/90002/tools/grit/pak_util.py#...
> tools/grit/pak_util.py:98: sub_parser = sub_parsers.add_parser('list',
> On 2017/07/17 13:50:52, agrieve wrote:
> > nit: Might be nice to just make "print" more capable rather than adding
> another
> > almost-the-same-sounding command.
> > 
> > E.g.: --output would be useful for "print".
> > 
> > How about doing:
> > 
> > pak_util.py print --output=FOO --format='{id}'
> > 
> > Have the default for format be: 'Entry(id={id}, len={len}, sha1={sha1}):
> > {value:.60}'
> > 
> > Then print lines via fmt.format(id=resource_id, len=len(data), sha1=sha1,
> > value=desc) 
> > 
> > Having the default like this should be enough to document what keys are
> allowed
> > :)
> 
> Resolved offline. Name changed to "list-id"
> 
>
https://codereview.chromium.org/2977993002/diff/90002/tools/resources/filter_...
> File tools/resources/filter_resource_whitelist.py (right):
> 
>
https://codereview.chromium.org/2977993002/diff/90002/tools/resources/filter_...
> tools/resources/filter_resource_whitelist.py:6: __doc__ =
> """filter_resource_whitelist.py [-h] [--input INPUT]
> On 2017/07/14 23:01:51, Dirk Pranke wrote:
> > Nit: you don't need to (and shouldn't) assign to __doc__ here; it's set
> > automatically.
> 
> Done.

lgtm

[commit-bot: I haz the power, 2017-07-17 15:45:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-17 15:45:07]

Dry run: This issue passed the CQ dry run.

[F, 2017-07-17 17:24:44]

The CQ bit was checked by zpeng@chromium.org

[F, 2017-07-17 17:24:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, agrieve@chromium.org
Link to the patchset: https://codereview.chromium.org/2977993002/#ps320001
(title: "Rebase")

[commit-bot: I haz the power, 2017-07-17 17:24:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-17 18:47:49]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1500312284068420,
"parent_rev": "7d1476b8f693f79a755cc70f93856add69dc1834", "commit_rev":
"368afac85d2ca5e22efcbb2271c0d379b2fc22ab"}

[commit-bot: I haz the power, 2017-07-17 18:48:11]

Description was changed from

==========
Reland of Deduplicate Monochrome locale .paks

Instead of using system webview's resource whitelist, now uses a
generated list of resource IDs that are actually packed into
Webview's locale paks. This fixes the missing strings issue.

Original issue:
https://codereview.chromium.org/2980773002/

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
# Not skipping CQ checks because original CL landed more than 1 days
ago.
BUG=724110, 742388
==========

to

==========
Reland of Deduplicate Monochrome locale .paks

Instead of using system webview's resource whitelist, now uses a
generated list of resource IDs that are actually packed into
Webview's locale paks. This fixes the missing strings issue.

Original issue:
https://codereview.chromium.org/2980773002/

TBR=agrieve@chromium.org,dpranke@chromium.org,thestig@chromium.org,sadrul@chr...
# Not skipping CQ checks because original CL landed more than 1 days
ago.
BUG=724110, 742388

Review-Url: https://codereview.chromium.org/2977993002
Cr-Commit-Position: refs/heads/master@{#487176}
Committed:
https://chromium.googlesource.com/chromium/src/+/368afac85d2ca5e22efcbb2271c0...
==========

[commit-bot: I haz the power, 2017-07-17 18:48:13]

Committed patchset #4 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/368afac85d2ca5e22efcbb2271c0...