Preventing our default handlers for ChromeOS to show up or confuse the user

chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc

Index: chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
diff --git a/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc b/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
index 67820c402177eee6e06b52d8eef425b10afb4806..1af669bc63783484d93747094a37cb5587a075bc 100644
--- a/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
+++ b/chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
@@ -20,7 +20,9 @@
 #include "content/test/test_browser_thread.h"
 #include "content/test/test_browser_thread.h"
 #include "content/test/test_renderer_host.h"
+#include "grit/generated_resources.h"
 #include "net/url_request/url_request.h"
+#include "ui/base/l10n/l10n_util.h"
 
 using content::BrowserThread;
 
@@ -31,8 +33,11 @@ class FakeDelegate : public ProtocolHandlerRegistry::Delegate {
   FakeDelegate() : force_os_failure_(false) {}
   virtual ~FakeDelegate() { }
   virtual void RegisterExternalHandler(const std::string& protocol) {
-    ASSERT_TRUE(
-        registered_protocols_.find(protocol) == registered_protocols_.end());
+#if defined(OS_CHROMEOS)
+    if (protocol != "mailto" && protocol != "webcal")
+#endif
+      ASSERT_TRUE(
+          registered_protocols_.find(protocol) == registered_protocols_.end());
     registered_protocols_.insert(protocol);
   }
 
@@ -530,13 +535,21 @@ TEST_F(ProtocolHandlerRegistryTest, TestGetHandlersFor) {
 TEST_F(ProtocolHandlerRegistryTest, TestGetRegisteredProtocols) {
   std::vector<std::string> protocols;
   registry()->GetRegisteredProtocols(&protocols);
+#if defined (OS_CHROMEOS)

[koz (OOO until 15th September), 2012/04/24 00:52:29]

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.

[Mr4D (OOO till 08-26), 2012/04/24 15:53:31]

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.

[koz (OOO until 15th September), 2012/04/30 02:11:25]

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.

+  ASSERT_EQ(static_cast<size_t>(2), protocols.size());
+#else
   ASSERT_EQ(static_cast<size_t>(0), protocols.size());
+#endif
 
   registry()->GetHandlersFor("test");
 
   protocols.clear();
   registry()->GetRegisteredProtocols(&protocols);
+#if defined (OS_CHROMEOS)
+  ASSERT_EQ(static_cast<size_t>(2), protocols.size());
+#else
   ASSERT_EQ(static_cast<size_t>(0), protocols.size());
+#endif
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestIsHandledProtocol) {
@@ -578,8 +591,13 @@ TEST_F(ProtocolHandlerRegistryTest, TestProtocolsWithNoDefaultAreHandled) {
   registry()->ClearDefault("test");
   std::vector<std::string> handled_protocols;
   registry()->GetRegisteredProtocols(&handled_protocols);
+#if defined (OS_CHROMEOS)
+  ASSERT_EQ(static_cast<size_t>(3), handled_protocols.size());
+  ASSERT_EQ("test", handled_protocols[1]);
+#else
   ASSERT_EQ(static_cast<size_t>(1), handled_protocols.size());
   ASSERT_EQ("test", handled_protocols[0]);
+#endif
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestDisablePreventsHandling) {
@@ -645,9 +663,9 @@ static void MakeRequest(const GURL& url, ProtocolHandlerRegistry* registry) {
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestMaybeCreateTaskWorksFromIOThread) {
-  ProtocolHandler ph1 = CreateProtocolHandler("mailto", "test1");
+  ProtocolHandler ph1 = CreateProtocolHandler("mailto-test", "test1");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
-  GURL url("mailto:someone@something.com");
+  GURL url("mailto-test:someone@something.com");
   scoped_refptr<ProtocolHandlerRegistry> r(registry());
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(MakeRequest, url, r));
@@ -662,7 +680,7 @@ static void CheckIsHandled(const std::string& scheme, bool expected,
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestIsHandledProtocolWorksOnIOThread) {
-  std::string scheme("mailto");
+  std::string scheme("mailto-test");
   ProtocolHandler ph1 = CreateProtocolHandler(scheme, "test1");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   scoped_refptr<ProtocolHandlerRegistry> r(registry());
@@ -673,9 +691,9 @@ TEST_F(ProtocolHandlerRegistryTest, TestIsHandledProtocolWorksOnIOThread) {
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestRemovingDefaultFallsBackToOldDefault) {
-  ProtocolHandler ph1 = CreateProtocolHandler("mailto", "test1");
-  ProtocolHandler ph2 = CreateProtocolHandler("mailto", "test2");
-  ProtocolHandler ph3 = CreateProtocolHandler("mailto", "test3");
+  ProtocolHandler ph1 = CreateProtocolHandler("mailto-test", "test1");
+  ProtocolHandler ph2 = CreateProtocolHandler("mailto-test", "test2");
+  ProtocolHandler ph3 = CreateProtocolHandler("mailto-test", "test3");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   registry()->OnAcceptRegisterProtocolHandler(ph2);
   registry()->OnAcceptRegisterProtocolHandler(ph3);
@@ -692,24 +710,23 @@ TEST_F(ProtocolHandlerRegistryTest, TestRemovingDefaultFallsBackToOldDefault) {
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestRemovingDefaultDoesntChangeHandlers) {
-  ProtocolHandler ph1 = CreateProtocolHandler("mailto", "test1");
-  ProtocolHandler ph2 = CreateProtocolHandler("mailto", "test2");
-  ProtocolHandler ph3 = CreateProtocolHandler("mailto", "test3");
+  ProtocolHandler ph1 = CreateProtocolHandler("mailto-test", "test1");
+  ProtocolHandler ph2 = CreateProtocolHandler("mailto-test", "test2");
+  ProtocolHandler ph3 = CreateProtocolHandler("mailto-test", "test3");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   registry()->OnAcceptRegisterProtocolHandler(ph2);
   registry()->OnAcceptRegisterProtocolHandler(ph3);
   registry()->RemoveHandler(ph3);
 
   ProtocolHandlerRegistry::ProtocolHandlerList handlers =
-      registry()->GetHandlersFor("mailto");
+      registry()->GetHandlersFor("mailto-test");
   ASSERT_EQ(static_cast<size_t>(2), handlers.size());
-
   ASSERT_EQ(ph2, handlers[0]);
   ASSERT_EQ(ph1, handlers[1]);
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestClearDefaultGetsPropagatedToIO) {
-  std::string scheme("mailto");
+  std::string scheme("mailto-test");
   ProtocolHandler ph1 = CreateProtocolHandler(scheme, "test1");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   registry()->ClearDefault(scheme);
@@ -736,54 +753,54 @@ TEST_F(ProtocolHandlerRegistryTest, TestLoadEnabledGetsPropogatedToIO) {
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestReplaceHandler) {
-  ProtocolHandler ph1 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph1 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/%s"), "test1");
-  ProtocolHandler ph2 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph2 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/updated-url/%s"), "test2");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   ASSERT_TRUE(registry()->AttemptReplace(ph2));
-  const ProtocolHandler& handler(registry()->GetHandlerFor("mailto"));
+  const ProtocolHandler& handler(registry()->GetHandlerFor("mailto-test"));
   ASSERT_EQ(handler.url(), ph2.url());
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestReplaceNonDefaultHandler) {
-  ProtocolHandler ph1 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph1 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/%s"), "test1");
-  ProtocolHandler ph2 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph2 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/updated-url/%s"), "test2");
-  ProtocolHandler ph3 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph3 = CreateProtocolHandler("mailto-test",
       GURL("http://else.com/%s"), "test3");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   registry()->OnAcceptRegisterProtocolHandler(ph3);
   ASSERT_TRUE(registry()->AttemptReplace(ph2));
-  const ProtocolHandler& handler(registry()->GetHandlerFor("mailto"));
+  const ProtocolHandler& handler(registry()->GetHandlerFor("mailto-test"));
   ASSERT_EQ(handler.url(), ph3.url());
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestReplaceRemovesStaleHandlers) {
-  ProtocolHandler ph1 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph1 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/%s"), "test1");
-  ProtocolHandler ph2 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph2 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/updated-url/%s"), "test2");
-  ProtocolHandler ph3 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph3 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/third/%s"), "test");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   registry()->OnAcceptRegisterProtocolHandler(ph2);
 
   // This should replace the previous two handlers.
   ASSERT_TRUE(registry()->AttemptReplace(ph3));
-  const ProtocolHandler& handler(registry()->GetHandlerFor("mailto"));
+  const ProtocolHandler& handler(registry()->GetHandlerFor("mailto-test"));
   ASSERT_EQ(handler.url(), ph3.url());
   registry()->RemoveHandler(ph3);
-  ASSERT_TRUE(registry()->GetHandlerFor("mailto").IsEmpty());
+  ASSERT_TRUE(registry()->GetHandlerFor("mailto-test").IsEmpty());
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestIsSameOrigin) {
-  ProtocolHandler ph1 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph1 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/%s"), "test1");
-  ProtocolHandler ph2 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph2 = CreateProtocolHandler("mailto-test",
       GURL("http://test.com/updated-url/%s"), "test2");
-  ProtocolHandler ph3 = CreateProtocolHandler("mailto",
+  ProtocolHandler ph3 = CreateProtocolHandler("mailto-test",
       GURL("http://other.com/%s"), "test");
   ASSERT_EQ(ph1.url().GetOrigin() == ph2.url().GetOrigin(),
       ph1.IsSameOrigin(ph2));
@@ -794,3 +811,14 @@ TEST_F(ProtocolHandlerRegistryTest, TestIsSameOrigin) {
   ASSERT_EQ(ph3.url().GetOrigin() == ph2.url().GetOrigin(),
       ph3.IsSameOrigin(ph2));
 }
+
+TEST_F(ProtocolHandlerRegistryTest, PredefinedHandlers) {
+#if defined(OS_CHROMEOS)
+  // For ChromeOS we install default handlers for mailto and webcal.
+  ASSERT_FALSE(registry()->GetHandlerFor("mailto").IsEmpty());
+  ASSERT_FALSE(registry()->GetHandlerFor("webcal").IsEmpty());
+#else
+  ASSERT_TRUE(registry()->GetHandlerFor("mailto").IsEmpty());
+  ASSERT_TRUE(registry()->GetHandlerFor("webcal").IsEmpty());
+#endif
+}