Bug 27428 – adding "base class" GdomDOMObject for GObject bindings

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore Misc.

[Luke Kenneth Casson Leighton]

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.

[Luke Kenneth Casson Leighton]

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.

[Luke Kenneth Casson Leighton]

Created attachment 33056 [details]
adds GdomDOMObject

nuts.  references wrong bug # in ChangeLog.  corrected.

[Luke Kenneth Casson Leighton]

Created attachment 33971 [details]
correct name and date in ChangeLog

[Eric Seidel (no email)]

Comment on attachment 33971 [details]
correct name and date in ChangeLog

Looks sane enough.

[Adam Barth]

Comment on attachment 33971 [details]
correct name and date in ChangeLog

Same question about the license block.

[Luke Kenneth Casson Leighton]

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).

[Eric Seidel (no email)]

Comment on attachment 34264 [details]
updates copyright date

Still looks fine.

[Adam Barth]

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

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Xan Lopez]

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.

[Luke Kenneth Casson Leighton]

(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.

[Xan Lopez]

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.

[Jan Alonzo]

(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).

[Luke Kenneth Casson Leighton]

(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.

[Xan Lopez]

(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.

[Xan Lopez]

Reopening.

[Eric Seidel (no email)]

Comment on attachment 33971 [details]
correct name and date in ChangeLog

Clearing review flag.

[Gour]

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)

[Gour]

Created attachment 34462 [details]
uploaded

[Gour]

Created attachment 34469 [details]
uploaded

[Eric Seidel (no email)]

Comment on attachment 34469 [details]
uploaded

See my comments in the mail to you.

[Mark Rowe (bdash)]

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.