Bug 41926 – [GTK] Misc improvements to the scrolling code

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

[Gustavo Noronha (kov)]

This bug is here to track a number of small patches I am going to propose to improve the scrolling code - removing unnecessary code, making code more robust, and refactoring to enhance reusability.

[Gustavo Noronha (kov)]

Created attachment 61001 [details]
no need to reset adjustment on detaching

A couple more will follow tomorrow.

[Gustavo Noronha (kov)]

Created attachment 61039 [details]
Make ScrollView and Scrollbar use GtkVersioning infrastructure

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/gtk/GtkVersioning.cpp:43:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 6 files


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

[Gustavo Noronha (kov)]

Created attachment 61044 [details]
Use signal connections for disconnecting signals, instead of matching

[Xan Lopez]

Comment on attachment 61039 [details]
Make ScrollView and Scrollbar use GtkVersioning infrastructure

Do you really need to #define the actual name to a private name? Why not simply implement the function when the GTK+ is too old?

[Xan Lopez]

Comment on attachment 61001 [details]
no need to reset adjustment on detaching

What I understand from the comment is that it's important to do this on destroy for some effects to happen, so waiting until something is re-attached would be wrong (?).

[Xan Lopez]

Comment on attachment 61044 [details]
Use signal connections for disconnecting signals, instead of matching

Was this causing any problem?

[Gustavo Noronha (kov)]

Comment on attachment 61001 [details]
no need to reset adjustment on detaching

(In reply to comment #6)
> (From update of attachment 61001 [details])
> What I understand from the comment is that it's important to do this on destroy for some effects to happen, so waiting until something is re-attached would be wrong (?).

Yes, after reading the original commit and testing I think you are totally right.

[Gustavo Noronha (kov)]

(In reply to comment #5)
> (From update of attachment 61039 [details])
> Do you really need to #define the actual name to a private name? Why not simply implement the function when the GTK+ is too old?

That would be more in line with the other stuff we do, indeed, I'll update the patch.

[Gustavo Noronha (kov)]

(In reply to comment #7)
> (From update of attachment 61044 [details])
> Was this causing any problem?

None that I could see, but since we can keep the IDs easily enough, and make the disconnection more explicit, it seems like a good idea to have.

[Gustavo Noronha (kov)]

Created attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

Address comments by Xan and that the first patch is no more.

[Xan Lopez]

Comment on attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

OK.

[Xan Lopez]

Comment on attachment 61044 [details]
Use signal connections for disconnecting signals, instead of matching

Seeing that this is more code and we don't really need the extra precision it brings I don't really agree with the change to be honest. So marking r- for now, you can try harder to convince me!

[Adam Barth]

Comment on attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

Shall we land this patch?

[Gustavo Noronha (kov)]

Comment on attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

I forgot about it, thanks Adam!

[WebKit Commit Bot]

Comment on attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

Clearing flags on attachment: 61223

Committed r65155: <http://trac.webkit.org/changeset/65155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.