|Summary:||adding "base class" GdomDOMObject for GObject bindings|
|Product:||WebKit||Reporter:||Luke Kenneth Casson Leighton <lkcl>|
|Component:||WebCore Misc.||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||abarth, eric, gour, jmalonzo, xan.lopez|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
|Bug Depends on:|
|Bug Blocks:||16401, 27430, 28228|
Description Luke Kenneth Casson Leighton 2009-07-19 14:10:14 PDT
these files are added as part of the #16401 series patch split arranged under an agreement with david. GdomDOMObject is like the glib/gobject equivalent of a c++ base class, for the gobject bindings. it is equivalent to objc's DOMObject and roughly mirrors the same concept in the W3C DOM. the exact objects which derive from it can be seen from examining the Gdom Bindings CodeGenerator but, from memory, it's pretty much everything except Window. this seems to work, and nothing's fallen over or found to be lacking from pretty extensive testing from pyjamas-desktop through pywebkitgtk so... *shrug* - although the exact usage isn't clearly known, the exact usage is clearly understood, empirically, to work.
Comment 1 Luke Kenneth Casson Leighton 2009-07-19 14:23:29 PDT
Created attachment 33055 [details] adds GdomDOMObject the three files added include GdomDOMObject (and its private hidden struct. this will in the future be used to store pointers to exceptions which occur when a c++ DOM object function gets called). review comments ... i do recall discussion about removing this; i do recall that adam managed to remove it entirely. personally, knowing that the work's required in the future, i don't like hacking stuff out that will make life easier in the future. also, this was an area where martin soto helped contribute: the first design involving GdomDOMObject had a private c++ object that had a virtual pointer function mirroring the JSBinding "type()" function, in order to determine the type even when the intermediate object was actually a c-based one. it turns out that glib / gobject already has something that does that sort of thing, so the redundant intermediate class was removed. hurrah. less code - everyone's happy. further modifications in this area it would be a wise move to leave them until after this series is all in, and leave this bugreport open as an indicator that further work is needed in this specific area. in the future.
Comment 2 Luke Kenneth Casson Leighton 2009-07-19 14:25:29 PDT
Created attachment 33056 [details] adds GdomDOMObject nuts. references wrong bug # in ChangeLog. corrected.
Comment 3 Luke Kenneth Casson Leighton 2009-08-03 06:25:22 PDT
Created attachment 33971 [details] correct name and date in ChangeLog
Comment 4 Eric Seidel (no email) 2009-08-06 19:02:25 PDT
Comment on attachment 33971 [details] correct name and date in ChangeLog Looks sane enough.
Comment 5 Adam Barth 2009-08-06 19:46:45 PDT
Comment on attachment 33971 [details] correct name and date in ChangeLog Same question about the license block.
Comment 6 Luke Kenneth Casson Leighton 2009-08-07 02:57:56 PDT
Created attachment 34264 [details] updates copyright date ok... i proobably made modifications in this year, to these files, so, yep, agreed. martin's mods were last year. code was copied last year - so, everyone else's dates are last year (i don't have the right to change them, anyway).
Comment 7 Eric Seidel (no email) 2009-08-07 12:48:53 PDT
Comment on attachment 34264 [details] updates copyright date Still looks fine.
Comment 8 Adam Barth 2009-08-07 16:51:51 PDT
Comment on attachment 34264 [details] updates copyright date Clearing review flag on attachment: 34264 Committing to http://svn.webkit.org/repository/webkit/trunk ... A WebCore/bindings/gdom/GdomDOMObject.cpp M WebKit/gtk/ChangeLog A WebKit/gtk/gdom/GdomDOMObject.h A WebKit/gtk/gdom/GdomDOMObjectPrivate.h Committed r46928 D LayoutTests/editing/input/textarea-arrow-navigation-expected.txt D LayoutTests/editing/input/textarea-arrow-navigation.html M LayoutTests/editing/inserting/insert-thai-characters-001-expected.txt M LayoutTests/editing/inserting/insert-thai-characters-001.html A LayoutTests/editing/selection/5213963-expected.txt M LayoutTests/editing/selection/find-in-text-control.html M LayoutTests/editing/selection/move-begin-end.html M LayoutTests/editing/selection/find-in-text-control-expected.txt M LayoutTests/editing/selection/5213963.html M LayoutTests/editing/selection/move-begin-end-expected.txt M LayoutTests/editing/deleting/delete-ligature-003-expected.txt M LayoutTests/editing/deleting/skip-virama-001-expected.txt M LayoutTests/editing/deleting/delete-ligature-003.html M LayoutTests/editing/deleting/skip-virama-001.html M LayoutTests/editing/deleting/delete-ligature-002-expected.txt M LayoutTests/editing/deleting/delete-ligature-002.html M LayoutTests/editing/text-iterator/thai-cursor-movement-expected.txt M LayoutTests/editing/text-iterator/thai-cursor-movement.html D LayoutTests/platform/mac/editing/selection/5213963-expected.png D LayoutTests/platform/mac/editing/selection/5213963-expected.txt D LayoutTests/platform/mac/editing/selection/5213963-expected.checksum D LayoutTests/platform/qt/editing/selection/5213963-expected.txt D LayoutTests/platform/win/editing/selection/5213963-expected.txt M LayoutTests/ChangeLog A LayoutTests/fast/forms/textarea-arrow-navigation.html A LayoutTests/fast/forms/textarea-arrow-navigation-expected.txt r46926 = ddd875026bd34a49a8a30289f5e7edb004e71222 (trunk) M WebKitTools/ChangeLog M WebKitTools/Scripts/commit-log-editor r46927 = f669ec64c60498cd62c27a7c6c32573b6a12409e (trunk) A WebKit/gtk/gdom/GdomDOMObject.h A WebKit/gtk/gdom/GdomDOMObjectPrivate.h M WebKit/gtk/ChangeLog A WebCore/bindings/gdom/GdomDOMObject.cpp r46928 = 934387916d69db12719cce1724064ffd7f6cdd58 (trunk) First, rewinding head to replay your work on top of it... Nothing to do. http://trac.webkit.org/changeset/46928
Comment 9 Adam Barth 2009-08-07 16:51:54 PDT
All reviewed patches have been landed. Closing bug.
Comment 10 Xan Lopez 2009-08-08 00:26:48 PDT
This is not using the WebKit prefix for the DOM bindings that was agreed in bug #16401, may I know why? It also was committed using ChangeLog entries from a completely different bug.
Comment 11 Luke Kenneth Casson Leighton 2009-08-08 02:32:42 PDT
(In reply to comment #10) > This is not using the WebKit prefix for the DOM bindings that was agreed in bug > #16401, may I know why? yes of course. i've explained a number of times, but am happy to explain again. 1) it's what alp originally chose - i just went along with it when i started this work in august 2008. 2) the work completed to a satisfactory milestone, i left it alone. much discussion ensued, requesting backtracking and removal of critical features that were part of meeting the original milestone. due to the implications of these request [mostly that full testing would not be possible] i declined. 3) adam took over, and complied with the request to remove critical features. 4) this _immediately_ excluded the test platform which had been utilised to perform extensive empirical validation of the work. thus, i was excluded from contributing any code, but i carried on participating in providing insights and advice. 5) a (sensible) decision was made to rename Gdom to Webkit, GDOM to WEBKIT etc. as part of those discussions (as you've observed, and i was part of, and i agree with) 6) adam's version of the patch was completed to a satisfactory milestone for yorba's needs. the discrepancy between this and the milestone reached in 2) was... vast. 6) yorba instructed adam to cease further work. 7) i reached an agreement with david that allowed further work to continue using sensible tools (multiple bugreports). 8) the gap between the code that i understood - and, much more importantly, could _validate_, was so large that i had to make a decision to return to the _original_ patch. 9) i posted a message expanding on 8) to the #16401 bugtracker, and awaited responses. 10) no responses were received. 11) perhaps more importantly, no offers of help were received either. 12) i therefore took the simple pragmatic approach, which was that if people weren't going to pay attention, or offer to help, it was probably best just get on with it. 13) once all the (15, due to be 30) patches are landed, i figured that then would be a good time to do the renaming, as it would then be tracked, managed, discussed; there would be svn diffs, revision history etc. etc. so, the summary is: due to wholly practical considerations, and due to lack of community input, i made a decision to get on with the job. > It also was committed using ChangeLog entries from a > completely different bug. acchh. darnit. i'm maintaining 15 separate patches (in the same svn) at the moment - that's due to go up to 30, today. i'll be writing a script which helps manage this, today, so that ChangeLog entries go at the top of the file etc. sorry. l.
Comment 12 Xan Lopez 2009-08-08 02:45:16 PDT
Landing patches that go against what was decided by consensus is the right thing to do is no way to do things, much less if done in a unilateral fashion. Unless you post a follow-up patch rectifying this I'll revert it later today (I have to go now). I'd also please ask Eric and others not to commit core GTK+ port patches without at least pinging the maintainers of the port before. I know you are trying to help, but I'd rather have you r- the patches if you consider the review queue is out of control and we fail to take action after you have asked us about it repeatedly.
Comment 13 Jan Alonzo 2009-08-08 03:18:04 PDT
(In reply to comment #12) > I'd also please ask Eric and others not to commit core GTK+ port patches > without at least pinging the maintainers of the port before. I know you are > trying to help, but I'd rather have you r- the patches if you consider the > review queue is out of control and we fail to take action after you have asked > us about it repeatedly. I really appreciate what Eric et al have been doing to the review and commit queues as of late. As port maintainers, we have the responsibility to be proactive in providing feedback to patches that may affect our port as well as making sure the WebKit project remains healthy. Ideally, this patches would've been r-'d due to the issues you raised. But if Luke is willing to provide a follow-up patch to fix those issues, I don't see why we can't leave this as it is (assuming that's the only issue you have with the patch).
Comment 14 Luke Kenneth Casson Leighton 2009-08-08 04:05:11 PDT
(In reply to comment #13) > Ideally, this patches would've been r-'d due to the issues you raised. But if > Luke is willing to provide a follow-up patch to fix those issues, yes of course - that was always the plan. ... _after_ the whole series of [what will later today become 30] patches is in. as a separate, distinct, self-contained, managed, easy-to-manage, controlled and reviewed patch. in this way, it's possible to go in incremental verifiable steps. basically, changing Gdom to Webkit etc. is a g/s/r whitespace change. you _never_ mix in whitespace changes in with code changes. so - land gdom first; whitespace next. otherwise: mess, and lost history. remember - the current revision history for the gobject bindings isn't in webkit svn... it's in http://github.com/lkcl/16401.master. l. p.s. of course, anyone _else_ is welcome to: a) take over the maintenance of this work b) provide the whitespace patch now c) provide the whitespace patch later. if nobody is willing to help, i'm choosing c.
Comment 15 Xan Lopez 2009-08-08 10:13:07 PDT
(In reply to comment #14) > (In reply to comment #13) > > > Ideally, this patches would've been r-'d due to the issues you raised. But if > > Luke is willing to provide a follow-up patch to fix those issues, > > yes of course - that was always the plan. > > ... _after_ the whole series of [what will later today become 30] patches is > in. > > as a separate, distinct, self-contained, managed, easy-to-manage, controlled > and reviewed patch. > > in this way, it's possible to go in incremental verifiable steps. > > basically, changing Gdom to Webkit etc. is a g/s/r whitespace change. > > you _never_ mix in whitespace changes in with code changes. This patch is nothing but boilerplate, there is nothing to it but the naming. If you are not even willing to do trivial changes that we already agreed to do in order to get your patches upstream I'm not sure what are we doing here exactly. > > so - land gdom first; whitespace next. otherwise: mess, and lost history. > > remember - the current revision history for the gobject bindings isn't in > webkit svn... it's in http://github.com/lkcl/16401.master. > > l. > > p.s. of course, anyone _else_ is welcome to: > > a) take over the maintenance of this work > b) provide the whitespace patch now > c) provide the whitespace patch later. > > if nobody is willing to help, i'm choosing c. Fair enough, I'll roll it out. A comment about the patch, for the future: - There is no need to add a finalize method if the only thing you are doing is chaining up to your parent class.
Comment 16 Xan Lopez 2009-08-08 10:14:44 PDT
Comment 17 Eric Seidel (no email) 2009-08-08 10:21:32 PDT
Comment on attachment 33971 [details] correct name and date in ChangeLog Clearing review flag.
Comment 18 Gour 2009-08-10 08:07:59 PDT
Hi! lkcl humbly asked if I could let you know this: ""xan, after thinking about this some more - the sheer number of files being submitted due to the split requested by eric - made me think that it would indeed be much more sensible to do the whitespace conversion now rather than later. so - that was successfully completed, yesterday. also, thanks for the thing about the finalize: i did a lot of cut/paste from tutorials and random code-snippets on the internet, to get these bindings working :)" Sincerely, Gour (happy pyjamas user)
Comment 21 Eric Seidel (no email) 2009-08-12 13:45:13 PDT
Comment on attachment 34469 [details] uploaded See my comments in the mail to you.
Comment 22 Mark Rowe (bdash) 2009-08-12 13:45:46 PDT
Comment on attachment 34469 [details] uploaded WEBKIT is not a good prefix for header files like this. The includes don't match our coding style. Is overriding the "finalize" method really necessary here? The overridden version doesn't *do* anything.