Pepper and WebKit API change to support a plugin knowing if a scrollbar is an overlay one.

Pepper and WebKit API change to support a plugin knowing if a scrollbar is an overlay one.

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

[jam, 2011-08-02 05:51:33]

Brett: can you review the non-WebKit parts? This is mostly done although there's
one bug left that I suspect is in the WebKit part, but I'd like to get this
reviewed in-parallel.

[brettw, 2011-08-02 16:41:38]

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppb_scrollbar_d...
File ppapi/c/dev/ppb_scrollbar_dev.h (right):

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppb_scrollbar_d...
ppapi/c/dev/ppb_scrollbar_dev.h:43: // Returns PP_TRUE if this is an overlay
scrollbar.
"this" -> it would be nice if this comment mentioned how one gets into an
"overlay scrollbar" state. Probably you want to say "the system scrollbar style
is an overlap scrollbar" or something.

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppp_scrollbar_d...
File ppapi/c/dev/ppp_scrollbar_dev.h (right):

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppp_scrollbar_d...
ppapi/c/dev/ppp_scrollbar_dev.h:14: #define PPP_SCROLLBAR_DEV_INTERFACE
"PPP_Scrollbar(Dev);0.2"
This needs to be revved.

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppp_scrollbar_d...
ppapi/c/dev/ppp_scrollbar_dev.h:22: // Informs the instance that the scrollbar's
overlay type has changed.
Can you elaborate on what it means to have one's overlap type changed? What will
cause this to happen?

[darin (slow to review), 2011-08-02 17:40:01]

WebKit API changes look fine to me.  If you post those to a WebKit bug, just cc
me (fishd@), and I'll R+.

[jam, 2011-08-03 16:12:09]

Brett: please diff patchset 3 and 6
Darin: I'll send you the WebKit changes soon

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppb_scrollbar_d...
File ppapi/c/dev/ppb_scrollbar_dev.h (right):

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppb_scrollbar_d...
ppapi/c/dev/ppb_scrollbar_dev.h:43: // Returns PP_TRUE if this is an overlay
scrollbar.
On 2011/08/02 16:41:38, brettw wrote:
> "this" -> it would be nice if this comment mentioned how one gets into an
> "overlay scrollbar" state. Probably you want to say "the system scrollbar
style
> is an overlap scrollbar" or something.

Done.

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppp_scrollbar_d...
File ppapi/c/dev/ppp_scrollbar_dev.h (right):

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppp_scrollbar_d...
ppapi/c/dev/ppp_scrollbar_dev.h:14: #define PPP_SCROLLBAR_DEV_INTERFACE
"PPP_Scrollbar(Dev);0.2"
On 2011/08/02 16:41:38, brettw wrote:
> This needs to be revved.

Done.

http://codereview.chromium.org/7538006/diff/10001/ppapi/c/dev/ppp_scrollbar_d...
ppapi/c/dev/ppp_scrollbar_dev.h:22: // Informs the instance that the scrollbar's
overlay type has changed.
On 2011/08/02 16:41:38, brettw wrote:
> Can you elaborate on what it means to have one's overlap type changed? What
will
> cause this to happen?

Done.

[brettw, 2011-08-03 17:13:58]

LGTM

http://codereview.chromium.org/7538006/diff/27001/webkit/plugins/ppapi/ppb_sc...
File webkit/plugins/ppapi/ppb_scrollbar_impl.cc (right):

http://codereview.chromium.org/7538006/diff/27001/webkit/plugins/ppapi/ppb_sc...
webkit/plugins/ppapi/ppb_scrollbar_impl.cc:241: // Try the old version.
Can you add here that this is OK to do because the old version is a subset of
the new one, with ValueChanged not changing?

[jam, 2011-08-05 08:00:41]

Brett & Darin: please have a look at this latest patchset. I've added a
ScrollbarGroup object to Pepper and WebKit per our discussion. If this looks
good to you, then I'll split it up and figure out how to check them in without
breaking either tree's compile.  I don't see any easy way of maintaining
functionality other than by renaming the modified classes to something new and
then renaming it back, which doesn't seem worth the cost.

