Save password bubble pops up automatically

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

Index: chrome/browser/ui/views/location_bar/location_bar_view.cc
diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc
index 4b2dbd2e88f0ae63b660051489ca93acfe3f9197..1a70f5ee7c5a82174ace096ac200dab131aea2f4 100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -529,6 +529,7 @@ void LocationBarView::UpdateContentSettingsIcons() {
   RefreshContentSettingViews();
 
   Layout();
+  UpdatePostLayout();
   SchedulePaint();
 }
 
@@ -1137,7 +1138,15 @@ int LocationBarView::GetHorizontalEdgeThickness() const {
 void LocationBarView::RefreshContentSettingViews() {

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

I realize you said that only one place can make the Save Password icon start
showing.  But it's not obvious to me why it's technically impossible for that to
happen at any other callsite of this function.  It's also not clear to me that,
even if that holds, we won't make another change in the future to add an
UpdatePostLayout() action for some other content setting which _can_ change at
another callsite.

So I'm still very worried about how this function can be called from multiple
places, but neither it nor all callers ensure UpdatePostLayout() gets called. 
This either needs fixing, or a lot more commentary about why it's not only
correct now but guaranteed to hold in the future.

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

In the other place where RefreshCOntentSettingViews() is called, we do not call
layout as far as I can see. So it makes little sense to call UpdatePostLayout,
if layouting doesn't happen. Am I wrong?

[npentrel, 2013/09/04 11:27:31]

Never mind, it is called. I added the call to UpdatePostLayout.

   for (ContentSettingViews::const_iterator i(content_setting_views_.begin());
        i != content_setting_views_.end(); ++i) {
-    (*i)->Update(GetToolbarModel()->input_in_progress() ?
+    (*i)->UpdatePreLayout(GetToolbarModel()->input_in_progress() ?
+        NULL : GetWebContents());
+  }
+}
+
+void LocationBarView::UpdatePostLayout() {

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

I think calling this UpdatePostLayout() is somewhat misleading.

Since it's only called in one place, maybe we should just avoid the problem of
finding a good name and inline it into the caller.

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

Done.

+  for (ContentSettingViews::const_iterator i(content_setting_views_.begin());
+       i != content_setting_views_.end(); ++i) {
+    (*i)->UpdatePostLayout(GetToolbarModel()->input_in_progress() ?
         NULL : GetWebContents());
   }
 }