Clarify/fix fullscreen semantics, and add GetScreenSize

Clarify/fix fullscreen semantics, and add GetScreenSize

BUG=none
TEST=youtube, nba, amazon

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

[viettrungluu, 2011-03-08 00:06:01]

Mostly nits/questions....

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/pepper_plugin_...
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/pepper_plugin_...
chrome/renderer/pepper_plugin_delegate_impl.cc:42: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebScreenInfo.h"
nit: alphabetical order

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/render_widget_...
File chrome/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/render_widget_...
chrome/renderer/render_widget_fullscreen_pepper.cc:96: // TODO(piman): figure
out IME and implement these if necessary.
Does leaving in the NOTIMPLEMENTED()s result in a lot of spew?

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/render_widget_...
chrome/renderer/render_widget_fullscreen_pepper.cc:215: if (plugin_)
Why do you sometimes check |plugin_| and sometimes not?

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:255: PP_Bool
GetScreenSize(PP_Instance instance_id, PP_Size* size) {
Maybe it should return an int32_t error code?

Though I guess that would be inconsistent with SetFullscreen(). Hrm.

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:256: PluginInstance* instance =
ResourceTracker::Get()->GetInstance(instance_id);
I usually like to do |if (!size) return PP_ERROR_BADARGUMENT;| (for non-optional
out params). Maybe you should do that (returning PP_FALSE, I guess)?

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1013: if (fullscreen ==
is_fullscreen)
Probably better to write this as |if (fullscreen ==
IsFullscreenOrPending())|....

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.h:192: // - normal
(fullscreen_container_ == NULL)
I wonder if it'd be nicer to just have a state variable with explicit states.

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.h:241: // Reports the current plugin
geometry to the plugin.
How does it report the geometry?

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.h:354: // fullscreen_ is true (see
above).
is false?

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppb_graph...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right):

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppb_graph...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:464:
unpainted_flush_callback_.Clear();
Is this Clear() really needed?

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppb_graph...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:470:
painted_flush_callback_.Clear();
"

[piman, 2011-03-08 00:55:30]

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/pepper_plugin_...
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/pepper_plugin_...
chrome/renderer/pepper_plugin_delegate_impl.cc:42: #include
"third_party/WebKit/Source/WebKit/chromium/public/WebScreenInfo.h"
On 2011/03/08 00:06:01, viettrungluu wrote:
> nit: alphabetical order

Done.

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/render_widget_...
File chrome/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/render_widget_...
chrome/renderer/render_widget_fullscreen_pepper.cc:96: // TODO(piman): figure
out IME and implement these if necessary.
On 2011/03/08 00:06:01, viettrungluu wrote:
> Does leaving in the NOTIMPLEMENTED()s result in a lot of spew?

Yes, 2 per frame when in full-screen, including in release, so I removed them.

http://codereview.chromium.org/6625034/diff/32/chrome/renderer/render_widget_...
chrome/renderer/render_widget_fullscreen_pepper.cc:215: if (plugin_)
On 2011/03/08 00:06:01, viettrungluu wrote:
> Why do you sometimes check |plugin_| and sometimes not?

The RenderWidgetFullscreenPepper checks for it because it can be set back to
NULL from RenderWidgetFullscreenPepper::Destroy.
The PepperWidget doesn't need to though, because it always keeps a reference to
the plugin.

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:256: PluginInstance* instance =
ResourceTracker::Get()->GetInstance(instance_id);
On 2011/03/08 00:06:01, viettrungluu wrote:
> I usually like to do |if (!size) return PP_ERROR_BADARGUMENT;| (for
non-optional
> out params). Maybe you should do that (returning PP_FALSE, I guess)?

Done.

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1013: if (fullscreen ==
is_fullscreen)
On 2011/03/08 00:06:01, viettrungluu wrote:
> Probably better to write this as |if (fullscreen ==
> IsFullscreenOrPending())|....

Done.

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.h:192: // - normal
(fullscreen_container_ == NULL)
On 2011/03/08 00:06:01, viettrungluu wrote:
> I wonder if it'd be nicer to just have a state variable with explicit states.

Now I would have 2 problems instead of 1 :)

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.h:241: // Reports the current plugin
geometry to the plugin.
On 2011/03/08 00:06:01, viettrungluu wrote:
> How does it report the geometry?

Done.

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppb_graph...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right):

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppb_graph...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:464:
unpainted_flush_callback_.Clear();
On 2011/03/08 00:06:01, viettrungluu wrote:
> Is this Clear() really needed?

Done.

http://codereview.chromium.org/6625034/diff/32/webkit/plugins/ppapi/ppb_graph...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:470:
painted_flush_callback_.Clear();
On 2011/03/08 00:06:01, viettrungluu wrote:
> "

Done.

[viettrungluu, 2011-03-08 00:58:39]

lgtm