[Status Report] Migration to new Mojo types - 11/30/2019

146 views
Skip to first unread message

Mario Sanchez Prada

unread,
Dec 2, 2019, 5:12:54 AM12/2/19
to chromium-mojo, platform-architecture-dev

Hi all,

First of all, apologies for having missed the previous bi-weekly report. An unfortunate combination of me being on holidays 1 week after BlinkOn with the fact that my laptop's motherboard died right on Nov 15th made it hard for me to send that report, so I'm updating now with the progress from the entire month, sorry about that.

As usual, we kept focused on converting types outside of Blink + migrating usages of InterfaceProvider API to the new BrowserInterfaceBroker, which naturally helped getting rid of the few old mojo types remaining inside Blink, getting us to the point where there's only one instance left! We'll remove that one now and then will propose a CL to enable presubmit errors inside Blink, so that no old types get now introduced. Note that these numbers now take into account the false positives that the grep query would find, which have been now subtracted from the numbers throughout the whole spreadsheet, see more details at [1].

As for //content and other directories, steady progress in there as well, with old mojo types in now reduced to less than 25% and 60% in those categories compared to what numbers used to look like one month ago, bringing the current progress of the project to 91% if we compared with the date we started, as you can see in the "Project Completion (%)" table I've added to the bottom of this email, after the graph reports.


10/23/2019 10/31/2019 11/7/2019 11/15/2019 11/23/2019 11/30/2019
Blink 6 3 3 3 3 1
Content 339 201 176 161 110 42
Other 957 721 647 607 538 408
TOTAL 1302 925 826 771 651 451



10/23/2019 10/31/2019 11/7/2019 11/15/2019 11/23/2019 11/30/2019
common/associated_interfaces 0 0 0 0 0 0
common/loader 0 0 0 0 0 0
public/common 0 0 0 0 0 0
public/platform 0 0 0 0 0 0
renderer/controller 0 0 0 0 0 0
renderer/core 2 2 2 2 2 0
renderer/modules 0 0 0 0 0 0
renderer/platform 4 1 1 1 1 1
tools/blinkpy 0 0 0 0 0 0
TOTAL 6 3 3 3 3 1


10/23/2019 10/31/2019 11/7/2019 11/15/2019 11/23/2019 11/30/2019
app_shim_remote_cocoa 2 0 0 0 0 0
browser 256 155 134 122 76 27
child 0 0 0 0 0 0
common 9 2 1 1 1 1
gpu 0 0 0 0 0 0
ppapi_plugin 0 0 0 0 0 0
public 21 14 14 13 9 8
renderer 43 24 21 19 18 5
shell 0 0 0 0 0 0
test 7 5 5 5 5 0
utility 1 1 1 1 1 1
TOTAL 339 201 176 161 110 42



10/23/2019 10/31/2019 11/7/2019 11/15/2019 11/23/2019 11/30/2019
android_webview 7 6 5 4 1 0
ash 9 4 4 4 4 4
cc 1 1 1 1 1 1
chrome 144 130 125 108 101 82
chromecast 21 21 19 18 17 16
chromeos 35 10 10 10 10 11
components 71 57 44 40 38 37
device 2 2 2 2 2 2
docs 2 2 2 2 2 2
extensions 22 20 19 15 14 1
fuchsia 14 13 13 13 13 13
google_apis 8 4 1 1 1 1
gpu 0 0 0 0 0 0
headless 1 1 1 1 1 0
ios 5 2 2 2 2 2
ipc 31 31 31 31 30 30
jingle 3 1 0 0 0 0
media 117 47 26 16 5 4
mojo 70 70 71 70 70 70
remoting 1 1 1 1 1 1
sandbox 0 0 0 0 0 0
services 367 289 269 268 225 131
skia 0 0 0 0 0 0
storage 1 1 0 0 0 0
ui 25 8 1 0 0 0
url 0 0 0 0 0 0
TOTAL 957 721 647 607 538 408


Project Completion (%)
Blink 99.77
Content 97.38
Other 86.71
Global 91.16

Find more detail in the base spreadsheet at:
https://docs.google.com/spreadsheets/d/1khYVrfnN74RSDqqXgCFlG9fI89IKWA9J32Ln_2ZcTaU

Thanks,
Mario

[1] https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/9vuGL48Lxik/6YscqLI5AwAJ

Mario Sanchez Prada

unread,
Dec 3, 2019, 4:55:48 AM12/3/19
to chromium-mojo, platform-architecture-dev, Jeremy Roman, hide...@chromium.org

Hi again,

After sending this mail yesterday, we had an internal discussion on what to do with //components/arc that we think might be worth bringing it up here as well, since a recent comment by Jeremy Roman in [1] made us realize not everyone might be aware of the issue at hand...

On 2/12/19 11:12, Mario Sanchez Prada wrote:

[...]


10/23/2019 10/31/2019 11/7/2019 11/15/2019 11/23/2019 11/30/2019
[...]
[...] [...] [...] [...] [...] [...]
components 71 57 44 40 38 37
[...]
[...] [...] [...] [...] [...] [...]
TOTAL 957 721 647 607 538 408

As you can see from the numbers above, there are 37 instances of old mojo types to be migrated inside //components according to the grep query we're using, and more than half of those 37 instances (20, to be exact) are inside //components/arc:

      3 components/arc/bluetooth/bluetooth_mojom_traits_unittest.cc
      2 components/arc/keymaster/arc_keymaster_bridge.cc
      1 components/arc/midis/arc_midis_bridge.cc
      2 components/arc/session/arc_bridge_host_impl.cc
      2 components/arc/session/arc_bridge_host_impl.h
      1 components/arc/session/arc_session_impl.cc
      5 components/arc/session/connection_holder.h
      2 components/arc/session/mojo_channel.h
      1 components/arc/timer/arc_timer_bridge.h
      1 components/arc/wake_lock/arc_wake_lock_bridge.h

We did an initial attempt to migrate those bits last month as part of crrev.com/c/1868870, but got a CR-1 because it seems that Android's libchrome repository is not yet ready to handle changes in the .mojom files from Chromium to the new types. The issue seems to be that those .mojom files are copied into the libchrome repo, which does not contain support for the new mojo types yet, so that process would not work anymore like that, see [2] for more details.

This is why we haven't prioritized converting types in //components/arc for now, but now we're approaching the completion of this migration project this question becomes relevant again: what to do with //components/arc?

Note that, as Ken commented in [3], the old and new mojo types are binary compatibles over the wire, so I believe there should be a way to move forward with the new types in the Chromium repo even if libchrome needs to stick to the old ones for a bit longer, but that would require some extra discussion, so adding Hidehiko to Cc here.

Thoughts?

Thanks,
Mario

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1940185/2#message-5cab01a211f6c201ef4e39a13c8a5c62e839f467
[2] https://chromium-review.googlesource.com/c/chromium/src/+/1868870/6#message-5a35c25604873c017644aca5d739f39ca452fe4f
[3] https://chromium-review.googlesource.com/c/chromium/src/+/1868870/6#message-77c2cadc1978b50d739b05cda7f64156b067fe51

Dave Tapuska

unread,
Dec 3, 2019, 9:38:57 AM12/3/19
to Mario Sanchez Prada, chromium-mojo, platform-architecture-dev, Jeremy Roman, hide...@chromium.org
Since they are binary compatible then it is reasonable to make this part of the process of copying them so that we can remove the legacy mojo interfaces entirely. 

Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy? Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?
dave.

--
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 view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/25c57f3a-7ffc-d2e2-71c1-a1c6d091fc54%40igalia.com.

Ken Rockot

