Use RenderWidget(Host) for full screen

Use RenderWidget(Host) for full screen

Add RenderWidgetFullscreen(Host) subclasses, InitAsFullscreen method
to RenderWidgetHostView, and add two new ipc messages for creating
and showing full screen.

Add createFullscreenWindow to RenderView that creates RenderWidgetFullscreen
and sends message to browser which eventually creates RenderWidgetFullscreenHost.

The show method on RenderWidgetFullscreen sends message to parent
RenderViewHost in browser, which calls InitAsFullscreen on the view.

BUG=16735
TEST=RenderViewHostTest.CreateFullscreenWidget

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56690

[boliu, 2010-07-22 00:23:34]

Choosing subclassing RenderWidget/Host since full screen widget still behaves
like a popup widget. However, some functions such as move/resize are no longer
applicable.

Is adding to RenderWidgetHostView sufficient or should it be a subclass?

Passing WebPopupType for the creation of full screen widget, though it is not
used. Should a new type like WebFullscreenType be used, or maybe the parameter
should be dropped. Also RenderWidget::Init creates WebPopupMenu regardless of
the WebPopupType, which seems wrong.

The creation path adds methods to RenderViewHostDelegate::View, which touches
some subclasses. Only TagContentsView fully implements the new methods.

WebWidget/Client is the interface for rendering into a RenderWidget. Are they
suitable for plugins to use or should some equivalent classes be created in
chromium code?

Going forward with full screen video, will need create a subclass of WebWidget
for the media player for rendering.

So is this a good direction to go for doing full screen? This patch is not
commit quality yet so please give feed back on high level design only.

[brettw, 2010-07-22 22:35:46]

Overall the hookup looks good, it's nice you figured this out so quickly. I
presume you looked at adding a new type to WebPopupType and that doesn't make
sense? (I don't actually know whether it does or doesn't.)

http://codereview.chromium.org/3055009/diff/1/19
File chrome/browser/tab_contents/interstitial_page.cc (right):

http://codereview.chromium.org/3055009/diff/1/19#newcode582
chrome/browser/tab_contents/interstitial_page.cc:582: << "InterstitialPage does
not support showing full screen popups yet.";
I would remove "yet" from the two functions you added. Conceivably we would want
to show popups, but I don't think we'd ever implement this.

[boliu, 2010-07-22 22:52:23]

I changed the interface for initializing full screen where a WebWidget* is
passed in instead of created by RenderWidget (presumably based on the
WebPopupType). Maybe a method should sit in WebKit that instantiates a WebWidget
based on WebPopupType.

I'm hacking together a class that hooks up WebMediaPlayer with RenderWidget.
It's a sublcass of WebWidget. If it works, then it would probably mean another
WebPopupType.

I will get this CL cleaned up some time tomorrow for a real review.

[boliu, 2010-07-23 19:17:22]

Added a ipc messages to get the size of the render widget, since for a full
screen widget, the browser dictates the dimensions instead of following what
WebKit tells it to size to. Will probably add the method to WebWidgetClient.

The experiment with hooking up with WebMediaPlayer works, so it can be added as
another WebPopupType. So either the WebWidget can be passed to RenderWidget as
it is created (as in RenderWidgetFullscreen right now) or a method should be
created in WebKit to instantiate WebWidgets based on WebPopupType. I am leaning
on the second option.

Should RenderWidgetFullscreen override methods that no longer applies for full
screen, such as setWindowRect()?

If things look well, I will look into writing tests for full screen widget. Any
pointers?

http://codereview.chromium.org/3055009/diff/7001/8014
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (left):

http://codereview.chromium.org/3055009/diff/7001/8014#oldcode537
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:537: // TODO(brettw)
possibly enable out-of-process painting here as well
This comment is out of date, right? Deleted in InitAsPopupHelper

[scherkus (not reviewing), 2010-07-26 17:14:44]

