Adds navigator.registerProtocolHandler.

Adds navigator.registerProtocolHandler.


BUG=11359
TEST=None at the moment

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

[tony, 2011-02-07 20:51:44]

I realize this is a work in progress, but since you sent it for review . . .

Some general questions for future patches:
- Should we detect if we're replacing a handler and have a different info bar
int that case?
- How will the user be able to manage/remove these?

http://codereview.chromium.org/6410115/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6410115/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:11287: Would you like to use "<ph
name="HANDLER_TITLE">$1<ex>Google Search</ex></ph>" to handle "<ph
name="PROTOCOL">$2<ex>search</ex></ph>://" links from now on?
Nit: This wording could probably be improved, but this is OK for now.

http://codereview.chromium.org/6410115/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:11291: Yes
Nit: Make these more specific like "Use $1" and "Do nothing"

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:22: // FIXME(koz)
Check for bad protocol and url here.
TODO(koz) is more common in chromium code.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:23: std::string
lowerProtocol(protocol);
Nit: lower_protocol

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:25: std::cout <<
"from " << protocol << " to " << lowerProtocol << std::endl;
remove debugging code here and other places.  you could use LOG(INFO) for this.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:40: std::string
remainder = urlToMangle.substr(offset + 3);
You can use url_cannon::Replacements to change the scheme rather than mangling
the URL by hand.  It should be safer.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:55:
result.replace(result.find("%s"), 2, value);
Can we share this code with the code for search engine keywords?

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:104: Value* value =
Encode();
Can we use a scoped_ptr here?

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:22: public:
Nit: one space indent

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:23: static
ProtocolHandler* CreateProtocolHandler(const std::string& protocol, const
std::string& url, const std::string& title);
Nit: 80 cols and lots of other style violations.  See the errors on codereview
or run the presubmit scripts.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:27: const
std::string& protocol() { return protocol_; }
Nit: I am skeptical of the benefits of returning references.  I would just
return a copy.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:72: // Returns a JSON
dictionary of protocols to protocol handlers.
Nit: Maybe mention that the caller is responsible for deleting the returned
value.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:92:
~ProtocolHandlerRegistry() {};
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
File
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc
(right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:8:
#include "ui/base/resource/resource_bundle.h"
Sort these

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:52:
//      IDR_INFOBAR_AUTOFILL);
dead code

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:86:
printf("Clicked the link!\n");
debug code

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
File chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h
(right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:6:
#define CHROME_BROWSER_AUTOFILL_AUTOFILL_CC_INFOBAR_DELEGATE_H_
copy/paste error :)

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:25:
const NavigationController::LoadCommittedDetails& details) const;
You can use OVERRIDE from base/compiler_specific.h to avoid
ConfirmInfoBarDelegate from renaming methods.  I think most of these methods
could use it.  See the comment on base/compiler_specific.h for more information.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:40:
AutoFillManager* host_;
Do you still need this?

http://codereview.chromium.org/6410115/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:14: #include "base/singleton.h"
Do you need this?

http://codereview.chromium.org/6410115/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager.cc:178:
profile->InitRegisteredProtocolHandlers();
Can we do this Init in the Profile() constructor?

http://codereview.chromium.org/6410115/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.cc:2635: this,
profile()->GetProtocolHandlerRegistry(), handler));
Have you thought about what happens if an extension tries to register a protocol
handler as it's background page?  I think extension pages also implement
RenderViewHostDelegate.

http://codereview.chromium.org/6410115/diff/1/chrome/common/render_messages_i...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6410115/diff/1/chrome/common/render_messages_i...
chrome/common/render_messages_internal.h:2537: std::string /* title */)
I would pass title as string16 to avoid ambiguity in the encoding.

http://codereview.chromium.org/6410115/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:5687: const WebString& url,
indenting is weird.  what is baseUrl?  Can we pass KURL for the url?  scheme is
a subset of ASCII, right?  Maybe we can pass a WebCString instead.

[koz (OOO until 15th September), 2011-02-13 22:33:29]

On 2011/02/07 20:51:44, tony wrote:
> I realize this is a work in progress, but since you sent it for review . . .

Yes, I was looking for some feedback, thanks!

> 
> Some general questions for future patches:
> - Should we detect if we're replacing a handler and have a different info bar
> int that case?
That's a good idea, I'll put that in.

> - How will the user be able to manage/remove these?

