Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13068002:
Send vsync notification from browser to renderer (Closed)
|
Created:
7 years, 9 months ago by Sami Modified:
7 years, 8 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, penghuang+watch_chromium.org, apatrick_chromium, joi+watch-content_chromium.org, darin-cc_chromium.org, James Su, jonathan.backer Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSend vsync notification from browser to renderer on Android
Allow the renderer to subscribe to a vsync notification from the
browser. This change adds the necessary IPC plumbing but does not
actually implement the vsync signal source; this is done with a
separate patch.
BUG=149227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195136
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rename EnableVSyncNotification to SetVSyncNotificationEnabled #
Total comments: 6
Patch Set 3 : Feedback from jam. #Patch Set 4 : Made the RWHV API Android-only. #
Total comments: 6
Patch Set 5 : Move code into Android ifdef. #
Total comments: 1
Patch Set 6 : Move ifdef'd messages to the end. #
Messages
Total messages: 27 (0 generated)
This patch adds the plumbing for vsync notifications between the browser and the renderer. PTAL.
lgtm https://codereview.chromium.org/13068002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/13068002/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_impl.h:584: void OnEnableVSyncNotification(bool enable); nit: Enable usually means a transition from disabled to enabled but not the other way around. Maybe ToggleVSyncNotification or On(Request|Set)VSyncNotificationEnabled?
https://codereview.chromium.org/13068002/diff/1/content/browser/renderer_host... File content/browser/renderer_host/render_widget_host_impl.h (right): https://codereview.chromium.org/13068002/diff/1/content/browser/renderer_host... content/browser/renderer_host/render_widget_host_impl.h:584: void OnEnableVSyncNotification(bool enable); On 2013/03/26 13:51:48, Martin Kosiba wrote: > nit: Enable usually means a transition from disabled to enabled but not the > other way around. Maybe ToggleVSyncNotification or > On(Request|Set)VSyncNotificationEnabled? Agreed, OnSetVSyncNotificationEnabled reads much better.
cc:backer
lgtm for content/renderer/, but I can't review the rest.
Thanks guys. Adding some owners: piman: Mind checking the changes in content/browser? palmer: Please check the new messages in view_messages.h. jam: New API under content/port/.
where are the view implementations? i.e. why aren't they part of this change? https://codereview.chromium.org/13068002/diff/5001/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/13068002/diff/5001/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_impl.cc:1637: "enabled", enabled); does this need to be traced? i.e. is this something that others care to see? I'm a bit concerned that all these internals of code in content are traced, and there are tests that look for specific trace events. i worry that it's a replacement of something like notification service, which we're trying to remove from content https://codereview.chromium.org/13068002/diff/5001/content/common/view_messag... File content/common/view_messages.h (right): https://codereview.chromium.org/13068002/diff/5001/content/common/view_messag... content/common/view_messages.h:761: IPC_MESSAGE_ROUTED1(ViewHostMsg_SetVSyncNotificationEnabled, nit: see the order of this file, where ViewMsg are together, and ViewHostMsg are together at the end https://codereview.chromium.org/13068002/diff/5001/content/renderer/gpu/compo... File content/renderer/gpu/compositor_output_surface.cc (right): https://codereview.chromium.org/13068002/diff/5001/content/renderer/gpu/compo... content/renderer/gpu/compositor_output_surface.cc:124: DCHECK(client_); nit: this dcheck, and the similar one below, is redundant. in release builds, it does nothing. in debug builds, the next line would crash which is just as strong of a signal to the developer that there's a null dereference
Thanks jam, all comments addressed. > where are the view implementations? i.e. why aren't they part of this change? I've opted to split this into smaller parts to keep the patch sizes manageable. See the http://crbug.com/149227#c12 for the complete list. The Android view implementation is at https://codereview.chromium.org/11959036/ and https://gerrit-int.chromium.org/#/c/30041/. https://codereview.chromium.org/13068002/diff/5001/content/browser/renderer_h... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/13068002/diff/5001/content/browser/renderer_h... content/browser/renderer_host/render_widget_host_impl.cc:1637: "enabled", enabled); On 2013/03/27 16:13:06, jam wrote: > does this need to be traced? i.e. is this something that others care to see? > > I'm a bit concerned that all these internals of code in content are traced, and > there are tests that look for specific trace events. i worry that it's a > replacement of something like notification service, which we're trying to remove > from content This particular trace was for human eyes only (i.e., debugging) and not really that necessary in the end, so I've removed it. https://codereview.chromium.org/13068002/diff/5001/content/common/view_messag... File content/common/view_messages.h (right): https://codereview.chromium.org/13068002/diff/5001/content/common/view_messag... content/common/view_messages.h:761: IPC_MESSAGE_ROUTED1(ViewHostMsg_SetVSyncNotificationEnabled, On 2013/03/27 16:13:06, jam wrote: > nit: see the order of this file, where ViewMsg are together, and ViewHostMsg are > together at the end Thanks, I completely missed the grouping here. https://codereview.chromium.org/13068002/diff/5001/content/renderer/gpu/compo... File content/renderer/gpu/compositor_output_surface.cc (right): https://codereview.chromium.org/13068002/diff/5001/content/renderer/gpu/compo... content/renderer/gpu/compositor_output_surface.cc:124: DCHECK(client_); On 2013/03/27 16:13:06, jam wrote: > nit: this dcheck, and the similar one below, is redundant. in release builds, it > does nothing. in debug builds, the next line would crash which is just as strong > of a signal to the developer that there's a null dereference Good point, I'll remove this and the other similar checks in this file. This specific function didn't even use client_ so the check was doubly redundant.
On 2013/03/27 17:38:06, Sami wrote: > Thanks jam, all comments addressed. > > > where are the view implementations? i.e. why aren't they part of this change? > > I've opted to split this into smaller parts to keep the patch sizes manageable. > See the http://crbug.com/149227#c12 for the complete list. The Android view > implementation is at https://codereview.chromium.org/11959036/ and > https://gerrit-int.chromium.org/#/c/30041/. > Thanks, now that you show me that, it looks like the RWHV::SetVSyncNotificationEnabled method is only needed for android. in that case, that method should be at the bottom of that interface in the android-only section, and RWHVBase shouldn't be modified.
On Wed, Mar 27, 2013 at 12:31 PM, <jam@chromium.org> wrote: > On 2013/03/27 17:38:06, Sami wrote: > >> Thanks jam, all comments addressed. >> > > > where are the view implementations? i.e. why aren't they part of this >> > change? > > I've opted to split this into smaller parts to keep the patch sizes >> > manageable. > >> See the http://crbug.com/149227#c12 for the complete list. The Android >> view >> implementation is at https://codereview.chromium.**org/11959036/<https://codereview.chromium.org/1... >> https://gerrit-int.chromium.**org/#/c/30041/<https://gerrit-int.chromium.org/... >> . >> > > > Thanks, now that you show me that, it looks like the > RWHV::**SetVSyncNotificationEnabled method is only needed for android. in > that > case, that method should be at the bottom of that interface in the > android-only > section, and RWHVBase shouldn't be modified. > I'm hoping this will be used on Aura as well. > > https://codereview.chromium.**org/13068002/<https://codereview.chromium.org/1... >
IPC security LGTM
On 2013/03/27 19:38:32, piman wrote: > On Wed, Mar 27, 2013 at 12:31 PM, <mailto:jam@chromium.org> wrote: > > > On 2013/03/27 17:38:06, Sami wrote: > > > >> Thanks jam, all comments addressed. > >> > > > > > where are the view implementations? i.e. why aren't they part of this > >> > > change? > > > > I've opted to split this into smaller parts to keep the patch sizes > >> > > manageable. > > > >> See the http://crbug.com/149227#c12 for the complete list. The Android > >> view > >> implementation is at > https://codereview.chromium.**org/11959036/%3Chttps://codereview.chromium.org... > >> > https://gerrit-int.chromium.**org/#/c/30041/%3Chttps://gerrit-int.chromium.or...> > >> . > >> > > > > > > Thanks, now that you show me that, it looks like the > > RWHV::**SetVSyncNotificationEnabled method is only needed for android. in > > that > > case, that method should be at the bottom of that interface in the > > android-only > > section, and RWHVBase shouldn't be modified. > > > > I'm hoping this will be used on Aura as well. At that point we can make it in outside the android ifdef?
On Wed, Mar 27, 2013 at 4:21 PM, <jam@chromium.org> wrote: > On 2013/03/27 19:38:32, piman wrote: > > On Wed, Mar 27, 2013 at 12:31 PM, <mailto:jam@chromium.org> wrote: >> > > > On 2013/03/27 17:38:06, Sami wrote: >> > >> >> Thanks jam, all comments addressed. >> >> >> > >> > > where are the view implementations? i.e. why aren't they part of this >> >> >> > change? >> > >> > I've opted to split this into smaller parts to keep the patch sizes >> >> >> > manageable. >> > >> >> See the http://crbug.com/149227#c12 for the complete list. The Android >> >> view >> >> implementation is at >> > > https://codereview.chromium.****org/11959036/%3Chttps://codere** > view.chromium.org/11959036/%**3Eand<http://codereview.chromium.org/11959036/%3Eand> > >> >> >> > > https://gerrit-int.chromium.****org/#/c/30041/%3Chttps://gerri** > t-int.chromium.org/#/c/30041/ <http://gerrit-int.chromium.org/#/c/30041/>> > >> >> . >> >> >> > >> > >> > Thanks, now that you show me that, it looks like the >> > RWHV::****SetVSyncNotificationEnabled method is only needed for >> android. in >> >> > that >> > case, that method should be at the bottom of that interface in the >> > android-only >> > section, and RWHVBase shouldn't be modified. >> > >> > > I'm hoping this will be used on Aura as well. >> > > At that point we can make it in outside the android ifdef? > Or you may end up with someone adding an aura-specific call, that will be reviewed by someone else than either of us and land that way. Why should we not try to converge platforms? ifdefs in APIs are much harder to deal with than ifdefs in implementations. Also, they leak all over the place, and encourage divergence between platforms (want to help me clean the mess in GpuProcesHost? its origin is due to adding APIs in ifdefs). > > https://codereview.chromium.**org/13068002/<https://codereview.chromium.org/1... >
On 2013/03/27 23:42:59, piman wrote: > Why should we not try to converge platforms? > > ifdefs in APIs are much harder to deal with than ifdefs in implementations. > Also, they leak all over the place, and encourage divergence between > platforms (want to help me clean the mess in GpuProcesHost? its origin is > due to adding APIs in ifdefs). I'd also prefer to not ifdef this since I don't think there is anything really Android specific about this part of the API. Jonathan, can you comment whether this would be useful for Aura in its current shape?
Looks like this got stalled a bit. jam/piman, I'm happy to move this inside an ANDROID ifdef if you prefer? I still think it's applicable to other platforms too, but having an ifdef would indeed make it clearer that only Android supports it at the moment.
On 2013/04/08 17:41:09, Sami wrote: > Looks like this got stalled a bit. jam/piman, I'm happy to move this inside an > ANDROID ifdef if you prefer? I still think it's applicable to other platforms > too, but having an ifdef would indeed make it clearer that only Android supports > it at the moment. I'd strongly prefer no ifdefs.
On 2013/04/08 18:00:14, piman wrote: > I'd strongly prefer no ifdefs. I agree. jam, wdyt?
Since this is all per-view: Would the compositor instance in the renderer for a given tab be responsible for calling SetVSyncNotificationEnabled(false) when the tab becomes invisible?
On 2013/04/09 16:13:05, Daniel Sievers wrote: > Since this is all per-view: Would the compositor instance in the renderer for a > given tab be responsible for calling SetVSyncNotificationEnabled(false) when the > tab becomes invisible? That's correct. It's tied to the same mechanism that disables the vsync timer when a tab becomes hidden. This is also why the browser needs to support multiple simultaneous vsync clients since the notifications may be disabled and enabled out of order during a tab switch.
Thanks all. jam, any thoughts on this patch?
sorry for the delay, I missed all these messages. in the future, feel free to IM me if I don't respond in a reasonable timeframe. I understand the tradeoff between making the API ifdef'd out or generic on all platforms. The issue with making it available on all platforms is that it's confusing. someone working on mac etc would wonder if it's a bug that they don't call this method or handle the callback. I think the likelihood that someone would add the same method on another platform without converging the too is minimal. I pay close attention to all reviews in content/public and content/port and I'll make sure this doesn't happen.
On 2013/04/12 16:34:02, jam wrote: > sorry for the delay, I missed all these messages. in the future, feel free to IM > me if I don't respond in a reasonable timeframe. > > I understand the tradeoff between making the API ifdef'd out or generic on all > platforms. The issue with making it available on all platforms is that it's > confusing. someone working on mac etc would wonder if it's a bug that they don't > call this method or handle the callback. > > I think the likelihood that someone would add the same method on another > platform without converging the too is minimal. I pay close attention to all > reviews in content/public and content/port and I'll make sure this doesn't > happen. Thanks, I've now made RWHV API Android-only. PTAL and let me know if this looks clearer.
https://codereview.chromium.org/13068002/diff/31001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/13068002/diff/31001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_impl.cc:367: IPC_MESSAGE_HANDLER(ViewHostMsg_SetVSyncNotificationEnabled, since these messages are only sent/received on android, send/dispatch them in RenderWidgetHostViewAndroid https://codereview.chromium.org/13068002/diff/31001/content/renderer/gpu/comp... File content/renderer/gpu/compositor_output_surface.cc (right): https://codereview.chromium.org/13068002/diff/31001/content/renderer/gpu/comp... content/renderer/gpu/compositor_output_surface.cc:134: IPC_MESSAGE_HANDLER(ViewMsg_DidVSync, DidVSync); nit: follow conventions of message handlers always starting with "On" https://codereview.chromium.org/13068002/diff/31001/content/renderer/gpu/comp... content/renderer/gpu/compositor_output_surface.cc:145: void CompositorOutputSurface::EnableVSyncNotification(bool enable) { can we ifdef these methods for android to make it clear that they're only called on that os?
Thanks. All comments addressed, PTAL. https://codereview.chromium.org/13068002/diff/31001/content/browser/renderer_... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/13068002/diff/31001/content/browser/renderer_... content/browser/renderer_host/render_widget_host_impl.cc:367: IPC_MESSAGE_HANDLER(ViewHostMsg_SetVSyncNotificationEnabled, On 2013/04/16 16:40:34, jam wrote: > since these messages are only sent/received on android, send/dispatch them in > RenderWidgetHostViewAndroid Done. I've also moved them into the Android-specific sections in view_messages.h https://codereview.chromium.org/13068002/diff/31001/content/renderer/gpu/comp... File content/renderer/gpu/compositor_output_surface.cc (right): https://codereview.chromium.org/13068002/diff/31001/content/renderer/gpu/comp... content/renderer/gpu/compositor_output_surface.cc:134: IPC_MESSAGE_HANDLER(ViewMsg_DidVSync, DidVSync); On 2013/04/16 16:40:34, jam wrote: > nit: follow conventions of message handlers always starting with "On" Done. https://codereview.chromium.org/13068002/diff/31001/content/renderer/gpu/comp... content/renderer/gpu/compositor_output_surface.cc:145: void CompositorOutputSurface::EnableVSyncNotification(bool enable) { On 2013/04/16 16:40:34, jam wrote: > can we ifdef these methods for android to make it clear that they're only called > on that os? Sure, I've added ifdefs here too.
lgtm https://codereview.chromium.org/13068002/diff/41001/content/renderer/gpu/comp... File content/renderer/gpu/compositor_output_surface.cc (right): https://codereview.chromium.org/13068002/diff/41001/content/renderer/gpu/comp... content/renderer/gpu/compositor_output_surface.cc:136: #if defined(OS_ANDROID) nit: convention is to put ifdef section at end of messages that are dispatched on all platforms (slightly more readable)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/13068002/47001
Message was sent while issue was closed.
Change committed as 195136 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