unread,
Dec 3, 2019, 9:47:08 AM12/3/19
to Dave Tapuska, Mario Sanchez Prada, chromium-mojo, platform-architecture-dev, Jeremy Roman, Hidehiko Abe
On Tue, Dec 3, 2019 at 6:38 AM Dave Tapuska <dtap...@chromium.org> wrote:
Since they are binary compatible then it is reasonable to make this part of the process of copying them so that we can remove the legacy mojo interfaces entirely. 

Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy? Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?
dave.

I think it's been done manually, but AFAIK typically the roll happens in the other direction -- source of truth in cros repo, rolled into chromium.

Automating translation in that direction is challenging because the old notation for interface remotes is grammatically ambiguous.

I do agree that we want to get rid of these things though, so I would suggest we switch to rolling cross-repo mojoms in the opposite direction, where a correct sed transform is trivial.

It's also possible that none of the cross-repo mojoms change frequently enough for this to require special action at this time.

You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAHgVhZViJgSCqUWwCTwAy4F3PWO9SKGCp-yi9KFH_a-dUHtUkQ%40mail.gmail.com.

Mario Sanchez Prada

unread,
Dec 4, 2019, 4:49:14 AM12/4/19
to Ken Rockot, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, Hidehiko Abe

Hi,

On 3/12/19 15:46, 'Ken Rockot' via chromium-mojo wrote:

On Tue, Dec 3, 2019 at 6:38 AM Dave Tapuska <dtap...@chromium.org> wrote:
Since they are binary compatible then it is reasonable to make this part of the process of copying them so that we can remove the legacy mojo interfaces entirely. 

Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy? Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?
dave.

I think it's been done manually, but AFAIK typically the roll happens in the other direction -- source of truth in cros repo, rolled into chromium.

Do you mean "source of truth in libchrome (Android) repo?" It's the first time I see CrOS mentioned in this topic, that's where my question comes from :-)

Automating translation in that direction is challenging because the old notation for interface remotes is grammatically ambiguous.

I do agree that we want to get rid of these things though, so I would suggest we switch to rolling cross-repo mojoms in the opposite direction, where a correct sed transform is trivial.

I agree it's definitely easier to roll cross-repo mojoms in the opposite direction (i.e. source of truth in Chromium repo) for the reasons you mentioned, but I'm not sure what the actual next steps to make that happen would be... I assume some devs who work on the other repo should obviously get involved, but not sure who to ping on that. Any idea?

It's also possible that none of the cross-repo mojoms change frequently enough for this to require special action at this time.

Just to double-check I'm getting this right... what you mean here is that, we could actually go ahead and change the local copy of the .mojom files in Chromium as long as the original sources in the other repository don't change frequently, right?

We'd still need some agreement from the people working on that other repo on (1) that changes are not frequent and (2) that they would be happy to start off a point where .mojom file would not use the same -old- types in both repos and take it from there, but that would certainly unblock the conversions in Chromium repo.

WDYT?

Thanks again,
Mario


Mario Sanchez Prada

unread,
Dec 9, 2019, 7:33:08 AM12/9/19
to Ken Rockot, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, Hidehiko Abe
Hi again,

On 4/12/19 10:49, Mario Sanchez Prada wrote:
> [...]
>> I do agree that we want to get rid of these things though, so I would suggest we switch to rolling cross-repo mojoms in the opposite direction, where a correct sed transform is trivial.
>
> I agree it's definitely easier to roll cross-repo mojoms in the opposite direction (i.e. source of truth in Chromium repo) for the reasons you mentioned, but I'm not sure what the actual next steps to make that happen would be... I assume some devs who work on the other repo should obviously get involved, but not sure who to ping on that. Any idea?

Pinging here after a few days.

>> It's also possible that none of the cross-repo mojoms change frequently enough for this to require special action at this time.
>
> Just to double-check I'm getting this right... what you mean here is that, we could actually go ahead and change the local /copy/ of the .mojom files in Chromium *as long as* the original sources in the other repository don't change frequently, right?
>
> We'd still need some agreement from the people working on that other repo on (1) that changes are not frequent and (2) that they would be happy to start off a point where .mojom file would not use the same -old- types in both repos and take it from there, but that would certainly unblock the conversions in Chromium repo.
>
> WDYT?

We'd still need to clarify this part as well, in order to determine how to move forward.

Thanks again,
Mario

Hidehiko Abe

unread,
Dec 19, 2019, 10:51:13 AM12/19/19
to Mario Sanchez Prada, Ken Rockot, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, Yusuke Sato, Josh Horwich, Elijah Taylor
Some questions seem based on some misunderstanding of the current situation, so let me give it a try to explain.

> We did an initial attempt to migrate those bits last month as part of crrev.com/c/1868870, but got a CR-1 because it seems that Android's libchrome repository is not yet ready to handle changes in the .mojom files from Chromium to the new types. The issue seems to be that those .mojom files are copied into the libchrome repo, which does not contain support for the new mojo types yet, so that process would not work anymore like that, see [2] for more details.

Two git repos are related, libchrome and ARC, both are a part of Android.
.mojom files are copied from Chrome's component/arc to ARC, not to libchrome. It is built with libchrome.
Current libchrome in Android is not new enough, so new syntax is not available yet.

As for libchrome, there are two libchrome, one in Android (the one explained just above), and the other in Chrome OS.
libchrome is the library extracted from Chrome repository. Now Android's one is upstream, and the it is downstreamed into Chrome OS's one.
I.e., files are copied: Chrome -> Android's libchrome -> Chrome OS libchrome.
There's a plan to swap the libchrome's upstream. Once it happens, the flow will be Chrome -> Chrome OS libchrome -> Android's libchrome. (probably, this is what Ken said).
Next libchrome uprev will be blocked by the upstream swap. The upstream swap is now blocked by Chrome OS libchrome uprev, which is actively on-going.
Once swap happens, Chrome OS libchrome will be uprev'ed more frequently. TBD for Android libchrome. We need a plan, based on Android is released once a year, IMHO.
I think libchrome uprev including new syntax will happen eventually, but it will take time.

> Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy?

Copying mojom files are totally manual operation. In most cases, just "cp" command. No script, AFAIK.

> Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?

It introduces technical complexity to mojom update procedure for ARC.
Also, it makes it difficult to take diff mechanically, thus it makes it difficult to find accidentally stale mojom files. In past, I took just diff, which wouldn't work.
I'm not very sure if ARC would like to accept such a change. CC'ed several ARC core engs.

To me, the straightforward approach is technically, update libchrome in Android, and then use new syntax in ARC's mojom files.

Thanks,
- hidehiko

Dave Tapuska

unread,
Dec 19, 2019, 11:10:07 AM12/19/19
to Hidehiko Abe, Mario Sanchez Prada, Ken Rockot, chromium-mojo, platform-architecture-dev, Jeremy Roman, Yusuke Sato, Josh Horwich, Elijah Taylor
Can we have an estimate when these libchrome changes will be done?  Issue 915258 has been opened for over a year. Also issue 909719 has seen some active progress but it isn't clear how much more work is remaining.  If these libraries take another year to update I don't believe that is an acceptable solution on our end.

dave.


--
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.

Daniel Cheng

unread,
Dec 19, 2019, 5:45:30 PM12/19/19
to Hidehiko Abe, Mario Sanchez Prada, Ken Rockot, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, Yusuke Sato, Josh Horwich, Elijah Taylor
On Fri, Dec 20, 2019 at 12:51 AM 'Hidehiko Abe' via platform-architecture-dev <platform-arc...@chromium.org> wrote:
Some questions seem based on some misunderstanding of the current situation, so let me give it a try to explain.

> We did an initial attempt to migrate those bits last month as part of crrev.com/c/1868870, but got a CR-1 because it seems that Android's libchrome repository is not yet ready to handle changes in the .mojom files from Chromium to the new types. The issue seems to be that those .mojom files are copied into the libchrome repo, which does not contain support for the new mojo types yet, so that process would not work anymore like that, see [2] for more details.

