[ios Mojo] iOS facade class for Mojo API.

[ios Mojo] iOS facade class for Mojo API.

On all platforms except iOS Mojo API is implemented natively. On iOS
Chrome is not allowed to use Bling and uses WKWebView. So the only way
to implement Mojo API is hybrid approach:
 - API frontent is implemented in JS;
 - frontend packs function name and arguments into JSON and sends to
   the native code;
 - window.prompt is used as communication channel, because this is the
   only way to send synchronous message from WKWebView;
 - native code will pass JSON to facade;
 - facade will respond with JSON;
 - native code will send JSON back to JS via window.prompt result;
 - JS frontent will unpack JSON and will return the result to the caller;

This CL introduces a facade class which takes JSON as in input, passes
it to Mojo and then returns the result as JSON.

Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08Bes/edit#heading=h.yjqp5garsqf9

BUG=567809
Committed: https://crrev.com/91b474353e27a83bd990556572418c2c9750abed
Cr-Commit-Position: refs/heads/master@{#394313}

[Eugene But (OOO till 7-30), 2016-05-06 23:19:36]

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

[commit-bot: I haz the power, 2016-05-06 23:20:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/1

[commit-bot: I haz the power, 2016-05-06 23:26:17]

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

[commit-bot: I haz the power, 2016-05-06 23:26:18]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Eugene But (OOO till 7-30), 2016-05-07 00:05:47]

Description was changed from

==========
[ios Mojo] iOS facade class for Mojo API.

Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08B...

BUG=567809
==========

to

==========
[ios Mojo] iOS facade class for Mojo API.

On all platforms except iOS Mojo API is implemented natively. On iOS
Chrome is not allowed to use Bling and uses WKWebView. So the only way
to implement Mojo API is hybrid approach:
 - API frontent is implemented in JS;
 - frontend packs function name arn arguments into JSON and sends to
   the native code;
 - window.prompt is used as communication channel, because this is the
   only way to send synchronous message from WKWebView;
 - native code will pass JSON to facade;
 - facade will respond with JSON;
 - native code will send JSON back to JS via window.prompt result;
 - JS frontent will unpack JSON and will return the result to the caller;
 
This CL introduces a facade class which takes JSON as in input, passes
it to Mojo and then returns the result as JSON.
 
Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08B...

BUG=567809
==========

[Eugene But (OOO till 7-30), 2016-05-07 00:06:07]

Description was changed from

==========
[ios Mojo] iOS facade class for Mojo API.

On all platforms except iOS Mojo API is implemented natively. On iOS
Chrome is not allowed to use Bling and uses WKWebView. So the only way
to implement Mojo API is hybrid approach:
 - API frontent is implemented in JS;
 - frontend packs function name arn arguments into JSON and sends to
   the native code;
 - window.prompt is used as communication channel, because this is the
   only way to send synchronous message from WKWebView;
 - native code will pass JSON to facade;
 - facade will respond with JSON;
 - native code will send JSON back to JS via window.prompt result;
 - JS frontent will unpack JSON and will return the result to the caller;
 
This CL introduces a facade class which takes JSON as in input, passes
it to Mojo and then returns the result as JSON.
 
Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08B...

BUG=567809
==========

to

==========
[ios Mojo] iOS facade class for Mojo API.

On all platforms except iOS Mojo API is implemented natively. On iOS
Chrome is not allowed to use Bling and uses WKWebView. So the only way
to implement Mojo API is hybrid approach:
 - API frontent is implemented in JS;
 - frontend packs function name and arguments into JSON and sends to
   the native code;
 - window.prompt is used as communication channel, because this is the
   only way to send synchronous message from WKWebView;
 - native code will pass JSON to facade;
 - facade will respond with JSON;
 - native code will send JSON back to JS via window.prompt result;
 - JS frontent will unpack JSON and will return the result to the caller;
 
This CL introduces a facade class which takes JSON as in input, passes
it to Mojo and then returns the result as JSON.
 
Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08B...

BUG=567809
==========

[Eugene But (OOO till 7-30), 2016-05-07 00:07:51]

eugenebut@chromium.org changed reviewers:
+ rockot@chromium.org

[Eugene But (OOO till 7-30), 2016-05-07 00:07:52]

PTAL. This is backend for iOS Mojo API. I will send frontent in a separate CL.

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp#newcode56
ios/web/ios_web.gyp:56: '../../services/shell/shell.gyp:shell_public',
This creates a cycle in file dependencies, because filesystem.gyp uses
shell.gyp. GYP throws an exception when it detects file cycles (at least on
iOS):
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/build...

The cycle was created this CL:
https://codereview.chromium.org/1942473002/diff/320001/services/shell/shell.gyp

Maybe I should not pull shell.gyp:shell_public? This works on other platforms
though...

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.mm
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.m...
ios/web/webui/mojo_facade.mm:122: return "";
On iOS we fail gracefully if JS message is malformed because any page can send
these messages. WebUI pages should run only Chrome scripts, so maybe we should
crash instead of returning empty json?

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade_u...
File ios/web/webui/mojo_facade_unittest.mm (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade_u...
ios/web/webui/mojo_facade_unittest.mm:352: 
This is very basic unit test, but I do have an integration test which covers the
whole flow:
https://codereview.chromium.org/1906993003/patch/140001/150058
Using test js page: 
https://codereview.chromium.org/1906993003/patch/140001/150032

In any case I would appreciate suggestions if you can see what else can be unit
tested here.

[Ken Rockot(use gerrit already), 2016-05-09 15:44:38]

Not a full review, just some high-level comments/questions for now

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp#newcode56
ios/web/ios_web.gyp:56: '../../services/shell/shell.gyp:shell_public',
On 2016/05/07 at 00:07:52, Eugene But wrote:
> This creates a cycle in file dependencies, because filesystem.gyp uses
> shell.gyp. GYP throws an exception when it detects file cycles (at least on
> iOS):
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/build...
> 
> The cycle was created this CL:
>
https://codereview.chromium.org/1942473002/diff/320001/services/shell/shell.gyp
> 
> Maybe I should not pull shell.gyp:shell_public? This works on other platforms
though...

Lame gyp behavior. There isn't really an interesting circular dependency (i.e.
among actual targets), just one among gyp files. Easiest solution would be to
move shell_public (and shell_interfaces) to a separate shell_public.gyp.

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp#newcode502
ios/web/ios_web.gyp:502: # generated cpp files be listed explicitly in
browser_ui.
You should be able to avoid this (manually listing generated source files) by
using mojom_bindings_generator_explicit.gypi. This target type would still be
none (and others could depend on it to trigger JS etc generation), but you would
make a second target as well: a static_library that depends on this one, which
targets can link against if they have link dependencies on the generated cpp
files. There are several examples of this around src/, just grep for
mojom_bindings_generator_explicit.

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.mm
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.m...
ios/web/webui/mojo_facade.mm:122: return "";
On 2016/05/07 at 00:07:52, Eugene But wrote:
> On iOS we fail gracefully if JS message is malformed because any page can send
these messages. WebUI pages should run only Chrome scripts, so maybe we should
crash instead of returning empty json?

What do you mean by "any page can send these messages?" On the surface this
sounds like a very, very scary statement.

[Eugene But (OOO till 7-30), 2016-05-09 16:08:10]

Replying to one comment. Will address others separately.

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.mm
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.m...
ios/web/webui/mojo_facade.mm:122: return "";
On 2016/05/09 15:44:38, Ken Rockot wrote:
> On 2016/05/07 at 00:07:52, Eugene But wrote:
> > On iOS we fail gracefully if JS message is malformed because any page can
send
> these messages. WebUI pages should run only Chrome scripts, so maybe we should
> crash instead of returning empty json?
> 
> What do you mean by "any page can send these messages?" On the surface this
> sounds like a very, very scary statement.
A lot of stuff on iOS is implemented in JavaScript. Chrome injects scripts and
then scripts send messages to the native code. Any web page could send those
messages to the native code and there was no reliable way to protect. We tried
multiple approaches to ensure authenticity, but we were able to break all of
them. So messages received from a web page could be trusted. And no message
should be able to crash the app.

Mojo case is very different, because mojo messages can be sent only from
chrome:// pages and only Chrome code can run on those pages. WKWebView provides
reliable api to check the sender's origin so every message is trusted.

[Ken Rockot(use gerrit already), 2016-05-09 16:11:57]

On 2016/05/09 at 16:08:10, eugenebut wrote:
> Replying to one comment. Will address others separately.
> 
> https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.mm
> File ios/web/webui/mojo_facade.mm (right):
> 
>
https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.m...
> ios/web/webui/mojo_facade.mm:122: return "";
> On 2016/05/09 15:44:38, Ken Rockot wrote:
> > On 2016/05/07 at 00:07:52, Eugene But wrote:
> > > On iOS we fail gracefully if JS message is malformed because any page can
send
> > these messages. WebUI pages should run only Chrome scripts, so maybe we
should
> > crash instead of returning empty json?
> > 
> > What do you mean by "any page can send these messages?" On the surface this
> > sounds like a very, very scary statement.
> A lot of stuff on iOS is implemented in JavaScript. Chrome injects scripts and
then scripts send messages to the native code. Any web page could send those
messages to the native code and there was no reliable way to protect. We tried
multiple approaches to ensure authenticity, but we were able to break all of
them. So messages received from a web page could be trusted. And no message
should be able to crash the app.
> 
> Mojo case is very different, because mojo messages can be sent only from
chrome:// pages and only Chrome code can run on those pages. WKWebView provides
reliable api to check the sender's origin so every message is trusted.

Ah, I see. Much less scary! CHECKing SGTM then.

[Eugene But (OOO till 7-30), 2016-05-10 02:00:56]

Still have to update GN version. But I'm not sure how to implement GYP correctly
either :)

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp#newcode502
ios/web/ios_web.gyp:502: # generated cpp files be listed explicitly in
browser_ui.
On 2016/05/09 15:44:37, Ken Rockot wrote:
> You should be able to avoid this (manually listing generated source files) by
> using mojom_bindings_generator_explicit.gypi. This target type would still be
> none (and others could depend on it to trigger JS etc generation), but you
would
> make a second target as well: a static_library that depends on this one, which
> targets can link against if they have link dependencies on the generated cpp
> files. There are several examples of this around src/, just grep for
> mojom_bindings_generator_explicit.