http://codereview.chromium.org/3055009/diff/7001/8004
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/3055009/diff/7001/8004#newcode886
chrome/browser/renderer_host/render_view_host.cc:886: WebKit::WebPopupType
popup_type) {
indentation

http://codereview.chromium.org/3055009/diff/7001/8010
File chrome/browser/renderer_host/render_widget_helper.h (right):

http://codereview.chromium.org/3055009/diff/7001/8010#newcode184
chrome/browser/renderer_host/render_widget_helper.h:184: int opener_id, int
route_id, WebKit::WebPopupType popup_type);
is it possible to match the style of OnCreateWidgetOnUI?

http://codereview.chromium.org/3055009/diff/7001/8015
File chrome/browser/renderer_host/render_widget_host_view_gtk.h (right):

http://codereview.chromium.org/3055009/diff/7001/8015#newcode122
chrome/browser/renderer_host/render_widget_host_view_gtk.h:122: const gfx::Rect&
pos, // Ignored if is_fullscreen is true.
nit: two spaces between , and //

http://codereview.chromium.org/3055009/diff/7001/8021
File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right):

http://codereview.chromium.org/3055009/diff/7001/8021#newcode136
chrome/browser/tab_contents/render_view_host_delegate_helper.cc:136:
widget_view->set_popup_type(popup_type);
so this looks like the final resting place for popup_type

what sort of value can we expect to see here?  WebPopupTypeNone,
WebPopupTypeSelect, WebPopupTypeSuggestion, or something else?

basically I'm wondering if a FullscreenWidget is specific enough to do without
passing a popup_type entirely and just set it here to something manually

http://codereview.chromium.org/3055009/diff/7001/8029
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/3055009/diff/7001/8029#newcode332
chrome/renderer/render_view.h:332: virtual void
createFullscreenWindow(WebKit::WebWidget* webwidget);
can you describe how this will get used?  (no code comment, just notes in the
code review)

for a moment I thought we would be calling createPopupMenu() with a "Fullscreen"
WebPopupType but it appears my preconcieved notion was wrong :)

What exactly is the WebWidget being passed in here?

http://codereview.chromium.org/3055009/diff/7001/8031
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/3055009/diff/7001/8031#newcode95
chrome/renderer/render_widget.h:95: virtual WebKit::WebSize size();
for my own (and other reviewers!) understanding, can you describe how this gets
used?

no need for actual code comment, just type it up in this code review

http://codereview.chromium.org/3055009/diff/7001/8032
File chrome/renderer/render_widget_fullscreen.cc (right):

http://codereview.chromium.org/3055009/diff/7001/8032#newcode11
chrome/renderer/render_widget_fullscreen.cc:11: /*static*/
nit: // instead of /* */

http://codereview.chromium.org/3055009/diff/7001/8032#newcode15
chrome/renderer/render_widget_fullscreen.cc:15: DCHECK(opener_id !=
MSG_ROUTING_NONE);
DCHECK_NE

http://codereview.chromium.org/3055009/diff/7001/8032#newcode41
chrome/renderer/render_widget_fullscreen.cc:41: DCHECK(false);
NOTREACHED

http://codereview.chromium.org/3055009/diff/7001/8032#newcode47
chrome/renderer/render_widget_fullscreen.cc:47: DCHECK(routing_id_ !=
MSG_ROUTING_NONE);
DCHECK_NE

http://codereview.chromium.org/3055009/diff/7001/8032#newcode48
chrome/renderer/render_widget_fullscreen.cc:48: DCHECK(opener_id_ !=
MSG_ROUTING_NONE);
DCHECK_NE

http://codereview.chromium.org/3055009/diff/7001/8033
File chrome/renderer/render_widget_fullscreen.h (right):

http://codereview.chromium.org/3055009/diff/7001/8033#newcode1
chrome/renderer/render_widget_fullscreen.h:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
copyright year

