Bug 72756 - REGRESSION (r100805): DOMHTMLElement’s accessKey property is declared as available in WebKit version that didn’t have it
: REGRESSION (r100805): DOMHTMLElement’s accessKey property is declared as avai...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit API
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To: Nobody
: InRadar, Regression
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-18 13:44 PST by mitz@webkit.org
Modified: 2012-02-12 02:28 PST (History)
15 users (show)

See Also:


Attachments
proposed patch (10.11 KB, patch)
2012-02-10 05:28 PST, Vineet Chaudhary (vineetc)
timothy: review-
Details | Formatted Diff | Diff
updated_patch (10.27 KB, patch)
2012-02-10 14:30 PST, Vineet Chaudhary (vineetc)
timothy: review-
Details | Formatted Diff | Diff
patch (10.23 KB, patch)
2012-02-10 15:27 PST, Vineet Chaudhary (vineetc)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz@webkit.org 2011-11-18 13:44:19 PST
<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.
Comment 1 mitz@webkit.org 2011-11-18 13:51:20 PST
<rdar://problem/10472271>
Comment 2 Ryosuke Niwa 2011-11-18 13:54:02 PST
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...
Comment 3 Timothy Hatcher 2011-11-19 08:33:24 PST
Yes.
Comment 4 Vineet Chaudhary (vineetc) 2012-02-10 05:28:52 PST
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.
Comment 5 WebKit Review Bot 2012-02-10 05:31:12 PST
Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.
Comment 6 Timothy Hatcher 2012-02-10 09:45:52 PST
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.
Comment 7 Timothy Hatcher 2012-02-10 09:46:52 PST
AVAILABLE_AFTER__WEBKIT_VERSION_5_1 Should not have the double "_" I inadvertently used.
Comment 8 Vineet Chaudhary (vineetc) 2012-02-10 13:43:36 PST
Thanks Timothy for the comments.

May I know the correct version macros to used.
Is AVAILABLE_AFTER_WEBKIT_VERSION_5_2 good?
Comment 9 Timothy Hatcher 2012-02-10 13:49:23 PST
The next version number isn't guaranteed to be 5.2. So only AVAILABLE_AFTER_WEBKIT_VERSION_5_1 is right.
Comment 10 Vineet Chaudhary (vineetc) 2012-02-10 14:30:18 PST
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.
Comment 11 Timothy Hatcher 2012-02-10 15:04:01 PST
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.
Comment 12 Vineet Chaudhary (vineetc) 2012-02-10 15:12:36 PST
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.
Comment 13 Vineet Chaudhary (vineetc) 2012-02-10 15:15:44 PST
(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. :)
Comment 14 Vineet Chaudhary (vineetc) 2012-02-10 15:27:56 PST
Created attachment 126593 [details]
patch
Comment 15 WebKit Review Bot 2012-02-10 16:42:53 PST
Comment on attachment 126593 [details]
patch

Clearing flags on attachment: 126593

Committed r107459: <http://trac.webkit.org/changeset/107459>
Comment 16 WebKit Review Bot 2012-02-10 16:42:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Csaba Osztrogonác 2012-02-11 00:31:49 PST
Reopen, because it broke the GTK build:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/18015/steps/compile-webkit/logs/stdio
Comment 18 Vineet Chaudhary (vineetc) 2012-02-11 05:39:48 PST
(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?
Comment 19 Philippe Normand 2012-02-12 02:28:09 PST
(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.