[chromium/mac] Respect system setting for rubber-banding.
Created attachment 159156 [details] Patch
Corresponds to http://crbug.com/133131
Nico: Please review.
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.
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
(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?
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?
(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?
(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.
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?
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.)
> With that, LGTM. (I'm not a review though.) *not a reviewer. Though I'm not a review either.
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?
(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"];
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)
(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.
Comment on attachment 159731 [details] Patch lgtm
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.
Comment on attachment 159739 [details] Patch R=me
Comment on attachment 159739 [details] Patch Clearing flags on attachment: 159739 Committed r126321: <http://trac.webkit.org/changeset/126321>
All reviewed patches have been landed. Closing bug.