Bug 121378 - Shrink factory functions
Summary: Shrink factory functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-15 00:14 PDT by Darin Adler
Modified: 2013-09-16 18:29 PDT (History)
5 users (show)

See Also:


Attachments
Patch (43.45 KB, patch)
2013-09-15 00:55 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (43.46 KB, patch)
2013-09-15 01:07 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (44.24 KB, patch)
2013-09-15 01:24 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (48.67 KB, patch)
2013-09-15 01:48 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2013-09-15 00:14:27 PDT
Shrink factory functions
Comment 1 Darin Adler 2013-09-15 00:55:02 PDT
Created attachment 211691 [details]
Patch
Comment 2 Darin Adler 2013-09-15 01:07:56 PDT
Created attachment 211693 [details]
Patch
Comment 3 Andreas Kling 2013-09-15 01:10:39 PDT
Comment on attachment 211693 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:55
> +        * html/HTMLElement.cpp:
> +        (WebCore::populateEventNameForAttributeLocalNameMap): Changed to use an
> +        AtomicStringImpl* as the key instead of AtomicString, since all the strings
> +        come from the local names of tags, and so don't need to be ref'd. This saves
> +        a bit of reference count churn when building the map and is the same pattern
> +        used in some maps in the make_names.pl script above.

Clever.

> Source/WebCore/ChangeLog:84
> +        (WebCore::WebVTTParser::constructTreeFromToken): Pass docuemnt reference rather than

Typo, document.

> Source/WebCore/dom/make_names.pl:999
> -        print F "        if (PassRefPtr<$parameters{namespace}Element> element = function(qName, document, formElement, createdByParser))\n";
> -        print F "            return element;\n";
> +        print F "        if (RefPtr<$parameters{namespace}Element> element = function(name, document, formElement, createdByParser))\n";
> +        print F "            return element.release();\n";

Heh, I resisted telling Sam to make this change just a few hours ago.
Comment 4 Darin Adler 2013-09-15 01:24:34 PDT
Created attachment 211695 [details]
Patch
Comment 5 Darin Adler 2013-09-15 01:33:35 PDT
I’ll land this once either EWS or my local build is done.
Comment 6 Early Warning System Bot 2013-09-15 01:38:07 PDT
Comment on attachment 211695 [details]
Patch

Attachment 211695 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1801789
Comment 7 Build Bot 2013-09-15 01:45:06 PDT
Comment on attachment 211695 [details]
Patch

Attachment 211695 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1808554
Comment 8 Early Warning System Bot 2013-09-15 01:45:39 PDT
Comment on attachment 211695 [details]
Patch

Attachment 211695 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1916277
Comment 9 Darin Adler 2013-09-15 01:48:41 PDT
Created attachment 211696 [details]
Patch
Comment 10 Darin Adler 2013-09-15 01:49:01 PDT
Committed r155801: <http://trac.webkit.org/changeset/155801>
Comment 11 Brent Fulgham 2013-09-15 19:35:35 PDT
Comment on attachment 211696 [details]
Patch

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

> Source/WebCore/dom/make_names.pl:755
> +    print(F "    new (NotNull, static_cast<void*>(&${lowerNamespace}NamespaceURI)) AtomicString(${lowerNamespace}NS);\n");

This change breaks the Windows build:

10>C:\Projects\WebKit\OpenSource\WebKitBuild\Release\obj32\WebCore\DerivedSources\WebKitFontFamilyNames.cpp(143): error C2440: 'static_cast' : cannot convert from 'const WTF::AtomicString *' to 'void *'
10>          Conversion loses qualifiers

Perhaps this was doing the wrong thing, and the C-style cast was masking the problem. But the build is now broken.

This may be related to the declaration of SKIP_STATIC_CONSTRUCTORS_ON_GCC, which is not active on Windows.

Any ideas for resolving this?
Comment 12 Darin Adler 2013-09-15 22:37:01 PDT
Seems fine to change it back to a C-style cast.
Comment 13 Darin Adler 2013-09-16 18:29:10 PDT
According to http://andreaskling.com/~kling/sizes/r155849/WebCore.txt, this patch actually made HTMLNames::init bigger. Something is going wrong.