Handler settings page.

Handler settings page.

This change implements a settings page that allows users to manage protocol
handlers registered via navigator.registerProtocolHandler.

tony: could you review the ProtocolHandlerRegistry stuff?
estade: could you review the webui stuff?

Thanks!

TEST=Unit tests provided.

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

[koz (OOO until 15th September), 2011-05-19 06:33:30]

Also, I'd like to put this behind a flag defined in WebKit, which means I need
to reference the flag from js and html. Is that possible? And if so, could
someone point me to a place where something like this is done already?

I see pp_ifdef() but I'm not sure what values I can use it with.

Also note, registerProtocolHandler is currently broken, but that is orthogonal
to this patch. I plan to fix that before landing this.

[Evan Stade, 2011-05-19 17:08:42]

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
File chrome/browser/resources/options/handler_options.css (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.css:1: #handlers-column-headers
{
I'm not sure why so many css files are missing their license headers, but it's
probably safer to add one

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.css:30: color: #555;
alphabetize attributes (hyphens at the end)

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
File chrome/browser/resources/options/handler_options.html (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.html:5: <div
class="handlers-site-column"><h3
i18n-content="handlers_site_column_header"></h3></div>
80

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
File chrome/browser/resources/options/handler_options.js (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.js:54: * Call to remove a
handler.
describe what the function does ("Call to remove a handler" is ambiguous: Who
should call it? Remove from what?)

on the other hand this function probably isn't necessary, why not just use
chrome.send directly

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.js:61:
HandlerOptions.setDefault = function(handler) {
function comment

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.js:79: 
remove excess newline

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
File chrome/browser/resources/options/handler_options_list.js (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:37: function
makeDiv(text) {
I don't think there's much point to this function. Just use
document.createElement on L55 and then set the inner text on L75.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:39: if (text) {
no curlies

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:59: for (var i in
data.handlers) {
don't use for...in for lists

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:77:
handlerElement.classList.add('handlers-site-column');
handlerElement.classname = 'handlers-site-column';

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:82:
makeDiv(localStrings.getString('handlers_remove_link'));
2 more spaces indent

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:88:
removeElement.classList.add('handlers-remove-column');
removeElement.className = 'handlers-remove-column handlers-remove-link';

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:133: setHandlers:
function(list) {
function comment

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options/handler_options_handler.cc (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.cc:71:
ProtocolHandlerRegistry::ProtocolHandlerList::const_iterator p;
variable names should be descriptive (p is not) (here and elsewhere)

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.cc:89: p !=
protocols.end(); p++) {
one more space of indent

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.cc:113: if
(!args->GetBoolean(0, &enabled)) {
prefer CHECK (here and elsewhere)

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.cc:117: if (enabled) {
no curlies (here and elsewhere)

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options/handler_options_handler.h (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.h:41: // @param args a
list of [protocol, url, title].
@param is only used for javascript. C++ style owuld be

|args| is a list of ...

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.h:53: void
RemoveHandler(const ListValue* args);
I'm wary of using the index as the identifier because it seems prone to race
conditions

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.h:55:
ProtocolHandlerRegistry* GetProtocolHandlerRegistry();
newline

[koz (OOO until 15th September), 2011-05-19 21:07:52]

Thanks for the prompt and thorough review, Evan.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
File chrome/browser/resources/options/handler_options.css (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.css:1: #handlers-column-headers
{
On 2011/05/19 17:08:42, Evan Stade wrote:
> I'm not sure why so many css files are missing their license headers, but it's
> probably safer to add one

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.css:30: color: #555;
On 2011/05/19 17:08:42, Evan Stade wrote:
> alphabetize attributes (hyphens at the end)

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
File chrome/browser/resources/options/handler_options.html (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.html:5: <div
class="handlers-site-column"><h3
i18n-content="handlers_site_column_header"></h3></div>
On 2011/05/19 17:08:42, Evan Stade wrote:
> 80

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
File chrome/browser/resources/options/handler_options.js (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.js:54: * Call to remove a
handler.
On 2011/05/19 17:08:42, Evan Stade wrote:
> describe what the function does ("Call to remove a handler" is ambiguous: Who
> should call it? Remove from what?)
> 
> on the other hand this function probably isn't necessary, why not just use
> chrome.send directly

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.js:61:
HandlerOptions.setDefault = function(handler) {
On 2011/05/19 17:08:42, Evan Stade wrote:
> function comment

I inilned this function as well.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options.js:79: 
On 2011/05/19 17:08:42, Evan Stade wrote:
> remove excess newline

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
File chrome/browser/resources/options/handler_options_list.js (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:37: function
makeDiv(text) {
On 2011/05/19 17:08:42, Evan Stade wrote:
> I don't think there's much point to this function. Just use
> document.createElement on L55 and then set the inner text on L75.

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:39: if (text) {
On 2011/05/19 17:08:42, Evan Stade wrote:
> no curlies

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:59: for (var i in
data.handlers) {
On 2011/05/19 17:08:42, Evan Stade wrote:
> don't use for...in for lists

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:77:
handlerElement.classList.add('handlers-site-column');
On 2011/05/19 17:08:42, Evan Stade wrote:
> handlerElement.classname = 'handlers-site-column';

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:82:
makeDiv(localStrings.getString('handlers_remove_link'));
On 2011/05/19 17:08:42, Evan Stade wrote:
> 2 more spaces indent

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:88:
removeElement.classList.add('handlers-remove-column');
On 2011/05/19 17:08:42, Evan Stade wrote:
> removeElement.className = 'handlers-remove-column handlers-remove-link';

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/resources/optio...
chrome/browser/resources/options/handler_options_list.js:133: setHandlers:
function(list) {
On 2011/05/19 17:08:42, Evan Stade wrote:
> function comment

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options/handler_options_handler.cc (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.cc:71:
ProtocolHandlerRegistry::ProtocolHandlerList::const_iterator p;
On 2011/05/19 17:08:42, Evan Stade wrote:
> variable names should be descriptive (p is not) (here and elsewhere)

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.cc:89: p !=
protocols.end(); p++) {
On 2011/05/19 17:08:42, Evan Stade wrote:
> one more space of indent

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.cc:113: if
(!args->GetBoolean(0, &enabled)) {
On 2011/05/19 17:08:42, Evan Stade wrote:
> prefer CHECK (here and elsewhere)

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.cc:117: if (enabled) {
On 2011/05/19 17:08:42, Evan Stade wrote:
> no curlies (here and elsewhere)

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
File chrome/browser/ui/webui/options/handler_options_handler.h (right):

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.h:41: // @param args a
list of [protocol, url, title].
On 2011/05/19 17:08:42, Evan Stade wrote:
> @param is only used for javascript. C++ style owuld be
> 
> |args| is a list of ...

Done.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.h:53: void
RemoveHandler(const ListValue* args);
On 2011/05/19 17:08:42, Evan Stade wrote:
> I'm wary of using the index as the identifier because it seems prone to race
> conditions

Ah sorry, the comment was out of date here.
Fixed.

http://codereview.chromium.org/7033018/diff/25/chrome/browser/ui/webui/option...
chrome/browser/ui/webui/options/handler_options_handler.h:55:
ProtocolHandlerRegistry* GetProtocolHandlerRegistry();
On 2011/05/19 17:08:42, Evan Stade wrote:
> newline

Done.

[Evan Stade, 2011-05-20 22:01:28]

webui parts LGTM with additional nits

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.js:70:
ContentSettings.updateHandlersEnabledRadios = function(enabled) {
var selector = '#handlers-section input[type=radio][value=' + (enabled ? 'allow'
: 'block') + ']';
document.querySelector(selector).checked = true;

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
File chrome/browser/resources/options/handler_options.js (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options.js:61: // Export
not sure this comment is necessary, but if you want to add it please use it in
the other file as well.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options.js:65: 
remove this newline

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
File chrome/browser/resources/options/handler_options_list.js (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:24: * protocol.
indent protocol 4 spaces

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:54: if (i ==
data.default_handler) {
optionElement.selected = i == data.default_handler;

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:60: }
insert newline

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:65: handlerElement =
document.createElement('div');
this line can be shared at handlerElement declaration time, no?

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/handler_options_handler.h (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.h:9: #include <vector>
include this in the cc file

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.h:41: // |args| a list
of [protocol, url, title].
nit: |args| _is_ a list...

[koz (OOO until 15th September), 2011-05-20 22:19:56]

Awesome, thanks!

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.js:70:
ContentSettings.updateHandlersEnabledRadios = function(enabled) {
On 2011/05/20 22:01:28, Evan Stade wrote:
> var selector = '#handlers-section input[type=radio][value=' + (enabled ?
'allow'
> : 'block') + ']';
> document.querySelector(selector).checked = true;

Done.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
File chrome/browser/resources/options/handler_options.js (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options.js:61: // Export
On 2011/05/20 22:01:28, Evan Stade wrote:
> not sure this comment is necessary, but if you want to add it please use it in
> the other file as well.

I probably got this comment from another *_options.js file, I'll remove it.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options.js:65: 
On 2011/05/20 22:01:28, Evan Stade wrote:
> remove this newline

Done.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
File chrome/browser/resources/options/handler_options_list.js (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:24: * protocol.
On 2011/05/20 22:01:28, Evan Stade wrote:
> indent protocol 4 spaces

Done.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:54: if (i ==
data.default_handler) {
On 2011/05/20 22:01:28, Evan Stade wrote:
> optionElement.selected = i == data.default_handler;

Done.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:60: }
On 2011/05/20 22:01:28, Evan Stade wrote:
> insert newline

Done.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/resources/opt...
chrome/browser/resources/options/handler_options_list.js:65: handlerElement =
document.createElement('div');
On 2011/05/20 22:01:28, Evan Stade wrote:
> this line can be shared at handlerElement declaration time, no?

Done.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/handler_options_handler.h (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.h:9: #include <vector>
On 2011/05/20 22:01:28, Evan Stade wrote:
> include this in the cc file

Done.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.h:41: // |args| a list
of [protocol, url, title].
On 2011/05/20 22:01:28, Evan Stade wrote:
> nit: |args| _is_ a list...

Done.

[willchan no longer on Chromium, 2011-05-20 22:48:50]

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:392: void
ProtocolHandlerRegistry::Notify(NotificationType type) {
What thread is Notify() being called on? That my main concern with these
changes. I think that for these notifications it needs to occur on the UI
thread. It's hard for me to verify here. It might be easier documentation if you
used DCHECK(BrowserThread::IsCurrentlyOn(BrowserThread::UI)) at the beginning of
functions to document which thread it's being accessed from.

Oh, and this doesn't look like it's part of the Delegate, so perhaps it should
move elsewhere? I think people like to have the .cc file mirror the order of the
.h file.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:18: 
spurious?

[koz (OOO until 15th September), 2011-05-20 23:18:57]

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:392: void
ProtocolHandlerRegistry::Notify(NotificationType type) {
On 2011/05/20 22:48:50, willchan wrote:
> What thread is Notify() being called on? That my main concern with these
> changes. I think that for these notifications it needs to occur on the UI
> thread. It's hard for me to verify here. It might be easier documentation if
you
> used DCHECK(BrowserThread::IsCurrentlyOn(BrowserThread::UI)) at the beginning
of
> functions to document which thread it's being accessed from.
> 
> Oh, and this doesn't look like it's part of the Delegate, so perhaps it should
> move elsewhere? I think people like to have the .cc file mirror the order of
the
> .h file.

Ah, I wasn't aware that the thread that Notify() is called from is significant.
I think all the NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED ones should
happen from the UI thread, but
NotificationType::PROTOCOL_HANDLER_REGISTRY_LOADED gets called from wherever the
profile is initialised from.

NotificationType::PROTOCOL_HANDLER_REGISTRY_LOADED is unused though, so I've
removed it.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:18: 
On 2011/05/20 22:48:50, willchan wrote:
> spurious?

Done.

[willchan no longer on Chromium, 2011-05-21 00:06:05]

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:55:
Notify(NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED);
Which thread does this happen on? Is this the UI thread? Or is it due to an IPC
on the IO thread?

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:86:
Notify(NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED);
Looks like this is probably the UI thread, right? In that case this is probably
kosher.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:245:
Notify(NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED);
What thread is this handled on?

[tony, 2011-05-23 21:42:00]

http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5324: Type
Nit: Content Type (to match Firefox)

http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5330: remove this site
Should this be capitalized?  Should this just be an X to remove a site (see
Locations or Notifications)?

http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5432: Manage Handlers...
If we can only manage exceptions, should this just use the existing
IDS_EXCEPTIONS_MANAGE?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:63:
p->second.push_back(handler);
Do you need to check for duplicates?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:192:
std::find(ignored_protocol_handlers_.begin(),
Nit: 4 space indent when wrapping a long line

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:198:
Notify(NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED);
Should Save() and Notify() be in the if?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:202: const
ProtocolHandler& handler) const {
This indenting is funny.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:368: void
ProtocolHandlerRegistry::Notify(NotificationType type) {
I would drop type since you always pass in PROTOCOL_HANDLER_REGISTRY_CHANGED. 
Maybe rename this function to NotifyChanged or something.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:287: }
Can we add some notification tests?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.js:73:
document.querySelector(selector).checked = true;
Nit: indent is off

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings_ui.js (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings_ui.js:48:
chrome.send('setHandlersEnabled', [this.value == 'allow']);
Why doesn't this use the ContentSettingsRadio/setContentFilter like all the
other sections on this page?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/options.html:22: <link rel="stylesheet"
href="handler_options.css">
Nit: alphabetize?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:279:
UpdateHandlersEnabledRadios();
Should this be part of UpdateAllExceptionsViewsFromModel?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:403:
GetProtocolHandlerRegistry()->enabled());
It looks like this is leaked.  Can you just stack allocate handlers_enabled?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/handler_options_handler.cc (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.cc:28: {
"handlers_tab_label", IDS_HANDLERS_TAB_LABEL },
Nit: I think we normally 4 space indent stuff like this.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.cc:140: ok = ok &&
args->GetString(2, &title);
This could just be:
bool ok = args->GetString(0, &protocol) && args->GetString(1, &url) &&
args->GetString(2, &title);

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/handler_options_handler.h (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.h:19: explicit
HandlerOptionsHandler();
Nit: no explicit

[koz (OOO until 15th September), 2011-05-24 08:47:49]

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:55:
Notify(NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED);
On 2011/05/21 00:06:05, willchan wrote:
> Which thread does this happen on? Is this the UI thread? Or is it due to an
IPC
> on the IO thread?

It will be called on Load(), but in that case it doesn't fire a notification.
The other time is when the user clicks on an infobar.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:86:
Notify(NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED);
On 2011/05/21 00:06:05, willchan wrote:
> Looks like this is probably the UI thread, right? In that case this is
probably
> kosher.

Yep, it's called when the user toggles the checkbox on the settings page.

http://codereview.chromium.org/7033018/diff/4003/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:245:
Notify(NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED);
On 2011/05/21 00:06:05, willchan wrote:
> What thread is this handled on?

The UI thread - it's called when the user clicks the "remove this site" link for
a protocol handler.

http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5324: Type
On 2011/05/23 21:42:00, tony wrote:
> Nit: Content Type (to match Firefox)

I'll defer to Glen on this one.

http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5330: remove this site
On 2011/05/23 21:42:00, tony wrote:
> Should this be capitalized?  Should this just be an X to remove a site (see
> Locations or Notifications)?

I'm not sure - currently the design is faithful to Glen's mocks. I'll add him to
this review.

http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:5432: Manage Handlers...
On 2011/05/23 21:42:00, tony wrote:
> If we can only manage exceptions, should this just use the existing
> IDS_EXCEPTIONS_MANAGE?

Unfortunately this is a "one of these is not like the others" situation - we
aren't managing exceptions here, we're managing what handlers are registered. I
guess you could construe them as exceptions to the regular flow of handling urls
with unknown protocols...

Again, I defer to Glen on this :-)

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:63:
p->second.push_back(handler);
On 2011/05/23 21:42:00, tony wrote:
> Do you need to check for duplicates?

I don't think so - this is private and only called by the above
RegisterProtocolHandler(), which performs the dupe check. Do you think I should
inline it?

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:192:
std::find(ignored_protocol_handlers_.begin(),
On 2011/05/23 21:42:00, tony wrote:
> Nit: 4 space indent when wrapping a long line

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:198:
Notify(NotificationType::PROTOCOL_HANDLER_REGISTRY_CHANGED);
On 2011/05/23 21:42:00, tony wrote:
> Should Save() and Notify() be in the if?

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:202: const
ProtocolHandler& handler) const {
On 2011/05/23 21:42:00, tony wrote:
> This indenting is funny.

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:368: void
ProtocolHandlerRegistry::Notify(NotificationType type) {
On 2011/05/23 21:42:00, tony wrote:
> I would drop type since you always pass in PROTOCOL_HANDLER_REGISTRY_CHANGED. 
> Maybe rename this function to NotifyChanged or something.

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:287: }
On 2011/05/23 21:42:00, tony wrote:
> Can we add some notification tests?

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings.js:73:
document.querySelector(selector).checked = true;
On 2011/05/23 21:42:00, tony wrote:
> Nit: indent is off

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/content_settings_ui.js (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/content_settings_ui.js:48:
chrome.send('setHandlersEnabled', [this.value == 'allow']);
On 2011/05/23 21:42:00, tony wrote:
> Why doesn't this use the ContentSettingsRadio/setContentFilter like all the
> other sections on this page?

We're managing a different kind of data here - the other sections all share a
blacklist/wildcard kind of data, here we are dealing with a map from protocols
to lists of protocol handlers.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/resources/opt...
chrome/browser/resources/options/options.html:22: <link rel="stylesheet"
href="handler_options.css">
On 2011/05/23 21:42:00, tony wrote:
> Nit: alphabetize?

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:279:
UpdateHandlersEnabledRadios();
On 2011/05/23 21:42:00, tony wrote:
> Should this be part of UpdateAllExceptionsViewsFromModel?

I think not, because it's not an exceptions view.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/content_settings_handler.cc:403:
GetProtocolHandlerRegistry()->enabled());
On 2011/05/23 21:42:00, tony wrote:
> It looks like this is leaked.  Can you just stack allocate handlers_enabled?

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/handler_options_handler.cc (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.cc:28: {
"handlers_tab_label", IDS_HANDLERS_TAB_LABEL },
On 2011/05/23 21:42:00, tony wrote:
> Nit: I think we normally 4 space indent stuff like this.

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.cc:140: ok = ok &&
args->GetString(2, &title);
On 2011/05/23 21:42:00, tony wrote:
> This could just be:
> bool ok = args->GetString(0, &protocol) && args->GetString(1, &url) &&
> args->GetString(2, &title);

Done.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/handler_options_handler.h (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/handler_options_handler.h:19: explicit
HandlerOptionsHandler();
On 2011/05/23 21:42:00, tony wrote:
> Nit: no explicit

Done.

[koz (OOO until 15th September), 2011-05-24 08:49:32]

Added glen as reviewer.

[tony, 2011-05-24 18:16:13]

LGTM.  My nits with the UI shouldn't block this patch landing.  My biggest UI
nit is the use of "remove this site" instead of an X.

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/7005/chrome/browser/custom_handle...
chrome/browser/custom_handlers/protocol_handler_registry.cc:63:
p->second.push_back(handler);
On 2011/05/24 08:47:49, koz wrote:
> On 2011/05/23 21:42:00, tony wrote:
> > Do you need to check for duplicates?
> 
> I don't think so - this is private and only called by the above
> RegisterProtocolHandler(), which performs the dupe check. Do you think I
should
> inline it?

Ah, I see, this is fine as is.

[Glen Murphy, 2011-05-24 23:30:00]

>
http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
> File chrome/app/generated_resources.grd (right):
> 
>
http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:5324: Type
> On 2011/05/23 21:42:00, tony wrote:
> > Nit: Content Type (to match Firefox)
> 
> I'll defer to Glen on this one.

Let's leave it as 'Type' - 'Content Type' doesn't add anything.


>
http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:5330: remove this site
> On 2011/05/23 21:42:00, tony wrote:
> > Should this be capitalized?  Should this just be an X to remove a site (see
> > Locations or Notifications)?
> 
> I'm not sure - currently the design is faithful to Glen's mocks. I'll add him
to
> this review.

This should probably be an 'X' - sorry for the poor mocks. It's OK if you do
this in a separate CL, and it shouldn't block landing of this patch for M13.


>
http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
> chrome/app/generated_resources.grd:5432: Manage Handlers...
> On 2011/05/23 21:42:00, tony wrote:
> > If we can only manage exceptions, should this just use the existing
> > IDS_EXCEPTIONS_MANAGE?
> 
> Unfortunately this is a "one of these is not like the others" situation - we
> aren't managing exceptions here, we're managing what handlers are registered.
I
> guess you could construe them as exceptions to the regular flow of handling
urls
> with unknown protocols...
> 
> Again, I defer to Glen on this :-)

"Manage Handlers..." is right.

[koz (OOO until 15th September), 2011-05-25 05:39:31]

Adding jam@ for content/common/notification_type.h.

[jam, 2011-05-25 16:18:29]

http://codereview.chromium.org/7033018/diff/19001/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/19001/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:373: if
(!in_unit_test_)
you don't need to change the code for this.  a lot of unit tests get around
BrowserThread::CurrentlyOn checks by creating fake BrowserThreads.  see an
example at
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/content/brows...

[koz (OOO until 15th September), 2011-05-25 21:32:31]

http://codereview.chromium.org/7033018/diff/19001/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/7033018/diff/19001/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:373: if
(!in_unit_test_)
On 2011/05/25 16:18:30, John Abd-El-Malek wrote:
> you don't need to change the code for this.  a lot of unit tests get around
> BrowserThread::CurrentlyOn checks by creating fake BrowserThreads.  see an
> example at
>
http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/content/brows...

Thanks for the tip! Done.

[jam, 2011-05-25 23:26:53]

lgtm

[Glen Murphy, 2011-05-27 20:48:04]

>
>
>
>
http://codereview.chromium.org/7033018/diff/7005/chrome/app/generated_resourc...
>
>> chrome/app/generated_resources.grd:5330: remove this site
>> On 2011/05/23 21:42:00, tony wrote:
>> > Should this be capitalized?  Should this just be an X to remove a site
>> (see
>> > Locations or Notifications)?
>>
>
>  I'm not sure - currently the design is faithful to Glen's mocks. I'll add
>> him
>>
> to
>
>> this review.
>>
>
> This should probably be an 'X' - sorry for the poor mocks. It's OK if you
> do
> this in a separate CL, and it shouldn't block landing of this patch for
> M13.


Ohh, I just took a look at the mocks - the 'remove this site' instead of an
'X' was intentional because it doesn't always remove the row (as the other
Xes do), just the selected item in the multiple handlers case.