views: Migrate the cases of "new .*NativeTextButton(" to LabelButton.

views: Migrate the cases of "new .*NativeTextButton(" to LabelButton.

BUG=155363
R=msw@chromium.org
TBR=ben@chromium.org

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

[tfarina, 2013-04-04 00:33:13]

Should be ready now Mike.

Please, take a look.

[msw, 2013-04-04 02:31:02]

+oshima for ui/views/view_unittest.cc
Thanks for tackling this so quickly, Thiago!

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_item_view.cc:1050:
save_button_->set_min_size(gfx::Size());
Interestingly, TextButton::set_ignore_minimum_size would set a local member that
was never used :p Remove the set_min_size call altogether, since it's initialize
to an empty size anyway.

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_item_view.cc:1056:
discard_button_->set_min_size(gfx::Size());
Remove set_min_size as per my earlier comment.

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/fu...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/fu...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:221:
button_view_->deny_button()->set_max_size(gfx::Size());
It's a little confusing, but the TextButton::ClearMaxTextSize() call actually
translates to LabelButton::set_min_size(gfx::Size()), since the idea is to
ignore the lower bound on the size set by potentially larger previous strings.

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:207:
set_default_button->SetHorizontalAlignment(gfx::ALIGN_CENTER);
Add set_default_button->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);

https://codereview.chromium.org/13584010/diff/4033/ui/views/view_unittest.cc
File ui/views/view_unittest.cc (left):

https://codereview.chromium.org/13584010/diff/4033/ui/views/view_unittest.cc#...
ui/views/view_unittest.cc:1853: TEST_F(ViewTest, ChangeVisibility) {
+oshima to see if this should be deleted or left to fix.
This was added in a block comment in http://crrev.com/27282

[tfarina, 2013-04-04 02:34:20]

Note that views_unittests and unit_tests are both not very happy with these
changes, looks like there are focus issues under the way.

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

[msw, 2013-04-04 02:44:22]

On 2013/04/04 02:34:20, tfarina wrote:
> Note that views_unittests and unit_tests are both not very happy with these
> changes, looks like there are focus issues under the way.
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...

I'm not terribly concerned. Back out the
ui/views/focus/focus_traversal_unittest.cc and
chrome/browser/ui/views/accessibility/accessibility_event_router_views_unittest.cc
changes for now, I'll investigate those failures. Bonus points if you file a bug
and attach a patch with just those files changed, so I can patch it in and
hammer away.

[tfarina, 2013-04-04 22:28:44]

Mike, I backed out the changes and filed crbug.com/226814

[msw, 2013-04-04 22:31:55]

On 2013/04/04 22:28:44, tfarina wrote:
> Mike, I backed out the changes and filed crbug.com/226814

Thanks. Please address my other CL comments and I'll look again.

[tfarina, 2013-04-04 22:36:52]

Sorry, somehow I missed them.

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/do...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_item_view.cc:1050:
save_button_->set_min_size(gfx::Size());
On 2013/04/04 02:31:03, msw wrote:
> Interestingly, TextButton::set_ignore_minimum_size would set a local member
that
> was never used :p Remove the set_min_size call altogether, since it's
initialize
> to an empty size anyway.

Done.

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/do...
chrome/browser/ui/views/download/download_item_view.cc:1056:
discard_button_->set_min_size(gfx::Size());
On 2013/04/04 02:31:03, msw wrote:
> Remove set_min_size as per my earlier comment.

Done.

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/fu...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/fu...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:221:
button_view_->deny_button()->set_max_size(gfx::Size());
On 2013/04/04 02:31:03, msw wrote:
> It's a little confusing, but the TextButton::ClearMaxTextSize() call actually
> translates to LabelButton::set_min_size(gfx::Size()), since the idea is to
> ignore the lower bound on the size set by potentially larger previous strings.
Removed.

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:207:
set_default_button->SetHorizontalAlignment(gfx::ALIGN_CENTER);
On 2013/04/04 02:31:03, msw wrote:
> Add set_default_button->SetStyle(views::Button::STYLE_NATIVE_TEXTBUTTON);

Done.

[msw, 2013-04-04 22:41:16]

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/fu...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/fu...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:221:
button_view_->deny_button()->set_max_size(gfx::Size());
On 2013/04/04 22:36:52, tfarina wrote:
> On 2013/04/04 02:31:03, msw wrote:
> > It's a little confusing, but the TextButton::ClearMaxTextSize() call
actually
> > translates to LabelButton::set_min_size(gfx::Size()), since the idea is to
> > ignore the lower bound on the size set by potentially larger previous
strings.
> Removed.

Incorrect; it should be changed to set_min_size(gfx::Size()).

[tfarina, 2013-04-05 00:10:38]

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/fu...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

https://codereview.chromium.org/13584010/diff/4033/chrome/browser/ui/views/fu...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:221:
button_view_->deny_button()->set_max_size(gfx::Size());
On 2013/04/04 22:41:16, msw wrote:
> On 2013/04/04 22:36:52, tfarina wrote:
> > On 2013/04/04 02:31:03, msw wrote:
> > > It's a little confusing, but the TextButton::ClearMaxTextSize() call
> actually
> > > translates to LabelButton::set_min_size(gfx::Size()), since the idea is to
> > > ignore the lower bound on the size set by potentially larger previous
> strings.
> > Removed.
> 
> Incorrect; it should be changed to set_min_size(gfx::Size()).

Done.

[msw, 2013-04-05 00:45:08]

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc (right):

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:285: this,
l10n_util::GetStringUTF16(IDS_CANCEL));
optional nit: fits on the line above.

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/f...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:47: views::LabelButton*
accept_button() { return accept_button_; }
nit: keep this const.

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/f...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:48: views::LabelButton*
deny_button() { return deny_button_; }
nit: keep this const.

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/f...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:221:
button_view_->set_min_size(gfx::Size());
This should be: button_view_->deny_button()->set_min_size(gfx::Size());

