Hide protocol handler icon in location bar after user is done

Hide protocol handler icon in location bar after user is done
interacting with it.

This fixes a regression where the icon lingered after the user pressed
"Done". Luckily this regression only affected the protocol handler icon
and didn't change functionality (the user's choice was still respected).

This also improves on the behavior of said icon by hiding it when the
user dismisses the bubble via other means (e.g. clicking outside the
bubble), so long as they've explicitly interacted with the bubble by changing
the setting.

BUG=629645
Committed: https://crrev.com/094178fd159b1c26e82e7a5fa7f33f28aec10dd1
Cr-Commit-Position: refs/heads/master@{#406857}

[Evan Stade, 2016-07-20 18:00:49]

estade@chromium.org changed reviewers:
+ mgiuca@chromium.org, msw@chromium.org

[msw, 2016-07-20 20:50:17]

Can you clarify how this now closes the bubble on Accept?
(I don't clearly see how this fixes the defect)

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1076: return;
Early return in a dtor is a bit odd... can you instead invert the check and wrap
the below code in a conditional block?

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:275: //
Initially false, set to true if the user explicitly interacts with the
optional nit: remove 'explicitly' for a one-liner

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:412: DCHECK(i !=
radio_group_.end());
nit: DCHECK_NE?

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.h (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.h:30: class LabelButton;
nit: remove

[Evan Stade, 2016-07-20 20:56:46]

(code not updated yet)

> Can you clarify how this now closes the bubble on Accept?
> (I don't clearly see how this fixes the defect)

closing the bubble wasn't the problem, it's hiding the icon in the location bar
after the bubble closes that was the problem.

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1076: return;
On 2016/07/20 20:50:16, msw wrote:
> Early return in a dtor is a bit odd... can you instead invert the check and
wrap
> the below code in a conditional block?

Is there a reason not to early return in a dtor? Most of the time reviewers show
a preference for reduced indentation.

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.cc (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.cc:412: DCHECK(i !=
radio_group_.end());
On 2016/07/20 20:50:16, msw wrote:
> nit: DCHECK_NE?

DCHECK_NE seems more useful when you have two variables of unknown value.
radio_group_.end() is not a unknown, hence if this DCHECK fails you know what i
is (and what the problem is).

[msw, 2016-07-20 21:15:47]

On 2016/07/20 20:56:46, Evan Stade wrote:
> closing the bubble wasn't the problem, it's hiding the icon in the location
bar
> after the bubble closes that was the problem.

Can you clarify how this CL fixes that problem?

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1076: return;
On 2016/07/20 20:56:46, Evan Stade wrote:
> On 2016/07/20 20:50:16, msw wrote:
> > Early return in a dtor is a bit odd... can you instead invert the check and
> wrap
> > the below code in a conditional block?
> 
> Is there a reason not to early return in a dtor? Most of the time reviewers
show
> a preference for reduced indentation.

Just thinking of someone thoughtlessly adding code after your early return.

[Evan Stade, 2016-07-20 22:11:05]

> Can you clarify [...]

When you click "Done", i.e. Accept() is called, we call OnDoneClicked().
ContentSettingRPHBubbleModel::OnDoneClicked used to send a notification which
the browser picks up and uses to refresh (hide) the icon, now, it's deferred to
the dtor but OnDoneClicked is still necessary to indicate the user has
interacted with the bubble.)

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1076: return;
On 2016/07/20 21:15:47, msw wrote:
> On 2016/07/20 20:56:46, Evan Stade wrote:
> > On 2016/07/20 20:50:16, msw wrote:
> > > Early return in a dtor is a bit odd... can you instead invert the check
and
> > wrap
> > > the below code in a conditional block?
> > 
> > Is there a reason not to early return in a dtor? Most of the time reviewers
> show
> > a preference for reduced indentation.
> 
> Just thinking of someone thoughtlessly adding code after your early return.

That could happen, but isn't that possible for any function that has an early
return?

[Evan Stade, 2016-07-20 22:16:17]

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/views/con...
File chrome/browser/ui/views/content_setting_bubble_contents.h (right):

https://codereview.chromium.org/2169453003/diff/1/chrome/browser/ui/views/con...
chrome/browser/ui/views/content_setting_bubble_contents.h:30: class LabelButton;
On 2016/07/20 20:50:17, msw wrote:
> nit: remove

Done.

[Evan Stade, 2016-07-20 22:16:22]

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

[commit-bot: I haz the power, 2016-07-20 22:16:58]

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

[msw, 2016-07-20 22:28:07]

lgtm

[commit-bot: I haz the power, 2016-07-20 23:14:52]

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

[commit-bot: I haz the power, 2016-07-20 23:14:53]

Dry run: This issue passed the CQ dry run.

[Matt Giuca, 2016-07-21 00:44:47]

mgiuca@chromium.org changed reviewers:
- mgiuca@chromium.org

[Matt Giuca, 2016-07-21 00:44:48]

Thanks for a quick fix (I didn't look over it; I have no state on this, just
helped triage).

[Evan Stade, 2016-07-21 15:24:34]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-07-21 15:25:00]

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

[Evan Stade, 2016-07-21 15:25:09]

estade@chromium.org changed reviewers:
+ bauerb@chromium.org

[Evan Stade, 2016-07-21 15:25:10]

+bauerb for content_settings ownership

[commit-bot: I haz the power, 2016-07-21 15:28:17]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-21 15:30:08]

Description was changed from

==========
Hide protocol handler icon in location bar after user is done
interacting with it.

This fixes a regression where the icon lingered after the user pressed
"Done". Luckily this regression only affected the protocol handler icon
and didn't change functionality (the user's choice was still respected).

This also improves on the behavior of said icon by hiding it when the
user dismisses the bubble via other means (e.g. clicking outside the
bubble), so long as they've explicitly interacted with the bubble by changing
the setting.

BUG=629645
==========

to

==========
Hide protocol handler icon in location bar after user is done
interacting with it.

This fixes a regression where the icon lingered after the user pressed
"Done". Luckily this regression only affected the protocol handler icon
and didn't change functionality (the user's choice was still respected).

This also improves on the behavior of said icon by hiding it when the
user dismisses the bubble via other means (e.g. clicking outside the
bubble), so long as they've explicitly interacted with the bubble by changing
the setting.

BUG=629645

Committed: https://crrev.com/094178fd159b1c26e82e7a5fa7f33f28aec10dd1
Cr-Commit-Position: refs/heads/master@{#406857}
==========

[commit-bot: I haz the power, 2016-07-21 15:30:10]

Patchset 3 (id:??) landed as
https://crrev.com/094178fd159b1c26e82e7a5fa7f33f28aec10dd1
Cr-Commit-Position: refs/heads/master@{#406857}