[Extensions OOPI] Update app window bindings for OOPI

[Extensions OOPI] Update app window bindings for OOPI

Update the app window custom bindings to use render frame logic
instead of render view logic.  This involves changes to the custom
bindings themselves, as well as app window creation logic. And, for fun,
a few small cleanups while I was in the area.

BUG=455776
TBR=dbeam@chromium.org (webui)

Committed: https://crrev.com/c3d6ba1b13ec1f050178e4f97c121b528c07520d
Cr-Commit-Position: refs/heads/master@{#338076}

[Devlin, 2015-07-08 15:37:56]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-07-08 15:38:08]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

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

lg but a couple of questions.

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
File extensions/browser/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
extensions/browser/api/app_window/app_window_api.cc:181:
created_frame->GetProcess()->GetID()) {
Why does the API do this? Why not unconditionally set the routing ID? Same as
below.

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/app_...
File extensions/browser/app_window/app_window_registry.cc (right):

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/app_...
extensions/browser/app_window/app_window_registry.cc:221: return std::string(); 
// Not a AppWindow.
an AppWindow

https://codereview.chromium.org/1211003006/diff/20001/extensions/renderer/app...
File extensions/renderer/app_window_custom_bindings.cc (right):

https://codereview.chromium.org/1211003006/diff/20001/extensions/renderer/app...
extensions/renderer/app_window_custom_bindings.cc:31: Dispatcher* dispatcher)
Can you change this to just pass in the ScriptContextSet, not the whole
Dispatcher? That way you can also remove the Dispatcher dependency entirely. You
only need the ScriptContextSet.

https://codereview.chromium.org/1211003006/diff/20001/extensions/renderer/app...
extensions/renderer/app_window_custom_bindings.cc:93: new
DidCreateDocumentElementObserver(app_frame, dispatcher_);
I feel like this shouldn't happen until we know for sure the window is actually
returned to the caller. Unnecessary/wasteful otherwise.

https://codereview.chromium.org/1211003006/diff/20001/extensions/renderer/app...
extensions/renderer/app_window_custom_bindings.cc:104: blink::WebFrame* opener =
context_render_view->GetWebView()->mainFrame();
I think the "opener" is equally correctly just
context()->GetRenderFrame()->GetWebFrame(). No need to dig into the RenderView.

[Devlin, 2015-07-08 21:08:47]

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
File extensions/browser/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
extensions/browser/api/app_window/app_window_api.cc:181:
created_frame->GetProcess()->GetID()) {
On 2015/07/08 20:22:19, kalman wrote:
> Why does the API do this? Why not unconditionally set the routing ID? Same as
> below.

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.

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/app_...
File extensions/browser/app_window/app_window_registry.cc (right):

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/app_...
extensions/browser/app_window/app_window_registry.cc:221: return std::string(); 
// Not a AppWindow.
On 2015/07/08 20:22:20, kalman wrote:
> an AppWindow

vim yank apparently doesn't also grammar check :P

https://codereview.chromium.org/1211003006/diff/20001/extensions/renderer/app...
File extensions/renderer/app_window_custom_bindings.cc (right):

https://codereview.chromium.org/1211003006/diff/20001/extensions/renderer/app...
extensions/renderer/app_window_custom_bindings.cc:31: Dispatcher* dispatcher)
On 2015/07/08 20:22:20, kalman wrote:
> Can you change this to just pass in the ScriptContextSet, not the whole
> Dispatcher? That way you can also remove the Dispatcher dependency entirely.
You
> only need the ScriptContextSet.

Sure, done.

https://codereview.chromium.org/1211003006/diff/20001/extensions/renderer/app...
extensions/renderer/app_window_custom_bindings.cc:93: new
DidCreateDocumentElementObserver(app_frame, dispatcher_);
On 2015/07/08 20:22:20, kalman wrote:
> I feel like this shouldn't happen until we know for sure the window is
actually
> returned to the caller. Unnecessary/wasteful otherwise.

Done.

https://codereview.chromium.org/1211003006/diff/20001/extensions/renderer/app...
extensions/renderer/app_window_custom_bindings.cc:104: blink::WebFrame* opener =
context_render_view->GetWebView()->mainFrame();
On 2015/07/08 20:22:20, kalman wrote:
> I think the "opener" is equally correctly just
> context()->GetRenderFrame()->GetWebFrame(). No need to dig into the
RenderView.

