Bug 27434 – adding necessary functions and properties to Document IDL gobject bindings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Luke Kenneth Casson Leighton]

this is part of the split of #16401 patch into separate patches
under an agreement suggested by david.

inexplicably, three key features that are available under javascript bindings have been shut out: ability to change a document's domain; ability to set its location (and thus do weird but essential stuff); call document.getSelection().

it should be fairly obvious as to why these would be needed: the fact that they exist in javascript is reason enough alone, to put webkit language bindings "on a par" with javascript and also with its peers.

[Luke Kenneth Casson Leighton]

Created attachment 33063 [details]
adds 3 essential features to gobject document idl

[Eric Seidel (no email)]

These again seem like they shoudl be !objc.

[Mark Rowe (bdash)]

Comment on attachment 33063 [details]
adds 3 essential features to gobject document idl

> Index: WebCore/dom/Document.idl
> ===================================================================
> --- WebCore/dom/Document.idl	(revision 44473)
> +++ WebCore/dom/Document.idl	(working copy)
> @@ -154,7 +154,8 @@
>  
>                   attribute [ConvertNullToNullString] DOMString title;
>          readonly attribute DOMString referrer;
> -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> +     (defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
>                   attribute [ConvertNullToNullString] DOMString domain;
>  #else
>          readonly attribute DOMString domain;

I wonder why the non-JavaScript case is marked as readonly. The HTML 5 spec suggests it should not be.  It may be that we should consider removing the #if's here.

> @@ -180,7 +181,8 @@
>  
>          NodeList getElementsByName(in DOMString elementName);
>  
> -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> +     (defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
>                   attribute [Custom] Location location;
>  #endif

This is flagged as being [Custom] but I don't see a gobject method implementing this property anywhere in the patch.  Did you forget to include it?

> @@ -193,7 +195,8 @@
>          Element            elementFromPoint(in long x, in long y);
>  
>          // Mozilla extensions
> -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> +     (defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
>          DOMSelection       getSelection();
>  #endif

The #if's should probably be removed from this one.  The method was added in <http://trac.webkit.org/changeset/30515>, where the ChangeLog notes that it was restricted to JS as it was only added for web compatibility.  Since then, the getSelection attribute on Document has made its way in to the HTML 5 specification: <http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#documents>.

[Mark Rowe (bdash)]

(In reply to comment #3)
> > @@ -180,7 +181,8 @@
> >  
> >          NodeList getElementsByName(in DOMString elementName);
> >  
> > -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> > +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> > +     (defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
> >                   attribute [Custom] Location location;
> >  #endif
> 
> This is flagged as being [Custom] but I don't see a gobject method implementing
> this property anywhere in the patch.  Did you forget to include it?

Thinking on this further, I suspect Custom may actually mean JSCustom so my question may not have been quite right.  The WebCore::Location object itself does not have any mutable members, while the IDL specifies that the properties are mutable.  The JavaScript bindings have custom logic for Location so that manipulating its properties results in the expected navigations.  Without this custom logic it's not clear to me how the bindings for Location will compile, and thus why it is useful to expose this particular attribute.

[Luke Kenneth Casson Leighton]

(In reply to comment #3)
> (From update of attachment 33063 [details])
> > Index: WebCore/dom/Document.idl
> > ===================================================================
> > --- WebCore/dom/Document.idl	(revision 44473)
> > +++ WebCore/dom/Document.idl	(working copy)
> > @@ -154,7 +154,8 @@
> >  
> >                   attribute [ConvertNullToNullString] DOMString title;
> >          readonly attribute DOMString referrer;
> > -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> > +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> > +     (defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
> >                   attribute [ConvertNullToNullString] DOMString domain;
> >  #else
> >          readonly attribute DOMString domain;
> 
> I wonder why the non-JavaScript case is marked as readonly. The HTML 5 spec
> suggests it should not be.  It may be that we should consider removing the
> #if's here.

 ok - i'm happy to submit a [separate] patch and you can make a decision
 then.

> > @@ -180,7 +181,8 @@
> >  
> >          NodeList getElementsByName(in DOMString elementName);
> >  
> > -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> > +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> > +     (defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
> >                   attribute [Custom] Location location;
> >  #endif
> 
> This is flagged as being [Custom] but I don't see a gobject method implementing
> this property anywhere in the patch.  Did you forget to include it?

 no - i added it on the basis that its inclusion will have no impact on
 the day-to-day compilation of webkit.

 the implementation comes automatically as part of the addition of
 gobject property auto-generation, in CodeGeneratorGObject.pm.

 and as you know, that's being reviewed separately.
 
> > @@ -193,7 +195,8 @@
> >          Element            elementFromPoint(in long x, in long y);
> >  
> >          // Mozilla extensions
> > -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> > +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> > +     (defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
> >          DOMSelection       getSelection();
> >  #endif
> 
> The #if's should probably be removed from this one.  The method was added in
> <http://trac.webkit.org/changeset/30515>, where the ChangeLog notes that it was
> restricted to JS as it was only added for web compatibility.  Since then, the
> getSelection attribute on Document has made its way in to the HTML 5
> specification:

> <http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#documents>.

 ack, ok, likewise, i'll include it in a separate patch, as both 
 DOMString domain and DOMSelection getSelection() changes aren't
 specifically gobject-related.

[Luke Kenneth Casson Leighton]

(In reply to comment #4)
> (In reply to comment #3)
> > > @@ -180,7 +181,8 @@
> > >  
> > >          NodeList getElementsByName(in DOMString elementName);
> > >  
> > > -#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> > > +#if (defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT) || \
> > > +     (defined(LANGUAGE_GOBJECT) && LANGUAGE_GOBJECT)
> > >                   attribute [Custom] Location location;
> > >  #endif
> > 
> > This is flagged as being [Custom] but I don't see a gobject method implementing
> > this property anywhere in the patch.  Did you forget to include it?
> 
> Thinking on this further, I suspect Custom may actually mean JSCustom so my
> question may not have been quite right.  The WebCore::Location object itself
> does not have any mutable members, while the IDL specifies that the properties
> are mutable.  The JavaScript bindings have custom logic for Location so that
> manipulating its properties results in the expected navigations.  Without this
> custom logic it's not clear to me how the bindings for Location will compile,
> and thus why it is useful to expose this particular attribute.

 ohhh, i see.  hah.  right.  that helps to know that there will be extra
 work required to make that happen at some point.

 context: the pyjamas API provides access to location because that's
 what GWT does.  the functions provided _mostly_ comprise "decoding"
 of location (reading from it).

 however, for personal projects that utilise the pyjamas API, i never
 used _setting_ of location, i always utilised Window.open().  that's
 not to say that someone else might try to use them and get unexpected
 results, so i will submit a patch which makes them read-only for now.

 thanks for noting this, mark.

[Luke Kenneth Casson Leighton]

Created attachment 33324 [details]
adds read-only access to Document.location pending writing of custom code to set location properties (at some indeterminate future point)

[Luke Kenneth Casson Leighton]

> Without this
> custom logic it's not clear to me how the bindings for Location will compile,

 they'll compile all right (and do) - it will just be that you set the
 location and nothing happens (because the custom code is missing).

 hmmm.... are we confusing setting of location itself with setting
 of location's properties?  looking at Location.idl, i'd be reluctant
 to submit even an interim patch that switches on read-only on an
 already-overcrowded IDL file just for gobject bindings, and would
 instead place warnings in the pyjamas API [or just say "nuts" to it,
 because XULrunner, as the more mature bindings, i'd expect to work
 fine]

[Mark Rowe (bdash)]

Comment on attachment 33324 [details]
adds read-only access to Document.location pending writing of custom code to set location properties (at some indeterminate future point)

As far as I can see this doesn't really address the issue, which relates to a member of type Location being exposed.  Document::location being read-only does not prevent you from mutating Location::href, which will fail unless your bindings explicitly handle setters for Location's members in a custom fashion (WebCore::Location has no setHref method).  Given that the URL of a document is trivially retrievable from the ports WebView object, I don't see any reason to #if this particular member until the problem here can be addressed in a more robust manner.

[Luke Kenneth Casson Leighton]

(In reply to comment #9)
> (From update of attachment 33324 [details])
> As far as I can see this doesn't really address the issue, which relates to a
> member of type Location being exposed.  Document::location being read-only does
> not prevent you from mutating Location::href, which will fail unless your
> bindings explicitly handle setters for Location's members in a custom fashion
> (WebCore::Location has no setHref method).

 as a first revision, i'd consider that to be acceptable.  anything else
 requires additional work.

>  Given that the URL of a document is
> trivially retrievable from the ports WebView object, I don't see any reason to
> #if this particular member until the problem here can be addressed in a more
> robust manner.

 ok - but that requires development.  i've made it clear that further
 _development_ is not going to happen, not until the existing work is
 accepted.

 there's just far _far_ too much that's outstanding, and to increase that
 burden of work further is simply not acceptable. 

 so, unfortunately, you'll need to accept the reality of that.

 i'll happily document the required work, mark it as "TODO" etc. etc. but there will be _no_ further development until this series of patches which comprise #16401 are landed.

 you _do_ need to accept that, and adjust accordingly.

[Luke Kenneth Casson Leighton]

ok - how about a patch which takes all of the properties in Location.idl
and makes them read-only?

that's simple (and i don't care if it complicates the IDL file). i'm _not_
adding yet more code to this series, there's far too much outstanding as it
is.

[Luke Kenneth Casson Leighton]

Created attachment 33344 [details]
makes Document.location and all Location properties read-only, pending writing of custom code (at some indeterminate future point)

like this?  what do you think?  it's an intermediate "safety" step.

i can't cut access to Location out entirely, because of the "get" code in the pyjamas API:

http://pyjamas.svn.sourceforge.net/viewvc/pyjamas/trunk/library/pyjamas/Location.py?content-type=text%2Fplain

[Luke Kenneth Casson Leighton]

(In reply to comment #5)
>  ack, ok, likewise, i'll include it in a separate patch, as both 
>  DOMString domain and DOMSelection getSelection() changes aren't
>  specifically gobject-related.

done as https://bugs.webkit.org/show_bug.cgi?id=27603

[Mark Rowe (bdash)]

Comment on attachment 33344 [details]
makes Document.location and all Location properties read-only, pending writing of custom code (at some indeterminate future point)

I would prefer that we skip generation of Location and Document::location until they can be supported properly.  As I noted in a previous comment, the WebKit APIs already provide mechanisms for retrieving the URL of a frame so I don't buy the argument that this property *needs* to be exposed *now*.

[Luke Kenneth Casson Leighton]

(In reply to comment #14)
> (From update of attachment 33344 [details])
> I would prefer that we skip generation of Location and Document::location until
> they can be supported properly.  As I noted in a previous comment, the WebKit
> APIs already provide mechanisms for retrieving the URL of a frame so I don't
> buy the argument that this property *needs* to be exposed *now*.

 this is similar to what sam asked, here:
  https://bugs.webkit.org/show_bug.cgi?id=27435#c10

 and, likewise, there are a number of reasons, repeated here.

 out of simplicity, to not massively disrupt the existing test platform - the _only_ comprehensive test platform, pyjamas-desktop, you don't go ripping sections of the available test suite out.

 you also don't go making developers' lives more difficult.  if code has
 to be written that doesn't conform to the API, that's more code to maintain.

 yes, it _needs_ to be exposed _now_ because it's being _used_.  _now_.  and has been for several months.

 this isn't some futuristic API: it's in actual use.

 fortunately, as i mentioned, actual changing of location isn't being used (and hasn't been asked for) as there is Window.open(.....)

 so - sorry, but utilising some quotes "existing" quotes API that isn't actually the DOM isn't acceptable.  forcing people to write code that makes exceptions for webkit, by having a non-standard, non-DOM-based version of the API is _not_ a good enough reason to turn down a review.

 thus, review- is denied, which leaves me free to re-enable it.

[Mark Rowe (bdash)]

(In reply to comment #15)
>  yes, it _needs_ to be exposed _now_ because it's being _used_.  _now_.  and
> has been for several months.
> 
>  this isn't some futuristic API: it's in actual use.

This argument is nonsense.  WebKit does not expose this method so by definition nothing is currently using it.

>  so - sorry, but utilising some quotes "existing" quotes API that isn't
> actually the DOM isn't acceptable.  forcing people to write code that makes
> exceptions for webkit, by having a non-standard, non-DOM-based version of the
> API is _not_ a good enough reason to turn down a review.

An application using WebKit by definition has to make use of WebKit-specific API.

>  thus, review- is denied, which leaves me free to re-enable it.

And yet doing so without addressing the points that reviewers raise serves no purpose other than wasting our time.  We don't like having our time wasted.  Please stop it.

[Luke Kenneth Casson Leighton]

(In reply to comment #16)
> >  thus, review- is denied, which leaves me free to re-enable it.
> 
> And yet doing so without addressing the points that reviewers raise serves no
> purpose other than wasting our time. 

 mark - with respect, in many cases, the complete opposite is the case.  i raise issues, and it is _you_ that do not answer them satisfactorily.  if at all.  in some cases, i have asked _nineteen_ times that you look at issues, and you have failed in your responsibility to do so.

 that having been said, i'm aware that in #16401 there have been issues overlooked, and i apologise for that, but if you look closely, you will see that the sheer _number_ of issues involved means that mistakes are going to be made.

 i _did_ say, _very_ early on, that a single bugreport would be inadviseable to discuss this complex topic, but you and the other reviewers over-ruled me, even to the extent of closing down bugreports that i created to keep track of the issues related to #16401.

 so, i had to wait until it was _david_ who suggested that the bug be split down into separate patches, and it's why i've quoted david's name at the top of each one, so that you and your associates don't go closing them down on a whim.


 so, it would do you quite a lot of credit, as a responsible reviewer, to be patient and accept that on complex issues, the "process" is creaking at the seams, and you have to work round that.



> We don't like having our time wasted. 
> Please stop it.

 neither do i like having my time wasted.
 please stop it.

 is that what you wanted me to say?  do you _want_ these discussions to go on, in this childish - and non-technical way?

 do you _remember_ your agreement in #16401 (around comment number 70), to stick to technical discussions?

 ehn?

 come on, mark.  if the process sucks, don't blame the people that are using it: learn from it, adapt and work with the _people_, not the process.  i'll do the same, ok?

[Luke Kenneth Casson Leighton]

(In reply to comment #16)
> (In reply to comment #15)
> >  yes, it _needs_ to be exposed _now_ because it's being _used_.  _now_.  and
> > has been for several months.
> > 
> >  this isn't some futuristic API: it's in actual use.
> 
> This argument is nonsense.  WebKit does not expose this method so by definition
> nothing is currently using it.

 you've lost me.

> >  so - sorry, but utilising some quotes "existing" quotes API that isn't
> > actually the DOM isn't acceptable.  forcing people to write code that makes
> > exceptions for webkit, by having a non-standard, non-DOM-based version of the
> > API is _not_ a good enough reason to turn down a review.
> 
> An application using WebKit by definition has to make use of WebKit-specific
> API.
> 
 
 sorry, but you've completely lost me.

 i thought we were discussing the definition of the webkit-specific API.

 please correct me if i'm wrong, but it sounds like you're saying that the webkit API is set in stone, and that anyone wishing to make use of webkit must live with that, even if it doesn't do what's actually needed.

 ... but i doubt very much that that's what you're saying.

 please could you clarify?

[Mark Rowe (bdash)]

(In reply to comment #17)
> (In reply to comment #16)
> > >  thus, review- is denied, which leaves me free to re-enable it.
> > 
> > And yet doing so without addressing the points that reviewers raise serves no
> > purpose other than wasting our time. 
> 
>  mark - with respect, in many cases, the complete opposite is the case.  i
> raise issues, and it is _you_ that do not answer them satisfactorily.  if at
> all.  in some cases, i have asked _nineteen_ times that you look at issues, and
> you have failed in your responsibility to do so.

You appear to think that because I disagree with you I must be uninformed.  I've looked at the issues involved and reached a different conclusion, as have the other WebKit reviewers.

>  i _did_ say, _very_ early on, that a single bugreport would be inadviseable to
> discuss this complex topic, but you and the other reviewers over-ruled me, even
> to the extent of closing down bugreports that i created to keep track of the
> issues related to #16401.
> 
>  so, i had to wait until it was _david_ who suggested that the bug be split
> down into separate patches, and it's why i've quoted david's name at the top of
> each one, so that you and your associates don't go closing them down on a whim.

You're conflating the recent filing of bugs (designed to get small patches landed) with your previous attempt to file bugs about things that would remain to be implemented if your original mega-patch were landed.  The former is quite reasonable. The latter simply doesn't make sense when the patch is so far from being landed.

> > We don't like having our time wasted. 
> > Please stop it.
> 
>  neither do i like having my time wasted.
>  please stop it.
> 
>  is that what you wanted me to say?  do you _want_ these discussions to go on,
> in this childish - and non-technical way?

What I would like you to do is quite simple:  stop wasting our time.  You can do this by being more reasonable and respectful in your approach to dealing with the WebKit community.  You can do this by simply going away.

To be quite blunt: I am sick of your incoherent ranting and your self-centered world view.  Even in the rare occasions that you stick to the technical issues it is barely possible to follow what you're saying.  Not because what you're saying is so brilliant that I cannot follow, but because it reads like it was written by a child with an ego the size of a small country.  You ramble and repeat yourself.  You demand special treatment.  You throw veiled insults at the people you're supposedly trying to work with.  You try and pass opinion off as fact.

The reason that no one responds in detail to comments that you make is because it's simply not possible to respond in a sensible way to something that doesn't make sense.  People have suggested many times that you be more concise and stick to the technical issues.  Unless you do that I'm afraid you're not going to make much in the way of progress.

>  do you _remember_ your agreement in #16401 (around comment number 70), to
> stick to technical discussions?

"Stop abusing the patch review process" is a technical issue.  

>  come on, mark.  if the process sucks, don't blame the people that are using
> it: learn from it, adapt and work with the _people_, not the process.  i'll do
> the same, ok?

The process isn't perfect, but you're the only one that has had such huge problems with it.  I'm certain that this says more of you than it does the process.


[ This is a lot more personal than I had originally intended but, frankly, I don't see how else to address the fact that the person is the problem. ]

[Mark Rowe (bdash)]

(In reply to comment #18)
> > >  so - sorry, but utilising some quotes "existing" quotes API that isn't
> > > actually the DOM isn't acceptable.  forcing people to write code that makes
> > > exceptions for webkit, by having a non-standard, non-DOM-based version of the
> > > API is _not_ a good enough reason to turn down a review.
> > 
> > An application using WebKit by definition has to make use of WebKit-specific
> > API.
> > 
> 
>  sorry, but you've completely lost me.
> 
>  i thought we were discussing the definition of the webkit-specific API.
> 
>  please correct me if i'm wrong, but it sounds like you're saying that the
> webkit API is set in stone, and that anyone wishing to make use of webkit must
> live with that, even if it doesn't do what's actually needed.
> 
>  ... but i doubt very much that that's what you're saying.

That would be a stupid thing for me to say.  Thanks for suggesting that it was what I said.

You were arguing that it is vitally important to expose this incomplete DOM binding immediately, even though the same information (the URL of a document) is exposed via the WebKit API in a different manner.

My point is simple: I don't see the value in exposing an incomplete DOM binding for this property since there is an existing means of retrieving this information.  It doesn't seem worth the mess of #if'ing up the IDL files and the confusion that would result from users who expected the exposed properties to behave in the standard manner.

[Luke Kenneth Casson Leighton]

> >  please correct me if i'm wrong, but it sounds like you're saying that the
> > webkit API is set in stone, and that anyone wishing to make use of webkit must
> > live with that, even if it doesn't do what's actually needed.
> > 
> >  ... but i doubt very much that that's what you're saying.
> 
> That would be a stupid thing for me to say.  Thanks for suggesting that it was
> what I said.

 this comes across as a sarcastic comment, which i am sure you did not intend it to be.

 i did no such thing: i respectfully requested clarificaiton, which you've kindly provided, below.  i apologise for giving the wrong impression.
 
> You were arguing that it is vitally important to expose this incomplete DOM
> binding immediately, 

 yes.  the read-only information is vital, and is in real-world usage, today.

 the writing can wait.


> even though the same information (the URL of a document)
> is exposed via the WebKit API in a different manner.

 in a manner which has knock-on complications for upstream development, yes.

 
> My point is simple: I don't see the value in exposing an incomplete DOM binding
> for this property since there is an existing means of retrieving this
> information.  It doesn't seem worth the mess of #if'ing up the IDL files and
> the confusion that would result from users who expected the exposed properties
> to behave in the standard manner.


 ahh, but mark, it was you who turned down the review on the simple
 approach, which was to just have the Document.location property made
 available and be done with it, on the basis that the custom functions
 can be added later.

 so, now i think we've covered the possibilities, we have three approaches to take.  which is it going to be?

 1) make life damn awkward for upstream development

 2) "complicate" the IDL file (but protect users from the consequences)

 3) "clean" IDL, make things easy for upstream developers, with known
   consequences and an upgrade / development path which can be fixed
   at a later date?

1) isn't acceptable.  2) i can live with.  3) i believe is the best, pragmatic workable path.

[Mark Rowe (bdash)]

(In reply to comment #21)
> > even though the same information (the URL of a document)
> > is exposed via the WebKit API in a different manner.
> 
>  in a manner which has knock-on complications for upstream development, yes.

I don't understand what you mean by "upstream development".  I suspect what you mean here is "it would affect my project".  That is indeed unfortunate.

However, I don't think it's a reasonable approach for you to write a patch, start using it in your own project before it has been accepted upstream, and then insist it be accepted in the form you provided because you're using it in that form and you don't want to adapt to changes that are requested.

> > My point is simple: I don't see the value in exposing an incomplete DOM binding
> > for this property since there is an existing means of retrieving this
> > information.  It doesn't seem worth the mess of #if'ing up the IDL files and
> > the confusion that would result from users who expected the exposed properties
> > to behave in the standard manner.
> 
> 
>  ahh, but mark, it was you who turned down the review on the simple
>  approach, which was to just have the Document.location property made
>  available and be done with it, on the basis that the custom functions
>  can be added later.

A simple approach that had similar issues to the current patch:  it exposes what is at best a confusing subset of the bindings.

>  so, now i think we've covered the possibilities, we have three approaches to
> take.  which is it going to be?
> 
>  1) make life damn awkward for upstream development
> 
>  2) "complicate" the IDL file (but protect users from the consequences)
> 
>  3) "clean" IDL, make things easy for upstream developers, with known
>    consequences and an upgrade / development path which can be fixed
>    at a later date?

Since you've injected hyperbole and omitted clarity from this list it's not clear to me what each of the options corresponds to.  I'll restate what I feel the situation is:

The patches so far expose a subset of the Location bindings that allows one to access the location of a Document.  The WebKit APIs expose existing means of accessing the location of a document.  I have not seen a good argument for why adding #if's to the IDL files in order to expose functionality that is already available via other means is a good thing.  Exposing full bindings for Location does seem like a good idea, but you've expressed reluctance to do that at this time.  That seems fine to me, since as I've mentioned the information is already exposed using other API.

[Luke Kenneth Casson Leighton]

(In reply to comment #22)
> (In reply to comment #21)
> > > even though the same information (the URL of a document)
> > > is exposed via the WebKit API in a different manner.
> > 
> >  in a manner which has knock-on complications for upstream development, yes.
> 
> I don't understand what you mean by "upstream development".

 ah, sorry.

>  I suspect what you
> mean here is "it would affect my project".  That is indeed unfortunate.

 it would affect "the" project which i happen to be working on, yes.

 as i explained when martin also used a personal pronoun to refer to some code that i had written, i don't use personal pronouns to indicate claims of ownership over code.  it's a standard rule of software engineering: the usual phrase to refer to it is "egoless programming".

 much appreciated if you could say "the project that you're working on" rather than "your project" etc. etc.

 so, anyway...



  i should explain [in the context of the project that i'm working on, which happens to be the only active major project / test case / use case for these bindings]

 * pyjamas-desktop now has three ports: XULrunner, webkit and MSHTML.

 * all three have python bindings.

 * python-xpcom has read-only access to Document location properties
   [through a weird read-only interface], and the nsIHTMLDOMDocument
   interface appears to be TR-DOM2 only, with the location properties
   themselves [if you follow the weird interface] are read-write:

   http://www.oxymoronical.com/experiments/apidocs/interface/nsIDOMNSDocument
   http://www.oxymoronical.com/experiments/apidocs/platform/1.9.2a1pre/interface/nsIDOMLocation

  through which, it's possible to get and set location, in exactly the way that's intuitively expected.

 * MSHTML's COM interface has read-write access to Document.location:

     http://msdn.microsoft.com/en-us/library/aa752596(VS.85).aspx

     http://msdn.microsoft.com/en-us/library/aa703641(VS.85).aspx

 through which, it's possible to get and set location, in exactly the way that's intuitively expected.

 * webkit's javascript interface has read-write access to Document.location

 * webkit's gobject interface has a bun-fight going on over its format,
   with several options being presented and discussed.  [one is read-only
   identical access to what the other competitor language bindings
   provide]

 * webkit's gtk interface has a non-W3C-DOM-spec-compliant read-write interface

so - with the option that has the least "knock-on complications for upstream development", the pseudo-code would look like this:

def getLocation(document):
    return document.location

and then, to get or set them, you just... do so.

that's pretty simple, huh?

with the method which you're recommending, _all_ upstream implementors [language bindings developers, users of the language bindings, or just people who use gobject c bindings direct] would have to do something like this:

def getLocation(document):
    if systemtype == 'mshtml' or systemtype == 'xulrunner':
        return document.location
    else # heeere we go....
        class LocationWrapperToDealWithStupidAPIDecision:
            def __init__(self, document):
                self.wrapped_document = document
            def get_hash(self):
                return webkit_web_frame_get_location_hash(self.wrapped_document)
            def set_hash(self, value):
                ......
            .....
            .....
            .....

        return LocationWrapperToDealWithStupidAPIDecision(document)


do you see how pointless and ridiculous that is?

do you also appreciate how much work it is?

now multiply it up by the number of developers of language bindings.



> However, I don't think it's a reasonable approach for you to write a patch,
> start using it in your own project

 [correction: using it in a project which is entirely free software,
 on which i happen to have chosen to contribute to]

> before it has been accepted upstream, and
> then insist it be accepted in the form you provided because you're using it in
> that form and you don't want to adapt to changes that are requested.

 ah.  the clue here is "that are requested".  there's an assumption that i agree with the changes requested.  as you've discovered: sometimes i do, sometimes i don't.  if i agree, i'll get on with it immediately, as you've noticed.  if i don't, i'll say so.

what i _won't_ do is simply make changes "that are requested" just _because_ they have been requested.

it's up to you to convince me that the changes requested don't have adverse consequences and/or don't force on people [times however many gobject language bindings autogenerators etc.] an extra workload.  and, as i'm working on the only project which is _actively_ dependent on these bindings, it gives me some active experience and a direct feel for what's needed, which i believe is very relevant.

so.

in this case, the changes "that are requested" have a nasty set of knock-on implications that will burden absolutely everyone using them.


not only that, but i really don't see what the problem is.  if people _need_ the write-capabilities to Location right now, right this second, then, yes, they'll use the [stupidly-named, non-W3C-compliant] webkitgtk+ functions.

if they don't, they can use the temporarily-made-read-only W3C-compliant versions.  or the temporarily-not-working-as-writeable W3C-compliant ones.



> > > My point is simple: I don't see the value in exposing an incomplete DOM binding
> > > for this property since there is an existing means of retrieving this
> > > information.  It doesn't seem worth the mess of #if'ing up the IDL files and
> > > the confusion that would result from users who expected the exposed properties
> > > to behave in the standard manner.
> > 
> > 
> >  ahh, but mark, it was you who turned down the review on the simple
> >  approach, which was to just have the Document.location property made
> >  available and be done with it, on the basis that the custom functions
> >  can be added later.
> 
> A simple approach that had similar issues to the current patch:  it exposes
> what is at best a confusing subset of the bindings.

 well, you can't have it both (all) ways, right now.  which would you prefer - make up your mind! :)



> >  so, now i think we've covered the possibilities, we have three approaches to
> > take.  which is it going to be?
> > 
> >  1) make life damn awkward for upstream development
> > 
> >  2) "complicate" the IDL file (but protect users from the consequences)
> > 
> >  3) "clean" IDL, make things easy for upstream developers, with known
> >    consequences and an upgrade / development path which can be fixed
> >    at a later date?
> 
> Since you've injected hyperbole and omitted clarity from this list

 make up your mind which you would like!  if i provide sufficient detail so as to be clear, you accuse me of wasting time! :)  ok, that's being silly, but it has a grain of truth in it.  

 the above pseudo-code helps clarify what 1) is.  2 lines of code (or not even that) or a ridiculous wrapper class which "normalises" the DOM API, redirecting from what would otherwise be a decent (albeit limited) and above all *standard* API, into some code that was added in a decision by the webkitgtk+ developers to avoid writing the gobject bindings.



> it's not
> clear to me what each of the options corresponds to.  I'll restate what I feel
> the situation is:

 ok.  good idea.

> The patches so far expose a subset of the Location bindings that allows one to
> access the location of a Document. 

 ack.

> The WebKit APIs expose existing means of
> accessing the location of a document. 

 yep.  and changing it, too.

> I have not seen a good argument for why
> adding #if's to the IDL files in order to expose functionality that is already
> available via other means is a good thing.  Exposing full bindings for Location
> does seem like a good idea, but you've expressed reluctance to do that at this
> time.  

 yes.  although, the amount of time spent just discussing this somewhat laboriously does lead me to wonder if i should reconsider that.

but - what's stopping me from doing that is that it will have knock-on implications for the CodeGeneratorGObject code, because anything that's "Custom" has to be special-cased right now [because many of the "Custom" functions and properties are either skipped, or the "Custom" property ignored entirely].

yet more changes and additions to an already complex generator, through non-webkit-standard controlled process, just seems like a truly bad idea, to me.


> That seems fine to me, since as I've mentioned the information is
> already exposed using other API.

 ok - it seems fine to you, because you're not the one who has to write the "wrapper" classes, upstream (in every single programming language which utilises the gobject bindings) to re-normalise the webkit-gobject API to one which matches the W3C specification _just_ for what will turn out to be a relatively small period of time.

 this is an _interim_ measure we're discussing, not a permanent one, after all.

 so.

 implications 1) : webkit is quotes nice and simple quotes, but everybody _else_ has to do a lot of work.

 implications 2) : read-only on Location properties and read-only on Document.location is quotes complicated in the IDL file quotes yet is also simple for upstream developers.  but it has the advantage that in between the time when the gobject patches are being landed and when somebody writes the necessary gobject custom location code, upstream developers are protected from nasty consequences.

 implications 3) : read-write on Location properties is quotes simple for the IDL file quotes and also simple for upstream developers, but has the nasty consequence that if the upstream developers try to set location properties... nothing happens.  fortunately, this will only be for a short period of time (until someone writes the necessary gobject custom location code), but even so...

 1) is definitely not an option i would deem acceptable [and, if there _were_ any other developers other than myself and yorba doing language bindings to gobject...]  2)... *shrug*.  3)... *shrug*.

also, for completeness, there's this:

 4) - i write the gobject custom bindings _before_ landing the gobject patches: nnnnno.  there's been enough svn-history / information been lost as it is, already, through the use of an external non-webkit-based repository (github.org) in the maintenance of the gobject bindings.

 5) other [anyone else got any ideas?]

 so - 2 3 or 5 - you choose, i don't mind which.

[Martin Robinson]

Pretty sure this is completed now. Feel free to re-open if I'm mistaken.