http://codereview.chromium.org/3055009/diff/7001/8033#newcode16
chrome/renderer/render_widget_fullscreen.h:16: static RenderWidgetFullscreen*
Create(int32 opener_id,
I'd drop all args to the next line

[boliu, 2010-07-26 18:58:06]

On 2010/07/26 17:14:44, scherkus wrote:

> http://codereview.chromium.org/3055009/diff/7001/8021
> File chrome/browser/tab_contents/render_view_host_delegate_helper.cc (right):
> 
> http://codereview.chromium.org/3055009/diff/7001/8021#newcode136
> chrome/browser/tab_contents/render_view_host_delegate_helper.cc:136:
> widget_view->set_popup_type(popup_type);
> so this looks like the final resting place for popup_type
> 
> what sort of value can we expect to see here?  WebPopupTypeNone,
> WebPopupTypeSelect, WebPopupTypeSuggestion, or something else?
> 
> basically I'm wondering if a FullscreenWidget is specific enough to do without
> passing a popup_type entirely and just set it here to something manually
> 

Right now looking at adding something to the effect of
WebPopupTypeFullscreenVideo. WebPopupType is used for
varying behavior of RenderWidgetHostView, eg NeedsInputGrab
for WebPopupTypeSelect.

Maybe a generic WebPopupFullscreen is good enough, but it will
dictate some of the changes on the renderer side discussed below.

> http://codereview.chromium.org/3055009/diff/7001/8029
> File chrome/renderer/render_view.h (right):
> 
> http://codereview.chromium.org/3055009/diff/7001/8029#newcode332
> chrome/renderer/render_view.h:332: virtual void
> createFullscreenWindow(WebKit::WebWidget* webwidget);
> can you describe how this will get used?  (no code comment, just notes in the
> code review)
> 
> for a moment I thought we would be calling createPopupMenu() with a
"Fullscreen"
> WebPopupType but it appears my preconcieved notion was wrong :)
> 
> What exactly is the WebWidget being passed in here?
> 

This is a possible change to the way a popup WebWidget is
initialized.

Currently in RenderWidget::Init, a WebPopupMenu is created regardless
of the WebPopupType being passed in. This won't work if we start
adding other popup types as mentioned above.

One way is to add a static method in WebKit that takes in a WebPopupType
and creates the corresponding WebWidget. This won't work if we only use
a generic Fullscreen type.

The other way is to just pass the WebWidget in (implemented here).
It's probably a good idea also to pass the corresponding WebPopupType
(not implemented here), but it seems like a duplication.

> http://codereview.chromium.org/3055009/diff/7001/8031
> File chrome/renderer/render_widget.h (right):
> 
> http://codereview.chromium.org/3055009/diff/7001/8031#newcode95
> chrome/renderer/render_widget.h:95: virtual WebKit::WebSize size();
> for my own (and other reviewers!) understanding, can you describe how this
gets
> used?
> 
> no need for actual code comment, just type it up in this code review
> 

This is used in my hacked up version of full screen video player
during painting. The player needs to know the size of the full
screen window for invalidate when a new frame becomes available.

[scherkus (not reviewing), 2010-07-28 18:39:19]

cool

well this looks good enough to keep working on but I think brett should take a
look at it as a sanity check

darin might also be interested in the WebPopupType/Chromium API stuff

[brettw, 2010-07-28 18:57:04]

This looks OK from a high level. I would be interested in trying to
simplify some of the widget stuff you mentioned, but that doesn't need
to block current work.

Brett

[brettw, 2010-07-28 19:34:04]

Just to clarify, if we simplified the widget stuff you were talking
about, this would allow us to remove a lot of the plumbing since we
can key off of the widget type, right?

Brett

On Wed, Jul 28, 2010 at 11:56 AM, Brett Wilson <brettw@chromium.org> wrote:
> This looks OK from a high level. I would be interested in trying to
> simplify some of the widget stuff you mentioned, but that doesn't need
> to block current work.
>
> Brett
>

[boliu, 2010-07-28 19:41:53]

Right. So looking for a function, given a WebPopupType, creates the
corresponding subclass of WebWidget.

I implemented something like it in RenderWidget this morning, but my local repo
is having unrelated compilation problems. Also seems like html select drop down
menu is broken for me right now and I'm tracing code to see where it broke.

