Save password bubble pops up automatically

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/command_line.h"
@@ skipping 511 matching lines @@
   else
     location_entry_->Update();
 
   OnChanged();
 }
 
 void LocationBarView::UpdateContentSettingsIcons() {
   RefreshContentSettingViews();
 
   Layout();
+  UpdatePostLayout();
   SchedulePaint();
 }
 
 void LocationBarView::UpdatePageActions() {
   size_t count_before = page_action_views_.size();
   RefreshPageActionViews();
   RefreshScriptBubble();
   if (page_action_views_.size() != count_before) {
     content::NotificationService::current()->Notify(
         chrome::NOTIFICATION_EXTENSION_PAGE_ACTION_COUNT_CHANGED,
@@ skipping 585 matching lines @@
   return (ui::GetDisplayLayout() == ui::LAYOUT_TOUCH) ?
       GetItemPadding() / 2 : 0;
 }
 
 int LocationBarView::GetHorizontalEdgeThickness() const {
   // In maximized popup mode, there isn't any edge.
   return (is_popup_mode_ && browser_ && browser_->window() &&
       browser_->window()->IsMaximized()) ? 0 : vertical_edge_thickness();
 }
 
 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());
+  }
+}
 
 void LocationBarView::DeletePageActionViews() {
   for (PageActionViews::const_iterator i(page_action_views_.begin());
        i != page_action_views_.end(); ++i)
     RemoveChildView(*i);
   STLDeleteElements(&page_action_views_);
 }
 
 void LocationBarView::RefreshPageActionViews() {
   if (is_popup_mode_)
@@ skipping 399 matching lines @@
 int LocationBarView::GetInternalHeight(bool use_preferred_size) {
   int total_height =
       use_preferred_size ? GetPreferredSize().height() : height();
   return std::max(total_height - (vertical_edge_thickness() * 2), 0);
 }
 
 bool LocationBarView::HasValidSuggestText() const {
   return suggested_text_view_->visible() &&
       !suggested_text_view_->size().IsEmpty();
 }