Save password bubble pops up automatically

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

Index: chrome/browser/ui/views/location_bar/content_setting_image_view.cc
diff --git a/chrome/browser/ui/views/location_bar/content_setting_image_view.cc b/chrome/browser/ui/views/location_bar/content_setting_image_view.cc
index 6d05e4fa6a7a3ec83dd6026879005839266391e2..2e8ed99ed67e19a89a59d12f809a925d3addbcc9 100644
--- a/chrome/browser/ui/views/location_bar/content_setting_image_view.cc
+++ b/chrome/browser/ui/views/location_bar/content_setting_image_view.cc
@@ -94,7 +94,38 @@ ContentSettingImageView::~ContentSettingImageView() {
     bubble_widget_->RemoveObserver(this);
 }
 
-void ContentSettingImageView::Update(content::WebContents* web_contents) {
+void ContentSettingImageView::UpdatePostLayout(

[Peter Kasting, 2013/09/03 23:50:43]

Function definition order needs to match declaration order.

[npentrel, 2013/09/04 09:55:36]

Done.

+    content::WebContents* web_contents) {
+  if (!content_setting_image_model_->is_visible()) {

[Peter Kasting, 2013/09/03 23:50:43]

Nit: No {}

[npentrel, 2013/09/04 09:55:36]

Done.

+    return;
+  }
+
+  TabSpecificContentSettings* content_settings = web_contents ?

[Peter Kasting, 2013/09/03 23:50:43]

Nit: This should be done inside the below conditional

[npentrel, 2013/09/04 09:55:36]

Done.

+      TabSpecificContentSettings::FromWebContents(web_contents) : NULL;
+
+  if (content_setting_image_model_->get_content_settings_type() ==
+      CONTENT_SETTINGS_TYPE_SAVE_PASSWORD) {

[Peter Kasting, 2013/09/03 23:50:43]

It would be nice if this was more generic than "is this a save password icon"...
perhaps some kind of "should auto-show bubble on blockage" method that the save
password model overrides to return true.

Nit: Maybe reverse this and early return (but see comment in next function)

[npentrel, 2013/09/04 09:55:36]

Done.

+    if (!content_settings->IsBlockageIndicated(
+             content_setting_image_model_->get_content_settings_type())) {
+      if (web_contents && !bubble_widget_) {
+        bubble_widget_ = parent_->delegate()->CreateViewsBubble(
+            new ContentSettingBubbleContents(
+                ContentSettingBubbleModel::CreateContentSettingBubbleModel(
+                    parent_->delegate()->GetContentSettingBubbleModelDelegate(),
+                    web_contents, parent_->profile(),
+                    content_setting_image_model_->get_content_settings_type()),
+                web_contents, this, views::BubbleBorder::TOP_RIGHT));
+        bubble_widget_->AddObserver(this);
+        bubble_widget_->Show();
+      }
+      content_settings->SetBlockageHasBeenIndicated(
+          content_setting_image_model_->get_content_settings_type());
+    }
+  }
+}
+
+void ContentSettingImageView::UpdatePreLayout(
+    content::WebContents* web_contents) {
   // Note: We explicitly want to call this even if |web_contents| is NULL, so we
   // get hidden properly while the user is editing the omnibox.
   content_setting_image_model_->UpdateFromWebContents(web_contents);
@@ -128,8 +159,13 @@ void ContentSettingImageView::Update(content::WebContents* web_contents) {
     slide_animator_.Show();
   }
 
-  content_settings->SetBlockageHasBeenIndicated(
-      content_setting_image_model_->get_content_settings_type());
+  // The blockage for |CONTENT_SETTINGS_TYPE_SAVE_PASSWORD| is set once the
+  // bubble pops up.

[Peter Kasting, 2013/09/03 23:50:43]

Nit: Say why, not what.  This comment currently just restates the code; instead
talk about why this needs to be, i.e. that the bubble needs to be anchored by
the laid-out icon, which isn't positioned correctly until after this returns.

[npentrel, 2013/09/04 09:55:36]

Done.

+  if (content_setting_image_model_->get_content_settings_type() !=
+      CONTENT_SETTINGS_TYPE_SAVE_PASSWORD) {
+    content_settings->SetBlockageHasBeenIndicated(
+        content_setting_image_model_->get_content_settings_type());

[Peter Kasting, 2013/09/03 23:50:43]

I wonder if for consistency's sake, we should move this out of this function
entirely and make the post-layout function more like:

if (should show bubble)
  show bubble;
set blockage has been indicated;

This way, we'd always call the "set indicated" method at the same time in the
call chain.  This might reduce the chance of weird timing bugs later?

[npentrel, 2013/09/04 09:55:36]

Done.

+  }
 }
 
 // static