Gets packaging of html_viewer for android working

Gets packaging of html_viewer for android working

BUG=none
TEST=none
R=jamesr@chromium.org, qsr@chromium.org

Committed: https://crrev.com/b5e25b58fc6e108d73f14b14a8b72c7642868d8f
Cr-Commit-Position: refs/heads/master@{#320726}

[sky, 2015-03-13 00:06:35]

James, you're an owner. I'm adding qsr in case you're not familiar with the
android side of things.

[jamesr, 2015-03-13 00:44:29]

I don't really know what's going on here, so please get qsr@ or someone familiar
with android to review, but once you do here's an lgtm for OWNERS

[qsr, 2015-03-13 16:53:12]

https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/B...
File mojo/services/html_viewer/BUILD.gn (right):

https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/B...
mojo/services/html_viewer/BUILD.gn:152:
android_library("html_viewer_gfx_classes") {
That's really, really, really weird. Why isn't this defined in //ui ? Compiling
some code from another component is not really supposed to happen.

https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/a...
File mojo/services/html_viewer/android/android_hooks.cc (right):

https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/a...
mojo/services/html_viewer/android/android_hooks.cc:41:
Java_Main_init(base::android::AttachCurrentThread());
The intention of those utility functions is for this to be in your Init()
method.

[sky, 2015-03-13 17:35:48]

https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/B...
File mojo/services/html_viewer/BUILD.gn (right):

https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/B...
mojo/services/html_viewer/BUILD.gn:152:
android_library("html_viewer_gfx_classes") {
On 2015/03/13 16:53:12, qsr wrote:
> That's really, really, really weird. Why isn't this defined in //ui ?
Compiling
> some code from another component is not really supposed to happen.

I'm not sure what ends up using that file in a real build. The only place it's
declared is here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/BUILD.gn&q=...
. But maybe I'm misunderstanding what gfx_jni_headers ends up doing. I'll play
with it shortly and see if I can figure this out.

[qsr, 2015-03-13 17:43:41]

On 2015/03/13 17:35:48, sky wrote:
>
https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/B...
> File mojo/services/html_viewer/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/B...
> mojo/services/html_viewer/BUILD.gn:152:
> android_library("html_viewer_gfx_classes") {
> On 2015/03/13 16:53:12, qsr wrote:
> > That's really, really, really weird. Why isn't this defined in //ui ?
> Compiling
> > some code from another component is not really supposed to happen.
> 
> I'm not sure what ends up using that file in a real build. The only place it's
> declared is here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/gfx/BUILD.gn&q=...
> . But maybe I'm misunderstanding what gfx_jni_headers ends up doing. I'll play
> with it shortly and see if I can figure this out.

It is in there:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/BUILD.g...
using DEPRECATED_java_in_dir

[sky, 2015-03-13 22:12:50]

Ok, changed to depend upon ui_java, which brings in way more but is ok for now.

https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/a...
File mojo/services/html_viewer/android/android_hooks.cc (right):

https://codereview.chromium.org/1002033004/diff/1/mojo/services/html_viewer/a...
mojo/services/html_viewer/android/android_hooks.cc:41:
Java_Main_init(base::android::AttachCurrentThread());
On 2015/03/13 16:53:12, qsr wrote:
> The intention of those utility functions is for this to be in your Init()
> method.

Done.

[qsr, 2015-03-16 08:58:01]

LGTM with a big but...

It seems to me that the only reason you need any of this is code that is in
mojo/services/html_viewer (all the request to PathService, and also the handling
of pak files there).

I really think we should refactor that code to not need to access the file
system. For the pak file, we can embed the content in the rodata section of the
.so, and for the screen resolution, we should get this information from a given
View as we already send this information from the native viewport service.

This would allow to have a sandboxed html viewer, while the one you are
currently creating has all the same privileges as the shell itself.

[sky, 2015-03-16 14:59:56]

On 2015/03/16 08:58:01, qsr wrote:
> LGTM with a big but...
> 
> It seems to me that the only reason you need any of this is code that is in
> mojo/services/html_viewer (all the request to PathService, and also the
handling
> of pak files there).
> 
> I really think we should refactor that code to not need to access the file
> system. For the pak file, we can embed the content in the rodata section of
the
> .so, and for the screen resolution, we should get this information from a
given
> View as we already send this information from the native viewport service.
> 
> This would allow to have a sandboxed html viewer, while the one you are
> currently creating has all the same privileges as the shell itself.

v8/gin also requires file system access, and content is doing something similar
to this already. So, I'm hesitant to attempt to refactor at this point. I would
like the project to get a bit further along, we can revisit then.

[sky, 2015-03-16 15:00:11]

On 2015/03/16 14:59:56, sky wrote:
> On 2015/03/16 08:58:01, qsr wrote:
> > LGTM with a big but...
> > 
> > It seems to me that the only reason you need any of this is code that is in
> > mojo/services/html_viewer (all the request to PathService, and also the
> handling
> > of pak files there).
> > 
> > I really think we should refactor that code to not need to access the file
> > system. For the pak file, we can embed the content in the rodata section of
> the
> > .so, and for the screen resolution, we should get this information from a
> given
> > View as we already send this information from the native viewport service.
> > 
> > This would allow to have a sandboxed html viewer, while the one you are
> > currently creating has all the same privileges as the shell itself.
> 
> v8/gin also requires file system access, and content is doing something
similar
> to this already. So, I'm hesitant to attempt to refactor at this point. I
would
> like the project to get a bit further along, we can revisit then.

I did add some TODOs though.

[sky, 2015-03-16 15:00:16]

The CQ bit was checked by sky@chromium.org

[sky, 2015-03-16 15:00:16]

The patchset sent to the CQ was uploaded after l-g-t-m from jamesr@chromium.org,
qsr@chromium.org
Link to the patchset: https://codereview.chromium.org/1002033004/#ps60001
(title: "merge 2 trunk")

[commit-bot: I haz the power, 2015-03-16 15:00:31]

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

[commit-bot: I haz the power, 2015-03-16 15:25:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-16 15:26:09]

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