Removing duplicate functions and symbol names in core/css

Removing duplicate functions and symbol names in core/css

In the unity build experiment I encountered a few colliding duplicate
symbol names in core/css and this patch removes them by replacing
duplicated symbols with a single code and by doing some symbol
renames.

Review-Url: https://codereview.chromium.org/2797973002
Cr-Commit-Position: refs/heads/master@{#462256}
Committed: https://chromium.googlesource.com/chromium/src/+/5fa4cb0d7a1a784fec7991eaeb1a80e43f9156e5

[Daniel Bratell, 2017-04-05 09:47:01]

bratell@opera.com changed reviewers:
+ rune@opera.com

[rune, 2017-04-05 10:54:39]

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:29: #include
"core/css/zoomAdjustedPixelValue.h"
Why was this include necessary?

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.h:341: double
CSSPrimitiveValue::computeLength(const CSSToLengthConversionData&) const;
Can you explain why these are necessary?

[Daniel Bratell, 2017-04-05 12:05:39]

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp (right):

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:29: #include
"core/css/zoomAdjustedPixelValue.h"
On 2017/04/05 10:54:39, rune wrote:
> Why was this include necessary?

I cannot see that it is. Probably a typo (or something remaining after earlier
experiments). I've removed it now.

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.h:341: double
CSSPrimitiveValue::computeLength(const CSSToLengthConversionData&) const;
On 2017/04/05 10:54:39, rune wrote:
> Can you explain why these are necessary?

The computeLength functions have explicit specializations in
CSSPrimitiveValue.cpp which are used, but only if the calling code knows about
them. In CSSGradientValue.cpp these functions were called and got automatic
instantiations which then collided with the explicit instantiations in the
compiler. I think the linker would have made them the same anyway, but it
doesn't work if it's in the same compilation unit.

The exact error message is:
In file included from gen/blink/core/css_jumbo.cpp:53:
gen/blink/core/../../../../../third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:310:26:
error: explicit specialization of 'computeLength<float>' after instantiation
float CSSPrimitiveValue::computeLength(
                         ^
gen/blink/core/../../../../../third_party/WebKit/Source/core/css/CSSGradientValue.cpp:495:35:
note: implicit instantiation first required here
          length = stop.m_offset->computeLength<float>(conversionData);
                                  ^
In file included from gen/blink/core/css_jumbo.cpp:53:
gen/blink/core/../../../../../third_party/WebKit/Source/core/css/CSSPrimitiveValue.cpp:316:27:
error: explicit specialization of 'computeLength<double>' after instantiation
double CSSPrimitiveValue::computeLength(
                          ^
gen/blink/core/../../../../../third_party/WebKit/Source/core/css/CSSCalculationValue.cpp:190:22:
note: implicit instantiation first required here
            m_value->computeLength<double>(conversionData) * multiplier);
                     ^

[rune, 2017-04-05 19:40:22]

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/CSSPrimitiveValue.h (right):

https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/CSSPrimitiveValue.h:341: double
CSSPrimitiveValue::computeLength(const CSSToLengthConversionData&) const;
On 2017/04/05 12:05:39, Daniel Bratell wrote:
> On 2017/04/05 10:54:39, rune wrote:
> > Can you explain why these are necessary?
> 
> The computeLength functions have explicit specializations in
> CSSPrimitiveValue.cpp which are used, but only if the calling code knows about
> them. In CSSGradientValue.cpp these functions were called and got automatic
> instantiations which then collided with the explicit instantiations in the
> compiler. I think the linker would have made them the same anyway, but it
> doesn't work if it's in the same compilation unit.

I don't know enough about templates and compilers to say if there's a better
way, and without a comment why they're here someone might just remove these
declarations later on. The rest looks good. If you remove these changes, I can
approve.

[rune, 2017-04-05 21:14:42]

On 2017/04/05 19:40:22, rune wrote:
>
https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/CSSPrimitiveValue.h (right):
> 
>
https://codereview.chromium.org/2797973002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/CSSPrimitiveValue.h:341: double
> CSSPrimitiveValue::computeLength(const CSSToLengthConversionData&) const;
> On 2017/04/05 12:05:39, Daniel Bratell wrote:
> > On 2017/04/05 10:54:39, rune wrote:
> > > Can you explain why these are necessary?
> > 
> > The computeLength functions have explicit specializations in
> > CSSPrimitiveValue.cpp which are used, but only if the calling code knows
about
> > them. In CSSGradientValue.cpp these functions were called and got automatic
> > instantiations which then collided with the explicit instantiations in the
> > compiler. I think the linker would have made them the same anyway, but it
> > doesn't work if it's in the same compilation unit.
> 
> I don't know enough about templates and compilers to say if there's a better
> way, and without a comment why they're here someone might just remove these
> declarations later on. The rest looks good. If you remove these changes, I can
> approve.

Discussed with bratell off-line. It would look better if specialization
declarations could be done inside the class, but apparently not. It's also a
question why we are using a template method here at all since all instantiations
are special and if you try to do e.g. computeLength<long>(), you'll get a link
error which is not evident from the header file. Changing this from e.g.
computeLength<float>() to computeLengthAsFloat(), etc. is a big change, though.

lgtm.

[Daniel Bratell, 2017-04-05 21:30:31]

The CQ bit was checked by bratell@opera.com

[commit-bot: I haz the power, 2017-04-05 21:31:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-05 23:04:38]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491427831786410,
"parent_rev": "56ef6868cf9634fb46912a9ce29f8667b9ad2577", "commit_rev":
"5fa4cb0d7a1a784fec7991eaeb1a80e43f9156e5"}

[commit-bot: I haz the power, 2017-04-05 23:05:27]

Description was changed from

==========
Removing duplicate functions and symbol names in core/css

In the unity build experiment I encountered a few colliding duplicate
symbol names in core/css and this patch removes them by replacing
duplicated symbols with a single code and by doing some symbol
renames.
==========

to

==========
Removing duplicate functions and symbol names in core/css

In the unity build experiment I encountered a few colliding duplicate
symbol names in core/css and this patch removes them by replacing
duplicated symbols with a single code and by doing some symbol
renames.

Review-Url: https://codereview.chromium.org/2797973002
Cr-Commit-Position: refs/heads/master@{#462256}
Committed:
https://chromium.googlesource.com/chromium/src/+/5fa4cb0d7a1a784fec7991eaeb1a...
==========

[commit-bot: I haz the power, 2017-04-05 23:05:28]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5fa4cb0d7a1a784fec7991eaeb1a...

[Guido Urdaneta, 2017-04-06 08:35:06]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2801683005/ by guidou@chromium.org.

The reason for reverting is: This is a speculative revert to determine if this
is causing a WebRTC Chrome bot to fail.

See
https://build.chromium.org/p/chromium.webrtc.fyi/builders/Android%20Tests%20%...

If the revert does not fix the bot, I will reland..