Add support for a "split" incognito behavior for extensions.

Add support for a "split" incognito behavior for extensions.
- On by default for apps, off by default for extensions.
- Split mode means "run incognito extensions in a separate process if the user
says OK, and the two processes can only see their own profile."
- Spanning mode is what we have now, and means "run a single extension process,
but allow it to access both profiles if the user says OK."

BUG=49232
BUG=49114
TEST=extensions still work in incognito when you check "Allow in Incognito".

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

[Matt Perry, 2010-08-27 00:25:09]

Still working on api tests.

[Aaron Boodman, 2010-08-27 18:53:52]

That was exhausting.

Strong work.

http://codereview.chromium.org/3210007/diff/2001/3001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3210007/diff/2001/3001#newcode3316
chrome/app/generated_resources.grd:3316: Incognito Extension: <ph
name="EXTENSION_NAME">$1<ex>Sample Extension</ex></ph>
As long as you're here, can you create separate labels for apps, so that we
don't refer to them as extensions in the task manager?

http://codereview.chromium.org/3210007/diff/2001/3003
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3003#newcode3059
chrome/browser/browser.cc:3059: if (profile_ == profile) {
Is this correct? Couldn't profile_ be the OTR profile?

http://codereview.chromium.org/3210007/diff/2001/3004
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3004#newcode62
chrome/browser/dom_ui/dom_ui_factory.cc:62: if (valid_extension)
Do we need to check if the extension is split mode?

http://codereview.chromium.org/3210007/diff/2001/3026
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3026#newcode36
chrome/browser/extensions/extension_process_manager.cc:36: const GURL& url,
weird indentation.

http://codereview.chromium.org/3210007/diff/2001/3026#newcode51
chrome/browser/extensions/extension_process_manager.cc:51: Extension*
GetExtensionForURL(const GURL& url);
Just to keep things clear, can you call this GetExtensionOrAppForURL? We have
methods elsewhere in the code called GetExtensionByURL that don't return
extensions.

http://codereview.chromium.org/3210007/diff/2001/3027
File chrome/browser/extensions/extension_process_manager.h (right):

http://codereview.chromium.org/3210007/diff/2001/3027#newcode56
chrome/browser/extensions/extension_process_manager.h:56: void
OpenOptionsPage(Extension* extension, Browser* browser);
This seems like a weird place to put this. I realize it was already here though.

http://codereview.chromium.org/3210007/diff/2001/3030
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3030#newcode1377
chrome/browser/extensions/extensions_service.cc:1377: // Unload the entire
extension. We want it to be in a consistent state:
Don't we only want to do this if the extension is spanning or it is split and
profile_ == source?

http://codereview.chromium.org/3210007/diff/2001/3044
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3044#newcode1528
chrome/common/extensions/extension.cc:1528: if
(!source.GetString(keys::kIncognitoBehavior, &value) || value.empty()) {
There's no need for the value.empty() check. The else clause below will catch
it.

http://codereview.chromium.org/3210007/diff/2001/3046
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3046#newcode24
chrome/common/extensions/extension_constants.cc:24: const char*
kIncognitoBehavior = "incognito_behavior";
What do you think about just calling this 'incognito'?

http://codereview.chromium.org/3210007/diff/2001/3059
File chrome/test/data/extensions/api_test/incognito/split/background.html
(right):

http://codereview.chromium.org/3210007/diff/2001/3059#newcode72
chrome/test/data/extensions/api_test/incognito/split/background.html:72: /*     
  chrome.tabs.executeScript(tab.id,
??

[Matt Perry, 2010-08-27 22:38:07]

updated

http://codereview.chromium.org/3210007/diff/2001/3001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3210007/diff/2001/3001#newcode3316
chrome/app/generated_resources.grd:3316: Incognito Extension: <ph
name="EXTENSION_NAME">$1<ex>Sample Extension</ex></ph>
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> As long as you're here, can you create separate labels for apps, so that we
> don't refer to them as extensions in the task manager?

Done, though as far as I can tell, they will always show up as "Tab".

http://codereview.chromium.org/3210007/diff/2001/3003
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3003#newcode3059
chrome/browser/browser.cc:3059: if (profile_ == profile) {
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> Is this correct? Couldn't profile_ be the OTR profile?

Yes, in which case we won't show the UI. I suppose we want to show it in all
browsers. I'll make the change.

http://codereview.chromium.org/3210007/diff/2001/3004
File chrome/browser/dom_ui/dom_ui_factory.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3004#newcode62
chrome/browser/dom_ui/dom_ui_factory.cc:62: if (valid_extension)
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> Do we need to check if the extension is split mode?

No, I think this check is redundant with the stuff in extension_resources that
prevents the resource from loading at all.

http://codereview.chromium.org/3210007/diff/2001/3026
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3026#newcode36
chrome/browser/extensions/extension_process_manager.cc:36: const GURL& url,
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> weird indentation.

Done.

http://codereview.chromium.org/3210007/diff/2001/3026#newcode51
chrome/browser/extensions/extension_process_manager.cc:51: Extension*
GetExtensionForURL(const GURL& url);
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> Just to keep things clear, can you call this GetExtensionOrAppForURL? We have
> methods elsewhere in the code called GetExtensionByURL that don't return
> extensions.

Done.

http://codereview.chromium.org/3210007/diff/2001/3030
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3030#newcode1377
chrome/browser/extensions/extensions_service.cc:1377: // Unload the entire
extension. We want it to be in a consistent state:
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> Don't we only want to do this if the extension is spanning or it is split and
> profile_ == source?

AFAICT, this message is only sent when the process crashes. It seems reasonable
to unload the extension even if its incognito process crashes. What do you
think?

http://codereview.chromium.org/3210007/diff/2001/3044
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3044#newcode1528
chrome/common/extensions/extension.cc:1528: if
(!source.GetString(keys::kIncognitoBehavior, &value) || value.empty()) {
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> There's no need for the value.empty() check. The else clause below will catch
> it.

Done.

http://codereview.chromium.org/3210007/diff/2001/3046
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3046#newcode24
chrome/common/extensions/extension_constants.cc:24: const char*
kIncognitoBehavior = "incognito_behavior";
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> What do you think about just calling this 'incognito'?

Sure, I'm down with the whole brevity thing.

http://codereview.chromium.org/3210007/diff/2001/3059
File chrome/test/data/extensions/api_test/incognito/split/background.html
(right):

http://codereview.chromium.org/3210007/diff/2001/3059#newcode72
chrome/test/data/extensions/api_test/incognito/split/background.html:72: /*     
  chrome.tabs.executeScript(tab.id,
On 2010/08/27 18:53:53, Aaron Boodman wrote:
> ??

Sorry, this was in progress. Fixed now.

[Matt Perry, 2010-08-27 22:38:09]

updated

[Aaron Boodman, 2010-08-27 22:47:52]

lgtm.

good luck w/ the landing.

http://codereview.chromium.org/3210007/diff/2001/3030
File chrome/browser/extensions/extensions_service.cc (right):

http://codereview.chromium.org/3210007/diff/2001/3030#newcode1377
chrome/browser/extensions/extensions_service.cc:1377: // Unload the entire
extension. We want it to be in a consistent state:
On 2010/08/27 22:38:08, Matt Perry wrote:
> On 2010/08/27 18:53:53, Aaron Boodman wrote:
> > Don't we only want to do this if the extension is spanning or it is split
and
> > profile_ == source?
> 
> AFAICT, this message is only sent when the process crashes. It seems
reasonable
> to unload the extension even if its incognito process crashes. What do you
> think?

Oh, that makes sense. n/m