Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10139002:
Preventing our default handlers for ChromeOS to show up or confuse the user (Closed)
|
Created:
8 years, 8 months ago by Mr4D (OOO till 08-26) Modified:
8 years, 7 months ago CC:
chromium-reviews, tony Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionPreventing our default handlers for ChromeOS to show up or confuse the user
BUG=123368
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134283
Patch Set 1 #Patch Set 2 : Added a few more comments #
Total comments: 8
Patch Set 3 : Addressed issues #Patch Set 4 : Second review changes #
Total comments: 12
Patch Set 5 : Addressed third review #
Total comments: 3
Patch Set 6 : Addressed 4th review #
Total comments: 4
Patch Set 7 : Addressing fifth review #
Total comments: 5
Patch Set 8 : Addressing 6th review #Patch Set 9 : Resolving merge issues #
Total comments: 2
Messages
Total messages: 24 (0 generated)
Here is the solution we were talking about. The 'internal ChromeOS' handlers are not presented anywhere.
I've no clue about this code. Normally I would pick someone from the owners file, but there isn't one in this directory. It looks like Tony created the file, but he's on vacation. benwells and koz have touched this file a bunch, maybe they can review for you (I made them the reviewers). -Scott
http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... chrome/browser/custom_handlers/protocol_handler_registry.cc:297: if (IsFixedHandler(scheme)) If I understand this change correctly, ChromeOS users won't be able to ever register other handlers. Is that what we want? Could we have the standard handlers always present but let users add others and set them to be the default? http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... File chrome/browser/custom_handlers/protocol_handler_registry.h (right): http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... chrome/browser/custom_handlers/protocol_handler_registry.h:201: void InstallSystemDefaultHandlers(); Can this be called InstallFixedHandlers to be consistent with above? Or IsFixedHandler to IsSysetmDefaultHandler? http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... chrome/browser/custom_handlers/protocol_handler_registry.h:288: bool locking_defaults_; This variable name is confusing. Can it be renamed? Perhaps if you reverse it you can call it fixed_handlers_loaded_.
Thanks for your review! Please have a second look! http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... chrome/browser/custom_handlers/protocol_handler_registry.cc:297: if (IsFixedHandler(scheme)) There are no 'apps' which can do these things. As such the assigned bug said that this should be set. I also checked with UX team and they agreed. So, yes, this is what we want. That all said: Someone might argue that since the user does not even get the option to overwrite this scheme, this will probably never be hit. But in this case I am better safe then sorry since if the user would replace the item he would have no way back.. http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... File chrome/browser/custom_handlers/protocol_handler_registry.h (right): http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... chrome/browser/custom_handlers/protocol_handler_registry.h:201: void InstallSystemDefaultHandlers(); On 2012/04/20 00:25:05, benwells wrote: > Can this be called InstallFixedHandlers to be consistent with above? Or > IsFixedHandler to IsSysetmDefaultHandler? Done. http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... chrome/browser/custom_handlers/protocol_handler_registry.h:288: bool locking_defaults_; On 2012/04/20 00:25:05, benwells wrote: > This variable name is confusing. Can it be renamed? Perhaps if you reverse it > you can call it fixed_handlers_loaded_. Done.
http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... chrome/browser/custom_handlers/protocol_handler_registry.cc:297: if (IsFixedHandler(scheme)) On 2012/04/20 13:44:41, Mr4D wrote: > There are no 'apps' which can do these things. As such the assigned bug said > that this should be set. I also checked with UX team and they agreed. So, yes, > this is what we want. > > That all said: Someone might argue that since the user does not even get the > option to overwrite this scheme, this will probably never be hit. But in this > case I am better safe then sorry since if the user would replace the item he > would have no way back.. registerProtocolHandler is a web standard, and can be used by any web site. This change will disable it for gmail and calendar, with users being fixed onto our hard-coded defaults. I don't get a sense from the bug that's intended. What I think should happen is that ChromeOS has default handlers which can't be removed, but which the user can override (i.e. set another default). If you do that the user won't get prompted about gmail and google calendar as they will always be registered. BTW I really like this change, providing we keep registerProtoclHandler working. Can we also remove the hardcoded ChromeOS redirection for mailto: and webcal: that is somewhere else in the code (I can't remember where, external_handlers.cc perhaps)? It shouldn't be necessary with your change.
Many thanks for your review! I addressed your comment. See the answer - since I was not able to find your other reference to "mailto" / "webcal". http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/2001/chrome/browser/custom_handl... chrome/browser/custom_handlers/protocol_handler_registry.cc:297: if (IsFixedHandler(scheme)) Okay, fine. I will remove it then. :) (Who am I messing around with web standards?). However - this also means that I have to reverse a few other things. I think the only remaining thing then is that we want to pre-populate the default (if there isn't one yet). For the other request: I have found a non ChromeOS specific string substitution (register_protocol_handler_infobar_delagate.cc). Furthermore I have found one thing in platform_util_chromeos.cc where the scheme 'mailto' gets intercepted and directed to gmail. But after staring on various portions of the code I am not sure how this is used (e.g. LaunchUrlWithoutSecurityCheck). So I'd rather not touch that for the moment.
So the intent of this change is to install handlers for mailto and webcal into the ProtocolHandlerRegistry when it is loaded, but still allow users to install their own handlers over these? Could you please add unit tests for this? http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:46: ProtocolHandler mail_handler = ProtocolHandler::CreateProtocolHandler( nit: mail_handler -> mailto_handler http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:54: ProtocolHandler cal_handler = ProtocolHandler::CreateProtocolHandler( nit: cal_handler -> webcal_handler http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:223: // On the next call the defaults will be locked. What does "locked" mean? Could you replace "will be locked" with "will be already loaded"? In fact, is this conditional even needed? Does Load() ever get called more than once on a single instance of PHR? If it is why do we want the behaviour to be different from the first time it is called? It seems okay to me to have the the mailto and webcal handlers reinstated (if they have been removed) on every call to Load().
http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:47: std::string("mailto"), Replace this with just "mailto" http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:49: UTF8ToUTF16(std::string("Google.com Mail"))); Could you move the user-facing strings (ie: the title and the URL) into a GRD file? Same with the webcal handler below. http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:55: std::string("webcal"), Replace this with just "webcal"
Points addressed. Please have another look! http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:46: ProtocolHandler mail_handler = ProtocolHandler::CreateProtocolHandler( On 2012/04/23 03:43:06, koz wrote: > nit: mail_handler -> mailto_handler Done. http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:47: std::string("mailto"), Not sure why this was done, but every other call of the function did it this way. http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:54: ProtocolHandler cal_handler = ProtocolHandler::CreateProtocolHandler( On 2012/04/23 03:43:06, koz wrote: > nit: cal_handler -> webcal_handler Done. http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:55: std::string("webcal"), Ditto http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:223: // On the next call the defaults will be locked. The "locked" comment was a remainder of some code which I had removed earlier. Forgot to remove it. I don't know if it ever gets called multiple times. When you say it only gets called once (which would make sense) I believe you and remove the call blocker. (Will test the default cases though).
Forgotten to mark this as done in the CL http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/11002/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:49: UTF8ToUTF16(std::string("Google.com Mail"))); On 2012/04/23 03:55:35, koz wrote: > Could you move the user-facing strings (ie: the title and the URL) into a GRD > file? Same with the webcal handler below. Done.
http://codereview.chromium.org/10139002/diff/21001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc (right): http://codereview.chromium.org/10139002/diff/21001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:538: #if defined (OS_CHROMEOS) The tests now require a lot of #ifdefs, which is unfortunate, because now every test has to be aware of the fact that ChromeOS is special. This makes the tests harder to read and change. Instead of how it currently is written, could you please make it so that callers of ProtocolHandlerRegistry define what the defaults are, rather than having them hardcoded in PHR itself? So the code that instantiates PHR would look like this: phr_.reset(new ProtocolHandlerRegistry(...)); // Note that this is the only #ifdef needed in the whole patch now. #ifdef CHROME_OS phr_->SetFixedProtocolHandler(ProtocolHandler::Create("mailto", ...)); phr_->SetFixedProtocolHandler(ProtocolHandler::Create("webcal", ...)); #endif phr_->Load(); PHR::Load() would still call PHR::InstallFixedHandlers(), but it would just install the handlers in "set<ProtocolHandler> fixed_handlers_;") Then you could just add extra tests to test that "fixed" protocol handlers work in general.
Changed again - moved and addressed. Please have another look. http://codereview.chromium.org/10139002/diff/21001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc (right): http://codereview.chromium.org/10139002/diff/21001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:538: #if defined (OS_CHROMEOS) In my opinion the installation of the default protocol handlers belongs into the base (here) because this is the encapsulation of the protocol handlers and therefore this location should know about it and others should not. More so because as you have pointed out that there are potentially more creators. And especially if the only reason is to make the unit tests "cleaner". That said - I have changed it as requested (again). I did one change though: Since the Load only gets called once I have implemented it the way that the installHandler function directly adds the service.
http://codereview.chromium.org/10139002/diff/29001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.h (right): http://codereview.chromium.org/10139002/diff/29001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.h:286: bool is_loaded_; Why is this necessary? http://codereview.chromium.org/10139002/diff/29001/chrome/browser/platform_ut... File chrome/browser/platform_util_chromeos.cc (left): http://codereview.chromium.org/10139002/diff/29001/chrome/browser/platform_ut... chrome/browser/platform_util_chromeos.cc:58: if (url.SchemeIs("mailto")) { I know I asked for this change, but ... we discussed it on chat and I thought had decided to add a TODO. There are a few callers to OpenExternal (e.g. Browser::EmailPageLocation). Have you check this won't break any of them?
Addressed. Please have another look! http://codereview.chromium.org/10139002/diff/29001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.h (right): http://codereview.chromium.org/10139002/diff/29001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.h:286: bool is_loaded_; It is a "debugging" helper: Default handlers can only be added before the load call. After the load call they could overwrite user settings which isn't allowed. As such a DCHECK will be used to check that this does not happen. http://codereview.chromium.org/10139002/diff/29001/chrome/browser/platform_ut... File chrome/browser/platform_util_chromeos.cc (left): http://codereview.chromium.org/10139002/diff/29001/chrome/browser/platform_ut... chrome/browser/platform_util_chromeos.cc:58: if (url.SchemeIs("mailto")) { Well - I have tried all cases I could come up with and they worked as expected. However - you are right there might be some other use cases I don't know. So I was adding it back in and added a comment.
http://codereview.chromium.org/10139002/diff/35001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/35001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:598: void ProtocolHandlerRegistry::AddFixedHandler(const ProtocolHandler& handler) { The name "Fixed" handler seems wrong. Isn't it a Default handler? http://codereview.chromium.org/10139002/diff/35001/chrome/browser/platform_ut... File chrome/browser/platform_util_chromeos.cc (right): http://codereview.chromium.org/10139002/diff/35001/chrome/browser/platform_ut... chrome/browser/platform_util_chromeos.cc:61: // cases left. I would rather see us remove this or replace the comment with why it's necessary to leave it instead of a TODO.
Addressed comments. Please have a look again! http://codereview.chromium.org/10139002/diff/35001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/35001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:598: void ProtocolHandlerRegistry::AddFixedHandler(const ProtocolHandler& handler) { Done. In deed. The meaning of this has changed due to reviews. :) http://codereview.chromium.org/10139002/diff/35001/chrome/browser/platform_ut... File chrome/browser/platform_util_chromeos.cc (right): http://codereview.chromium.org/10139002/diff/35001/chrome/browser/platform_ut... chrome/browser/platform_util_chromeos.cc:61: // cases left. Okay, I have changed the comment after reviewing the code paths thoroughly. There are some cases in which we might break things when removing this.
lgtm although benwells should confirm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10139002/42001
Can't apply patch for file chrome/browser/custom_handlers/protocol_handler_registry.cc. While running patch -p1 --forward --force; patching file chrome/browser/custom_handlers/protocol_handler_registry.cc Hunk #2 succeeded at 152 with fuzz 1 (offset 116 lines). Hunk #3 succeeded at 249 with fuzz 1 (offset 52 lines). Hunk #4 FAILED at 595. 1 out of 4 hunks FAILED -- saving rejects to file chrome/browser/custom_handlers/protocol_handler_registry.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skuhne@chromium.org/10139002/51001
Change committed as 134283
http://codereview.chromium.org/10139002/diff/51001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.h (right): http://codereview.chromium.org/10139002/diff/51001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.h:289: bool is_loaded_; Nit: has_loaded_ is more grammatically correct. http://codereview.chromium.org/10139002/diff/51001/chrome/browser/profiles/pr... File chrome/browser/profiles/profile_impl.cc (right): http://codereview.chromium.org/10139002/diff/51001/chrome/browser/profiles/pr... chrome/browser/profiles/profile_impl.cc:458: GURL(l10n_util::GetStringUTF8(IDS_GOOGLE_MAILTO_HANDLER_URL)), Why do the URLs have / repeated in the grd file? Also, do we need to localize the URLs or is this something we can handle in code? Having to include 50+ copies of this string to localize is a lot of download size.
http://codereview.chromium.org/10139002/diff/21001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc (right): http://codereview.chromium.org/10139002/diff/21001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc:538: #if defined (OS_CHROMEOS) On 2012/04/24 15:53:31, Mr4D wrote: > In my opinion the installation of the default protocol handlers belongs into the > base (here) because this is the encapsulation of the protocol handlers and > therefore this location should know about it and others should not. More so > because as you have pointed out that there are potentially more creators. And > especially if the only reason is to make the unit tests "cleaner". > > That said - I have changed it as requested (again). I did one change though: > Since the Load only gets called once I have implemented it the way that the > installHandler function directly adds the service. I disagree that the PHR is the class that should be responsible for knowing what protocol handlers on what platform should be preinstalled, by your own argument. If the callers of PHR now require #ifdefs to function correctly because of #ifdefs inside PHR then instead of encapsulating the concern you have pushed it out to all callers. Clean unit tests are obviously not the goal, but if a change you make requires modifications to many tests (especially the addition of #ifdefs) then it is an indication that you are invalidating many assumptions that code in the wild may have made. http://codereview.chromium.org/10139002/diff/35001/chrome/browser/custom_hand... File chrome/browser/custom_handlers/protocol_handler_registry.cc (right): http://codereview.chromium.org/10139002/diff/35001/chrome/browser/custom_hand... chrome/browser/custom_handlers/protocol_handler_registry.cc:598: void ProtocolHandlerRegistry::AddFixedHandler(const ProtocolHandler& handler) { On 2012/04/26 16:34:37, Mr4D wrote: > Done. In deed. The meaning of this has changed due to reviews. :) No, fixed handler is right. "Default handler" is already a concept in this class, so with this rename we now have SetDefault() and RemoveDefaultHanlder(), both of which mean the handler that is invoked by default for a given protocol, and this, AddDefaultHandler(), which is about registering "pre-installed" handlers at Load() time. Perhaps "preinstalled" handlers is a better name than fixed, but we shouldn't overload the term "default" here. (Looking at the comment in the final revision "// The installation of any pre-defined protocol handlers.", maybe "predefined" is a better word?) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