Two git repos are related, libchrome and ARC, both are a part of Android.
.mojom files are copied from Chrome's component/arc to ARC, not to libchrome. It is built with libchrome.
Current libchrome in Android is not new enough, so new syntax is not available yet.

As for libchrome, there are two libchrome, one in Android (the one explained just above), and the other in Chrome OS.
libchrome is the library extracted from Chrome repository. Now Android's one is upstream, and the it is downstreamed into Chrome OS's one.
I.e., files are copied: Chrome -> Android's libchrome -> Chrome OS libchrome.
There's a plan to swap the libchrome's upstream. Once it happens, the flow will be Chrome -> Chrome OS libchrome -> Android's libchrome. (probably, this is what Ken said).
Next libchrome uprev will be blocked by the upstream swap. The upstream swap is now blocked by Chrome OS libchrome uprev, which is actively on-going.
Once swap happens, Chrome OS libchrome will be uprev'ed more frequently. TBD for Android libchrome. We need a plan, based on Android is released once a year, IMHO.
I think libchrome uprev including new syntax will happen eventually, but it will take time.

> Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy?

Copying mojom files are totally manual operation. In most cases, just "cp" command. No script, AFAIK.

> Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?

It introduces technical complexity to mojom update procedure for ARC.
Also, it makes it difficult to take diff mechanically, thus it makes it difficult to find accidentally stale mojom files. In past, I took just diff, which wouldn't work.
I'm not very sure if ARC would like to accept such a change. CC'ed several ARC core engs.


I think it wouldn't be that bad to introduce a rewriter, as long as it were automated. The workflow would basically be:
- copy mojom files from //component/arc to staging directory
- run the script across the staging directory to transform it to the old-style syntax
- cp staging directory into ARC and see if there are diffs with git diff

Daniel
 
To me, the straightforward approach is technically, update libchrome in Android, and then use new syntax in ARC's mojom files.

Thanks,
- hidehiko

On Mon, Dec 9, 2019 at 9:33 PM Mario Sanchez Prada <ma...@igalia.com> wrote:
Hi again,

On 4/12/19 10:49, Mario Sanchez Prada wrote:
> [...]
>> I do agree that we want to get rid of these things though, so I would suggest we switch to rolling cross-repo mojoms in the opposite direction, where a correct sed transform is trivial.
>
> I agree it's definitely easier to roll cross-repo mojoms in the opposite direction (i.e. source of truth in Chromium repo) for the reasons you mentioned, but I'm not sure what the actual next steps to make that happen would be... I assume some devs who work on the other repo should obviously get involved, but not sure who to ping on that. Any idea?

Pinging here after a few days.

>> It's also possible that none of the cross-repo mojoms change frequently enough for this to require special action at this time.
>
> Just to double-check I'm getting this right... what you mean here is that, we could actually go ahead and change the local /copy/ of the .mojom files in Chromium *as long as* the original sources in the other repository don't change frequently, right?
>
> We'd still need some agreement from the people working on that other repo on (1) that changes are not frequent and (2) that they would be happy to start off a point where .mojom file would not use the same -old- types in both repos and take it from there, but that would certainly unblock the conversions in Chromium repo.
>
> WDYT?

We'd still need to clarify this part as well, in order to determine how to move forward.

Thanks again,
Mario

--
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.

Andrew Moylan

unread,
Dec 19, 2019, 6:13:46 PM12/19/19
to Ken Rockot, Dave Tapuska, Mario Sanchez Prada, chromium-mojo, platform-architecture-dev, Jeremy Roman, Hidehiko Abe
On Wed, 4 Dec 2019 at 01:47, 'Ken Rockot' via chromium-mojo <chromi...@chromium.org> wrote:


On Tue, Dec 3, 2019 at 6:38 AM Dave Tapuska <dtap...@chromium.org> wrote:
Since they are binary compatible then it is reasonable to make this part of the process of copying them so that we can remove the legacy mojo interfaces entirely. 

Do we know if this is an automated process of copying the mojom files or a manual one? Is there a script that does the copy? Are we able to write a script that turns them (ie. sed) into the appropriate binary compatible version while copying it over?
dave.

I think it's been done manually, but AFAIK typically the roll happens in the other direction -- source of truth in cros repo, rolled into chromium.

Automating translation in that direction is challenging because the old notation for interface remotes is grammatically ambiguous.

I do agree that we want to get rid of these things though, so I would suggest we switch to rolling cross-repo mojoms in the opposite direction, where a correct sed transform is trivial.

It's also possible that none of the cross-repo mojoms change frequently enough for this to require special action at this time.

Sorry to jump in, but to color this story a bit with a non-ARC example of this:

For ML Service, we usually are rolling mojoms from CrOS -> Chrome direction, because the CrOS side is where the API is usually "expanding".
We do it with a standard cp + sed as shown in the description of this commit:

Now that the Chromium side has been migrated to the new types, next time I roll I plan to simply add another sed command(s) to convert old to new types. Probably an explicit separate sed for each and every ThingPtr -> pending_remote<Thing>.

As Ken suggested, this is infrequent enough not to be a worry.
The gerrit code review interface will reveal if we missed any ThingPtrs in our sed(s).

And if the bash command gets too ugly to paste into Gerrit description with a straight face, a roll_mojoms.sh can be constructed.



 

Hidehiko Abe

unread,
Dec 20, 2019, 4:28:03 AM12/20/19
to Andrew Moylan, Ken Rockot, Dave Tapuska, Mario Sanchez Prada, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii
> Can we have an estimate when these libchrome changes will be done?  Issue 915258 has been opened for over a year. Also issue 909719 has seen some active progress but it isn't clear how much more work is remaining.  If these libraries take another year to update I don't believe that is an acceptable solution on our end.

Current libchrome uprev focus is Chrome OS, and TBD for Android's libchrome (At least there's no plan in Q1). CC'ed libchrome team.

I had quick chat with yusukes@ and he told me that, if mid-term solution is needed,
probably the alternative approach is to run a script on build time, instead of uprev time, since it is fully automated. Then, the mirroring between Chrome and ARC does not need to be worried.
Though, I don't think sed is a way to go, because it looks too weak. It wouldn't work even just with an additional line break, and there's no guarantee that it works for any mojom files in the future.

If it is necessary, could you develop and maintain the script which ensures that "downgrading mojom to mojom" works properly for any valid mojom files? Then, it can be run in the ARC build, I think.

Thanks,
- hidehiko


Ken Rockot

unread,
Dec 20, 2019, 10:17:50 AM12/20/19
to Hidehiko Abe, Andrew Moylan, Dave Tapuska, Mario Sanchez Prada, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii


On Fri, Dec 20, 2019, 1:28 AM Hidehiko Abe <hide...@google.com> wrote:
> Can we have an estimate when these libchrome changes will be done?  Issue 915258 has been opened for over a year. Also issue 909719 has seen some active progress but it isn't clear how much more work is remaining.  If these libraries take another year to update I don't believe that is an acceptable solution on our end.

Current libchrome uprev focus is Chrome OS, and TBD for Android's libchrome (At least there's no plan in Q1). CC'ed libchrome team.

I had quick chat with yusukes@ and he told me that, if mid-term solution is needed,
probably the alternative approach is to run a script on build time, instead of uprev time, since it is fully automated. Then, the mirroring between Chrome and ARC does not need to be worried.
Though, I don't think sed is a way to go, because it looks too weak. It wouldn't work even just with an additional line break, and there's no guarantee that it works for any mojom files in the future.

If it is necessary, could you develop and maintain the script which ensures that "downgrading mojom to mojom" works properly for any valid mojom files? Then, it can be run in the ARC build, I think.

Sounds reasonable to me. The script would be trivial to write and maintain.

