Change CSSToStyleMap functions to take const CSSValue&s

Change CSSToStyleMap functions to take const CSSValue&s

Change CSSToStyleMap functions to take const CSSValue&s instead of
CSSValue*s. Introduced some temporary const_casts while the rest of
StyleBuilderFunctions is changed.

BUG=526586
Committed: https://crrev.com/e4b891f347185cc0b79c3497c7860890110d3cf9
Cr-Commit-Position: refs/heads/master@{#352777}

[sashab, 2015-09-28 05:14:23]

sashab@chromium.org changed reviewers:
+ timloh@chromium.org

[sashab, 2015-09-30 03:17:41]

sashab@chromium.org changed reviewers:
+ alancutter@chromium.org

[alancutter (OOO until 2018), 2015-10-01 00:37:15]

lgtm!

https://chromiumcodereview.appspot.com/1373753002/diff/20001/third_party/WebK...
File third_party/WebKit/Source/core/css/resolver/CSSToStyleMap.cpp (right):

https://chromiumcodereview.appspot.com/1373753002/diff/20001/third_party/WebK...
third_party/WebKit/Source/core/css/resolver/CSSToStyleMap.cpp:445: const
CSSValue* current = borderImage.item(i);
This should be a const CSSValue& since most reads are dereferencing it.

[Timothy Loh, 2015-10-01 03:03:41]

Why don't we just make CSSImageValues have mutable members first instead of
adding the same TODO everywhere?

[sashab, 2015-10-02 04:37:34]

I think that making CSSImageValue const (well, really, making
ElementStyleResources take const references) is a much harder project. Having a
few const casts here for now seems better as it supports the 90% of cases, and
we can work on making ElementStyleResources methods take const CSSValues later
(e.g. should StyleGeneratedImage::create() take a const CSSImageGeneratorValue&
as a parameter? If so, then CSSImageGeneratorValue::addClient() should be a
const method. But addClient() is clearly not a const method. Etc...)

[sashab, 2015-10-06 02:28:55]

ping timloh, see previous comment.

[Timothy Loh, 2015-10-06 02:50:46]

If we're making the CSSImageValue members mutable here, then update the patch
description and remove the const_casts...

https://codereview.chromium.org/1373753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1373753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:80:
NinePieceImage mask;
the if check you're adding should start from here

[sashab, 2015-10-07 01:37:58]

Thanks Tim. Rebased this onto the const StyleResolverState.styleImage() change,
and removed unecessary changes to CSSImageValue. Now ready for your review
again.

https://codereview.chromium.org/1373753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1373753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:80:
NinePieceImage mask;
On 2015/10/06 02:50:46, Timothy Loh wrote:
> the if check you're adding should start from here

Are you sure? Surely we want mask.setMaskDefaults() to be called even if 
reflectValue->mask() is null... This would match the existing behavior at least?

https://codereview.chromium.org/1373753002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/CSSToStyleMap.cpp (right):

https://codereview.chromium.org/1373753002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSToStyleMap.cpp:198: //
TODO(sashab): Make StyleBuilderConverter take const&s and remove these
const_casts.
convertLengthOrAuto can't be made to take a const CSSValue& because it has
generated calls from StyleBuilderFunctions.cpp.tmpl that pass it a pointer.

[Timothy Loh, 2015-10-07 02:27:00]

lgtm

https://codereview.chromium.org/1373753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1373753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:80:
NinePieceImage mask;
On 2015/10/07 01:37:58, sashab wrote:
> On 2015/10/06 02:50:46, Timothy Loh wrote:
> > the if check you're adding should start from here
> 
> Are you sure? Surely we want mask.setMaskDefaults() to be called even if 
> reflectValue->mask() is null... This would match the existing behavior at
least?

We don't need to setMask at all in this case though

https://codereview.chromium.org/1373753002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/CSSToStyleMap.cpp (right):

https://codereview.chromium.org/1373753002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/CSSToStyleMap.cpp:198: //
TODO(sashab): Make StyleBuilderConverter take const&s and remove these
const_casts.
On 2015/10/07 01:37:58, sashab wrote:
> convertLengthOrAuto can't be made to take a const CSSValue& because it has
> generated calls from StyleBuilderFunctions.cpp.tmpl that pass it a pointer.

OK

[sashab, 2015-10-07 02:33:35]

https://codereview.chromium.org/1373753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp
(right):

https://codereview.chromium.org/1373753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp:80:
NinePieceImage mask;
On 2015/10/07 02:27:00, Timothy Loh wrote:
> On 2015/10/07 01:37:58, sashab wrote:
> > On 2015/10/06 02:50:46, Timothy Loh wrote:
> > > the if check you're adding should start from here
> > 
> > Are you sure? Surely we want mask.setMaskDefaults() to be called even if 
> > reflectValue->mask() is null... This would match the existing behavior at
> least?
> 
> We don't need to setMask at all in this case though

Oh I see, good point. Fixed up now.

[sashab, 2015-10-07 04:37:32]

The CQ bit was checked by sashab@chromium.org

[sashab, 2015-10-07 04:37:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
alancutter@chromium.org, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1373753002/#ps100001
(title: "Small fix for mask")

[commit-bot: I haz the power, 2015-10-07 04:38:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1373753002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1373753002/100001

[commit-bot: I haz the power, 2015-10-07 05:45:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-07 05:45:46]

Patchset 6 (id:??) landed as
https://crrev.com/e4b891f347185cc0b79c3497c7860890110d3cf9
Cr-Commit-Position: refs/heads/master@{#352777}