[Mac] Rubber-banding on Lion.

[Mac] Rubber-banding on Lion.

BUG=88353
TEST=On Lion, scroll past the edge of the web content area and see textured over-scroll area.

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

[Nico, 2011-08-08 17:30:53]

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1235: -
(void)beginGestureWithEvent:(NSEvent*)theEvent {
s/theEvent/event/

This override means the same override in browser_window_controller will no
longer be called and swipe-for-history will regress. That code needs to be
redone anyway so that's ok, but maybe you don't want to land this immediately

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1248: }];
do you want to retain the monitor?

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1620:
[NSGraphicsContext saveGraphicsState];
We have a ScopedSaveNSGraphicsState somewhere in base

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1621: NSBezierPath
*drawingPath = [NSBezierPath bezierPathWithRect:rect];
space after *, not before

http://codereview.chromium.org/7582009/diff/14/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/7582009/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host.h:555: // The current size of
the underlying contents.
Needs longer comment (what is this good for, how is it different from
current_size_, etc)

[Alexei Svitkine (slow), 2011-08-08 18:31:31]

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1248: }];
On 2011/08/08 17:30:53, Nico wrote:
> do you want to retain the monitor?

Apple's examples don't do it:

http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/EventO...

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1620:
[NSGraphicsContext saveGraphicsState];
On 2011/08/08 17:30:53, Nico wrote:
> We have a ScopedSaveNSGraphicsState somewhere in base

Done.

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1621: NSBezierPath
*drawingPath = [NSBezierPath bezierPathWithRect:rect];
On 2011/08/08 17:30:53, Nico wrote:
> space after *, not before

Done.

http://codereview.chromium.org/7582009/diff/14/content/browser/renderer_host/...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/7582009/diff/14/content/browser/renderer_host/...
content/browser/renderer_host/render_widget_host.h:555: // The current size of
the underlying contents.
On 2011/08/08 17:30:53, Nico wrote:
> Needs longer comment (what is this good for, how is it different from
> current_size_, etc)

Done.

[Nico, 2011-08-08 18:49:15]

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1248: }];
On 2011/08/08 18:31:31, asvitkine_ wrote:
> On 2011/08/08 17:30:53, Nico wrote:
> > do you want to retain the monitor?
> 
> Apple's examples don't do it:
> 
>
http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/EventO...

Did you check if that example is built with garbage collection or ARC?

[Alexei Svitkine (slow), 2011-08-08 18:58:49]

>http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/EventOverview/MonitoringEvents/MonitoringEvents.html
>
> Did you check if that example is built with garbage collection or ARC?

From that link:

"Both class methods return the monitor object, which the calling
object does not own (and thus has no need to retain or release)."

[Nico, 2011-08-08 19:44:59]

The code looks ok. Can you give me a high-level description of how this works,
with all pieces (chrome, webkit)?

(Why did you override beingGesture…: instead of doing this in scrollEvent:?)

