Bug 94356 – Respect system setting for rubber-banding in ScrollAnimatorMac.

• Component: New Bugs
• Status: RESOLVED FIXED

[chromium/mac] Respect system setting for rubber-banding.

Created attachment 159156 [details]
Patch

Corresponds to http://crbug.com/133131

Nico: Please review.

[Nico Weber]

Comment on attachment 159156 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159156&action=review

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:661
> +#else

Does the apple port get this for free somehow?

(In reply to comment #4)
> (From update of attachment 159156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159156&action=review
> 
> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:661
> > +#else
> 
> Does the apple port get this for free somehow?

No, they don't respect the setting, as far as I could tell.

[Nico Weber]

Hi Beth,

it looks like the rest of OS X honors this setting. Is the apple port interested in honoring it too? If so, we can remove the ifdef.

Nico

[Beth Dakin]

(In reply to comment #6)
> Hi Beth,
> 
> it looks like the rest of OS X honors this setting. Is the apple port interested in honoring it too? If so, we can remove the ifdef.
> 
> Nico

Hmm, maybe! Sam, what do you think?

[Sam Weinig]

Seems reasonable to support.  If you are ever considering making something like this Chromium only in WebCore, don't!  

Please add a test.

(In reply to comment #8)
> Please add a test.

Do you have a suggestion of how to go about writing a test for this?

This relies on both system settings and platform-specific wheel events and timings. I'm not sure what the convention is for testing such changes. Can you point me in the right direction?

[Sam Weinig]

(In reply to comment #9)
> (In reply to comment #8)
> > Please add a test.
> 
> Do you have a suggestion of how to go about writing a test for this?
> 
> This relies on both system settings and platform-specific wheel events and timings. I'm not sure what the convention is for testing such changes. Can you point me in the right direction?

Beth, what is the current story with rubber-band testing?  Is it non-existent?

[Beth Dakin]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Please add a test.
> > 
> > Do you have a suggestion of how to go about writing a test for this?
> > 
> > This relies on both system settings and platform-specific wheel events and timings. I'm not sure what the convention is for testing such changes. Can you point me in the right direction?
> 
> Beth, what is the current story with rubber-band testing?  Is it non-existent?

Yeah, I don't think we ever devised a way to test this.

Created attachment 159478 [details]
Patch

(In reply to comment #12)
> Created an attachment (id=159478) [details]
> Patch

Updated patch removes the CHROMIUM ifdefs so that the behavior applies to the Mac port too.

[Nico Weber]

Comment on attachment 159478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159478&action=review

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:657
> +        initialized = true;

NSUserDefaults usually are supposed to be "live", you shouldn't have to restart your browser after you changed them.

Is this method hot? If so, maybe you could give WebScrollAnimationHelperDelegate a +initialize method and call registerDefaults: from there?

Comment on attachment 159478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159478&action=review

>> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:657
>> +        initialized = true;
> 
> NSUserDefaults usually are supposed to be "live", you shouldn't have to restart your browser after you changed them.
> 
> Is this method hot? If so, maybe you could give WebScrollAnimationHelperDelegate a +initialize method and call registerDefaults: from there?

For this particular setting, other applications require a restart for the setting to take effect.

Regarding whether the method is hot: it's invoked for every mouse wheel event - so perhaps warm, but not hot?

[Nico Weber]

Comment on attachment 159478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159478&action=review

>>> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:657
>>> +        initialized = true;
>> 
>> NSUserDefaults usually are supposed to be "live", you shouldn't have to restart your browser after you changed them.
>> 
>> Is this method hot? If so, maybe you could give WebScrollAnimationHelperDelegate a +initialize method and call registerDefaults: from there?
> 
> For this particular setting, other applications require a restart for the setting to take effect.
> 
> Regarding whether the method is hot: it's invoked for every mouse wheel event - so perhaps warm, but not hot?

If it's consistent with other apps, then I guess it's fine. "// Other apps like XXX require a restart after changing this default." makes a better comment than "// If the key hasn't been set, default to enabling rubber banding." which doesn't add much.

With that, LGTM. (I'm not a review though.)

[Nico Weber]

> With that, LGTM. (I'm not a review though.)

*not a reviewer. Though I'm not a review either.

[Beth Dakin]

Comment on attachment 159478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159478&action=review

I think this patch is close, but I would like to see the things I commented on cleaned up.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:651
> +        NSString* key = @"NSScrollViewRubberbanding";

For Objective-C objects, our style guide dictates that the * should go next to the variable name, not the type name. In other words, this should be: NSString *key…

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:652
> +        NSUserDefaults* defaults = [NSUserDefaults standardUserDefaults];

Same as above.

> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:654
> +        id value = [defaults objectForKey:key];

Can you just use boolForKey here instead of objectForKey? And then avoid all of the conversions below?

[Beth Dakin]

(In reply to comment #18)
> (From update of attachment 159478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159478&action=review
> 
> I think this patch is close, but I would like to see the things I commented on cleaned up.
> 
> > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:651
> > +        NSString* key = @"NSScrollViewRubberbanding";
> 

Actually, there is no need to have a variable for the key here anyway. You can collapse these three lines of code into:

enabled = [[NSUserDefaults standardUserDefaults] boolForKey:@"NSScrollViewRubberbanding"];

[Nico Weber]

Comment on attachment 159478 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159478&action=review

>> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:654
>> +        id value = [defaults objectForKey:key];
> 
> Can you just use boolForKey here instead of objectForKey? And then avoid all of the conversions below?

boolForKey defaults to NO for values that are not present. This wants to default to YES instead (see my suggestion about registerDefaults: above)

[Beth Dakin]

(In reply to comment #20)
> (From update of attachment 159478 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159478&action=review
> 
> >> Source/WebCore/platform/mac/ScrollAnimatorMac.mm:654
> >> +        id value = [defaults objectForKey:key];
> > 
> > Can you just use boolForKey here instead of objectForKey? And then avoid all of the conversions below?
> 
> boolForKey defaults to NO for values that are not present. This wants to default to YES instead (see my suggestion about registerDefaults: above)

Oh, that makes sense.

Created attachment 159731 [details]
Patch

(In reply to comment #22)
> Created an attachment (id=159731) [details]
> Patch

I've updated the patch to add a comment per Nico's suggestion and to inline some things per Beth's suggestion. I've kept (and clarified) the comment about the default value and why the code doesn't use -boolForKey:.

Please take another look.

[Nico Weber]

Comment on attachment 159731 [details]
Patch

lgtm

[Nico Weber]

Comment on attachment 159731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159731&action=review

> Source/WebCore/ChangeLog:3
> +        [chromium/mac] Respect system setting for rubber-banding.

(nit: Remove [chromium/mac] here)

Created attachment 159739 [details]
Patch

(In reply to comment #25)
> (From update of attachment 159731 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159731&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [chromium/mac] Respect system setting for rubber-banding.
> 
> (nit: Remove [chromium/mac] here)

Done.

[James Robinson]

Comment on attachment 159739 [details]
Patch

R=me

[WebKit Review Bot]

Comment on attachment 159739 [details]
Patch

Clearing flags on attachment: 159739

Committed r126321: <http://trac.webkit.org/changeset/126321>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.