Bug 168899 – Re-add deprecated functions to WKPageGroup.h

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Alexey Proskuryakov]

Some functions were removed from SPI in r207149, but we need the declarations in headers so that they would match exports.

[Alexey Proskuryakov]

Created attachment 302814 [details]
proposed patch

Isn’t there a sharper instrument that can be used when a symbol needs to be visible solely for binary compatibility or for use by another target in the same project? Something like declaring it in an “extra private header” that is really just an project header?

[Alexey Proskuryakov]

There is one, but I just got an r- today for using a nearly identical instrument.

-extra-private-header is an out-of-band signal, most contributors won't know about it, it doesn't follow the code across renames and refactoring, etc.

Is there no mechanism that allows an entire directory to be designated? If you specify an -extra-private-header, is that header allowed include other headers that aren’t private and aren’t themselves designated as -extra-private-header?

I see the similarities to bug 168878, but I also see differences.

If we must declare these (and a few more) in a framework header, the rather than add a comment, can we annotate them with __attribute__((unavailable)) or similar?

[Alexey Proskuryakov]

> Is there no mechanism that allows an entire directory to be designated?

A directory can be provided, yes.

> If we must declare these (and a few more) in a framework header, the rather than add a comment, can we annotate them with __attribute__((unavailable)) or similar?

I'm not sure. I don't see __attribute__((unavailable)) anywhere in WebKit2, and I don't know how it would interact with InstallAPI.

Not sure if this is worth creating a whole new mechanism for at this time. We've been living with "deprecated" comments for years, with no problems that I'm aware of.

(In reply to comment #5)
> > Is there no mechanism that allows an entire directory to be designated?
> 
> A directory can be provided, yes.

That seems like a good path going forward. The declarations in this patch used to be in an SPI header, so I see how it’s tempting to just put them back. But other internal symbols which need to be visible were never declared in a framework header, and unnecessarily publishing internal details in framework headers has no precedent.

> > If we must declare these (and a few more) in a framework header, the rather than add a comment, can we annotate them with __attribute__((unavailable)) or similar?
> 
> I'm not sure. I don't see __attribute__((unavailable)) anywhere in WebKit2,
> and I don't know how it would interact with InstallAPI.

We use the NS_UNAVAILABLE macro, which expands to that attribute, throughout the project. Can you check the InstallAPI angle?

Since we’re talking about declarations that were either deprecated or never intended to be adopted, it would be good to make it practically impossible for clients to use. A comment does not accomplish that as well as a compiler-readable annotation.

> Not sure if this is worth creating a whole new mechanism for at this time.
> We've been living with "deprecated" comments for years, with no problems
> that I'm aware of.

The mechanism for annotating declarations as deprecated or unavailable are widely deployed in the Modern WebKit API (private declarations included). But it still seems best to never introduce those declarations in the first place.

[Alexey Proskuryakov]

NS_UNAVAILABLE is indeed used for three Objective-C methods in WebKit2. But I doubt that it can be used in this cross-platform C header.

I can't verify what happens with __attribute__((unavailable)) until all other errors are resolved, as the tbd file can't be generated because of those.

> A comment does not accomplish that as well as a compiler-readable annotation.

I think that in practice, it did accomplish that.

[Tim Horton]

(In reply to comment #7)
> NS_UNAVAILABLE is indeed used for three Objective-C methods in WebKit2. But
> I doubt that it can be used in this cross-platform C header.
> 
> I can't verify what happens with __attribute__((unavailable)) until all
> other errors are resolved, as the tbd file can't be generated because of
> those.
> 
> > A comment does not accomplish that as well as a compiler-readable annotation.
> 
> I think that in practice, it did accomplish that.

NS_UNAVAILABLE is just __attribute__((unavailable)). Use it! It must work with InstallAPI...

[Alexey Proskuryakov]

Created attachment 303010 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 303010 [details]
patch for landing

Clearing flags on attachment: 303010

Committed r213198: <http://trac.webkit.org/changeset/213198>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

*** Bug 168995 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

(In reply to comment #8)
> NS_UNAVAILABLE is just __attribute__((unavailable)). Use it! It must work
> with InstallAPI...

See bug #169243

[Alexey Proskuryakov]

For the record, __attribute__((unavailable)) has a different meaning, and should not be used like this, see rdar://problem/31139070.

We will soon be able to remove these symbols altogether (in fact, I believe that they are already unnecessary, but there is also _WKVisitedLinkProvider which still is).