REGRESSION (r100805): DOMHTMLElement’s accessKey property is declared as available in WebKit version that didn’t have it

• Component: WebKit API
• Status: RESOLVED FIXED

<http://trac.webkit.org/changeset/100805/trunk/Source/WebCore/bindings/objc/PublicDOMInterfaces.h> added an accessKey property to DOMHTMLEmenet. This property is new so an appropriate availability macro should indicate that.

<rdar://problem/10472271>

[Ryosuke Niwa]

So revert removals and add new entry with AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER? That might require us adding back entries to .idl files with ObjC if-defs though...

[Timothy Hatcher]

Yes.

[Vineet Chaudhary (vineetc)]

Created attachment 126506 [details]
proposed patch

Adding AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER in WebKitAvailability.h.
Also reverting the chenges in idl files moving then under #if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C.

In PublicDOMInterface.h marking accessKey property for HTMLAnchorElement, HTMLAreaElement, HTMLButtonElement, HTMLInputElement, HTMLLabelElement, HTMLLegendElement  and HTMLTextAreaElement 
 with an  availability macro AVAILABLE_WEBKIT_VERSION_1_3_AND_LATER_BUT_DEPRECATED_AFTER_WEBKIT_VERSION_4_0.

Please let me know your review comments on these changes. Thanks.

[WebKit Review Bot]

Please wait for approval from [e-mail] (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.

[Timothy Hatcher]

Comment on attachment 126506 [details]
proposed patch

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

r- for the wrong versions in the macros.

> Source/JavaScriptCore/API/WebKitAvailability.h:916
> + * AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER
> + * 
> + * Used on functions introduced in WebKit 5.1
> + */
> +#define AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER

Safari/WebKit 5.1 has already shipped. So this is not the right version.

AVAILABLE_AFTER__WEBKIT_VERSION_5_1?

Mark, thoughts?

> Source/WebCore/bindings/objc/PublicDOMInterfaces.h:334
> +@property(copy) NSString *accessKey AVAILABLE_WEBKIT_VERSION_1_3_AND_LATER_BUT_DEPRECATED_AFTER_WEBKIT_VERSION_4_0;

This (and all other uses you added) should be AVAILABLE_WEBKIT_VERSION_1_3_AND_LATER_BUT_DEPRECATED_AFTER_WEBKIT_VERSION_5_1. So you need to add that new macro. Because version 5 and 5.1 of WebKit had these.

> Source/WebCore/html/HTMLInputElement.idl:56
>  #if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C
> +        attribute [Reflect] DOMString accessKey;
> +#endif
> +#if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C
>          attribute [ObjCImplementedAsUnsignedLong] DOMString size; // DOM level 2 changed this to a long, but ObjC API is a string

You can group these #ifdefs.

[Timothy Hatcher]

AVAILABLE_AFTER__WEBKIT_VERSION_5_1 Should not have the double "_" I inadvertently used.

[Vineet Chaudhary (vineetc)]

Thanks Timothy for the comments.

May I know the correct version macros to used.
Is AVAILABLE_AFTER_WEBKIT_VERSION_5_2 good?

[Timothy Hatcher]

The next version number isn't guaranteed to be 5.2. So only AVAILABLE_AFTER_WEBKIT_VERSION_5_1 is right.

[Vineet Chaudhary (vineetc)]

Created attachment 126585 [details]
updated_patch

Updated patch as per review comments.
Added AVAILABLE_WEBKIT_VERSION_1_3_AND_LATER_BUT_DEPRECATED_AFTER_WEBKIT_VERSION_5_1.

[Timothy Hatcher]

Comment on attachment 126585 [details]
updated_patch

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

> Source/JavaScriptCore/API/WebKitAvailability.h:913
> + * Used on functions introduced in WebKit 5.1
> + */
> +#define AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER

This is still wrong. 5.1 has shipped, so changes in TOT can't be available in 5.1. This needs to be AVAILABLE_AFTER_WEBKIT_VERSION_5_1. I know it reads funny, but thats the best we can do without predicting the future version number.

[Vineet Chaudhary (vineetc)]

Comment on attachment 126585 [details]
updated_patch

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

> Source/JavaScriptCore/API/WebKitAvailability.h:914
> +

Oke I will modify this repalcing AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER by AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER.

[Vineet Chaudhary (vineetc)]

(In reply to comment #12)
> (From update of attachment 126585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126585&action=review
> 
> > Source/JavaScriptCore/API/WebKitAvailability.h:914
> > +
> 
> Oke I will modify this repalcing AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER by AVAILABLE_WEBKIT_VERSION_5_1_AND_LATER.

Sorry for typo replace by AVAILABLE_AFTER_WEBKIT_VERSION_5_1. :)

[Vineet Chaudhary (vineetc)]

Created attachment 126593 [details]
patch

[WebKit Review Bot]

Comment on attachment 126593 [details]
patch

Clearing flags on attachment: 126593

Committed r107459: <http://trac.webkit.org/changeset/107459>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke the GTK build:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/18015/steps/compile-webkit/logs/stdio

[Vineet Chaudhary (vineetc)]

(In reply to comment #17)
> Reopen, because it broke the GTK build:
> http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/18015/steps/compile-webkit/logs/stdio

Looking at logs it seems to be issue after enabling MutationObserver flag but related to these changes.
Can someone please confirm this?

[Philippe Normand]

(In reply to comment #18)
> (In reply to comment #17)
> > Reopen, because it broke the GTK build:
> > http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/18015/steps/compile-webkit/logs/stdio
> 
> Looking at logs it seems to be issue after enabling MutationObserver flag but related to these changes.
> Can someone please confirm this?

Indeed, this failure is tracked in bug 78433.