Handle about:blank in the Origin Info Bubble better.

chrome/browser/ui/views/location_bar/location_icon_view.cc

Index: chrome/browser/ui/views/location_bar/location_icon_view.cc
diff --git a/chrome/browser/ui/views/location_bar/location_icon_view.cc b/chrome/browser/ui/views/location_bar/location_icon_view.cc
index f1b4fc8d396218d5b32f8973a8dcc5f903ba2775..1112783efd28cee526cc8b0fc06b44a77cdad0fb 100644
--- a/chrome/browser/ui/views/location_bar/location_icon_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_icon_view.cc
@@ -12,6 +12,7 @@
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/components_scaled_resources.h"
 #include "grit/theme_resources.h"
@@ -20,6 +21,7 @@
 #include "ui/gfx/color_palette.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/painter.h"
+#include "url/origin.h"
 
 using content::NavigationController;
 using content::NavigationEntry;
@@ -34,15 +36,32 @@ void ProcessEventInternal(LocationBarView* view) {
 
   // Important to use GetVisibleEntry to match what's showing in the omnibox.
   NavigationEntry* entry = contents->GetController().GetVisibleEntry();
-  // The visible entry can be nullptr in the case of window.open("").
-  if (!entry)
-    return;
+  GURL url;
+  if (entry) {
+    if (entry->GetURL().SchemeIs(url::kAboutScheme)) {

[Charlie Reis, 2016/04/18 19:58:38]

I'd recommend comparing against kAboutBlankURL (if we keep this check).

[palmer, 2016/04/18 23:37:35]

Acknowledged.

+      const WebContents* opener = contents->GetOpener();

[Charlie Reis, 2016/04/18 19:58:38]

I tried to point out in the doc that opener is not the right thing to look at to
determine origin.  There should be no calls to GetOpener() here.

I'm guessing this code should look something like:

// Show the origin if it's not unique, and otherwise fall back to the URL.
Origin origin = entry ? entry->GetOrigin() : ...GetLastCommittedOrigin();
GURL url = GURL(origin.Serialize());
if (origin.unique() && entry)
  url = entry->GetURL();

[palmer, 2016/04/18 23:37:35]

I'm sorry; I misunderstood you as saying there might not be an opener and we
have to do something else if not. Anyway:
This resulted in showing "about:blank" for a tab that results from calling
window.open("about:blank") from a google.com page. (It showed "google.com" for a
tab that resulted from calling window.open(""), however.)

I currently have:

  url::Origin origin = entry
                           ? entry->GetOrigin()
                           : contents->GetMainFrame()->GetLastCommittedOrigin();
  GURL url(origin.Serialize());
  if (entry && origin.unique()) {
    url = GURL(contents->GetMainFrame()->GetLastCommittedOrigin().Serialize());
  }

which works correctly (showing "google.com" for both window.open("about:blank")
and window.open(""), including after closing the google.com opener) in manual
testing. (It shows "" for when you open a fresh tab and navigate to about:blank
in the Omnibox. I have a TODO about that.)

[Charlie Reis, 2016/04/19 22:32:52]

This is probably due to the bug I mentioned in your NavigationControllerImpl
change, since we're setting the last committed entry's origin to the URL, not to
the effective origin.

Can you switch back to the version I suggested if that's fixed?
Yep, we'll want to show about:blank there.  Maybe something like:

if (origin.unique() {
  if (entry)
    url = entry->GetURL();
  else
    url = GURL(kAboutBlankURL);
}

+      if (opener) {
+        entry = opener->GetController().GetVisibleEntry();
+      } else {
+        // TODO(palmer): Should we show the "this is a secure Chromium page"
+        // OIB?
+      }
+      url = GURL(entry->GetOrigin().Serialize());
+    } else {
+      url = GURL(entry->GetOrigin().Serialize());

[Charlie Reis, 2016/04/18 19:58:38]

Why do this line on both sides of the branch?

I could see using entry->GetURL() here, given that we know the URL isn't
about:blank.  Or we could use GetOrigin() in both cases (outside the
conditional), if we trust it to be correct in all cases.

[palmer, 2016/04/18 23:37:35]

Acknowledged.

+    }
+  } else {
+    // The visible entry can be nullptr in the case of window.open("").
+    url = GURL(contents->GetMainFrame()->GetLastCommittedOrigin().Serialize());

[Charlie Reis, 2016/04/18 19:58:38]

Alex, is there a good way to do this, or should this be blocked on moving
ShowWebsiteSettings to an Origin rather than a URL?  I'm concerned about cases
where we try to create a GURL on the string "null".

Maybe falling back to the URL for unique origins (as I suggest above) would be
sufficient?

[alexmos, 2016/04/19 00:04:37]

Yeah, unfortunately this is the best you can do right now, and ideally we'd move
ShowWebsiteSettings to use origins instead.  If we wanted something in the
meantime, having a similar special case to fall back to the URL for unique
origins seems ok.  (Though which URL would you use here?)

The situation we want to avoid in general is constructing two GURLs like this
out of unique origins and then later on comparing them and concluding
equivalency (since both GURLs will fail validation and normalize to the empty
URL).  This is probably less of an issue here, since the url seems to be for
display purposes, though I haven't looked into how ShowWebsiteSettings works.

Random question -- would all of this work properly for file:/// URLs?

+  }
 
   ChromeSecurityStateModelClient* model_client =
       ChromeSecurityStateModelClient::FromWebContents(contents);
   DCHECK(model_client);
 
-  view->delegate()->ShowWebsiteSettings(contents, entry->GetURL(),
+  // TODO(palmer): Change ShowWebsiteSettings to take an Origin, rather than a
+  // GURL.
+  view->delegate()->ShowWebsiteSettings(contents, url,
                                         model_client->GetSecurityInfo());
 }