Incognito-allowed package apps produce duplicate items in context menu

chrome/browser/profiles/off_the_record_profile_impl.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/profiles/off_the_record_profile_impl.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
@@ skipping 107 matching lines @@
 
   ChromePluginServiceFilter::GetInstance()->RegisterResourceContext(
       PluginPrefs::GetForProfile(this), io_data_.GetResourceContextNoInit());
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&NotifyOTRProfileCreatedOnIOThread, profile_, this));
 }
 
 OffTheRecordProfileImpl::~OffTheRecordProfileImpl() {
   MaybeSendDestroyedNotification();

[sail, 2012/10/25 19:22:00]

Hm.. do you really need to change profiles code to fix this? Can't you listen to
NOTIFICATION_PROFILE_DESTROYED?

[Joe Thomas, 2012/10/25 19:49:42]

MenuManager for Extensions is owned by the ExtensionService and it is
constructed with Normal Profile instance. There is no separate MenuManager
instance created for incognito profile. So MenuManager will not get notified for
Incognito profile destruction. This is a special case in which manifest.json of
extension contains "incognito" defined as "split".

[sail, 2012/10/25 19:56:14]

This isn't idea. Simply adding every feature into profiles/* won't scale. It
seems like adding extra code to observer the incognito notifications shouldn't
be that hard.

If you really can't add it as a part of this CL then please file a bug to do it
separate and put a TODO here.

[Joe Thomas, 2012/10/26 03:49:54]

I moved this code out of chrome/browser/profile* to
extension_process_manager.cc.

[sail, 2012/10/26 04:36:30]

Thanks.

 
   ChromePluginServiceFilter::GetInstance()->UnregisterResourceContext(
     io_data_.GetResourceContextNoInit());
 
   ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(this)->extension_service();
   if (extension_service && extension_service->extensions_enabled()) {
     extension_service->extension_prefs()->
         ClearIncognitoSessionOnlyContentSettings();
+    extension_service->menu_manager()->RemoveAllIncognitoContextItems();

[sail, 2012/10/25 19:22:00]

This needs a test (something that doesn't pass without your patch and passes
with your patch).

[Joe Thomas, 2012/10/25 19:49:42]

I already added a unit test for the new function that I created in
menu_manager_unittest.cc.

Testing the complete use case requires creating and destroying incognito window
multiple times and navigate through the context menu items of incognito window.
I am not quite sure how to do that. Do we have any similar testcase that I can
look into?

[sail, 2012/10/25 19:56:14]

Take a look at profile_browsertest.cc and profile_manager_browsertest.cc.
It's fairly easy to create new incognito windows from browser tests.

   }
 
   ProfileDependencyManager::GetInstance()->DestroyProfileServices(this);
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
       base::Bind(&NotifyOTRProfileDestroyedOnIOThread, profile_, this));
 
   if (host_content_settings_map_)
     host_content_settings_map_->ShutdownOnUIThread();
@@ skipping 353 matching lines @@
   if (!profile)
     profile = new OffTheRecordProfileImpl(this);
   profile->Init();
   return profile;
 }
 
 base::Callback<ChromeURLDataManagerBackend*(void)>
     OffTheRecordProfileImpl::GetChromeURLDataManagerBackendGetter() const {
   return io_data_.GetChromeURLDataManagerBackendGetter();
 }