It is potentially a behavior change.  But let's try it and see what happens.

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

lgtm

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
File extensions/browser/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
extensions/browser/api/app_window/app_window_api.cc:177:
content::RenderFrameHost* created_frame =
"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".

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
extensions/browser/api/app_window/app_window_api.cc:181:
created_frame->GetProcess()->GetID()) {
On 2015/07/08 21:08:46, Devlin wrote:
> On 2015/07/08 20:22:19, kalman wrote:
> > Why does the API do this? Why not unconditionally set the routing ID? Same
as
> > below.
> 
> 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.

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]

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
File extensions/browser/api/app_window/app_window_api.cc (right):

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
extensions/browser/api/app_window/app_window_api.cc:177:
content::RenderFrameHost* created_frame =
On 2015/07/08 21:34:29, kalman wrote:
> "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". 

Done.

https://codereview.chromium.org/1211003006/diff/20001/extensions/browser/api/...
extensions/browser/api/app_window/app_window_api.cc:181:
created_frame->GetProcess()->GetID()) {
On 2015/07/08 21:34:29, kalman wrote:
> On 2015/07/08 21:08:46, Devlin wrote:
> > On 2015/07/08 20:22:19, kalman wrote:
> > > Why does the API do this? Why not unconditionally set the routing ID? Same
> as
> > > below.
> > 
> > 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.
> 
> 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.

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).

[Devlin, 2015-07-08 21:48:28]

rdevlin.cronin@chromium.org changed reviewers:
+ creis@chromium.org

[Devlin, 2015-07-08 21:48:29]

+Charlie for content/

[Charlie Reis, 2015-07-08 22:52:18]

content/ LGTM with nits.

https://codereview.chromium.org/1211003006/diff/60001/content/public/renderer...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1211003006/diff/60001/content/public/renderer...
content/public/renderer/render_frame.h:59: static RenderFrame*
FromRoutingID(int32 routing_id);
Is there a reason to explicitly use int32 here rather than int?  We use int in
both RenderView::FromRoutingID and RenderFrameImpl::FromRoutingID, as well as
GetRoutingID below.

https://codereview.chromium.org/1211003006/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1211003006/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:535: RenderFrame*
RenderFrame::FromRoutingID(int32 routing_id) {
nit: int

(The one below should be int as well to match the .h file.)

[Devlin, 2015-07-08 23:55:04]

Patchset #5 (id:100001) has been deleted

[Devlin, 2015-07-09 17:05:42]

rdevlin.cronin@chromium.org changed reviewers:
+ dbeam@chromium.org

[Devlin, 2015-07-09 17:05:47]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-07-09 17:05:47]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1211003006/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-07-09 17:06:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1211003006/120001

[commit-bot: I haz the power, 2015-07-09 17:37:06]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-07-09 17:39:12]

Patchset 5 (id:??) landed as
https://crrev.com/c3d6ba1b13ec1f050178e4f97c121b528c07520d
Cr-Commit-Position: refs/heads/master@{#338076}

[Devlin, 2015-07-09 20:09:11]

Whoops, forgot to mention the nits were done.

https://codereview.chromium.org/1211003006/diff/60001/content/public/renderer...
File content/public/renderer/render_frame.h (right):

https://codereview.chromium.org/1211003006/diff/60001/content/public/renderer...
content/public/renderer/render_frame.h:59: static RenderFrame*
FromRoutingID(int32 routing_id);
On 2015/07/08 22:52:18, Charlie Reis wrote:
> Is there a reason to explicitly use int32 here rather than int?  We use int in
> both RenderView::FromRoutingID and RenderFrameImpl::FromRoutingID, as well as
> GetRoutingID below.

Apparently I copied the one place it was int32 (the .cc file). Changed.

https://codereview.chromium.org/1211003006/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1211003006/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:535: RenderFrame*
RenderFrame::FromRoutingID(int32 routing_id) {
On 2015/07/08 22:52:18, Charlie Reis wrote:
> nit: int
> 
> (The one below should be int as well to match the .h file.)

Done.