Convert ProtocolHandlerRegistry to be a ProfileKeyedService.

Convert ProtocolHandlerRegistry to be a ProfileKeyedService.

BUG=129200
TEST=protocol_handler_registry_browsertest.cc,protocol_handler_registry_unittest.cc

willchan@ -> profile_io changes
+bauerb -> content_settings changes


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

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

[Steve McKay, 2012-06-09 01:03:38]

Sending off to the try server. I think things are working correctly now provided
everyone is okay with the ProtocolHandlerRegistry being destroyed on the IO
thread.

[James Hawkins, 2012-06-10 22:57:51]

High-level review comments:
* I think this CL needs to be cut down; it feels unnecessarily large, e.g. is it
necessary to add the ProtocolHandlerRegistryFactory in this CL?
* The number of TODOs added makes me uncomfortable; why are there so many
unresolved issues?  I'd prefer most of them be resolved before committing.

[Peter Kasting, 2012-06-11 00:41:26]

Drive-by.  Destroying a PKS on the IO thread sounds scary to me.  You may want
to ensure one of the core Profile wranglers like erg or rlp looks at this if
they haven't already.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:21: 
Nit: Extra blank lines (3 places)

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:41: new
ProtocolHandlerRegistry::Delegate());
Nit: Could put on previous line

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:49: return
false;
This is already the default behavior.

[Elliot Glaysher, 2012-06-11 19:43:16]

