Closed Bug 704311 Opened 9 years ago Closed 8 years ago

*Element::CopyInnerTo shouldn't be const


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

Not set





(Reporter: Ms2ger, Assigned: capella)




(Whiteboard: [good first bug][mentor=Ms2ger])


(2 files, 2 obsolete files)

Turns out that most implementations are just const_casting anyway. Same for its caller, nsINode::Clone.
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
Assignee: nobody → markcapella
Attached patch Patch (v1) rough experiment (obsolete) — Splinter Review
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 ...
Attachment #616974 - Flags: feedback?(Ms2ger)
Attached patch Patch (v2) (obsolete) — Splinter Review
Ok, cool! This built and tested ok locally ... got some strange errors involving 'satchel' tests ... next step push-to-try I guess.
Attachment #616974 - Attachment is obsolete: true
Attachment #616974 - Flags: feedback?(Ms2ger)
Attachment #617078 - Flags: feedback?(Ms2ger)
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);


::: 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);

Attachment #617078 - Flags: feedback?(Ms2ger) → feedback+
Attached patch Patch (v3Splinter Review
Cool, yah you were right ... overkill on my part. This was rebuilt and retested locally ... same "toolkit/component/satchel issues ... else looks ok :)
Attachment #617078 - Attachment is obsolete: true
Whiteboard: [good first bug][mentor=Ms2ger] → [autoland:-b do -p linux64 -u all -t none][good first bug][mentor=Ms2ger]
Whiteboard: [autoland:-b do -p linux64 -u all -t none][good first bug][mentor=Ms2ger] → [good first bug][mentor=Ms2ger]
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?
The second part is making nsINode::Clone not be const; that should allow us to remove the casts Mark is adding here.
Right ... we didn't want to do all those all at once ...
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.
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.?)
FYI, returning this one to the wild until interest returns...
Assignee: markcapella → nobody
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
Attachment #617301 - Flags: review?(jst) → review+
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
On to inbound:
Target Milestone: --- → mozilla16
Assignee: nobody → markcapella
Closed: 8 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.