Mario Sanchez Prada

unread,
Dec 23, 2019, 7:29:28 AM12/23/19
to Ken Rockot, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii

Hi,

On 20/12/19 16:17, 'Ken Rockot' via chromium-mojo wrote:

On Fri, Dec 20, 2019, 1:28 AM Hidehiko Abe <hide...@google.com> wrote:
> Can we have an estimate when these libchrome changes will be done?  Issue 915258 has been opened for over a year. Also issue 909719 has seen some active progress but it isn't clear how much more work is remaining.  If these libraries take another year to update I don't believe that is an acceptable solution on our end.

Current libchrome uprev focus is Chrome OS, and TBD for Android's libchrome (At least there's no plan in Q1). CC'ed libchrome team.

I had quick chat with yusukes@ and he told me that, if mid-term solution is needed,
probably the alternative approach is to run a script on build time, instead of uprev time, since it is fully automated. Then, the mirroring between Chrome and ARC does not need to be worried.
Though, I don't think sed is a way to go, because it looks too weak. It wouldn't work even just with an additional line break, and there's no guarantee that it works for any mojom files in the future.

If it is necessary, could you develop and maintain the script which ensures that "downgrading mojom to mojom" works properly for any valid mojom files? Then, it can be run in the ARC build, I think.

Sounds reasonable to me. The script would be trivial to write and maintain.

Looks like we have a plan then, thank you all for your feedback and ideas!

I'm going OoO today until January 7th (back on that day), so I'll take a look into writing that script then if nobody beats me to it. I'll probably start with a bash script and see how it goes, but if it gets too ugly I'll likely switch to python, which should be ok too from a build-time perspective I think.

Anyway, time to go now... happy holidays and happy new year in advance!

Thanks,
Mario

PS: I'm not sure I'll be able to send the status report on the 1st of January due to me being OoO, so I'll just share here that //content has been fully migrated now too, joining Blink in the 100% achievement! See https://docs.google.com/spreadsheets/d/1khYVrfnN74RSDqqXgCFlG9fI89IKWA9J32Ln_2ZcTaU/edit#gid=346422195

Mario Sanchez Prada

unread,
Jan 7, 2020, 7:42:26 AM1/7/20
to Ken Rockot, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii

Hi all,

On 23/12/19 13:29, Mario Sanchez Prada wrote:
On 20/12/19 16:17, 'Ken Rockot' via chromium-mojo wrote:
On Fri, Dec 20, 2019, 1:28 AM Hidehiko Abe <hide...@google.com> wrote:
[...]

I had quick chat with yusukes@ and he told me that, if mid-term solution is needed,
probably the alternative approach is to run a script on build time, instead of uprev time, since it is fully automated. Then, the mirroring between Chrome and ARC does not need to be worried.
Though, I don't think sed is a way to go, because it looks too weak. It wouldn't work even just with an additional line break, and there's no guarantee that it works for any mojom files in the future.

If it is necessary, could you develop and maintain the script which ensures that "downgrading mojom to mojom" works properly for any valid mojom files? Then, it can be run in the ARC build, I think.

Sounds reasonable to me. The script would be trivial to write and maintain.

Looks like we have a plan then, thank you all for your feedback and ideas!

I'm going OoO today until January 7th (back on that day), so I'll take a look into writing that script then if nobody beats me to it. I'll probably start with a bash script and see how it goes, but if it gets too ugly I'll likely switch to python, which should be ok too from a build-time perspective I think

I've taken a look to having such a script this morning and came up with a pretty simple one that takes the target directory as its only parameter and converts all the .mojom files inside of it from the new to the old types, using sed.

It's attached as mojo_types_converted.sh in this mail, but the gist of it is this:

find "${SRCDIR}" -type f -name "*.mojom" | while read file; do
sed --in-place --regexp-extended \
-e 's/pending_remote<(.*)>/\1/g' \
-e 's/pending_receiver<(.*)>/\1\&/g' \
-e 's/pending_associated_remote<(.*)>/associated \1/g' \
-e 's/pending_associated_receiver<(.*)>/associated \1\&/g' \
"${file}"
done

@hidehiko I considered your comment on avoiding sed here, but I honestly think it's good enough since the bits we'd be translating are never split across line breaks AFAICS, and using sed seemed simple yet powerful enough to me in this particular case, where we're only translating the 4 cases mentioned above. WDYT?

Also, for testing purposes, I ran it over the src/third_party/blink/public/mojom directory and then attached the output of running `git diff` afterwards in mojo_types_converter.diff. See below an excerpt of it:

--- a/third_party/blink/public/mojom/devtools/devtools_agent.mojom
+++ b/third_party/blink/public/mojom/devtools/devtools_agent.mojom
@@ -72,9 +72,9 @@ interface DevToolsAgent {
// |client_expects_binary_response| indicates that responses (and
// notifications) sent from this session should use binary (CBOR)
// encoding as opposed to JSON encoding.
- AttachDevToolsSession(pending_associated_remote<DevToolsSessionHost> host,
- pending_associated_receiver<DevToolsSession> session,
- pending_receiver<DevToolsSession> io_session,
+ AttachDevToolsSession(associated DevToolsSessionHost host,
+ associated DevToolsSession& session,
+ DevToolsSession& io_session,
DevToolsSessionState? reattach_session_state,
bool client_expects_binary_responses);
@@ -102,8 +102,8 @@ interface DevToolsAgentHost {
// |waiting_for_debugger| is true if worker was paused on startup and
// should be resumed by debugger to actually start.
ChildWorkerCreated(
- pending_remote<DevToolsAgent> worker_devtools_agent,
- pending_receiver<DevToolsAgentHost> worker_devtools_agent_host,
+ DevToolsAgent worker_devtools_agent,
+ DevToolsAgentHost& worker_devtools_agent_host,
url.mojom.Url url,
string name,
mojo_base.mojom.UnguessableToken devtools_worker_token,


If this is looking like something that could get accepted, next step would be to get the script (or simply the sed line) integrated with libchrome's build system, both for Android and ChromeOS, so that the conversion is fully automated.

@hidehiko Since I've never contributed directly to libchrome maybe someone else from that team could pick this script and integrate it there? Alternatively, I could try to do that myself, in which case I'd appreciate some guidance.

Thanks,
Mario
mojo_types_converter.diff
mojo_types_converter.sh

Mario Sanchez Prada

unread,
Jan 8, 2020, 4:42:43 AM1/8/20
to Ken Rockot, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii
On 7/1/20 13:42, Mario Sanchez Prada wrote:

[...]

It's attached as mojo_types_converted.sh in this mail, but the gist of it is this:

find "${SRCDIR}" -type f -name "*.mojom" | while read file; do
sed --in-place --regexp-extended \
-e 's/pending_remote<(.*)>/\1/g' \
-e 's/pending_receiver<(.*)>/\1\&/g' \
-e 's/pending_associated_remote<(.*)>/associated \1/g' \
-e 's/pending_associated_receiver<(.*)>/associated \1\&/g' \
"${file}"
done

Actually, this is not correct, as sed's greedy nature means that it could end up doing the wrong conversion sometimes, as it was the case for native_file_system_file_writer.mojom:

- Write(uint64 offset, pending_remote<Blob> data) => (NativeFileSystemError result,
+ Write(uint64 offset, Blob> data) = (NativeFileSystemError result,
uint64 bytes_written);

Thus, we should avoid that, which we can do with a minor change to the regular expression:

find "${SRCDIR}" -type f -name "*.mojom" | while read file; do
sed --in-place --regexp-extended \
-e 's/pending_remote<([^>]*)>/\1/g' \
-e 's/pending_receiver<([^>]*)>/\1\&/g' \
-e 's/pending_associated_remote<([^>]*)>/associated \1/g' \
-e 's/pending_associated_receiver<([^>]*)>/associated \1\&/g' \
"${file}"
done

See attached the updated script and diff after running this new script over Blink's mojom files.

Apologies for the extra noise. Everything else from my previous email still applies, though

Thanks
Mario


mojo_types_converter.sh
mojo_types_converter.diff

Hidehiko Abe

unread,
Jan 10, 2020, 8:09:56 AM1/10/20
to Mario Sanchez Prada, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato
Hi,

I think we have miscommunication.
Actually, running "sed" to conver mojom files looks fragile. E.g., even if just adding a linebreak after "<" due to the length of identifiers, the script wouldn't work,
but it is still valid in mojom_bindings_generator, IIUC.
What I asked was; Could you give it a try to parse the mojom file by the actual parser, and generate a code which is compatible, so that it is guaranteed that the script works for any mojom files supported by mojom_bindings_generator?

Thanks,
- hidehiko


Ken Rockot

unread,
Jan 10, 2020, 11:35:21 AM1/10/20
to Hidehiko Abe, Mario Sanchez Prada, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato


On Fri, Jan 10, 2020, 5:09 AM Hidehiko Abe <hide...@google.com> wrote:
Hi,

I think we have miscommunication.
Actually, running "sed" to conver mojom files looks fragile. E.g., even if just adding a linebreak after "<" due to the length of

I agree that in theory it seems fragile, but in practice the use of such a script is a temporary concern and we have hundreds of mojoms over years of development without a line break in these type names. So very unlikely to be a real issue.

identifiers, the script wouldn't work,
but it is still valid in mojom_bindings_generator, IIUC.
What I asked was; Could you give it a try to parse the mojom file by the actual parser, and generate a code which is compatible, so that it is guaranteed that the script works for any mojom files supported by mojom_bindings_generator?

Doing this would require changes to the parser, which is part of the repo we're trying to uprev.  We would have to develop this change within the libchrome repo and update the dependency in each repo that uses it, wouldn't we?

I worry that this would be significantly more complex than having a simple copyable script and separate build step that can be stuffed into individual builds as needed.

Mario Sanchez Prada

unread,
Jan 14, 2020, 6:24:37 AM1/14/20
to Ken Rockot, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato

Hi,

On 10/1/20 17:35, 'Ken Rockot' via chromium-mojo wrote:
[...]


On Fri, Jan 10, 2020, 5:09 AM Hidehiko Abe <hide...@google.com> wrote:
Hi,

I think we have miscommunication.
Actually, running "sed" to conver mojom files looks fragile. E.g., even if just adding a linebreak after "<" due to the length of

I agree that in theory it seems fragile, but in practice the use of such a script is a temporary concern and we have hundreds of mojoms over years of development without a line break in these type names. So very unlikely to be a real issue.

This was my thinking exactly: I realize that a sed line might not be perfect and future-proof if we eventually start getting affected by line breaks, but considering that this is a temporary measure to work around this situation while those libchrome repos don't support the new types, I thought it could be a good trade-off.

identifiers, the script wouldn't work,
but it is still valid in mojom_bindings_generator, IIUC.
What I asked was; Could you give it a try to parse the mojom file by the actual parser, and generate a code which is compatible, so that it is guaranteed that the script works for any mojom files supported by mojom_bindings_generator?

Doing this would require changes to the parser, which is part of the repo we're trying to uprev.  We would have to develop this change within the libchrome repo and update the dependency in each repo that uses it, wouldn't we?

I worry that this would be significantly more complex than having a simple copyable script and separate build step that can be stuffed into individual builds as needed.

FWIW, I also share these concerns. Personally, I was hoping we could move on with the simpler approach proposed, since this is the last roadblock preventing us from removing the old types from the chromium repository, but it's not clear to me at the moment whether that would be accepted. So, I think it would be good to try to clarify what the situation is, first of all.

hidehiko@ +others: WDYT? Would you be open in the end to this simpler approach of using a sed script? Or would you rather push for a more complex alternative?

Thanks,
Mario

Hidehiko Abe

unread,
Jan 21, 2020, 11:40:18 AM1/21/20
to Mario Sanchez Prada, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato
# Sorry for delayed response.



What I'd like to see is the guarantee that the script produces the mojom files which are collectly parsable by older mojom bindings generator, in order not to break the build.
Another idea I can think of is to write tests inside Chrome tree verifying all mojom files exported to Android repository can be successfully down-coverted (successfully parsed by an old parser) by the sed script.
Because Chrome repository is the upstream of those mojom files, it can help to capture the possible errors in an earlier stage.
Though, this is still more fragile than the down conversion by the actual parser. I'd like to hear the opinion by yusukes@, too.

Thanks,
- hidehiko

Thanks,
Mario

Hidehiko Abe

unread,
Jan 23, 2020, 7:24:19 AM1/23/20
to Mario Sanchez Prada, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara
Had a chat with yusukes@ offline, and it looks like to be ok for him.
To be clear, my alternate proposal is;
1) Check in the downconver script to chromium repository.
2) Add unittests which apply the downcoverter script to each mojom files exported to ARC, then parse the results by the older parser to make sure it wouldn't generate wrong code.
In this plan, it is still necessary to keep older parser in mojom tool in chromium repository. Though, it is not necessary to be usable within the chromium tree, except the unittests.

Mario, Ken, WDYT?
(CC+=haraken@, thank you for helping)

Thanks,
- hidehiko

Ken Rockot

unread,
Jan 23, 2020, 12:21:04 PM1/23/20
to Hidehiko Abe, Mario Sanchez Prada, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara
I don't think we need to clone the forked old parser into the Chromium tree, but it would be reasonable for someone to add a switch to the bindings generator which forces its parser to reject pending_* syntax. One could pretty easily build tests upon that.

Mario Sanchez Prada

unread,
Jan 27, 2020, 4:27:34 AM1/27/20
to Ken Rockot, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara

Hi all,

First of all apologies for the late reply, see my comments below:

On 23/1/20 18:20, 'Ken Rockot' via chromium-mojo wrote:
On Thu, Jan 23, 2020 at 4:24 AM Hidehiko Abe <hide...@google.com> wrote:
[...]

Had a chat with yusukes@ offline, and it looks like to be ok for him.
To be clear, my alternate proposal is;
1) Check in the downconver script to chromium repository.
2) Add unittests which apply the downcoverter script to each mojom files exported to ARC, then parse the results by the older parser to make sure it wouldn't generate wrong code.
In this plan, it is still necessary to keep older parser in mojom tool in chromium repository. Though, it is not necessary to be usable within the chromium tree, except the unittests.

Mario, Ken, WDYT?
(CC+=haraken@, thank you for helping)

I miss the step in which the downconverter script will be integrated with the libchrome repos, but I assume that will be done independently in there once this script gets checked in for chromium. Is that correct?

I don't think we need to clone the forked old parser into the Chromium tree, but it would be reasonable for someone to add a switch to the bindings generator which forces its parser to reject pending_* syntax. One could pretty easily build tests upon that.

IIUC, what you're suggesting is to add a new command line argument to mojom_bindings_generator.py (e.g. --force-old-mojo-syntax?) that would only parse and generate bindings for .mojom files using the old syntax, and then add a new test case to mojom_bindings_generator_unittest.py to validate that the "downconverted" .mojom files can be successfully parsed and generated code for, right?

One thing I'm not 100% sure about, though, is what you mean by "forked old parser"... isn't the only parser (currently supporting new and old syntax) the one in mojo/public/tools/bindings/pylib/mojom/parse/parser.py, referenced from mojom_bindings_generator.py?

Thanks again,
Mario

Hidehiko Abe

unread,
Jan 29, 2020, 11:44:25 AM1/29/20
to Mario Sanchez Prada, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara
Hi,

On Mon, Jan 27, 2020 at 6:27 PM Mario Sanchez Prada <ma...@igalia.com> wrote:

Hi all,

First of all apologies for the late reply, see my comments below:

On 23/1/20 18:20, 'Ken Rockot' via chromium-mojo wrote:
On Thu, Jan 23, 2020 at 4:24 AM Hidehiko Abe <hide...@google.com> wrote:
[...]
Had a chat with yusukes@ offline, and it looks like to be ok for him.
To be clear, my alternate proposal is;
1) Check in the downconver script to chromium repository.
2) Add unittests which apply the downcoverter script to each mojom files exported to ARC, then parse the results by the older parser to make sure it wouldn't generate wrong code.
In this plan, it is still necessary to keep older parser in mojom tool in chromium repository. Though, it is not necessary to be usable within the chromium tree, except the unittests.

