Incognito-allowed package apps produce duplicate items in context menu

Incognito-allowed package apps produce duplicate items in context menu

Clean-up all context menu items created for 'incognito' 'split' mode
while destroying incognito profile.

Patch from Joe Thomas <mhx348@motorola.com>;.

BUG=138487
TEST=As mentioned in the bug report


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165029

[asargent_no_longer_on_chrome, 2012-10-24 00:21:10]

This seems reasonable overall. Can you please add a browser test to verify the
change?

Also, what happens when you uncheck the "incognito" checkbox on the
chrome://extensions page? Do we get a EXTENSION_UNLOADED notification followed
by a EXTENSION_LOADED notification?

[Joe Thomas, 2012-10-24 02:25:03]

Thanks for the review.

On 2012/10/24 00:21:10, Antony Sargent wrote:
> This seems reasonable overall. Can you please add a browser test to verify the
> change?

Done.

> 
> Also, what happens when you uncheck the "incognito" checkbox on the
> chrome://extensions page? Do we get a EXTENSION_UNLOADED notification followed
> by a EXTENSION_LOADED notification?

Yes, there is a NOTIFICATION_EXTENSION_UNLOADED notification followed by
NOTIFICATION_EXTENSION_LOADED when we uncheck the "Allow in Incognito" checkbox.

[Joe Thomas, 2012-10-25 17:48:52]

@asargent:

Could you please review the patch again.?

Thanks
Joe

[asargent_no_longer_on_chrome, 2012-10-25 17:49:31]

LGTM - want me to hit the commit queue button for you?

[commit-bot: I haz the power, 2012-10-25 17:50:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11226037/4001

[commit-bot: I haz the power, 2012-10-25 17:51:03]

Presubmit check for 11226037-4001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/profiles

Presubmit checks took 2.5s to calculate.

[Joe Thomas, 2012-10-25 17:51:19]

On 2012/10/25 17:49:31, Antony Sargent wrote:
> LGTM - want me to hit the commit queue button for you?

Thanks for the review. I just did it. :)

[Joe Thomas, 2012-10-25 18:00:36]

@sail:

Could you please review chrome/browser/profiles ?

Thanks
Joe

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

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:128:
MaybeSendDestroyedNotification();
Hm.. do you really need to change profiles code to fix this? Can't you listen to
NOTIFICATION_PROFILE_DESTROYED?

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:138:
extension_service->menu_manager()->RemoveAllIncognitoContextItems();
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]

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:128:
MaybeSendDestroyedNotification();
On 2012/10/25 19:22:00, sail wrote:
> Hm.. do you really need to change profiles code to fix this? Can't you listen
to
> NOTIFICATION_PROFILE_DESTROYED?

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".

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:138:
extension_service->menu_manager()->RemoveAllIncognitoContextItems();
On 2012/10/25 19:22:00, sail wrote:
> This needs a test (something that doesn't pass without your patch and passes
> with your patch).

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]

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:128:
MaybeSendDestroyedNotification();
On 2012/10/25 19:49:42, Joe Thomas wrote:
> On 2012/10/25 19:22:00, sail wrote:
> > Hm.. do you really need to change profiles code to fix this? Can't you
listen
> to
> > NOTIFICATION_PROFILE_DESTROYED?
> 
> 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".

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.

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:138:
extension_service->menu_manager()->RemoveAllIncognitoContextItems();
On 2012/10/25 19:49:42, Joe Thomas wrote:
> On 2012/10/25 19:22:00, sail wrote:
> > This needs a test (something that doesn't pass without your patch and passes
> > with your patch).
> 
> 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?

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

[Joe Thomas, 2012-10-26 03:49:53]

@Antony Sargent:

Could you please review chrome/browser/extensions/extension_process_manager.cc? 
I moved the code in off_the_record_profile_impl.cc  to here.

Thanks
Joe

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:128:
MaybeSendDestroyedNotification();
On 2012/10/25 19:56:14, sail wrote:
> On 2012/10/25 19:49:42, Joe Thomas wrote:
> > On 2012/10/25 19:22:00, sail wrote:
> > > Hm.. do you really need to change profiles code to fix this? Can't you
> listen
> > to
> > > NOTIFICATION_PROFILE_DESTROYED?
> > 
> > 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".
> 
> 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.

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

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