On 2010/07/28 19:34:04, brettw wrote:
> Just to clarify, if we simplified the widget stuff you were talking
> about, this would allow us to remove a lot of the plumbing since we
> can key off of the widget type, right?
> 
> Brett
> 
> On Wed, Jul 28, 2010 at 11:56 AM, Brett Wilson <mailto:brettw@chromium.org>
wrote:
> > This looks OK from a high level. I would be interested in trying to
> > simplify some of the widget stuff you mentioned, but that doesn't need
> > to block current work.
> >
> > Brett
> >
>

[darin (slow to review), 2010-07-28 20:44:43]

I have some concerns about the design.  I've just reviewed the bits in
chrome/renderer/.

http://codereview.chromium.org/3055009/diff/20002/21029
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/3055009/diff/20002/21029#newcode334
chrome/renderer/render_view.h:334: virtual void
createFullscreenWindow(WebKit::WebWidget* webwidget);
i'm surprised that this method does not return WebKit::WebWidget*.  it would
seem instead that you have webkit creating a WebWidget internally and then
passing it out to the consumer to ask the consumer to show it fullscreen. 
that's backwards from how these things normally work.

normally, webkit asks the embedder to create a FooWidget (view or popupmenu),
and the embedder calls into WebKit to instantiate such an object, passing it a
WebWidgetClient (or WebViewClient).

http://codereview.chromium.org/3055009/diff/20002/21030
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/3055009/diff/20002/21030#newcode712
chrome/renderer/render_widget.cc:712: Send(new
ViewHostMsg_GetViewBounds(routing_id_, &rect));
do you need this IPC?  we already have an IPC to query the bounds of the screen.
 or, what's wrong with the Resize IPC that the browser sends?

http://codereview.chromium.org/3055009/diff/20002/21031
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/3055009/diff/20002/21031#newcode95
chrome/renderer/render_widget.h:95: virtual WebKit::WebSize size();
is there a webkit API patch corresponding to this?  i'm not sure what this
"size()" method means.

http://codereview.chromium.org/3055009/diff/20002/21032
File chrome/renderer/render_widget_fullscreen.cc (right):

http://codereview.chromium.org/3055009/diff/20002/21032#newcode24
chrome/renderer/render_widget_fullscreen.cc:24: DCHECK(!webwidget_);
it is a shame to duplicate code from RenderWidget::Init.  You should instead
find a way to refactor that code so that the common bits can be shared.  this
should be relatively straightforward given that RenderWidgetFullscreen extends
from RenderWidget.

[boliu, 2010-07-29 00:53:18]

Refactored how WebWidget is created. Added static method
RenderWidget::CreateWebWidget, and similar in RenderWidgetFullscreen. This way
the WebWidget is created based on the WebPopupType.

Currently the WebKit patch only involves adding the createFullscreenWindow
method to WebViewClient, so it is not uploaded. With RenderWidgetFullscreen
being a subclass of RenderWidget, it might be reasonable to create corresponding
WebWidgetFullscreen, WebWidgetFullscreenClient, WebFullscreenType subclasses in
WebKit. However the existing WebWidget* classes meet the needs of full screen
media player. What level of separation from RenderWidget should full screen be?

http://codereview.chromium.org/3055009/diff/20002/21030
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/3055009/diff/20002/21030#newcode712
chrome/renderer/render_widget.cc:712: Send(new
ViewHostMsg_GetViewBounds(routing_id_, &rect));
I did not realize screenSize() did the same thing. Removed size() and related
code.

On 2010/07/28 20:44:44, darin wrote:
> do you need this IPC?  we already have an IPC to query the bounds of the
screen.
>  or, what's wrong with the Resize IPC that the browser sends?

