Bug 235674 – Symbols not always properly hidden when using WebKitAdditions to introduce new API

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Symbols not always properly hidden when using WebKitAdditions to introduce new API.

[Chris Dumez]

<rdar://87999257>

[Chris Dumez]

Created attachment 450070 [details]
Patch

[Wenson Hsieh]

Comment on attachment 450070 [details]
Patch

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

> Source/WebKit/mac/replace-webkit-additions-includes.py:93
> +    additions_import_pattern = re.compile(r"\#if ENABLE\(API_WEBKIT_ADDITIONS\)\n#import <WebKitAdditions/(.*)>\n#endif")

(Not necessarily in this patch, but) I wonder if it's even worth throwing a

#if ENABLE(API_WEBKIT_ADDITIONS)
#error "…"
#endif

…somewhere. Not exactly sure where, though.

[EWS]

Committed r288658 (246466@main): <https://commits.webkit.org/246466@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450070 [details].

[Alexey Proskuryakov]

Comment on attachment 450070 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.h:74
> +#if ENABLE(API_WEBKIT_ADDITIONS)

Given that this foes not behave like an ENABLE macro, why make it look like one? Something like this seems like it would convey the purpose better:

#if 0 // API_WEBKIT_ADDITIONS_REPLACEMENT

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 450070 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450070&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.h:74
> > +#if ENABLE(API_WEBKIT_ADDITIONS)
> 
> Given that this foes not behave like an ENABLE macro, why make it look like
> one? Something like this seems like it would convey the purpose better:
> 
> #if 0 // API_WEBKIT_ADDITIONS_REPLACEMENT

Sure, this seems reasonable. I'll discuss this with Wenson & Tim and follow-up.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 450244 [details]
Follow-up to address ap's comments.

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> (In reply to Alexey Proskuryakov from comment #5)
> > Comment on attachment 450070 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=450070&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebpagePreferences.h:74
> > > +#if ENABLE(API_WEBKIT_ADDITIONS)
> > 
> > Given that this foes not behave like an ENABLE macro, why make it look like
> > one? Something like this seems like it would convey the purpose better:
> > 
> > #if 0 // API_WEBKIT_ADDITIONS_REPLACEMENT
> 
> Sure, this seems reasonable. I'll discuss this with Wenson & Tim and
> follow-up.

Thanks for the improvement suggestion :)

[EWS]

Committed r288747 (246540@main): <https://commits.webkit.org/246540@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 450244 [details].