Bug 191067 – [GTK] Scrollbars not following gtk-primary-button-warps-slider setting

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Jim Mason]

When you click in the scrollbar trough, WebKit GTK will always warp the slider to the clicked position, regardless of the setting 'gtk-primary-button-warps-slider'.

Expected behaviour:  If 'gtk-primary-button-warps-slider' is false, then a click in the trough should page forward or backward; if true, then it should warp.  Holding down the shift key or clicking the middle button reverses the sense of the setting.

Reference: https://developer.gnome.org/gtk3/stable/GtkSettings.html#GtkSettings--gtk-primary-button-warps-slider

[Jim Mason]

Created attachment 353366 [details]
Patch

[EWS Watchlist]

Attachment 353366 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:780:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:781:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:781:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:783:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:784:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 5 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jim Mason]

Created attachment 353367 [details]
Patch

[EWS Watchlist]

Attachment 353367 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:780:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:784:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jim Mason]

Created attachment 353369 [details]
Patch

[Michael Catanzaro]

Did you close this accidentally?

[Michael Catanzaro]

Comment on attachment 353369 [details]
Patch

Oooh, cool! Nice, especially if this is your first contribution.

I have two suggestions:

 (1) WebKit style is that the comment should be a complete sentence that starts with a capital and ends with a period.

 (2) The ^ is too clever for my taste; can you write it out with conditionals so that it's easier to read?

Thanks!

[Jim Mason]

Created attachment 353387 [details]
Patch

[Jim Mason]

(In reply to Michael Catanzaro from comment #7)
> Comment on attachment 353369 [details]
> Patch
> 
> Oooh, cool! Nice, especially if this is your first contribution.
> 
> I have two suggestions:
> 
>  (1) WebKit style is that the comment should be a complete sentence that
> starts with a capital and ends with a period.
> 
>  (2) The ^ is too clever for my taste; can you write it out with
> conditionals so that it's easier to read?
> 
> Thanks!

Good feedback, thanks!  The changes have been applied.

[Michael Catanzaro]

Comment on attachment 353387 [details]
Patch

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

> Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:785
> +        return warp_slider?

OK one last change: leave one space before the ?

warp_slider ?

Sorry. :P

[Jim Mason]

Created attachment 353389 [details]
Patch

[Jim Mason]

(In reply to Michael Catanzaro from comment #10)
> Comment on attachment 353387 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353387&action=review
> 
> > Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:785
> > +        return warp_slider?
> 
> OK one last change: leave one space before the ?
> 
> warp_slider ?
> 
> Sorry. :P

No prob, 'tis done.

[WebKit Commit Bot]

Comment on attachment 353389 [details]
Patch

Clearing flags on attachment: 353389

Committed r237617: <https://trac.webkit.org/changeset/237617>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #10)
> Comment on attachment 353387 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353387&action=review
> 
> > Source/WebCore/platform/gtk/ScrollbarThemeGtk.cpp:785
> > +        return warp_slider?
> 
> OK one last change: leave one space before the ?
> 
> warp_slider ?
> 
> Sorry. :P

warpSlider...

[Michael Catanzaro]

Sorry, I missed that. Will you rename it, or do you want me to?

[Jim Mason]

Reopening to attach new patch.

[Jim Mason]

Created attachment 355518 [details]
Patch

[EWS Watchlist]

Attachment 355518 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:10:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jim Mason]

Created attachment 355519 [details]
Patch

[Michael Catanzaro]

Comment on attachment 355519 [details]
Patch

OK, thanks.

[WebKit Commit Bot]

Comment on attachment 355519 [details]
Patch

Clearing flags on attachment: 355519

Committed r238462: <https://trac.webkit.org/changeset/238462>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.