This code still requires a test (as per my pervious comment).

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
File chrome/browser/profiles/off_the_record_profile_impl.cc (right):

https://codereview.chromium.org/11226037/diff/4001/chrome/browser/profiles/of...
chrome/browser/profiles/off_the_record_profile_impl.cc:128:
MaybeSendDestroyedNotification();
On 2012/10/26 03:49:54, Joe Thomas wrote:
> On 2012/10/25 19:56:14, sail wrote:
> > On 2012/10/25 19:49:42, Joe Thomas wrote:
> > > On 2012/10/25 19:22:00, sail wrote:
> > > > Hm.. do you really need to change profiles code to fix this? Can't you
> > listen
> > > to
> > > > NOTIFICATION_PROFILE_DESTROYED?
> > > 
> > > 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".
> > 
> > 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.
> 
> I moved this code out of chrome/browser/profile* to
> extension_process_manager.cc.

Thanks.

[Joe Thomas, 2012-10-26 15:58:16]

On 2012/10/26 04:36:30, sail wrote:
> This code still requires a test (as per my pervious comment).
> 

Apologies if I am wrong, I am still not convinced that the code change requires
a browser test covering the entire usecase. I already added a unit test for the
new method that I introduced.

I went through the browser tests and I don't see a new browser test for each and
every bug. I am new to chromium, please correct me if I am wrong, and I can add
the test if necessary.

[Joe Thomas, 2012-10-26 16:00:42]

On 2012/10/26 04:36:30, sail wrote:
> This code still requires a test (as per my pervious comment).
> 

Apologies if I am wrong, I am still not convinced that the code change requires
a browser test covering the entire usecase. I already added a unit test for the
new method that I introduced.

I went through the browser tests and I don't see a new browser test for each and
every bug. I am new to chromium, please correct me if I am wrong, and I can add
the test if necessary.

[sail, 2012-10-26 17:14:05]

On 2012/10/26 16:00:42, Joe Thomas wrote:
> On 2012/10/26 04:36:30, sail wrote:
> > This code still requires a test (as per my pervious comment).
> > 
> 
> Apologies if I am wrong, I am still not convinced that the code change
requires
> a browser test covering the entire usecase. I already added a unit test for
the
> new method that I introduced.
> 
> I went through the browser tests and I don't see a new browser test for each
and
> every bug. I am new to chromium, please correct me if I am wrong, and I can
add
> the test if necessary.

Fixing a bug without a test is not very helpful. Most likely this will regress
again and we'll have to fix it all over again.

A test should be a part of each and every bug fix. Whether the test is a unit
test or a browser test is irrelevant.

Your unit test only tests the utility function you've added. You haven't added a
 test to verify that the bug is actually fixed.

[Joe Thomas, 2012-10-29 19:26:08]

Added Browser test.

@Antony Sargent:

Could you please review the CL again?

Thanks
Joe

[Joe Thomas, 2012-10-29 19:33:23]

On 2012/10/26 17:14:05, sail wrote:
> On 2012/10/26 16:00:42, Joe Thomas wrote:
> > On 2012/10/26 04:36:30, sail wrote:
> > > This code still requires a test (as per my pervious comment).
> > > 
> > 
> > Apologies if I am wrong, I am still not convinced that the code change
> requires
> > a browser test covering the entire usecase. I already added a unit test for
> the
> > new method that I introduced.
> > 
> > I went through the browser tests and I don't see a new browser test for each
> and
> > every bug. I am new to chromium, please correct me if I am wrong, and I can
> add
> > the test if necessary.
> 
> Fixing a bug without a test is not very helpful. Most likely this will regress
> again and we'll have to fix it all over again.
> 
> A test should be a part of each and every bug fix. Whether the test is a unit
> test or a browser test is irrelevant.
> 
> Your unit test only tests the utility function you've added. You haven't added
a
>  test to verify that the bug is actually fixed.

Thanks sail for the clarification. I added a browser test. Please review.

[sail, 2012-10-29 20:55:06]

