Refactor ViewsDelegate and MD-ify the icon-to-text spacing for checkbox and radiobutton

chrome/browser/ui/views/toolbar/reload_button_unittest.cc

 // Copyright 2013 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/toolbar/reload_button.h"

[sky, 2017/02/28 20:11:52]

newline between 5/6 (see style guide for details).

[kylix_rd, 2017/03/01 17:33:27]

Interesting... this was changed by git cl format, which I would have assumed
would do the right thing.

[kylix_rd, 2017/03/01 17:33:27]

Done.

 #include "base/run_loop.h"
-#include "chrome/browser/ui/views/toolbar/reload_button.h"
 #include "chrome/test/base/chrome_render_view_host_test_harness.h"
 #include "chrome/test/base/testing_profile.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/events/event_utils.h"
+#include "ui/views/test/test_views_delegate.h"
 
 class ReloadButtonTest : public ChromeRenderViewHostTestHarness {
  public:
   ReloadButtonTest();
 
   void CheckState(bool enabled,
                   ReloadButton::Mode intended_mode,
                   ReloadButton::Mode visible_mode,
                   bool double_click_timer_running,
                   bool stop_to_reload_timer_running);
 
   // These accessors eliminate the need to declare each testcase as a friend.
   void set_mouse_hovered(bool hovered) {
     reload_.testing_mouse_hovered_ = hovered;
   }
   int reload_count() { return reload_.testing_reload_count_; }
 
+ private:

[sky, 2017/02/28 20:11:52]

Style guide says private section should be last (after protected, which is after
public).

[kylix_rd, 2017/03/01 17:33:27]

It seems that there is a chicken-and-egg problem then. Placing the
TestViewsDelegate field lexically after the ReloadButton field causes the
ReloadButton to be constructed prior to the TestViewsDelegate. This will lead to
a crash in the construction of the ReloadButton.

The solution would be to move the ReloadButton field to the private section and
then add a getter function returning a ReloadButton& to the protected section
and refactor the tests which reference the reload_ field.

[Peter Kasting, 2017/03/01 23:32:23]

I was gonna leave a comment about how this test really shouldn't have a
protected data member, but I didn't want to burden you with unnecessary code
cleanup in the midst of trying to land this.

But... yeah, that seems like the correct way to solve :)

+  views::TestViewsDelegate views_delegate_;
+
  protected:
   ReloadButton reload_;
 };

[Peter Kasting, 2017/03/01 06:32:36]

Nit: DISALLOW_COPY_AND_ASSIGN

[kylix_rd, 2017/03/01 17:33:27]

Done.

 
 ReloadButtonTest::ReloadButtonTest() : reload_(profile(), nullptr) {
   // Set the timer delays to 0 so that timers will fire as soon as we tell the
   // message loop to run pending tasks.
   reload_.double_click_timer_delay_ = base::TimeDelta();
   reload_.stop_to_reload_timer_delay_ = base::TimeDelta();
 }
 
 void ReloadButtonTest::CheckState(bool enabled,
                                   ReloadButton::Mode intended_mode,
@@ skipping 106 matching lines @@
   reload_.ButtonPressed(&reload_, e);
   reload_.ChangeMode(ReloadButton::MODE_STOP, false);
   set_mouse_hovered(true);
   reload_.ChangeMode(ReloadButton::MODE_RELOAD, false);
 
   // Now fire the stop-to-reload timer.  This should reset the button.
   base::RunLoop().RunUntilIdle();
   CheckState(true, ReloadButton::MODE_RELOAD, ReloadButton::MODE_RELOAD, false,
              false);
 }