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

[Mario Sanchez Prada]

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]

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]

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]

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]

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]

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]

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]

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.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CANPWpyDTDnH5XteDi7E4vqPSidTic_JVxXjuNPx3EirZOg5pPg%40mail.gmail.com.

[Daniel Cheng]

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.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CANPWpyDTDnH5XteDi7E4vqPSidTic_JVxXjuNPx3EirZOg5pPg%40mail.gmail.com.

[Andrew Moylan]

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:

crrev.com/c/1809024

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.

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CA%2BapAgFYMJXvJr5-t%3Dg7%3DvRdBgaCuYdx_S%3DJvZRVkBMswJpe6Q%40mail.gmail.com.

[Hidehiko Abe]

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

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]

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]

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

[Mario Sanchez Prada]

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

[Hidehiko Abe]

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]

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]

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]

# 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]

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]

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]

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