Mario, Ken, WDYT?
(CC+=haraken@, thank you for helping)

I miss the step in which the downconverter script will be integrated with the libchrome repos, but I assume that will be done independently in there once this script gets checked in for chromium. Is that correct?

It is not automatic procedure. The script is necessary to be added to libchrome in several ARC branches in Android repository.

Thanks,
- hidehiko

Mario Sanchez Prada

unread,
Feb 5, 2020, 4:26:42 AM2/5/20
to Hidehiko Abe, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara
Hi all,

Sorry again for the slowness replying here again, got my head wrapped around a different task and didn't realize of this reply...

On 29/1/20 17:44, 'Hidehiko Abe' via chromium-mojo wrote:
> [...]
> I miss the step in which the downconverter script will be integrated with the libchrome repos, but I assume that will be done independently in there once this script gets checked in for chromium. Is that correct?
>
> It is not automatic procedure. The script is necessary to be added to libchrome in several ARC branches in Android repository.

Ok, but just to handle expectations: can we move ahead proposing the necessary changes in chromium (i.e. add the script + changes to mojom_binding_generator.py and mojom_binding_generator_unittest.py files) assuming that someone else working on libchrome will add the script to those several branches?

>> I don't think we need to clone the forked old parser into the Chromium tree, but it would be reasonable for someone to add a switch to the bindings generator which forces its parser to reject pending_* syntax. One could pretty easily build tests upon that.
>
> IIUC, what you're suggesting is to add a new command line argument to mojom_bindings_generator.py (e.g. --force-old-mojo-syntax?) that would only parse and generate bindings for .mojom files using the old syntax, and then add a new test case to mojom_bindings_generator_unittest.py to validate that the "downconverted" .mojom files can be successfully parsed and generated code for, right?

This hasn't been confirmed or otherwise yet, please speak up if it's not correct. I was planning to start working on this soon (this week or next time, once I clear some roadblocks on a different task I'm currently working on). Thanks!

> One thing I'm not 100% sure about, though, is what you mean by "forked old parser"... isn't the only parser (currently supporting new and old syntax) the one in mojo/public/tools/bindings/pylib/mojom/parse/parser.py, referenced from mojom_bindings_generator.py?

Still a valid question.

Thanks,
Mario

Hidehiko Abe

unread,
Feb 6, 2020, 6:27:47 AM2/6/20
to Mario Sanchez Prada, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara
On Wed, Feb 5, 2020 at 6:26 PM Mario Sanchez Prada <ma...@igalia.com> wrote:
Hi all,

Sorry again for the slowness replying here again, got my head wrapped around a different task and didn't realize of this reply...

On 29/1/20 17:44, 'Hidehiko Abe' via chromium-mojo wrote:
> [...]
>     I miss the step in which the downconverter script will be integrated with the libchrome repos, but I assume that will be done independently in there once this script gets checked in for chromium. Is that correct?
>
> It is not automatic procedure. The script is necessary to be added to libchrome in several ARC branches in Android repository.

Ok, but just to handle expectations: can we move ahead proposing the necessary changes in chromium (i.e. add the script + changes to mojom_binding_generator.py and mojom_binding_generator_unittest.py files) assuming that someone else working on libchrome will add the script to those several branches?


I was thinking you would maintain the build...?
At least, I don't think I myself have enough capacity to maintain this, unfortunately.
 
>>     I don't think we need to clone the forked old parser into the Chromium tree, but it would be reasonable for someone to add a switch to the bindings generator which forces its parser to reject pending_* syntax. One could pretty easily build tests upon that.
>
>     IIUC, what you're suggesting is to add a new command line argument to mojom_bindings_generator.py (e.g. --force-old-mojo-syntax?) that would only parse and generate bindings for .mojom files using the old syntax, and then add a new test case to mojom_bindings_generator_unittest.py to validate that the "downconverted" .mojom files can be successfully parsed and generated code for, right?

This hasn't been confirmed or otherwise yet, please speak up if it's not correct. I was planning to start working on this soon (this week or next time, once I clear some roadblocks on a different task I'm currently working on). Thanks!

>     One thing I'm not 100% sure about, though, is what you mean by "forked old parser"... isn't the only parser (currently supporting new and old syntax) the one in mojo/public/tools/bindings/pylib/mojom/parse/parser.py, referenced from mojom_bindings_generator.py?

Still a valid question.


Defer to Ken.

Mario Sanchez Prada

unread,
Feb 7, 2020, 11:22:36 AM2/7/20
to Hidehiko Abe, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara

Hi all,

On 6/2/20 12:27, 'Hidehiko Abe' via chromium-mojo wrote:
On Wed, Feb 5, 2020 at 6:26 PM Mario Sanchez Prada <ma...@igalia.com> wrote:
[...]

Ok, but just to handle expectations: can we move ahead proposing the necessary changes in chromium (i.e. add the script + changes to mojom_binding_generator.py and mojom_binding_generator_unittest.py files) assuming that someone else working on libchrome will add the script to those several branches?


I was thinking you would maintain the build...?
At least, I don't think I myself have enough capacity to maintain this, unfortunately.

As we (Kentaro, Hidehiko & me) discussed offline, I probably won't able to maintain the libchrome build myself, so someone else (from the libchrome or arc-core team maybe) would have to take care of that part.

From my side, though, I can as promised provide a version of the script that we can check in the chromium repository along with some kind of unittests, so I've spent some time today and yesterday working on that, and put the result of my work so far in https://chromium-review.googlesource.com/c/chromium/src/+/2041759

As you can see in there, I ended up writing the script in Python instead of bash since I think it's perhaps a better idea (more clear, more maintainable, better integration with unittests...), and also took that chance to write it in a way so that it works with odd spaces and linebreaks, which should address one of the concerns from @hidehiko.

I also added a new unittest to check all the cases I could think of (including multiline declarations, linebreaks and response callbacks), made it so that it would use a modified version of the current parser & lexer that won't accept the new Mojo types... and everything passes for me locally, so probably moving it to review later today.

@rockot: I've tried to implement the --force-old-mojo-syntax idea you suggested to avoid checking-in the a separate parser & lexer for this, but I concluded it was not as easy because the way parser.py and lexer.py are handled by the yacc.py module does not make it easy (i.e. I couldn't find a way) to reuse those files. So, I ended up removing the bits I didn't want to parse out of them and saving them as oldparser.py and oldlexer.py, which I believe is what Hidehiko suggested would have to happen in his previous mail[*], so hoping it's not bad).

I hope this helps untangle this situation. Once this script lands in chromium and we get an agreement on that someone will take care of using it for libchrome, we should be able to resume work on http://crrev.com/c/1868870 and migrate //components/arc to the new mojo types once and for all.

PTAL and let me know what you think.

Thanks!
Mario

[*] From Hidehiko's previous mail: "In this plan, it is still necessary to keep older parser in mojom tool in chromium repository. Though, it is not necessary to be usable within the chromium tree, except the unittests."

