Bug 130256 - AX: Replace DEPRECATED_DEFINE_STATIC_LOCAL by static NeverDestroyed<T>
Summary: AX: Replace DEPRECATED_DEFINE_STATIC_LOCAL by static NeverDestroyed<T>
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks: 130185
  Show dependency treegraph
 
Reported: 2014-03-14 12:42 PDT by Sergio Villar Senin
Modified: 2014-06-04 00:51 PDT (History)
16 users (show)

See Also:


Attachments
Patch (18.11 KB, patch)
2014-03-14 12:43 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2014-03-14 12:42:28 PDT
AX: Replace DEPRECATED_DEFINE_STATIC_LOCAL by static NeverDestroyed<T>
Comment 1 Sergio Villar Senin 2014-03-14 12:43:10 PDT
Created attachment 226751 [details]
Patch
Comment 2 James Craig 2014-03-14 13:02:03 PDT
Comment on attachment 226751 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1535
> -    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusFalse, ("false", AtomicString::ConstructFromLiteral));
> -    DEPRECATED_DEFINE_STATIC_LOCAL(const AtomicString, invalidStatusTrue, ("true", AtomicString::ConstructFromLiteral));
> +    static NeverDestroyed<const AtomicString> invalidStatusFalse("false", AtomicString::ConstructFromLiteral);
> +    static NeverDestroyed<const AtomicString> invalidStatusTrue("true", AtomicString::ConstructFromLiteral);

Please remove the invalidStatus changes from this patch. It's being handled in bug 130071 and your patch will cause conflicts.
Comment 3 Anders Carlsson 2014-03-14 13:12:31 PDT
Comment on attachment 226751 [details]
Patch

I don't think we should be using NeverDestroyed for Strings, and definitely not AtomicStrings.
Comment 4 Sergio Villar Senin 2014-03-17 09:09:46 PDT
(In reply to comment #3)
> (From update of attachment 226751 [details])
> I don't think we should be using NeverDestroyed for Strings, and definitely not AtomicStrings.

Should we leave them as they are then?
Comment 5 Sergio Villar Senin 2014-03-17 09:38:23 PDT
(In reply to comment #3)
> (From update of attachment 226751 [details])
> I don't think we should be using NeverDestroyed for Strings, and definitely not AtomicStrings.

BTW I changed them also because I saw a similar usage in many different places, from platform/ to html/. Maybe we should fix those too.
Comment 6 Brent Fulgham 2014-04-01 10:17:43 PDT
It's also confusing that we now have a DEPRECATED_DEFINE_STATIC_LOCAL macro, if we should still be using it for AtomicStrings.

I think we need some Style Guide guidance on this!
Comment 7 Darin Adler 2014-04-01 13:54:44 PDT
(In reply to comment #6)
> It's also confusing that we now have a DEPRECATED_DEFINE_STATIC_LOCAL macro, if we should still be using it for AtomicStrings.

We should not be using DEPRECATED_DEFINE_STATIC_LOCAL at all, definitely not for AtomicString. It can always be replaced by NeverDestroyed.

But, as a separate related issue, Anders thinks that we are overusing static locals for AtomicString, and thinks we should cut down on this overuse during the process of converting from DEPRECATED_DEFINE_STATIC_LOCAL to NeverDestroyed, by not converting some and instead making them not use a static local AtomicString at all.
Comment 8 Sergio Villar Senin 2014-05-06 09:34:03 PDT
This was overruled by an exact same bug reported later but that landed before
Comment 9 Csaba Osztrogonác 2014-06-04 00:51:40 PDT
Comment on attachment 226751 [details]
Patch

Cleared review? from attachment 226751 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).