RenderWidget/RenderView: encapsulate ViewHostMsg_Show, etc, in a callback

android_webview/renderer/aw_content_renderer_client.cc

Index: android_webview/renderer/aw_content_renderer_client.cc
diff --git a/android_webview/renderer/aw_content_renderer_client.cc b/android_webview/renderer/aw_content_renderer_client.cc
index 20754a7156cdf43ad711b98db8ba4daffa74cafe..cf06e10423d0c120520fbc3c96d998e6d2fb54a4 100644
--- a/android_webview/renderer/aw_content_renderer_client.cc
+++ b/android_webview/renderer/aw_content_renderer_client.cc
@@ -92,7 +92,7 @@ void AwContentRendererClient::RenderThreadStarted() {
 bool AwContentRendererClient::HandleNavigation(
     content::RenderFrame* render_frame,
     bool is_content_initiated,
-    int opener_id,
+    bool render_view_was_created_with_opener,
     blink::WebFrame* frame,
     const blink::WebURLRequest& request,
     blink::WebNavigationType type,
@@ -129,7 +129,14 @@ bool AwContentRendererClient::HandleNavigation(
 
   // use NavigationInterception throttle to handle the call as that can
   // be deferred until after the java side has been constructed.

[alexmos, 2016/11/17 09:51:09]

Do you understand this comment?  I can't tell why this would imply the need for
the opener check and the return false. 

[ncarter (slow), 2016/11/17 19:33:11]

I interpret this comment to mean: "when a new WebContents is created, there's a
built-in chance to hook its initial navigation on the java/browser side, e.g.
because such creation goes through AwWebContentsDelegate.java or something
similar." I don't know what java code is actually being referenced.

This reasoning seems incorrect for subsequent navigations; my goal with these
edits was to not disturb the existing behavior, and to maybe draw attention to
this potential bug. I know there's a long term plan to eliminate this method, so
preserving weird behavior seems like a reasonable course here, so long as it's
not insecure. I get the sense that Android Webview has some pretty serious
reverse-compatibility constraints.

I am looping sgurun into this CL for the android webview perspective.

[alexmos, 2016/11/17 22:27:43]

Acknowledged.

-  if (opener_id != MSG_ROUTING_NONE) {
+  //
+  // TODO(nick): |render_view_was_created_with_opener| was plumbed in to
+  // preserve the existing code behavior, but it doesn't appear to be correct.
+  // In particular, this value will be true for the initial navigation of a
+  // RenderView created via window.open(), but it will also be true for all
+  // subsequent navigations in that RenderView, no matter how they are
+  // initiated.
+  if (render_view_was_created_with_opener) {

[alexmos, 2016/11/17 09:51:09]

You could also name this render_view_was_created_by_renderer.  I'm split about
which is better -- the created_by_renderer conveys its actual meaning better
(now that opener_id_ is gone) but the current name better matches old behavior. 
(Then again, I don't understand the reason for that behavior and the comment
above).

[ncarter (slow), 2016/11/17 19:33:11]

render_view_was_created_by_renderer is a better more consistent name. Done.

     return false;
   }