Sailesh: I can't figure out how to make the scrollbars show up on resize. I
plumbed through willStartLiveResize/willEndLiveResize in this patch, and in a
local tree contentAreaDidShow/contentAreaDidHide, but nothing helped. Stranger
still, I commented out all these calls in ScrollAnimatorChromiumMac for the
regular web content case and the scrollbars still show up in web content when
resizing. So I'm not sure at this point what needs to be connected for them to
show up.

[darin (slow to review), 2011-08-05 15:23:59]

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_d...
File ppapi/c/dev/ppb_scrollbar_dev.h (right):

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_d...
ppapi/c/dev/ppb_scrollbar_dev.h:32: PP_Instance scrollbar_group,
nit: no longer need a PP_Instance parameter (since you can get it from the
scrollbar_group), and the type of "scrollbar_group" should be PP_Resource.

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
File ppapi/c/dev/ppb_scrollbar_group_dev.h (right):

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
ppapi/c/dev/ppb_scrollbar_group_dev.h:22: // Used on platforms that animate the
scrollbars when resizing or when the
when we discussed what to call this interface, i had no idea it would have all
of these methods.  these methods make me think this interface should have a name
more like PPB_ScrollableArea_Dev.

like we were talking yesterday, we'll probably want to move away from allowing
plugins to allocate individual scrollbars, and instead build up the concept of a
scrollable area that the plugin can render into.  this could be the start of
that?

[jam, 2011-08-05 17:07:01]

Brett: you can hold off on reviewing

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
File ppapi/c/dev/ppb_scrollbar_group_dev.h (right):

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
ppapi/c/dev/ppb_scrollbar_group_dev.h:22: // Used on platforms that animate the
scrollbars when resizing or when the
On 2011/08/05 15:24:00, darin wrote:
> when we discussed what to call this interface, i had no idea it would have all
> of these methods.  these methods make me think this interface should have a
name
> more like PPB_ScrollableArea_Dev.
> 
> like we were talking yesterday, we'll probably want to move away from allowing
> plugins to allocate individual scrollbars, and instead build up the concept of
a
> scrollable area that the plugin can render into.  this could be the start of
> that?

I agree. It seems cumbersome for the embedder to create two scrollbars and a
scrollbar group, and implement three interfaces (widget/scrollbar/resize) so
they can show scrollbars.

I'll take a stab to change this to one interface each way. I'm going to try to
figure out why they're not showing on resize though, because that will probably
change the API.

[sail1, 2011-08-05 17:14:48]