I was thinking we could make a chrome:// url point to a config page.

[koz (OOO until 15th September), 2011-02-13 22:33:48]

http://codereview.chromium.org/6410115/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/6410115/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:11287: Would you like to use "<ph
name="HANDLER_TITLE">$1<ex>Google Search</ex></ph>" to handle "<ph
name="PROTOCOL">$2<ex>search</ex></ph>://" links from now on?
On 2011/02/07 20:51:45, tony wrote:
> Nit: This wording could probably be improved, but this is OK for now.

Yeah, I figured the UX guys would probably want to put their own thing here,
anyway.

http://codereview.chromium.org/6410115/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:11291: Yes
On 2011/02/07 20:51:45, tony wrote:
> Nit: Make these more specific like "Use $1" and "Do nothing"

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:22: // FIXME(koz)
Check for bad protocol and url here.
On 2011/02/07 20:51:45, tony wrote:
> TODO(koz) is more common in chromium code.

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:23: std::string
lowerProtocol(protocol);
On 2011/02/07 20:51:45, tony wrote:
> Nit: lower_protocol

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:25: std::cout <<
"from " << protocol << " to " << lowerProtocol << std::endl;
On 2011/02/07 20:51:45, tony wrote:
> remove debugging code here and other places.  you could use LOG(INFO) for
this.

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:40: std::string
remainder = urlToMangle.substr(offset + 3);
On 2011/02/07 20:51:45, tony wrote:
> You can use url_cannon::Replacements to change the scheme rather than mangling
> the URL by hand.  It should be safer.

Right - we'd like to clear the scheme, but it is explicitly not supported by
GURL because it "doesn't make any sense" :-)

Concatenating each of the components that we do want doesn't work either - GURL
parses something like "search://foo" as a URL with an empty host and a path of
"//foo", so instead of pulling out the "foo" we get "//foo".

Given that we are dealing with fragments of URLs it is probably safer to deal
with strings instead of getting an unexpected parse like the above from a GURL.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:55:
result.replace(result.find("%s"), 2, value);
On 2011/02/07 20:51:45, tony wrote:
> Can we share this code with the code for search engine keywords?

The search engine code just calls ReplaceSubstringsAfterOffset for this, so I've
changed my code to that.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.cc:104: Value* value =
Encode();
On 2011/02/07 20:51:45, tony wrote:
> Can we use a scoped_ptr here?

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:22: public:
On 2011/02/07 20:51:45, tony wrote:
> Nit: one space indent

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:23: static
ProtocolHandler* CreateProtocolHandler(const std::string& protocol, const
std::string& url, const std::string& title);
On 2011/02/07 20:51:45, tony wrote:
> Nit: 80 cols and lots of other style violations.  See the errors on codereview
> or run the presubmit scripts.

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:27: const
std::string& protocol() { return protocol_; }
On 2011/02/07 20:51:45, tony wrote:
> Nit: I am skeptical of the benefits of returning references.  I would just
> return a copy.

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:72: // Returns a JSON
dictionary of protocols to protocol handlers.
On 2011/02/07 20:51:45, tony wrote:
> Nit: Maybe mention that the caller is responsible for deleting the returned
> value.

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/protocol_handler_registry.h:92:
~ProtocolHandlerRegistry() {};
On 2011/02/07 20:51:45, tony wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
File
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc
(right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:8:
#include "ui/base/resource/resource_bundle.h"
On 2011/02/07 20:51:45, tony wrote:
> Sort these

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:52:
//      IDR_INFOBAR_AUTOFILL);
On 2011/02/07 20:51:45, tony wrote:
> dead code

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:86:
printf("Clicked the link!\n");
On 2011/02/07 20:51:45, tony wrote:
> debug code

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
File chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h
(right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:6:
#define CHROME_BROWSER_AUTOFILL_AUTOFILL_CC_INFOBAR_DELEGATE_H_
On 2011/02/07 20:51:45, tony wrote:
> copy/paste error :)

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:25:
const NavigationController::LoadCommittedDetails& details) const;
On 2011/02/07 20:51:45, tony wrote:
> You can use OVERRIDE from base/compiler_specific.h to avoid
> ConfirmInfoBarDelegate from renaming methods.  I think most of these methods
> could use it.  See the comment on base/compiler_specific.h for more
information.

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/custom_handlers/...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:40:
AutoFillManager* host_;
On 2011/02/07 20:51:45, tony wrote:
> Do you still need this?

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_impl.cc:14: #include "base/singleton.h"
On 2011/02/07 20:51:45, tony wrote:
> Do you need this?

