Re-implement the screensaver to use WebView instead of ExtensionDialogHost.

Re-implement the screensaver to use WebView instead of ExtensionDialogHost.
Use WebView to render the screensaver extension instead of ExtensionDialogHost. Using the RenderViewGone override to detect termination of the renderer process to restart it.
Added browser tests.

R=ben@chromium.org,sky@chromium.org
BUG=chromium-os:28211
TEST=Tested that the screensaver comes up; tested the reload via crashing the extension renderer with SIG_ABRT; also ran browser tests.

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

[Ben Goodger (Google), 2012-04-23 21:16:08]

So... can you implement this in src/ash... and delegate the bits that need code
in src/chrome via some delegate interface? (see the pattern used for
ShellDelegate, SysTrayDelegate etc)?

[Ben Goodger (Google), 2012-04-23 21:16:50]

On 2012/04/23 21:16:08, Ben Goodger (Google) wrote:
> So... can you implement this in src/ash... and delegate the bits that need
code
> in src/chrome via some delegate interface? (see the pattern used for
> ShellDelegate, SysTrayDelegate etc)?

Once you've done this, don't forget to add a button to WindowTypeLauncher in ash
that shows the screen saver on command. The default implementation of the screen
saver in ash could just show a blank or otherwise colored view.

[rkc, 2012-04-28 04:11:40]