I shouldn't comment on the threading issues, but here are some points on your
service:

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry.cc:564: void
ProtocolHandlerRegistry::Finalize() {
Rename Finalize() to Shutdown(), which is part of the PKS interface. Finalize()
was called during ~ProfileImpl(), but before the destruction phase starts.
Renaming this to Shutdown() will have this happen at the equivalent time, when
the rest of the object graph is still live.

(Unless you're really sure that you want to change this so that it runs during
destruction instead.)

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry.cc:588: Finalize();
(And remove this call)

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:30: //
DependsOn(WebIntentsRegistryFactory::GetInstance());
Is this supposed to be uncommented? I see that WebIntentsRegistryFactory exists.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:263:
registry_->Finalize();
(And change the name here)

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/test/base/te...
File chrome/test/base/testing_profile.cc (right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/test/base/te...
chrome/test/base/testing_profile.cc:667: return protocol_handler_registry_;
Edit: Now that I look at this, you may just be able to return NULL here;
CreateProtocolHandlerRegistry() looks like dead code.

Previous: There's testing infrastructure for conditionally building a service
during only certain tests. See uses of
ProfileKeyedServiceFactory::SetTestingFactory() for more details. (Or look at
how it's used in autofill_manager_unittest.cc)

[willchan no longer on Chromium, 2012-06-11 21:17:22]

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry.cc:589:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
I think this class is used on both the UI and IO thread. Are you doing anything
to make sure it's not concurrently being used on the IO thread when this is
being deleted? If not, it'll probably lead to race conditions.

[Steve McKay, 2012-06-11 22:09:38]

Okay. Still trying to figure out if there's a threading issue. There are no
immediate behavioral indications (tests pass, browser runs, exists...), but
willchan is cautious and possibly concerned.

Else, I've addressed code review comments and the CL is ready for another look.

willchan, please see my comments inline.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry.cc:564: void
ProtocolHandlerRegistry::Finalize() {
On 2012/06/11 19:43:16, Elliot Glaysher wrote:
> Rename Finalize() to Shutdown(), which is part of the PKS interface.
Finalize()
> was called during ~ProfileImpl(), but before the destruction phase starts.
> Renaming this to Shutdown() will have this happen at the equivalent time, when
> the rest of the object graph is still live.
> 
> (Unless you're really sure that you want to change this so that it runs during
> destruction instead.)

Done.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry.cc:588: Finalize();
On 2012/06/11 19:43:16, Elliot Glaysher wrote:
> (And remove this call)

Done.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry.cc:589:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/06/11 21:17:22, willchan wrote:
> I think this class is used on both the UI and IO thread. Are you doing
anything
> to make sure it's not concurrently being used on the IO thread when this is
> being deleted? If not, it'll probably lead to race conditions.

So what I need to tease apart here, for my understanding and because
WebIntentsRegistry uses a ProfileKeyedService as well, is the distinction
between the overall design of the class and it's consumers and the changes I'm
making (switching to a PKS). I'm not doing anything new in this CL to
orchestrate thread access to this class. If there's a race condition, can you
help me understand if it is intrinsic to the design or if emerges from the
refactoring in this CL.

In either case if there is a race condition I would appreciate your advice on
how I can remedy this (again, I'm new to Chrome and even C++ and I might require
a little more hand holding than a seasoned Chrome engineer :)

Thanks for your help.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:21: 
On 2012/06/11 00:41:26, Peter Kasting wrote:
> Nit: Extra blank lines (3 places)

Done. Though allow me to proselytize for more whitespace a bit. An empty line
after a line wrap makes it easier to distinguish content from the wrapped
content preceding it, especially so in the middle of a block.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:30: //
DependsOn(WebIntentsRegistryFactory::GetInstance());
On 2012/06/11 19:43:16, Elliot Glaysher wrote:
> Is this supposed to be uncommented? I see that WebIntentsRegistryFactory
exists.

Trying to segregate CLs to keep the switch to PKS separate from integration with
WebIntentsRegistry. I can remove if this is offensive, but otherwise like it
here for later uncommenting.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:41: new
ProtocolHandlerRegistry::Delegate());
On 2012/06/11 00:41:26, Peter Kasting wrote:
> Nit: Could put on previous line

Done.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:49: return
false;
On 2012/06/11 00:41:26, Peter Kasting wrote:
> This is already the default behavior.

Done.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:263:
registry_->Finalize();
On 2012/06/11 19:43:16, Elliot Glaysher wrote:
> (And change the name here)

Done.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/test/base/te...
File chrome/test/base/testing_profile.cc (right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/test/base/te...
chrome/test/base/testing_profile.cc:667: return protocol_handler_registry_;
On 2012/06/11 19:43:16, Elliot Glaysher wrote:
> Edit: Now that I look at this, you may just be able to return NULL here;
> CreateProtocolHandlerRegistry() looks like dead code.
> 
> Previous: There's testing infrastructure for conditionally building a service
> during only certain tests. See uses of
> ProfileKeyedServiceFactory::SetTestingFactory() for more details. (Or look at
> how it's used in autofill_manager_unittest.cc)

Done.

[Peter Kasting, 2012-06-12 00:31:37]

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:21: 
On 2012/06/11 22:09:38, smckay1 wrote:
> On 2012/06/11 00:41:26, Peter Kasting wrote:
> > Nit: Extra blank lines (3 places)
> 
> Done. Though allow me to proselytize for more whitespace a bit. An empty line
> after a line wrap makes it easier to distinguish content from the wrapped
> content preceding it, especially so in the middle of a block.

I don't mind whitespace in general, but a blank line at the beginning of a
function is something I've never seen elsewhere in Chrome code.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:30: //
DependsOn(WebIntentsRegistryFactory::GetInstance());
On 2012/06/11 22:09:38, smckay1 wrote:
> On 2012/06/11 19:43:16, Elliot Glaysher wrote:
> > Is this supposed to be uncommented? I see that WebIntentsRegistryFactory
> exists.
> 
> Trying to segregate CLs to keep the switch to PKS separate from integration
with
> WebIntentsRegistry. I can remove if this is offensive, but otherwise like it
> here for later uncommenting.

Do you mean that right now the ProtocolHandlerRegistry doesn't actually depend
on the WebIntentsRegistry at all?  If so this line should probably be entirely
absent.

[willchan no longer on Chromium, 2012-06-12 02:09:56]

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry.cc:589:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2012/06/11 22:09:38, smckay1 wrote:
> On 2012/06/11 21:17:22, willchan wrote:
> > I think this class is used on both the UI and IO thread. Are you doing
> anything
> > to make sure it's not concurrently being used on the IO thread when this is
> > being deleted? If not, it'll probably lead to race conditions.
> 
> So what I need to tease apart here, for my understanding and because
> WebIntentsRegistry uses a ProfileKeyedService as well, is the distinction
> between the overall design of the class and it's consumers and the changes I'm
> making (switching to a PKS). I'm not doing anything new in this CL to
> orchestrate thread access to this class. If there's a race condition, can you
> help me understand if it is intrinsic to the design or if emerges from the
> refactoring in this CL.
> 
> In either case if there is a race condition I would appreciate your advice on
> how I can remedy this (again, I'm new to Chrome and even C++ and I might
require
> a little more hand holding than a seasoned Chrome engineer :)
> 
> Thanks for your help.

The full answer is complicated :) But in short, this object is being used on two
threads, and may be getting deleted on the UI thread while it's being used on
the IO thread, which would be problematic. With an object that is accessed on
multiple threads, you need to do some sort of coordination across threads to
destroy the object without race conditions.

I don't have time right now to explain further...and in my own selfish interest
of saving my own time, can I ask you to ask your teammates about it first? Or
Elliot (erg) or someone. And they can punt to me if they want.

[Steve McKay, 2012-06-13 23:40:53]

Talked with Ben about this a bit offline (well, off review). General consensus
(I believe) is that we're probably good to go with the current state of affairs
(assuming all tests are passing):

Ben said,
"There are no tests but there are DCHECKs.

This is the code where the registry's finalize was added:
http://codereview.chromium.org/7080023/diff2/10001:13001/chrome/browser/custo...

Looking at this in some detail, I think the important things were:
- the default protocol observers in the registry are freed on the UI thread
- the registry itself was created and deleted on the IO thread.

The first one of these is important, the other was a circumstance of what owned
the registry and can change if you need it to.

I think if you're code is running correctly, things are getting freed properly,
tests are passing, and you don't detect any problems with DCHECKs, you're
probably good. Having said that, for this kind of thing the more you understand
the threading aspects of the code the better. "

Steve said,

"Thanks for your help with this Ben. Sounds like deletion of the registry via
the PKS lifecycle management on the UI thread is okay given its association with
a particular user profile.

Also, WebIntentsRegistry and ProtocolHandlerRegistry are quite similar in their
use cases. Greg and I talked about possible threading issues yesterday and
believe that any locking or other remedies we might impose to provide further
correctness on either of these registries aren't likely to provide any meaning
improvements (i.e. guard against actually scary race conditions). The only good
reason I could see to do anything here is as a prophylactic against future
scenarios where actually scary race conditions might arise. But even projecting
forward the worst issues we can see emerging from race conditions are missing
records in the registry which has an extremely low impact on the user (i.e. it
doesn't cause the browser to crash or corrupt the users settings)."

[Steve McKay, 2012-06-14 04:05:04]

Okay. Most try bots are happy with this patch (excluding win_rel, which I'll
retry in the am). I've done a bit more cleanup.

Ben, please see my inline comments for questions/discussions about changes I
made. Else, I think this CL is ready for review. Thanks again for your
assistance. I guarantee a noob like me doubly appreciates it :)

http://codereview.chromium.org/10546083/diff/2007/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (left):

http://codereview.chromium.org/10546083/diff/2007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:324: virtual void
InitRegisteredProtocolHandlers() = 0;
FYI, this is now handled by the factory which is called by ProfileImpl.

http://codereview.chromium.org/10546083/diff/2007/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (left):

http://codereview.chromium.org/10546083/diff/2007/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:339: InitRegisteredProtocolHandlers();
I've been trying to figure out if we need to preserve this early initialization
of registered protocol handlers. In a lot of cases I think we may never need to
initialize the object (assuming that the "http" protocol is short circuited). If
all access to registered protocol handlers is via this registry, then it seems
safe to assume that we can defer initialization of default handlers until the
time that the registry is first accessed...and that we don't need to do that
here.

Does this sound reasonable? Or are there other ways that default protocol
handlers are accessed/utilized?

[Elliot Glaysher, 2012-06-14 21:32:01]

If you really need early initialization (I hope that you don't), you can always
override PKSF::ServiceIsCreatedWithProfile().

[smckay, 2012-06-14 21:55:43]

On 2012/06/14 21:32:01, Elliot Glaysher wrote:
> If you really need early initialization (I hope that you don't), you can
always
> override PKSF::ServiceIsCreatedWithProfile().

Good to know. Thanks!

[Steve McKay, 2012-06-14 22:06:23]

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:21: 
On 2012/06/12 00:31:37, Peter Kasting wrote:
> On 2012/06/11 22:09:38, smckay1 wrote:
> > On 2012/06/11 00:41:26, Peter Kasting wrote:
> > > Nit: Extra blank lines (3 places)
> > 
> > Done. Though allow me to proselytize for more whitespace a bit. An empty
line
> > after a line wrap makes it easier to distinguish content from the wrapped
> > content preceding it, especially so in the middle of a block.
> 
> I don't mind whitespace in general, but a blank line at the beginning of a
> function is something I've never seen elsewhere in Chrome code.

Done.

https://chromiumcodereview.appspot.com/10546083/diff/2001/chrome/browser/cust...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:30: //
DependsOn(WebIntentsRegistryFactory::GetInstance());
On 2012/06/12 00:31:37, Peter Kasting wrote:
> On 2012/06/11 22:09:38, smckay1 wrote:
> > On 2012/06/11 19:43:16, Elliot Glaysher wrote:
> > > Is this supposed to be uncommented? I see that WebIntentsRegistryFactory
> > exists.
> > 
> > Trying to segregate CLs to keep the switch to PKS separate from integration
> with
> > WebIntentsRegistry. I can remove if this is offensive, but otherwise like it
> > here for later uncommenting.
> 
> Do you mean that right now the ProtocolHandlerRegistry doesn't actually depend
> on the WebIntentsRegistry at all?  If so this line should probably be entirely
> absent.

Done.

[benwells, 2012-06-18 04:27:50]

The registry needs to be loaded at initialization time, in case (for example)
mailto: is registered and chrome is the handler for mailto:. Has that been
tested?

The relevant code is now in StartupBrowserCreatorImpl::OpenTabsInBrowser in
startup_browser_creator_impl.cc. This used to be covered by a UI test but I
think in the transition to browser tests it was lost.

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

http://codereview.chromium.org/10546083/diff/8005/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:60: #endif
I don't mind moving this here, as long as there is no or understood / desired
changes in behavior. Does this get run now when in incognito mode, while it
didn't before? Is that desired?

http://codereview.chromium.org/10546083/diff/8005/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:259:
base::Unretained(this),
Is this safe? Is the registry guaranteed to be destroyed after all posted tasks
are run or discarded?

[smckay, 2012-06-18 22:23:29]

Thanks for the pointer to OpenTabsInBrowser. I think that particular case fits
nicely with the lazy initialization. When OpenTabsInBrowser is called and it
asks for a reference to the registry the factory (behind the scenes) will have
the registry perform initialization immediately after being constructed, then
the factory will return the registry. That looks like it fits nicely.

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

http://codereview.chromium.org/10546083/diff/8005/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:60: #endif
On 2012/06/18 04:27:51, benwells wrote:
> I don't mind moving this here, as long as there is no or understood / desired
> changes in behavior. Does this get run now when in incognito mode, while it
> didn't before? Is that desired?

I updated the factory to request initialization with the profile, but in looking
at StartupBrowserCreatorImpl::OpenTabsInBrowser I don't know that it is
necessary. The factory (called under the covers from
profile->GetProtocolHandlerRegistry()) will perform initialization prior to
returning a pointer to the registry. Can you think of other cases where early
initialization might be required?

http://codereview.chromium.org/10546083/diff/8005/chrome/browser/custom_handl...
chrome/browser/custom_handlers/protocol_handler_registry.cc:259:
base::Unretained(this),
On 2012/06/18 04:27:51, benwells wrote:
> Is this safe? Is the registry guaranteed to be destroyed after all posted
tasks
> are run or discarded?

Have refactored to use a separate ref counted "Core" object.

[Steve McKay, 2012-06-18 22:41:08]

I should probably provide a little background on the "Core" changes. I talked
with Will Chan about the interaction between UI and IO thread and he suggested
the refactoring where a "core" object is separated and ref counted to ensure no
thread is ever surprised by an object being deleted unexpectedly.

As to initialization, I've been poking around and currently can't find anything
that appears to require early initialization. If someone does know of this,
please let me know, it's a small tweak to init the object along with the
profile.

[Steve McKay, 2012-06-19 00:59:55]

Test are still broken. I'll ping once a new patch is uploaded.

[Steve McKay, 2012-06-22 17:10:11]

Okay, reviewers, please dive in. After a lot of learning and some occasional
beating of my head on the keyboard, the try bots are looking good.

[Elliot Glaysher, 2012-06-22 17:32:48]

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:21: 
Please don't do this. One line of whitespace between logical sections in a
function is fine, but there's no place else in chromium where there's a
linebreak before the first statement in a function.

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:55: 
-whitespace

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.h (right):

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.h:22: 
-whitespace

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.h:44: 
-whitespace

http://codereview.chromium.org/10546083/diff/48002/chrome/test/base/testing_p...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/10546083/diff/48002/chrome/test/base/testing_p...
chrome/test/base/testing_profile.h:255: ProtocolHandlerRegistry*
protocol_handler_registry_;
Is this ever used now? If not, cut it out.

[Steve McKay, 2012-06-22 18:01:58]

Responded to Elliot's comments. PTAL.

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:21: 
On 2012/06/22 17:32:48, Elliot Glaysher wrote:
> Please don't do this. One line of whitespace between logical sections in a
> function is fine, but there's no place else in chromium where there's a
> linebreak before the first statement in a function.

Done.

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:55: 
On 2012/06/22 17:32:48, Elliot Glaysher wrote:
> -whitespace

Done.

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.h (right):

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.h:22: 
On 2012/06/22 17:32:48, Elliot Glaysher wrote:
> -whitespace

Done.

http://codereview.chromium.org/10546083/diff/48002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.h:44: 
On 2012/06/22 17:32:48, Elliot Glaysher wrote:
> -whitespace

Done.

http://codereview.chromium.org/10546083/diff/48002/chrome/test/base/testing_p...
File chrome/test/base/testing_profile.h (right):

http://codereview.chromium.org/10546083/diff/48002/chrome/test/base/testing_p...
chrome/test/base/testing_profile.h:255: ProtocolHandlerRegistry*
protocol_handler_registry_;
On 2012/06/22 17:32:48, Elliot Glaysher wrote:
> Is this ever used now? If not, cut it out.

Nope. Nuked!

[willchan no longer on Chromium, 2012-06-22 19:01:56]

I need to read the protocol handler registry core stuff in detail. I'll submit
more comments later (probably after lunch).

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:86: 
-whitespace

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:88: 
-whitespace

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.h:34: 
-whitespace

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:166:
net::URLRequestJobFactory::Interceptor* protocol_handler_url_interceptor;
I believe this will lead to leaks. Sometimes we create a Profile and a
ProfileIOData, but never initialize the ProfileIOData, since it's done lazily,
so the transfer of ownership to the URLRequestJobFactory never happens. This
will cause issues on our memory bots. You should keep this within a scoped_ptr
and use .release() to perform the explicit transfer.

[willchan no longer on Chromium, 2012-06-22 23:15:44]

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:62: // only chromeos
has default protocol handlers at this point.
s/only/Only/

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:89: explicit
Core(bool);
please name the variable

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:201: //
TODO(smckay): release what ever type of pointer we have
?

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.h:206: class
IOURLInterceptor;
Why does this need to be a private nested class? Can't this be defined
completely in the .cc file? Also, my minor nit is that it should just be named
URLInterceptor. All the Interceptors are on the IO thread in chrome. Naming it
IOURLInterceptor will confuse others into thinking that there are some that work
on a different thread.

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:171: ProtocolHandlerRegistry*
proto_handlers =
The style guide discourages abbreviations. Please call this
protocol_handler_registry.

[benwells, 2012-06-22 23:31:27]

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:200: // on the C
instance.
I dont understand this todo. What is the C instance? If this is a leak it should
be fixed before landing.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:393:
default_handlers_.erase(scheme);
This is just a thought, but could Core be used to implement the UI stuff as
well? It feels like there is a lot of redundancy between this class and Core.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.h:204: class Core;
Maybe call this something like IORegistry, CoreIO, .... or something to indicate
that it is the registry's implementation on the IO thread.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:59: 
nit: remove some blank lines.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.h (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.h:14: class
ProtocolHandlerRegistry;
Is both this and #include ...._registry.h" needed?

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:332:
ProtocolHandlerRegistry* registry_;
I think you've got a leak now. You need to delete this, or used a scoped_ptr.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:49: #include
"chrome/browser/intents/web_intents_registry_factory.h"
Why are these includes needed?

[smckay, 2012-06-22 23:44:30]

Thanks for the review.
New patch uploaded. PTAL.

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:62: // only chromeos
has default protocol handlers at this point.
On 2012/06/22 23:15:44, willchan wrote:
> s/only/Only/

Done.

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:89: explicit
Core(bool);
On 2012/06/22 23:15:44, willchan wrote:
> please name the variable

Done.

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:201: //
TODO(smckay): release what ever type of pointer we have
On 2012/06/22 23:15:44, willchan wrote:
> ?

Didn't know if I had to do something special to deal with the new scoped_refptr.
Looks like I don't.

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.h:206: class
IOURLInterceptor;
On 2012/06/22 23:15:44, willchan wrote:
> Why does this need to be a private nested class? Can't this be defined
> completely in the .cc file?

Since Core is private I have to declare URLInterceptor in the same scope so it
can have access to it. Else I'd have to make URLInterceptor a friend, but I
can't do that because it isn't declared in the .h file. Sorta a catch 22.

> Also, my minor nit is that it should just be named
> URLInterceptor. All the Interceptors are on the IO thread in chrome. Naming it
> IOURLInterceptor will confuse others into thinking that there are some that
work
> on a different thread.

Done. Cleaner naming.

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/10546083/diff/52002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:171: ProtocolHandlerRegistry*
proto_handlers =
On 2012/06/22 23:15:44, willchan wrote:
> The style guide discourages abbreviations. Please call this
> protocol_handler_registry.

Done.

[Steve McKay, 2012-06-27 21:49:28]

PTAL.

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:86: 
On 2012/06/22 19:01:56, willchan wrote:
> -whitespace

Done.

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:88: 
On 2012/06/22 19:01:56, willchan wrote:
> -whitespace

Done.

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.h:34: 
On 2012/06/22 19:01:56, willchan wrote:
> -whitespace

Done.

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

http://codereview.chromium.org/10546083/diff/47002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:166:
net::URLRequestJobFactory::Interceptor* protocol_handler_url_interceptor;
On 2012/06/22 19:01:56, willchan wrote:
> I believe this will lead to leaks. Sometimes we create a Profile and a
> ProfileIOData, but never initialize the ProfileIOData, since it's done lazily,
> so the transfer of ownership to the URLRequestJobFactory never happens. This
> will cause issues on our memory bots. You should keep this within a scoped_ptr
> and use .release() to perform the explicit transfer.

Great tip. Done!

[Steve McKay, 2012-06-27 21:56:45]

Responding to Ben's comments.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:200: // on the C
instance.
On 2012/06/22 23:31:27, benwells wrote:
> I dont understand this todo. What is the C instance? If this is a leak it
should
> be fixed before landing.

Obsolete. Nuked.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:393:
default_handlers_.erase(scheme);
On 2012/06/22 23:31:27, benwells wrote:
> This is just a thought, but could Core be used to implement the UI stuff as
> well? It feels like there is a lot of redundancy between this class and Core.

I'll be back in the code in the near future. Can we consider this a topic to
revisit?

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.h:204: class Core;
On 2012/06/22 23:31:27, benwells wrote:
> Maybe call this something like IORegistry, CoreIO, .... or something to
indicate
> that it is the registry's implementation on the IO thread.

I had originally called this IOPartner, but renamed at Will's suggestion. I'm
fine with a rename if you want, but you and Will should agree.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.cc
(right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.cc:59: 
On 2012/06/22 23:31:27, benwells wrote:
> nit: remove some blank lines.

Done.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.h (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_factory.h:14: class
ProtocolHandlerRegistry;
On 2012/06/22 23:31:27, benwells wrote:
> Is both this and #include ...._registry.h" needed?

Fixed.

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:332:
ProtocolHandlerRegistry* registry_;
On 2012/06/22 23:31:27, benwells wrote:
> I think you've got a leak now. You need to delete this, or used a scoped_ptr.

Test is updated to explicitly manage the lifecycle of the object. Shouldn't be a
leak (as far as my understanding of this topic goes). PTAL.

[Steve McKay, 2012-06-27 23:14:05]

TestOSRegistration is failing on the try servers (mac and linux), but passing
locally. Unclear why this would be happening. I'm investigating.

[Steve McKay, 2012-06-28 01:07:05]

On 2012/06/27 23:14:05, smckay1 wrote:
> TestOSRegistration is failing on the try servers (mac and linux), but passing
> locally. Unclear why this would be happening. I'm investigating.

Okay. It looks like ProtocolHandlerRegistryTest.TestOSRegistration test is
clearly flaky. It has both passed and failed on win_rel against the same patch
in the last hour or so. This test and FAILS_TestOSRegistrationFailure are the
two that depend on the FILE thread. Interesting that these are the only two
tests where we're try to make assertions against values posted back to the UI
thread from another thread. All interaction with the IO thread results in
assertions being made on the IO thread.

I'm going to investigate more, but if anyone has any ideas on how this can be
orchestrated to make the test reliable...I'm all ears.

[benwells, 2012-06-28 08:15:46]

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:393:
default_handlers_.erase(scheme);
On 2012/06/27 21:56:45, smckay1 wrote:
> On 2012/06/22 23:31:27, benwells wrote:
> > This is just a thought, but could Core be used to implement the UI stuff as
> > well? It feels like there is a lot of redundancy between this class and
Core.
> 
> I'll be back in the code in the near future. Can we consider this a topic to
> revisit?

Certainly!

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/70001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.h:204: class Core;
On 2012/06/27 21:56:45, smckay1 wrote:
> On 2012/06/22 23:31:27, benwells wrote:
> > Maybe call this something like IORegistry, CoreIO, .... or something to
> indicate
> > that it is the registry's implementation on the IO thread.
> 
> I had originally called this IOPartner, but renamed at Will's suggestion. I'm
> fine with a rename if you want, but you and Will should agree.

I agree Core is superior to IOPartner. I'm fine with it as is.

http://codereview.chromium.org/10546083/diff/96001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/10546083/diff/96001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:318:
ProtocolHandlerRegistry* registry_;
You should use a scoped_ptr for registry_, unless there is some reason not too.

If you do that your registry resetting code becomes something like:
   delegate_ = new FakeDelegate();
   registry_->Finalize();	
   registry_.reset(new ProtocolHandlerRegistry(profile(), delegate()));
   .....

[Steve McKay, 2012-06-28 23:05:58]

Okay. Here's what I did. I updated the test to use a single MessageLoop instance
across all threads. Presumably this means (Will, please confirm) that calling
RunAllPending will permit all threads to handle (and post new) pending messages.
This approach resulted in a DCHECK failing in URLRequest as it was using the
MessageLoop::Type as a proxy for a BrowserThread::Id DCHECK. This necessitated
some small changes to MessageLoop allowing me to override id comparison on the
MessageLoop in a testing specific sub-class.

Where does this leave us?

I was *never* able to repro the flakiness on my local machine. I loaded down all
the CPUs on my machine and ran the tests 500 times in a row with no failures.

The current patch is now with the try bots. We just have to see how things work
out there.

So I propose we submit as-is if:
- the try bots turn up green again
- everyone likes the the code now

If the flakiness continues we can revisit the issue with a wee bit o' sadness.

PTALs all around.

http://codereview.chromium.org/10546083/diff/96001/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/10546083/diff/96001/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:318:
ProtocolHandlerRegistry* registry_;
On 2012/06/28 08:15:46, benwells wrote:
> You should use a scoped_ptr for registry_, unless there is some reason not
too.
> 
> If you do that your registry resetting code becomes something like:
>    delegate_ = new FakeDelegate();
>    registry_->Finalize();	
>    registry_.reset(new ProtocolHandlerRegistry(profile(), delegate()));
>    .....

Done.

[willchan no longer on Chromium, 2012-06-29 21:59:50]

Sorry, I don't have time to finish this review. I looked at the profiles/ stuff
and that LGTM. I defer all the protocol_handler_registry.* stuff to benwells.

http://codereview.chromium.org/10546083/diff/85008/chrome/browser/custom_hand...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/85008/chrome/browser/custom_hand...
chrome/browser/custom_handlers/protocol_handler_registry.cc:197: // on the C
instance.
? If you mean the scoped_refptr<Core>, it'll get auto-released when
URLInterceptor is destroyed.

[Steve McKay, 2012-06-29 23:18:50]

On 2012/06/29 21:59:50, willchan wrote:
> Sorry, I don't have time to finish this review. I looked at the profiles/
stuff
> and that LGTM. I defer all the protocol_handler_registry.* stuff to benwells.
> 
>
http://codereview.chromium.org/10546083/diff/85008/chrome/browser/custom_hand...
> File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):
> 
>
http://codereview.chromium.org/10546083/diff/85008/chrome/browser/custom_hand...
> chrome/browser/custom_handlers/protocol_handler_registry.cc:197: // on the C
> instance.
> ? If you mean the scoped_refptr<Core>, it'll get auto-released when
> URLInterceptor is destroyed.

That was an old comment (I thought I'd already removed).

[Steve McKay, 2012-07-16 21:49:25]

Hi folks,

I'm back from vacation. Merged and put this on the try server. Looks like mac
flaked, but I think the CL is ready to submit. PTAL. Would love to submit this
ASAP.

[Elliot Glaysher, 2012-07-16 22:05:38]

profiles lgtm

[James Hawkins, 2012-07-17 00:45:28]

LGTM with nits.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:87: explicit
Core(bool enabled);
nit: Document |enabled|.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:115: virtual
~Core();
nit: This section could use some documentation.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:148: if
(handler.IsEmpty()) {
nit: Remove braces from single-line blocks to match the precedence set by this
file.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:63: // The tersely
named |Delegate| class provides an interface for interacting
nit: I'd keep subjective terms out of the comments :-)

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:209: // Forward
declaration of our internal implementation classes.
nit: s/our/the/

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:278:
scoped_refptr<Core> core_;
nit: Document.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.h (right):

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_factory.h:29: virtual
bool ServiceIsCreatedWithProfile() OVERRIDE;
nit: I'm assuming this is also an implementation section for
ProfileKeyedServiceFactory.  If this is correct, add // Blah implementation.  If
not, we have some issues!

[Steve McKay, 2012-07-17 01:12:41]

PTAL.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.cc (right):

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:87: explicit
Core(bool enabled);
On 2012/07/17 00:45:28, James Hawkins wrote:
> nit: Document |enabled|.

Done.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:115: virtual
~Core();
On 2012/07/17 00:45:28, James Hawkins wrote:
> nit: This section could use some documentation.

Done...the fields.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.cc:148: if
(handler.IsEmpty()) {
On 2012/07/17 00:45:28, James Hawkins wrote:
> nit: Remove braces from single-line blocks to match the precedence set by this
> file.

Done.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:63: // The tersely
named |Delegate| class provides an interface for interacting
On 2012/07/17 00:45:28, James Hawkins wrote:
> nit: I'd keep subjective terms out of the comments :-)

Are you referencing the user of "tersely named"? If so, then Done. Else, kindly
ping back with details.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:278:
scoped_refptr<Core> core_;
On 2012/07/17 00:45:28, James Hawkins wrote:
> nit: Document.

Done.

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_factory.h (right):

http://codereview.chromium.org/10546083/diff/111004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_factory.h:29: virtual
bool ServiceIsCreatedWithProfile() OVERRIDE;
On 2012/07/17 00:45:28, James Hawkins wrote:
> nit: I'm assuming this is also an implementation section for
> ProfileKeyedServiceFactory.  If this is correct, add // Blah implementation. 
If
> not, we have some issues!

Done.

[Peter Kasting, 2012-07-17 01:39:17]

I'm listed on the reviewers line but I don't see anything telling me to review a
specific piece.  Is there something you want me to look at?

[benwells, 2012-07-17 01:50:20]

lgtm with nit

http://codereview.chromium.org/10546083/diff/113004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/113004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:69: // updated.
Nit: remove this TODO while you're here. It's no longer relevant.

[Steve McKay, 2012-07-17 16:56:15]

Done. I think we're good to commit now WRT owner approvals.

http://codereview.chromium.org/10546083/diff/113004/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry.h (right):

http://codereview.chromium.org/10546083/diff/113004/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry.h:69: // updated.
On 2012/07/17 01:50:21, benwells wrote:
> Nit: remove this TODO while you're here. It's no longer relevant.

Done.

[commit-bot: I haz the power, 2012-07-17 23:16:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/116006

[commit-bot: I haz the power, 2012-07-17 23:16:22]

Failed to apply patch for chrome/browser/profiles/profile_impl.h:
While running patch -p1 --forward --force;
patching file chrome/browser/profiles/profile_impl.h
Hunk #1 succeeded at 102 (offset -3 lines).
Hunk #2 succeeded at 145 (offset -3 lines).
Hunk #3 FAILED at 209.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_impl.h.rej

[commit-bot: I haz the power, 2012-07-17 23:50:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/116006

[commit-bot: I haz the power, 2012-07-17 23:50:48]

Failed to apply patch for chrome/browser/profiles/profile_impl.h:
While running patch -p1 --forward --force;
patching file chrome/browser/profiles/profile_impl.h
Hunk #1 succeeded at 102 (offset -3 lines).
Hunk #2 succeeded at 145 (offset -3 lines).
Hunk #3 FAILED at 209.
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/profiles/profile_impl.h.rej

[smckay, 2012-07-18 18:05:54]

NOTE: Change https://chromiumcodereview.appspot.com/10584042 introduced an
incompatible change necessitating some refactoring to supporting use of
ProtocolHandlerRegistry in testing of the content settings bubble model.

Background: We disable creation of the ProtocolHandlerRegistry instance at
the factory level for all test profiles (which was necessary to avoid
fixing the litany of tests that don't correctly harness threads (causing
DCHECK failures in the PHR code).

I'll ping the pertinent reviewers once the necessary changes are complete.


Steve McKay | Sr. Software Engineer | smckay@google.com | 310-359-8331




On Tue, Jul 17, 2012 at 4:50 PM, <commit-bot@chromium.org> wrote:

> Failed to apply patch for chrome/browser/profiles/**profile_impl.h:
> While running patch -p1 --forward --force;
> patching file chrome/browser/profiles/**profile_impl.h
> Hunk #1 succeeded at 102 (offset -3 lines).
> Hunk #2 succeeded at 145 (offset -3 lines).
> Hunk #3 FAILED at 209.
> 1 out of 3 hunks FAILED -- saving rejects to file
> chrome/browser/profiles/**profile_impl.h.rej
>
>
>
https://chromiumcodereview.**appspot.com/10546083/<https://chromiumcodereview...
>

[Steve McKay, 2012-07-19 01:40:12]

Okay. Integrated Greg's changes and updated the content_setting bubble tests. No
substantial changes outside of the content settings code, so I think just Greg
needs to have a look.

[Greg Billock, 2012-07-19 14:49:57]

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:306: void
SetUpRegistry(bool initilize) {
spelling

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:628:
registry_(profile->GetProtocolHandlerRegistry()),
This needs to use the PKS factory, right?

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:139: class
ContentSettingTitleAndLinkModel : public ContentSettingBubbleModel {
Looks like this still needs merged.

[Steve McKay, 2012-07-19 16:59:54]

PTAL, Greg.

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/custom_han...
File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
(right):

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/custom_han...
chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:306: void
SetUpRegistry(bool initilize) {
On 2012/07/19 14:49:57, Greg Billock wrote:
> spelling

Done.

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:628:
registry_(profile->GetProtocolHandlerRegistry()),
On 2012/07/19 14:49:57, Greg Billock wrote:
> This needs to use the PKS factory, right?

Ah. Yeah. I was thinking since this is deprecated we use the old way, but that
was obviously as backwards thinking.

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:139: class
ContentSettingTitleAndLinkModel : public ContentSettingBubbleModel {
On 2012/07/19 14:49:57, Greg Billock wrote:
> Looks like this still needs merged.

Done.

[Greg Billock, 2012-07-19 20:31:00]

On 2012/07/19 16:59:54, Steve McKay wrote:
> PTAL, Greg.
> 
>
http://codereview.chromium.org/10546083/diff/123001/chrome/browser/custom_han...
> File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
> (right):
> 
>
http://codereview.chromium.org/10546083/diff/123001/chrome/browser/custom_han...
> chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:306: void
> SetUpRegistry(bool initilize) {
> On 2012/07/19 14:49:57, Greg Billock wrote:
> > spelling
> 
> Done.
> 
>
http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
> File chrome/browser/ui/content_settings/content_setting_bubble_model.cc
(right):
> 
>
http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
> chrome/browser/ui/content_settings/content_setting_bubble_model.cc:628:
> registry_(profile->GetProtocolHandlerRegistry()),
> On 2012/07/19 14:49:57, Greg Billock wrote:
> > This needs to use the PKS factory, right?
> 
> Ah. Yeah. I was thinking since this is deprecated we use the old way, but that
> was obviously as backwards thinking.
> 
>
http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
> File chrome/browser/ui/content_settings/content_setting_bubble_model.h
(right):
> 
>
http://codereview.chromium.org/10546083/diff/123001/chrome/browser/ui/content...
> chrome/browser/ui/content_settings/content_setting_bubble_model.h:139: class
> ContentSettingTitleAndLinkModel : public ContentSettingBubbleModel {
> On 2012/07/19 14:49:57, Greg Billock wrote:
> > Looks like this still needs merged.
> 
> Done.

lgtm

[commit-bot: I haz the power, 2012-07-19 20:36:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/134005

[commit-bot: I haz the power, 2012-07-19 20:36:29]

Presubmit check for 10546083-134005 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/content_settings

Presubmit checks took 2.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Bernhard Bauer, 2012-07-19 20:52:56]

LGTM with one issue below fixed:

http://codereview.chromium.org/10546083/diff/134005/chrome/browser/ui/content...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/10546083/diff/134005/chrome/browser/ui/content...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:160: //
Deprecated. Use constructor that accepts a |ProtocolHandlerRegistry|.
Remove this constructor please :)

[commit-bot: I haz the power, 2012-07-19 21:02:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/130010

[commit-bot: I haz the power, 2012-07-19 21:30:07]

Try job failure for 10546083-130010 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-07-19 23:14:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/130012

[commit-bot: I haz the power, 2012-07-20 00:11:22]

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

[commit-bot: I haz the power, 2012-07-20 00:11:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/121034

[commit-bot: I haz the power, 2012-07-20 01:52:04]

Change committed as 147597

[Steve McKay, 2012-07-20 16:43:03]

FYI: The chromeos tests went flaky after this was submitted. It was reverted.
I'll resubmit once I can iron out the chromeos issues.

[Greg Billock, 2012-07-20 20:44:25]

On 2012/07/20 16:43:03, Steve McKay wrote:
> FYI: The chromeos tests went flaky after this was submitted. It was reverted.
> I'll resubmit once I can iron out the chromeos issues.

LGTM

[commit-bot: I haz the power, 2012-07-20 20:47:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/123025

[commit-bot: I haz the power, 2012-07-20 22:13:17]

Try job failure for 10546083-123025 (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-07-20 22:23:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/123025

[commit-bot: I haz the power, 2012-07-20 22:53:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/smckay@chromium.org/10546083/123025

[commit-bot: I haz the power, 2012-07-21 00:26:34]

Change committed as 147745