>
> Sailesh: I can't figure out how to make the scrollbars show up on resize. I
> plumbed through willStartLiveResize/**willEndLiveResize in this patch, and
> in a
> local tree contentAreaDidShow/**contentAreaDidHide, but nothing helped.
> Stranger
> still, I commented out all these calls in ScrollAnimatorChromiumMac for the
> regular web content case and the scrollbars still show up in web content
> when
> resizing. So I'm not sure at this point what needs to be connected for them
> to
> show up.
>
>
That's really weird. I'll try this on my machine and see if I can figure it
out. Which patches should I apply to get my source to the same state as
yours?

Thanks!
Sailesh

[sail1, 2011-08-05 23:12:51]

Just an update, this is working on my machine now.

On Fri, Aug 5, 2011 at 10:14 AM, Sailesh Agrawal <sail@google.com> wrote:

> Sailesh: I can't figure out how to make the scrollbars show up on resize. I
>> plumbed through willStartLiveResize/**willEndLiveResize in this patch,
>> and in a
>> local tree contentAreaDidShow/**contentAreaDidHide, but nothing helped.
>> Stranger
>> still, I commented out all these calls in ScrollAnimatorChromiumMac for
>> the
>> regular web content case and the scrollbars still show up in web content
>> when
>> resizing. So I'm not sure at this point what needs to be connected for
>> them to
>> show up.
>>
>>
> That's really weird. I'll try this on my machine and see if I can figure it
> out. Which patches should I apply to get my source to the same state as
> yours?
>
> Thanks!
> Sailesh
>

[jam, 2011-08-06 02:15:58]

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
File ppapi/c/dev/ppb_scrollbar_group_dev.h (right):

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
ppapi/c/dev/ppb_scrollbar_group_dev.h:22: // Used on platforms that animate the
scrollbars when resizing or when the
Rereading what you wrote, I realize I might have misunderstood what you meant
initially. I was thinking of having one interface in each direction for this. I
think the plugin will still want to control painting. The reason is for most
plugins, I suspect they will want to make scrollbars synchronous with the data
which shouldn't be very expensive to render. In the pdf case, the document is
sometimes very slow to render and this makes us want to render the scrollbars
first always. This necessitates a complicated PaintManager which I think would
only be useful for the pdf plugin.

[jam, 2011-08-06 15:57:47]

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
File ppapi/c/dev/ppb_scrollbar_group_dev.h (right):

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
ppapi/c/dev/ppb_scrollbar_group_dev.h:22: // Used on platforms that animate the
scrollbars when resizing or when the
On 2011/08/06 02:15:58, John Abd-El-Malek wrote:
> Rereading what you wrote, I realize I might have misunderstood what you meant
> initially. I was thinking of having one interface in each direction for this.
I
> think the plugin will still want to control painting. The reason is for most
> plugins, I suspect they will want to make scrollbars synchronous with the data
> which shouldn't be very expensive to render. In the pdf case, the document is
> sometimes very slow to render and this makes us want to render the scrollbars
> first always. This necessitates a complicated PaintManager which I think would
> only be useful for the pdf plugin.

actually, thinking more about this, we probably don't even need to involve the
plugin. i.e. instead of having the plugin need to implement the new resize api
and pass the callbacks from there to scroll_group, we can do this automatically.

[jam, 2011-08-06 17:29:47]

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
File ppapi/c/dev/ppb_scrollbar_group_dev.h (right):

http://codereview.chromium.org/7538006/diff/41001/ppapi/c/dev/ppb_scrollbar_g...
ppapi/c/dev/ppb_scrollbar_group_dev.h:22: // Used on platforms that animate the
scrollbars when resizing or when the
ok, patch uploaded. what do you think of this approach?

[brettw, 2011-08-08 20:22:47]

LGTM

http://codereview.chromium.org/7538006/diff/6122/ppapi/cpp/dev/scrollbar_dev.cc
File ppapi/cpp/dev/scrollbar_dev.cc (right):

http://codereview.chromium.org/7538006/diff/6122/ppapi/cpp/dev/scrollbar_dev....
ppapi/cpp/dev/scrollbar_dev.cc:34: instance.pp_instance(),
scrollbar_group.pp_resource(),
This line should be indented 2 more spaces (aligned with the one below).

http://codereview.chromium.org/7538006/diff/6122/ppapi/thunk/ppb_scrollbar_gr...
File ppapi/thunk/ppb_scrollbar_group_api.h (right):

http://codereview.chromium.org/7538006/diff/6122/ppapi/thunk/ppb_scrollbar_gr...
ppapi/thunk/ppb_scrollbar_group_api.h:13: class PPB_ScrollbarGroup_API {
Can you add a comment here why this is necessary even though it has no
functions?

http://codereview.chromium.org/7538006/diff/47001/ppapi/c/dev/ppb_scrollbar_d...
File ppapi/c/dev/ppb_scrollbar_dev.h (right):

http://codereview.chromium.org/7538006/diff/47001/ppapi/c/dev/ppb_scrollbar_d...
ppapi/c/dev/ppb_scrollbar_dev.h:31: PP_Resource (*Create)(PP_Instance
scrollbar_group,
I think you meant to change the first arg type to PP_Resource.

[jam, 2011-08-08 20:27:46]

http://codereview.chromium.org/7538006/diff/6122/ppapi/thunk/ppb_scrollbar_gr...
File ppapi/thunk/ppb_scrollbar_group_api.h (right):

http://codereview.chromium.org/7538006/diff/6122/ppapi/thunk/ppb_scrollbar_gr...
ppapi/thunk/ppb_scrollbar_group_api.h:13: class PPB_ScrollbarGroup_API {
On 2011/08/08 20:22:47, brettw wrote:
> Can you add a comment here why this is necessary even though it has no
> functions?

Done.

http://codereview.chromium.org/7538006/diff/47001/ppapi/c/dev/ppb_scrollbar_d...
File ppapi/c/dev/ppb_scrollbar_dev.h (right):

http://codereview.chromium.org/7538006/diff/47001/ppapi/c/dev/ppb_scrollbar_d...
ppapi/c/dev/ppb_scrollbar_dev.h:31: PP_Resource (*Create)(PP_Instance
scrollbar_group,
On 2011/08/08 20:22:47, brettw wrote:
> I think you meant to change the first arg type to PP_Resource.

Done.

[jam, 2011-08-08 20:40:38]

Darin: can you take a look at the WebKit changes?  If it's good, then I'll split
the two changes.

[darin (slow to review), 2011-08-08 22:22:06]

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/public/WebScrollbar.h (right):

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/public/WebScrollbar.h:67:
WEBKIT_EXPORT static WebScrollbar* create(WebScrollbarClient*,
there is a little bit of convention in the WebKit API to put the Client pointer
as the last parameter on functions like this.  WebScrollbar was the one
exception.

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/public/WebScrollbarGroup.h
(right):

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/public/WebScrollbarGroup.h:46:
WEBKIT_EXPORT static WebScrollbarGroup* create(WebPluginContainer*);
hmm... is it really necessary for this to take a WebPluginContainer?  why does
it need to be associated with a plugin?  if we do need this, then we should
probably call the method "createForPlugin" or something.

i suppose you needed this in order to support the case where it is not just a
full-page plugin allocating scrollbars?

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp
(right):

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:312:
(*it)->contentResized();
do we need to worry about contentResized() triggering code that might mutate
m_scrollbarGroups?

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:458:
(*it)->willStartLiveResize();
same question about mutation

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:464:
(*it)->willEndLiveResize();
same question about mutation

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:507:
(*it)->mouseMovedInContentArea();
ditto, ditto, ditto

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/src/WebScrollbarGroupImpl.cpp
(right):

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/WebScrollbarGroupImpl.cpp:61:
WEBKIT_ASSERT(!m_horizontalScrollbar);
it looked like ~WebScrollbarGroup was public.  these assertions suggest that
people need to tear down the WebScrollbar objects first, before they delete the
WebScrollbarGroupImpl.  maybe reference counting would help?

http://codereview.chromium.org/7538006/diff/48034/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/WebScrollbarGroupImpl.cpp:273:
bool WebScrollbarGroupImpl::isOnActivePage() const {
nit: opening bracket

[jam, 2011-08-10 17:48:01]

Brett/Darin: can you guys please take another look? thanks

[darin (slow to review), 2011-08-10 18:00:34]

WebKit changes look good

http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/public/WebScrollbar.h (right):

http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/public/WebScrollbar.h:68:
WEBKIT_EXPORT static WebScrollbar* create(Orientation,
nit: Let's call this createForPlugin just to be a bit more self-documenting.

http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/public/WebScrollbar.h:78: virtual bool
isOverlay() = 0;
can this be a const method?  (like value())

http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
File third_party/WebKit/Source/WebKit/chromium/src/ScrollbarGroup.cpp (right):

http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
third_party/WebKit/Source/WebKit/chromium/src/ScrollbarGroup.cpp:41: using
namespace std;
is 'using namespace std' needed?  doesn't look like it, but i might have missed
something.

[darin (slow to review), 2011-08-10 18:00:56]

Pepper API changes look good.

[jam, 2011-08-10 18:03:06]

all done, thanks. creating webkit patch now.

On 2011/08/10 18:00:34, darin wrote:
> WebKit changes look good
> 
>
http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
> File third_party/WebKit/Source/WebKit/chromium/public/WebScrollbar.h (right):
> 
>
http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
> third_party/WebKit/Source/WebKit/chromium/public/WebScrollbar.h:68:
> WEBKIT_EXPORT static WebScrollbar* create(Orientation,
> nit: Let's call this createForPlugin just to be a bit more self-documenting.
> 
>
http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
> third_party/WebKit/Source/WebKit/chromium/public/WebScrollbar.h:78: virtual
bool
> isOverlay() = 0;
> can this be a const method?  (like value())
> 
>
http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
> File third_party/WebKit/Source/WebKit/chromium/src/ScrollbarGroup.cpp (right):
> 
>
http://codereview.chromium.org/7538006/diff/60052/third_party/WebKit/Source/W...
> third_party/WebKit/Source/WebKit/chromium/src/ScrollbarGroup.cpp:41: using
> namespace std;
> is 'using namespace std' needed?  doesn't look like it, but i might have
missed
> something.

[brettw, 2011-08-10 18:17:31]

Pepper stuff LGTM