Thanks for the test. LGTM

[Joe Thomas, 2012-10-29 20:58:29]

On 2012/10/29 20:55:06, sail wrote:
> Thanks for the test. LGTM

Thanks for the review. Do I need another LGTM from Antony Sargent to hit the
commit button as I modified extension_process_manager.cc after his last review?

[sail, 2012-10-29 21:01:51]

On 2012/10/29 20:58:29, Joe Thomas wrote:
> On 2012/10/29 20:55:06, sail wrote:
> > Thanks for the test. LGTM
> 
> Thanks for the review. Do I need another LGTM from Antony Sargent to hit the
> commit button as I modified extension_process_manager.cc after his last
review?

I'd wait for a LGTM from an extensions/* OWNER.
Your change to extension_process_manager.cc looks good to me but I don't know
all the details about how that code works.

[Joe Thomas, 2012-10-29 21:13:08]

@Antony Sargent:

Could you please review extension_process_manager.cc?

Thanks
Joe

[Yoyo Zhou, 2012-10-29 21:26:51]

https://codereview.chromium.org/11226037/diff/18001/chrome/browser/extensions...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/11226037/diff/18001/chrome/browser/extensions...
chrome/browser/extensions/extension_process_manager.cc:626: Profile* profile =
content::Source<Profile>(source).ptr();
Note that only the incognito profile's ExtensionProcessManager will see this
event from the incognito profile. So you could do this cleanup in the destructor
of IncognitoExtensionProcessManager instead, since it has the same lifetime as
the incognito profile.

This is probably okay for now (to fix this bug), but there's definitely a
modularity issue, since this code isn't related to the ExtensionProcessManager,
but only to the lifetime of the incognito profile. Can you add a
TODO(yoz): This cleanup code belongs in the MenuManager.

[Yoyo Zhou, 2012-10-29 22:23:40]

LGTM for extension_process_manager, but please fix nit before submitting

https://codereview.chromium.org/11226037/diff/18003/chrome/browser/extensions...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/11226037/diff/18003/chrome/browser/extensions...
chrome/browser/extensions/extension_process_manager.cc:826:
IncognitoExtensionProcessManager::~IncognitoExtensionProcessManager() {
Please keep the function definitions in the same order as the declarations. (You
can also move ExtensionProcessManager::ViewSet to a more correct location in
this file.)

[Joe Thomas, 2012-10-29 23:04:42]

https://codereview.chromium.org/11226037/diff/18003/chrome/browser/extensions...
File chrome/browser/extensions/extension_process_manager.cc (right):

https://codereview.chromium.org/11226037/diff/18003/chrome/browser/extensions...
chrome/browser/extensions/extension_process_manager.cc:826:
IncognitoExtensionProcessManager::~IncognitoExtensionProcessManager() {
On 2012/10/29 22:23:40, Yoyo Zhou wrote:
> Please keep the function definitions in the same order as the declarations.
(You
> can also move ExtensionProcessManager::ViewSet to a more correct location in
> this file.)

Done.

Apparently there are other functions in ExtensionProcessManager and
IncognitoExtensionProcessManager which are out-of-order. I will create a CL to
correct that once this lands.

[commit-bot: I haz the power, 2012-10-29 23:05:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11226037/27001

[commit-bot: I haz the power, 2012-10-29 23:46:34]

Retried try job too often for step(s) browser_tests

[Joe Thomas, 2012-10-30 02:01:26]

On 2012/10/29 23:46:34, I haz the power (commit-bot) wrote:
> Retried try job too often for step(s) browser_tests

There is no ExtensionService created while executing
OffTheRecordProfileImplTest.GetHostZoomMap unittest and so it crashed in the
IncognitoExtensionProcessManager destructor. Added a null check for
ExtensionService in the IncognitoExtensionProcessManager destructor.

@Yoyo Zhou: Could you please review it again? Thanks.

[Yoyo Zhou, 2012-10-30 20:21:46]

LGTM

[commit-bot: I haz the power, 2012-10-30 20:21:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/11226037/21006

[commit-bot: I haz the power, 2012-10-30 22:14:33]

Change committed as 165029