Ken Rockot

unread,
Feb 7, 2020, 1:58:41 PM2/7/20
to Mario Sanchez Prada, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara
Thanks for putting together that CL and exploring these options in such detail. It's really helpful.

Can you clarify what the --force-old-mojo-syntax switch was going to do? In my mind at least, we would only have needed to touch mojom_cpp_generator.py and superficially have the flag emit InterfacePtr and InterfaceRequest any time we would have normally emitted PendingRemote and PendingReceiver. In theory we should be able to cherry-pick a newer generator into libchrome since I don't *think* we've made any backwards-incompatible changes to it in the past few years.

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

Mario Sanchez Prada

unread,
Feb 10, 2020, 6:42:53 AM2/10/20
to Ken Rockot, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara

Hi Ken,

On 7/2/20 19:58, 'Ken Rockot' via chromium-mojo wrote:
Thanks for putting together that CL and exploring these options in such detail. It's really helpful.

Can you clarify what the --force-old-mojo-syntax switch was going to do?

As I mentioned earlier on this thread, I understood that the plan was to modify the parser (not the generator) so that it would only accept the old mojo types if --force-old-mojo-syntax was passed, producing errors such as "Unexpected '<':" otherwise if new types like PendingRemote<T> were found in the source .mojom file.

This would allow me to test that whatever my downconverter script produced was still parsable by a parser accepting the old types only, which would be something I'd use on the unittests. This would be inline, I believe, with what Hidehiko proposed earlier on this thread (i.e. checking-in the script + unittests).

Unfortunately, looking at how the yacc parser and the lexer are written, passing a parameter to modify their behaviour didn't seem obvious at all, and ended up checking in old versions of both and not implementing that parameter at all (which again seemed to match what Hidehiko anticipated too).

In my mind at least, we would only have needed to touch mojom_cpp_generator.py and superficially have the flag emit InterfacePtr and InterfaceRequest any time we would have normally emitted PendingRemote and PendingReceiver. In theory we should be able to cherry-pick a newer generator into libchrome since I don't *think* we've made any backwards-incompatible changes to it in the past few years.

Hmm.. reading this I believe there has been a misunderstanding, because you're talking about modifying the generator, not the parser, to use that new optional parameter. If I'm getting it right now, you might be proposing something like what I've just uploaded in the following CL:

  https://chromium-review.googlesource.com/c/chromium/src/+/2043885

...where I just modified the C++ generator to make sure it produces code using the old mojo types, regardless of whether the new types were specified in the .mojom file.

I'll move that one to review process as well so that we can continue the discussion in there and free this ML, if that's ok. And once we make a decision on which approach we want to take (dowconverted script vs changes to the C++ generator) I'll abandon the other CL.

WDYT?

Thanks,
Mario

Ken Rockot

unread,
Feb 10, 2020, 8:23:26 AM2/10/20
to Mario Sanchez Prada, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara
Sorry, my fault for not being more clear in the last message. Your understanding of things was 100% accurate.

The relative complexity of the changes needed to support down-conversion -- compared to the original proposal of a one-line sed filter -- started to make me wonder if generator changes could be the simpler approach after all.

WDYT?

Thanks,
Mario

Mario Sanchez Prada

unread,
Feb 10, 2020, 10:54:12 AM2/10/20
to Ken Rockot, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara

Hi Ken,

On 10/2/20 14:23, 'Ken Rockot' via chromium-mojo wrote:
[...]

Hmm.. reading this I believe there has been a misunderstanding, because you're talking about modifying the generator, not the parser, to use that new optional parameter. If I'm getting it right now, you might be proposing something like what I've just uploaded in the following CL:

  https://chromium-review.googlesource.com/c/chromium/src/+/2043885

...where I just modified the C++ generator to make sure it produces code using the old mojo types, regardless of whether the new types were specified in the .mojom file.

I'll move that one to review process as well so that we can continue the discussion in there and free this ML, if that's ok. And once we make a decision on which approach we want to take (dowconverted script vs changes to the C++ generator) I'll abandon the other CL.

Sorry, my fault for not being more clear in the last message. Your understanding of things was 100% accurate.

No problem, thanks for clarifying.

The relative complexity of the changes needed to support down-conversion -- compared to the original proposal of a one-line sed filter -- started to make me wonder if generator changes could be the simpler approach after all.

Well, the second CL I've just uploaded seems simple enough to be honest, and perhaps easier to integrate in external repos like libchrome (i.e. just pass --force_old_mojo_types), but I'm finding it a bit harder to add unit tests for it than in the other CL that included the downgraded script.

Either way, I'm happy to move forward with whichever option that looks better to you (Ken) and Hidehiko (others' comments welcome too, of course).

PTAL and let me know what you prefer.

Thanks,
Mario

Ken Rockot

unread,
Feb 10, 2020, 1:18:46 PM2/10/20
to Mario Sanchez Prada, Hidehiko Abe, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara
I agree that the second CL seems simpler.  I think we could get by with a simple compile test, i.e. add GN support to optionally set the force_old_mojo_types flag, define a mojom interface method that takes a pending_receiver<> and pending_remote<> type, and statically assert in some existing unit test file (maybe new_endpoint_types_unittest.cc) that the signature of the generated interface uses mojo::InterfaceRequest<> and mojo::InterfacePtr<>.

I would like to hear back from Hidehiko though regarding his thoughts on the feasibility of partially uprevving only the bindings generator.

I also wonder if that option would be made any easier by having the bindings generator mirrored in its own repository, which is a proposal I floated last week for various reasons. If it would help this along I think we could probably make that happen this week.

Thanks,
Mario

Hidehiko Abe

unread,
Feb 13, 2020, 2:50:07 AM2/13/20
to Ken Rockot, Mario Sanchez Prada, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi
On Mon, Feb 10, 2020 at 10:18 AM Ken Rockot <roc...@google.com> wrote:


On Mon, Feb 10, 2020 at 7:54 AM Mario Sanchez Prada <ma...@igalia.com> wrote:

Hi Ken,

On 10/2/20 14:23, 'Ken Rockot' via chromium-mojo wrote:
[...]
Hmm.. reading this I believe there has been a misunderstanding, because you're talking about modifying the generator, not the parser, to use that new optional parameter. If I'm getting it right now, you might be proposing something like what I've just uploaded in the following CL:

  https://chromium-review.googlesource.com/c/chromium/src/+/2043885

...where I just modified the C++ generator to make sure it produces code using the old mojo types, regardless of whether the new types were specified in the .mojom file.

I'll move that one to review process as well so that we can continue the discussion in there and free this ML, if that's ok. And once we make a decision on which approach we want to take (dowconverted script vs changes to the C++ generator) I'll abandon the other CL.

Sorry, my fault for not being more clear in the last message. Your understanding of things was 100% accurate.

No problem, thanks for clarifying.

The relative complexity of the changes needed to support down-conversion -- compared to the original proposal of a one-line sed filter -- started to make me wonder if generator changes could be the simpler approach after all.

Well, the second CL I've just uploaded seems simple enough to be honest, and perhaps easier to integrate in external repos like libchrome (i.e. just pass --force_old_mojo_types), but I'm finding it a bit harder to add unit tests for it than in the other CL that included the downgraded script.

Either way, I'm happy to move forward with whichever option that looks better to you (Ken) and Hidehiko (others' comments welcome too, of course).

PTAL and let me know what you prefer.

I agree that the second CL seems simpler.  I think we could get by with a simple compile test, i.e. add GN support to optionally set the force_old_mojo_types flag, define a mojom interface method that takes a pending_receiver<> and pending_remote<> type, and statically assert in some existing unit test file (maybe new_endpoint_types_unittest.cc) that the signature of the generated interface uses mojo::InterfaceRequest<> and mojo::InterfacePtr<>.

