Re: Question about "Onion Soup" and mojo usage from blink modules

[Kentaro Hara]

On Fri, Feb 5, 2016 at 12:27 AM, Shalamov, Alexander <alexander...@intel.com> wrote:

Hello Elliott,

I am in the process of removing extra layers from NFC module and rewriting patches that are not yet landed / reviewed.
I've checked status of battery [1] and geolocation [2] refactoring and it looks like there is no consensus about how mojo services would be used.

Can I use mojo services directly from modules (WebKit/Source/modules) or I need to keep proxies in platform layer?
Direct dependency to mojo can eliminate need for dispatchers and conversion between mojo and platform proxy classes.

I'd prefer allowing the {core/, modules/} => mojo dependency and remove the indirection layer in platform/, as darin suggested.

Elliott?

 

At the moment nfc codepath looks like:

navigator.nfc.push(NFCMessage)
   convert to platform::NFCMessage
   nfcDispatcher->push(platform::NFCMessage)
       convert platform::NFCMessage to mojo::NFCMessagePtr
       mojoNFCServiceProxy->push(mojo::NFCMessagePtr)

Can be:

navigator.nfc.push(NFCMessage)
   convert NFCMessage to mojo::NFCMessagePtr
   mojoNFCServiceProxy->push(mojo::NFCMessagePtr)

According to Onion Soup [3] document, only platform can depend on mojo.
Is there a technical / architectural limitation that prohibits dependency from blink modules to mojo services?

[1] https://codereview.chromium.org/1538803002
[2] https://codereview.chromium.org/1367853002
[3] https://docs.google.com/document/d/13XsbaBz7A2H0PZIdFcytHf5-fVOlAfkLlIUKhxKzs44/edit#heading=h.tu76yuktjdtq

Best regards,
Alex

--

Kentaro Hara, Tokyo, Japan

[Elliott Sprehn]

I don't want to allow this until Mojo can generate data types that are compatible with WTF. Right now anything that uses mojo::String is causing string copies, the more you use it, the worse things are. Same with vectors or anything else.

[Kentaro Hara]

On Fri, Feb 5, 2016 at 9:16 AM, Elliott Sprehn <esp...@chromium.org> wrote:

I don't want to allow this until Mojo can generate data types that are compatible with WTF. Right now anything that uses mojo::String is causing string copies, the more you use it, the worse things are. Same with vectors or anything else.

When platform/ calls Mojo, we'll anyway need to implement the conversion from WTF types to Mojo types, won't it? (i.e., how helpful is it to restrict Mojo usage only to platform/ from the perspective of efficient type conversions?)

I think rockot@ is working on the WTF => Mojo type conversion (on demand).

[Ken Rockot]

Yuzhu is currently working on better array and string types from mojom. Our intent (IIRC) is to generate std::vector and std::string for Chromium, and WebVector and WebString for Blink.

[Elliott Sprehn]

Please don't generate WebVector, that still requires copies. You want WTF::Vector and WTF::String. :)

[Ken Rockot]

Got it :)

[Yuzhu Shen]

Tracking bug: https://code.google.com/p/chromium/issues/detail?id=583738

On Thu, Feb 4, 2016 at 4:24 PM, Ken Rockot <roc...@chromium.org> wrote:

[Kentaro Hara]

On Fri, Feb 5, 2016 at 9:24 AM, Ken Rockot <roc...@chromium.org> wrote:

Yuzhu is currently working on better array and string types from mojom. Our intent (IIRC) is to generate std::vector and std::string for Chromium, and WebVector and WebString for Blink.

Why do we still need the abstraction type like WebXXX? i.e., would it be possible to just implement a conversion between Mojo and WTF::Vector/String etc?

[Elliott Sprehn]

On Thu, Feb 4, 2016 at 4:22 PM, Kentaro Hara <har...@chromium.org> wrote:

On Fri, Feb 5, 2016 at 9:16 AM, Elliott Sprehn <esp...@chromium.org> wrote:

I don't want to allow this until Mojo can generate data types that are compatible with WTF. Right now anything that uses mojo::String is causing string copies, the more you use it, the worse things are. Same with vectors or anything else.

When platform/ calls Mojo, we'll anyway need to implement the conversion from WTF types to Mojo types, won't it? (i.e., how helpful is it to restrict Mojo usage only to platform/ from the perspective of efficient type conversions?)

