Bug 32674 – Fix link failure on LTCG builds on Windows for chromium port

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Marc-Antoine Ruel]

Created attachment 45090 [details]
Patch

Reenable SVGAllInOne.cpp. This required some fixups.

Namely, there was conflicts in constants between HTMLNames and
SVGNames, and conflicts between static functions names.

No new tests as functionality has not changed.

[WebKit Review Bot]

Attachment 45090 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/svg/SVGAllInOne.cpp:99:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/svg/SVGAllInOne.cpp:105:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/svg/SVGAllInOne.cpp:137:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/svg/SVGAllInOne.cpp:138:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/svg/SVGAllInOne.cpp:151:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5

[Marc-Antoine Ruel]

Created attachment 45091 [details]
Patch

[Marc-Antoine Ruel]

Created attachment 45092 [details]
Patch

[WebKit Review Bot]

Attachment 45091 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/svg/SVGAllInOne.cpp:152:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45092 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 45092 [details]
Patch

Do we have a build-bot which uses this (or that will), otherwise this will certainly break again.

This is kinda an ugly fix, but that's because any of the AllInOne filese are kinda hacks.  What is a LTCG build?  Why should we be prefixing the SVGNames which conflict with HTMLNames instead of the reverse?

It seems that at least some of these changes need comments.  Like the naming of mapSvgAttributeToCSSProperty should have a comment.  maybe we should just share that code using StyledElement.h/StyledElement.cpp instead of renaming our copy?  Or does the copy actually do something different in the SVG case?

This looks fine, but would be nice to hear answers to above before landing.

Better to leave the "Reviewed by" line as the default "Reviewed by NOBODY(OOPS!)" because our scripts like svn-apply, the commit-queue and normal bugzilla-tool land-diff/land-patches know how to fix that up with the proper reviewer from the bug anyway.

[Eric Seidel (no email)]

The chromium side of this was:
http://codereview.chromium.org/500099
and:
http://code.google.com/p/chromium/issues/detail?id=30494

[Eric Seidel (no email)]

It's possible that Apple's windows builders will want to use a similar optimization.  CCing the Apple windows build folks.

[Marc-Antoine Ruel]

LTCG means link time code generation.

SVGNames are the only one conflicting. I use the SVG ones because it is SVG compile units.

I'll look at removing mapSvgAttributeToCSSProperty.

[Eric Seidel (no email)]

Thank you very much.  Certainly not required that we remove mapSvgAttributeToCSSProperty, but if it's obvious to share code there, we might as well.

[Marc-Antoine Ruel]

Created attachment 45102 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45102 [details] without any errors.

[Marc-Antoine Ruel]

Created attachment 45103 [details]
Patch

[Marc-Antoine Ruel]

- Removed the duplicate function.
- Running try jobs (experimental):
http://build.chromium.org/buildbot/webkit-try/builders/Try%20Apple%20Leopard%20Intel%20Debug/builds/15
http://build.chromium.org/buildbot/webkit-try/builders/Try%20Apple%20Windows%20Debug/builds/14

[Eric Seidel (no email)]

Comment on attachment 45103 [details]
Patch

Yay!  Thank you for the update ChangeLog.

I think I would have just made it a protected static member of SVGStyledElement, the callsite in SVGFontFaceElement would not have had to change.

But also think this is totally fine as a free function.

r+

[Eric Seidel (no email)]

Comment on attachment 45103 [details]
Patch

Ah.  I keep forgetting you're not a committer.  cq+ too! :)

[WebKit Commit Bot]

Comment on attachment 45103 [details]
Patch

Clearing flags on attachment: 45103

Committed r52288: <http://trac.webkit.org/changeset/52288>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.