Bug 168579 – Clean up how WebKit exports _WebCreateFragment

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

WebKit exports the _WebCreateFragment symbol, but it is not present in any framework headers.

[Alexey Proskuryakov]

Created attachment 302102 [details]
proposed patch

[WebKit Commit Bot]

Attachment 302102 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebCreateFragmentInternal.h:37:  _WebCreateFragment is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 302102 [details]
proposed patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:794
> +		E13E782C1E5A7365001849D1 /* WebCreateFragmentInternal.h in Headers */ = {isa = PBXBuildFile; fileRef = E13E782B1E5A7365001849D1 /* WebCreateFragmentInternal.h */; settings = {ATTRIBUTES = (Private, ); }; };

We are we calling this “Internal” if it’s a private header?

> Source/WebKit/mac/WebCoreSupport/WebCreateFragmentInternal.h:33
> +#ifdef __cplusplus

This has got to be true because otherwise the above declarations would fail to compile.

[Alexey Proskuryakov]

> We are we calling this “Internal” if it’s a private header?

This naming is used elsewhere in WebKit for things that have to be framework headers, but are not API or SPI. Do you have a different suggestion for the name?

We need to expose this header to have symbols match between framework binaries and stubs.

> This has got to be true because otherwise the above declarations would fail
> to compile.

Good point.

[Alexey Proskuryakov]

Created attachment 302107 [details]
proposed patch

Removed __cplusplus checks, added a radar number.

(In reply to comment #4)
> > We are we calling this “Internal” if it’s a private header?
> 
> This naming is used elsewhere in WebKit for things that have to be framework
> headers, but are not API or SPI.

Filtering the Headers build phase of the WebCore, WebKitLegacy and WebKit targets for “Internal.h”, the only private header matching the pattern is npruntime_internal.h in WebCore. All other headers with names ending with Internal.h are project headers.

I believe that the convention is to add the suffixes “Private” and “Internal” only when needed to disambiguate, i.e. when there is also a separate header with the base name or with another suffix (one possible exception is that if the object being declared in a header has a name that ends with Private, then the header’s name may end with Private by virtue of being named after what it is declaring).

So unless I’m missing something, I suggest calling this header WebCreateFragment.h.

Comment on attachment 302107 [details]
proposed patch

r=me but I’d prefer the standard naming.

[Alexey Proskuryakov]

Hmm, it's less frequent than I thought. But there are four of those in JavaScriptCore.

Naming an exposed header that's purely internal to the WebKit stack in a "standard" way still seems less than great to me.

[WebKit Commit Bot]

Comment on attachment 302107 [details]
proposed patch

Clearing flags on attachment: 302107

Committed r212642: <http://trac.webkit.org/changeset/212642>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.