If I do this:


    {
      # GN version: //ios/web:test_mojo_bindings_mojom
      'target_name': 'test_mojo_bindings_mojom',
      'type': 'none',
      'variables': {
        'mojom_files': [
          'test/mojo_test.mojom',
        ],
      },
      'include_dirs': [
        '..',
      ],
      'includes': [ '../../mojo/mojom_bindings_generator_explicit.gypi' ],
    },
    {
      # GN version: //ios/web:test_mojo_bindings
      'target_name': 'test_mojo_bindings',
      'type': 'static_library',
      'dependencies': [
        '../../mojo/mojo_base.gyp:mojo_common_lib',
        '../../mojo/mojo_base.gyp:mojo_url_type_converters',
        '../../mojo/mojo_public.gyp:mojo_cpp_bindings',
        'test_mojo_bindings_mojom',
      ],
      'include_dirs': [
        '..',
      ],
    },
Then GYP gives me an error:

gyp: src/ios/web/ios_web_resources.gyp not found

Adding another gyp file and one extra target does not look like a better
approach. Is there any way to avoid adding ios_web_resources just for the test?

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.mm
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/webui/mojo_facade.m...
ios/web/webui/mojo_facade.mm:122: return "";
On 2016/05/09 16:08:10, Eugene But wrote:
> On 2016/05/09 15:44:38, Ken Rockot wrote:
> > On 2016/05/07 at 00:07:52, Eugene But wrote:
> > > On iOS we fail gracefully if JS message is malformed because any page can
> send
> > these messages. WebUI pages should run only Chrome scripts, so maybe we
should
> > crash instead of returning empty json?
> > 
> > What do you mean by "any page can send these messages?" On the surface this
> > sounds like a very, very scary statement.
> A lot of stuff on iOS is implemented in JavaScript. Chrome injects scripts and
> then scripts send messages to the native code. Any web page could send those
> messages to the native code and there was no reliable way to protect. We tried
> multiple approaches to ensure authenticity, but we were able to break all of
> them. So messages received from a web page could be trusted. And no message
> should be able to crash the app.
> 
> Mojo case is very different, because mojo messages can be sent only from
> chrome:// pages and only Chrome code can run on those pages. WKWebView
provides
> reliable api to check the sender's origin so every message is trusted. 
> 
Replaced returns with CHECKs