http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1236: -
(void)beginGestureWithEvent:(NSEvent*)theEvent {
DCHECK(IsOnSnowLeopardOrLater())

http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1636:
frameRect.origin = NSMakePoint(0, 0);
(does all this work in HighDPI mode? Just curious, it's not very high priority)

http://codereview.chromium.org/7582009/diff/8008/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1223: if ([theEvent
subtype] == kIOHIDEventTypeScroll) {
can you DCHECK this instead?

[Alexei Svitkine (slow), 2011-08-08 20:06:29]

> (Why did you override beingGesture…: instead of doing this in scrollEvent:?)

I followed how it's done in WebKit2. I didn't try scrollEvent:.

> The code looks ok. Can you give me a high-level description of how this works,
> with all pieces (chrome, webkit)?

Sure. So all the logic for controlling the scroll behaviour was already in
webkit in ScrollAnimatorChromiumMac.mm, which we had copied from Apple's
ScrollAnimatorMac.mm. It was merely inactive.

To activate it, three things were needed:
1. Building WebKit with ENABLE(RUBBER_BANDING)
2. Forwarding gesture events to WebKit
3. Exposing more properties on scroll wheel events to WebKit.

So my WebKit patch adds all the various plumbing needed for that (including
additional classes for gesture events and conversion code).

On the Chrome side, what's needed is:
1. Detecting when we're in overscroll mode, and drawing the gutters.
2. Forwarding the gesture events to WebKit.
3. Forwarding extra mouse wheel event properties to WebKit (this is part of the
WebKit patch - as it's done in WebInputEventFactory::mouseWheelEvent()).

In order to do 1, I had to make the browser process aware of the visible content
area rect - it already had access to scroll offsets, but not the size of the
content area. So by forwarding the content size, the rwvhm could figure out if
the visible content area doesn't cover the whole view area, and thus requires
painting of the gutter pattern.

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1235: -
(void)beginGestureWithEvent:(NSEvent*)theEvent {
On 2011/08/08 17:30:53, Nico wrote:
> This override means the same override in browser_window_controller will no
> longer be called and swipe-for-history will regress. That code needs to be
> redone anyway so that's ok, but maybe you don't want to land this immediately

I've fixed this by also forwarding it to the next responder.

http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1236: if ([theEvent
subtype] == kIOHIDEventTypeScroll) {
On 2011/08/08 19:44:59, Nico wrote:
> DCHECK(IsOnSnowLeopardOrLater())

In browser_window_controller.mm there's a comment saying that it can get called
on 10.5. What's your reason for wanting that DCHECK()?

http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1636:
frameRect.origin = NSMakePoint(0, 0);
On 2011/08/08 19:44:59, Nico wrote:
> (does all this work in HighDPI mode? Just curious, it's not very high
priority)

How would I test that?

http://codereview.chromium.org/7582009/diff/8008/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1223:
WebGestureEvent event = WebInputEventFactory::gestureEvent(theEvent, self);
On 2011/08/08 19:44:59, Nico wrote:
> can you DCHECK this instead?

I don't think so, since there might be other types of gestures?

[Nico, 2011-08-08 20:10:09]

http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1236: -
(void)beginGestureWithEvent:(NSEvent*)theEvent {
On 2011/08/08 20:06:29, asvitkine_ wrote:
> On 2011/08/08 19:44:59, Nico wrote:
> > DCHECK(IsOnSnowLeopardOrLater())
> 
> In browser_window_controller.mm there's a comment saying that it can get
called
> on 10.5. What's your reason for wanting that DCHECK()?

Ah, then you need to add that to the if I think?
addLocalMonitorForEventsMatchingMask: and blocks are 10.6+ only and will crash
with unresolved symbol on 10.5 (I think).

http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1636:
frameRect.origin = NSMakePoint(0, 0);
On 2011/08/08 20:06:29, asvitkine_ wrote:
> On 2011/08/08 19:44:59, Nico wrote:
> > (does all this work in HighDPI mode? Just curious, it's not very high
> priority)
> 
> How would I test that?

Open Quartz Debug, hit cmd-2, check the checkbox, log out & back in, open system
preferences, displays, select a highdpi resolution

[Nico, 2011-08-08 20:11:27]

http://codereview.chromium.org/7582009/diff/8008/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7582009/diff/8008/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:1223: if ([theEvent
subtype] == kIOHIDEventTypeScroll) {
On 2011/08/08 20:06:29, asvitkine_ wrote:
> On 2011/08/08 19:44:59, Nico wrote:
> > can you DCHECK this instead?
> 
> I don't think so, since there might be other types of gestures?

Isn't they only called the one in beginGestureWithEvent? …ah, got it. Ok then
:-)

[Nico, 2011-08-08 20:13:19]

On Mon, Aug 8, 2011 at 1:06 PM,  <asvitkine@chromium.org> wrote:
>> (Why did you override beingGesture…: instead of doing this in
>> scrollEvent:?)
>
> I followed how it's done in WebKit2. I didn't try scrollEvent:.
>
>> The code looks ok. Can you give me a high-level description of how this
>> works,
>> with all pieces (chrome, webkit)?
>
> Sure. So all the logic for controlling the scroll behaviour was already in
> webkit in ScrollAnimatorChromiumMac.mm, which we had copied from Apple's
> ScrollAnimatorMac.mm. It was merely inactive.
>
> To activate it, three things were needed:
> 1. Building WebKit with ENABLE(RUBBER_BANDING)
> 2. Forwarding gesture events to WebKit
> 3. Exposing more properties on scroll wheel events to WebKit.
>
> So my WebKit patch adds all the various plumbing needed for that (including
> additional classes for gesture events and conversion code).
>
> On the Chrome side, what's needed is:
> 1. Detecting when we're in overscroll mode, and drawing the gutters.
> 2. Forwarding the gesture events to WebKit.
> 3. Forwarding extra mouse wheel event properties to WebKit (this is part of
> the
> WebKit patch - as it's done in WebInputEventFactory::mouseWheelEvent()).
>
> In order to do 1, I had to make the browser process aware of the visible
> content
> area rect - it already had access to scroll offsets, but not the size of the
> content area. So by forwarding the content size, the rwvhm could figure out
> if
> the visible content area doesn't cover the whole view area, and thus
> requires
> painting of the gutter pattern.

Thanks for this overview!

>
>
>
http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm
> (right):
>
>
http://codereview.chromium.org/7582009/diff/14/chrome/browser/renderer_host/r...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1235: -
> (void)beginGestureWithEvent:(NSEvent*)theEvent {
> On 2011/08/08 17:30:53, Nico wrote:
>>
>> This override means the same override in browser_window_controller
>
> will no
>>
>> longer be called and swipe-for-history will regress. That code needs
>
> to be
>>
>> redone anyway so that's ok, but maybe you don't want to land this
>
> immediately
>
> I've fixed this by also forwarding it to the next responder.
>
>
http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1236: if
> ([theEvent subtype] == kIOHIDEventTypeScroll) {
> On 2011/08/08 19:44:59, Nico wrote:
>>
>> DCHECK(IsOnSnowLeopardOrLater())
>
> In browser_window_controller.mm there's a comment saying that it can get
> called on 10.5. What's your reason for wanting that DCHECK()?
>
>
http://codereview.chromium.org/7582009/diff/14003/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1636:
> frameRect.origin = NSMakePoint(0, 0);
> On 2011/08/08 19:44:59, Nico wrote:
>>
>> (does all this work in HighDPI mode? Just curious, it's not very high
>
> priority)
>
> How would I test that?
>
>
http://codereview.chromium.org/7582009/diff/8008/chrome/browser/renderer_host...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1223:
> WebGestureEvent event = WebInputEventFactory::gestureEvent(theEvent,
> self);
> On 2011/08/08 19:44:59, Nico wrote:
>>
>> can you DCHECK this instead?
>
> I don't think so, since there might be other types of gestures?
>
> http://codereview.chromium.org/7582009/
>

[Alexei Svitkine (slow), 2011-08-08 20:26:18]

On Mon, Aug 8, 2011 at 4:11 PM,  <thakis@chromium.org> wrote:
>
>
http://codereview.chromium.org/7582009/diff/8008/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm
> (right):
>
>
http://codereview.chromium.org/7582009/diff/8008/chrome/browser/renderer_host...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:1223: if
> ([theEvent subtype] == kIOHIDEventTypeScroll) {
> On 2011/08/08 20:06:29, asvitkine_ wrote:
>>
>> On 2011/08/08 19:44:59, Nico wrote:
>> > can you DCHECK this instead?
>
>> I don't think so, since there might be other types of gestures?
>
> Isn't they only called the one in beginGestureWithEvent? …ah, got it. Ok
> then :-)

That's funny, that's how I was reasoning about that code too until
your DCHECK() comment made me examine that code more carefully.

Now that I realise it doesn't work like that, I'm not sure we actually
need to use -addLocalMonitorForEventsMatchingMask: / blocks
here, since I could just implement -endGestureWithEvent: to get the
same effect.

I'll try that.

[Alexei Svitkine (slow), 2011-08-08 20:30:41]

Updated version which doesn't use blocks (and thus doesn't need 10.6 SDK), but
works just as well.

[Mark Mentovai, 2011-08-08 20:56:08]

So I wrote the blocks support for nothing?

http://codereview.chromium.org/7582032/

[Alexei Svitkine (slow), 2011-08-08 21:05:12]

On Mon, Aug 8, 2011 at 4:56 PM, Mark Mentovai <mark@chromium.org> wrote:
> So I wrote the blocks support for nothing?
>
> http://codereview.chromium.org/7582032/
>

Hmm. Didn't Nico also need them for page transitions, or can we get
away without using them there too?

I image we'd want to use blocks for other things in the future, too.

[Nico, 2011-08-08 21:08:59]

LGTM

[Nico, 2011-08-09 03:37:40]

One issue with drawing the background in the browser is that if there's a
vertical scroller and you bounce-scroll out horizontally, that scroller gets
kinda awkwardly clipped. That's not very common in practice, but still a bit
ugly. How annoying would it be to do the bg drawing in the renderer?

[Alexei Svitkine (slow), 2011-08-09 04:58:48]

On Mon, Aug 8, 2011 at 11:37 PM,  <thakis@chromium.org> wrote:
> One issue with drawing the background in the browser is that if there's a
> vertical scroller and you bounce-scroll out horizontally, that scroller gets
> kinda awkwardly clipped. That's not very common in practice, but still a bit
> ugly. How annoying would it be to do the bg drawing in the renderer?

Yes, I've noticed this as well, and mentioned it to Mark.

I was going to take a look at how hard it would be to draw the scroll
bars over the gutter areas in-browser. I honestly am not familiar
enough with WebKit to even know where to start, if I ever to implement
it in the renderer. Still, it might be the right way to go - since it
would allow the browser-side code to just be "dumb" and draw whatever
is given.

-Alexei

[Nico, 2011-08-09 05:34:14]

On Mon, Aug 8, 2011 at 9:58 PM, Alexei Svitkine <asvitkine@chromium.org> wrote:
> On Mon, Aug 8, 2011 at 11:37 PM,  <thakis@chromium.org> wrote:
>> One issue with drawing the background in the browser is that if there's a
>> vertical scroller and you bounce-scroll out horizontally, that scroller gets
>> kinda awkwardly clipped. That's not very common in practice, but still a bit
>> ugly. How annoying would it be to do the bg drawing in the renderer?
>
> Yes, I've noticed this as well, and mentioned it to Mark.
>
> I was going to take a look at how hard it would be to draw the scroll
> bars over the gutter areas in-browser.

That's not worth it, that way lies madness :-O

> I honestly am not familiar
> enough with WebKit to even know where to start, if I ever to implement
> it in the renderer. Still, it might be the right way to go - since it
> would allow the browser-side code to just be "dumb" and draw whatever
> is given.

It would probably be very similar code to what you have, just hooked
up from render_view.cc (all in chromium code, chrome/renderer or
content/renderer, not webkit). The renderer has a CoreGraphics context
and you can create an NSDrawingContext from there and use appkit to
draw etc (you might need to call _linenTexture to warm up the texture
before the sandbox is started).

[Alexei Svitkine (slow), 2011-08-09 13:43:31]

> It would probably be very similar code to what you have, just hooked
> up from render_view.cc (all in chromium code, chrome/renderer or
> content/renderer, not webkit). The renderer has a CoreGraphics context
> and you can create an NSDrawingContext from there and use appkit to
> draw etc (you might need to call _linenTexture to warm up the texture
> before the sandbox is started).

I'll take a look at this today. The trick would be to do it before the
scrollbar painting code runs, but after (or instead of) whatever
paints the gutters white.

-Alexei

[Alexei Svitkine (slow), 2011-08-09 16:56:47]

So it looks like the "correct" place to draw the gutters from the
WebKit-side is in:

src/third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:

ScrollView::paintOverhangAreas()

It doesn't look like we can do anything in our renderer code (i.e.
render_view.cc and friends) to paint the gutters before scrollbars are
painted.

I guess one possibility is to re-architecture the webkit code so that
there's an ScrollOverhandPainter interface, which we could then supply
an implementation of for the Chromium platform in WebKit.

Given that the changes for the above are likely pretty involved, I
think we can land this CL in the mean time (once webkit patch 65707
lands and is rolled).

I'll investigate what we'd need to do in WebKit to have the renderer
paint the gutters, which is probably what we want to do in the long
term.

-Alexei

[Alexei Svitkine (slow), 2011-08-09 17:03:28]

Ah, looks like we can have a Chromium subclass of ScrollView which
overrides paintOverhangAreas().

-Alexei

On Tue, Aug 9, 2011 at 12:56 PM, Alexei Svitkine <asvitkine@chromium.org> wrote:
> So it looks like the "correct" place to draw the gutters from the
> WebKit-side is in:
>
> src/third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:
>
> ScrollView::paintOverhangAreas()
>
> It doesn't look like we can do anything in our renderer code (i.e.
> render_view.cc and friends) to paint the gutters before scrollbars are
> painted.
>
> I guess one possibility is to re-architecture the webkit code so that
> there's an ScrollOverhandPainter interface, which we could then supply
> an implementation of for the Chromium platform in WebKit.
>
> Given that the changes for the above are likely pretty involved, I
> think we can land this CL in the mean time (once webkit patch 65707
> lands and is rolled).
>
> I'll investigate what we'd need to do in WebKit to have the renderer
> paint the gutters, which is probably what we want to do in the long
> term.
>
> -Alexei
>

[Alexei Svitkine (slow), 2011-08-10 18:42:03]

Brett, could I get an OWNERS approval for the renderer changes?

[brettw, 2011-08-11 00:08:27]

LGTM with comment enhancement.

http://codereview.chromium.org/7582009/diff/19009/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/7582009/diff/19009/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host.h:557: //
|last_scroll_offset_|.
We should make it clear that this isn't always available, and do our best to
explain when it is.

It's my understanding that this is only available on Mac. If this is correct, we
should mention that, and also that it's only available for RenderViews and may
be 0 if there hasn't been a paint yet (or something like that).

[commit-bot: I haz the power, 2011-08-11 14:39:21]

Change committed as 96387