Remove BrowserPlugin's -internal-attach method

chrome/renderer/extensions/guest_view_internal_custom_bindings.cc

+// Copyright 2014 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 "chrome/renderer/extensions/guest_view_internal_custom_bindings.h"
+
+#include <string>
+
+#include "base/bind.h"
+#include "chrome/common/extensions/chrome_extension_messages.h"
+#include "content/public/renderer/render_frame.h"
+#include "content/public/renderer/render_view.h"
+#include "content/public/renderer/v8_value_converter.h"
+#include "extensions/common/extension.h"
+#include "extensions/renderer/script_context.h"
+#include "v8/include/v8.h"
+
+using content::V8ValueConverter;
+
+namespace extensions {
+
+GuestViewInternalCustomBindings::GuestViewInternalCustomBindings(
+    ScriptContext* context)
+    : ObjectBackedNativeHandler(context) {
+  RouteFunction("AttachGuest",
+                base::Bind(&GuestViewInternalCustomBindings::AttachGuest,
+                           base::Unretained(this)));
+}
+
+void GuestViewInternalCustomBindings::AttachGuest(
+    const v8::FunctionCallbackInfo<v8::Value>& args) {
+  content::RenderFrame* render_frame = context()->GetRenderFrame();

[not at google - send to devlin, 2014/08/13 23:40:16]

Nit: do this after argument validation.

[Fady Samuel, 2014/08/14 19:41:18]

Done.

+  if (!render_frame)
+    return;
+
+  if (args.Length() < 3 || !args[0]->IsInt32() || !args[1]->IsInt32() ||

[not at google - send to devlin, 2014/08/13 23:40:16]

!=3. Be explicit.

[Fady Samuel, 2014/08/14 19:41:18]

Done.

+      !args[2]->IsObject()) {

[not at google - send to devlin, 2014/08/13 23:40:16]

Add NOTREACHED() here as well. Or even CHECK. Best not to silently swallow
errors, it's cost me much time in the past debugging JS.

[Fady Samuel, 2014/08/14 19:41:18]

Done.

+    return;
+  }
+
+  int element_instance_id = args[0]->Int32Value();
+  int guest_instance_id = args[1]->Int32Value();
+  scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
+  scoped_ptr<base::Value> params_value(
+      converter->FromV8Value(args[2], context()->v8_context()));
+  if (base::Value::TYPE_DICTIONARY != params_value->GetType()) {

[not at google - send to devlin, 2014/08/13 23:40:16]

If it's not a dictionary then V8ValueConverter is having issues, since you
checked it was an Object. Maybe NOTREACHED() this.

Also, don't use a Yoda condition, use params_value->IsType(DICTIONARY).

Final nit: it's nice not to leave dangling temporary variables like
|params_value| hanging around, asking to be incorrectly used. It's nice to scope
them locally, like

scoped_ptr<DictionaryValue> params;
{
  scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
  scoped_ptr<Value> params_as_value(
      converter->FromV8Value(args[2], context()->v8_context());
  CHECK(params_as_value->IsType(DICTIONARY));
  params = params_as_value.PassAs<DictionaryValue>();
}

Oops wrote more code there than I intended. You get the gist.

[Fady Samuel, 2014/08/14 19:41:18]

Done.

+    return;
+  }
+
+  // Step 1, send the attach params to chrome/.
+  scoped_ptr<base::DictionaryValue> params_dictionary(
+      static_cast<base::DictionaryValue*>(params_value.release()));

[not at google - send to devlin, 2014/08/13 23:40:16]

Nit: use params_value.PassAs()?

[Fady Samuel, 2014/08/14 19:41:17]

Can't. PassAs can only be used for upcasting and not downcasting.

+  render_frame->Send(new ChromeExtensionHostMsg_GuestViewSetAttachParams(
+      render_frame->GetRenderView()->GetRoutingID(),
+      element_instance_id,
+      guest_instance_id,
+      *params_dictionary));
+
+  // Step 2, attach plugin through content/.
+  render_frame->AttachToBrowserPlugin(element_instance_id, guest_instance_id);
+
+  v8::Isolate* isolate = args.GetIsolate();
+  args.GetReturnValue().Set(v8::Boolean::New(isolate, true));

[not at google - send to devlin, 2014/08/13 23:40:16]

Nit: inline |isolate|. You could also use context()->isolate() but whatevs, same
diff.

[Fady Samuel, 2014/08/14 19:41:18]

Done.

+}
+
+}  // namespace extensions