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

chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
 
 #include <set>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/custom_handlers/protocol_handler.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_pref_service.h"
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_source.h"
 #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;
 
 namespace {
 
 class FakeDelegate : public ProtocolHandlerRegistry::Delegate {
  public:
   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);
   }
 
   virtual void DeregisterExternalHandler(const std::string& protocol) {
     registered_protocols_.erase(protocol);
   }
 
   virtual ShellIntegration::DefaultProtocolClientWorker* CreateShellWorker(
     ShellIntegration::DefaultWebClientObserver* observer,
     const std::string& protocol);
@@ skipping 477 matching lines @@
   ASSERT_EQ(static_cast<size_t>(3), handlers.size());
 
   ASSERT_EQ(ph3, handlers[0]);
   ASSERT_EQ(ph2, handlers[1]);
   ASSERT_EQ(ph1, handlers[2]);
 }
 
 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) {
   registry()->GetHandlersFor("test");
   ASSERT_FALSE(registry()->IsHandledProtocol("test"));
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestNotifications) {
   ProtocolHandler ph1 = CreateProtocolHandler("test", "test1");
   NotificationCounter counter(profile());
@@ skipping 21 matching lines @@
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   ASSERT_TRUE(queryer.called_);
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestProtocolsWithNoDefaultAreHandled) {
   ProtocolHandler ph1 = CreateProtocolHandler("test", "test1");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   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) {
   ProtocolHandler ph1 = CreateProtocolHandler("test", "test1");
   registry()->OnAcceptRegisterProtocolHandler(ph1);
   ASSERT_TRUE(registry()->IsHandledProtocol("test"));
   registry()->Disable();
   ASSERT_FALSE(registry()->IsHandledProtocol("test"));
 }
 
@@ skipping 45 matching lines @@
 
 static void MakeRequest(const GURL& url, ProtocolHandlerRegistry* registry) {
   net::URLRequest request(url, NULL);
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           MessageLoop::QuitClosure());
   scoped_refptr<net::URLRequestJob> job(registry->MaybeCreateJob(&request));
   ASSERT_TRUE(job.get() != NULL);
 }
 
 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));
   MessageLoop::current()->Run();
 }
 
 static void CheckIsHandled(const std::string& scheme, bool expected,
     ProtocolHandlerRegistry* registry) {
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           MessageLoop::QuitClosure());
   ASSERT_EQ(expected, registry->IsHandledProtocolIO(scheme));
 }
 
 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());
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(CheckIsHandled, scheme, true, r));
 }
 
 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);
 
   ASSERT_TRUE(registry()->IsDefault(ph3));
   registry()->RemoveHandler(ph3);
   ASSERT_TRUE(registry()->IsDefault(ph2));
   registry()->OnAcceptRegisterProtocolHandler(ph3);
   ASSERT_TRUE(registry()->IsDefault(ph3));
   registry()->RemoveHandler(ph2);
   ASSERT_TRUE(registry()->IsDefault(ph3));
   registry()->RemoveHandler(ph3);
   ASSERT_TRUE(registry()->IsDefault(ph1));
 }
 
 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);
   scoped_refptr<ProtocolHandlerRegistry> r(registry());
 
   BrowserThread::PostTask(
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(CheckIsHandled, scheme, false, r));
 }
 
 static void QuitUILoop() {
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
                           MessageLoop::QuitClosure());
 }
 
 TEST_F(ProtocolHandlerRegistryTest, TestLoadEnabledGetsPropogatedToIO) {
   registry()->Disable();
   ReloadProtocolHandlerRegistry();
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(QuitUILoop));
   MessageLoop::current()->Run();
   ASSERT_FALSE(enabled_io());
 }
 
 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));
   ASSERT_EQ(ph1.url().GetOrigin() == ph2.url().GetOrigin(),
       ph2.IsSameOrigin(ph1));
   ASSERT_EQ(ph2.url().GetOrigin() == ph3.url().GetOrigin(),
       ph2.IsSameOrigin(ph3));
   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
+}