Nope, not anymore. Cheers.
Done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/profiles/profile...
File chrome/browser/profiles/profile_manager.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/profiles/profile...
chrome/browser/profiles/profile_manager.cc:178:
profile->InitRegisteredProtocolHandlers();
On 2011/02/07 20:51:45, tony wrote:
> Can we do this Init in the Profile() constructor?

Yep, done.

http://codereview.chromium.org/6410115/diff/1/chrome/browser/tab_contents/tab...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/browser/tab_contents/tab...
chrome/browser/tab_contents/tab_contents.cc:2635: this,
profile()->GetProtocolHandlerRegistry(), handler));
On 2011/02/07 20:51:45, tony wrote:
> Have you thought about what happens if an extension tries to register a
protocol
> handler as it's background page?  I think extension pages also implement
> RenderViewHostDelegate.

Hm, I hadn't thought of that. I guess if the extension requests permission to
register protocol handlers it should be able to register them without a user
prompt, but maybe we don't want to let extensions register them at all?

http://codereview.chromium.org/6410115/diff/1/chrome/common/render_messages_i...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6410115/diff/1/chrome/common/render_messages_i...
chrome/common/render_messages_internal.h:2537: std::string /* title */)
On 2011/02/07 20:51:45, tony wrote:
> I would pass title as string16 to avoid ambiguity in the encoding.

Done.

http://codereview.chromium.org/6410115/diff/1/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6410115/diff/1/chrome/renderer/render_view.cc#...
chrome/renderer/render_view.cc:5687: const WebString& url,
On 2011/02/07 20:51:45, tony wrote:
> indenting is weird.  what is baseUrl?  Can we pass KURL for the url?  scheme
is
> a subset of ASCII, right?  Maybe we can pass a WebCString instead.

baseURL is the baseURI of the document of the page that RPH is called on -
actually there's a bug here because the url argument here is a relative URL
fragment.

I'll join baseUrl together with url into a GURL and pass that around (unifying
the relative and absolute url paths for registerProtocolHandler), and I'll use a
WebCString for url, too.

[tony, 2011-02-14 23:40:07]

I asked aa and arv about calling registerProtocolHandler from a background page.
 Doing nothing for now is fine (extension authors can still open a foreground
tab and call registerProtocolHandler).  In the future, we may want to add this
permission to an extension manifest.

This code looks really close to landing, the only thing I'm hesitant about right
now is exposing it before we're ready to enable it for everyone.

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

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:30: // TODO(koz)
Check for bad protocol and url here.
We should probably make sure we have this check before .registerProtocolHandler
is available on the dev channel.  I'm worried about users who will get junk in
their prefs and once checks are added, it'll be hard to fix.

Alternately, this feature might want to start out behind a command line flag
until it's a bit more mature.  If you put it behind a command line flag, try and
make sure window.registerProtocolHandler is undefined without the flag.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:36: ProtocolHandler*
ProtocolHandler::CreateProtocolHandler(const DictionaryValue* value) {
80 cols here and in lots of other places.  Please run the cpplinter.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:103: }
Remove empty else?

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:142:
reinterpret_cast<ChromeURLRequestContext*>(request->context());
I think this should be a static_cast.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.h:32: Value* Encode();
Nit: Maybe make it clear that the caller should delete the returned value.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
File
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc
(right):

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:86:
return string16();
We'll probably want a "Learn more" link for this feature (like e.g., the
geolocation info bar).  Maybe just a TODO for now.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:212:
scoped_refptr<ProtocolHandlerRegistry> protocol_handler_registry_;
What is the expected lifetime of this object?  It looks like it's currently
scoped to the lifetime of the ProfileImpl (since no one else grabs a ref?).  If
that's the case, this could probably just be a scoped_ptr.

