Revert 134283. Speculative revert, it looks like this caused many leaks (http://crbug.com/125564).

Revert 134283. Speculative revert, it looks like this caused many leaks (http://crbug.com/125564).
If the revert doesn't help, I'll reland.

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


BUG=123368
TEST=None


Review URL: http://codereview.chromium.org/10139002

TBR=skuhne@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=134541

[Mr4D (OOO till 08-26), 2012-04-30 16:53:15]

I did last Friday investigations after this showed up - and according to my
debugging the new functions were only called in my own unit tests which
were not marked by Valgrid.

Let's see then.

On Mon, Apr 30, 2012 at 9:30 AM, <thakis@chromium.org> wrote:

> Reviewers: Mr4D,
>
> Description:
> Revert 134283. Speculative revert, it looks like this caused many leaks
> (http://crbug.com/125564).
> If the revert doesn't help, I'll reland.
>
> - Preventing our default handlers for ChromeOS to show up or confuse the
> user
>
>
> BUG=123368
> TEST=None
>
>
> Review URL:
http://codereview.chromium.**org/10139002<http://codereview.chromium.org/1013...
>
> TBR=skuhne@chromium.org
>
> Please review this at
https://chromiumcodereview.**appspot.com/10266012/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     chrome/app/generated_**resources.grd
>  M     chrome/browser/custom_**handlers/protocol_handler_**registry.h
>  M     chrome/browser/custom_**handlers/protocol_handler_**registry.cc
>  M     chrome/browser/custom_**handlers/protocol_handler_**
> registry_unittest.cc
>  M     chrome/browser/platform_util_**chromeos.cc
>  M     chrome/browser/profiles/**profile_impl.h
>  M     chrome/browser/profiles/**profile_impl.cc
>
>
> Index: chrome/app/generated_**resources.grd
> ==============================**==============================**=======
> --- chrome/app/generated_**resources.grd  (revision 134540)
> +++ chrome/app/generated_**resources.grd  (working copy)
> @@ -11652,28 +11652,6 @@
>         </message>
>       </if>
>
> -      <if expr="pp_ifdef('chromeos')">
> -        <!-- The URL for the Google mailto service. -->
> -        <message name="IDS_GOOGLE_MAILTO_**HANDLER_URL">
> -          https:////mail.google.com//**mail//?extsrc=mailto&amp;url=%**s
> -        </message>
> -
> -        <!-- The name for the Google mailto service. -->
> -        <message name="IDS_GOOGLE_MAILTO_**HANDLER_NAME">
> -          Google.com Mail
> -        </message>
> -
> -        <!-- The URL for the Google webcal service. -->
> -        <message name="IDS_GOOGLE_WEBCAL_**HANDLER_URL">
> -          https:////www.google.com//**calendar//render?cid=%s
> -        </message>
> -
> -        <!-- The name for the Google webcal service. -->
> -        <message name="IDS_GOOGLE_WEBCAL_**HANDLER_NAME">
> -          Google Calendar
> -        </message>
> -      </if>
> -
>       <!-- Sync promo page chrome://signin -->
>       <message name="IDS_SYNC_PROMO_TAB_**TITLE" desc="The title of the
> sync promo tab.">
>         Sign in
> Index: chrome/browser/custom_**handlers/protocol_handler_**registry.cc
> ==============================**==============================**=======
> --- chrome/browser/custom_**handlers/protocol_handler_**registry.cc
> (revision 134540)
> +++ chrome/browser/custom_**handlers/protocol_handler_**registry.cc
> (working copy)
> @@ -151,8 +151,7 @@
>       delegate_(delegate),
>       enabled_(true),
>       enabled_io_(enabled_),
> -      is_loading_(false),
> -      is_loaded_(false) {
> +      is_loading_(false) {
>  }
>
>  bool ProtocolHandlerRegistry::**SilentlyHandleRegisterHandlerR**equest(
> @@ -248,8 +247,6 @@
>  }
>
>  void ProtocolHandlerRegistry::Load(**) {
> -  // Any further default additions to the table will get rejected from
> now on.
> -  is_loaded_ = true;
>   DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::UI)**);
>   is_loading_ = true;
>   PrefService* prefs = profile_->GetPrefs();
> @@ -711,13 +708,3 @@
>   DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::UI)**);
>   ignored_protocol_handlers_.**push_back(handler);
>  }
> -
> -void ProtocolHandlerRegistry::**AddDefaultHandler(
> -    const ProtocolHandler& handler) {
> -  // If called after the load command was issued this function will fail.
> -  DCHECK(!is_loaded_);
> -  RegisterProtocolHandler(**handler);
> -  SetDefault(handler);
> -}
> -
> -
> Index: chrome/browser/custom_**handlers/protocol_handler_**registry.h
> ==============================**==============================**=======
> --- chrome/browser/custom_**handlers/protocol_handler_**registry.h
>  (revision 134540)
> +++ chrome/browser/custom_**handlers/protocol_handler_**registry.h
>  (working copy)
> @@ -191,10 +191,6 @@
>
>   bool enabled() const { return enabled_; }
>
> -  // Add a predefined protocol handler. This has to be called before the
> first
> -  // load command was issued, otherwise the command will be ignored.
> -  void AddDefaultHandler(const ProtocolHandler& handler);
> -
>  private:
>   friend class base::DeleteHelper<**ProtocolHandlerRegistry>;
>   friend struct content::BrowserThread::**DeleteOnThread<
> @@ -284,10 +280,6 @@
>   // Whether or not we are loading.
>   bool is_loading_;
>
> -  // When the table gets loaded this flag will be set and any further
> calls to
> -  // AddDefaultHandler will be rejected.
> -  bool is_loaded_;
> -
>   DefaultClientObserverList default_client_observers_;
>
>   // Copy of default_handlers_ that is only accessed on the IO thread.
> Index: chrome/browser/custom_**handlers/protocol_handler_**
> registry_unittest.cc
> ==============================**==============================**=======
> --- chrome/browser/custom_**handlers/protocol_handler_**registry_unittest.cc
>        (revision 134540)
> +++ chrome/browser/custom_**handlers/protocol_handler_**registry_unittest.cc
>        (working copy)
> @@ -229,16 +229,6 @@
>     registry_->Load();
>   }
>
> -  void ReloadProtocolHandlerRegistryA**ndInstallDefaultHandler() {
> -    delegate_ = new FakeDelegate();
> -    registry_->Finalize();
> -    registry_ = NULL;
> -    registry_ = new ProtocolHandlerRegistry(**profile(), delegate());
> -    registry_->AddDefaultHandler(**CreateProtocolHandler(
> -        "test", GURL("http://test.com/%s"), "Test"));
> -    registry_->Load();
> -  }
> -
>   virtual void SetUp() {
>     ui_message_loop_.reset(new MessageLoopForUI());
>     ui_thread_.reset(new content::TestBrowserThread(**BrowserThread::UI,
> @@ -806,10 +796,3 @@
>   ASSERT_EQ(ph3.url().GetOrigin(**) == ph2.url().GetOrigin(),
>       ph3.IsSameOrigin(ph2));
>  }
> -
> -TEST_F(**ProtocolHandlerRegistryTest, TestInstallDefaultHandler) {
> -  ReloadProtocolHandlerRegistryA**ndInstallDefaultHandler();
> -  std::vector<std::string> protocols;
> -  registry()->**GetRegisteredProtocols(&**protocols);
> -  ASSERT_EQ(static_cast<size_t>(**1), protocols.size());
> -}
> Index: chrome/browser/platform_util_**chromeos.cc
> ==============================**==============================**=======
> --- chrome/browser/platform_util_**chromeos.cc    (revision 134540)
> +++ chrome/browser/platform_util_**chromeos.cc    (working copy)
> @@ -55,15 +55,6 @@
>  }
>
>  void OpenExternal(const GURL& url) {
> -  // This code should be obsolete since we have default handlers in
> ChromeOS
> -  // which should handle this. However - there are two things which make
> it
> -  // necessary to keep it in:
> -  // a.) The user might have deleted the default handler in this session.
> -  //     In this case we would need to have this in place.
> -  // b.) There are several code paths which are not clear if they would
> call
> -  //     this function directly and which would therefore break (e.g.
> -  //     "Browser::EmailPageLocation" (to name only one).
> -  // As such we should keep this code here.
>   if (url.SchemeIs("mailto")) {
>     std::string string_url = kGmailComposeUrl;
>     string_url.append(url.spec());
> Index: chrome/browser/profiles/**profile_impl.cc
> ==============================**==============================**=======
> --- chrome/browser/profiles/**profile_impl.cc     (revision 134540)
> +++ chrome/browser/profiles/**profile_impl.cc     (working copy)
> @@ -83,7 +83,6 @@
>  #include "content/public/browser/**notification_service.h"
>  #include "content/public/browser/user_**metrics.h"
>  #include "grit/chromium_strings.h"
> -#include "grit/generated_resources.h"
>  #include "ui/base/l10n/l10n_util.h"
>
>  #if defined(OS_WIN)
> @@ -443,28 +442,9 @@
>     return;
>   protocol_handler_registry_ = new ProtocolHandlerRegistry(this,
>       new ProtocolHandlerRegistry::**Delegate());
> -
> -  // Install predefined protocol handlers.
> -  InstallDefaultProtocolHandlers**();
> -
>   protocol_handler_registry_->**Load();
>  }
>
> -void ProfileImpl::**InstallDefaultProtocolHandlers**() {
> -#if defined(OS_CHROMEOS)
> -  protocol_handler_registry_->**AddDefaultHandler(
> -      ProtocolHandler::**CreateProtocolHandler(
> -          "mailto",
> -          GURL(l10n_util::GetStringUTF8(**IDS_GOOGLE_MAILTO_HANDLER_URL)*
> *),
> -          l10n_util::GetStringUTF16(IDS_**GOOGLE_MAILTO_HANDLER_NAME)));
> -  protocol_handler_registry_->**AddDefaultHandler(
> -      ProtocolHandler::**CreateProtocolHandler(
> -          "webcal",
> -          GURL(l10n_util::GetStringUTF8(**IDS_GOOGLE_WEBCAL_HANDLER_URL)*
> *),
> -          l10n_util::GetStringUTF16(IDS_**GOOGLE_WEBCAL_HANDLER_NAME)));
> -#endif
> -}
> -
>  FilePath ProfileImpl::last_selected_**directory() {
>   return GetPrefs()->GetFilePath(prefs:**:kSelectFileLastDirectory);
>  }
> Index: chrome/browser/profiles/**profile_impl.h
> ==============================**==============================**=======
> --- chrome/browser/profiles/**profile_impl.h      (revision 134540)
> +++ chrome/browser/profiles/**profile_impl.h      (working copy)
> @@ -148,9 +148,6 @@
>
>   void InitHostZoomMap();
>
> -  // The installation of any pre-defined protocol handlers.
> -  void InstallDefaultProtocolHandlers**();
> -
>   // Does final prefs initialization and calls Init().
>   void OnPrefsLoaded(bool success);
>
>
>
>

[Nico, 2012-04-30 16:58:04]

(valgrind)(6) has cycled green after the revert:
http://build.chromium.org/p/chromium.memory.fyi/console (click "merge"
at the very bottom). Because of that, I expect that the others will
cycle green too.

On Mon, Apr 30, 2012 at 9:53 AM, Stefan Kuhne <skuhne@chromium.org> wrote:
> I did last Friday investigations after this showed up - and according to my
> debugging the new functions were only called in my own unit tests which were
> not marked by Valgrid.
>
> Let's see then.
>
> On Mon, Apr 30, 2012 at 9:30 AM, <thakis@chromium.org> wrote:
>>
>> Reviewers: Mr4D,
>>
>> Description:
>> Revert 134283. Speculative revert, it looks like this caused many leaks
>> (http://crbug.com/125564).
>> If the revert doesn't help, I'll reland.
>>
>> - Preventing our default handlers for ChromeOS to show up or confuse the
>> user
>>
>>
>> BUG=123368
>> TEST=None
>>
>>
>> Review URL: http://codereview.chromium.org/10139002
>>
>> TBR=skuhne@chromium.org
>>
>> Please review this at https://chromiumcodereview.appspot.com/10266012/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>>
>> Affected files:
>>  M     chrome/app/generated_resources.grd
>>  M     chrome/browser/custom_handlers/protocol_handler_registry.h
>>  M     chrome/browser/custom_handlers/protocol_handler_registry.cc
>>  M
>> chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
>>  M     chrome/browser/platform_util_chromeos.cc
>>  M     chrome/browser/profiles/profile_impl.h
>>  M     chrome/browser/profiles/profile_impl.cc
>>
>>
>> Index: chrome/app/generated_resources.grd
>> ===================================================================
>> --- chrome/app/generated_resources.grd  (revision 134540)
>> +++ chrome/app/generated_resources.grd  (working copy)
>> @@ -11652,28 +11652,6 @@
>>         </message>
>>       </if>
>>
>> -      <if expr="pp_ifdef('chromeos')">
>> -        <!-- The URL for the Google mailto service. -->
>> -        <message name="IDS_GOOGLE_MAILTO_HANDLER_URL">
>> -          https:////mail.google.com//mail//?extsrc=mailto&amp;url=%s
>> -        </message>
>> -
>> -        <!-- The name for the Google mailto service. -->
>> -        <message name="IDS_GOOGLE_MAILTO_HANDLER_NAME">
>> -          Google.com Mail
>> -        </message>
>> -
>> -        <!-- The URL for the Google webcal service. -->
>> -        <message name="IDS_GOOGLE_WEBCAL_HANDLER_URL">
>> -          https:////www.google.com//calendar//render?cid=%s
>> -        </message>
>> -
>> -        <!-- The name for the Google webcal service. -->
>> -        <message name="IDS_GOOGLE_WEBCAL_HANDLER_NAME">
>> -          Google Calendar
>> -        </message>
>> -      </if>
>> -
>>       <!-- Sync promo page chrome://signin -->
>>       <message name="IDS_SYNC_PROMO_TAB_TITLE" desc="The title of the sync
>> promo tab.">
>>         Sign in
>> Index: chrome/browser/custom_handlers/protocol_handler_registry.cc
>> ===================================================================
>> --- chrome/browser/custom_handlers/protocol_handler_registry.cc (revision
>> 134540)
>> +++ chrome/browser/custom_handlers/protocol_handler_registry.cc (working
>> copy)
>> @@ -151,8 +151,7 @@
>>       delegate_(delegate),
>>       enabled_(true),
>>       enabled_io_(enabled_),
>> -      is_loading_(false),
>> -      is_loaded_(false) {
>> +      is_loading_(false) {
>>  }
>>
>>  bool ProtocolHandlerRegistry::SilentlyHandleRegisterHandlerRequest(
>> @@ -248,8 +247,6 @@
>>  }
>>
>>  void ProtocolHandlerRegistry::Load() {
>> -  // Any further default additions to the table will get rejected from
>> now on.
>> -  is_loaded_ = true;
>>   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
>>   is_loading_ = true;
>>   PrefService* prefs = profile_->GetPrefs();
>> @@ -711,13 +708,3 @@
>>   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
>>   ignored_protocol_handlers_.push_back(handler);
>>  }
>> -
>> -void ProtocolHandlerRegistry::AddDefaultHandler(
>> -    const ProtocolHandler& handler) {
>> -  // If called after the load command was issued this function will fail.
>> -  DCHECK(!is_loaded_);
>> -  RegisterProtocolHandler(handler);
>> -  SetDefault(handler);
>> -}
>> -
>> -
>> Index: chrome/browser/custom_handlers/protocol_handler_registry.h
>> ===================================================================
>> --- chrome/browser/custom_handlers/protocol_handler_registry.h  (revision
>> 134540)
>> +++ chrome/browser/custom_handlers/protocol_handler_registry.h  (working
>> copy)
>> @@ -191,10 +191,6 @@
>>
>>   bool enabled() const { return enabled_; }
>>
>> -  // Add a predefined protocol handler. This has to be called before the
>> first
>> -  // load command was issued, otherwise the command will be ignored.
>> -  void AddDefaultHandler(const ProtocolHandler& handler);
>> -
>>  private:
>>   friend class base::DeleteHelper<ProtocolHandlerRegistry>;
>>   friend struct content::BrowserThread::DeleteOnThread<
>> @@ -284,10 +280,6 @@
>>   // Whether or not we are loading.
>>   bool is_loading_;
>>
>> -  // When the table gets loaded this flag will be set and any further
>> calls to
>> -  // AddDefaultHandler will be rejected.
>> -  bool is_loaded_;
>> -
>>   DefaultClientObserverList default_client_observers_;
>>
>>   // Copy of default_handlers_ that is only accessed on the IO thread.
>> Index:
>> chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
>> ===================================================================
>> --- chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
>>      (revision 134540)
>> +++ chrome/browser/custom_handlers/protocol_handler_registry_unittest.cc
>>      (working copy)
>> @@ -229,16 +229,6 @@
>>     registry_->Load();
>>   }
>>
>> -  void ReloadProtocolHandlerRegistryAndInstallDefaultHandler() {
>> -    delegate_ = new FakeDelegate();
>> -    registry_->Finalize();
>> -    registry_ = NULL;
>> -    registry_ = new ProtocolHandlerRegistry(profile(), delegate());
>> -    registry_->AddDefaultHandler(CreateProtocolHandler(
>> -        "test", GURL("http://test.com/%s"), "Test"));
>> -    registry_->Load();
>> -  }
>> -
>>   virtual void SetUp() {
>>     ui_message_loop_.reset(new MessageLoopForUI());
>>     ui_thread_.reset(new content::TestBrowserThread(BrowserThread::UI,
>> @@ -806,10 +796,3 @@
>>   ASSERT_EQ(ph3.url().GetOrigin() == ph2.url().GetOrigin(),
>>       ph3.IsSameOrigin(ph2));
>>  }
>> -
>> -TEST_F(ProtocolHandlerRegistryTest, TestInstallDefaultHandler) {
>> -  ReloadProtocolHandlerRegistryAndInstallDefaultHandler();
>> -  std::vector<std::string> protocols;
>> -  registry()->GetRegisteredProtocols(&protocols);
>> -  ASSERT_EQ(static_cast<size_t>(1), protocols.size());
>> -}
>> Index: chrome/browser/platform_util_chromeos.cc
>> ===================================================================
>> --- chrome/browser/platform_util_chromeos.cc    (revision 134540)
>> +++ chrome/browser/platform_util_chromeos.cc    (working copy)
>> @@ -55,15 +55,6 @@
>>  }
>>
>>  void OpenExternal(const GURL& url) {
>> -  // This code should be obsolete since we have default handlers in
>> ChromeOS
>> -  // which should handle this. However - there are two things which make
>> it
>> -  // necessary to keep it in:
>> -  // a.) The user might have deleted the default handler in this session.
>> -  //     In this case we would need to have this in place.
>> -  // b.) There are several code paths which are not clear if they would
>> call
>> -  //     this function directly and which would therefore break (e.g.
>> -  //     "Browser::EmailPageLocation" (to name only one).
>> -  // As such we should keep this code here.
>>   if (url.SchemeIs("mailto")) {
>>     std::string string_url = kGmailComposeUrl;
>>     string_url.append(url.spec());
>> Index: chrome/browser/profiles/profile_impl.cc
>> ===================================================================
>> --- chrome/browser/profiles/profile_impl.cc     (revision 134540)
>> +++ chrome/browser/profiles/profile_impl.cc     (working copy)
>> @@ -83,7 +83,6 @@
>>  #include "content/public/browser/notification_service.h"
>>  #include "content/public/browser/user_metrics.h"
>>  #include "grit/chromium_strings.h"
>> -#include "grit/generated_resources.h"
>>  #include "ui/base/l10n/l10n_util.h"
>>
>>  #if defined(OS_WIN)
>> @@ -443,28 +442,9 @@
>>     return;
>>   protocol_handler_registry_ = new ProtocolHandlerRegistry(this,
>>       new ProtocolHandlerRegistry::Delegate());
>> -
>> -  // Install predefined protocol handlers.
>> -  InstallDefaultProtocolHandlers();
>> -
>>   protocol_handler_registry_->Load();
>>  }
>>
>> -void ProfileImpl::InstallDefaultProtocolHandlers() {
>> -#if defined(OS_CHROMEOS)
>> -  protocol_handler_registry_->AddDefaultHandler(
>> -      ProtocolHandler::CreateProtocolHandler(
>> -          "mailto",
>> -          GURL(l10n_util::GetStringUTF8(IDS_GOOGLE_MAILTO_HANDLER_URL)),
>> -          l10n_util::GetStringUTF16(IDS_GOOGLE_MAILTO_HANDLER_NAME)));
>> -  protocol_handler_registry_->AddDefaultHandler(
>> -      ProtocolHandler::CreateProtocolHandler(
>> -          "webcal",
>> -          GURL(l10n_util::GetStringUTF8(IDS_GOOGLE_WEBCAL_HANDLER_URL)),
>> -          l10n_util::GetStringUTF16(IDS_GOOGLE_WEBCAL_HANDLER_NAME)));
>> -#endif
>> -}
>> -
>>  FilePath ProfileImpl::last_selected_directory() {
>>   return GetPrefs()->GetFilePath(prefs::kSelectFileLastDirectory);
>>  }
>> Index: chrome/browser/profiles/profile_impl.h
>> ===================================================================
>> --- chrome/browser/profiles/profile_impl.h      (revision 134540)
>> +++ chrome/browser/profiles/profile_impl.h      (working copy)
>> @@ -148,9 +148,6 @@
>>
>>   void InitHostZoomMap();
>>
>> -  // The installation of any pre-defined protocol handlers.
>> -  void InstallDefaultProtocolHandlers();
>> -
>>   // Does final prefs initialization and calls Init().
>>   void OnPrefsLoaded(bool success);
>>
>>
>>
>