Experimental app list: Custom launcher pages have app APIs and style.

Experimental app list: Custom launcher pages have app APIs and style.

Added CustomLauncherPageContents, which is copied from AppWindowContents
and modified to provide launcher pages instead. This correctly
dispatches messages to the extensions system.

The extension inside the launcher page is now "activated" which ensures
that it is styled as a platform app and given the correct APIs by the
extension dispatcher.

BUG=391137
BUG=391202
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282530

[Matt Giuca, 2014-07-03 08:25:57]

Hey, this CL is kind of two separate changes (the two files), but I think it
makes sense to do them in one CL because the DCHECK in dispatcher.cc guards
against the problem I am fixing in app_list_view_delegate.cc.

benwells: app_list_view_delegate.cc.
kalman: dispatcher.cc.

The DCHECK is good -- I *think* -- but I would like you (kalman) to think over
it because I don't understand this stuff well (though I talked with sammc about
it). I initially had this DCHECK for all extensions, but it crashed when an
extension tried to use a content script. Sam also told me that if you run an
extension in an iframe, it would not be activated.

It seems like for extensions, adding a script context without activating it is
by design -- it means you are running the extension in a restricted context.
Whereas for platform apps, it is always a bug, because the app won't have access
to its own APIs. So it seems logical to perform this DCHECK if and only if the
extension is actually a platform app. WDYT?

[Matt Giuca, 2014-07-04 01:48:23]

Oops, just realised that the APIs don't actually work (they must not be hooked
up correctly), but they do exist in the JavaScript context, so I think this
patch is still worth submitting.

Also realised that kalman is probably on holidays, so I might split the
Dispatcher change out into a separate (lower priority) CL if I get an LGTM from
benwells.

[benwells, 2014-07-04 04:06:39]

As discussed:
a) I don't know if the dispatcher change is good or not. I understand the logic
but i) there might be weird cases it breaks and ii) it might limit us in future.
I trust kalman's judgement
b) for the other change I'd rather commit it once we have everything working.

[Matt Giuca, 2014-07-04 04:16:22]

OK I have split the Dispatcher changes into
https://codereview.chromium.org/371563005/ and removed kalman as a reviewer for
this CL.

I will wait until I have the APIs working before re-sending this for review.

[not at google - send to devlin, 2014-07-07 14:50:10]

https://codereview.chromium.org/365013003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/365013003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:169: // which allows
Dispatcher to give it the proper context.
creating a [Chrome]ExtensionWebContentsObserver does more than just set up the
context, it makes the entire extension infrastructure work.

[Matt Giuca, 2014-07-09 15:05:30]

Full implementation as requested by Ben (Wells).

As he is on vacation, I can get Trent to look at it (unless kalman has all
owners; on mobile and haven't checked).

https://codereview.chromium.org/365013003/diff/20001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/365013003/diff/20001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:169: // which allows
Dispatcher to give it the proper context.
On 2014/07/07 14:50:10, kalman wrote:
> creating a [Chrome]ExtensionWebContentsObserver does more than just set up the
> context, it makes the entire extension infrastructure work.

Done.

[not at google - send to devlin, 2014-07-09 15:14:39]

lgtm

I am owner of apps/ but I don't have the product background. code looks fine.

https://codereview.chromium.org/365013003/diff/60001/apps/custom_launcher_pag...
File apps/custom_launcher_page_contents.h (right):

https://codereview.chromium.org/365013003/diff/60001/apps/custom_launcher_pag...
apps/custom_launcher_page_contents.h:28: explicit CustomLauncherPageContents();
don't need explicit

https://codereview.chromium.org/365013003/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/365013003/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:172:
extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
seems like it would be better for CustomLauncherPageContents to do this itself.

[Matt Giuca, 2014-07-10 03:45:17]

Hi Trent,

In benwells' absence, can you look at this CL? Thanks.

https://codereview.chromium.org/365013003/diff/60001/apps/custom_launcher_pag...
File apps/custom_launcher_page_contents.h (right):

https://codereview.chromium.org/365013003/diff/60001/apps/custom_launcher_pag...
apps/custom_launcher_page_contents.h:28: explicit CustomLauncherPageContents();
On 2014/07/09 15:14:39, kalman wrote:
> don't need explicit

Done.

https://codereview.chromium.org/365013003/diff/60001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/365013003/diff/60001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_view_delegate.cc:172:
extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
Yeah I agree. I didn't do it for consistency with AppWindowContents (which does
this part on the outside) but I think it's best to encapsulate it so I will
change.