Is there fear of ProfileImpl going away and the network stack still holding on
to the factory (maybe that's not possible?).

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:2635: return;
Nit: 2 space indent.  I would probably add a TODO(koz) about replacing existing
handlers.

http://codereview.chromium.org/6410115/diff/10004/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6410115/diff/10004/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1174:
'browser/custom_handlers/protocol_handler_registry_unittest.cc',
I don't see this file in the patch.

[koz (OOO until 15th September), 2011-02-15 03:37:27]

Thanks for the review! I've got some checks that I can put into WebKit to
address that TODO of mine about checking for bad protocols and such, after those
checks are added would you be comfortable enabling it for everyone, or is there
something else that concerns you?

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

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:30: // TODO(koz)
Check for bad protocol and url here.
On 2011/02/14 23:40:07, tony wrote:
> We should probably make sure we have this check before
.registerProtocolHandler
> is available on the dev channel.  I'm worried about users who will get junk in
> their prefs and once checks are added, it'll be hard to fix.
> 
> Alternately, this feature might want to start out behind a command line flag
> until it's a bit more mature.  If you put it behind a command line flag, try
and
> make sure window.registerProtocolHandler is undefined without the flag.

I talked with Ojan earlier about validating input and he said that it is
desirable to do all our sanitising in WebKit itself so that all ports benefit
from it. There are some checks in WebKit currently, but there are a few more
checks I would like to add, such as ensuring that only valid characters appear
in the scheme (which I'll have to research). I can't think of any checks we
would want to do for the handler URL templates (though I'd appreciate any
thoughts you have :-) ).

Currently this feature is turned on and off by flipping WebKit flags in
features.gypi and features_override.gypi, which controls whether or not the
navigator.registerProtocolHandler stub is defined. Is it possible to put
something like that behind a command line flag?

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:36: ProtocolHandler*
ProtocolHandler::CreateProtocolHandler(const DictionaryValue* value) {
On 2011/02/14 23:40:07, tony wrote:
> 80 cols here and in lots of other places.  Please run the cpplinter.

Done.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:103: }
On 2011/02/14 23:40:07, tony wrote:
> Remove empty else?

Done.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:142:
reinterpret_cast<ChromeURLRequestContext*>(request->context());
On 2011/02/14 23:40:07, tony wrote:
> I think this should be a static_cast.

Done.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.h:32: Value* Encode();
On 2011/02/14 23:40:07, tony wrote:
> Nit: Maybe make it clear that the caller should delete the returned value.

Done.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
File
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc
(right):

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:86:
return string16();
On 2011/02/14 23:40:07, tony wrote:
> We'll probably want a "Learn more" link for this feature (like e.g., the
> geolocation info bar).  Maybe just a TODO for now.

Yep, true.
Done.

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.h (right):

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.h:212:
scoped_refptr<ProtocolHandlerRegistry> protocol_handler_registry_;
On 2011/02/14 23:40:07, tony wrote:
> What is the expected lifetime of this object?  It looks like it's currently
> scoped to the lifetime of the ProfileImpl (since no one else grabs a ref?). 
If
> that's the case, this could probably just be a scoped_ptr.
> 
> Is there fear of ProfileImpl going away and the network stack still holding on
> to the factory (maybe that's not possible?).

I made this a scoped_refptr because it is referenced in
chrome_url_request_context, whose members are all reference counted. I'm not
sure if this implies that url requests can still be being handled when the
profile is cleaned up...

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6410115/diff/10004/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:2635: return;
On 2011/02/14 23:40:07, tony wrote:
> Nit: 2 space indent.  I would probably add a TODO(koz) about replacing
existing
> handlers.

Actually I've implemented the logic for showing a different message when we are
replacing a protocol handler - it's in
register_protocol_handler_infobar_delegate.cc. This line is so that if the site
tries to register an identical protocol handler to one that's already registered
then we don't bother the user with it and drop the request.

Actually, we probably want to at least notify the user of that. FireFox does
that, and if a site presents a link or button to let the user register a
protocol handler, then having no feedback for it in some cases would be
confusing.

I'll add that in this patch (it should only be one line here).

http://codereview.chromium.org/6410115/diff/10004/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/6410115/diff/10004/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:1174:
'browser/custom_handlers/protocol_handler_registry_unittest.cc',
On 2011/02/14 23:40:07, tony wrote:
> I don't see this file in the patch.

Ah yes - I deleted that unit test. It was not really useful because the
behaviour I'm trying to achieve is the result of various other parts of the
system. I'm in the process of writing a browser test for it now.

[tony, 2011-02-15 18:03:47]

LGTM!

I think you're right that we don't need a command line flag and we can just
enable this.  I thought there was more functionality to be flushed out, but I'm
wrong and this seems mostly functional now.

We should still do validation for the inputs in the browser process.  If someone
exploits a renderer, they can send arbitrary inputs back.  This can be a follow
up patch (with the browser tests).

The scoped_refptr looks correct.  I think that will keep the registry alive long
enough even if the profile goes away.

[jam, 2011-02-15 18:48:39]

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.h:25: class
ProtocolHandler {
this class should just go into its own file

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.h:54: typedef
std::map<std::string, ProtocolHandler*> ProtocolHandlerMap;
it's preferable that this typedef is inside a class or a namespace

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
File
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc
(right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:65:
if (button == BUTTON_OK)
need brace brackets for this block of code per google style guide

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h
(right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:26:
const NavigationController::LoadCommittedDetails& details) const OVERRIDE;
nit: 4 spaces needed

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:32:
ConfirmInfoBarDelegate::InfoBarButton button) const OVERRIDE;
ditto

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:483: // Register URLRequestFactories for
protocols registered with registerProtocolHandler.
nit: 80 character limit in chrome code

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:1227:
delegate_->RegisterProtocolHandler(protocol, url, title);
there's no point in dispatching the message just to call a function in
TabContents through the RenderViewHostDelegate interface.  Just dispatch the
message in TabContents directly

http://codereview.chromium.org/6410115/diff/17002/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/common/render_messag...
chrome/common/render_messages_internal.h:2532: // Register protocol handler
--------------------------------------------------
nit: most comments don't have the dashes at the end, and there's an extra
newline.  please just add a grammatically correct sentence like most of the
comments

http://codereview.chromium.org/6410115/diff/17002/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:5689: GURL base(baseUrl);
nit:
-two space indents
-parameter names need to use chromium's unix_hacker style, i.e. base_url

http://codereview.chromium.org/6410115/diff/17002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:5693: routing_id_, UTF16ToUTF8(scheme),
absoluteUrl, title));
nit: the third line needs to be indented 4 spaces after the one before it

