Bug 156844 - Remove UsePointersEvenForNonNullableObjectArguments keyword
Summary: Remove UsePointersEvenForNonNullableObjectArguments keyword
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on: 156897 156898 156899 156901 156903 156904 156905 156908 156909 156977 156978 156979
Blocks:
  Show dependency treegraph
 
Reported: 2016-04-21 03:40 PDT by youenn fablet
Modified: 2016-04-29 01:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (51.83 KB, patch)
2016-04-21 03:46 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (2.12 KB, patch)
2016-04-28 23:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-04-21 03:40:14 PDT
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.
Comment 1 youenn fablet 2016-04-21 03:46:34 PDT
Created attachment 276905 [details]
Patch
Comment 2 Darin Adler 2016-04-21 09:00:52 PDT
I don’t agree with this strategy. Instead I suggest we mark arguments as nullable and then remove the nullability over time.
Comment 3 Darin Adler 2016-04-21 09:02:01 PDT
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.
Comment 4 Chris Dumez 2016-04-21 09:03:21 PDT
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.
Comment 5 youenn fablet 2016-04-21 10:18:58 PDT
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?
Comment 6 Chris Dumez 2016-04-21 10:50:17 PDT
(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 :/
Comment 7 Chris Dumez 2016-04-21 10:54:55 PDT
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.
Comment 8 Chris Dumez 2016-04-21 10:56:46 PDT
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.
Comment 9 Chris Dumez 2016-04-21 11:05:27 PDT
(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.
Comment 10 Darin Adler 2016-04-21 13:37:00 PDT
Yes, lets keep doing this one file at a time!
Comment 11 Chris Dumez 2016-04-21 14:00:32 PDT
(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.
Comment 12 youenn fablet 2016-04-28 23:30:08 PDT
Created attachment 277675 [details]
Patch
Comment 13 WebKit Commit Bot 2016-04-29 01:28:43 PDT
Comment on attachment 277675 [details]
Patch

Clearing flags on attachment: 277675

Committed r200236: <http://trac.webkit.org/changeset/200236>
Comment 14 WebKit Commit Bot 2016-04-29 01:28:49 PDT
All reviewed patches have been landed.  Closing bug.