Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2647583002:
Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance (Closed)
|
Created:
3 years, 11 months ago by Saman Sami Modified:
3 years, 10 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitching to CompositorFrameSinkSupport in android_webview::SurfacesInstance
This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance
and replaces it with CompositorFrameSinkSupport. We're doing this to
unify the CompositorFrameSink API used by clients, and to simplify
transitioning to SurfaceReferences.
BUG=674102
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2647583002
Cr-Commit-Position: refs/heads/master@{#450035}
Committed: https://chromium.googlesource.com/chromium/src/+/9eb0f5a037f216e9a0d824ebadc3b204ce479e7a
Patch Set 1 #
Total comments: 6
Patch Set 2 : c #
Total comments: 4
Patch Set 3 : c #Patch Set 4 : c #
Total comments: 2
Patch Set 5 : c #Patch Set 6 : c #
Total comments: 6
Patch Set 7 : c #Patch Set 8 : c #
Total comments: 2
Patch Set 9 : c #
Total comments: 5
Patch Set 10 : c #
Total comments: 1
Patch Set 11 : c #Patch Set 12 : c #
Total comments: 8
Patch Set 13 : c #
Messages
Total messages: 78 (52 generated)
The CQ bit was checked by samans@chromium.org 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...
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
Just a couple of points. Please get boliu's opinion on this. Bo, this isn't introducing mojo, this is just using CompositorFrameSinkSupport to unify code paths. We eventually want to get rid of SurfaceFactory. https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... File android_webview/browser/surfaces_instance.cc (left): https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... android_webview/browser/surfaces_instance.cc:144: display_->DrawAndSwap(); I guess this is more a question to boliu@. Why is this here? Shouldn't display scheduler take care of this? https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... android_webview/browser/surfaces_instance.cc:55: surface_manager_->RegisterFrameSinkId(frame_sink_id_); I don't think this is necessary? CompositorFrameSinkSupport should take care of it. https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... android_webview/browser/surfaces_instance.cc:90: surface_manager_->InvalidateFrameSinkId(frame_sink_id_); I don't think this is necessary, CompositorFrameSinkSupport destruction should take care of it.
Description was changed from ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance BUG=674102 ========== to ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance BUG=674102 ==========
samans@chromium.org changed reviewers: + boliu@chromium.org
The CQ bit was checked by samans@chromium.org 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...
boliu: Please review all files. https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... android_webview/browser/surfaces_instance.cc:55: surface_manager_->RegisterFrameSinkId(frame_sink_id_); On 2017/01/18 23:14:40, Fady Samuel wrote: > I don't think this is necessary? CompositorFrameSinkSupport should take care of > it. Forgot to remove it. https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... android_webview/browser/surfaces_instance.cc:90: surface_manager_->InvalidateFrameSinkId(frame_sink_id_); On 2017/01/18 23:14:40, Fady Samuel wrote: > I don't think this is necessary, CompositorFrameSinkSupport destruction should > take care of it. Forgot to remove it.
Description was changed from ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance BUG=674102 ========== to ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. BUG=674102 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... File android_webview/browser/surfaces_instance.cc (left): https://codereview.chromium.org/2647583002/diff/1/android_webview/browser/sur... android_webview/browser/surfaces_instance.cc:144: display_->DrawAndSwap(); On 2017/01/18 23:14:40, Fady Samuel wrote: > I guess this is more a question to boliu@. Why is this here? Shouldn't display > scheduler take care of this? Basically android does the scheduling part for us, ie when it wants to draw, it calls DrawGL, which calls this. This means everything, from receiving and submitting a frame, all the way to command buffer swap frame needs to happen within that call. If you really want to, can probably massage DisplayScheduler and BeginFrameSource to fit into this, but needs to be super careful. Eg I see it takes a TaskRunner, which doesn't exist on this thread. https://codereview.chromium.org/2647583002/diff/20001/android_webview/browser... File android_webview/browser/surfaces_instance.cc (left): https://codereview.chromium.org/2647583002/diff/20001/android_webview/browser... android_webview/browser/surfaces_instance.cc:143: display_->Resize(viewport); this viewport and the frame size may not be the same https://codereview.chromium.org/2647583002/diff/20001/android_webview/browser... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/20001/android_webview/browser... android_webview/browser/surfaces_instance.h:34: public cc::DisplayClient, does this still need to be displayclient? is DisplayOutputSurfaceLost still going to be called?
The CQ bit was checked by samans@chromium.org 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by samans@chromium.org 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 commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL Display now resizes to viewport. I have also created another CL that removes the resizing code from CompositorFrameSinkSupport to avoid resizing twice. https://codereview.chromium.org/2647583002/diff/20001/android_webview/browser... File android_webview/browser/surfaces_instance.cc (left): https://codereview.chromium.org/2647583002/diff/20001/android_webview/browser... android_webview/browser/surfaces_instance.cc:143: display_->Resize(viewport); On 2017/01/19 00:24:13, boliu wrote: > this viewport and the frame size may not be the same Done. https://codereview.chromium.org/2647583002/diff/20001/android_webview/browser... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/20001/android_webview/browser... android_webview/browser/surfaces_instance.h:34: public cc::DisplayClient, On 2017/01/19 00:24:13, boliu wrote: > does this still need to be displayclient? is DisplayOutputSurfaceLost still > going to be called? No. Removed it.
On 2017/01/19 22:36:27, Saman Sami wrote: > PTAL > > Display now resizes to viewport. I have also created another CL that > removes the resizing code from CompositorFrameSinkSupport to > avoid resizing twice. That should be merged with this CL right? Or at least should land before this CL? https://codereview.chromium.org/2647583002/diff/60001/android_webview/browser... File android_webview/browser/surfaces_instance.cc (left): https://codereview.chromium.org/2647583002/diff/60001/android_webview/browser... android_webview/browser/surfaces_instance.cc:93: LOG(FATAL) << "Render thread context loss"; need to keep this behavior, so now this needs to be a SupportClient?
On 2017/01/19 22:44:39, boliu wrote: > On 2017/01/19 22:36:27, Saman Sami wrote: > > PTAL > > > > Display now resizes to viewport. I have also created another CL that > > removes the resizing code from CompositorFrameSinkSupport to > > avoid resizing twice. > > That should be merged with this CL right? Or at least should land before this > CL? > > https://codereview.chromium.org/2647583002/diff/60001/android_webview/browser... > File android_webview/browser/surfaces_instance.cc (left): > > https://codereview.chromium.org/2647583002/diff/60001/android_webview/browser... > android_webview/browser/surfaces_instance.cc:93: LOG(FATAL) << "Render thread > context loss"; > need to keep this behavior, so now this needs to be a SupportClient? Yes, that CL will land earlier. I will make this class a client of CompositorFrameSinkSupport to preserve this log message.
On 2017/01/19 22:47:48, Saman Sami wrote: > On 2017/01/19 22:44:39, boliu wrote: > > On 2017/01/19 22:36:27, Saman Sami wrote: > > > PTAL > > > > > > Display now resizes to viewport. I have also created another CL that > > > removes the resizing code from CompositorFrameSinkSupport to > > > avoid resizing twice. > > > > That should be merged with this CL right? Or at least should land before this > > CL? > > > > > https://codereview.chromium.org/2647583002/diff/60001/android_webview/browser... > > File android_webview/browser/surfaces_instance.cc (left): > > > > > https://codereview.chromium.org/2647583002/diff/60001/android_webview/browser... > > android_webview/browser/surfaces_instance.cc:93: LOG(FATAL) << "Render thread > > context loss"; > > need to keep this behavior, so now this needs to be a SupportClient? > > Yes, that CL will land earlier. I will make this class a client of > CompositorFrameSinkSupport to preserve this log message. fwiw, that's a FATAL log message, ie it will cause a crash
Description was changed from ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. BUG=674102 ========== to ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. BUG=674102 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by samans@chromium.org 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...
Description was changed from ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. BUG=674102 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. BUG=674102 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: + danakj@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
boliu: Please review android_webview/ danakj: Please review cc/ PTAL The issue with resizing is gone as of crrev.com/445606 The fatal log message is back. https://codereview.chromium.org/2647583002/diff/60001/android_webview/browser... File android_webview/browser/surfaces_instance.cc (left): https://codereview.chromium.org/2647583002/diff/60001/android_webview/browser... android_webview/browser/surfaces_instance.cc:93: LOG(FATAL) << "Render thread context loss"; On 2017/01/19 22:44:38, boliu wrote: > need to keep this behavior, so now this needs to be a SupportClient? Yes. We can do that.
https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... File android_webview/browser/surfaces_instance.cc (left): https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... android_webview/browser/surfaces_instance.cc:173: CHECK(resources.empty()); should probably keep this CHECK by implementing ReclaimResources? https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... android_webview/browser/surfaces_instance.h:19: class CompositorFrameSinkSupportClient; need to include this class in order to inherit from it, in which case don't need this forward declaration https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... android_webview/browser/surfaces_instance.h:55: ~SurfacesInstance(); still need override for CompositorFrameSinkSupportClient
The CQ bit was checked by samans@chromium.org 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...
PTAL https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... File android_webview/browser/surfaces_instance.cc (left): https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... android_webview/browser/surfaces_instance.cc:173: CHECK(resources.empty()); On 2017/01/24 16:16:52, boliu wrote: > should probably keep this CHECK by implementing ReclaimResources? Done. https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... android_webview/browser/surfaces_instance.h:19: class CompositorFrameSinkSupportClient; On 2017/01/24 16:16:52, boliu wrote: > need to include this class in order to inherit from it, in which case don't need > this forward declaration Done. https://codereview.chromium.org/2647583002/diff/100001/android_webview/browse... android_webview/browser/surfaces_instance.h:55: ~SurfacesInstance(); On 2017/01/24 16:16:52, boliu wrote: > still need override for CompositorFrameSinkSupportClient Done.
https://codereview.chromium.org/2647583002/diff/140001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support_client.h (right): https://codereview.chromium.org/2647583002/diff/140001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support_client.h:15: class CompositorFrameSinkSupportClient : public DisplayClient { This is not what I would expect, I don't think it's common for a client to be another client, and it seems like it can impose architectural limitations on our code that are not required.
The CQ bit was checked by samans@chromium.org 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...
PTAL https://codereview.chromium.org/2647583002/diff/140001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support_client.h (right): https://codereview.chromium.org/2647583002/diff/140001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support_client.h:15: class CompositorFrameSinkSupportClient : public DisplayClient { On 2017/01/24 17:12:33, danakj (slow) wrote: > This is not what I would expect, I don't think it's common for a client to be > another client, and it seems like it can impose architectural limitations on our > code that are not required. Done.
https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... android_webview/browser/surfaces_instance.h:59: // cc::CompositorFrameSinkSupport implementation. CompositorFrameSinkSupportClient https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... android_webview/browser/surfaces_instance.h:60: void DidReceiveCompositorFrameAck() override {} there is no need to override methods that are no-op?
The CQ bit was checked by samans@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... android_webview/browser/surfaces_instance.h:59: // cc::CompositorFrameSinkSupport implementation. On 2017/01/24 17:41:50, boliu wrote: > CompositorFrameSinkSupportClient Done. https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... android_webview/browser/surfaces_instance.h:60: void DidReceiveCompositorFrameAck() override {} On 2017/01/24 17:41:50, boliu wrote: > there is no need to override methods that are no-op? It's a pure virtual in the parent.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... android_webview/browser/surfaces_instance.h:60: void DidReceiveCompositorFrameAck() override {} On 2017/01/24 17:44:57, Saman Sami wrote: > On 2017/01/24 17:41:50, boliu wrote: > > there is no need to override methods that are no-op? > > It's a pure virtual in the parent. Oh oops, misread. Then the question is why are there a mix of pure virtual and default implemented methods then?
On 2017/01/24 17:50:57, boliu wrote: > https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... > File android_webview/browser/surfaces_instance.h (right): > > https://codereview.chromium.org/2647583002/diff/160001/android_webview/browse... > android_webview/browser/surfaces_instance.h:60: void > DidReceiveCompositorFrameAck() override {} > On 2017/01/24 17:44:57, Saman Sami wrote: > > On 2017/01/24 17:41:50, boliu wrote: > > > there is no need to override methods that are no-op? > > > > It's a pure virtual in the parent. > > Oh oops, misread. Then the question is why are there a mix of pure virtual and > default implemented methods then? I didn't touch the existing methods. I'm not sure if they should stay the same or not. I think Dana should be able to say if it makes sense to change them too.
https://codereview.chromium.org/2647583002/diff/180001/cc/surfaces/compositor... File cc/surfaces/compositor_frame_sink_support_client.h (right): https://codereview.chromium.org/2647583002/diff/180001/cc/surfaces/compositor... cc/surfaces/compositor_frame_sink_support_client.h:35: // DisplayClient methods. Hm this is weird too. Why is the CompositorFrameSinkSupport a DisplayClient if its just going to pass things thru?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Is there anything blocking this patch now?
The CQ bit was checked by samans@chromium.org 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 checked by samans@chromium.org 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...
https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.cc:55: this, surface_manager_.get(), frame_sink_id_, true)); true /* submits_to_display_compositor */ https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:67: void DidReceiveCompositorFrameAck() override {} Empty impl in cc body is the preferred style for virtual methods. https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:68: void OnBeginFrame(const cc::BeginFrameArgs& args) override {} Empty impl in cc body is the preferred style for virtual methods. https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:69: void WillDrawSurface() override {} Empty impl in cc body is the preferred style for virtual methods.
https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.cc:55: this, surface_manager_.get(), frame_sink_id_, true)); true /* submits_to_display_compositor */ https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:67: void DidReceiveCompositorFrameAck() override {} Empty impl in cc body is the preferred style for virtual methods. https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:68: void OnBeginFrame(const cc::BeginFrameArgs& args) override {} Empty impl in cc body is the preferred style for virtual methods. https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:69: void WillDrawSurface() override {} Empty impl in cc body is the preferred style for virtual methods.
samans@chromium.org changed reviewers: - danakj@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org 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...
boliu@: Please review all files fsamuel@: Please review all files https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... File android_webview/browser/surfaces_instance.cc (right): https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.cc:55: this, surface_manager_.get(), frame_sink_id_, true)); On 2017/02/13 17:54:09, Fady Samuel wrote: > true /* submits_to_display_compositor */ Done. https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... File android_webview/browser/surfaces_instance.h (right): https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:67: void DidReceiveCompositorFrameAck() override {} On 2017/02/13 17:54:09, Fady Samuel wrote: > Empty impl in cc body is the preferred style for virtual methods. Done. https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:68: void OnBeginFrame(const cc::BeginFrameArgs& args) override {} On 2017/02/13 17:54:09, Fady Samuel wrote: > Empty impl in cc body is the preferred style for virtual methods. Done. https://codereview.chromium.org/2647583002/diff/220001/android_webview/browse... android_webview/browser/surfaces_instance.h:69: void WillDrawSurface() override {} On 2017/02/13 17:54:09, Fady Samuel wrote: > Empty impl in cc body is the preferred style for virtual methods. Done.
BTW, in your CL description, please explain that we're doing this to unify the CompositorFrameSink API used by clients, and to simplify transitioning to SurfaceReferences.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. BUG=674102 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. We're doing this to unify the CompositorFrameSink API used by clients, and to simplify transitioning to SurfaceReferences. BUG=674102 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1487012011682560,
"parent_rev": "2cf5bd7d93f0d0fe88d69d13c7eb63a9465f2950", "commit_rev":
"9eb0f5a037f216e9a0d824ebadc3b204ce479e7a"}
Message was sent while issue was closed.
Description was changed from ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. We're doing this to unify the CompositorFrameSink API used by clients, and to simplify transitioning to SurfaceReferences. BUG=674102 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Switching to CompositorFrameSinkSupport in android_webview::SurfacesInstance This CL gets rid of SurfaceFactory in android_webview::SurfacesInstance and replaces it with CompositorFrameSinkSupport. We're doing this to unify the CompositorFrameSink API used by clients, and to simplify transitioning to SurfaceReferences. BUG=674102 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2647583002 Cr-Commit-Position: refs/heads/master@{#450035} Committed: https://chromium.googlesource.com/chromium/src/+/9eb0f5a037f216e9a0d824ebadc3... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/9eb0f5a037f216e9a0d824ebadc3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