[koz (OOO until 15th September), 2011-02-16 03:37:48]

Thanks for the review, John!

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.h:25: class
ProtocolHandler {
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> this class should just go into its own file

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.h:54: typedef
std::map<std::string, ProtocolHandler*> ProtocolHandlerMap;
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> it's preferable that this typedef is inside a class or a namespace

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
File
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc
(right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.cc:65:
if (button == BUTTON_OK)
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> need brace brackets for this block of code per google style guide

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h
(right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:26:
const NavigationController::LoadCommittedDetails& details) const OVERRIDE;
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> nit: 4 spaces needed

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/register_protocol_handler_infobar_delegate.h:32:
ConfirmInfoBarDelegate::InfoBarButton button) const OVERRIDE;
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> ditto

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:483: // Register URLRequestFactories for
protocols registered with registerProtocolHandler.
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> nit: 80 character limit in chrome code

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_view_host.cc:1227:
delegate_->RegisterProtocolHandler(protocol, url, title);
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> there's no point in dispatching the message just to call a function in
> TabContents through the RenderViewHostDelegate interface.  Just dispatch the
> message in TabContents directly

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/common/render_messag...
chrome/common/render_messages_internal.h:2532: // Register protocol handler
--------------------------------------------------
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> nit: most comments don't have the dashes at the end, and there's an extra
> newline.  please just add a grammatically correct sentence like most of the
> comments

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6410115/diff/17002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:5689: GURL base(baseUrl);
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> nit:
> -two space indents
> -parameter names need to use chromium's unix_hacker style, i.e. base_url

Done.

http://codereview.chromium.org/6410115/diff/17002/chrome/renderer/render_view...
chrome/renderer/render_view.cc:5693: routing_id_, UTF16ToUTF8(scheme),
absoluteUrl, title));
On 2011/02/15 18:48:39, John Abd-El-Malek wrote:
> nit: the third line needs to be indented 4 spaces after the one before it

Done.

[koz (OOO until 15th September), 2011-02-16 03:51:37]

Great! Thanks for reviewing this and for answering my numerous questions along
the way, it's much appreciated :-)

I'll continue to work on the follow-up patches.

On 2011/02/15 18:03:47, tony wrote:
> LGTM!
> 
> I think you're right that we don't need a command line flag and we can just
> enable this.  I thought there was more functionality to be flushed out, but
I'm
> wrong and this seems mostly functional now.
> 
> We should still do validation for the inputs in the browser process.  If
someone
> exploits a renderer, they can send arbitrary inputs back.  This can be a
follow
> up patch (with the browser tests).
> 
> The scoped_refptr looks correct.  I think that will keep the registry alive
long
> enough even if the profile goes away.

