sync compositor: Memory policy async IPC

content/browser/android/synchronous_compositor_host.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/android/synchronous_compositor_host.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/containers/hash_tables.h"
@@ skipping 78 matching lines @@
 
 SynchronousCompositorHost::~SynchronousCompositorHost() {
   client_->DidDestroyCompositor(this);
   if (weak_ptr_factory_.HasWeakPtrs())
     UpdateStateTask();
 }
 
 bool SynchronousCompositorHost::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(SynchronousCompositorHost, message)
+    IPC_MESSAGE_HANDLER(SyncCompositorHostMsg_OutputSurfaceCreated,
+                        OutputSurfaceCreated)
     IPC_MESSAGE_HANDLER(SyncCompositorHostMsg_UpdateState, ProcessCommonParams)
     IPC_MESSAGE_HANDLER(SyncCompositorHostMsg_OverScroll, OnOverScroll)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void SynchronousCompositorHost::DidBecomeCurrent() {
   client_->DidBecomeCurrent(this);
 }
@@ skipping 197 matching lines @@
 }
 
 void SynchronousCompositorHost::ReturnResources(
     uint32_t output_surface_id,
     const cc::CompositorFrameAck& frame_ack) {
   DCHECK(!frame_ack.resources.empty());
   sender_->Send(new SyncCompositorMsg_ReclaimResources(
       routing_id_, output_surface_id, frame_ack));
 }
 
 void SynchronousCompositorHost::SetMemoryPolicy(size_t bytes_limit) {

[hush (inactive), 2016/05/12 19:23:03]

again, for multiprocess cross-bit (32bit renderer + 64bit browser) , we need
fixed bit types across IPC. So size_t needs to be uint32_t here.
I'm not sure clang plugins work, but this should have been caught by clang
plugins introduced by this CL?
https://codereview.chromium.org/1810243002/
Or maybe we're just not running these plugins?

[boliu, 2016/05/12 20:16:45]

As discussed over lunch. The wire format is uint32_t, so this is not an issue in
cross-bit. I agree on principle that implicit casting is still bad, but in
practice it's fine since the number won't exceed uint32, and fixing both sides
is way out of scope for this CL

   if (bytes_limit_ == bytes_limit)
     return;
-  bytes_limit_ = bytes_limit;
-  SendAsyncCompositorStateIfNeeded();
+
+  if (sender_->Send(
+          new SyncCompositorMsg_SetMemoryPolicy(routing_id_, bytes_limit))) {
+    bytes_limit_ = bytes_limit;
+  }
 }
 
 void SynchronousCompositorHost::DidChangeRootLayerScrollOffset(
     const gfx::ScrollOffset& root_offset) {
   if (root_scroll_offset_ == root_offset)
     return;
   root_scroll_offset_updated_by_browser_ = true;
   root_scroll_offset_ = root_offset;
   SendAsyncCompositorStateIfNeeded();
 }
@@ skipping 81 matching lines @@
   PopulateCommonParams(&common_browser_params);
   SyncCompositorCommonRendererParams common_renderer_params;
   if (!sender_->Send(
           new SyncCompositorMsg_BeginFrame(routing_id_, common_browser_params,
                                            args, &common_renderer_params))) {
     return;
   }
   ProcessCommonParams(common_renderer_params);
 }
 
+void SynchronousCompositorHost::OutputSurfaceCreated() {
+  // New output surface is not aware of state from Browser side. So need to
+  // re-send all browser side state here.
+  sender_->Send(
+      new SyncCompositorMsg_SetMemoryPolicy(routing_id_, bytes_limit_));

[hush (inactive), 2016/05/12 19:23:03]

so... memory policy is *all* the browser side states? What about the states in
PopulateCommonParams?

[boliu, 2016/05/12 20:16:45]

It's not everything. I'm moving things to async IPCs, and out of
SynchronousCompositorProxy one at a time. If you go by state that's in
OutputSurface but *not* in Proxy, then yes, bytes_limit_ is the only one right
now.

[boliu, 2016/05/12 21:12:47]

Actually... this *is* everything for OutputSurface.

If you look at the remaining state in SyncCompositorCommonBrowserParams, scroll
goes to input handler, and begin_frame_source_paused goes to BeginFrameSource.
And that's it.

+}
+
 void SynchronousCompositorHost::OnOverScroll(
     const SyncCompositorCommonRendererParams& params,
     const DidOverscrollParams& over_scroll_params) {
   ProcessCommonParams(params);
   DidOverscroll(over_scroll_params);
 }
 
 void SynchronousCompositorHost::PopulateCommonParams(
     SyncCompositorCommonBrowserParams* params) {
   DCHECK(params);
-  params->bytes_limit = bytes_limit_;
   if (root_scroll_offset_updated_by_browser_) {
     params->root_scroll_offset = root_scroll_offset_;
     params->update_root_scroll_offset = root_scroll_offset_updated_by_browser_;
     root_scroll_offset_updated_by_browser_ = false;
   }
   params->begin_frame_source_paused = !is_active_;
 
   weak_ptr_factory_.InvalidateWeakPtrs();
 }
 
@@ skipping 33 matching lines @@
         params.scrollable_size, params.page_scale_factor,
         params.min_page_scale_factor, params.max_page_scale_factor);
   }
 }
 
 void SynchronousCompositorHost::UpdateNeedsBeginFrames() {
   rwhva_->OnSetNeedsBeginFrames(is_active_ && need_begin_frame_);
 }
 
 }  // namespace content