[Eugene But (OOO till 7-30), 2016-05-12 16:40:40]

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

[commit-bot: I haz the power, 2016-05-12 16:41:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/40001

[Eugene But (OOO till 7-30), 2016-05-12 16:41:35]

The CQ bit was unchecked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2016-05-12 16:42:53]

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

[commit-bot: I haz the power, 2016-05-12 16:43:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/60001

[commit-bot: I haz the power, 2016-05-12 16:48:54]

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

[commit-bot: I haz the power, 2016-05-12 16:48:55]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Eugene But (OOO till 7-30), 2016-05-12 17:28:30]

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

[commit-bot: I haz the power, 2016-05-12 17:28:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/80001

[commit-bot: I haz the power, 2016-05-12 17:39:38]

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

[commit-bot: I haz the power, 2016-05-12 17:39:39]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Eugene But (OOO till 7-30), 2016-05-12 19:49:18]

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

[commit-bot: I haz the power, 2016-05-12 19:49:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/100001

[commit-bot: I haz the power, 2016-05-12 19:56:50]

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

[commit-bot: I haz the power, 2016-05-12 19:56:51]

Dry run: Try jobs failed on following builders:
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Eugene But (OOO till 7-30), 2016-05-12 20:19:35]

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

[commit-bot: I haz the power, 2016-05-12 20:20:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/120001

[commit-bot: I haz the power, 2016-05-12 20:32:53]

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

[commit-bot: I haz the power, 2016-05-12 20:32:54]

Dry run: Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[Eugene But (OOO till 7-30), 2016-05-12 20:48:10]

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp#newcode56
ios/web/ios_web.gyp:56: '../../services/shell/shell.gyp:shell_public',
On 2016/05/09 15:44:37, Ken Rockot wrote:
> On 2016/05/07 at 00:07:52, Eugene But wrote:
> > This creates a cycle in file dependencies, because filesystem.gyp uses
> > shell.gyp. GYP throws an exception when it detects file cycles (at least on
> > iOS):
> >
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/build...
> > 
> > The cycle was created this CL:
> >
>
https://codereview.chromium.org/1942473002/diff/320001/services/shell/shell.gyp
> > 
> > Maybe I should not pull shell.gyp:shell_public? This works on other
platforms
> though...
> 
> Lame gyp behavior. There isn't really an interesting circular dependency (i.e.
> among actual targets), just one among gyp files. Easiest solution would be to
> move shell_public (and shell_interfaces) to a separate shell_public.gyp.
This is now fixed.