Added jam@ for content/* review.

Moved the screensaver implementation to ash.
Added unit tests to ash, making changes to content/test and webview to allow
testing a WebContents container via aura_shell_unittests

I haven't added the screensaver button to the WindowTypeLauncher yet since I am
running into some issues. I'll discuss them with ben@ and then check-in the
changes to WindowTypeLauncher in a follow on CL.

On 2012/04/23 21:16:50, Ben Goodger (Google) wrote:
> On 2012/04/23 21:16:08, Ben Goodger (Google) wrote:
> > So... can you implement this in src/ash... and delegate the bits that need
> code
> > in src/chrome via some delegate interface? (see the pattern used for
> > ShellDelegate, SysTrayDelegate etc)?
> 
> Once you've done this, don't forget to add a button to WindowTypeLauncher in
ash
> that shows the screen saver on command. The default implementation of the
screen
> saver in ash could just show a blank or otherwise colored view.

[Ben Goodger (Google), 2012-05-01 16:57:42]

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.cc (right):

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
ash/screensaver/screensaver_view.cc:101: screensaver_webview_ =
webview_factory_.Run(context);
See my note about how to do this without a factory.

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.h (right):

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
ash/screensaver/screensaver_view.h:54: ScreensaverView(const GURL& url);
explicit

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
File ash/screensaver/screensaver_view_unittest.cc (right):

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
ash/screensaver/screensaver_view_unittest.cc:19: class ScreensaverViewTest :
public ash::test::AshTestBase {
can you create a helper class in views test support called
views::WebViewTestHelper. It'd create/own the ui thread, rph factory, rvh
factory. This test class would then just have one as a member.

http://codereview.chromium.org/10191010/diff/10008/ui/views/controls/webview/...
File ui/views/controls/webview/webview.h (right):

http://codereview.chromium.org/10191010/diff/10008/ui/views/controls/webview/...
ui/views/controls/webview/webview.h:79: virtual content::WebContents*
CreateWebContents(
So, I'm generally pretty leery of subclassing of concrete base classes like
this. I think it's overused in Chrome.

How about this for an alternate. Add a method to src/ui/views/views_delegate.h
that returns true if the app is running in a unit test, and then have the
WebView implicitly construct a test webcontents in that case.

[Ben Goodger (Google), 2012-05-01 16:57:53]

jam: ping re content stuff.

[jam, 2012-05-01 18:55:33]

content lgtm

http://codereview.chromium.org/10191010/diff/10008/content/test/test_content_...
File content/test/test_content_client_initializer.h (right):

http://codereview.chromium.org/10191010/diff/10008/content/test/test_content_...
content/test/test_content_client_initializer.h:14: namespace content {
put this whole class in the content namespace

http://codereview.chromium.org/10191010/diff/10008/content/test/test_content_...
content/test/test_content_client_initializer.h:19: class
TestContentClientInitializer {
nit: please add a comment about what this class is used for

http://codereview.chromium.org/10191010/diff/10008/content/test/test_content_...
content/test/test_content_client_initializer.h:23: private:
nit: blank line above private

[rkc, 2012-05-02 00:51:03]

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.cc (right):

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
ash/screensaver/screensaver_view.cc:101: screensaver_webview_ =
webview_factory_.Run(context);
On 2012/05/01 16:57:42, Ben Goodger (Google) wrote:
> See my note about how to do this without a factory.

Done.

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.h (right):

http://codereview.chromium.org/10191010/diff/10008/ash/screensaver/screensave...
ash/screensaver/screensaver_view.h:54: ScreensaverView(const GURL& url);
On 2012/05/01 16:57:42, Ben Goodger (Google) wrote:
> explicit

Done.

http://codereview.chromium.org/10191010/diff/10008/content/test/test_content_...
File content/test/test_content_client_initializer.h (right):

http://codereview.chromium.org/10191010/diff/10008/content/test/test_content_...
content/test/test_content_client_initializer.h:14: namespace content {
On 2012/05/01 18:55:33, John Abd-El-Malek wrote:
> put this whole class in the content namespace

Done.

http://codereview.chromium.org/10191010/diff/10008/content/test/test_content_...
content/test/test_content_client_initializer.h:19: class
TestContentClientInitializer {
On 2012/05/01 18:55:33, John Abd-El-Malek wrote:
> nit: please add a comment about what this class is used for

Done.

http://codereview.chromium.org/10191010/diff/10008/content/test/test_content_...
content/test/test_content_client_initializer.h:23: private:
On 2012/05/01 18:55:33, John Abd-El-Malek wrote:
> nit: blank line above private

Done.

http://codereview.chromium.org/10191010/diff/10008/ui/views/controls/webview/...
File ui/views/controls/webview/webview.h (right):

http://codereview.chromium.org/10191010/diff/10008/ui/views/controls/webview/...
ui/views/controls/webview/webview.h:79: virtual content::WebContents*
CreateWebContents(
On 2012/05/01 16:57:42, Ben Goodger (Google) wrote:
> So, I'm generally pretty leery of subclassing of concrete base classes like
> this. I think it's overused in Chrome.
> 
> How about this for an alternate. Add a method to src/ui/views/views_delegate.h
> that returns true if the app is running in a unit test, and then have the
> WebView implicitly construct a test webcontents in that case.

Done.

http://codereview.chromium.org/10191010/diff/10008/ui/views/controls/webview/...
ui/views/controls/webview/webview.h:79: virtual content::WebContents*
CreateWebContents(
On 2012/05/01 16:57:42, Ben Goodger (Google) wrote:
> So, I'm generally pretty leery of subclassing of concrete base classes like
> this. I think it's overused in Chrome.
> 
> How about this for an alternate. Add a method to src/ui/views/views_delegate.h
> that returns true if the app is running in a unit test, and then have the
> WebView implicitly construct a test webcontents in that case.

Done.

[Ben Goodger (Google), 2012-05-02 01:19:16]

http://codereview.chromium.org/10191010/diff/28002/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.cc (right):

http://codereview.chromium.org/10191010/diff/28002/ash/screensaver/screensave...
ash/screensaver/screensaver_view.cc:140: views::WebView*
ScreensaverView::CreateWebView(
do you still need this method?

http://codereview.chromium.org/10191010/diff/28002/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.h (right):

http://codereview.chromium.org/10191010/diff/28002/ash/screensaver/screensave...
ash/screensaver/screensaver_view.h:54: explicit ScreensaverView(const GURL&
url);
ctor/dtors should be after friend class...

http://codereview.chromium.org/10191010/diff/28002/ui/views/controls/webview/...
File ui/views/controls/webview/webview.cc (right):

http://codereview.chromium.org/10191010/diff/28002/ui/views/controls/webview/...
ui/views/controls/webview/webview.cc:20: #include
"ui/views/test/webview_test_helper.h"
don't think you need this

http://codereview.chromium.org/10191010/diff/30002/ui/views/test/test_views_d...
File ui/views/test/test_views_delegate.cc (right):

http://codereview.chromium.org/10191010/diff/30002/ui/views/test/test_views_d...
ui/views/test/test_views_delegate.cc:8: #include
"content/test/web_contents_tester.h"
I think you're going to have to add a DEPS file in this test/ dir to allow this
include. Did you run checkdeps locally?

[rkc, 2012-05-02 01:34:45]

http://codereview.chromium.org/10191010/diff/28002/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.cc (right):

http://codereview.chromium.org/10191010/diff/28002/ash/screensaver/screensave...
ash/screensaver/screensaver_view.cc:140: views::WebView*
ScreensaverView::CreateWebView(
On 2012/05/02 01:19:17, Ben Goodger (Google) wrote:
> do you still need this method?

Removed.
Done.

http://codereview.chromium.org/10191010/diff/28002/ash/screensaver/screensave...
File ash/screensaver/screensaver_view.h (right):

http://codereview.chromium.org/10191010/diff/28002/ash/screensaver/screensave...
ash/screensaver/screensaver_view.h:54: explicit ScreensaverView(const GURL&
url);
On 2012/05/02 01:19:17, Ben Goodger (Google) wrote:
> ctor/dtors should be after friend class...

Done.

http://codereview.chromium.org/10191010/diff/28002/ui/views/controls/webview/...
File ui/views/controls/webview/webview.cc (right):

http://codereview.chromium.org/10191010/diff/28002/ui/views/controls/webview/...
ui/views/controls/webview/webview.cc:20: #include
"ui/views/test/webview_test_helper.h"
On 2012/05/02 01:19:17, Ben Goodger (Google) wrote:
> don't think you need this

Removed.
Done.

http://codereview.chromium.org/10191010/diff/30002/ui/views/test/test_views_d...
File ui/views/test/test_views_delegate.cc (right):

http://codereview.chromium.org/10191010/diff/30002/ui/views/test/test_views_d...
ui/views/test/test_views_delegate.cc:8: #include
"content/test/web_contents_tester.h"
On 2012/05/02 01:19:17, Ben Goodger (Google) wrote:
> I think you're going to have to add a DEPS file in this test/ dir to allow
this
> include. Did you run checkdeps locally?

Ran checkdeps and fixed/added all the DEPS files needed.
Done.

[Ben Goodger (Google), 2012-05-02 01:46:52]

LGTM - thanks again for doing this!

[commit-bot: I haz the power, 2012-05-02 02:06:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/10191010/34002

[commit-bot: I haz the power, 2012-05-02 09:12:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rkc@chromium.org/10191010/50003

[commit-bot: I haz the power, 2012-05-02 10:42:50]

Change committed as 134889

[caseq, 2012-05-02 12:06:12]

On 2012/05/02 10:42:50, I haz the power (commit-bot) wrote:
> Change committed as 134889

Reverted as of r134893 due to build failure on a number of bots, e.g:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...
http://build.chromium.org/p/chromium/builders/Win%20Aura/builds/11591/steps/c...

[jam, 2012-05-30 18:19:09]

it looks like this broke view_examples when using the WebView, because
WebView::CreateWebContents() ends up creating a test web contents instead of a
real one. i.e. if I step over the "if (ViewsDelegate::views_delegate) {" line,
and just call "content::WebContents::Create", then it works.

[rkc, 2012-05-30 21:05:38]

Thanks for catching this.
Fix out for review at http://codereview.chromium.org/10447096/

On 2012/05/30 18:19:09, John Abd-El-Malek wrote:
> it looks like this broke view_examples when using the WebView, because
> WebView::CreateWebContents() ends up creating a test web contents instead of a
> real one. i.e. if I step over the "if (ViewsDelegate::views_delegate) {" line,
> and just call "content::WebContents::Create", then it works.