Implement put_accvalue for textfields and location bar.

Implement put_accvalue for textfields and location bar.

All interactive views implement GetAccessibleState
to provide accessibility information about the current
state of the view. This struct now includes a callback
that can be used to change the string value, since
automation software wants to fill in text fields,
and this interface is implemented for Textfield
and LocationBarView, and hooked up to put_accValue.

BUG=260266
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221688

[msw, 2013-08-31 20:22:36]

Thank you so much for addressing this! I have some minor comments.

https://codereview.chromium.org/23441024/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/23441024/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1305:
location_entry_->SelectAll(false);
This does not clear the selection as the AccessibilitySetValue function comment
claims. This selects all the text; false simply means that the select-all
selection will not be reversed. I believe the OmniboxView::SetUserText call
above is sufficient and will do the right thing on its own, since it's
implementation calls SetWindowTextAndCaretPos with |caret_pos| at the length of
the input, |display_text.length()|.

https://codereview.chromium.org/23441024/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1351:
&LocationBarView::AccessibilitySetValue, base::Unretained(this));
base::Unretained is probably not correct here, I think you might be able to
construct a WeakPtrFactory for |location_entry_| and pass a binding to its
OmniboxView::SetUserText, since that's stored as a scoped_ptr by this class. I'm
sure other similar options are possible if that's not ideal.

https://codereview.chromium.org/23441024/diff/1/ui/base/accessibility/accessi...
File ui/base/accessibility/accessible_view_state.h (right):

https://codereview.chromium.org/23441024/diff/1/ui/base/accessibility/accessi...
ui/base/accessibility/accessible_view_state.h:65: // This callback is not
expected to persist beyond the scope of the
I think it'd be better to try to use WeakPtr than base::Unretained. Then revise
this notice to explain that the Callback is only valid for the lifetime of the
associated view, and will simply/safely no-op otherwise.

https://codereview.chromium.org/23441024/diff/1/ui/views/accessibility/native...
File ui/views/accessibility/native_view_accessibility_win.cc (right):

https://codereview.chromium.org/23441024/diff/1/ui/views/accessibility/native...
ui/views/accessibility/native_view_accessibility_win.cc:611: VARIANT var_id,
BSTR new_value) {
nit: these arguments should be one-per line as-per our style guide.

https://codereview.chromium.org/23441024/diff/1/ui/views/accessibility/native...
File ui/views/accessibility/native_view_accessibility_win.h (right):

https://codereview.chromium.org/23441024/diff/1/ui/views/accessibility/native...
ui/views/accessibility/native_view_accessibility_win.h:120: // Set the value of
the control. Not used by screen readers, but used
optional nit: put this and get_accValue together with a combined comment.

https://codereview.chromium.org/23441024/diff/1/ui/views/controls/textfield/t...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/23441024/diff/1/ui/views/controls/textfield/t...
ui/views/controls/textfield/textfield.cc:510: state->set_value_callback =
base::Bind(
nit: wrap the line after '='.

https://codereview.chromium.org/23441024/diff/1/ui/views/controls/textfield/t...
ui/views/controls/textfield/textfield.cc:511: &Textfield::AccessibilitySetValue,
base::Unretained(this));
Ditto; avoid base::Unretained with a new Textfield WeakPtrFactory or similar.

[msw, 2013-08-31 20:23:21]

Also, unit test(s) and a more complete CL description would be awesome.

[dmazzoni, 2013-09-03 20:20:11]

https://codereview.chromium.org/23441024/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/23441024/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1305:
location_entry_->SelectAll(false);
On 2013/08/31 20:22:36, msw wrote:
> This does not clear the selection as the AccessibilitySetValue function
comment
> claims. This selects all the text; false simply means that the select-all
> selection will not be reversed. I believe the OmniboxView::SetUserText call
> above is sufficient and will do the right thing on its own, since it's
> implementation calls SetWindowTextAndCaretPos with |caret_pos| at the length
of
> the input, |display_text.length()|.

Done.

https://codereview.chromium.org/23441024/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1351:
&LocationBarView::AccessibilitySetValue, base::Unretained(this));
On 2013/08/31 20:22:36, msw wrote:
> base::Unretained is probably not correct here, I think you might be able to
> construct a WeakPtrFactory for |location_entry_| and pass a binding to its
> OmniboxView::SetUserText, since that's stored as a scoped_ptr by this class.
I'm
> sure other similar options are possible if that's not ideal.

