Bug 41926 - [GTK] Misc improvements to the scrolling code
Summary: [GTK] Misc improvements to the scrolling code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-07-08 19:24 PDT by Gustavo Noronha (kov)
Modified: 2010-08-11 07:44 PDT (History)
3 users (show)

See Also:


Attachments
no need to reset adjustment on detaching (1.99 KB, patch)
2010-07-08 20:37 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Make ScrollView and Scrollbar use GtkVersioning infrastructure (8.00 KB, patch)
2010-07-09 06:26 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Use signal connections for disconnecting signals, instead of matching (5.21 KB, patch)
2010-07-09 07:31 PDT, Gustavo Noronha (kov)
xan.lopez: review-
Details | Formatted Diff | Diff
make ScrollView and Scrollbar GtkVersioning users (8.82 KB, patch)
2010-07-12 06:46 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gustavo Noronha (kov) 2010-07-08 19:24:58 PDT
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.
Comment 1 Gustavo Noronha (kov) 2010-07-08 20:37:59 PDT
Created attachment 61001 [details]
no need to reset adjustment on detaching

A couple more will follow tomorrow.
Comment 2 Gustavo Noronha (kov) 2010-07-09 06:26:44 PDT
Created attachment 61039 [details]
Make ScrollView and Scrollbar use GtkVersioning infrastructure
Comment 3 WebKit Review Bot 2010-07-09 06:27:57 PDT
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.
Comment 4 Gustavo Noronha (kov) 2010-07-09 07:31:18 PDT
Created attachment 61044 [details]
Use signal connections for disconnecting signals, instead of matching
Comment 5 Xan Lopez 2010-07-11 03:13:34 PDT
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?
Comment 6 Xan Lopez 2010-07-11 03:15:02 PDT
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 (?).
Comment 7 Xan Lopez 2010-07-11 03:15:44 PDT
Comment on attachment 61044 [details]
Use signal connections for disconnecting signals, instead of matching

Was this causing any problem?
Comment 8 Gustavo Noronha (kov) 2010-07-12 06:11:53 PDT
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.
Comment 9 Gustavo Noronha (kov) 2010-07-12 06:13:07 PDT
(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.
Comment 10 Gustavo Noronha (kov) 2010-07-12 06:16:10 PDT
(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.
Comment 11 Gustavo Noronha (kov) 2010-07-12 06:46:36 PDT
Created attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

Address comments by Xan and that the first patch is no more.
Comment 12 Xan Lopez 2010-07-12 06:57:20 PDT
Comment on attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

OK.
Comment 13 Xan Lopez 2010-08-02 14:14:37 PDT
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!
Comment 14 Adam Barth 2010-08-10 22:56:58 PDT
Comment on attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

Shall we land this patch?
Comment 15 Gustavo Noronha (kov) 2010-08-11 05:57:33 PDT
Comment on attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

I forgot about it, thanks Adam!
Comment 16 WebKit Commit Bot 2010-08-11 07:44:06 PDT
Comment on attachment 61223 [details]
make ScrollView and Scrollbar GtkVersioning users

Clearing flags on attachment: 61223

Committed r65155: <http://trac.webkit.org/changeset/65155>
Comment 17 WebKit Commit Bot 2010-08-11 07:44:10 PDT
All reviewed patches have been landed.  Closing bug.