http://codereview.chromium.org/3055009/diff/36001/1071#newcode106
chrome/renderer/render_widget.cc:106: void RenderWidget::Init(int32 opener_id) {
Is there a subclass of WebWidget for Suggest?

http://codereview.chromium.org/3055009/diff/36001/1071#newcode108
chrome/renderer/render_widget.cc:108: 
default is here since some types are handled in
RenderWidgetFullscreen::CreateWebWidget

[boliu, 2010-08-02 17:56:59]

ping?

I am also working on the patches that uses full screen for video. Trying to get
those cleaned up and uploaded to code review some time today.

[boliu, 2010-08-03 00:31:13]

Subsequent patches for full screen video:
http://codereview.chromium.org/3026043
https://bugs.webkit.org/show_bug.cgi?id=43394

[scherkus (not reviewing), 2010-08-03 22:57:51]

so this Looks Good To Me as it looks like darin's comments were addressed --
brettw mind taking another look at the browser stuff?

http://codereview.chromium.org/3055009/diff/42001/43011
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/3055009/diff/42001/43011#newcode74
chrome/browser/renderer_host/render_widget_host_view.h:74: virtual void
InitAsFullscreen(RenderWidgetHostView* parent_host_view) {}
maybe implement this in the .cc with a NOTIMPLEMENTED() + message?

http://codereview.chromium.org/3055009/diff/42001/43028
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/3055009/diff/42001/43028#newcode106
chrome/renderer/render_widget.cc:106: // TODO(boliu):
todo what?

http://codereview.chromium.org/3055009/diff/42001/43029
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/3055009/diff/42001/43029#newcode135
chrome/renderer/render_widget.h:135: void DoInit(int32 opener_id,
should have a comment considering there's Init, DoInit and CompleteInit

[scherkus (not reviewing), 2010-08-03 22:58:23]

also... any chance for some unit tests?

[boliu, 2010-08-06 22:11:44]

I have spent a day looking at writing unit tests, but have not really found
anything that lends itself to testing. Even though the patch touches a lot of
files, each files changes very little. Also the code path follows existing code
paths that are already tested.

I think integration testing is probably better in this case, ie trigger a full
screen from renderer and check for the resulting window (or the call to
InitAsFullscreen, etc). Similar for closing the full screen window. Will look
into it. Any advice/help on where to look? Reading the testing page on
chromium.org didn't really help.

[John Grabowski, 2010-08-06 22:51:23]

Remember that all unit tests are run under valgrind.  Even a basic
allocation/deallocation of an object without any EXPECTS_ calls can be
useful.  Using this information you can write test which simply creates then
deletes a fullscreen widget and gain some benefit.  Incidentally, doing so
"the hard way" can exercise quite a bit of the plumbing you hooked up (e.g.
RenderWidgerHelper::CreateNewFullscreenWidget,
RenderWidgetHelper::OnCreateFullscreenWidgetOnUI, ...)

I noticed you created a new IPC.  Are you sure you got
the marshaling correct?  See Darin's buzz for the cost of getting it just a
little wrong (and see how it was caught in valgrind).  Write a test which
uses your new IPC call.

jrg

On Fri, Aug 6, 2010 at 3:11 PM, <boliu@chromium.org> wrote:

> I have spent a day looking at writing unit tests, but have not really found
> anything that lends itself to testing. Even though the patch touches a lot
> of
> files, each files changes very little. Also the code path follows existing
> code
> paths that are already tested.
>
> I think integration testing is probably better in this case, ie trigger a
> full
> screen from renderer and check for the resulting window (or the call to
> InitAsFullscreen, etc). Similar for closing the full screen window. Will
> look
> into it. Any advice/help on where to look? Reading the testing page on
> chromium.org didn't really help.
>
>
> http://codereview.chromium.org/3055009/show
>

[boliu, 2010-08-16 21:32:11]

After spending two days on writing some unit tests, only wrote a 4-line unit
test that runs through parts of the code for creating a full screen widget host.

[boliu, 2010-08-17 19:19:17]

So, is this patch close to being commit quality yet?

I'm asking because this is the last week of my internship and would like to
commit some of the numerous patches that I have for full screen video.

[scherkus (not reviewing), 2010-08-18 19:12:31]

yes LGTM assuming them trybots pass