Bug 156844 – Remove UsePointersEvenForNonNullableObjectArguments keyword

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[youenn fablet]

As a step towards removing all uses of UsePointersEvenForNonNullableObjectArguments, it might be good to remove all uses of it as an interface keyword.
Instead, it might be used for methods, which should get fixed progressively.

[youenn fablet]

Created attachment 276905 [details]
Patch

[Darin Adler]

I don’t agree with this strategy. Instead I suggest we mark arguments as nullable and then remove the nullability over time.

[Darin Adler]

Basically, I don’t think we need two different forms of nullable, one that is "nullable on purpose" and another that is "nullable but should be fixed". To me that seems over-engineered.

[Chris Dumez]

I would also prefer to use regular nullable (?) with a FIXME comment rather than yet another WebKit-specific IDL attribute. I am looking forward to getting rid of [UsePointersEvenForNonNullableObjectArguments] entirely as soon as possible.

[youenn fablet]

OK, I'll update the patch to remove this keyword using the FIXME approach.

ddkilzer, is it ok to remove support for this keyword in the binding generator?

[Chris Dumez]

(In reply to comment #5)
> OK, I'll update the patch to remove this keyword using the FIXME approach.
> 
> ddkilzer, is it ok to remove support for this keyword in the binding
> generator?

No, not yet. Someone would need to clean up non-OpenSource ones first :/

[Chris Dumez]

Can we do it IDL file by IDL file (like we've been doing so far) rather than in one huge patch please? It is easier to review carefully and make sure the nullable is actually needed and the FIXME comments are accurate.

[Chris Dumez]

For e.g., ideally I would not want to add a nullable in the IDL if the implementation does a null-check and throws when getting a null pointer.

Also FIXME comments need to be added only when the added nullable in the IDL does not match the corresponding spec.

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #5)
> > OK, I'll update the patch to remove this keyword using the FIXME approach.
> > 
> > ddkilzer, is it ok to remove support for this keyword in the binding
> > generator?
> 
> No, not yet. Someone would need to clean up non-OpenSource ones first :/

I'll take care of this.

[Darin Adler]

Yes, lets keep doing this one file at a time!

[Chris Dumez]

(In reply to comment #6)
> (In reply to comment #5)
> > OK, I'll update the patch to remove this keyword using the FIXME approach.
> > 
> > ddkilzer, is it ok to remove support for this keyword in the binding
> > generator?
> 
> No, not yet. Someone would need to clean up non-OpenSource ones first :/

Ok, it should now be safe once we get rid of all the uses in trunk.

[youenn fablet]

Created attachment 277675 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 277675 [details]
Patch

Clearing flags on attachment: 277675

Committed r200236: <http://trac.webkit.org/changeset/200236>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.