Bring up a content settings icon for ungestured registerProtocolHandler call.

Bring up a content settings icon for ungestured registerProtocolHandler call.

R=benwells@chromium.org
BUG=122020
TEST=None

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

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147045

[koz (OOO until 15th September), 2012-06-21 01:50:53]

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:173: void
SetRegisterProtocolHandlerCalledUngestured(ProtocolHandler handler) {
This code could do with an explanatory comment.

Also, what do you think of the name SetPendingRegisterProtocolHandler()? It is
more suggestive about what happens with this protocol handler (ie: gets
registered later, when the user accepts).

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:177: bool
IsRegisterProtocolHandlerCalledUngestured() const {
IsRegisterProtocolHandlerPending()?

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:181: const
ProtocolHandler& UngesturedProtocolHandler() const {
GetPendingProtocolHandler()

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:185: void
SetOldRegisterProtocolHandlerTitle(const string16 title) {
SetProtocolHandlerTitleToBeReplaced()?

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:189: const
string16& OldRegisterProtocolHandlerTitle() const {
GetProtocolHandlerTitleToBeReplaced()

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:304: // Set to
the registered protocol descriptor if registerProtocolHandler was
Maybe rephrase as "The protocol handler that is pending user approval for
registration. This situation arises when a call to registerProtocolHandler() is
made without a user gesture. An empty protocol handler means that no protocol
handler is pending registration."

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:307:
ProtocolHandler register_protocol_handler_called_ungestured_;
If the above name changes are followed, this could be called
pending_register_protocol_handler_ or similar.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:309: // If there
is an old protocol handler, this is the title. (Used for the
Maybe rephrase this as "The title of the protocol handler that will be replaced
if the pending call to registerProtocolHandler goes through. Empty if no
protocol handler will be replaced."

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:311: string16
old_protocol_handler_title_;
protocol_handler_to_be_replaced_title_?

Also, consider just storing the whole ProtocolHandler, rather than just the
title. Then you could omit Title from all the variable and method names above.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:2297:
content_settings->SetRegisterProtocolHandlerCalledUngestured(
Could change to

content_settings->ClearPendingRegisterProtocolHandlerCall();

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/content_sett...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/content_sett...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:555:
std::string radio_block_label =
radio_block_label -> radio_deny_label

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/content_sett...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:585:
UnregisterProtocolHandler();
Is the idea here that there are two radio boxes that the user can switch between
at will, each change resulting in an OnRadioClicked() call? If so I'm not sure
this will have the desired result, because UnregisterProtocolHandler() here
makes a call to ProtocolHandlerRegistry::OnDenyRegisterProtocolHandler(), which
is identical to OnAllowRegisterProtocolHandler(), except it doesn't set the
protocol handler as the default handler for that protocol.

Or, in other words, once the user selects the "allow" radio button, that
protocol handler will stay as the default from now on, even if they click the
"deny" button afterwards.

You could solve this by storing the entire ProtocolHandler that is to be
replaced in content_settings(), rather than just the title (as per my previous
comment) and then having the behaviour here be that clicking deny causes a call
to OnAcceptRegisterProtocolHandler() with the original handler as an argument.

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5561: LOG(INFO) << "Got RPH in
RenderViewImpl";
Don't forget to remove this.

[Greg Billock, 2012-06-21 19:59:11]

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:173: void
SetRegisterProtocolHandlerCalledUngestured(ProtocolHandler handler) {
On 2012/06/21 01:50:53, koz wrote:
> This code could do with an explanatory comment.
> 
> Also, what do you think of the name SetPendingRegisterProtocolHandler()? It is
> more suggestive about what happens with this protocol handler (ie: gets
> registered later, when the user accepts).

Done.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:177: bool
IsRegisterProtocolHandlerCalledUngestured() const {
On 2012/06/21 01:50:53, koz wrote:
> IsRegisterProtocolHandlerPending()?

Killed this in favor of just doing PendingProtocolHandler().IsEmpty().

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:181: const
ProtocolHandler& UngesturedProtocolHandler() const {
On 2012/06/21 01:50:53, koz wrote:
> GetPendingProtocolHandler()

Done.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:185: void
SetOldRegisterProtocolHandlerTitle(const string16 title) {
On 2012/06/21 01:50:53, koz wrote:
> SetProtocolHandlerTitleToBeReplaced()?

Called this 'Previous'. Sound good?

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:189: const
string16& OldRegisterProtocolHandlerTitle() const {
On 2012/06/21 01:50:53, koz wrote:
> GetProtocolHandlerTitleToBeReplaced()

Done.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:304: // Set to
the registered protocol descriptor if registerProtocolHandler was
On 2012/06/21 01:50:53, koz wrote:
> Maybe rephrase as "The protocol handler that is pending user approval for
> registration. This situation arises when a call to registerProtocolHandler()
is
> made without a user gesture. An empty protocol handler means that no protocol
> handler is pending registration."

I like it. Making all these changes.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:307:
ProtocolHandler register_protocol_handler_called_ungestured_;
On 2012/06/21 01:50:53, koz wrote:
> If the above name changes are followed, this could be called
> pending_register_protocol_handler_ or similar.

Done.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:309: // If there
is an old protocol handler, this is the title. (Used for the
On 2012/06/21 01:50:53, koz wrote:
> Maybe rephrase this as "The title of the protocol handler that will be
replaced
> if the pending call to registerProtocolHandler goes through. Empty if no
> protocol handler will be replaced."

Done.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:311: string16
old_protocol_handler_title_;
On 2012/06/21 01:50:53, koz wrote:
> protocol_handler_to_be_replaced_title_?
> 
> Also, consider just storing the whole ProtocolHandler, rather than just the
> title. Then you could omit Title from all the variable and method names above.

At this point I don't think we need the whole thing -- just the title to build
the right message. Do we end up needing it for API calls later?

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:2297:
content_settings->SetRegisterProtocolHandlerCalledUngestured(
On 2012/06/21 01:50:53, koz wrote:
> Could change to
> 
> content_settings->ClearPendingRegisterProtocolHandlerCall();

You think the extra method is worth it?

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/content_sett...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/content_sett...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:555:
std::string radio_block_label =
On 2012/06/21 01:50:53, koz wrote:
> radio_block_label -> radio_deny_label

Done.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/content_sett...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:585:
UnregisterProtocolHandler();
On 2012/06/21 01:50:53, koz wrote:
> Is the idea here that there are two radio boxes that the user can switch
between
> at will, each change resulting in an OnRadioClicked() call? If so I'm not sure
> this will have the desired result, because UnregisterProtocolHandler() here
> makes a call to ProtocolHandlerRegistry::OnDenyRegisterProtocolHandler(),
which
> is identical to OnAllowRegisterProtocolHandler(), except it doesn't set the
> protocol handler as the default handler for that protocol.
> 
> Or, in other words, once the user selects the "allow" radio button, that
> protocol handler will stay as the default from now on, even if they click the
> "deny" button afterwards.
> 
> You could solve this by storing the entire ProtocolHandler that is to be
> replaced in content_settings(), rather than just the title (as per my previous
> comment) and then having the behaviour here be that clicking deny causes a
call
> to OnAcceptRegisterProtocolHandler() with the original handler as an argument.

Ah, that's a good point. I'll just save the whole thing.

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5561: LOG(INFO) << "Got RPH in
RenderViewImpl";
On 2012/06/21 01:50:53, koz wrote:
> Don't forget to remove this.

I won't. There looks like a race bug, at least on linux. Without this line, an
ungestured RPH call in the page's onload() handler almost never gets through to
the browser to be handled. With this call, it almost always does get through. I
emailed Ben about this, but do you know how this can be fixed?

[koz (OOO until 15th September), 2012-06-21 23:03:50]

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/content_setting...
chrome/browser/content_settings/tab_specific_content_settings.h:185: void
SetOldRegisterProtocolHandlerTitle(const string16 title) {
On 2012/06/21 19:59:11, Greg Billock wrote:
> On 2012/06/21 01:50:53, koz wrote:
> > SetProtocolHandlerTitleToBeReplaced()?
> 
> Called this 'Previous'. Sound good?

Yep, sounds fine.

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:2297:
content_settings->SetRegisterProtocolHandlerCalledUngestured(
On 2012/06/21 19:59:11, Greg Billock wrote:
> On 2012/06/21 01:50:53, koz wrote:
> > Could change to
> > 
> > content_settings->ClearPendingRegisterProtocolHandlerCall();
> 
> You think the extra method is worth it?

Yeah, I don't think it's a big cost, up to you though. In my opinion it makes
the calling code's intent clearer.

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5561: LOG(INFO) << "Got RPH in
RenderViewImpl";
On 2012/06/21 19:59:11, Greg Billock wrote:
> On 2012/06/21 01:50:53, koz wrote:
> > Don't forget to remove this.
> 
> I won't. There looks like a race bug, at least on linux. Without this line, an
> ungestured RPH call in the page's onload() handler almost never gets through
to
> the browser to be handled. With this call, it almost always does get through.
I
> emailed Ben about this, but do you know how this can be fixed?

That's strange. I wonder if something changed recently in this area because RPH
is usually called onload() currently (eg: by calendar). Does it happen before
this patch?

http://codereview.chromium.org/10584042/diff/9001/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/9001/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:602: if
(!previous_handler_.IsEmpty()) {
There's still a subtle bug here. If the user clicks accept and then deny,
previous_handler_ will be empty and so nothing will happen (OnDenyRPH() being
idempotent once OnAcceptRPH() is called), meaning the original protocol handler
will still be registered. In the case where previous_handler_ is empty, I think
the correct thing to do is to call ClearDefault(). That, rather than
RemoveHandler(pending_handler_), because the latter will cause the same prompt
to appear the next time the user visits the page, as all memory of the protocol
handler will be erased.

The code for this method should be simply:

if (previous_handler_.IsEmpty())
  phr->ClearDefault(pending_handler_.scheme());
else
  phr->OnAcceptRegisterProtocolHandler(previous_handler_);

[Greg Billock, 2012-06-21 23:10:58]

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10584042/diff/1/chrome/browser/ui/browser.cc#n...
chrome/browser/ui/browser.cc:2297:
content_settings->SetRegisterProtocolHandlerCalledUngestured(
Ok. Adding.

On 2012/06/21 23:03:50, koz wrote:
> On 2012/06/21 19:59:11, Greg Billock wrote:
> > On 2012/06/21 01:50:53, koz wrote:
> > > Could change to
> > > 
> > > content_settings->ClearPendingRegisterProtocolHandlerCall();
> > 
> > You think the extra method is worth it?
> 
> Yeah, I don't think it's a big cost, up to you though. In my opinion it makes
> the calling code's intent clearer.

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5561: LOG(INFO) << "Got RPH in
RenderViewImpl";
Yes. It was happening when I was testing the user gesture stuff, before any of
this.

On 2012/06/21 23:03:50, koz wrote:
> On 2012/06/21 19:59:11, Greg Billock wrote:
> > On 2012/06/21 01:50:53, koz wrote:
> > > Don't forget to remove this.
> > 
> > I won't. There looks like a race bug, at least on linux. Without this line,
an
> > ungestured RPH call in the page's onload() handler almost never gets through
> to
> > the browser to be handled. With this call, it almost always does get
through.
> I
> > emailed Ben about this, but do you know how this can be fixed?
> 
> That's strange. I wonder if something changed recently in this area because
RPH
> is usually called onload() currently (eg: by calendar). Does it happen before
> this patch?

http://codereview.chromium.org/10584042/diff/9001/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/9001/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:602: if
(!previous_handler_.IsEmpty()) {
I found this as well. Also added a tracker to make sure the current status of
the flag is known in between the bubble showing up.

On 2012/06/21 23:03:50, koz wrote:
> There's still a subtle bug here. If the user clicks accept and then deny,
> previous_handler_ will be empty and so nothing will happen (OnDenyRPH() being
> idempotent once OnAcceptRPH() is called), meaning the original protocol
handler
> will still be registered. In the case where previous_handler_ is empty, I
think
> the correct thing to do is to call ClearDefault(). That, rather than
> RemoveHandler(pending_handler_), because the latter will cause the same prompt
> to appear the next time the user visits the page, as all memory of the
protocol
> handler will be erased.
> 
> The code for this method should be simply:
> 
> if (previous_handler_.IsEmpty())
>   phr->ClearDefault(pending_handler_.scheme());
> else
>   phr->OnAcceptRegisterProtocolHandler(previous_handler_);

[koz (OOO until 15th September), 2012-06-22 00:41:14]

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5561: LOG(INFO) << "Got RPH in
RenderViewImpl";
On 2012/06/21 23:10:59, Greg Billock wrote:
> Yes. It was happening when I was testing the user gesture stuff, before any of
> this.

I'm not sure why this would be failing. Recently the unit tests for this class
became really flaky on linux and chromeos, but I haven't seen any changes that
might have caused that.

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:603:
radio_group.default_item = 0;
Lift these into constants?

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:643: if
(!previous_handler_.IsEmpty()) {
nit: swap the if and the else clause here (same with below). Consider pulling
this logic out into a method.

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:653:
profile()->GetProtocolHandlerRegistry()->OnIgnoreRegisterProtocolHandler(
If the user clicks "ignore" and then "add" that will put us in a weird state.
Could you add a call to phr->RemoveIgnoredHandler(pending_handler_) in your
RegisterProtocolHandler() method? This should do nothing if the user hasn't
pressed ignore, and it should undo the ignore if the user has.

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:289:
content_setting_bubble_model->OnRadioClicked(1);
If you make these constants as per my previous comment you can reuse them here.

[Greg Billock, 2012-06-22 19:45:39]

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10584042/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:5561: LOG(INFO) << "Got RPH in
RenderViewImpl";
On 2012/06/22 00:41:14, koz wrote:
> On 2012/06/21 23:10:59, Greg Billock wrote:
> > Yes. It was happening when I was testing the user gesture stuff, before any
of
> > this.
> 
> I'm not sure why this would be failing. Recently the unit tests for this class
> became really flaky on linux and chromeos, but I haven't seen any changes that
> might have caused that.

Looks like this is a prerendering thing. Proper threads/bugs opened.

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:603:
radio_group.default_item = 0;
On 2012/06/22 00:41:14, koz wrote:
> Lift these into constants?

Done.

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:643: if
(!previous_handler_.IsEmpty()) {
On 2012/06/22 00:41:14, koz wrote:
> nit: swap the if and the else clause here (same with below). Consider pulling
> this logic out into a method.

Done.

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:653:
profile()->GetProtocolHandlerRegistry()->OnIgnoreRegisterProtocolHandler(
On 2012/06/22 00:41:14, koz wrote:
> If the user clicks "ignore" and then "add" that will put us in a weird state.
> Could you add a call to phr->RemoveIgnoredHandler(pending_handler_) in your
> RegisterProtocolHandler() method? This should do nothing if the user hasn't
> pressed ignore, and it should undo the ignore if the user has.

Fixed this up and augmented the test.

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):

http://codereview.chromium.org/10584042/diff/10021/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:289:
content_setting_bubble_model->OnRadioClicked(1);
On 2012/06/22 00:41:14, koz wrote:
> If you make these constants as per my previous comment you can reuse them
here.

I put them on the hidden class, so I can't get to them from here. I'm reluctant
to boost them out somewhere visible, even though it'd definitely be convenient
here.

[koz (OOO until 15th September), 2012-06-25 01:18:27]

Awesome! lgtm

[commit-bot: I haz the power, 2012-06-25 15:45:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10584042/33001

[commit-bot: I haz the power, 2012-06-25 15:45:54]

Presubmit check for 10584042-33001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/gtk
    chrome/browser/ui
    chrome/browser/ui/content_settings
    chrome/browser/ui/webui/options2

Presubmit checks took 1.4s to calculate.

[Greg Billock, 2012-06-25 15:59:56]

On 2012/06/25 15:45:54, I haz the power (commit-bot) wrote:
> Presubmit check for 10584042-33001 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/browser/ui/gtk
>     chrome/browser/ui
>     chrome/browser/ui/content_settings
>     chrome/browser/ui/webui/options2
> 
> Presubmit checks took 1.4s to calculate.

Owners? Time for you to do your thing.

[sky, 2012-06-25 16:47:08]

I'm swapping myself with Peter who knows more about content settings than I do.

[Bernhard Bauer, 2012-06-25 16:52:20]

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_default_provider.cc:47:
CONTENT_SETTING_ASK,      // CONTENT_SETTINGS_TYPE_REGISTER_PROTOCOL_HANDLER
Do we actually store content settings for this type in the
HostContentSettingsMap? If not, you should set the default value to DEFAULT.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
chrome/browser/content_settings/tab_specific_content_settings.h:174: void
SetPendingProtocolHandler(const ProtocolHandler& handler) {
Trivial setters/getters should be named unix_hacker_style.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
chrome/browser/content_settings/tab_specific_content_settings.h:330: // The
setting on the pending protocol handler registration.
I think this is only interesting if we open the bubble, change something, then
open it again, right? Could you explain that in the comment?

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:133:
IDS_HANDLERS_BUBBLE_MANAGE_LINK}
Nit: Indent three more spaces?

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:604: if
(setting == CONTENT_SETTING_BLOCK)
Nit: else if?

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:621: if
(radio_index == RPH_ALLOW) {
You can either leave out the braces or leave them in, but you should be
consistent with above.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:278:
content_setting_bubble_model->OnRadioClicked(0);
You could move these into constants.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:658: if (type ==
CONTENT_SETTINGS_TYPE_REGISTER_PROTOCOL_HANDLER)
I think this should be moved into UpdateExceptionsViewFromModel() below (along
with AUTO_SELECT_CERTIFICATE).

http://codereview.chromium.org/10584042/diff/33001/chrome/common/content_sett...
File chrome/common/content_settings_types.h (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/common/content_sett...
chrome/common/content_settings_types.h:27:
CONTENT_SETTINGS_TYPE_REGISTER_PROTOCOL_HANDLER,
I think the feature itself is just called "protocol handlers", right? Naming the
enum just PROTOCOL_HANDLER would also make it a bit shorter :)

[Evan Stade, 2012-06-25 23:31:07]

gtk lgtm

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:342:
"register-protocol-handler"},
2 more indent

[Peter Kasting, 2012-06-25 23:55:30]

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1998: if (!user_gesture && window) {
It worries me that you NULL-check |window| here and then blindly deref it on
line 2009.  DCHECK() it atop this function or else handle it being NULL in all
cases (preferably the first).

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:133:
IDS_HANDLERS_BUBBLE_MANAGE_LINK}
On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> Nit: Indent three more spaces?

I'd do two more.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:548:
ContentSettingRPHBubbleModel(Delegate* delegate,
Nit: For consistency, people should de-inline class member definitions even when
the declaration is in a .cc file.  The whole rest of this file is also wrong,
and I won't make you fix that (unless you want to!), but if you could separate
this class into a declaration and definition that'd be great.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:569: if
(pending_handler_.protocol() == "webcal") {
Nit: For clarity I'd make this "else if" and put the UTF8ToUTF16(...) call from
above in a final "else" block.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:604: if
(setting == CONTENT_SETTING_BLOCK)
On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> Nit: else if?

Not just a nit -- all "allow"s will get set to RPH_IGNORE without this :)

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:234:
"mailto", GURL("http://www.toplevel.example/"),
Nit: If you're going to combine args on the same line anyway, might as well put
arg 1 on the line above, then args 2 and 3 here. (multiple places)

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:246:
EXPECT_EQ(0U, bubble_content.domain_lists.size());
Nit: Why not _TRUE(...empty()) here too?

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_image_model.cc:183:
set_icon(IDR_REGISTER_PROTOCOL_HANDLER_LOCATIONBAR_ICON);
Nit: Can you set the icon and tooltip in the constructor?

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/gtk/gtk_...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_theme_service.cc:116:
IDR_REGISTER_PROTOCOL_HANDLER_LOCATIONBAR_ICON,
Hmm, I'm not convinced the geolocation icons or this one should be in these
lists.

Please verify with erg.

[Bernhard Bauer, 2012-06-26 09:08:34]

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:246:
EXPECT_EQ(0U, bubble_content.domain_lists.size());
On 2012/06/25 23:55:30, Peter Kasting wrote:
> Nit: Why not _TRUE(...empty()) here too?

An advantage of this version is that the error message tells you the actual size
if there's a mismatch.

[Peter Kasting, 2012-06-26 17:18:22]

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:246:
EXPECT_EQ(0U, bubble_content.domain_lists.size());
On 2012/06/26 09:08:34, Bernhard Bauer wrote:
> On 2012/06/25 23:55:30, Peter Kasting wrote:
> > Nit: Why not _TRUE(...empty()) here too?
> 
> An advantage of this version is that the error message tells you the actual
size
> if there's a mismatch.

I know, but several other things here (radio_items, popup_items) use empty()
instead of size() == 0, and this sticks out as inconsistent.  It's not clear why
knowing the size on this one is useful but it's not for the others.

[Greg Billock, 2012-06-26 19:17:04]

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_default_provider.cc
(right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_default_provider.cc:47:
CONTENT_SETTING_ASK,      // CONTENT_SETTINGS_TYPE_REGISTER_PROTOCOL_HANDLER
On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> Do we actually store content settings for this type in the
> HostContentSettingsMap? If not, you should set the default value to DEFAULT.

No, we're not storing them. Fixed.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
File chrome/browser/content_settings/tab_specific_content_settings.h (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
chrome/browser/content_settings/tab_specific_content_settings.h:174: void
SetPendingProtocolHandler(const ProtocolHandler& handler) {
On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> Trivial setters/getters should be named unix_hacker_style.

Done.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/content_set...
chrome/browser/content_settings/tab_specific_content_settings.h:330: // The
setting on the pending protocol handler registration.
You got it. Added clarification.

On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> I think this is only interesting if we open the bubble, change something, then
> open it again, right? Could you explain that in the comment?

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:1998: if (!user_gesture && window) {
On 2012/06/25 23:55:30, Peter Kasting wrote:
> It worries me that you NULL-check |window| here and then blindly deref it on
> line 2009.  DCHECK() it atop this function or else handle it being NULL in all
> cases (preferably the first).

You're right, that is an error. (It can be null for cases of ChromeFrame, it
looks like.)

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:133:
IDS_HANDLERS_BUBBLE_MANAGE_LINK}
On 2012/06/25 23:55:30, Peter Kasting wrote:
> On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> > Nit: Indent three more spaces?
> 
> I'd do two more.

Done.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:548:
ContentSettingRPHBubbleModel(Delegate* delegate,
Fixed up me and one neighbor. :-)

On 2012/06/25 23:55:30, Peter Kasting wrote:
> Nit: For consistency, people should de-inline class member definitions even
when
> the declaration is in a .cc file.  The whole rest of this file is also wrong,
> and I won't make you fix that (unless you want to!), but if you could separate
> this class into a declaration and definition that'd be great.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:569: if
(pending_handler_.protocol() == "webcal") {
On 2012/06/25 23:55:30, Peter Kasting wrote:
> Nit: For clarity I'd make this "else if" and put the UTF8ToUTF16(...) call
from
> above in a final "else" block.

Done.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:604: if
(setting == CONTENT_SETTING_BLOCK)
On 2012/06/25 23:55:30, Peter Kasting wrote:
> On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> > Nit: else if?
> 
> Not just a nit -- all "allow"s will get set to RPH_IGNORE without this :)

Done.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:621: if
(radio_index == RPH_ALLOW) {
On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> You can either leave out the braces or leave them in, but you should be
> consistent with above.

Done.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:234:
"mailto", GURL("http://www.toplevel.example/"),
On 2012/06/25 23:55:30, Peter Kasting wrote:
> Nit: If you're going to combine args on the same line anyway, might as well
put
> arg 1 on the line above, then args 2 and 3 here. (multiple places)

Done, but it doesn't always fit. :-(

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:246:
EXPECT_EQ(0U, bubble_content.domain_lists.size());
On 2012/06/25 23:55:30, Peter Kasting wrote:
> Nit: Why not _TRUE(...empty()) here too?

Done.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:278:
content_setting_bubble_model->OnRadioClicked(0);
On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> You could move these into constants.

Yes. I'd have to promote them out of the CC file, though, which is distasteful.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_image_model.cc:183:
set_icon(IDR_REGISTER_PROTOCOL_HANDLER_LOCATIONBAR_ICON);
On 2012/06/25 23:55:30, Peter Kasting wrote:
> Nit: Can you set the icon and tooltip in the constructor?

Done.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/gtk/gtk_...
File chrome/browser/ui/gtk/gtk_theme_service.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/gtk/gtk_...
chrome/browser/ui/gtk/gtk_theme_service.cc:116:
IDR_REGISTER_PROTOCOL_HANDLER_LOCATIONBAR_ICON,
Will do. Although Evan, do you know what these are for? Themed tinting of icons?

On 2012/06/25 23:55:30, Peter Kasting wrote:
> Hmm, I'm not convinced the geolocation icons or this one should be in these
> lists.
> 
> Please verify with erg.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:342:
"register-protocol-handler"},
On 2012/06/25 23:31:08, Evan Stade wrote:
> 2 more indent

Done.

http://codereview.chromium.org/10584042/diff/33001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:658: if (type ==
CONTENT_SETTINGS_TYPE_REGISTER_PROTOCOL_HANDLER)
On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> I think this should be moved into UpdateExceptionsViewFromModel() below (along
> with AUTO_SELECT_CERTIFICATE).

This method gets called from InitializePage. Do you mean in addition to this?

http://codereview.chromium.org/10584042/diff/33001/chrome/common/content_sett...
File chrome/common/content_settings_types.h (right):

http://codereview.chromium.org/10584042/diff/33001/chrome/common/content_sett...
chrome/common/content_settings_types.h:27:
CONTENT_SETTINGS_TYPE_REGISTER_PROTOCOL_HANDLER,
On 2012/06/25 16:52:20, Bernhard Bauer wrote:
> I think the feature itself is just called "protocol handlers", right? Naming
the
> enum just PROTOCOL_HANDLER would also make it a bit shorter :)

Done.

[Peter Kasting, 2012-06-26 19:53:00]

chrome/browser/ui/ OWNERS LGTM

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:586:
delegate, tab_contents, profile, content_type),
Nit: Indent 4

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_image_model.cc:178: 
Nit: These blank lines are optional (ContentSettingGeolocationModel seems to
omit them)

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:697: case
CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE:
Did you mean to include this?

[Bernhard Bauer, 2012-06-26 20:17:49]

LGTM!

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_utils.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_utils.cc:39:
"register-protocol-handler"
Nit: If you append a comma, it makes diffs ever so slightly easier when adding
content types in the future. The compiler doesn't complain either.

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:664:
IgnoreProtocolHandler();
Could you add an `else NOTREACHED()`?

[Peter Kasting, 2012-06-26 20:28:52]

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:664:
IgnoreProtocolHandler();
On 2012/06/26 20:17:49, Bernhard Bauer wrote:
> Could you add an `else NOTREACHED()`?

Urgh, I hate those :/

[Greg Billock, 2012-06-26 20:31:41]

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/content_set...
File chrome/browser/content_settings/content_settings_utils.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/content_set...
chrome/browser/content_settings/content_settings_utils.cc:39:
"register-protocol-handler"
On 2012/06/26 20:17:49, Bernhard Bauer wrote:
> Nit: If you append a comma, it makes diffs ever so slightly easier when adding
> content types in the future. The compiler doesn't complain either.

Done. That bit me in this merge, where it took way to long to see the
string-paste. :-/

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:586:
delegate, tab_contents, profile, content_type),
On 2012/06/26 19:53:00, Peter Kasting wrote:
> Nit: Indent 4

Not sure what you mean. These lines are all at indent 4. I agree it is a pain to
read this, but better options are >80cols.

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:664:
IgnoreProtocolHandler();
On 2012/06/26 20:17:49, Bernhard Bauer wrote:
> Could you add an `else NOTREACHED()`?

Done.

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_image_model.cc:178: 
On 2012/06/26 19:53:00, Peter Kasting wrote:
> Nit: These blank lines are optional (ContentSettingGeolocationModel seems to
> omit them)

I think they set off this text-heavy para better.

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/webui/op...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/webui/op...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:697: case
CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE:
On 2012/06/26 19:53:00, Peter Kasting wrote:
> Did you mean to include this?

Yes. This is the one you thought should to be included here as well as in
UpdateAllExceptionsViewsFromModel. I think you're right that if they're in
UpdateAll* they should be in Update* -- UpdateAll* calls this method, but other
parts of the system must as well.

[Greg Billock, 2012-06-26 20:33:27]

erg, would you mind having a look at chrome/browser/ui/gtk/gtk_theme_service.cc
? Thanks.

The question is whether we need the content settings icons included there.
(geolocation and protocol handler).

[Peter Kasting, 2012-06-26 20:35:17]

On 2012/06/26 20:33:27, Greg Billock wrote:
> erg, would you mind having a look at
chrome/browser/ui/gtk/gtk_theme_service.cc
> ? Thanks.
> 
> The question is whether we need the content settings icons included there.
> (geolocation and protocol handler).

Especially considering the other content settings icons are not listed here,
just those two.

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:586:
delegate, tab_contents, profile, content_type),
On 2012/06/26 20:31:41, Greg Billock wrote:
> On 2012/06/26 19:53:00, Peter Kasting wrote:
> > Nit: Indent 4
> 
> Not sure what you mean. These lines are all at indent 4. I agree it is a pain
to
> read this, but better options are >80cols.

I meant:

    : ContentSettingTitleAndLinkModel(
          delegate, ...

Note that "delegate" is 4 in from "ContentSetting..."

[Greg Billock, 2012-06-26 20:42:49]

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:586:
delegate, tab_contents, profile, content_type),
On 2012/06/26 20:35:17, Peter Kasting wrote:
> On 2012/06/26 20:31:41, Greg Billock wrote:
> > On 2012/06/26 19:53:00, Peter Kasting wrote:
> > > Nit: Indent 4
> > 
> > Not sure what you mean. These lines are all at indent 4. I agree it is a
pain
> to
> > read this, but better options are >80cols.
> 
> I meant:
> 
>     : ContentSettingTitleAndLinkModel(
>           delegate, ...
> 
> Note that "delegate" is 4 in from "ContentSetting..."

Ah, ok. Will do.

[Bernhard Bauer, 2012-06-26 21:06:28]

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:664:
IgnoreProtocolHandler();
On 2012/06/26 20:28:52, Peter Kasting wrote:
> On 2012/06/26 20:17:49, Bernhard Bauer wrote:
> > Could you add an `else NOTREACHED()`?
> 
> Urgh, I hate those :/

Could you explain why? The problem is that we're not passing in an enum, so
|radio_index| could have other values. It seems like a valid precondition to
check that it's one of the three.

[Peter Kasting, 2012-06-26 21:31:56]

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:569: enum
RPHState {
Nit: This goes above the methods.

Also, if you friend the (ContentSettingBubbleModelTest, RPHAllow) test, you can
use these values in it instead of the constants there, and drop the comments,
improving both readability and robustness.

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:664:
IgnoreProtocolHandler();
On 2012/06/26 21:06:28, Bernhard Bauer wrote:
> On 2012/06/26 20:28:52, Peter Kasting wrote:
> > On 2012/06/26 20:17:49, Bernhard Bauer wrote:
> > > Could you add an `else NOTREACHED()`?
> > 
> > Urgh, I hate those :/
> 
> Could you explain why? The problem is that we're not passing in an enum, so
> |radio_index| could have other values. It seems like a valid precondition to
> check that it's one of the three.

It's fine in this instance.  Never mind my comment.

[Greg Billock, 2012-06-26 22:03:40]

I added some more test machinery here. The RPH call pollutes the global web-safe
scheme registry irreversibly, so I need to prevent that from happening or other
tests fail.

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10584042/diff/43001/chrome/browser/ui/content_...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:569: enum
RPHState {
On 2012/06/26 21:31:56, Peter Kasting wrote:
> Nit: This goes above the methods.

Moved.

> 
> Also, if you friend the (ContentSettingBubbleModelTest, RPHAllow) test, you
can
> use these values in it instead of the constants there, and drop the comments,
> improving both readability and robustness.

It's in the .cc file. I agree it'd be better for the test to use the defines,
but I don't think it's worth exposing them in the header to do so.

[Elliot Glaysher, 2012-06-28 16:36:07]

gtk lgtm

[Elliot Glaysher, 2012-06-28 18:32:43]

As requested by Peter, a statement of what should be done:

In GTK, the omnibox can be have any color background, therefore any icon that
shows up against it can't assume a white background. 

If you're icon is colorful (blocked popup comes to mind), you shouldn't take any
action. If it's a monochrome icon, it needs to be tinted like the
globe/bookmark/geolocation icons and should be in the list.

Also, (I forgot to mention this before) please keep that list in sync with
chrome/browser/ui/libgtk2ui/gtk2_ui.cc, which is the version of this file that
will exist after the full gtk port is gone.

[Greg Billock, 2012-06-29 15:47:49]

On 2012/06/28 18:32:43, Elliot Glaysher wrote:
> As requested by Peter, a statement of what should be done:
> 
> In GTK, the omnibox can be have any color background, therefore any icon that
> shows up against it can't assume a white background. 
> 
> If you're icon is colorful (blocked popup comes to mind), you shouldn't take
any
> action. If it's a monochrome icon, it needs to be tinted like the
> globe/bookmark/geolocation icons and should be in the list.
> 
> Also, (I forgot to mention this before) please keep that list in sync with
> chrome/browser/ui/libgtk2ui/gtk2_ui.cc, which is the version of this file that
> will exist after the full gtk port is gone.

Added

[commit-bot: I haz the power, 2012-06-29 16:16:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10584042/62001

[commit-bot: I haz the power, 2012-06-29 17:23:32]

Try job failure for 10584042-62001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-29 18:22:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10584042/62001

[commit-bot: I haz the power, 2012-06-29 19:21:16]

Try job failure for 10584042-62001 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Greg Billock, 2012-06-29 22:24:13]

Bernhard, can you have a look at the Mac changes? The XIB file and the .mm file?
Thanks.

[Bernhard Bauer, 2012-07-02 23:13:15]

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
(right):

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:406: -
(void)initializeProtocolHandlers {
I think you could generalize |initializeRadioGroup| to deal with arbitrarily
many radio items, then use that method instead?

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:560: virtual
void OnCustomLinkClicked();
Nit: add "override"?

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:652:
RegisterProtocolHandler();
I think this method isn't called?

[Greg Billock, 2012-07-03 20:33:02]

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
(right):

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:406: -
(void)initializeProtocolHandlers {
On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> I think you could generalize |initializeRadioGroup| to deal with arbitrarily
> many radio items, then use that method instead?

Done.

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:560: virtual
void OnCustomLinkClicked();
On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> Nit: add "override"?

Done.

https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:652:
RegisterProtocolHandler();
On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> I think this method isn't called?

Done.

[Greg Billock, 2012-07-17 00:52:53]

Fixed nib file for multi-line title layout. (Thanks, Chris)

There's a spare image laying in there that Chris and I couldn't figure out why
is there. It came in with my copy-paste of another nib, in which I think it is
also superfluous.

On 2012/07/03 20:33:02, Greg Billock wrote:
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> File chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
> (right):
> 
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:406:
-
> (void)initializeProtocolHandlers {
> On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > I think you could generalize |initializeRadioGroup| to deal with arbitrarily
> > many radio items, then use that method instead?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> File chrome/browser/ui/content_settings/content_setting_bubble_model.cc
(right):
> 
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> chrome/browser/ui/content_settings/content_setting_bubble_model.cc:560:
virtual
> void OnCustomLinkClicked();
> On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > Nit: add "override"?
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> chrome/browser/ui/content_settings/content_setting_bubble_model.cc:652:
> RegisterProtocolHandler();
> On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > I think this method isn't called?
> 
> Done.

[Bernhard Bauer, 2012-07-17 08:17:13]

LGTM!111!1

On 2012/07/17 00:52:53, Greg Billock wrote:
> Fixed nib file for multi-line title layout. (Thanks, Chris)
> 
> There's a spare image laying in there that Chris and I couldn't figure out why
> is there. It came in with my copy-paste of another nib, in which I think it is
> also superfluous.

What spare image? I didn't see one in the nib file.

> On 2012/07/03 20:33:02, Greg Billock wrote:
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > File
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
> > (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> >
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:406:
> -
> > (void)initializeProtocolHandlers {
> > On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > > I think you could generalize |initializeRadioGroup| to deal with
arbitrarily
> > > many radio items, then use that method instead?
> > 
> > Done.
> > 
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > File chrome/browser/ui/content_settings/content_setting_bubble_model.cc
> (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:560:
> virtual
> > void OnCustomLinkClicked();
> > On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > > Nit: add "override"?
> > 
> > Done.
> > 
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:652:
> > RegisterProtocolHandler();
> > On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > > I think this method isn't called?
> > 
> > Done.

[Greg Billock, 2012-07-17 16:41:30]

On 2012/07/17 08:17:13, Bernhard Bauer wrote:
> LGTM!111!1
> 
> On 2012/07/17 00:52:53, Greg Billock wrote:
> > Fixed nib file for multi-line title layout. (Thanks, Chris)
> > 
> > There's a spare image laying in there that Chris and I couldn't figure out
why
> > is there. It came in with my copy-paste of another nib, in which I think it
is
> > also superfluous.
> 
> What spare image? I didn't see one in the nib file.

Chris figured this out -- it looks like it is a UI builder artifact it places on
the selected item when it saves.

> 
> > On 2012/07/03 20:33:02, Greg Billock wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > > File
> chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm
> > > (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > >
> chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa.mm:406:
> > -
> > > (void)initializeProtocolHandlers {
> > > On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > > > I think you could generalize |initializeRadioGroup| to deal with
> arbitrarily
> > > > many radio items, then use that method instead?
> > > 
> > > Done.
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > > File chrome/browser/ui/content_settings/content_setting_bubble_model.cc
> > (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:560:
> > virtual
> > > void OnCustomLinkClicked();
> > > On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > > > Nit: add "override"?
> > > 
> > > Done.
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10584042/diff/68024/chrome/browser/ui/...
> > > chrome/browser/ui/content_settings/content_setting_bubble_model.cc:652:
> > > RegisterProtocolHandler();
> > > On 2012/07/02 23:13:15, Bernhard Bauer wrote:
> > > > I think this method isn't called?
> > > 
> > > Done.

[commit-bot: I haz the power, 2012-07-17 16:55:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10584042/91003

[commit-bot: I haz the power, 2012-07-17 18:09:58]

Change committed as 147045

[Nico, 2013-03-13 17:31:56]

https://chromiumcodereview.appspot.com/10584042/diff/91003/chrome/browser/ui/...
File chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10584042/diff/91003/chrome/browser/ui/...
chrome/browser/ui/content_settings/content_setting_bubble_model_unittest.cc:292:
LOG(INFO) << "Register With OS";
What's the point of LOG(INFO) statements in tests?

The bots print LOG(INFO), which means this shows up in test output and confuses
people (well, at least me) who look at failing tests, thinking the output is
related to this output somehow.

Can you remove info logging from tests, please?