I would like to hear back from Hidehiko though regarding his thoughts on the feasibility of partially uprevving only the bindings generator.


(CC+=satorux@, my manager for visibility).

From libchrome perspective, downcovert script looks simpler and preferred from ARC maintenance point of view.
Technically, indeed it probably could be possible to take newer mojom_bindings_generator exposed as a standard repository to Android and Chrome OS. However, there is non-trivial amount of work needed.

With some discussion, libchrome team (fqj@, hscham@, hidehiko@) can take a look Android build side.
I will send PSA to arc-eng@, who're actually affected by this change.

Thanks,
- hidehiko

Hidehiko Abe

unread,
Feb 13, 2020, 3:28:09 AM2/13/20
to Ken Rockot, Mario Sanchez Prada, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi
BTW, are there any documents describing the diff from old syntax to new syntax? Also, do you have summary of the changes?

Mario Sanchez Prada

unread,
Feb 13, 2020, 3:53:45 AM2/13/20
to Hidehiko Abe, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi
On 13/2/20 8:49, Hidehiko Abe wrote:
On Mon, Feb 10, 2020 at 10:18 AM Ken Rockot <roc...@google.com> wrote:
On Mon, Feb 10, 2020 at 7:54 AM Mario Sanchez Prada <ma...@igalia.com> wrote:

[...]


PTAL and let me know what you prefer.

I agree that the second CL seems simpler.  I think we could get by with a simple compile test, i.e. add GN support to optionally set the force_old_mojo_types flag, define a mojom interface method that takes a pending_receiver<> and pending_remote<> type, and statically assert in some existing unit test file (maybe new_endpoint_types_unittest.cc) that the signature of the generated interface uses mojo::InterfaceRequest<> and mojo::InterfacePtr<>.

I would like to hear back from Hidehiko though regarding his thoughts on the feasibility of partially uprevving only the bindings generator.


(CC+=satorux@, my manager for visibility).

From libchrome perspective, downcovert script looks simpler and preferred from ARC maintenance point of view.
Technically, indeed it probably could be possible to take newer mojom_bindings_generator exposed as a standard repository to Android and Chrome OS. However, there is non-trivial amount of work needed.

With some discussion, libchrome team (fqj@, hscham@, hidehiko@) can take a look Android build side.
I will send PSA to arc-eng@, who're actually affected by this change.

Wonderful, thanks! I'll move the CL with the downconverter script to the review process then, and will abandon the other one for now. Once that lands and we get the green light from the libchrome team, we could resume the conversion of the types in //components/arc, if that's ok.

Thanks again for your help & feedback with this,
Mario

Mario Sanchez Prada

unread,
Feb 13, 2020, 4:06:35 AM2/13/20
to Hidehiko Abe, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi
On 13/2/20 9:27, Hidehiko Abe wrote:
BTW, are there any documents describing the diff from old syntax to new syntax?

Yes, the "Mojo Bindings Conversion Cheatsheet" describes the changes that have been done in the context of this migration, both to C++ code and .mojom files:

  https://docs.google.com/document/d/1Jwfbzbe8ozaoilhqj5mAPYbYGpgZCen_XAAAdwmyP1E/edit#

Note that for the purpose of what's been discussed in this thread, we are only concerned with the changes in the .mojom files, that is, the following kind changes:

             Foo& request <=> pending_receiver<Foo> request
               Foo client <=> pending_remote<Foo> client
  associated Foo& request <=> pending_associated_receiver<Foo> request
    associated Foo client <=> pending_associated_remote<Foo> client

...as those are the ones that would be imported by libchrome through ARC, and which need downgrading.

Also, do you have summary of the changes?

You can find all the CLs done in the context of the conversions done inside and outside of Blink so far in https://crbug.com/955171 and https://crbug.com/978694, respectively.

If you're asking about the changes that would be done in the context of //components/arc, there's a WIP CL in crrev.com/c/1868870 that I think summarizes well the changes that would follow in those ARC-related .mojom files once we get the green light.

Hope this helps!

Thanks,
Mario

Hidehiko Abe

unread,
Feb 13, 2020, 11:43:36 AM2/13/20
to Mario Sanchez Prada, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi
Thanks.
I have sent a PSA to ARC team.

- hidehiko

Mario Sanchez Prada

unread,
Feb 28, 2020, 4:09:41 AM2/28/20
to Hidehiko Abe, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi

Hi all,

On 13/2/20 9:53, Mario Sanchez Prada wrote:
On 13/2/20 8:49, Hidehiko Abe wrote:
[...]

(CC+=satorux@, my manager for visibility).

From libchrome perspective, downcovert script looks simpler and preferred from ARC maintenance point of view.
Technically, indeed it probably could be possible to take newer mojom_bindings_generator exposed as a standard repository to Android and Chrome OS. However, there is non-trivial amount of work needed.

With some discussion, libchrome team (fqj@, hscham@, hidehiko@) can take a look Android build side.
I will send PSA to arc-eng@, who're actually affected by this change.

Wonderful, thanks! I'll move the CL with the downconverter script to the review process then, and will abandon the other one for now.

I'm happy to report that the CL with the downconverter script + the relevant unittest has finally landed upstream:
https://chromium.googlesource.com/chromium/src.git/+/58c0eb9cd7d4d62a5df6bfb7c42d4705f86bee2b


Once that lands and we get the green light from the libchrome team, we could resume the conversion of the types in //components/arc, if that's ok.

@hidehiko Now waiting for this "green light" from the libchrome team to know when it would be ok to resume work on https://crrev.com/c/1868870. Also, I think we can end this long thread here now and continue further discussion in the bug at  https://crbug.com/1035484.

Thanks again to everyone involved!
Mario

Hidehiko Abe

unread,
Mar 5, 2020, 11:05:13 PM3/5/20
to Mario Sanchez Prada, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi, Grace Cham
Sorry for taking time. I discussed it with the libchrome team.
As for eng resource allocation from the libchrome team, let me defer to chavey@, who's the manager of the team.

Thanks!
- hidehiko

Mario Sanchez Prada

unread,
Mar 6, 2020, 4:11:54 AM3/6/20
to Hidehiko Abe, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Laurent Chavey, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi, Grace Cham

Hi Hidehiko,

On 6/3/20 5:04, Hidehiko Abe wrote:
[...]
Sorry for taking time. I discussed it with the libchrome team.
As for eng resource allocation from the libchrome team, let me defer to chavey@, who's the manager of the team.

No problem at all, and thanks for helping with this. I'll wait for you to get back to me then before doing anything.

Thanks!
Mario

Laurent Chavey

unread,
Mar 16, 2020, 9:50:58 AM3/16/20
to Mario Sanchez Prada, Hidehiko Abe, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi, Grace Cham
Mario, 

We are tracking the bug and fqj@ and hscham@ will work on the fixes in Q2.

Mario Sanchez Prada

unread,
Mar 16, 2020, 1:12:57 PM3/16/20
to Laurent Chavey, Hidehiko Abe, Ken Rockot, Andrew Moylan, Dave Tapuska, chromium-mojo, platform-architecture-dev, Jeremy Roman, chromeos-libchrome, Qijiang Yūki Ishii, Yusuke Sato, Kentaro Hara, Satoru Takabayashi, Grace Cham
Hi Laurent,

On 16/3/20 14:50, Laurent Chavey wrote:
> Mario, 
>
> We are tracking the bug and fqj@ and hscham@ will work on the fixes in Q2.

Thanks the heads-up, just one clarification though: does this mean we
can resume the migration of //components/arc to the new Mojo types now?

Or would you rather have us waiting until those fixes (e.g. integrating
the downgrading script in libchrome) are developed in Q2?

Thanks in advance,
Mario
Reply all
Reply to author
Forward
0 new messages