I don't think we need to bind to location_entry_, right? It
doesn't look like it will ever get cleared. I switched to a
WeakPtr bound to this object, that should work fine.

https://codereview.chromium.org/23441024/diff/1/ui/base/accessibility/accessi...
File ui/base/accessibility/accessible_view_state.h (right):

https://codereview.chromium.org/23441024/diff/1/ui/base/accessibility/accessi...
ui/base/accessibility/accessible_view_state.h:65: // This callback is not
expected to persist beyond the scope of the
On 2013/08/31 20:22:36, msw wrote:
> I think it'd be better to try to use WeakPtr than base::Unretained. Then
revise
> this notice to explain that the Callback is only valid for the lifetime of the
> associated view, and will simply/safely no-op otherwise.

Done.

https://codereview.chromium.org/23441024/diff/1/ui/views/accessibility/native...
File ui/views/accessibility/native_view_accessibility_win.cc (right):

https://codereview.chromium.org/23441024/diff/1/ui/views/accessibility/native...
ui/views/accessibility/native_view_accessibility_win.cc:611: VARIANT var_id,
BSTR new_value) {
On 2013/08/31 20:22:36, msw wrote:
> nit: these arguments should be one-per line as-per our style guide.

Done.

https://codereview.chromium.org/23441024/diff/1/ui/views/accessibility/native...
File ui/views/accessibility/native_view_accessibility_win.h (right):

https://codereview.chromium.org/23441024/diff/1/ui/views/accessibility/native...
ui/views/accessibility/native_view_accessibility_win.h:120: // Set the value of
the control. Not used by screen readers, but used
On 2013/08/31 20:22:36, msw wrote:
> optional nit: put this and get_accValue together with a combined comment.

Done.

https://codereview.chromium.org/23441024/diff/1/ui/views/controls/textfield/t...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/23441024/diff/1/ui/views/controls/textfield/t...
ui/views/controls/textfield/textfield.cc:510: state->set_value_callback =
base::Bind(
On 2013/08/31 20:22:36, msw wrote:
> nit: wrap the line after '='.

Done.

[msw, 2013-09-03 21:06:57]

Great unit test! Just one suggestion, but I'm open to your thoughts on
WeakPtrFactory vs. SupportsWeakPtr.

https://codereview.chromium.org/23441024/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/23441024/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1351:
&LocationBarView::AccessibilitySetValue, base::Unretained(this));
On 2013/09/03 20:20:11, Dominic Mazzoni wrote:
> On 2013/08/31 20:22:36, msw wrote:
> > base::Unretained is probably not correct here, I think you might be able to
> > construct a WeakPtrFactory for |location_entry_| and pass a binding to its
> > OmniboxView::SetUserText, since that's stored as a scoped_ptr by this class.
> I'm
> > sure other similar options are possible if that's not ideal.
> 
> I don't think we need to bind to location_entry_, right? It
> doesn't look like it will ever get cleared. I switched to a
> WeakPtr bound to this object, that should work fine.

Binding to a LocationBarView WeakPtr is just as good.

https://codereview.chromium.org/23441024/diff/12001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/23441024/diff/12001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.h:81: public
base::SupportsWeakPtr<LocationBarView> {
I'd suggest using a WeakPtrFactory instead of SupportsWeakPtr, which mentions
that since it's "destructor won't invalidate weak pointers to the class until
after the derived class' members have been destroyed, its use can lead to subtle
use-after-destroy issues." WeakPtrFactory uses composition instead of
inheritance, and you can optionally call InvalidateWeakPtrs in the
LocationBarView/Textfield dtor right off the bat to avoid use-after-destroy.

[dmazzoni, 2013-09-04 06:48:03]

+sky for OWNERS review

https://codereview.chromium.org/23441024/diff/12001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/23441024/diff/12001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.h:81: public
base::SupportsWeakPtr<LocationBarView> {
On 2013/09/03 21:06:57, msw wrote:
> I'd suggest using a WeakPtrFactory instead of SupportsWeakPtr, which mentions
> that since it's "destructor won't invalidate weak pointers to the class until
> after the derived class' members have been destroyed, its use can lead to
subtle
> use-after-destroy issues." WeakPtrFactory uses composition instead of
> inheritance, and you can optionally call InvalidateWeakPtrs in the
> LocationBarView/Textfield dtor right off the bat to avoid use-after-destroy.

Sure. Switched to WeakPtrFactory.

[sky, 2013-09-04 16:20:12]

https://codereview.chromium.org/23441024/diff/24001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/23441024/diff/24001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:227:
weak_ptr_factory_.InvalidateWeakPtrs();
Is there a reason you're explicitly invoking this?

https://codereview.chromium.org/23441024/diff/24001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1305: void
LocationBarView::AccessibilitySetValue(const string16& new_value) {
Make position match header.

https://codereview.chromium.org/23441024/diff/24001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1306:
location_entry_->SetUserText(new_value);
Does this need to do nothing if is_popup_mode_, or maybe the callback shouldn't
be set in that case?

https://codereview.chromium.org/23441024/diff/24001/ui/views/controls/textfie...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/23441024/diff/24001/ui/views/controls/textfie...
ui/views/controls/textfield/textfield.cc:563: void
Textfield::AccessibilitySetValue(const string16& new_value) {
Make position match header.

https://codereview.chromium.org/23441024/diff/24001/ui/views/controls/textfie...
ui/views/controls/textfield/textfield.cc:564: SetText(new_value);
Doesn't this need to check readonly?

[dmazzoni, 2013-09-04 17:16:16]

https://codereview.chromium.org/23441024/diff/24001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/23441024/diff/24001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:227:
weak_ptr_factory_.InvalidateWeakPtrs();
On 2013/09/04 16:20:12, sky wrote:
> Is there a reason you're explicitly invoking this?

It's sometimes safer, as they might not be invalidated
while an object is being deleted - but in this case I
don't think it helps, I'll remove it.

https://codereview.chromium.org/23441024/diff/24001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1305: void
LocationBarView::AccessibilitySetValue(const string16& new_value) {
On 2013/09/04 16:20:12, sky wrote:
> Make position match header.

The order was all over the place. I moved all of the private
member function implementations to the correct place and 
correct order.

https://codereview.chromium.org/23441024/diff/24001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1306:
location_entry_->SetUserText(new_value);
On 2013/09/04 16:20:12, sky wrote:
> Does this need to do nothing if is_popup_mode_, or maybe the callback
shouldn't
> be set in that case?

Good idea, I'll set the readonly flag if is_popup_mode is true, and not set the
callback.

https://codereview.chromium.org/23441024/diff/24001/ui/views/controls/textfie...
File ui/views/controls/textfield/textfield.cc (right):

https://codereview.chromium.org/23441024/diff/24001/ui/views/controls/textfie...
ui/views/controls/textfield/textfield.cc:563: void
Textfield::AccessibilitySetValue(const string16& new_value) {
On 2013/09/04 16:20:12, sky wrote:
> Make position match header.

I think it is in the right place - I added a section header.

https://codereview.chromium.org/23441024/diff/24001/ui/views/controls/textfie...
ui/views/controls/textfield/textfield.cc:564: SetText(new_value);
On 2013/09/04 16:20:12, sky wrote:
> Doesn't this need to check readonly?

Done. I also had it not set the callback, above.

[sky, 2013-09-04 20:11:45]

LGTM

[msw, 2013-09-04 20:18:20]

LGTM with a couple minor nits.

https://codereview.chromium.org/23441024/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/23441024/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1384: //////
nit: finish the slash comment art or remove it.

https://codereview.chromium.org/23441024/diff/35001/ui/views/controls/textfie...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/23441024/diff/35001/ui/views/controls/textfie...
ui/views/controls/textfield/textfield.h:45: class VIEWS_EXPORT Textfield
nit: you don't need to split this line anymore.

[commit-bot: I haz the power, 2013-09-04 20:18:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23441024/35001

[dmazzoni, 2013-09-04 22:01:08]

https://codereview.chromium.org/23441024/diff/35001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/23441024/diff/35001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1384: //////
On 2013/09/04 20:18:20, msw wrote:
> nit: finish the slash comment art or remove it.

Done.

https://codereview.chromium.org/23441024/diff/35001/ui/views/controls/textfie...
File ui/views/controls/textfield/textfield.h (right):

https://codereview.chromium.org/23441024/diff/35001/ui/views/controls/textfie...
ui/views/controls/textfield/textfield.h:45: class VIEWS_EXPORT Textfield
On 2013/09/04 20:18:20, msw wrote:
> nit: you don't need to split this line anymore.

Done.

[commit-bot: I haz the power, 2013-09-04 22:03:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23441024/53001

[commit-bot: I haz the power, 2013-09-04 23:01:46]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-05 15:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23441024/66001

[commit-bot: I haz the power, 2013-09-05 15:58:11]

Failed to apply patch for
chrome/browser/ui/views/location_bar/location_bar_view.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/views/location_bar/location_bar_view.cc
  Hunk #2 succeeded at 635 (offset -1 lines).
  Hunk #3 FAILED at 1112.
  Hunk #4 succeeded at 1329 (offset 7 lines).
  Hunk #5 succeeded at 1341 (offset 7 lines).
  Hunk #6 succeeded at 1564 (offset 7 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/location_bar/location_bar_view.cc.rej

Patch:       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..c385772ea8d29fe02fc2dd32a02c3f513169cba6
100644
--- a/chrome/browser/ui/views/location_bar/location_bar_view.cc
+++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc
@@ -190,7 +190,8 @@ LocationBarView::LocationBarView(Browser* browser,
       is_popup_mode_(is_popup_mode),
       show_focus_rect_(false),
       template_url_service_(NULL),
-      animation_offset_(0) {
+      animation_offset_(0),
+      weak_ptr_factory_(this) {
   if (!views::Textfield::IsViewsTextfieldEnabled())
     set_id(VIEW_ID_OMNIBOX);
 
@@ -635,17 +636,6 @@ void LocationBarView::ZoomChangedForActiveTab(bool
can_show_bubble) {
     ZoomBubbleView::ShowBubble(delegate_->GetWebContents(), true);
 }
 
-void LocationBarView::RefreshZoomView() {
-  DCHECK(zoom_view_);
-  WebContents* web_contents = GetWebContents();
-  if (!web_contents)
-    return;
-
-  ZoomController* zoom_controller =
-      ZoomController::FromWebContents(web_contents);
-  zoom_view_->Update(zoom_controller);
-}
-
 gfx::Point LocationBarView::GetLocationEntryOrigin() const {
   gfx::Point origin(location_entry_view_->bounds().origin());
   // If the UI layout is RTL, the coordinate system is not transformed and
@@ -1122,184 +1112,6 @@ const ToolbarModel* LocationBarView::GetToolbarModel()
const {
   return delegate_->GetToolbarModel();
 }
 
-// static
-int LocationBarView::GetBuiltInHorizontalPaddingForChildViews() {
-  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() {
-  for (ContentSettingViews::const_iterator i(content_setting_views_.begin());
-       i != content_setting_views_.end(); ++i) {
-    (*i)->Update(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_)
-    return;
-
-  // Remember the previous visibility of the page actions so that we can
-  // notify when this changes.
-  std::map<ExtensionAction*, bool> old_visibility;
-  for (PageActionViews::const_iterator i(page_action_views_.begin());
-       i != page_action_views_.end(); ++i) {
-    old_visibility[(*i)->image_view()->page_action()] = (*i)->visible();
-  }
-
-  std::vector<ExtensionAction*> new_page_actions;
-
-  WebContents* contents = delegate_->GetWebContents();
-  if (contents) {
-    extensions::TabHelper* extensions_tab_helper =
-        extensions::TabHelper::FromWebContents(contents);
-    extensions::LocationBarController* controller =
-        extensions_tab_helper->location_bar_controller();
-    new_page_actions = controller->GetCurrentActions();
-  }
-
-  // On startup we sometimes haven't loaded any extensions. This makes sure
-  // we catch up when the extensions (and any page actions) load.
-  if (page_actions_ != new_page_actions) {
-    page_actions_.swap(new_page_actions);
-    DeletePageActionViews();  // Delete the old views (if any).
-
-    page_action_views_.resize(page_actions_.size());
-    View* right_anchor = open_pdf_in_reader_view_;
-    if (!right_anchor)
-      right_anchor = star_view_;
-    if (!right_anchor)
-      right_anchor = script_bubble_icon_view_;
-    DCHECK(right_anchor);
-
-    // Add the page actions in reverse order, so that the child views are
-    // inserted in left-to-right order for accessibility.
-    for (int i = page_actions_.size() - 1; i >= 0; --i) {
-      page_action_views_[i] = new PageActionWithBadgeView(
-          delegate_->CreatePageActionImageView(this, page_actions_[i]));
-      page_action_views_[i]->SetVisible(false);
-      AddChildViewAt(page_action_views_[i], GetIndexOf(right_anchor));
-    }
-  }
-
-  if (!page_action_views_.empty() && contents) {
-    Browser* browser = chrome::FindBrowserWithWebContents(contents);
-    GURL url = browser->tab_strip_model()->GetActiveWebContents()->GetURL();
-
-    for (PageActionViews::const_iterator i(page_action_views_.begin());
-         i != page_action_views_.end(); ++i) {
-      (*i)->UpdateVisibility(
-          GetToolbarModel()->input_in_progress() ? NULL : contents, url);
-
-      // Check if the visibility of the action changed and notify if it did.
-      ExtensionAction* action = (*i)->image_view()->page_action();
-      if (old_visibility.find(action) == old_visibility.end() ||
-          old_visibility[action] != (*i)->visible()) {
-        content::NotificationService::current()->Notify(
-            chrome::NOTIFICATION_EXTENSION_PAGE_ACTION_VISIBILITY_CHANGED,
-            content::Source<ExtensionAction>(action),
-            content::Details<WebContents>(contents));
-      }
-    }
-  }
-}
-
-size_t LocationBarView::ScriptBubbleScriptsRunning() {
-  WebContents* contents = delegate_->GetWebContents();
-  if (!contents)
-    return false;
-  extensions::TabHelper* extensions_tab_helper =
-      extensions::TabHelper::FromWebContents(contents);
-  if (!extensions_tab_helper)
-    return false;
-  extensions::ScriptBubbleController* script_bubble_controller =
-      extensions_tab_helper->script_bubble_controller();
-  if (!script_bubble_controller)
-    return false;
-  size_t script_count =
-      script_bubble_controller->extensions_running_scripts().size();
-  return script_count;
-}
-
-void LocationBarView::RefreshScriptBubble() {
-  if (!script_bubble_icon_view_)
-    return;
-  size_t script_count = ScriptBubbleScriptsRunning();
-  script_bubble_icon_view_->SetVisible(script_count > 0);
-  if (script_count > 0)
-    script_bubble_icon_view_->SetScriptCount(script_count);
-}
-
-#if defined(OS_WIN) && !defined(USE_AURA)
-void LocationBarView::OnMouseEvent(const ui::MouseEvent& event, UINT msg) {
-  OmniboxViewWin* omnibox_win = GetOmniboxViewWin(location_entry_.get());
-  if (omnibox_win) {
-    UINT flags = event.native_event().wParam;
-    gfx::Point screen_point(event.location());
-    ConvertPointToScreen(this, &screen_point);
-    omnibox_win->HandleExternalMsg(msg, flags, screen_point.ToPOINT());
-  }
-}
-#endif
-
-void LocationBarView::ShowFirstRunBubbleInternal() {
-#if !defined(OS_CHROMEOS)
-  // First run bubble doesn't make sense for Chrome OS.
-  Browser* browser = GetBrowserFromDelegate(delegate_);
-  if (!browser)
-    return; // Possible when browser is shutting down.
-
-  FirstRunBubble::ShowBubble(browser, location_icon_view_);
-#endif
-}
-
-void LocationBarView::PaintPageActionBackgrounds(gfx::Canvas* canvas) {
-  WebContents* web_contents = GetWebContents();
-  // web_contents may be NULL while the browser is shutting down.
-  if (!web_contents)
-    return;
-
-  const int32 tab_id = SessionID::IdForTab(web_contents);
-  const ToolbarModel::SecurityLevel security_level =
-      GetToolbarModel()->GetSecurityLevel(false);
-  const SkColor text_color = GetColor(security_level, TEXT);
-  const SkColor background_color = GetColor(security_level, BACKGROUND);
-
-  for (PageActionViews::const_iterator
-           page_action_view = page_action_views_.begin();
-       page_action_view != page_action_views_.end();
-       ++page_action_view) {
-    gfx::Rect bounds = (*page_action_view)->bounds();
-    int horizontal_padding =
-        GetItemPadding() - GetBuiltInHorizontalPaddingForChildViews();
-    // Make the bounding rectangle include the whole vertical range of the
-    // location bar, and the mid-point pixels between adjacent page actions.
-    //
-    // For odd horizontal_paddings, "horizontal_padding + 1" includes the
-    // mid-point between two page actions in the bounding rectangle.  For even
-    // paddings, the +1 is dropped, which is right since there is no pixel at
-    // the mid-point.
-    bounds.Inset(-(horizontal_padding + 1) / 2, 0);
-    location_bar_util::PaintExtensionActionBackground(
-        *(*page_action_view)->image_view()->page_action(),
-        tab_id, canvas, bounds, text_color, background_color);
-  }
-}
-
 const char* LocationBarView::GetClassName() const {
   return kViewClassName;
 }
@@ -1332,6 +1144,9 @@ bool LocationBarView::SkipDefaultKeyEventProcessing(const
ui::KeyEvent& event) {
 }
 
 void LocationBarView::GetAccessibleState(ui::AccessibleViewState* state) {
+  if (!location_entry_)
+    return;
+
   state->role = ui::AccessibilityTypes::ROLE_LOCATION_BAR;
   state->name = l10n_util::GetStringUTF16(IDS_ACCNAME_LOCATION);
   state->value = location_entry_->GetText();
@@ -1341,6 +1156,14 @@ void
LocationBarView::GetAccessibleState(ui::AccessibleViewState* state) {
   location_entry_->GetSelectionBounds(&entry_start, &entry_end);
   state->selection_start = entry_start;
   state->selection_end = entry_end;
+
+  if (is_popup_mode_) {
+    state->st…
(message too large)

[commit-bot: I haz the power, 2013-09-05 16:23:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23441024/69001

[commit-bot: I haz the power, 2013-09-05 17:03:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-09-06 07:56:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/23441024/84001

[commit-bot: I haz the power, 2013-09-06 12:14:52]

Change committed as 221688