[jam, 2011-02-16 18:29:37]

lgtm

http://codereview.chromium.org/6410115/diff/19002/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/protocol_handler.cc (right):

http://codereview.chromium.org/6410115/diff/19002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler.cc:10: 
nit: extra line

[koz (OOO until 15th September), 2011-02-17 03:32:03]

http://codereview.chromium.org/6410115/diff/19002/chrome/browser/custom_handl...
File chrome/browser/custom_handlers/protocol_handler.cc (right):

http://codereview.chromium.org/6410115/diff/19002/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler.cc:10: 
On 2011/02/16 18:29:37, John Abd-El-Malek wrote:
> nit: extra line

Removed, cheers.

[tony, 2011-02-22 17:52:08]

LGTM2

http://codereview.chromium.org/6410115/diff/25001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/6410115/diff/25001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:259:
context->set_protocol_handler_registry(profile_params.protocol_handler_registry);
Nit: 80 cols

[willchan no longer on Chromium, 2011-02-24 20:57:10]

Hey, I just saw this due to a refcounting bug in it. I don't see anyone in
chrome/browser/net/OWNERS on the reviewers list here, yet
ChromeURLRequestContext is being changed. The refcounting needs to be
threadsafe. Also, it looks like you're doing a
static_cast<ChromeURLRequestContext*>(request->context()) here. That's wrong.
Anyone trying to test out the URI via chrome://net-internals/#tests is going to
create a subtype of URLRequestContext that is NOT ChromeURLRequestContext, and
it will crash on your attempt to cast it to ChromeURLRequestContext.

It looks like you are trying to use URLRequest::ProtocolFactory. I recommend
that when you try to use the net/ library, you ask someone in net/OWNERS to see
if your usage is correct. Your current usage will lead to crashes. Can you fix
this?

[tony, 2011-02-24 21:35:24]

The refcount bug is tracked by
http://code.google.com/p/chromium/issues/detail?id=73972 .

Can you open another bug for the bad cast?

On 2011/02/24 20:57:10, willchan wrote:
> Hey, I just saw this due to a refcounting bug in it. I don't see anyone in
> chrome/browser/net/OWNERS on the reviewers list here, yet
> ChromeURLRequestContext is being changed. The refcounting needs to be
> threadsafe. Also, it looks like you're doing a
> static_cast<ChromeURLRequestContext*>(request->context()) here. That's wrong.
> Anyone trying to test out the URI via chrome://net-internals/#tests is going
to
> create a subtype of URLRequestContext that is NOT ChromeURLRequestContext, and
> it will crash on your attempt to cast it to ChromeURLRequestContext.
> 
> It looks like you are trying to use URLRequest::ProtocolFactory. I recommend
> that when you try to use the net/ library, you ask someone in net/OWNERS to
see
> if your usage is correct. Your current usage will lead to crashes. Can you fix
> this?

[willchan no longer on Chromium, 2011-02-24 22:00:21]

Filed http://code.google.com/p/chromium/issues/detail?id=74063.

On Thu, Feb 24, 2011 at 1:35 PM,  <tony@chromium.org> wrote:
> The refcount bug is tracked by
> http://code.google.com/p/chromium/issues/detail?id=73972 .
>
> Can you open another bug for the bad cast?
>
> On 2011/02/24 20:57:10, willchan wrote:
>>
>> Hey, I just saw this due to a refcounting bug in it. I don't see anyone in
>> chrome/browser/net/OWNERS on the reviewers list here, yet
>> ChromeURLRequestContext is being changed. The refcounting needs to be
>> threadsafe. Also, it looks like you're doing a
>> static_cast<ChromeURLRequestContext*>(request->context()) here. That's
>> wrong.
>> Anyone trying to test out the URI via chrome://net-internals/#tests is
>> going
>
> to
>>
>> create a subtype of URLRequestContext that is NOT ChromeURLRequestContext,
>> and
>> it will crash on your attempt to cast it to ChromeURLRequestContext.
>
>> It looks like you are trying to use URLRequest::ProtocolFactory. I
>> recommend
>> that when you try to use the net/ library, you ask someone in net/OWNERS
>> to
>
> see
>>
>> if your usage is correct. Your current usage will lead to crashes. Can you
>> fix
>> this?
>
>
>
> http://codereview.chromium.org/6410115/
>