Add resize gutter layers to DelegatedFrameHost

content/browser/compositor/delegated_frame_host.cc

Index: content/browser/compositor/delegated_frame_host.cc
diff --git a/content/browser/compositor/delegated_frame_host.cc b/content/browser/compositor/delegated_frame_host.cc
index 7686744da1db7e1e9299a28ce77d141c49f76bc1..14b4ff82f5b0b3c4dc17ac7e5e8889aa165464a6 100644
--- a/content/browser/compositor/delegated_frame_host.cc
+++ b/content/browser/compositor/delegated_frame_host.cc
@@ -69,6 +69,7 @@ DelegatedFrameHost::DelegatedFrameHost(DelegatedFrameHostClient* client)
       last_output_surface_id_(0),
       pending_delegated_ack_count_(0),
       skipped_frames_(false),
+      background_color_(SK_ColorWHITE),

[danakj, 2016/03/08 00:09:43]

There's some work to be done to not show the wrong color during navigation:
https://bugs.chromium.org/p/chromium/issues/detail?id=470669

And this adds yet another WHITE constant somewhere, as well as another place
that decides on the right color to use, which I'm worried will cause some weird
stuff like black background white gutters sometimes or something. Can you
alleviate my fears or find a way to get a shared value for this?

[jbauman, 2016/03/08 01:45:29]

Actually this initialization isn't really necessary (I just did it because I
wanted to be sure we weren't reading uninitialized memory in crazy cases - I
could remove it).

We only create the gutters after we've received a frame from the renderer which
informs us of the metadata.root_background_color. That color was originally
intended to be used to create resize gutters, and we already use it for that
purpose on android. It's not guaranteed to always be correct, but in general it
seems pretty good.

[danakj, 2016/03/10 22:53:18]

Can you make it some more crazy noticeable color then? Red or Yellow or
something.

       current_scale_factor_(1.f),
       can_lock_compositor_(YES_CAN_LOCK),
       delegated_frame_evictor_(new DelegatedFrameEvictor(this)) {
@@ -253,6 +254,45 @@ void DelegatedFrameHost::WasResized() {
       !client_->DelegatedFrameHostIsVisible())
     EvictDelegatedFrame();
   MaybeCreateResizeLock();
+  UpdateGutters();
+}
+
+void DelegatedFrameHost::UpdateGutters() {
+  if (surface_id_.is_null()) {
+    right_gutter_.reset();
+    bottom_gutter_.reset();
+    return;
+  }
+  if (current_frame_size_in_dip_.width() <
+      client_->DelegatedFrameHostDesiredSizeInDIP().width()) {
+    // The right gutter also include the bottom-right corner, if necessary.

[danakj, 2016/03/08 00:09:44]

includes

[jbauman, 2016/03/08 01:45:29]

Done.

+    right_gutter_.reset(new ui::Layer(ui::LAYER_SOLID_COLOR));
+    right_gutter_->SetColor(background_color_);
+    int width = client_->DelegatedFrameHostDesiredSizeInDIP().width() -
+                current_frame_size_in_dip_.width();
+    right_gutter_->SetBounds(
+        gfx::Rect(current_frame_size_in_dip_.width(), 0, width,
+                  client_->DelegatedFrameHostDesiredSizeInDIP().height()));

[danakj, 2016/03/08 00:09:44]

nit: put this value in a temp |height| and move the comment above down onto that
variable?

[jbauman, 2016/03/08 01:45:29]

Done.

+
+    client_->DelegatedFrameHostGetLayer()->Add(right_gutter_.get());

[danakj, 2016/03/08 00:09:43]

One design question.. could cc::SurfaceLayer just show the background color as
solid color quads gutters from its AppendQuads? Does it have the info it would
need to do so? Would that be simpler or worse?

[jbauman, 2016/03/08 01:45:29]

I've thought about it, but currently the SurfaceLayer doesn't know the right
size or the correct background color to use (because it doesn't get the frame
data itself). So it's possible, but I don't know that it would actually make
more sense.

+  } else {
+    right_gutter_.reset();
+  }
+
+  if (current_frame_size_in_dip_.height() <
+      client_->DelegatedFrameHostDesiredSizeInDIP().height()) {
+    bottom_gutter_.reset(new ui::Layer(ui::LAYER_SOLID_COLOR));
+    bottom_gutter_->SetColor(background_color_);
+    int height = client_->DelegatedFrameHostDesiredSizeInDIP().height() -
+                 current_frame_size_in_dip_.height();
+    bottom_gutter_->SetBounds(gfx::Rect(0, current_frame_size_in_dip_.height(),
+                                        current_frame_size_in_dip_.width(),

[danakj, 2016/03/08 00:09:43]

nit: put this into a |width| var too, it's nice to compute them both beside each
other?

[jbauman, 2016/03/08 01:45:29]

Done.

+                                        height));
+    client_->DelegatedFrameHostGetLayer()->Add(bottom_gutter_.get());
+
+  } else {
+    bottom_gutter_.reset();
+  }
 }
 
 gfx::Size DelegatedFrameHost::GetRequestedRendererSize() const {
@@ -400,6 +440,8 @@ void DelegatedFrameHost::SwapDelegatedFrame(
   bool skip_frame = false;
   pending_delegated_ack_count_++;
 
+  background_color_ = frame->metadata.root_background_color;
+
   if (frame_size.IsEmpty()) {
     DCHECK(frame_data->resource_list.empty());
     EvictDelegatedFrame();
@@ -447,6 +489,8 @@ void DelegatedFrameHost::SwapDelegatedFrame(
   current_frame_size_in_dip_ = frame_size_in_dip;
   CheckResizeLock();
 
+  UpdateGutters();
+
   if (!damage_rect_in_dip.IsEmpty())
     client_->DelegatedFrameHostGetLayer()->OnDelegatedFrameDamage(
         damage_rect_in_dip);

[danakj, 2016/03/08 00:09:43]

When if ever should damage include the gutters?

[jbauman, 2016/03/08 01:45:29]

Never, AFAIK. This is only supposed to use damage from the delegated frame, and
in practice we use this only to detect whether a video is playing or not, so
including the gutters doesn't make sense.

@@ -524,6 +568,7 @@ void DelegatedFrameHost::EvictDelegatedFrame() {
     surface_id_ = cc::SurfaceId();
   }
   delegated_frame_evictor_->DiscardedFrame();
+  UpdateGutters();
 }
 
 // static