[tapted, 2014-07-10 04:29:34]

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
File apps/custom_launcher_page_contents.cc (right):

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
apps/custom_launcher_page_contents.cc:35:
content::WebContentsObserver::Observe(web_contents_.get());
nit: omit content::WebContentsObserver:: ?

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
File apps/custom_launcher_page_contents.h (right):

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
apps/custom_launcher_page_contents.h:12: class GURL;
this is a data member, so should probably have an #include. Alternatively, I
think you can remove the data member as well as LoadContents(), and just do the
LoadURL step at the end of Initialize (unless you have other plans for these)

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
apps/custom_launcher_page_contents.h:37: content::WebContents* GetWebContents()
const;
nit: use a hacker_style inline accessor? Or call GetAssociatedWebContents()
instead? (I'd probably go hacker_style, and prefer web_contents() over
web_contents_.get() in the .cc)

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
apps/custom_launcher_page_contents.h:40: // content::WebContentsObserver
nit: I usualy see a colon after these

https://codereview.chromium.org/365013003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/365013003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:167:
custom_page_contents_->Initialize(profile, custom_launcher_page_url);
Should this be in OnProfileChanged() to properly handle profile switching, and
deleting a profile from chrome://settings?

Alternatively, I think you could you just move this into
AppListViewDelegate::CreateCustomPageWebView, and reset it when that's called
via AppListMainView->ModelChanged() which will create a new ContentsView

https://codereview.chromium.org/365013003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:486: new
views::WebView(web_contents->GetBrowserContext());
is web_contents->GetBrowserContext() equivalent to this->profile_? You could use
that and 
  views::WebView* web_view = new views::WebView(profile_);
  web_view->SetWebContents(custom_page_contents_->web_contents());

[Matt Giuca, 2014-07-10 07:37:03]

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
File apps/custom_launcher_page_contents.cc (right):

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
apps/custom_launcher_page_contents.cc:35:
content::WebContentsObserver::Observe(web_contents_.get());
On 2014/07/10 04:29:34, tapted wrote:
> nit: omit content::WebContentsObserver:: ? 

Done.

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
File apps/custom_launcher_page_contents.h (right):

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
apps/custom_launcher_page_contents.h:12: class GURL;
On 2014/07/10 04:29:34, tapted wrote:
> this is a data member, so should probably have an #include. Alternatively, I
> think you can remove the data member as well as LoadContents(), and just do
the
> LoadURL step at the end of Initialize (unless you have other plans for these)

Done (latter).

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
apps/custom_launcher_page_contents.h:37: content::WebContents* GetWebContents()
const;
On 2014/07/10 04:29:34, tapted wrote:
> nit: use a hacker_style inline accessor? Or call GetAssociatedWebContents()
> instead? (I'd probably go hacker_style, and prefer web_contents() over
> web_contents_.get() in the .cc)

Done.

https://codereview.chromium.org/365013003/diff/100001/apps/custom_launcher_pa...
apps/custom_launcher_page_contents.h:40: // content::WebContentsObserver
On 2014/07/10 04:29:34, tapted wrote:
> nit: I usualy see a colon after these

Done.

https://codereview.chromium.org/365013003/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/365013003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:167:
custom_page_contents_->Initialize(profile, custom_launcher_page_url);
Good catch! This is actually a pre-existing bug. So I have filed it and will
address it in a follow-up:
https://code.google.com/p/chromium/issues/detail?id=392763

https://codereview.chromium.org/365013003/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:486: new
views::WebView(web_contents->GetBrowserContext());
Hmm, yeah they SHOULD be equivalent (but actually is not, due to the
aforementioned bug, Issue 392763).

I think the right thing to do is still to use GetBrowserContext() since you are
creating a WebView for a WebContents and the main source of truth about which
profile is the GetBrowserContext().

But it is good to add a DCHECK to ensure these are the same. I've done that in
Chris's code just above (which I copied this from). However, I can't add the
DCHECK to my code right now; added a TODO.

[tapted, 2014-07-10 09:26:52]

lgtm

https://codereview.chromium.org/365013003/diff/140001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/365013003/diff/140001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:471:
DCHECK_EQ(this->profile_, web_contents->GetBrowserContext());
nit: no `this->` (sorry - probably my bad since I used that earlier to clarify
which profile variable I was talking about :p)

[Matt Giuca, 2014-07-11 00:16:12]

https://codereview.chromium.org/365013003/diff/140001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_view_delegate.cc (right):

https://codereview.chromium.org/365013003/diff/140001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_view_delegate.cc:471:
DCHECK_EQ(this->profile_, web_contents->GetBrowserContext());
On 2014/07/10 09:26:52, tapted wrote:
> nit: no `this->` (sorry - probably my bad since I used that earlier to clarify
> which profile variable I was talking about :p)

Done.

[Matt Giuca, 2014-07-11 00:23:37]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-07-11 00:25:52]

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

[commit-bot: I haz the power, 2014-07-11 03:49:14]

Change committed as 282530