Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1711)

Issue 7714017: Reimplement the Pepper fullscreen API to use webkitRequestFullScreen and friends. (Closed)

Created:
9 years, 4 months ago by jeremya
Modified:
9 years, 3 months ago
Reviewers:
polina, brettw
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, koz (OOO until 15th September)
Base URL:
ssh://matter.syd/usr/local/google/chromium2/src@master
Visibility:
Public.

Description

Reimplement the Pepper fullscreen API to use webkitRequestFullScreen and friends. This depends on https://bugs.webkit.org/show_bug.cgi?id=66746 and http://codereview.chromium.org/7461059/. BUG= TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98767

Patch Set 1 #

Total comments: 4

Patch Set 2 : review comments #

Patch Set 3 : mock_plugin_delegate #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+32 lines, -116 lines) Patch
M content/renderer/pepper_plugin_delegate_impl.h View 1 2 1 chunk +0 lines, -3 lines 0 comments Download
M content/renderer/pepper_plugin_delegate_impl.cc View 1 2 2 chunks +0 lines, -13 lines 0 comments Download
M webkit/plugins/ppapi/mock_plugin_delegate.h View 1 2 1 chunk +0 lines, -2 lines 0 comments Download
M webkit/plugins/ppapi/mock_plugin_delegate.cc View 1 2 1 chunk +0 lines, -5 lines 0 comments Download
M webkit/plugins/ppapi/plugin_delegate.h View 1 2 2 chunks +0 lines, -6 lines 0 comments Download
M webkit/plugins/ppapi/ppapi_plugin_instance.h View 1 2 3 chunks +5 lines, -9 lines 1 comment Download
M webkit/plugins/ppapi/ppapi_plugin_instance.cc View 1 2 13 chunks +25 lines, -72 lines 4 comments Download
M webkit/plugins/ppapi/ppapi_webplugin_impl.cc View 1 2 3 chunks +2 lines, -6 lines 1 comment Download

Messages

Total messages: 8 (0 generated)
brettw
http://codereview.chromium.org/7714017/diff/1/content/renderer/pepper_plugin_delegate_impl.cc File content/renderer/pepper_plugin_delegate_impl.cc (right): http://codereview.chromium.org/7714017/diff/1/content/renderer/pepper_plugin_delegate_impl.cc#newcode1364 content/renderer/pepper_plugin_delegate_impl.cc:1364: void PepperPluginDelegateImpl::SetFullscreen( Do we even need this function anymore? ...
9 years, 4 months ago (2011-08-24 23:14:04 UTC) #1
jeremya
One thing I'm not sure about is whether a "DidChangeView" event is sufficient... that function ...
9 years, 4 months ago (2011-08-24 23:40:45 UTC) #2
brettw
On 2011/08/24 23:40:45, jeremya wrote: > One thing I'm not sure about is whether a ...
9 years, 4 months ago (2011-08-24 23:43:36 UTC) #3
brettw
LGTM. Please send a message to pepper-dev when you land this to let people know ...
9 years, 4 months ago (2011-08-24 23:44:24 UTC) #4
jeremya
On 2011/08/24 23:44:24, brettw wrote: > LGTM. Please send a message to pepper-dev when you ...
9 years, 4 months ago (2011-08-24 23:45:12 UTC) #5
polina
Should this note be take out of the header? I think you mentioned to me ...
9 years, 3 months ago (2011-08-30 18:31:32 UTC) #6
jeremya
Yep, that's no longer true. On Wed, Aug 31, 2011 at 4:31 AM, <polina@google.com> wrote: ...
9 years, 3 months ago (2011-08-30 21:18:45 UTC) #7
polina
9 years, 3 months ago (2011-09-02 00:19:26 UTC) #8
The things I identified below explain the differences in the test behavior that
I mentioned on the other thread with respect to binding graphics devices, etc. I
am suggesting the fixes to put in the CL I am working on that will provide both
implementations. Thanks!

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (left):

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:785: if (container_ &&
!fullscreen_container_)
!fullscreen_container means "normal state", so under new implementation
shouldn't this be:
if (container_ && !fullscreen)
?

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:974: BindGraphics(pp_instance(),
0);
do we no longer want to invalidate graphics resources under the new
implementation when switching to/from fullscreen?
(we could also only do this if the new state is different from the
current/pending one)

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1313: if (fullscreen_container_)
fullscreen_container_ != NULL means "in fullscreen or transitioning", so
shouldn't this now be

if (desired_fullscreen_state_)

?

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:1362: if (fullscreen_container_ &&
!fullscreen_)
This should now be
if (desired_fullscreen_state_ != fullscreen_) // in transition to/from

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_p...
File webkit/plugins/ppapi/ppapi_plugin_instance.h (right):

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_p...
webkit/plugins/ppapi/ppapi_plugin_instance.h:212: // are 3 states:
This comment is out of date for the new implementation. We now have 4 states:
normal, fullscreen pending, fullscreen, normal pending.

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_w...
File webkit/plugins/ppapi/ppapi_webplugin_impl.cc (left):

http://codereview.chromium.org/7714017/diff/2011/webkit/plugins/ppapi/ppapi_w...
webkit/plugins/ppapi/ppapi_webplugin_impl.cc:121: if
(!instance_->IsFullscreenOrPending())
why are we no longer checking this?

Powered by Google App Engine
This is Rietveld 408576698