704311 - *Element::CopyInnerTo shouldn't be const

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Turns out that most implementations are just const_casting anyway. Same for its caller, nsINode::Clone.

Comment 1

[Mark Capella [:capella]]

I haven't worked in the COM component alot, so I've assumed the \content directory is the target code ... can you clarify if this is what you're looking to address / cleanup? I can't imagine you'd want to mass scrub 149 files in example 2 ...

dir \content scan for ::Clone\(.*\) const     Found 14 matching lines in 13 files
dir \content scan for Clone\(.*\) const       Found 149 matching lines in 123 files
dir \content scan for CopyInnerTo\(.*\) const Found 22 matching lines in 18 files

Comment 2

[Mark Capella [:capella]]

Attachment: Patch (v1) rough experiment

I took a quick tinker at this and got my C++ stuck in a corner ... the attached patch build/fails with nsSVGImageElement.cpp(102) : error C2662: 'nsSVGImageElement::CopyInnerTo' : cannot convert 'this' pointer from 'const nsSVGImageElement' to 'nsSVGImageElement &'

This traces back to the files' NS_IMPL_ELEMENT_CLONE_WITH_INIT(nsSVGImageElement) macro expansion of ::Clone() which is also a const function ...

In-lining the source of the macro and removing the const keyword gets me stuck on the ::Clone() line rv |= CopyInnerTo(it); which burps up nsSVGImageElement.cpp(64) : error C2259: 'nsSVGImageElement' : cannot instantiate abstract class which refers back to macro NS_IMPL_NS_NEW_SVG_ELEMENT(Image) ...

And that's where my brain explodes ...

Comment 3

[Mark Capella [:capella]]

Attachment: Patch (v2)

Ok, cool! This built and tested ok locally ... got some strange errors involving 'satchel' tests ... next step push-to-try I guess.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 617078 [details] [diff] [review]
Patch (v2)

Review of attachment 617078 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, but there are a few const_casts that shouldn't be necessary; see below. Thanks for doing this!

::: content/html/content/src/nsGenericHTMLFrameElement.cpp
@@ +230,2 @@
>  {
> +  nsresult rv = const_cast<nsGenericHTMLFrameElement*>(this)->nsGenericHTMLElement::CopyInnerTo(aDest);

You shouldn't need the const_cast here.

::: content/html/content/src/nsHTMLCanvasElement.cpp
@@ +157,2 @@
>  {
> +  nsresult rv = const_cast<nsHTMLCanvasElement*>(this)->nsGenericHTMLElement::CopyInnerTo(aDest);

Or here.

::: content/html/content/src/nsHTMLImageElement.cpp
@@ +656,5 @@
>  {
>    if (aDest->OwnerDoc()->IsStaticDocument()) {
>      CreateStaticImageClone(static_cast<nsHTMLImageElement*>(aDest));
>    }
> +  return const_cast<nsHTMLImageElement*>(this)->nsGenericHTMLElement::CopyInnerTo(aDest);

Or here.

::: content/html/content/src/nsHTMLInputElement.cpp
@@ +681,5 @@
>    nsCOMPtr<nsINodeInfo> ni = aNodeInfo;
>    nsRefPtr<nsHTMLInputElement> it =
>      new nsHTMLInputElement(ni.forget(), NOT_FROM_PARSER);
>  
> +  nsresult rv = const_cast<nsHTMLInputElement*>(this)->CopyInnerTo(it);

(Though it is needed here.)

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2876,2 @@
>  {
> +  nsresult rv = const_cast<nsHTMLMediaElement*>(this)->nsGenericHTMLElement::CopyInnerTo(aDest);

But not here.

::: content/html/content/src/nsHTMLObjectElement.cpp
@@ +569,2 @@
>  {
> +  nsresult rv = const_cast<nsHTMLObjectElement*>(this)->nsGenericHTMLFormElement::CopyInnerTo(aDest);

Or here.

::: content/html/content/src/nsHTMLOptionElement.cpp
@@ +439,2 @@
>  {
> +  nsresult rv = const_cast<nsHTMLOptionElement*>(this)->nsGenericHTMLElement::CopyInnerTo(aDest);

here

::: content/html/content/src/nsHTMLSharedObjectElement.cpp
@@ +517,2 @@
>  {
> +  nsresult rv = const_cast<nsHTMLSharedObjectElement*>(this)->nsGenericHTMLElement::CopyInnerTo(aDest);

or here.

::: content/html/content/src/nsHTMLTextAreaElement.cpp
@@ +1307,2 @@
>  {
> +  nsresult rv = const_cast<nsHTMLTextAreaElement*>(this)->nsGenericHTMLFormElement::CopyInnerTo(aDest);

Or here.

::: content/svg/content/src/nsSVGImageElement.cpp
@@ +301,5 @@
>  {
>    if (aDest->OwnerDoc()->IsStaticDocument()) {
>      CreateStaticImageClone(static_cast<nsSVGImageElement*>(aDest));
>    }
> +  return const_cast<nsSVGImageElement*>(this)->nsSVGImageElementBase::CopyInnerTo(aDest);

Id.

Comment 5

[Mark Capella [:capella]]

Attachment: Patch (v3

Cool, yah you were right ... overkill on my part. This was rebuilt and retested locally ... same "toolkit/component/satchel issues ... else looks ok :)

Comment 6

[Mark Capella [:capella]]

Comment on attachment 617301 [details] [diff] [review]
Patch (v3

Try results ...

https://tbpl.mozilla.org/?tree=Try&rev=d2207cd3fc20

Comment 7

[Johnny Stenback (:jst)]

Ms2ger, I must be missing something here, what const_casts are we trying to avoid by fixing this? Seems Mark's patch adds a bunch more, but doesn't remove any, is this a piece of a puzzle that'll lead to us being able to remove more, or what's the plan here?

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The second part is making nsINode::Clone not be const; that should allow us to remove the casts Mark is adding here.

Comment 9

[Mark Capella [:capella]]

Right ... we didn't want to do all those all at once ...

Comment 10

[Johnny Stenback (:jst)]

Ok. I wonder if we'd get away with less const_cast adding/removal if we do that piece first? If that turns out to be even worse, then I'm happy to land this patch as is (still want to read through it one more time), but if we can achieve the same result with less casting back n' fourth then I'd be in favor of that.

Comment 11

[Mark Capella [:capella]]

For my part I'm happy to try it out the other way around, if that's what ya'll decide. 

I was going on the assumption that we're working in the content/ folder where a rough scan (vs. the regex one above I was tinkering with) for "Const(" Found 270 matching lines in 181 files ... in all folders it Found 892 matching lines in 407 files ... both quite large numbers ...

Is this change constrained / smaller than the rough scan suggests? And will a change of such a large size be exposed to other ongoing change efforts? (multiple re-basings etc.?)

Comment 12

[Mark Capella [:capella]]

FYI, returning this one to the wild until interest returns...

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 617301 [details] [diff] [review]
Patch (v3

Sorry for letting this drop on the floor here. :(

I was just hesitant to take this patch given that it seems a step backwards, but given that it opens the door to move further forward I'm fine taking this as is. r=jst

Comment 14

[Mark Capella [:capella]]

Attachment: Patch (v4) - rebased

Lost track of this bug, didn't notice the approval till today ... this is updated / rebased ... I want to repush to TRY first, but can move this towards landing if the review+ is still valid -- mark

Comment 15

[Mark Capella [:capella]]

TRY:
https://tbpl.mozilla.org/?tree=Try&rev=14a50688c871

Comment 16

[Mark Capella [:capella]]

On to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4da377ae2e2f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4da377ae2e2f