[Mac] Test for Security State Bubble Decoration

[Mac] Test for Security State Bubble Decoration

- Wrote unit tests for the decoration
- Fixed the issues found from the tests

BUG=687347
Review-Url: https://codereview.chromium.org/2700243002
Cr-Commit-Position: refs/heads/master@{#452863}
Committed: https://chromium.googlesource.com/chromium/src/+/0153b22ba239a1b863b80a39d1c163c5e6a983e0

[spqchan, 2017-02-21 23:19:43]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 23:21:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 01:27:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 01:27:19]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[spqchan, 2017-02-22 05:51:54]

Description was changed from

==========
SecurityStateBubbleDecoration tests


BUG=
==========

to

==========
[Mac] Test for Security State Bubble Decoration

- Wrote unit tests for the decoration
- Fixed the issues found from the tests

BUG=687347
==========

[spqchan, 2017-02-22 05:51:54]

spqchan@chromium.org changed reviewers:
+ rsesek@chromium.org

[spqchan, 2017-02-22 05:52:11]

PTAL

[Robert Sesek, 2017-02-23 21:06:51]

Thanks for the test!

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:18: 
You can place all your test classes inside an anonymous namespace to ensure that
any other tests that define things of the same don't produce a linker error.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:20: class
MockOmniboxView : public OmniboxViewMac {
Why is this Mock but the rest of the test helpers Test?

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:29: void
SetIsEmpty(bool is_empty) { is_empty_ = is_empty; }
Generally use hacker_case for simple setters.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:36: bool
is_empty_;
Initialize this in the ctor or inline.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:37: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:111: bool
has_animated_;
Initialize these.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:187: //
The mocked omnibox view.
"Weak, owned by location_bar_."

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:193: //
Tests to see if the security decoration shows and animates correctly.
Rather than just say "correctly", I'd describe what the test does, e.g.:

"Tests that: the dangerous state animates in, going from dangerous to none
animates out, going from none to secure shows a decoration without animation,
and that EV and none security states do not animate."

Or do it inline like you do for some of the other tests.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:256: //
Show the secure decoration
nit: punctuation

[spqchan, 2017-02-23 22:45:07]

PTAL

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:18: 
On 2017/02/23 21:06:51, Robert Sesek wrote:
> You can place all your test classes inside an anonymous namespace to ensure
that
> any other tests that define things of the same don't produce a linker error.

Done.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:20: class
MockOmniboxView : public OmniboxViewMac {
On 2017/02/23 21:06:51, Robert Sesek wrote:
> Why is this Mock but the rest of the test helpers Test?

This is Mock because we're mocking it but we're not actually testing it.
TestingLocationBarViewMac and TestingSecurityDecoration are actually being
tested. They just have a bit of changes made to them so that we can control what
we test. Hope this clears things up!

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:29: void
SetIsEmpty(bool is_empty) { is_empty_ = is_empty; }
On 2017/02/23 21:06:50, Robert Sesek wrote:
> Generally use hacker_case for simple setters.

Done.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:36: bool
is_empty_;
On 2017/02/23 21:06:50, Robert Sesek wrote:
> Initialize this in the ctor or inline.

Done.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:37: };
On 2017/02/23 21:06:50, Robert Sesek wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:111: bool
has_animated_;
On 2017/02/23 21:06:51, Robert Sesek wrote:
> Initialize these.

Done.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:187: //
The mocked omnibox view.
On 2017/02/23 21:06:50, Robert Sesek wrote:
> "Weak, owned by location_bar_."

Done.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:193: //
Tests to see if the security decoration shows and animates correctly.
On 2017/02/23 21:06:50, Robert Sesek wrote:
> Rather than just say "correctly", I'd describe what the test does, e.g.:
> 
> "Tests that: the dangerous state animates in, going from dangerous to none
> animates out, going from none to secure shows a decoration without animation,
> and that EV and none security states do not animate."
> 
> Or do it inline like you do for some of the other tests.

Done.

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:256: //
Show the secure decoration
On 2017/02/23 21:06:51, Robert Sesek wrote:
> nit: punctuation

Done.

[spqchan, 2017-02-23 22:45:12]

The CQ bit was checked by spqchan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 22:46:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 00:30:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 00:30:07]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Robert Sesek, 2017-02-24 17:00:39]

LGTM!

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm
(right):

https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:20: class
MockOmniboxView : public OmniboxViewMac {
On 2017/02/23 22:45:06, spqchan wrote:
> On 2017/02/23 21:06:51, Robert Sesek wrote:
> > Why is this Mock but the rest of the test helpers Test?
> 
> This is Mock because we're mocking it but we're not actually testing it.
> TestingLocationBarViewMac and TestingSecurityDecoration are actually being
> tested. They just have a bit of changes made to them so that we can control
what
> we test. Hope this clears things up!

Acknowledged.

[spqchan, 2017-02-24 17:02:40]

On 2017/02/24 17:00:39, Robert Sesek wrote:
> LGTM!
> 
>
https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
> File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm
> (right):
> 
>
https://codereview.chromium.org/2700243002/diff/40001/chrome/browser/ui/cocoa...
> chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_unittest.mm:20:
class
> MockOmniboxView : public OmniboxViewMac {
> On 2017/02/23 22:45:06, spqchan wrote:
> > On 2017/02/23 21:06:51, Robert Sesek wrote:
> > > Why is this Mock but the rest of the test helpers Test?
> > 
> > This is Mock because we're mocking it but we're not actually testing it.
> > TestingLocationBarViewMac and TestingSecurityDecoration are actually being
> > tested. They just have a bit of changes made to them so that we can control
> what
> > we test. Hope this clears things up!
> 
> Acknowledged.

Thanks!

[spqchan, 2017-02-24 17:02:45]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2017-02-24 17:03:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 17:47:08]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487955765123750,
"parent_rev": "705b13eab5b3969e6f351cd23bb536ec204a78fc", "commit_rev":
"0153b22ba239a1b863b80a39d1c163c5e6a983e0"}

[commit-bot: I haz the power, 2017-02-24 17:47:52]

Description was changed from

==========
[Mac] Test for Security State Bubble Decoration

- Wrote unit tests for the decoration
- Fixed the issues found from the tests

BUG=687347
==========

to

==========
[Mac] Test for Security State Bubble Decoration

- Wrote unit tests for the decoration
- Fixed the issues found from the tests

BUG=687347

Review-Url: https://codereview.chromium.org/2700243002
Cr-Commit-Position: refs/heads/master@{#452863}
Committed:
https://chromium.googlesource.com/chromium/src/+/0153b22ba239a1b863b80a39d1c1...
==========

[commit-bot: I haz the power, 2017-02-24 17:47:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/0153b22ba239a1b863b80a39d1c1...