[mojo] Make HTML viewer load the blink resources from the generated pak file.

[mojo] Make HTML viewer load the blink resources from the generated pak file.

Blink is about to make use of blink_resources.grd for the inline resources of user agent stylesheets.
This removes the dependency upon using make-file-arrays.py which embeds these resources as strings.
See part 1, 2 and 3.

Also the .rodata section of libblink_web (in component build mode) is reduced by ~33kb.

In this CL, the html viewer would implement the loadResource() method and use the DataPack to
load the required resources. The resources are generated from blink_resources.grd which has been
added as the deps.

Part 1: https://codereview.chromium.org/436843004/
Part 2: https://codereview.chromium.org/422023008
Part 3: https://codereview.chromium.org/573553002

BUG=312586
Committed: https://crrev.com/ce9bcf3e3b788f8ded1cde7702bdc9cfd1f8859e
Cr-Commit-Position: refs/heads/master@{#311420}

[vivekg, 2014-12-30 07:03:13]

vivekg@chromium.org changed reviewers:
+ aa@chromium.org, jamesr@chromium.org, jochen@chromium.org

[vivekg, 2014-12-30 07:04:12]

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

[vivekg, 2014-12-30 07:04:13]

PTAL as this is blocking https://codereview.chromium.org/573553002 from landing.

[vivekg, 2014-12-30 07:04:40]

vivekg@chromium.org changed reviewers:
- vivekg@chromium.org

[vivekg, 2015-01-05 02:29:55]

On 2014/12/30 07:04:13, vivekg_ wrote:
> PTAL as this is blocking https://codereview.chromium.org/573553002 from
landing.

jamesr@, aa@: friendly ping.

[jamesr, 2015-01-05 23:54:40]

This is wrong - mojo apps in general can't just use the file service since they
aren't necessarily loaded from disk.  We don't currently have a better way to
bundle resources into a mojo app than linking them into the binary itself.

https://codereview.chromium.org/827223002/diff/1/mojo/services/html_viewer/bl...
File mojo/services/html_viewer/blink_platform_impl.cc (right):

https://codereview.chromium.org/827223002/diff/1/mojo/services/html_viewer/bl...
mojo/services/html_viewer/blink_platform_impl.cc:44: #if !defined(OS_WIN)
we use linux on everything that's not windows?

[vivekg, 2015-01-07 15:51:16]

On 2015/01/05 at 23:54:40, jamesr wrote:
> This is wrong - mojo apps in general can't just use the file service since
they aren't necessarily loaded from disk.  We don't currently have a better way
to bundle resources into a mojo app than linking them into the binary itself.
> 
>
https://codereview.chromium.org/827223002/diff/1/mojo/services/html_viewer/bl...
> File mojo/services/html_viewer/blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/827223002/diff/1/mojo/services/html_viewer/bl...
> mojo/services/html_viewer/blink_platform_impl.cc:44: #if !defined(OS_WIN)
> we use linux on everything that's not windows?

@jamesr: thank you for the review.

I have couple of questions in terms of embedding the resources in the binary
itself.

1. Should we use the generated blink_resources.pak file from blink_resources.grd
and use the tools/grit to unpack and then generate a .h/.cpp file containing the
resources as arrays?
This file can be mapped with the resource names to the arrays and can be
returned back.
Pros:
* No need to keep track of what resources are being exported from blink to
chrome. 
* Unified way of controlling the resources from blink.

Cons:
* Grit dependency to extract the contents of the pak file prior to compiling
mojo apps.
* Not sure if grit can be used as library to extract the contents.

2. White list only required resources (in this case the bunch of CSS files) as
arrays from a python script and embed them.
Pros:
* No dependency on grit and its extraction feature being exported as library.
* Simple method of exporting only what is required.

Cons:
* Everytime blink changes its resources being exported, this white list needs to
be managed.

Please suggest which way would be a better approach!

[jamesr, 2015-01-07 16:35:27]

For simplicity I would try to link the whole pak file in to rodata and parse it
normally.

[vivekg, 2015-01-07 16:46:19]

On 2015/01/07 at 16:35:27, jamesr wrote:
> For simplicity I would try to link the whole pak file in to rodata and parse
it normally.

Yeah I wanted to do the same using the base/resource/data_pack.h.
Does this support loading the pak file from the rodata? As from the class
documentation, I thought it currently supports loading the pak contents only
from on-disk files.

[jamesr, 2015-01-08 01:43:19]

Hmm yeah, I hadn't looked at ui/base/resource/data_pack very closely.  It does
appear to only load from files.  You could ask the owners to expose
functionality to parse a memory region already loaded in, or try one of the
other approaches you outlined.

[vivekg, 2015-01-09 10:01:13]

On 2015/01/08 at 01:43:19, jamesr wrote:
> Hmm yeah, I hadn't looked at ui/base/resource/data_pack very closely.  It does
appear to only load from files.  You could ask the owners to expose
functionality to parse a memory region already loaded in, or try one of the
other approaches you outlined.

Hi jamesr@, 

I have uploaded a new patch using the python script to generate the required
resources.

The generated files look like as -

blink_resource_map.h : http://paste.ubuntu.com/9697468/
blink_resource_map.cc: http://paste.ubuntu.com/9697494/

PTAL, thanks!

[vivekg, 2015-01-09 12:20:37]

Seems like std::tuple is not yet ported on android toolchain. Hence replacing
the usage of tuple.

Generated files:

blink_resource_map.h : http://paste.ubuntu.com/9698040/
blink_resource_map.cc: http://paste.ubuntu.com/9698044/

[jamesr, 2015-01-09 19:11:49]

std::tuple is a C++11 library feature, so it's not allowed on any platform.

lgtm

https://codereview.chromium.org/827223002/diff/40001/mojo/services/html_viewe...
File mojo/services/html_viewer/BUILD.gn (right):

https://codereview.chromium.org/827223002/diff/40001/mojo/services/html_viewe...
mojo/services/html_viewer/BUILD.gn:8: copy("copy_blink_resources") {
why do you need this copy?

https://codereview.chromium.org/827223002/diff/40001/mojo/services/html_viewe...
mojo/services/html_viewer/BUILD.gn:24:
rebase_path("$root_out_dir/blink_resources.pak"),
can't you just point this at the existing blink_resources.pak instead of another
copy?

[vivekg, 2015-01-10 02:10:42]

On 2015/01/09 at 19:11:49, jamesr wrote:
Thank you for the review.

>
https://codereview.chromium.org/827223002/diff/40001/mojo/services/html_viewe...
> mojo/services/html_viewer/BUILD.gn:8: copy("copy_blink_resources") {
> why do you need this copy?

Oh yeah. Missed to remove it. Removed the copying and pointing to the generated
pak file location. Uploaded a new patch, PTAL.

[vivekg, 2015-01-10 02:24:15]

jamesr@, I am getting chromium_presubmit error as - 

Missing LGTM from OWNERS of dependencies added to DEPS:
    '+blink/public/resources',

Whom shall I add to review this in order to pass the pre-submit stage?

[vivekg, 2015-01-10 02:26:38]

ah got it.

+darin@, PTAL, thank you.

[vivekg, 2015-01-10 02:27:02]

vivekg@chromium.org changed reviewers:
+ darin@chromium.org, vivekg@chromium.org

[darin (slow to review), 2015-01-10 05:12:19]

https://codereview.chromium.org/827223002/diff/1/mojo/services/html_viewer/bl...
File mojo/services/html_viewer/blink_platform_impl.h (right):

https://codereview.chromium.org/827223002/diff/1/mojo/services/html_viewer/bl...
mojo/services/html_viewer/blink_platform_impl.h:56: virtual blink::WebData
loadResource(const char* name) override;
nit: when overriding methods from the blink public API, we do not use the
"override" keyword. we want to make it easier on folks who wish to change blink
public API and have to deal with making such changes across repositories.

[vivekg, 2015-01-10 05:21:43]

On 2015/01/10 at 05:12:19, darin wrote:
>
https://codereview.chromium.org/827223002/diff/1/mojo/services/html_viewer/bl...
> File mojo/services/html_viewer/blink_platform_impl.h (right):
> 
>
https://codereview.chromium.org/827223002/diff/1/mojo/services/html_viewer/bl...
> mojo/services/html_viewer/blink_platform_impl.h:56: virtual blink::WebData
loadResource(const char* name) override;
> nit: when overriding methods from the blink public API, we do not use the
"override" keyword. we want to make it easier on folks who wish to change blink
public API and have to deal with making such changes across repositories.

Thank you darin@, I have modified the patch to remove 'override'. PTAL, thanks.

[vivekg, 2015-01-12 03:19:32]

Friendly ping.

[vivekg, 2015-01-13 19:25:17]

darin@, awaiting your feedback, thank you.

[darin (slow to review), 2015-01-13 19:27:44]

On 2015/01/13 19:25:17, vivekg_ wrote:
> darin@, awaiting your feedback, thank you.

Apologies for the review latency.

[darin (slow to review), 2015-01-13 19:29:59]

LGTM

https://codereview.chromium.org/827223002/diff/80001/mojo/services/html_viewe...
File mojo/services/html_viewer/blink_platform_impl.cc (right):

https://codereview.chromium.org/827223002/diff/80001/mojo/services/html_viewe...
mojo/services/html_viewer/blink_platform_impl.cc:34: const DataResource
kDataResources[] = {
nit: I'd probably move this out to a separate file for clarity as
blink_platform_impl.cc could otherwise grow very large if every function
implementation added a little bit of code like this CL does.

[vivekg, 2015-01-13 19:33:24]

On 2015/01/13 at 19:29:59, darin wrote:
> LGTM
> 
>
https://codereview.chromium.org/827223002/diff/80001/mojo/services/html_viewe...
> File mojo/services/html_viewer/blink_platform_impl.cc (right):
> 
>
https://codereview.chromium.org/827223002/diff/80001/mojo/services/html_viewe...
> mojo/services/html_viewer/blink_platform_impl.cc:34: const DataResource
kDataResources[] = {
> nit: I'd probably move this out to a separate file for clarity as
blink_platform_impl.cc could otherwise grow very large if every function
implementation added a little bit of code like this CL does.

Thank you darin@ for your time on the review and I really appreciate your
feedback. 

I will move the resource mapping into a separate file as you pointed out. Thank
you.

[vivekg, 2015-01-14 05:09:10]

I have uploaded a new patch as per darin@'s review.

Can darin@ or jamesr@ rubber stamp this before I hit CQ?

[jamesr, 2015-01-14 05:12:49]

lgtm

[vivekg, 2015-01-14 06:41:19]

The CQ bit was checked by vivekg@chromium.org

[commit-bot: I haz the power, 2015-01-14 06:42:06]

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

[commit-bot: I haz the power, 2015-01-14 06:43:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-01-14 06:44:38]

Patchset 6 (id:??) landed as
https://crrev.com/ce9bcf3e3b788f8ded1cde7702bdc9cfd1f8859e
Cr-Commit-Position: refs/heads/master@{#311420}