*Element::CopyInnerTo shouldn't be const

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: capella)

Tracking

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Turns out that most implementations are just const_casting anyway. Same for its caller, nsINode::Clone.
(Assignee)

Comment 1

5 years ago
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)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 616974 [details] [diff] [review]
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 ...
Attachment #616974 - Flags: feedback?(Ms2ger)
(Assignee)

Comment 3

5 years ago
Created attachment 617078 [details] [diff] [review]
Patch (v2)

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)
(Reporter)

Comment 4

5 years ago
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.
Attachment #617078 - Flags: feedback?(Ms2ger) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 617301 [details] [diff] [review]
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 :)
Attachment #617078 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [good first bug][mentor=Ms2ger] → [autoland:-b do -p linux64 -u all -t none][good first bug][mentor=Ms2ger]
(Assignee)

Updated

5 years ago
Whiteboard: [autoland:-b do -p linux64 -u all -t none][good first bug][mentor=Ms2ger] → [good first bug][mentor=Ms2ger]
(Assignee)

Comment 6

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

Try results ...

https://tbpl.mozilla.org/?tree=Try&rev=d2207cd3fc20
Attachment #617301 - Flags: review?(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?
(Reporter)

Comment 8

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

Comment 9

5 years ago
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.
(Assignee)

Comment 11

5 years ago
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.?)
(Assignee)

Comment 12

5 years ago
FYI, returning this one to the wild until interest returns...
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
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+
(Assignee)

Comment 14

5 years ago
Created attachment 640038 [details] [diff] [review]
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
(Assignee)

Comment 15

5 years ago
TRY:
https://tbpl.mozilla.org/?tree=Try&rev=14a50688c871
(Assignee)

Comment 16

5 years ago
On to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4da377ae2e2f
Target Milestone: --- → mozilla16
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/4da377ae2e2f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.