https://codereview.chromium.org/1956113002/diff/1/ios/web/ios_web.gyp#newcode502
ios/web/ios_web.gyp:502: # generated cpp files be listed explicitly in
browser_ui.
On 2016/05/10 02:00:55, Eugene But wrote:
> On 2016/05/09 15:44:37, Ken Rockot wrote:
> > You should be able to avoid this (manually listing generated source files)
by
> > using mojom_bindings_generator_explicit.gypi. This target type would still
be
> > none (and others could depend on it to trigger JS etc generation), but you
> would
> > make a second target as well: a static_library that depends on this one,
which
> > targets can link against if they have link dependencies on the generated cpp
> > files. There are several examples of this around src/, just grep for
> > mojom_bindings_generator_explicit.
> 
> If I do this:
> 
> 
>     {
>       # GN version: //ios/web:test_mojo_bindings_mojom
>       'target_name': 'test_mojo_bindings_mojom',
>       'type': 'none',
>       'variables': {
>         'mojom_files': [
>           'test/mojo_test.mojom',
>         ],
>       },
>       'include_dirs': [
>         '..',
>       ],
>       'includes': [ '../../mojo/mojom_bindings_generator_explicit.gypi' ],
>     },
>     {
>       # GN version: //ios/web:test_mojo_bindings
>       'target_name': 'test_mojo_bindings',
>       'type': 'static_library',
>       'dependencies': [
>         '../../mojo/mojo_base.gyp:mojo_common_lib',
>         '../../mojo/mojo_base.gyp:mojo_url_type_converters',
>         '../../mojo/mojo_public.gyp:mojo_cpp_bindings',
>         'test_mojo_bindings_mojom',
>       ],
>       'include_dirs': [
>         '..',
>       ],
>     },
> Then GYP gives me an error:
> 
> gyp: src/ios/web/ios_web_resources.gyp not found
> 
> Adding another gyp file and one extra target does not look like a better
> approach. Is there any way to avoid adding ios_web_resources just for the
test?
> 
That gyp error was related to other changes. Done.

https://codereview.chromium.org/1956113002/diff/120001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1956113002/diff/120001/ios/web/BUILD.gn#newco...
ios/web/BUILD.gn:291: "//ios/web/test:mojo_bindings",
This gives me gn error: 
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn...

[Ken Rockot(use gerrit already), 2016-05-12 21:13:06]

Was just looking at that GN failure. I think you may unfortunately have to
sprinkle some ifdefs in
mojo/public/cpp/bindings/lib/native_struct_serialization.h to block out use of
//ipc stuff on IOS. You'll also then want to remove this dependency on //ipc
when is_ios.

As a general question/comment: is there a reason you didn't use
mojo/public/cpp/system headers instead of mojo/public/c/system? The C++ library
does a lot of work for you and can probably reduce the size of this code.

I think you would at least benefit from using the mojo::Watcher defined there.
This provides you with a scoped watch and abstracts away calls to MojoWatch and
MojoCancelWatch (which you are completely missing btw). But probably even for
MessagePipe operations, the C++ library is better than using the C library
directly.