[tfarina, 2013-04-05 13:09:32]

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/c...
File chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc (right):

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/c...
chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc:285: this,
l10n_util::GetStringUTF16(IDS_CANCEL));
On 2013/04/05 00:45:09, msw wrote:
> optional nit: fits on the line above.

Done.

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/f...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:47: views::LabelButton*
accept_button() { return accept_button_; }
On 2013/04/05 00:45:09, msw wrote:
> nit: keep this const.

Done.

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/f...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:48: views::LabelButton*
deny_button() { return deny_button_; }
On 2013/04/05 00:45:09, msw wrote:
> nit: keep this const.

Done.

https://codereview.chromium.org/13584010/diff/26001/chrome/browser/ui/views/f...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:221:
button_view_->set_min_size(gfx::Size());
On 2013/04/05 00:45:09, msw wrote:
> This should be: button_view_->deny_button()->set_min_size(gfx::Size());
This time built and tested. ;)

[msw, 2013-04-05 15:02:05]

LGTM; thanks!

[commit-bot: I haz the power, 2013-04-05 22:39:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/13584010/37001

[commit-bot: I haz the power, 2013-04-05 22:39:13]

Presubmit check for 13584010-37001 failed and returned exit status 1.

INFO:root:Found 32 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/ui/views/critical_notification_bubble_view.h
    chrome/browser/ui/views/critical_notification_bubble_view.cc
   
chrome/browser/ui/views/accessibility/accessibility_event_router_views_unittest.cc
    chrome/browser/ui/views/open_pdf_in_reader_bubble_view.h
    ui/views/focus/focus_manager_unittest_win.cc
    chrome/browser/ui/views/outdated_upgrade_bubble_view.h
    chrome/browser/ui/views/location_bar/zoom_bubble_view.cc
    chrome/browser/ui/views/open_pdf_in_reader_bubble_view.cc
    chrome/browser/ui/views/speech_recognition_bubble_views.cc
    chrome/browser/ui/views/download/download_item_view.cc
    chrome/browser/ui/views/location_bar/zoom_bubble_view.h
    chrome/browser/ui/views/content_setting_bubble_contents.cc
    chrome/browser/first_run/try_chrome_dialog_view.cc
    chrome/browser/ui/views/extensions/disabled_extensions_view.cc
    ui/views/test/child_modal_window.h
    chrome/browser/ui/views/content_setting_bubble_contents.h
    ui/views/test/child_modal_window.cc
    chrome/browser/ui/views/network_profile_bubble_view.cc
    chrome/browser/ui/views/chrome_to_mobile_bubble_view.h
    chrome/browser/ui/views/extensions/disabled_extensions_view.h
    chrome/browser/ui/views/fullscreen_exit_bubble_views.cc
    chrome/browser/ui/views/sync/one_click_signin_bubble_view.cc
    chrome/browser/ui/views/password_generation_bubble_view.h
    ui/views/view_unittest.cc
    chrome/browser/ui/views/sync/one_click_signin_bubble_view_browsertest.cc
    chrome/browser/ui/views/sync/one_click_signin_bubble_view.h
    ui/views/focus/focus_traversal_unittest.cc
    chrome/browser/ui/views/outdated_upgrade_bubble_view.cc
    chrome/browser/ui/views/chrome_to_mobile_bubble_view.cc
    chrome/browser/ui/views/password_generation_bubble_view.cc
    chrome/browser/ui/views/download/download_item_view.h
    chrome/browser/ui/views/global_error_bubble_view.cc

Presubmit checks took 2.5s to calculate.

[tfarina, 2013-04-05 22:40:31]

TBRing Ben for this.

[commit-bot: I haz the power, 2013-04-05 22:42:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/13584010/37001

[commit-bot: I haz the power, 2013-04-05 23:07:25]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
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.

[Ben Goodger (Google), 2013-04-05 23:18:57]

lgtm

[commit-bot: I haz the power, 2013-04-06 00:18:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/13584010/39003

[commit-bot: I haz the power, 2013-04-06 00:45:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
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-04-06 01:57:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/13584010/47001

[commit-bot: I haz the power, 2013-04-06 02:12:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/13584010/57001

[commit-bot: I haz the power, 2013-04-06 07:00:02]

Change committed as 192722