Refactor guest view availability to be API not permission based.

Refactor guest view availability to be API based not permission based.
This is a step towards allowing WebUI to directly embed guest views. 

BUG=386838
R=fsamuel@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288407

[not at google - send to devlin, 2014-07-31 22:07:46]

Hey Fady WDYT about this approach? All the tests seem to be passing.

https://codereview.chromium.org/426593007/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/426593007/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_base.cc:121: // TODO: determine the correct
context type from the process ID.
There is a CL I need to write before being able to implement this, but it'll be
quite simple.

https://codereview.chromium.org/426593007/diff/1/chrome/browser/guest_view/gu...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/426593007/diff/1/chrome/browser/guest_view/gu...
chrome/browser/guest_view/guest_view_manager.h:60: content::WebContents*
embedder_web_contents,
Pity I needed to do this, all I want is a URL.

https://codereview.chromium.org/426593007/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/426593007/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/_api_features.json:367: },
e.g. I would change this rule to be:

"extensionOptionsInternal": [{
  "internal": true,
  "contexts": ["blessed_extension"],
  "dependencies": ["permission:embeddedExtensionOptions"]
}, {
  "internal": true,
  "contexts": ["webui"],
  "matches": ["chrome://extensions/*",
              "chrome://extensions-frame:/*"]
}]

[Fady Samuel, 2014-07-31 22:25:12]

Try the WebViewInteractiveTest.* that tests the new window API. Does this work
with that?

[not at google - send to devlin, 2014-07-31 22:53:34]

On 2014/07/31 22:25:12, Fady Samuel wrote:
> Try the WebViewInteractiveTest.* that tests the new window API. Does this work
> with that?

Yep, those pass. I had to revert the _api_features.json change to make the app
view tests pass, which means currently anything can embed app view. When I add
the process type checks it'll be restricted to extension processes.

[not at google - send to devlin, 2014-08-07 00:25:29]

Fady, the precursor CL is in the CQ. I've based this CL off that one, but am
running a real tryjob here:

https://codereview.chromium.org/444133003

PTAL a more serious look at this. I've marked the places I'll be making changes
tomorrow, but you have 3 hours on me.

https://codereview.chromium.org/426593007/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/app_view/app_view_guest.cc (right):

https://codereview.chromium.org/426593007/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/app_view/app_view_guest.cc:142: "appViewInternal");
I'll define symbolic constants for each of these "appViewInternal" etc.