You're less likely to write a bunch of eager conversion code, or loops that do conversions, etc. You end up writing one way conversions WTF -> Mojo, instead of converting back and forth.

- E

[Kentaro Hara]

Thanks, my question is resolved :)

[Kentaro Hara]

Hmm. I understand your point. 

That said, I'm not quite sure if it's a good idea to introduce a dispatcher class just for that reason. I think the advantage of the Onion Soup is removing unnecessary layering (as the project name implies :-)

We might want to hear other opinions.

==== Original email from Shalamov ====

I am in the process of removing extra layers from NFC module and rewriting patches that are not yet landed / reviewed.
I've checked status of battery [1] and geolocation [2] refactoring and it looks like there is no consensus about how mojo services would be used.

Can I use mojo services directly from modules (WebKit/Source/modules) or I need to keep proxies in platform layer?
Direct dependency to mojo can eliminate need for dispatchers and conversion between mojo and platform proxy classes.

At the moment nfc codepath looks like:

navigator.nfc.push(NFCMessage)
   convert to platform::NFCMessage
   nfcDispatcher->push(platform::NFCMessage)
       convert platform::NFCMessage to mojo::NFCMessagePtr
       mojoNFCServiceProxy->push(mojo::NFCMessagePtr)

Can be:

navigator.nfc.push(NFCMessage)
   convert NFCMessage to mojo::NFCMessagePtr
   mojoNFCServiceProxy->push(mojo::NFCMessagePtr)

According to Onion Soup [3] document, only platform can depend on mojo.
Is there a technical / architectural limitation that prohibits dependency from blink modules to mojo services?

[1] https://codereview.chromium.org/1538803002
[2] https://codereview.chromium.org/1367853002
[3] https://docs.google.com/document/d/13XsbaBz7A2H0PZIdFcytHf5-fVOlAfkLlIUKhxKzs44/edit#heading=h.tu76yuktjdtq

[John Abd-El-Malek]

On Thu, Feb 4, 2016 at 4:42 PM, Kentaro Hara <har...@chromium.org> wrote:

On Fri, Feb 5, 2016 at 9:31 AM, Elliott Sprehn <esp...@chromium.org> wrote:

On Thu, Feb 4, 2016 at 4:22 PM, Kentaro Hara <har...@chromium.org> wrote:

On Fri, Feb 5, 2016 at 9:16 AM, Elliott Sprehn <esp...@chromium.org> wrote:

I don't want to allow this until Mojo can generate data types that are compatible with WTF. Right now anything that uses mojo::String is causing string copies, the more you use it, the worse things are. Same with vectors or anything else.

When platform/ calls Mojo, we'll anyway need to implement the conversion from WTF types to Mojo types, won't it? (i.e., how helpful is it to restrict Mojo usage only to platform/ from the perspective of efficient type conversions?)

You're less likely to write a bunch of eager conversion code, or loops that do conversions, etc. You end up writing one way conversions WTF -> Mojo, instead of converting back and forth.

Hmm. I understand your point. 

Can you elaborate on this point more? I don't quite understand how it would be more likely to add extra code if Mojo is called from modules vs platform.

Regardless, if the concern is that code in modules would be less efficient than platform, can that be addressed by having a small set of reviewers for callers of Mojo from core/modules/platform (i.e. the platform architecture team)?

In general, I agree that it's good to remove as many layers as possible in this conversion, and specifically this patch.

[Elliott Sprehn]

You need an interface that converts from WTF types used in blink, to mojo types used in the IPC (and the other way). If that interface is in platform or core shouldn't mean more code. Forcing it to be in platform forces you to keep the conversion layer at the boundaries.

This was a very deliberate choice in Onion Soup to scope what std lib classes get used in core, and what conversions end up proliferated into the codebase. Content today has lots of issues causing string copies and conversions all over the place.

Once the bindings for mojo are in terms of WTF types you can put this abstraction layer in core/.

[Darin Fisher]

Aren't these copies already present with the current Blink API? The danger is already present with WebVector and WebString, no?

If you create extra abstractions in platform/ now, then isn't that just technical debt to be removed later?

I agree we should generate Blink friendly bindings. Maybe there is a rational way to do this in parallel?

-Darin

[John Abd-El-Malek]

On Thu, Feb 4, 2016 at 4:56 PM, Elliott Sprehn <esp...@chromium.org> wrote:

