|
|
Created:
4 years, 2 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 2 months ago CC:
Aaron Boodman, abarth-chromium, cbentzel+watch_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert miscellaneous RenderThreadImpl messages to mojom
ViewMsg_NetworkConnectionChanged
ViewMsg_UpdateScrollbarTheme
ViewMsg_SystemColorsChanged
ViewMsg_SetWebKitSharedTimersSuspended
ViewMsg_PurgePluginListCache
Some complexity here due to some messages being Mac only and
using Mac-only enum types.
We would still like to avoid any support for preproccessing or
platform-specific declarations in mojom, but this CL does add
support for platform-specific (Mac only atm) typemap
configuration.
BUG=612500
Committed: https://crrev.com/a2db0da89dd2726e057e8af9b49b18a8a3a4813a
Cr-Commit-Position: refs/heads/master@{#426003}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : . #Patch Set 6 : rebase #
Total comments: 2
Patch Set 7 : . #Patch Set 8 : rebase #
Messages
Total messages: 65 (49 generated)
Description was changed from ========== Convert miscellaneous RenderThreadImpl messages to mojom BUG=612500 ========== to ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended Some complexity here due to some messages being Mac only and using Mac-only enum types. Added support for platform-specific typemap configuration. BUG=612500 ==========
Description was changed from ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended Some complexity here due to some messages being Mac only and using Mac-only enum types. Added support for platform-specific typemap configuration. BUG=612500 ========== to ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended ViewMsg_PurgePluginListCache Some complexity here due to some messages being Mac only and using Mac-only enum types. Added support for platform-specific typemap configuration. BUG=612500 ==========
Description was changed from ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended ViewMsg_PurgePluginListCache Some complexity here due to some messages being Mac only and using Mac-only enum types. Added support for platform-specific typemap configuration. BUG=612500 ========== to ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended ViewMsg_PurgePluginListCache Some complexity here due to some messages being Mac only and using Mac-only enum types. Added support for platform-specific typemap configuration. BUG=612500 ==========
Description was changed from ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended ViewMsg_PurgePluginListCache Some complexity here due to some messages being Mac only and using Mac-only enum types. Added support for platform-specific typemap configuration. BUG=612500 ========== to ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended ViewMsg_PurgePluginListCache Some complexity here due to some messages being Mac only and using Mac-only enum types. We would still like to avoid any support for preproccessing or platform-specific declarations in mojom, but this CL does add support for platform-specific (Mac only atm) typemap configuration. BUG=612500 ==========
The CQ bit was checked by [email protected] to run a CQ dry run
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
[email protected] changed reviewers: + [email protected], [email protected], [email protected]
PTAL nick@ for content/ sammc@ for mojo/ tsepez@ for mojom+messages These are straightforward message conversions, with the only real annoyance being treatment of platform-specific types.
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Messages are fine, but it's a pity we're now including code on all platforms -- do the mojom folks have an answer about platform-specific mojom? A separate file conditionalized in GN?
We've tried pretty hard to avoid platform-specific blocks in mojom, but I think it might be worth doing. There's more platform-specific IPC in Chrome than I think we had originally anticipated. I'll send a proposal to chromium-mojo for discussion. On Mon, Oct 10, 2016 at 10:26 AM, <[email protected]> wrote: > Messages are fine, but it's a pity we're now including code on all > platforms -- > do the mojom folks have an answer about platform-specific mojom? A separate > file conditionalized in GN? > > https://codereview.chromium.org/2400313002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
The NetInfoBrowserTest.VerifyNetworkStateInitialized failures look real and I assume are related to ViewMsg_NetworkConnectionChanged.
Ok, I'll give you an LGTM on the messages modulo the red bots.
On 2016/10/10 at 18:33:08, nick wrote: > The NetInfoBrowserTest.VerifyNetworkStateInitialized failures look real and I assume are related to ViewMsg_NetworkConnectionChanged. Sorry, I was temporarily pulled away from this CL. You're right, it's a real failure. Looking into it now.
On 2016/10/11 at 17:57:02, Ken Rockot wrote: > On 2016/10/10 at 18:33:08, nick wrote: > > The NetInfoBrowserTest.VerifyNetworkStateInitialized failures look real and I assume are related to ViewMsg_NetworkConnectionChanged. > > Sorry, I was temporarily pulled away from this CL. You're right, it's a real failure. Looking into it now. Right, so the problem is that -- sort of unavoidably -- it's impossible to request and use Channel-associated mojom interfaces before the Channel exists. This test actually causes an IPC to be sent on the RPH before RPHI::Init is called. RPHI queues outgoing messages internally if the |channel_| doesn't exist and Init hasn't been called yet. Because I recently added queueing logic to the channel implementation to address similar issues around ordering during process launch (see https://goo.gl/REW75h if curious), I believe everything should be correct if I completely remove |queued_messages_| from RPHI and instead ensure that |channel_| is always non-null. This will require a few small additional RPHI changes to track process state, since non-null |channel_| is currently interpreted roughly as "RPHI is initialized the process is still alive." WDYT?
On 2016/10/11 19:34:31, Ken Rockot wrote: > On 2016/10/11 at 17:57:02, Ken Rockot wrote: > > On 2016/10/10 at 18:33:08, nick wrote: > > > The NetInfoBrowserTest.VerifyNetworkStateInitialized failures look real and > I assume are related to ViewMsg_NetworkConnectionChanged. > > > > Sorry, I was temporarily pulled away from this CL. You're right, it's a real > failure. Looking into it now. > > Right, so the problem is that -- sort of unavoidably -- it's impossible to > request and use Channel-associated mojom interfaces before the Channel exists. > This test actually causes an IPC to be sent on the RPH before RPHI::Init is > called. > > RPHI queues outgoing messages internally if the |channel_| doesn't exist and > Init hasn't been called yet. > > Because I recently added queueing logic to the channel implementation to address > similar issues around ordering during process launch (see https://goo.gl/REW75h > if curious), I believe everything should be correct if I completely remove > |queued_messages_| from RPHI and instead ensure that |channel_| is always > non-null. This will require a few small additional RPHI changes to track process > state, since non-null |channel_| is currently interpreted roughly as "RPHI is > initialized the process is still alive." WDYT? This sounds reasonable; no reason to have two queueing mechanisms, and since we just branched we can probably tolerate whatever risk this entails.
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:120001) has been deleted
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: This issue passed the CQ dry run.
I think this is ready for content review now.
lgtm modulo one question https://codereview.chromium.org/2400313002/diff/200001/content/browser/theme_... File content/browser/theme_helper_mac.mm (right): https://codereview.chromium.org/2400313002/diff/200001/content/browser/theme_... content/browser/theme_helper_mac.mm:156: params->redraw = redraw; What happened to the rphi->RecomputeAndUpdateWebKitPreferences() ?
The CQ bit was checked by [email protected] to run a CQ dry run
https://codereview.chromium.org/2400313002/diff/200001/content/browser/theme_... File content/browser/theme_helper_mac.mm (right): https://codereview.chromium.org/2400313002/diff/200001/content/browser/theme_... content/browser/theme_helper_mac.mm:156: params->redraw = redraw; On 2016/10/17 at 21:26:22, ncarter wrote: > What happened to the rphi->RecomputeAndUpdateWebKitPreferences() ? Hmm. Seems to have disappeared. I have un-disappeared it.
The CQ bit was unchecked by [email protected]
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected], [email protected] Link to the patchset: https://codereview.chromium.org/2400313002/#ps240001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by [email protected]
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended ViewMsg_PurgePluginListCache Some complexity here due to some messages being Mac only and using Mac-only enum types. We would still like to avoid any support for preproccessing or platform-specific declarations in mojom, but this CL does add support for platform-specific (Mac only atm) typemap configuration. BUG=612500 ========== to ========== Convert miscellaneous RenderThreadImpl messages to mojom ViewMsg_NetworkConnectionChanged ViewMsg_UpdateScrollbarTheme ViewMsg_SystemColorsChanged ViewMsg_SetWebKitSharedTimersSuspended ViewMsg_PurgePluginListCache Some complexity here due to some messages being Mac only and using Mac-only enum types. We would still like to avoid any support for preproccessing or platform-specific declarations in mojom, but this CL does add support for platform-specific (Mac only atm) typemap configuration. BUG=612500 Committed: https://crrev.com/a2db0da89dd2726e057e8af9b49b18a8a3a4813a Cr-Commit-Position: refs/heads/master@{#426003} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/a2db0da89dd2726e057e8af9b49b18a8a3a4813a Cr-Commit-Position: refs/heads/master@{#426003} |