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
: WebKit
WebKit API
: 528+ (Nightly build)
: Unspecified Unspecified
: P1 Normal
Assigned To:
:
: InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2011-11-18 13:44 PST by
Modified: 2012-02-12 02:28 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-18 13:51:20 PST -------
<rdar://problem/10472271>
------- Comment #2 From 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 From 2011-11-19 08:33:24 PST -------
Yes.
------- Comment #4 From 2012-02-10 05:28:52 PST -------
Created an attachment (id=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 From 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 From 2012-02-10 09:45:52 PST -------
(From update of attachment 126506 [details])
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 From 2012-02-10 09:46:52 PST -------
AVAILABLE_AFTER__WEBKIT_VERSION_5_1 Should not have the double "_" I inadvertently used.
------- Comment #8 From 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 From 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 From 2012-02-10 14:30:18 PST -------
Created an attachment (id=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 From 2012-02-10 15:04:01 PST -------
(From update of attachment 126585 [details])
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 From 2012-02-10 15:12:36 PST -------
(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.
------- Comment #13 From 2012-02-10 15:15:44 PST -------
(In reply to comment #12)
> (From update of attachment 126585 [details] [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 From 2012-02-10 15:27:56 PST -------
Created an attachment (id=126593) [details]
patch
------- Comment #15 From 2012-02-10 16:42:53 PST -------
(From update of attachment 126593 [details])
Clearing flags on attachment: 126593

Committed r107459: <http://trac.webkit.org/changeset/107459>
------- Comment #16 From 2012-02-10 16:42:59 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #17 From 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 From 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 From 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.