[Extensions OOPI] Update app window bindings for OOPI

extensions/browser/api/app_window/app_window_api.cc

 // Copyright (c) 2012 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 "extensions/browser/api/app_window/app_window_api.h"
 
 #include "base/command_line.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
-#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/app_window/app_window_client.h"
 #include "extensions/browser/app_window/app_window_contents.h"
 #include "extensions/browser/app_window/app_window_registry.h"
 #include "extensions/browser/app_window/native_app_window.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/common/api/app_window.h"
 #include "extensions/common/features/simple_feature.h"
@@ skipping 141 matching lines @@
           content::CONSOLE_MESSAGE_LEVEL_WARNING,
           "The 'singleton' option in chrome.apps.window.create() is deprecated!"
           " Change your code to no longer rely on this.");
       }
 
       if (!options->singleton || *options->singleton) {
         AppWindow* window = AppWindowRegistry::Get(browser_context())
                                 ->GetAppWindowForAppAndKey(
                                     extension_id(), create_params.window_key);
         if (window) {
-          content::RenderViewHost* created_view =
-              window->web_contents()->GetRenderViewHost();
-          int view_id = MSG_ROUTING_NONE;
+          content::RenderFrameHost* created_frame =

[not at google - send to devlin, 2015/07/08 21:34:29]

"created_frame" is actually a lie. The whole point of this code is that you
*didn't* create any frame, you're getting one that already existed. Maybe call
it "main_frame" or something.

Or "existing_frame". I'd like to see "window" renamed "existing_window". 

[Devlin, 2015/07/08 21:47:01]

Done.

+              window->web_contents()->GetMainFrame();
+          int frame_id = MSG_ROUTING_NONE;
           if (render_frame_host()->GetProcess()->GetID() ==
-              created_view->GetProcess()->GetID()) {
-            view_id = created_view->GetRoutingID();
+              created_frame->GetProcess()->GetID()) {

[not at google - send to devlin, 2015/07/08 20:22:19]

Why does the API do this? Why not unconditionally set the routing ID? Same as
below.

[Devlin, 2015/07/08 21:08:46]

My guess is because if they are in different processes (though I'm not sure
when/how that happens), sending over the frame id is wrong.  Frame ids are
unique in a process, but not (I think) across processes.  Since we respond to
render_frame_host(), if it is in a different process, it could (hypothetically)
get a completely wrong frame.

[not at google - send to devlin, 2015/07/08 21:34:29]

Yeah I feel like this should be a CHECK that they're the same process, because
it's an invariant that they are. The only process that can call
app-window-create is the app process itself, which is where the window is
created. It's misleading to do this check.

Or at least a DCHECK.

Also you should be able to compare GetProcess() calls directly (i.e. pointer
equality), no need to grab their IDs.

And, the same deal below. In fact it's sad that this code and the other don't
share much code, because they do basically the same thing.

[Devlin, 2015/07/08 21:47:01]

As discovered offline, this was added because component apps can create app
windows for other apps (in which case this won't be true).

I don't wanna dive into it here, since that's not the point of this CL, but I
added a TODO to look into it (and potentially kill it).

+            frame_id = created_frame->GetRoutingID();
           }
 
           if (!options->hidden.get() || !*options->hidden.get()) {
             if (options->focused.get() && !*options->focused.get())
               window->Show(AppWindow::SHOW_INACTIVE);
             else
               window->Show(AppWindow::SHOW_ACTIVE);
           }
 
           base::DictionaryValue* result = new base::DictionaryValue;
-          result->Set("viewId", new base::FundamentalValue(view_id));
+          result->Set("frameId", new base::FundamentalValue(frame_id));
           window->GetSerializedState(result);
           result->SetBoolean("existingWindow", true);
           // TODO(benwells): Remove HTML titlebar injection.
           result->SetBoolean("injectTitlebar", false);
           SetResult(result);
           SendResponse(true);
           return true;
         }
       }
     }
@@ skipping 132 matching lines @@
 
   AppWindow* app_window =
       AppWindowClient::Get()->CreateAppWindow(browser_context(), extension());
   app_window->Init(url, new AppWindowContentsImpl(app_window), create_params);
 
   if (ExtensionsBrowserClient::Get()->IsRunningInForcedAppMode() &&
       !app_window->is_ime_window()) {
     app_window->ForcedFullscreen();
   }
 
-  content::RenderViewHost* created_view =
-      app_window->web_contents()->GetRenderViewHost();
-  int view_id = MSG_ROUTING_NONE;
-  if (create_params.creator_process_id == created_view->GetProcess()->GetID())
-    view_id = created_view->GetRoutingID();
+  content::RenderFrameHost* created_frame =
+      app_window->web_contents()->GetMainFrame();
+  int frame_id = MSG_ROUTING_NONE;
+  if (create_params.creator_process_id == created_frame->GetProcess()->GetID())
+    frame_id = created_frame->GetRoutingID();
 
   base::DictionaryValue* result = new base::DictionaryValue;
-  result->Set("viewId", new base::FundamentalValue(view_id));
+  result->Set("frameId", new base::FundamentalValue(frame_id));
   result->Set("injectTitlebar",
       new base::FundamentalValue(inject_html_titlebar_));
   result->Set("id", new base::StringValue(app_window->window_key()));
   app_window->GetSerializedState(result);
   SetResult(result);
 
   if (AppWindowRegistry::Get(browser_context())
-          ->HadDevToolsAttached(created_view)) {
+          ->HadDevToolsAttached(app_window->web_contents())) {
     AppWindowClient::Get()->OpenDevToolsWindow(
         app_window->web_contents(),
         base::Bind(&AppWindowCreateFunction::SendResponse, this, true));
     return true;
   }
 
   SendResponse(true);
   app_window->WindowEventsReady();
 
   return true;
@@ skipping 174 matching lines @@
 
   if (options.frame->as_frame_options->inactive_color.get()) {
     error_ = app_window_constants::kInactiveColorWithoutColor;
     return false;
   }
 
   return true;
 }
 
 }  // namespace extensions