https://codereview.chromium.org/1956113002/diff/40001/ios/web/webui/mojo_faca...
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/40001/ios/web/webui/mojo_faca...
ios/web/webui/mojo_facade.mm:125: if (name ==
"service_provider.connectToService") {
I mean I guess efficiency is already tossed out because of WKWebView
constraints, but maybe this could be a map instead of a series of string
compares?

https://codereview.chromium.org/1956113002/diff/120001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/120001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:26: // Wraps Mojo integer into |base::Value| as
|TYPE_INTEGER|.
Can you make this comment explicit? Wraps a MojoResult or MojoHandle

https://codereview.chromium.org/1956113002/diff/120001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:314: CHECK(args->GetString("callback", &callback));
Am I understanding correctly that we take the callback function as a string and
evaluate it every time the handle signals?

[Eugene But (OOO till 7-30), 2016-05-13 02:54:26]

Thanks! Using C++ library allowed to throw away a singleton, which is great. I
switched to C++ API everywhere for consistency.

Will fix GN problem in a parallel CL.

PTAL.

https://codereview.chromium.org/1956113002/diff/40001/ios/web/webui/mojo_faca...
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/40001/ios/web/webui/mojo_faca...
ios/web/webui/mojo_facade.mm:125: if (name ==
"service_provider.connectToService") {
On 2016/05/12 21:13:05, Ken Rockot wrote:
> I mean I guess efficiency is already tossed out because of WKWebView
> constraints, but maybe this could be a map instead of a series of string
> compares?
I doubt that you will see any performance difference for 7 handlers :).
Having a map from string to Callback will make this code move verbose and less
readable. 

And yeah this calls involves IPC, JSON encoding/decoding and JavaScript
execution... so I would not try to optimize anything on string comparison.

https://codereview.chromium.org/1956113002/diff/120001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/120001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:26: // Wraps Mojo integer into |base::Value| as
|TYPE_INTEGER|.
On 2016/05/12 21:13:06, Ken Rockot wrote:
> Can you make this comment explicit? Wraps a MojoResult or MojoHandle

This method now takes also int, so changed comments to more generic.

https://codereview.chromium.org/1956113002/diff/120001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:314: CHECK(args->GetString("callback", &callback));
On 2016/05/12 21:13:06, Ken Rockot wrote:
> Am I understanding correctly that we take the callback function as a string
and
> evaluate it every time the handle signals?
Yes. But I changed this to callback ID, as I need to also pass MojoResult.

[Ken Rockot(use gerrit already), 2016-05-13 15:21:45]

Looks great, just some nits.

Also would it be possible for you to test message read and write?

And what about watching? Could you use the facade to watch one end of a pipe,
write to the other end, and verify that signalWatch would be called with the
expected callback ID and result?

https://codereview.chromium.org/1956113002/diff/160001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/160001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:135: // Extract options when this codepath is hit.
nit: Can you just CHECK that the dictionary is empty for now? There are no
options defined for CreateMessagePipe yet. No real benefit to leaving a
NOTIMPLEMENTED here.

https://codereview.chromium.org/1956113002/diff/160001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:263: watch_it->second.Cancel();
nit: No need to call this explicitly, the Watcher's dtor cancels

[Eugene But (OOO till 7-30), 2016-05-14 00:05:09]

Write tests for Read, Write, Watch. Let me know if you think that I need tests
for CancelWatch as well. PTAL.

https://codereview.chromium.org/1956113002/diff/160001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/160001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:135: // Extract options when this codepath is hit.
On 2016/05/13 15:21:45, Ken Rockot wrote:
> nit: Can you just CHECK that the dictionary is empty for now? There are no
> options defined for CreateMessagePipe yet. No real benefit to leaving a
> NOTIMPLEMENTED here.

Done.

https://codereview.chromium.org/1956113002/diff/160001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:263: watch_it->second.Cancel();
On 2016/05/13 15:21:45, Ken Rockot wrote:
> nit: No need to call this explicitly, the Watcher's dtor cancels
Cool. Done.

[Eugene But (OOO till 7-30), 2016-05-14 04:02:42]

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

[commit-bot: I haz the power, 2016-05-14 04:03:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/180001

[commit-bot: I haz the power, 2016-05-14 04:10:41]

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

[commit-bot: I haz the power, 2016-05-14 04:10:42]

Dry run: Try jobs failed on following builders:
  ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[Ken Rockot(use gerrit already), 2016-05-15 02:06:41]

CancelWatch isn't easy to test and it's a pretty simple implementation here, so
I don't think it's worth writing a test.

It might be kind of nice in general to test that for all of these messages, the
argument values make it through to the handlers as expected. I could see that
working if you added a layer of indirection between the facade and its
implementation, so you could mock the impl... but again I don't actually think
this is worth the trouble.

So LGTM pending a fix for GN deps (see inline comment)

https://codereview.chromium.org/1956113002/diff/180001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade_unittest.mm (right):

https://codereview.chromium.org/1956113002/diff/180001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade_unittest.mm:66: mojo::edk::Init();
Please remove these EDK calls from this file and change
components/test/run_all_unittests.cc to always initialize mojo (right now it
does it on Chrome OS only for some reason).

Also, components_unittests only depends on //mojo/edk/system for non-iOS only,
but this isn't really correct. It should instead depend on //mojo/edk/embedder,
and it should do so on all platforms.

This should resolve your new GN errors.

[Eugene But (OOO till 7-30), 2016-05-16 16:14:56]

eugenebut@chromium.org changed reviewers:
+ jyquinn@chromium.org

[Eugene But (OOO till 7-30), 2016-05-16 16:14:57]

Thanks! +Jackie

https://codereview.chromium.org/1956113002/diff/180001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade_unittest.mm (right):

https://codereview.chromium.org/1956113002/diff/180001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade_unittest.mm:66: mojo::edk::Init();
On 2016/05/15 02:06:41, Ken Rockot wrote:
> Please remove these EDK calls from this file and change
> components/test/run_all_unittests.cc to always initialize mojo (right now it
> does it on Chrome OS only for some reason).
> 
> Also, components_unittests only depends on //mojo/edk/system for non-iOS only,
> but this isn't really correct. It should instead depend on
//mojo/edk/embedder,
> and it should do so on all platforms.
> 
> This should resolve your new GN errors.
1.) Moved Mojo initialization to ios/web/test/run_all_unittests.cc

2.) //mojo/edk/embedder dependency leads to this error:

