Fixing guest processes to use the proper storage partition.

Fixing guest processes to use the proper storage partition.

Pikcing up from Nasko. Original CL here: https://codereview.chromium.org/11320018/

BUG=158128

[awong, 2012-10-27 01:00:08]

Hey Charlie, ready for a review.

[Charlie Reis, 2012-10-27 01:11:43]

LGTM with nits below.

https://codereview.chromium.org/11308024/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11308024/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1613: // privileges.
nit: This comment is confusing unless you know more.  Can you clarfiy that guest
URLs have a guest:// scheme and that they may have an Extension associated with
their host?

https://codereview.chromium.org/11308024/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://codereview.chromium.org/11308024/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:91:
source2->GetWebContents()->GetRenderProcessHost()->GetStoragePartition());
Either we should get rid of this (to make it easier to remove
GetStoragePartition) or add a test for the bug (compare against the
StoragePartition of a normal tab).

[awong, 2012-10-27 01:23:54]

comments addressed.

OWNERS:
willchan -- ProfileImplIOHateMyLifeBlah
finnur -- extensions test

https://codereview.chromium.org/11308024/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11308024/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:1613: // privileges.
On 2012/10/27 01:11:43, creis wrote:
> nit: This comment is confusing unless you know more.  Can you clarfiy that
guest
> URLs have a guest:// scheme and that they may have an Extension associated
with
> their host?

Done.

https://codereview.chromium.org/11308024/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://codereview.chromium.org/11308024/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:91:
source2->GetWebContents()->GetRenderProcessHost()->GetStoragePartition());
On 2012/10/27 01:11:43, creis wrote:
> Either we should get rid of this (to make it easier to remove
> GetStoragePartition) or add a test for the bug (compare against the
> StoragePartition of a normal tab).

I TODOed it.

[willchan no longer on Chromium, 2012-10-27 01:32:53]

lgtm

[Finnur, 2012-10-29 10:14:41]

https://codereview.chromium.org/11308024/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://codereview.chromium.org/11308024/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/web_view_browsertest.cc:118: EXPECT_EQ("guest1=true;
guest2=true", cookie_value);
This is a bit hard to understand in isolation. What is the significance of
guest1=true and guest2=true?

[awong, 2012-10-29 19:02:34]

Added a comment.  It's a value set earlier in the test.

[Finnur, 2012-10-30 10:19:03]

Test LGTM.

[commit-bot: I haz the power, 2012-10-30 15:25:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/11308024/14001

[commit-bot: I haz the power, 2012-10-30 15:25:56]

Presubmit check for 11308024-14001 failed and returned exit status 1.


Running presubmit commit checks ...

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

Presubmit checks took 2.0s to calculate.

[nasko, 2012-10-30 15:27:05]

Adding thakis@ for chrome/ OWNERS review.

[Nico, 2012-10-31 01:26:05]

lgtm stamp, but some naming comments for the future below.

https://codereview.chromium.org/11308024/diff/14001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11308024/diff/14001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:494: if
(site.SchemeIs(chrome::kGuestScheme))
"guest" means "a process embedded in another process" or some such in this
context and is unrelated to cros guest accounts, right? Can this be renamed so
that that's more explicit?

https://codereview.chromium.org/11308024/diff/14001/chrome/browser/profiles/p...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/11308024/diff/14001/chrome/browser/profiles/p...
chrome/browser/profiles/profile_impl_io_data.cc:491: bool is_guest_process =
StartsWithASCII(app_id, "guest-", true);
Also, it looks like "guest-" is scattered about in many places. Can you add a
few functions to abstract this? (e.g. "IsGuestProcess(app_id)")