Implement <appview>

Implement <appview>

This is the first working CL of <appview>. This CL implements the basic permission API with the onAppEmbeddingRequest event.

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

[Fady Samuel, 2014-07-02 22:46:18]

Please take a look.

[lazyboy, 2014-07-07 21:34:45]

Still need to review app_view_guest.cc and app_runtime*.

Will do that in next pass, sending some comment.s

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/browser/ap...
File chrome/browser/apps/web_view_browsertest.cc (left):

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/browser/ap...
chrome/browser/apps/web_view_browsertest.cc:680: 
Revert this change?

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/browser/ex...
File chrome/browser/extensions/api/guest_view/app_view_internal_api.h (right):

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/browser/ex...
chrome/browser/extensions/api/guest_view/app_view_internal_api.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
this file should go under
c/b/extensions/api/app_view/...

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_base.cc (right):

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_base.cc:363: delete this;
Can you call callback.Run(this) after "delete this"?

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/browser/gu...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/browser/gu...
chrome/browser/guest_view/guest_view_manager.h:94: content::WebContents*
GetGuestByInstanceID(
nit: can fit in one line.

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/common/ext...
File chrome/common/extensions/api/app_view_internal.json (right):

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/common/ext...
chrome/common/extensions/api/app_view_internal.json:15: "type": "function",
Please add a description of what this function does.

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/common/ext...
chrome/common/extensions/api/app_view_internal.json:42: "type": "function",
Same as above, add description of this function.

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/renderer/r...
File chrome/renderer/resources/extensions/app_view.js (right):

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/renderer/r...
chrome/renderer/resources/extensions/app_view.js:17: this.errorNode =
document.createElement('div');
Can we lazily create this error node when we actually need it?

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/renderer/r...
chrome/renderer/resources/extensions/app_view.js:71: return
this.browserPluginNode['-internal-attach'](instanceId, params);
You need to add the code for display:none handling from web_view.js

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/renderer/r...
chrome/renderer/resources/extensions/app_view.js:81: this.style.position =
'absolute';
I'm not totally sure why you'd need this?
position absolute with relative width/height 100% feels ....

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/test/data/...
File chrome/test/data/extensions/platform_apps/app_view/shim/main.js (right):

https://chromiumcodereview.appspot.com/354483004/diff/80001/chrome/test/data/...
chrome/test/data/extensions/platform_apps/app_view/shim/main.js:104:
chrome.test.sendMessage("Launched");
Single quotes/'Launched'

https://chromiumcodereview.appspot.com/354483004/diff/80001/content/browser/b...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://chromiumcodereview.appspot.com/354483004/diff/80001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:172: // Once a
BrowserPluginGuest has an embedder WebContents, it's considered to
Was there any particular reason to move this earlier or just cleanup?
Curious to know.

https://chromiumcodereview.appspot.com/354483004/diff/80001/content/browser/b...
content/browser/browser_plugin/browser_plugin_guest.cc:320: return gfx::Point();
return relative_position;

[Fady Samuel, 2014-07-08 15:47:09]

PTAL

https://codereview.chromium.org/354483004/diff/80001/chrome/browser/apps/web_...
File chrome/browser/apps/web_view_browsertest.cc (left):

https://codereview.chromium.org/354483004/diff/80001/chrome/browser/apps/web_...
chrome/browser/apps/web_view_browsertest.cc:680: 
On 2014/07/07 21:34:44, lazyboy wrote:
> Revert this change?

Done.

https://codereview.chromium.org/354483004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/guest_view/app_view_internal_api.h (right):

https://codereview.chromium.org/354483004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/guest_view/app_view_internal_api.h:1: // Copyright
2014 The Chromium Authors. All rights reserved.
On 2014/07/07 21:34:44, lazyboy wrote:
> this file should go under
> c/b/extensions/api/app_view/...

Done.

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

https://codereview.chromium.org/354483004/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_base.cc:363: delete this;
On 2014/07/07 21:34:44, lazyboy wrote:
> Can you call callback.Run(this) after "delete this"?

Yes, this callback is a local variable. We can't access members.

https://codereview.chromium.org/354483004/diff/80001/chrome/browser/guest_vie...
File chrome/browser/guest_view/guest_view_manager.h (right):

https://codereview.chromium.org/354483004/diff/80001/chrome/browser/guest_vie...
chrome/browser/guest_view/guest_view_manager.h:94: content::WebContents*
GetGuestByInstanceID(
On 2014/07/07 21:34:44, lazyboy wrote:
> nit: can fit in one line.

Done.

https://codereview.chromium.org/354483004/diff/80001/chrome/common/extensions...
File chrome/common/extensions/api/app_view_internal.json (right):

https://codereview.chromium.org/354483004/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/app_view_internal.json:15: "type": "function",
On 2014/07/07 21:34:44, lazyboy wrote:
> Please add a description of what this function does.

Done.

https://codereview.chromium.org/354483004/diff/80001/chrome/common/extensions...
chrome/common/extensions/api/app_view_internal.json:42: "type": "function",
On 2014/07/07 21:34:44, lazyboy wrote:
> Same as above, add description of this function.

Done.

https://codereview.chromium.org/354483004/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/app_view.js (right):

https://codereview.chromium.org/354483004/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/app_view.js:17: this.errorNode =
document.createElement('div');
On 2014/07/07 21:34:44, lazyboy wrote:
> Can we lazily create this error node when we actually need it?

Done.

https://codereview.chromium.org/354483004/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/app_view.js:71: return
this.browserPluginNode['-internal-attach'](instanceId, params);
On 2014/07/07 21:34:44, lazyboy wrote:
> You need to add the code for display:none handling from web_view.js

This is not currently necessary because <appview> doesn't currently support any
attributes. It will not renavigate if detached and reattached.

https://codereview.chromium.org/354483004/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/app_view.js:81: this.style.position =
'absolute';
On 2014/07/07 21:34:44, lazyboy wrote:
> I'm not totally sure why you'd need this?
> position absolute with relative width/height 100% feels ....

So that the errorNode can overlap the BrowserPlugin. Is there a better way to do
this?

https://codereview.chromium.org/354483004/diff/80001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/app_view/shim/main.js (right):

https://codereview.chromium.org/354483004/diff/80001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/app_view/shim/main.js:104:
chrome.test.sendMessage("Launched");
On 2014/07/07 21:34:44, lazyboy wrote:
> Single quotes/'Launched'

Done.

https://codereview.chromium.org/354483004/diff/80001/content/browser/browser_...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/354483004/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:172: // Once a
BrowserPluginGuest has an embedder WebContents, it's considered to
On 2014/07/07 21:34:45, lazyboy wrote:
> Was there any particular reason to move this earlier or just cleanup?
> Curious to know.

Done.

https://codereview.chromium.org/354483004/diff/80001/content/browser/browser_...
content/browser/browser_plugin/browser_plugin_guest.cc:320: return gfx::Point();
On 2014/07/07 21:34:45, lazyboy wrote:
> return relative_position;

Done.

[lazyboy, 2014-07-08 18:24:34]

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/browser/e...
File chrome/browser/extensions/api/app_view/app_view_internal_api.cc (right):

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/browser/e...
chrome/browser/extensions/api/app_view/app_view_internal_api.cc:45: if
(!AppViewGuest::CompletePendingRequest(GURL(),
Can we also add a note that: since GURL() is invalid, so this will result in not
creating the WebContents/appview.

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/browser/e...
File chrome/browser/extensions/api/guest_view/guest_view_internal_api.h (right):

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/browser/e...
chrome/browser/extensions/api/guest_view/guest_view_internal_api.h:10: namespace
content {
Are changes in this file necessary?

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/browser/g...
File chrome/browser/guest_view/app_view/app_view_guest.cc (right):

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/browser/g...
chrome/browser/guest_view/app_view/app_view_guest.cc:67: // An app is sending
invalid responses. We should probably kill it.
Why "probably" kill?
If we're not sure about killing the process, then this should be a TODO instead?

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/renderer/...
File chrome/renderer/resources/extensions/app_view.js (right):

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/renderer/...
chrome/renderer/resources/extensions/app_view.js:15: this.shadowRoot =
this.appviewNode.createShadowRoot();
It isn't necessary to store the reference, this can be retrieved later by
|this.appViewNode.shadowRoot|

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/renderer/...
chrome/renderer/resources/extensions/app_view.js:85: this.style.position =
'absolute';
Both the original plugin node and error node shouldn't require
position absolute.

Can we keep this as is and make the error node 
absolute, positioned at (0, 0), 100% X 100%, that
should work.

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/test/data...
File chrome/test/data/extensions/platform_apps/app_view/shim/skeleton/main.js
(right):

https://chromiumcodereview.appspot.com/354483004/diff/100001/chrome/test/data...
chrome/test/data/extensions/platform_apps/app_view/shim/skeleton/main.js:6:
request.allow('main.html');
what happens if we call request.deny() right after this line, etc?

https://chromiumcodereview.appspot.com/354483004/diff/100001/content/renderer...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://chromiumcodereview.appspot.com/354483004/diff/100001/content/renderer...
content/renderer/browser_plugin/browser_plugin.cc:282: render_view_routing_id_,
guest_instance_id_));
DCHECK_NE(guest_instance_id_, guest_instance_id)?

[Fady Samuel, 2014-07-08 20:23:46]

https://codereview.chromium.org/354483004/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/app_view/app_view_internal_api.cc (right):

https://codereview.chromium.org/354483004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/app_view/app_view_internal_api.cc:45: if
(!AppViewGuest::CompletePendingRequest(GURL(),
On 2014/07/08 18:24:33, lazyboy wrote:
> Can we also add a note that: since GURL() is invalid, so this will result in
not
> creating the WebContents/appview.

Done.

https://codereview.chromium.org/354483004/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/guest_view/guest_view_internal_api.h (right):

https://codereview.chromium.org/354483004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/guest_view/guest_view_internal_api.h:10: namespace
content {
On 2014/07/08 18:24:33, lazyboy wrote:
> Are changes in this file necessary?

No. Reverted.

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

https://codereview.chromium.org/354483004/diff/100001/chrome/browser/guest_vi...
chrome/browser/guest_view/app_view/app_view_guest.cc:67: // An app is sending
invalid responses. We should probably kill it.
On 2014/07/08 18:24:33, lazyboy wrote:
> Why "probably" kill?
> If we're not sure about killing the process, then this should be a TODO
instead?

Done.

https://codereview.chromium.org/354483004/diff/100001/chrome/renderer/resourc...
File chrome/renderer/resources/extensions/app_view.js (right):

https://codereview.chromium.org/354483004/diff/100001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/app_view.js:15: this.shadowRoot =
this.appviewNode.createShadowRoot();
On 2014/07/08 18:24:34, lazyboy wrote:
> It isn't necessary to store the reference, this can be retrieved later by
> |this.appViewNode.shadowRoot|

Done.

https://codereview.chromium.org/354483004/diff/100001/chrome/renderer/resourc...
chrome/renderer/resources/extensions/app_view.js:85: this.style.position =
'absolute';
On 2014/07/08 18:24:33, lazyboy wrote:
> Both the original plugin node and error node shouldn't require
> position absolute.
> 
> Can we keep this as is and make the error node 
> absolute, positioned at (0, 0), 100% X 100%, that
> should work.

Done.

https://codereview.chromium.org/354483004/diff/100001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_view/shim/skeleton/main.js
(right):

https://codereview.chromium.org/354483004/diff/100001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_view/shim/skeleton/main.js:6:
request.allow('main.html');
On 2014/07/08 18:24:34, lazyboy wrote:
> what happens if we call request.deny() right after this line, etc?

Nothing. The first call works, the second is discarded, for now. In a future CL,
I'd like to enforce the same semantics as the <webview> permission API, but I'd
like to limit complexity in this patch. It's already pushing 1000 lines.

https://codereview.chromium.org/354483004/diff/100001/content/renderer/browse...
File content/renderer/browser_plugin/browser_plugin.cc (right):

https://codereview.chromium.org/354483004/diff/100001/content/renderer/browse...
content/renderer/browser_plugin/browser_plugin.cc:282: render_view_routing_id_,
guest_instance_id_));
On 2014/07/08 18:24:34, lazyboy wrote:
> DCHECK_NE(guest_instance_id_, guest_instance_id)?


I just added an early exit in this case. There's nothing particularly useful we
can do here. Reattaching to the same guest doesn't make snese.

[Fady Samuel, 2014-07-08 20:27:16]

+kalman@ for extensions changes.

[not at google - send to devlin, 2014-07-08 20:54:14]

https://codereview.chromium.org/354483004/diff/120001/chrome/common/extension...
File chrome/common/extensions/api/app_view_internal.json (right):

https://codereview.chromium.org/354483004/diff/120001/chrome/common/extension...
chrome/common/extensions/api/app_view_internal.json:21: "nodoc": true
you don't need "nodoc" anywhere, since it's implicit by it being an internal API
(or rather, because you're not adding a HTML file for it)

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:45: [inline_doc] dictionary
AppEmbeddingRequest {
inline_doc is automatic for types that are only used once

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:52: [nocompile] static void deny();
these all need comments

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:57: static void
onAppEmbeddingRequest(AppEmbeddingRequest request);
hum. this change needs to go through actual API review.

[Fady Samuel, 2014-07-08 21:53:11]

PTAL

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:45: [inline_doc] dictionary
AppEmbeddingRequest {
On 2014/07/08 20:54:14, kalman wrote:
> inline_doc is automatic for types that are only used once

Done.

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:52: [nocompile] static void deny();
On 2014/07/08 20:54:14, kalman wrote:
> these all need comments

Done.

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:57: static void
onAppEmbeddingRequest(AppEmbeddingRequest request);
On 2014/07/08 20:54:14, kalman wrote:
> hum. this change needs to go through actual API review.

I'll write up an API proposal in parallel. appview is behind a flag and all
these APIs cannot be accessed unless --enable-app-view is turned on. Let's land
this first, and then we can chat about it and iterate over API once everyone can
play around with it.

[lazyboy, 2014-07-08 21:54:28]

lgtm

[not at google - send to devlin, 2014-07-08 22:10:47]

sorry, more comments. I don't like doing that :(

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:57: static void
onAppEmbeddingRequest(AppEmbeddingRequest request);
On 2014/07/08 21:53:10, Fady Samuel wrote:
> On 2014/07/08 20:54:14, kalman wrote:
> > hum. this change needs to go through actual API review.
> 
> I'll write up an API proposal in parallel. appview is behind a flag and all
> these APIs cannot be accessed unless --enable-app-view is turned on. Let's
land
> this first, and then we can chat about it and iterate over API once everyone
can
> play around with it.

That's not quite true, this event can still be listened to even if there's not
really a way to fire it.

since this documentation will go live basically immediately please add a note
that it only makes sense to listen to this event from dev channel and with the
--enable-app-view flag.

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:50: DOMString embedderExtensionId;
"embedderId"? since this is chrome.app.runtime it feels a bit awkward using the
word "extension" in here.

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:53: // has specified a |url| within the
app package to embed.
phrase these comments from the perspective of the app that's being embedded. so,
"Allows <code>embedderId</code> to embed this app in an &lt;appview&gt; element.

|url| ^^^ I actually don't know what this means. why does the embedee pass up a
url?

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:56: // This indicates that the guest app
has denied the embedding request.
likewise

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:62: static void
onAppEmbeddingRequest(AppEmbeddingRequest request);
I think "onEmbedRequested" would be better:
- the "app" is a tautology (this is on chrome.app.runtime, and besides,
extensions can embed apps too right?)
- events are phrased like "launched" not "launch(ing)", "restarted" not
"restart(ing)", so "requested" not "request".
- "embedding" just sounds awkward to me, but, I don't feel strongly about that.
maybe "embed" sounds a different kind of awkward.

[Fady Samuel, 2014-07-09 15:15:49]

PTAL Ben.

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/354483004/diff/120001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:57: static void
onAppEmbeddingRequest(AppEmbeddingRequest request);
On 2014/07/08 22:10:46, kalman wrote:
> On 2014/07/08 21:53:10, Fady Samuel wrote:
> > On 2014/07/08 20:54:14, kalman wrote:
> > > hum. this change needs to go through actual API review.
> > 
> > I'll write up an API proposal in parallel. appview is behind a flag and all
> > these APIs cannot be accessed unless --enable-app-view is turned on. Let's
> land
> > this first, and then we can chat about it and iterate over API once everyone
> can
> > play around with it.
> 
> That's not quite true, this event can still be listened to even if there's not
> really a way to fire it.
> 
> since this documentation will go live basically immediately please add a note
> that it only makes sense to listen to this event from dev channel and with the
> --enable-app-view flag.

Done.

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:50: DOMString embedderExtensionId;
On 2014/07/08 22:10:46, kalman wrote:
> "embedderId"? since this is chrome.app.runtime it feels a bit awkward using
the
> word "extension" in here.

Done.

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:53: // has specified a |url| within the
app package to embed.
On 2014/07/08 22:10:46, kalman wrote:
> phrase these comments from the perspective of the app that's being embedded.
so,
> "Allows <code>embedderId</code> to embed this app in an &lt;appview&gt;
element.
> 
> |url| ^^^ I actually don't know what this means. why does the embedee pass up
a
> url?

This app decides what to embed in the embedder :-)

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:56: // This indicates that the guest app
has denied the embedding request.
On 2014/07/08 22:10:46, kalman wrote:
> likewise

Done.

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:62: static void
onAppEmbeddingRequest(AppEmbeddingRequest request);
On 2014/07/08 22:10:46, kalman wrote:
> I think "onEmbedRequested" would be better:
> - the "app" is a tautology (this is on chrome.app.runtime, and besides,
> extensions can embed apps too right?)
> - events are phrased like "launched" not "launch(ing)", "restarted" not
> "restart(ing)", so "requested" not "request".
> - "embedding" just sounds awkward to me, but, I don't feel strongly about
that.
> maybe "embed" sounds a different kind of awkward.

Done.

[Fady Samuel, 2014-07-09 15:17:44]

+isherman@ for extension_function_histogram_value

[not at google - send to devlin, 2014-07-09 15:18:11]

lgtm

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
File extensions/common/api/app_runtime.idl (right):

https://codereview.chromium.org/354483004/diff/140001/extensions/common/api/a...
extensions/common/api/app_runtime.idl:53: // has specified a |url| within the
app package to embed.
On 2014/07/09 15:15:48, Fady Samuel wrote:
> On 2014/07/08 22:10:46, kalman wrote:
> > phrase these comments from the perspective of the app that's being embedded.
> so,
> > "Allows <code>embedderId</code> to embed this app in an &lt;appview&gt;
> element.
> > 
> > |url| ^^^ I actually don't know what this means. why does the embedee pass
up
> a
> > url?
> 
> This app decides what to embed in the embedder :-)

ah right. I got my direction mixed up :)

[Ilya Sherman, 2014-07-09 19:31:44]

Please run the script to update histograms.xml

[Fady Samuel, 2014-07-09 20:43:47]

PTAL Ilya. I'm unable to run update_extension_functions.py. It terminates with
an error. I have updated histograms.xml manually.

[Fady Samuel, 2014-07-09 21:00:05]

PTAL IIlya. I finally got the script to work.

[Ilya Sherman, 2014-07-09 21:01:15]

histograms LGTM, thanks.

[Fady Samuel, 2014-07-09 21:01:38]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-07-09 21:03:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fsamuel@chromium.org/354483004/200001

[commit-bot: I haz the power, 2014-07-09 21:35:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26680)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31115)

[commit-bot: I haz the power, 2014-07-09 21:37:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-09 21:37:21]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26706)

[Fady Samuel, 2014-07-10 00:38:11]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-07-10 00:41:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fsamuel@chromium.org/354483004/200002

[commit-bot: I haz the power, 2014-07-10 03:56:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31282)

[commit-bot: I haz the power, 2014-07-10 04:14:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-10 04:14:41]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)

[Fady Samuel, 2014-07-10 19:03:06]

PTAL Ben. I've moved over the AppViewInternal API to the extensions component.
I'm using ExtensionBrowserClient to talk to Chrome from there.

[Fady Samuel, 2014-07-10 19:32:06]

For the extension component changes.

[not at google - send to devlin, 2014-07-10 19:44:17]

https://codereview.chromium.org/354483004/diff/230001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/354483004/diff/230001/extensions/browser/exte...
extensions/browser/extensions_browser_client.h:186: virtual bool
CompletePendingEmbedRequest(
kinda weird that there is a "complete" in this interface, but nothing about how
these are initiated. why does it need to be in this interface?

+yoz

[James Cook, 2014-07-10 19:58:07]

+rockot, not to add to the reviewers list, just FYI for a question.

https://codereview.chromium.org/354483004/diff/230001/extensions/browser/exte...
File extensions/browser/extensions_browser_client.h (right):

https://codereview.chromium.org/354483004/diff/230001/extensions/browser/exte...
extensions/browser/extensions_browser_client.h:186: virtual bool
CompletePendingEmbedRequest(
On 2014/07/10 19:44:16, kalman wrote:
> kinda weird that there is a "complete" in this interface, but nothing about
how
> these are initiated. why does it need to be in this interface?
> 
> +yoz

I think the primary problem here is that:
1. <appview> is a feature mostly implemented in src/chrome
2. However, it associates its onEmbedded event in app_runtime, which lives in
src/extensions
3. We don't currently have a very good way to express API features that require
code in both places.

Can more of the guest_view / app_view code move into src/extensions?

If not, I think this method should be split into AppViewInternalAttachFrame and
AppViewInternalDenyRequest and moved in ExtensionsApiClient.

rockot might also have an opinion, as he had to deal with this for some
AppWindow refactoring recently.

[Fady Samuel, 2014-07-10 20:01:08]

On 2014/07/10 19:58:07, James Cook wrote:
> +rockot, not to add to the reviewers list, just FYI for a question.
> 
>
https://codereview.chromium.org/354483004/diff/230001/extensions/browser/exte...
> File extensions/browser/extensions_browser_client.h (right):
> 
>
https://codereview.chromium.org/354483004/diff/230001/extensions/browser/exte...
> extensions/browser/extensions_browser_client.h:186: virtual bool
> CompletePendingEmbedRequest(
> On 2014/07/10 19:44:16, kalman wrote:
> > kinda weird that there is a "complete" in this interface, but nothing about
> how
> > these are initiated. why does it need to be in this interface?
> > 
> > +yoz
> 
> I think the primary problem here is that:
> 1. <appview> is a feature mostly implemented in src/chrome
> 2. However, it associates its onEmbedded event in app_runtime, which lives in
> src/extensions
> 3. We don't currently have a very good way to express API features that
require
> code in both places.
> 
> Can more of the guest_view / app_view code move into src/extensions?
> 
> If not, I think this method should be split into AppViewInternalAttachFrame
and
> AppViewInternalDenyRequest and moved in ExtensionsApiClient.
> 
> rockot might also have an opinion, as he had to deal with this for some
> AppWindow refactoring recently.

In the future we would like <webview> and <appview> to move into src/extensions
but that's not a small undertaking. If you or Ken don't have the cycles for it,
I have an engineer, hanxi@, who might be interested in helping out with that.

[Fady Samuel, 2014-07-10 20:24:15]

PTAL James. I've moved it over to ExtensionAPIClient.

[James Cook, 2014-07-11 03:16:01]

Looking good, and ExtensionsApiClient feels right, but I have a few nits.

https://codereview.chromium.org/354483004/diff/250001/chrome/browser/extensio...
File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right):

https://codereview.chromium.org/354483004/diff/250001/chrome/browser/extensio...
chrome/browser/extensions/api/chrome_extensions_api_client.cc:43: return
AppViewGuest::CompletePendingRequest(url,
I like how this just calls into a helper function, which keeps this file nice
and clean.

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

https://codereview.chromium.org/354483004/diff/250001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.cc:329: Profile* profile =
Profile::FromBrowserContext(browser_context_);
Not for this CL: This doesn't have to be a Profile, and just using
browser_context_ directly would get rid of the profile.h include and make this
easier to move out of src/chrome.

https://codereview.chromium.org/354483004/diff/250001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_view/shim/main.js (right):

https://codereview.chromium.org/354483004/diff/250001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_view/shim/main.js:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
nit: 2014

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:31:
extensions::ExtensionSystem* system =
extensions:: not needed

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:37:
extensions::EventRouter* event_router = system->event_router();
ditto

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:65: // static.
super nit: no "." after "static" for consistency with the rest of this file (and
most of chrome)

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.h (right):

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.h:12: #include
"base/values.h"
Forward declare base::DictionaryValue, you don't need this header

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
File extensions/browser/api/app_view/app_view_internal_api.cc (right):

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_view/app_view_internal_api.cc:27:
extensions::ExtensionsAPIClient* extensions_client =
extensions:: not needed

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_view/app_view_internal_api.cc:29: if
(!extensions_client->AppViewInternalAttachFrame(url,
You could just "return extensions_client->..." instead of the if()

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_view/app_view_internal_api.cc:48:
extensions::ExtensionsAPIClient* extensions_client =
ditto extensions::

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_view/app_view_internal_api.cc:52: if
(!extensions_client->AppViewInternalDenyRequest(params->guest_instance_id,
ditto if()

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
File extensions/browser/api/extensions_api_client.h (right):

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:12: #include "url/gurl.h"
Forward declare GURL please

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:52: // |guest_instance_id|.
Document the meaning of the returned boolean.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:54: const GURL& url,
super nit: For these sorts of Extension interfaces I've been making the
BrowserContext the first parameter, since it's often used immediately to route
the request.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:60: // |guest_instance_id|.
Ditto document meaning of return value

[Fady Samuel, 2014-07-11 15:28:56]

PTAL James. I've addressed your comments. I'm going to stick this in the CQ but
feel free to add more comments. I'll address them here if the patch hasn't yet
landed or in a subsequent patch, if it has.

https://codereview.chromium.org/354483004/diff/250001/chrome/browser/extensio...
File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right):

https://codereview.chromium.org/354483004/diff/250001/chrome/browser/extensio...
chrome/browser/extensions/api/chrome_extensions_api_client.cc:43: return
AppViewGuest::CompletePendingRequest(url,
On 2014/07/11 03:16:00, James Cook wrote:
> I like how this just calls into a helper function, which keeps this file nice
> and clean.

Acknowledged.

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

https://codereview.chromium.org/354483004/diff/250001/chrome/browser/guest_vi...
chrome/browser/guest_view/guest_view_base.cc:329: Profile* profile =
Profile::FromBrowserContext(browser_context_);
On 2014/07/11 03:16:00, James Cook wrote:
> Not for this CL: This doesn't have to be a Profile, and just using
> browser_context_ directly would get rid of the profile.h include and make this
> easier to move out of src/chrome.

Good catch. It's a two line change so I just went ahead and did it.

https://codereview.chromium.org/354483004/diff/250001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/app_view/shim/main.js (right):

https://codereview.chromium.org/354483004/diff/250001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/app_view/shim/main.js:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2014/07/11 03:16:00, James Cook wrote:
> nit: 2014

Oops, copy and paste error.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.cc (right):

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:31:
extensions::ExtensionSystem* system =
On 2014/07/11 03:16:01, James Cook wrote:
> extensions:: not needed

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:37:
extensions::EventRouter* event_router = system->event_router();
On 2014/07/11 03:16:00, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.cc:65: // static.
On 2014/07/11 03:16:01, James Cook wrote:
> super nit: no "." after "static" for consistency with the rest of this file
(and
> most of chrome)

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
File extensions/browser/api/app_runtime/app_runtime_api.h (right):

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_runtime/app_runtime_api.h:12: #include
"base/values.h"
On 2014/07/11 03:16:01, James Cook wrote:
> Forward declare base::DictionaryValue, you don't need this header

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
File extensions/browser/api/app_view/app_view_internal_api.cc (right):

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_view/app_view_internal_api.cc:27:
extensions::ExtensionsAPIClient* extensions_client =
On 2014/07/11 03:16:01, James Cook wrote:
> extensions:: not needed

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_view/app_view_internal_api.cc:29: if
(!extensions_client->AppViewInternalAttachFrame(url,
On 2014/07/11 03:16:01, James Cook wrote:
> You could just "return extensions_client->..." instead of the if()

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_view/app_view_internal_api.cc:48:
extensions::ExtensionsAPIClient* extensions_client =
On 2014/07/11 03:16:01, James Cook wrote:
> ditto extensions::

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/app_view/app_view_internal_api.cc:52: if
(!extensions_client->AppViewInternalDenyRequest(params->guest_instance_id,
On 2014/07/11 03:16:01, James Cook wrote:
> ditto if()

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
File extensions/browser/api/extensions_api_client.h (right):

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:12: #include "url/gurl.h"
On 2014/07/11 03:16:01, James Cook wrote:
> Forward declare GURL please

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:52: // |guest_instance_id|.
On 2014/07/11 03:16:01, James Cook wrote:
> Document the meaning of the returned boolean.

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:54: const GURL& url,
On 2014/07/11 03:16:01, James Cook wrote:
> super nit: For these sorts of Extension interfaces I've been making the
> BrowserContext the first parameter, since it's often used immediately to route
> the request.

Done.

https://codereview.chromium.org/354483004/diff/250001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:60: // |guest_instance_id|.
On 2014/07/11 03:16:01, James Cook wrote:
> Ditto document meaning of return value

Done.

[Fady Samuel, 2014-07-11 15:29:01]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-07-11 15:29:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fsamuel@chromium.org/354483004/270001

[Fady Samuel, 2014-07-11 15:46:00]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-07-11 15:46:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fsamuel@chromium.org/354483004/290001

[Fady Samuel, 2014-07-11 16:19:12]

The CQ bit was checked by fsamuel@chromium.org

[commit-bot: I haz the power, 2014-07-11 16:19:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fsamuel@chromium.org/354483004/310001

[commit-bot: I haz the power, 2014-07-11 19:27:29]

Change committed as 282664