Use more const references in MarkupAccumulator and its subclasses

Use more const references in MarkupAccumulator and its subclasses

Use more const references in MarkupAccumulator and its subclasses. This makes
the code look safer.

R=adamk

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166059

[adamk, 2014-01-28 22:12:34]

https://codereview.chromium.org/148673002/diff/1/Source/core/editing/markup.cpp
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/148673002/diff/1/Source/core/editing/markup.c...
Source/core/editing/markup.cpp:231: const bool useRenderedText =
!enclosingNodeWithTag(firstPositionInNode(const_cast<Text*>(&text)), selectTag);
This and the various const_casts below make me a bit sad. Are these just cases
where const-correctness needs to be pushed down further? Or is there actual
mutation happening in the code these things are calling?

https://codereview.chromium.org/148673002/diff/1/Source/core/page/PageSeriali...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/148673002/diff/1/Source/core/page/PageSeriali...
Source/core/page/PageSerializer.cpp:268: ASSERT(styleSheet->ownerDocument());
This makes me wonder when ownerDocument() can return null...

[Inactive, 2014-01-29 17:13:21]

https://codereview.chromium.org/148673002/diff/1/Source/core/editing/markup.cpp
File Source/core/editing/markup.cpp (right):

https://codereview.chromium.org/148673002/diff/1/Source/core/editing/markup.c...
Source/core/editing/markup.cpp:231: const bool useRenderedText =
!enclosingNodeWithTag(firstPositionInNode(const_cast<Text*>(&text)), selectTag);
On 2014/01/28 22:12:35, adamk wrote:
> This and the various const_casts below make me a bit sad. Are these just cases
> where const-correctness needs to be pushed down further? Or is there actual
> mutation happening in the code these things are calling? 

Yes, this is unfortunate. I will be less agressive on the constness to avoid
this in the next iteration.

https://codereview.chromium.org/148673002/diff/1/Source/core/page/PageSeriali...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/148673002/diff/1/Source/core/page/PageSeriali...
Source/core/page/PageSerializer.cpp:268: ASSERT(styleSheet->ownerDocument());
On 2014/01/28 22:12:35, adamk wrote:
> This makes me wonder when ownerDocument() can return null...

Based on its implementation, it can. I don't see any reason why it could not
return null here. I think we should add a null check, what do you think?

[Inactive, 2014-01-29 17:15:41]

Second iteration is up. No more const casting and I transformed the ASSERT into
a null check for safety (let me know if you think we should keep the ASSERT).

[adamk, 2014-01-29 17:26:25]

lgtm once you've done a bit more to the ownerDocument stuff (I don't feel
strongly, but adding a bailout where there wasn't one before seems a bit wrong
for a change that otherwise doesn't change behavior).

https://codereview.chromium.org/148673002/diff/1/Source/core/page/PageSeriali...
File Source/core/page/PageSerializer.cpp (right):

https://codereview.chromium.org/148673002/diff/1/Source/core/page/PageSeriali...
Source/core/page/PageSerializer.cpp:268: ASSERT(styleSheet->ownerDocument());
On 2014/01/29 17:13:21, Chris Dumez wrote:
> On 2014/01/28 22:12:35, adamk wrote:
> > This makes me wonder when ownerDocument() can return null...
> 
> Based on its implementation, it can. I don't see any reason why it could not
> return null here. I think we should add a null check, what do you think?

Sorry, I was too vague. I don't think it's possible for ownerDocument to be NULL
here, so the ASSERT seems better to me (if we're serializing a stylesheet, I'd
really think it's associated with a document). But there's no need to ASSERT
each time through the loop, let's move that ASSERT up to the to of the method.
Sound ok? If you'd feel better with the bailout, move that to the top and add a
FIXME.

[Inactive, 2014-01-29 17:50:47]

On 2014/01/29 17:26:25, adamk wrote:
> lgtm once you've done a bit more to the ownerDocument stuff (I don't feel
> strongly, but adding a bailout where there wasn't one before seems a bit wrong
> for a change that otherwise doesn't change behavior).
> 
>
https://codereview.chromium.org/148673002/diff/1/Source/core/page/PageSeriali...
> File Source/core/page/PageSerializer.cpp (right):
> 
>
https://codereview.chromium.org/148673002/diff/1/Source/core/page/PageSeriali...
> Source/core/page/PageSerializer.cpp:268: ASSERT(styleSheet->ownerDocument());
> On 2014/01/29 17:13:21, Chris Dumez wrote:
> > On 2014/01/28 22:12:35, adamk wrote:
> > > This makes me wonder when ownerDocument() can return null...
> > 
> > Based on its implementation, it can. I don't see any reason why it could not
> > return null here. I think we should add a null check, what do you think?
> 
> Sorry, I was too vague. I don't think it's possible for ownerDocument to be
NULL
> here, so the ASSERT seems better to me (if we're serializing a stylesheet, I'd
> really think it's associated with a document). But there's no need to ASSERT
> each time through the loop, let's move that ASSERT up to the to of the method.
> Sound ok? If you'd feel better with the bailout, move that to the top and add
a
> FIXME.

Ok, I brought back the ASSERT.

[commit-bot: I haz the power, 2014-01-29 17:51:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/148673002/40001

[commit-bot: I haz the power, 2014-01-29 23:32:19]

Change committed as 166059

[commit-bot: I haz the power, 2014-01-29 23:36:52]

CQ bit was unchecked on CL. Ignoring.