You need an interface that converts from WTF types used in blink, to mojo types used in the IPC (and the other way). If that interface is in platform or core shouldn't mean more code.

Ok so far I follow :)

 

Forcing it to be in platform forces you to keep the conversion layer at the boundaries.

I'm not following this point.

I totally get the point below about not mixing stl and WTF in core. I just don't understand why code in core that calls mojo can't also be considered "at the boundary"? Seems that code reviewers can be careful to ensure that stl isn't used except to call mojo, until mojo bindings support wtf natively?

[Ojan Vafai]

Elliott, Kentaro and I chatted a bit about this. Elliott/Kentaro: please chime in if I misrepresent the discussion in any way.

DISCUSSION SUMMARY

We agree that a dispatcher class that only wraps mojo types is not good, but, we think most of the time we have these it's an indication of poor layering and we should just fix the layering.

In the Onion Soup model platform is exposing a set high-level OS APIs and core/modules is implementing the web-specific behavior. So, more of the logic should move from modules to platform.

BatteryDispatcher is a good example of this. BatteryDispatcher should move wholesale to platform (probably along with BatteryStatus) and BatteryManager/NavigatorBattery should use those classes directly out of platform. That removes the need for BatteryStatusDispatcher and improves the layering story at the same time.

We think it will be very rare that the right layer to expose to core/modules will be the raw mojo API because those APIs will be lower-level, so this won't be a common problem in the short-term and we won't accrue a lot of technical debt. In the long-term, we'll have mojo bindings generating WTF types and can use mojo directly in core/modules in the rare cases where it's appropriate.

Sound OK?

WHY THIS MATTERS

Not allowing mojo in core/modules until the WTF bindings issue is resolved comes down to relying on tooling to enforce good layering instead of codereview, which:

• leads to long-term maintenance as the set of appropriate reviewers changes over time.
  
• catches more cases that human reviewers will miss
  
• scales better to growing more reviewers as there are fewer subtleties they need to wrap their heads around. For example, the reviewers for the battery API inside of core/modules shouldn't need to understand all the subtleties of the core/modules/platform architecture to make changes to the battery code.

If we disallow stl entirely inside of core, then we can have the tooling (mostly) enforce that instead of codereview. We can't do that if there are subjective rules about when stl is allowed.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To post to this group, send email to platform-arc...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CALhVsw03%3D50-ej7RDoAuDF1HPwx_H64MX6OAEohawA8xrUn%2BPw%40mail.gmail.com.

[Darin Fisher]

Makes sense.

Maybe a some things worth considering as well:

1- Perhaps it would be ideal to not have a monolithic platform library at all? Mojo is designed to enable this. Wouldn't it be better to have all client-side code for battery in one place and then all service-side code in another? Avoiding libraries like platform/ that are the union of many (most) features is a good thing, right?

2- It is not a lot of work to make WTF friendly mojo bindings, so seems best to plan for what you would do if you had that, and then treat the interim as temporary.

-Darin

[Yuki Shiino]

Ojan's summary is very helpful for me, and I basically agree the design policy.

However, I think the battery status is that rare case to be exposed directly to modules/ because there is nothing complicated to be abstracted in platform/.  I think

    device/battery/battery_monitor.mojom and

    device/battery/battery_status.mojom

are already good abstracted interfaces, and I have nothing to add to it within platform/.

Note that what Battery Status Web API should provide is a set of simple values of charging time, discharging time, etc. and what battery_monitor.mojom and battery_status.mojom provide is just those things, no more and no less.

Plus, note that

    core/frame/PlatformEventController.h and

    core/frame/PlatformEventDispatcher.h

live in core/, and BatteryManager/BatteryDispatcher is now following the PlatformEvent{Controller,Dispatcher} model.  So we cannot simply move modules/battery/BatteryDispatcher into platform/battery/.  In order to support PlatformEvent{Controller,Dispatcher} model, we need to keep BatteryDispatcher in modules/battery/.

Ojan> BatteryManager/NavigatorBattery should use those classes directly out of platform.

I think this idea conflicts with the PlatformEvent{Controller,Dispatcher} model.

I think that the battery status is very simple, and this is a case that we'd like to expose directly to modules/ once Mojo gets to support the type conversion.

Does this sound reasonable?

Anyway, by the time Mojo supports WTF types, I'll keep to limit the use of Mojo in platform/.