Bug 133726 - [GTK] Limit the amount of API exposed to GObject DOM bindings API
Summary: [GTK] Limit the amount of API exposed to GObject DOM bindings API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 133728
Blocks: 133724
  Show dependency treegraph
 
Reported: 2014-06-11 01:04 PDT by Carlos Garcia Campos
Modified: 2014-06-19 23:24 PDT (History)
14 users (show)

See Also:


Attachments
Patch (247.19 KB, patch)
2014-06-13 10:57 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch to fix binding tests (331.37 KB, patch)
2014-06-16 03:44 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Fix typo in ChangeLog (331.40 KB, patch)
2014-06-17 06:09 PDT, Carlos Garcia Campos
gustavo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2014-06-11 01:04:12 PDT
We currently expose every single idl file added to the source tree. This is very problematic because we keep backwards API/ABI compatibility and unstable DOM API is very likely to change. Then we end up adapting our API to not break it for things nobody has ever used.
Comment 1 Carlos Garcia Campos 2014-06-11 03:01:11 PDT
An alternative would be to split our current API in stable/unstable, which I think it's similar to what apple does (they call it public/private, though). We have a very limited API declared stable and included by the main header webkitdom.h. This would be API/ABI stable . The rest of the API is considered unstable, and it can change. We only need to care about the ABI in this case I guess, since it's the same library in the end. The unstable API wouldn't be included by the main header, so that people have to explicitly include every header to use it. We could also force people to define a typical macro THIS_IS_UNSTABLE_AND_I_KNOW_WHAT_I_AM_DOING.
Comment 2 Carlos Garcia Campos 2014-06-11 04:25:52 PDT
hmm, we would still need a way to split new unstable api added to stable classes. I guess the code generator could use the symbols file to know which symbols are stable. We don't have signals and for properties we always generate at least a getter, so it will be a new symbols in the symbols file. New symbols that are not in the stable symbol list would be added to a FooUnstable.h header, for example. The problem is that the property would be added to the class in any case, although changing a property doesn't actually break the API/ABI (you will get a runtime warning)
Comment 3 Martin Robinson 2014-06-11 06:54:09 PDT
(In reply to comment #2)
> hmm, we would still need a way to split new unstable api added to stable classes. I guess the code generator could use the symbols file to know which symbols are stable. We don't have signals and for properties we always generate at least a getter, so it will be a new symbols in the symbols file. New symbols that are not in the stable symbol list would be added to a FooUnstable.h header, for example. The problem is that the property would be added to the class in any case, although changing a property doesn't actually break the API/ABI (you will get a runtime warning)

We could, although it would be super ugly, call those properties something like whatever-unstable.
Comment 4 Carlos Garcia Campos 2014-06-11 07:08:49 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > hmm, we would still need a way to split new unstable api added to stable classes. I guess the code generator could use the symbols file to know which symbols are stable. We don't have signals and for properties we always generate at least a getter, so it will be a new symbols in the symbols file. New symbols that are not in the stable symbol list would be added to a FooUnstable.h header, for example. The problem is that the property would be added to the class in any case, although changing a property doesn't actually break the API/ABI (you will get a runtime warning)
> 
> We could, although it would be super ugly, call those properties something like whatever-unstable.

But when we make those stable, the name will change so and people will have runtime warnings in any case.
Comment 5 Martin Robinson 2014-06-11 07:23:01 PDT
(In reply to comment #4)

> But when we make those stable, the name will change so and people will have runtime warnings in any case.

Ah, I see the issue is that the properties cannot be hidden with an #ifdef.
Comment 6 Carlos Garcia Campos 2014-06-11 07:29:48 PDT
(In reply to comment #5)
> (In reply to comment #4)
> 
> > But when we make those stable, the name will change so and people will have runtime warnings in any case.
> 
> Ah, I see the issue is that the properties cannot be hidden with an #ifdef.

hmm, well, if we have the THIS_IS_UNSTABLE_AND_I_KNOW_WHAT_I_AM_DOING define, we could indeed hide the property.
Comment 7 Martin Robinson 2014-06-11 07:34:15 PDT
(In reply to comment #6)

> hmm, well, if we have the THIS_IS_UNSTABLE_AND_I_KNOW_WHAT_I_AM_DOING define, we could indeed hide the property.

Properties are added at runtime and not defined in the headers though, right?
Comment 8 Carlos Garcia Campos 2014-06-11 07:50:55 PDT
(In reply to comment #7)
> (In reply to comment #6)
> 
> > hmm, well, if we have the THIS_IS_UNSTABLE_AND_I_KNOW_WHAT_I_AM_DOING define, we could indeed hide the property.
> 
> Properties are added at runtime and not defined in the headers though, right?

hmm, yes you are right, we can't add ifdefs in the cpp file for the users.
So, I think we could keep the property in the class, but with the getter/setter in the FooUnstable.h header.
Comment 9 Carlos Garcia Campos 2014-06-13 10:57:56 PDT
Created attachment 233059 [details]
Patch
Comment 10 Carlos Garcia Campos 2014-06-13 10:59:42 PDT
This patch will need a clean build too, so I guess it will fail in EWS. We need to check the DOM bindings cmake generation rules.
Comment 11 Carlos Garcia Campos 2014-06-16 03:44:16 PDT
Created attachment 233154 [details]
Updated patch to fix binding tests

Updated patch to include new results for headers in binding tests, since the test cases are not handled as unstable API by the code generator.
Comment 12 Gustavo Noronha (kov) 2014-06-16 09:09:44 PDT
Comment on attachment 233154 [details]
Updated patch to fix binding tests

View in context: https://bugs.webkit.org/attachment.cgi?id=233154&action=review

Looks great to me, though I haven't read the build changes in detail.

> Source/WebCore/ChangeLog:9
> +        unstable API is not included in the main webkitfom.h file, so that

s/fom/dom/
Comment 13 Carlos Garcia Campos 2014-06-17 06:09:28 PDT
Created attachment 233227 [details]
Fix typo in ChangeLog
Comment 14 Gustavo Noronha (kov) 2014-06-19 13:29:37 PDT
Comment on attachment 233227 [details]
Fix typo in ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=233227&action=review

LGTM!

> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:980
>      # Insert introspection annotations
> -    push(@hBody, "/**\n");
> -    push(@hBody, " * ${functionName}:\n");
> -    push(@hBody, " * \@self: A #${className}\n");
> +    push(@functionHeader, "/**");
> +    push(@functionHeader, " * ${functionName}:");
> +    push(@functionHeader, " * \@self: A #${className}");

You don't need to provide the newlines anymore?
Comment 15 Carlos Garcia Campos 2014-06-19 22:47:03 PDT
(In reply to comment #14)
> (From update of attachment 233227 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233227&action=review
> 
> LGTM!

Thanks for the review!

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:980
> >      # Insert introspection annotations
> > -    push(@hBody, "/**\n");
> > -    push(@hBody, " * ${functionName}:\n");
> > -    push(@hBody, " * \@self: A #${className}\n");
> > +    push(@functionHeader, "/**");
> > +    push(@functionHeader, " * ${functionName}:");
> > +    push(@functionHeader, " * \@self: A #${className}");
> 
> You don't need to provide the newlines anymore?

No, because I'm doing join("\n", @functionHeader) below
Comment 16 Carlos Garcia Campos 2014-06-19 23:24:45 PDT
Committed r170173: <http://trac.webkit.org/changeset/170173>