[Mac] Fix for the security chip

[Mac] Fix for the security chip

There was a race condition with the Page Info icon where it might switch
between LocationIconDecoration and SecurityStateBubbleDecoration

BUG=694396
Review-Url: https://codereview.chromium.org/2752623003
Cr-Commit-Position: refs/heads/master@{#457521}
Committed: https://chromium.googlesource.com/chromium/src/+/4cf8e7e6215f686f727736826109303c3b115eec

[spqchan, 2017-03-15 02:17:43]

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

[commit-bot: I haz the power, 2017-03-15 02:18:24]

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-03-15 02:38:21]

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

[commit-bot: I haz the power, 2017-03-15 02:38:22]

Dry run: This issue passed the CQ dry run.

[spqchan, 2017-03-15 02:52:12]

Description was changed from

==========
[Mac] Fix for the security chip

BUG=694396
==========

to

==========
[Mac] Fix for the security chip

The was a race condition with the Page Info icon where it might switch
between LocationIconDecoration and SecurityStateBubbleDecoration

BUG=694396
==========

[spqchan, 2017-03-15 02:52:12]

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

[spqchan, 2017-03-15 02:57:29]

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

[commit-bot: I haz the power, 2017-03-15 02:58:02]

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-03-15 03:24:25]

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

[commit-bot: I haz the power, 2017-03-15 03:24:26]

Dry run: This issue passed the CQ dry run.

[spqchan, 2017-03-15 16:39:35]

Patchset #2 (id:20001) has been deleted

[spqchan, 2017-03-15 16:39:51]

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

[spqchan, 2017-03-15 16:39:52]

PTAL

[spqchan, 2017-03-15 16:40:02]

Description was changed from

==========
[Mac] Fix for the security chip

The was a race condition with the Page Info icon where it might switch
between LocationIconDecoration and SecurityStateBubbleDecoration

BUG=694396
==========

to

==========
[Mac] Fix for the security chip

There was a race condition with the Page Info icon where it might switch
between LocationIconDecoration and SecurityStateBubbleDecoration

BUG=694396
==========

[Robert Sesek, 2017-03-15 17:35:09]

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h
(right):

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:41:
LocationBarDecoration* decoration_;
  // Weak.

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:285: -
(void)showWindow:(id)sender {
What about the ![self hasVisibileLocationBar] check above?

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:292: -
(void)close {
What's the destruction order of the bubble w.r.t. the BWC? Can the decoration
(assuming weak pointer in this class) be deleted before this runs?

[spqchan, 2017-03-15 20:48:18]

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

[commit-bot: I haz the power, 2017-03-15 20:49:05]

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-03-15 21:16:36]

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

[commit-bot: I haz the power, 2017-03-15 21:16:37]

Dry run: This issue passed the CQ dry run.

[spqchan, 2017-03-15 21:18:47]

PTAL

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h
(right):

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.h:41:
LocationBarDecoration* decoration_;
On 2017/03/15 17:35:09, Robert Sesek wrote:
>   // Weak.

Done.

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm
(right):

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:285: -
(void)showWindow:(id)sender {
On 2017/03/15 17:35:09, Robert Sesek wrote:
> What about the ![self hasVisibileLocationBar] check above?

Whoops, thanks for catching that!

https://codereview.chromium.org/2752623003/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/website_settings/permission_bubble_controller.mm:292: -
(void)close {
On 2017/03/15 17:35:09, Robert Sesek wrote:
> What's the destruction order of the bubble w.r.t. the BWC? Can the decoration
> (assuming weak pointer in this class) be deleted before this runs?

The bubble will close before the decoration and the BWC gets dealloc

[Robert Sesek, 2017-03-15 21:59:09]

LGTM, though bonus points for a unittest

[spqchan, 2017-03-16 17:46:46]

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

[commit-bot: I haz the power, 2017-03-16 17:47: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-03-16 17:52:22]

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

[commit-bot: I haz the power, 2017-03-16 17:52:24]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[spqchan, 2017-03-16 18:04:53]

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

[commit-bot: I haz the power, 2017-03-16 18:05:38]

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-03-16 18:27:45]

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

[commit-bot: I haz the power, 2017-03-16 18:27:46]

Dry run: This issue passed the CQ dry run.

[spqchan, 2017-03-16 18:34:05]

On 2017/03/15 21:59:09, Robert Sesek wrote:
> LGTM, though bonus points for a unittest

Sure why not, PTAL at the test :)

[Robert Sesek, 2017-03-16 19:32:43]

⭐️⭐️ LGTM ⭐️⭐️

[spqchan, 2017-03-16 19:34:46]

On 2017/03/16 19:32:43, Robert Sesek wrote:
> ⭐️⭐️ LGTM ⭐️⭐️

thanks~

[spqchan, 2017-03-16 19:34:50]

The CQ bit was checked by spqchan@chromium.org

[commit-bot: I haz the power, 2017-03-16 19:35:53]

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-03-16 19:41:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489692890243230,
"parent_rev": "46cc48ccb505f956b0f1a6fe5fc02ccba2ac03e8", "commit_rev":
"4cf8e7e6215f686f727736826109303c3b115eec"}

[commit-bot: I haz the power, 2017-03-16 19:42:16]

Description was changed from

==========
[Mac] Fix for the security chip

There was a race condition with the Page Info icon where it might switch
between LocationIconDecoration and SecurityStateBubbleDecoration

BUG=694396
==========

to

==========
[Mac] Fix for the security chip

There was a race condition with the Page Info icon where it might switch
between LocationIconDecoration and SecurityStateBubbleDecoration

BUG=694396

Review-Url: https://codereview.chromium.org/2752623003
Cr-Commit-Position: refs/heads/master@{#457521}
Committed:
https://chromium.googlesource.com/chromium/src/+/4cf8e7e6215f686f727736826109...
==========

[commit-bot: I haz the power, 2017-03-16 19:42:17]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/4cf8e7e6215f686f727736826109...