ERROR at //build/config/ios/rules.gni:114:3: Dependency not allowed.
  executable(_generate_executable) {
  ^---------------------------------
The item //components:components_unittests_generate_executable

//mojo/edk/system works well though.

[Ken Rockot(use gerrit already), 2016-05-16 16:22:23]

Oh right - I guess we explicitly forbid direct dependency on
//mojo/edk/embedder. My bad. this is fine then.

https://codereview.chromium.org/1956113002/diff/200001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1956113002/diff/200001/ios/web/BUILD.gn#newco...
ios/web/BUILD.gn:18: "//mojo/public:sdk",
Hmm - I'd rather we delete this target (in fact I thought we already did). It's
a weird target. Please just depend on the specific targets you use
(//mojo/public/cpp/system, //mojo/public/cpp/bindings, etc)

[Eugene But (OOO till 7-30), 2016-05-16 16:40:02]

https://codereview.chromium.org/1956113002/diff/200001/ios/web/BUILD.gn
File ios/web/BUILD.gn (right):

https://codereview.chromium.org/1956113002/diff/200001/ios/web/BUILD.gn#newco...
ios/web/BUILD.gn:18: "//mojo/public:sdk",
On 2016/05/16 16:22:23, Ken Rockot wrote:
> Hmm - I'd rather we delete this target (in fact I thought we already did).
It's
> a weird target. Please just depend on the specific targets you use
> (//mojo/public/cpp/system, //mojo/public/cpp/bindings, etc)

Done.

[Eugene But (OOO till 7-30), 2016-05-16 18:39:04]

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

[commit-bot: I haz the power, 2016-05-16 18:39:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/220001

[commit-bot: I haz the power, 2016-05-16 19:56:25]

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

[commit-bot: I haz the power, 2016-05-16 19:56:26]

Dry run: This issue passed the CQ dry run.

[Jackie Quinn, 2016-05-16 22:28:17]

Some comments on commenting and some questions about how things work.

https://codereview.chromium.org/1956113002/diff/220001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/ios_web.gyp#ne...
ios/web/ios_web.gyp:506: 'target_name': 'test_mojo_bindings_mojom',
Should (can?) this target be in the test.gypi file?

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.h (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:57: // from WebUI page. This method either succeeds
or crashes the app.
"crashes the app" seems like an unfortunate outcome. Could we not do that, or
add a comment here why the app _should_ crash if the message extraction fails?

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:64: // contain serviceName key, which is a string
representing a service name).
Unpaired paren? Also should "serviceName" be in quotes, as other keys are below?

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:80: // operation.). Returns a dictionary with
"handle0" and "handle1" keys (the
So much punctuation! Remove the periods inside the parentheses? Also take out
(ports), or s/endpoints/ports.

Also if it returns a dictionary, why not return DictionaryValue? Seeing the
implementation, I assume it's because you can't assign a DictionaryValue to a
Value, but please confirm. Same question for methods below.

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:86: // dictionary which must contain the following
keys: "handle" (a number
Might be more readable to format these (and above) as bullet points? i.e.

// dictionary which must contain the following keys:
// - handle: a number representing MojoHandle, the endpoint to write to
// - buffer: a dictionary representing the message data, may be empty
// - handles: ... etc

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:166: flags = MOJO_WRITE_MESSAGE_FLAG_NONE;
Is this redundant, given that it's initialized as MOJO_WRITE_MESSAGE_FLAG_NONE
(or does GetAsInteger overwrite the value)?

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade_unittest.mm (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade_unittest.mm:267: NSArray* expected_message = @[ @9,
@2, @216 ];  // 2008 does not fit 8-bit.
How does 2008 become 216 then?

[Eugene But (OOO till 7-30), 2016-05-16 23:39:59]

Thanks! PTAL

https://codereview.chromium.org/1956113002/diff/220001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/ios_web.gyp#ne...
ios/web/ios_web.gyp:506: 'target_name': 'test_mojo_bindings_mojom',
On 2016/05/16 22:28:16, Jackie Quinn wrote:
> Should (can?) this target be in the test.gypi file?
We don't have test.gypi in web. |test_mojo_bindings_mojom| target is used in
|ios_web_test_support| which is defined here.

So I guess the answer is no :)

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.h (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:57: // from WebUI page. This method either succeeds
or crashes the app.
On 2016/05/16 22:28:16, Jackie Quinn wrote:
> "crashes the app" seems like an unfortunate outcome. Could we not do that, or
> add a comment here why the app _should_ crash if the message extraction fails?
This is what Mojo does on other platforms. Updated the comments.

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:64: // contain serviceName key, which is a string
representing a service name).
On 2016/05/16 22:28:16, Jackie Quinn wrote:
> Unpaired paren? Also should "serviceName" be in quotes, as other keys are
below?

Done.

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:80: // operation.). Returns a dictionary with
"handle0" and "handle1" keys (the
On 2016/05/16 22:28:16, Jackie Quinn wrote:
> So much punctuation! Remove the periods inside the parentheses? Also take out
> (ports), or s/endpoints/ports.
> 
> Also if it returns a dictionary, why not return DictionaryValue? Seeing the
> implementation, I assume it's because you can't assign a DictionaryValue to a
> Value, but please confirm. Same question for methods below.
Updated the comments.

Correct. I can not assign std::unique_ptr<base::DictionaryValue> to
std::unique_ptr<base::Value>.

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:86: // dictionary which must contain the following
keys: "handle" (a number
On 2016/05/16 22:28:16, Jackie Quinn wrote:
> Might be more readable to format these (and above) as bullet points? i.e.
> 
> // dictionary which must contain the following keys:
> // - handle: a number representing MojoHandle, the endpoint to write to
> // - buffer: a dictionary representing the message data, may be empty
> // - handles: ... etc

Done.

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.mm (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.mm:166: flags = MOJO_WRITE_MESSAGE_FLAG_NONE;
On 2016/05/16 22:28:17, Jackie Quinn wrote:
> Is this redundant, given that it's initialized as MOJO_WRITE_MESSAGE_FLAG_NONE
> (or does GetAsInteger overwrite the value)?
GetAsInteger does not override anything, but if it did it would not be wrong.
|GetAsInteger| contract does not guarantee that it's argument will not be
changed.

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade_unittest.mm (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade_unittest.mm:267: NSArray* expected_message = @[ @9,
@2, @216 ];  // 2008 does not fit 8-bit.
On 2016/05/16 22:28:17, Jackie Quinn wrote:
> How does 2008 become 216 then?
2008 is 0111 1101 1000
216 is    0000 1101 1000

[Jackie Quinn, 2016-05-17 23:07:06]

lgtm

https://codereview.chromium.org/1956113002/diff/220001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/ios_web.gyp#ne...
ios/web/ios_web.gyp:506: 'target_name': 'test_mojo_bindings_mojom',
On 2016/05/16 23:39:58, Eugene But wrote:
> On 2016/05/16 22:28:16, Jackie Quinn wrote:
> > Should (can?) this target be in the test.gypi file?
> We don't have test.gypi in web. |test_mojo_bindings_mojom| target is used in
> |ios_web_test_support| which is defined here.
> 
> So I guess the answer is no :)

heh. okay. It just seems like all the testing stuff should live with the
unittests rather than in the main gyp file.

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
File ios/web/webui/mojo_facade.h (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/webui/mojo_fac...
ios/web/webui/mojo_facade.h:57: // from WebUI page. This method either succeeds
or crashes the app.
On 2016/05/16 23:39:58, Eugene But wrote:
> On 2016/05/16 22:28:16, Jackie Quinn wrote:
> > "crashes the app" seems like an unfortunate outcome. Could we not do that,
or
> > add a comment here why the app _should_ crash if the message extraction
fails?
> This is what Mojo does on other platforms. Updated the comments.

Thanks :-)

[Eugene But (OOO till 7-30), 2016-05-18 00:09:32]

Thanks!

https://codereview.chromium.org/1956113002/diff/220001/ios/web/ios_web.gyp
File ios/web/ios_web.gyp (right):

https://codereview.chromium.org/1956113002/diff/220001/ios/web/ios_web.gyp#ne...
ios/web/ios_web.gyp:506: 'target_name': 'test_mojo_bindings_mojom',
On 2016/05/17 23:07:06, Jackie Quinn wrote:
> On 2016/05/16 23:39:58, Eugene But wrote:
> > On 2016/05/16 22:28:16, Jackie Quinn wrote:
> > > Should (can?) this target be in the test.gypi file?
> > We don't have test.gypi in web. |test_mojo_bindings_mojom| target is used in
> > |ios_web_test_support| which is defined here.
> > 
> > So I guess the answer is no :)
> 
> heh. okay. It just seems like all the testing stuff should live with the
> unittests rather than in the main gyp file.
Well not all the tests are unit tests. We have 2 other types of tests:
integration tests and EG tests. BTW in GN world there is only one .gn file per
directory.

[Eugene But (OOO till 7-30), 2016-05-18 00:09:41]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2016-05-18 00:09:41]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1956113002/#ps240001
(title: "Updated the comments.")

[commit-bot: I haz the power, 2016-05-18 00:10:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1956113002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1956113002/240001

[commit-bot: I haz the power, 2016-05-18 01:59:06]

Description was changed from

==========
[ios Mojo] iOS facade class for Mojo API.

On all platforms except iOS Mojo API is implemented natively. On iOS
Chrome is not allowed to use Bling and uses WKWebView. So the only way
to implement Mojo API is hybrid approach:
 - API frontent is implemented in JS;
 - frontend packs function name and arguments into JSON and sends to
   the native code;
 - window.prompt is used as communication channel, because this is the
   only way to send synchronous message from WKWebView;
 - native code will pass JSON to facade;
 - facade will respond with JSON;
 - native code will send JSON back to JS via window.prompt result;
 - JS frontent will unpack JSON and will return the result to the caller;
 
This CL introduces a facade class which takes JSON as in input, passes
it to Mojo and then returns the result as JSON.
 
Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08B...

BUG=567809
==========

to

==========
[ios Mojo] iOS facade class for Mojo API.

On all platforms except iOS Mojo API is implemented natively. On iOS
Chrome is not allowed to use Bling and uses WKWebView. So the only way
to implement Mojo API is hybrid approach:
 - API frontent is implemented in JS;
 - frontend packs function name and arguments into JSON and sends to
   the native code;
 - window.prompt is used as communication channel, because this is the
   only way to send synchronous message from WKWebView;
 - native code will pass JSON to facade;
 - facade will respond with JSON;
 - native code will send JSON back to JS via window.prompt result;
 - JS frontent will unpack JSON and will return the result to the caller;
 
This CL introduces a facade class which takes JSON as in input, passes
it to Mojo and then returns the result as JSON.
 
Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08B...

BUG=567809
==========

[commit-bot: I haz the power, 2016-05-18 01:59:07]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-05-18 02:00:37]

Description was changed from

==========
[ios Mojo] iOS facade class for Mojo API.

On all platforms except iOS Mojo API is implemented natively. On iOS
Chrome is not allowed to use Bling and uses WKWebView. So the only way
to implement Mojo API is hybrid approach:
 - API frontent is implemented in JS;
 - frontend packs function name and arguments into JSON and sends to
   the native code;
 - window.prompt is used as communication channel, because this is the
   only way to send synchronous message from WKWebView;
 - native code will pass JSON to facade;
 - facade will respond with JSON;
 - native code will send JSON back to JS via window.prompt result;
 - JS frontent will unpack JSON and will return the result to the caller;
 
This CL introduces a facade class which takes JSON as in input, passes
it to Mojo and then returns the result as JSON.
 
Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08B...

BUG=567809
==========

to

==========
[ios Mojo] iOS facade class for Mojo API.

On all platforms except iOS Mojo API is implemented natively. On iOS
Chrome is not allowed to use Bling and uses WKWebView. So the only way
to implement Mojo API is hybrid approach:
 - API frontent is implemented in JS;
 - frontend packs function name and arguments into JSON and sends to
   the native code;
 - window.prompt is used as communication channel, because this is the
   only way to send synchronous message from WKWebView;
 - native code will pass JSON to facade;
 - facade will respond with JSON;
 - native code will send JSON back to JS via window.prompt result;
 - JS frontent will unpack JSON and will return the result to the caller;

This CL introduces a facade class which takes JSON as in input, passes
it to Mojo and then returns the result as JSON.

Design doc:
https://docs.google.com/document/d/1iUJXX_zoY2BbnecpfxjznOeasf29KUzl_9S_LG08B...

BUG=567809

Committed: https://crrev.com/91b474353e27a83bd990556572418c2c9750abed
Cr-Commit-Position: refs/heads/master@{#394313}
==========

[commit-bot: I haz the power, 2016-05-18 02:00:39]

Patchset 13 (id:??) landed as
https://crrev.com/91b474353e27a83bd990556572418c2c9750abed
Cr-Commit-Position: refs/heads/master@{#394313}

[Nico, 2016-05-18 17:28:25]

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

[Nico, 2016-05-18 17:28:26]

Together with https://codereview.chromium.org/1984353002/  , this broke the ios
gn bots like so:

https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn...

ERROR at //ios/web/public/test/web_view_interaction_test_util.mm:10:11: Can't
include this header from here.
#include "base/strings/stringprintf.h"
          ^--------------------------
The target:
  //ios/web:earl_grey_test_support
is including a file from the target:
  //base:base

It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.

Dependency chain (there may also be others):
  //ios/web:earl_grey_test_support -->
  //ios/web:test_support --[private]-->
  //ios/web:web --[private]-->
  //base:base


I think it's best to revert this change and reland the other one, but I'll leave
that to you iOS folks.

[Eugene But (OOO till 7-30), 2016-05-18 18:24:08]

+baxley@, but this looks like already fixed.

On Wed, May 18, 2016 at 10:28 AM, <thakis@chromium.org> wrote:

> Together with https://codereview.chromium.org/1984353002/ , this broke
> the ios
> gn bots like so:
>
>
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn...
>
> ERROR at //ios/web/public/test/web_view_interaction_test_util.mm:10:11:
> Can't
> include this header from here.
> #include "base/strings/stringprintf.h"
> ^--------------------------
> The target:
> //ios/web:earl_grey_test_support
> is including a file from the target:
> //base:base
>
> It's usually best to depend directly on the destination target.
> In some cases, the destination target is considered a subcomponent
> of an intermediate target. In this case, the intermediate target
> should depend publicly on the destination to forward the ability
> to include headers.
>
> Dependency chain (there may also be others):
> //ios/web:earl_grey_test_support -->
> //ios/web:test_support --[private]-->
> //ios/web:web --[private]-->
> //base:base
>
>
> I think it's best to revert this change and reland the other one, but I'll
> leave
> that to you iOS folks.
>
> https://codereview.chromium.org/1956113002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sdefresne, 2016-05-18 18:34:28]

On 2016/05/18 18:24:08, Eugene But wrote:
> +baxley@, but this looks like already fixed.

thakis reverted my CL enabling "--check" for "//ios".

I've updated my CL with a fourth patchset to fix the regression introduced by
this CL, see https://codereview.chromium.org/1984353002/.