https://codereview.chromium.org/426593007/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/426593007/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:131: if
(availability.is_available()) {
err, sorry about this reorganisation. I'll change it back.

https://codereview.chromium.org/426593007/diff/40001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/426593007/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.h:152: // This Feature should be an
API feature so that it can declare the contexts
This comment, and the boilerplate, is making me consider whether this method
should actually return a string representing the API name.

A bit odd, and less generic, but given the comment... safer.

https://codereview.chromium.org/426593007/diff/40001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.h:172: content::WebContents*
web_contents,
I think I'll just replace the |embedder_render_process_id| argument with this
|web_contents|. Rename to |embedder_web_contents|. The process ID can be
determined from the WebContents.

[Fady Samuel, 2014-08-07 17:29:31]

https://codereview.chromium.org/426593007/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/426593007/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:125: feature->IsAvailableToContext(
appViewInternal is available in all apps I think. Does that mean <appview> will
work anywhere?

https://codereview.chromium.org/426593007/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:131: if
(availability.is_available()) {
Early exit preferred:

if (!availability.is_available()) {
  callback.Run(NULL);
  return;
}

[not at google - send to devlin, 2014-08-07 17:34:41]

https://codereview.chromium.org/426593007/diff/60001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/426593007/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:125: feature->IsAvailableToContext(
On 2014/08/07 17:29:31, Fady Samuel wrote:
> appViewInternal is available in all apps I think. Does that mean <appview>
will
> work anywhere?

appViewInternal is defined like:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

so yes it means that <appview> will work for any app or extension (on dev
channel). do we need to separate the availability of the appview embedder and
the appview guest? i'm not sure what the model is supposed to be for appview.

https://codereview.chromium.org/426593007/diff/60001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:131: if
(availability.is_available()) {
On 2014/08/07 17:29:31, Fady Samuel wrote:
> Early exit preferred:
> 
> if (!availability.is_available()) {
>   callback.Run(NULL);
>   return;
> }

yeah this was an accidental change :)

[not at google - send to devlin, 2014-08-07 21:25:41]

aaand done.

[Fady Samuel, 2014-08-07 23:01:19]

https://codereview.chromium.org/426593007/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/426593007/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.cc:115: CHECK(feature);
Do we really want to kill the browser in this case?

https://codereview.chromium.org/426593007/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/426593007/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.h:150: virtual const char*
GetApiNamespace() = 0;
nit: Should this be GetAPINamespace?

[not at google - send to devlin, 2014-08-07 23:02:56]

https://codereview.chromium.org/426593007/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/426593007/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.cc:115: CHECK(feature);
On 2014/08/07 23:01:18, Fady Samuel wrote:
> Do we really want to kill the browser in this case?

yes, because it can't possibly happen unless somebody mistypes the API name, and
that would be bad, and we don't want to hide that.

https://codereview.chromium.org/426593007/diff/120001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_base.h (right):

https://codereview.chromium.org/426593007/diff/120001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.h:150: virtual const char*
GetApiNamespace() = 0;
On 2014/08/07 23:01:18, Fady Samuel wrote:
> nit: Should this be GetAPINamespace?

up to you I suppose, I'm not fussed. GetAPINamespace looked odd to me, but now
GetApiNamespace looks odd. i guess it depends on my mood.

[not at google - send to devlin, 2014-08-08 00:28:33]

rename'd

[Fady Samuel, 2014-08-08 14:55:19]

lgtm with nits + question

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/app_view/app_view_guest.cc (right):

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/app_view/app_view_guest.cc:139: return
appview::kEmbedderApiNamespace;
nit: rename constant to kEmbedderAPINamespace for consistency?

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:40:
return extensionoptions::kApiNamespace;
nit: rename constant to kEmbedderAPINamespace for consistency?

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.cc:131:
process_map->GuessContextType(embedder_extension,
Why is this called *Guess*ContextType. Is it possible to get this wrong? Is this
a security issue?

[not at google - send to devlin, 2014-08-08 15:25:25]

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/app_view/app_view_guest.cc (right):

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/app_view/app_view_guest.cc:139: return
appview::kEmbedderApiNamespace;
On 2014/08/08 14:55:18, Fady Samuel wrote:
> nit: rename constant to kEmbedderAPINamespace for consistency?

Done.

today, Api looks better than API. maybe it's an afternoon thing.

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/extension_options/extension_options_guest.cc
(right):

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/extension_options/extension_options_guest.cc:40:
return extensionoptions::kApiNamespace;
On 2014/08/08 14:55:18, Fady Samuel wrote:
> nit: rename constant to kEmbedderAPINamespace for consistency?

Done.

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://codereview.chromium.org/426593007/diff/140001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.cc:131:
process_map->GuessContextType(embedder_extension,
On 2014/08/08 14:55:19, Fady Samuel wrote:
> Why is this called *Guess*ContextType. Is it possible to get this wrong? Is
this
> a security issue?

It's not really possible to get this wrong. I called it "Guess" because really
it's not *actually* a context type, that's a JS concept, it's more... privilege
level.  Perhaps all of this stuff should be phrased that way since it's not
really renderer-specific.

Perhaps "guess" wasn't the right term after all. GetMostLikelyContextType?
GetContextTypeForProcessType? GetContextType? MapToContextType?
MapProcessToContextType? I don't really mind renaming it.

... alright, I renamed it, and made the comment much better, hopefully it makes
more sense now.

[not at google - send to devlin, 2014-08-08 15:25:31]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-08-08 15:26:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kalman@chromium.org/426593007/160001

[not at google - send to devlin, 2014-08-08 15:34:48]

The CQ bit was checked by kalman@chromium.org

[commit-bot: I haz the power, 2014-08-08 15:35:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kalman@chromium.org/426593007/180001

[commit-bot: I haz the power, 2014-08-08 19:01:54]

Change committed as 288407