<webview>: resizing with display:none set should resize on attachment

extensions/renderer/guest_view/guest_view_container.cc

Index: extensions/renderer/guest_view/guest_view_container.cc
diff --git a/extensions/renderer/guest_view/guest_view_container.cc b/extensions/renderer/guest_view/guest_view_container.cc
index 8ef8054ae714ce18059109e5f2029960d9d28113..e814125ebb493f0761b52b9729cf3041de56332d 100644
--- a/extensions/renderer/guest_view/guest_view_container.cc
+++ b/extensions/renderer/guest_view/guest_view_container.cc
@@ -23,6 +23,27 @@ static base::LazyInstance<GuestViewContainerMap> g_guest_view_container_map =
 
 namespace extensions {
 
+GuestViewContainer::AttachRequest::AttachRequest(
+    int element_instance_id,
+    int guest_instance_id,
+    scoped_ptr<base::DictionaryValue> params,
+    v8::Handle<v8::Function> callback,
+    v8::Isolate* isolate)
+    : element_instance_id_(element_instance_id),
+      guest_instance_id_(guest_instance_id),
+      params_(params.Pass()),
+      callback_(callback),
+      isolate_(isolate) {
+}
+
+GuestViewContainer::AttachRequest::~AttachRequest() {
+}
+
+v8::Handle<v8::Function>
+GuestViewContainer::AttachRequest::GetCallback() const {
+  return callback_.NewHandle(isolate_);
+}
+
 GuestViewContainer::GuestViewContainer(
     content::RenderFrame* render_frame,
     const std::string& mime_type)
@@ -32,8 +53,7 @@ GuestViewContainer::GuestViewContainer(
       element_instance_id_(guestview::kInstanceIDNone),
       render_view_routing_id_(render_frame->GetRenderView()->GetRoutingID()),
       attached_(false),
-      attach_pending_(false),
-      isolate_(NULL) {
+      ready_(false) {
 }
 
 GuestViewContainer::~GuestViewContainer() {
@@ -52,29 +72,12 @@ GuestViewContainer* GuestViewContainer::FromID(int render_view_routing_id,
   return it == guest_view_containers->end() ? NULL : it->second;
 }
 
-
-void GuestViewContainer::AttachGuest(int element_instance_id,
-                                     int guest_instance_id,
-                                     scoped_ptr<base::DictionaryValue> params,
-                                     v8::Handle<v8::Function> callback,
-                                     v8::Isolate* isolate) {
-  // GuestViewContainer supports reattachment (i.e. attached_ == true) but not
-  // while a current attach process is pending.
-  if (attach_pending_)
+void GuestViewContainer::AttachGuest(linked_ptr<AttachRequest> request) {
+  if (!ready_ || !pending_requests_.empty() || pending_response_.get()) {
+    pending_requests_.push_back(request);

[lazyboy, 2014/10/03 20:30:05]

Can you add a TODO here as we discussed: about queues with > 1 element wouldn't
be necessary to maintain.

[Fady Samuel, 2014/10/03 21:52:30]

Done.

     return;
-
-  // Step 1, send the attach params to chrome/.
-  render_frame()->Send(new ExtensionHostMsg_AttachGuest(render_view_routing_id_,
-                                                        element_instance_id,
-                                                        guest_instance_id,
-                                                        *params));
-
-  // Step 2, attach plugin through content/.
-  render_frame()->AttachGuest(element_instance_id);
-
-  callback_.reset(callback);
-  isolate_ = isolate;
-  attach_pending_ = true;
+  }
+  AttachGuestInternal(request);
 }
 
 void GuestViewContainer::SetElementInstanceID(int element_instance_id) {
@@ -100,6 +103,17 @@ void GuestViewContainer::DidReceiveData(const char* data, int data_length) {
   html_string_ += value;
 }
 
+void GuestViewContainer::Ready() {
+  ready_ = true;
+  CHECK(!pending_response_.get());
+  if (pending_requests_.empty())
+    return;
+
+  linked_ptr<AttachRequest> pending_request = pending_requests_.front();
+  pending_requests_.pop_front();
+  AttachGuestInternal(pending_request);
+}
+
 void GuestViewContainer::OnDestruct() {
   // GuestViewContainer's lifetime is managed by BrowserPlugin so don't let
   // RenderFrameObserver self-destruct here.
@@ -137,11 +151,12 @@ void GuestViewContainer::OnCreateMimeHandlerViewGuestACK(
 
 void GuestViewContainer::OnGuestAttached(int element_instance_id,
                                          int guest_routing_id) {
+  CHECK(pending_response_.get());
+
   attached_ = true;
-  attach_pending_ = false;
 
   // If we don't have a callback then there's nothing more to do.
-  if (callback_.IsEmpty())
+  if (!pending_response_->has_callback())
     return;

[lazyboy, 2014/10/03 20:30:05]

Couldn't there be more stuff in the queue to be processed?

[Fady Samuel, 2014/10/03 21:52:30]

Good catch!

 
   content::RenderView* guest_proxy_render_view =
@@ -150,8 +165,8 @@ void GuestViewContainer::OnGuestAttached(int element_instance_id,
   if (!guest_proxy_render_view)
     return;
 
-  v8::HandleScope handle_scope(isolate_);
-  v8::Handle<v8::Function> callback = callback_.NewHandle(isolate_);
+  v8::HandleScope handle_scope(pending_response_->isolate());
+  v8::Handle<v8::Function> callback = pending_response_->GetCallback();
   v8::Handle<v8::Context> context = callback->CreationContext();
   if (context.IsEmpty())
     return;
@@ -168,7 +183,29 @@ void GuestViewContainer::OnGuestAttached(int element_instance_id,
   // Call the AttachGuest API's callback with the guest proxy as the first
   // parameter.
   callback->Call(context->Global(), argc, argv);
-  callback_.reset();
+
+  pending_response_.reset();
+  if (!pending_requests_.empty()) {
+    linked_ptr<AttachRequest> pending_request = pending_requests_.front();

[lazyboy, 2014/10/03 20:30:05]

Can this picking from queue and putting it to pending_response_ be in one place?

e.g.
PutTask(request) { pending_requests_.insert(request); }
PickTask() {
  if (!ready || pending_response_ || pending_requests.empty()) return;
  AttachGuestInternal(pending_requests_.pop_front()); // ya that's void, but you
get the point
}

So ::Ready() would just call PickTask();
OnGuestAttached() would call PickTask() after it cleared pending_response_
AttachGuest() would call PutTask(); PickTask();

[Fady Samuel, 2014/10/03 21:52:30]

Done. I gave them slightly more descriptive names.

+    pending_requests_.pop_front();
+    AttachGuestInternal(pending_request);
+  }
+}
+
+void GuestViewContainer::AttachGuestInternal(
+    linked_ptr<AttachRequest> request) {
+  CHECK(!pending_response_.get());
+  // Step 1, send the attach params to chrome/.
+  render_frame()->Send(
+      new ExtensionHostMsg_AttachGuest(render_view_routing_id_,
+                                       request->element_instance_id(),
+                                       request->guest_instance_id(),
+                                       *request->attach_params()));
+
+  // Step 2, attach plugin through content/.
+  render_frame()->AttachGuest(request->element_instance_id());
+
+  pending_response_ = request;
 }
 
 // static