Last Comment Bug 704311 - *Element::CopyInnerTo shouldn't be const
: *Element::CopyInnerTo shouldn't be const
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mark Capella [:capella]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-21 14:55 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-07-09 18:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) rough experiment (1.70 KB, patch)
2012-04-20 08:15 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (28.00 KB, patch)
2012-04-20 13:33 PDT, Mark Capella [:capella]
Ms2ger: feedback+
Details | Diff | Splinter Review
Patch (v3 (26.73 KB, patch)
2012-04-22 03:49 PDT, Mark Capella [:capella]
jst: review+
Details | Diff | Splinter Review
Patch (v4) - rebased (27.11 KB, patch)
2012-07-08 02:19 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-11-21 14:55:57 PST
Turns out that most implementations are just const_casting anyway. Same for its caller, nsINode::Clone.
Comment 1 Mark Capella [:capella] 2012-04-19 05:26:13 PDT
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] 2012-04-20 08:15:41 PDT
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 ...
Comment 3 Mark Capella [:capella] 2012-04-20 13:33:34 PDT
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.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-04-21 12:37:02 PDT
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] 2012-04-22 03:49:53 PDT
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 :)
Comment 6 Mark Capella [:capella] 2012-04-23 11:03:46 PDT
Comment on attachment 617301 [details] [diff] [review]
Patch (v3

Try results ...

https://tbpl.mozilla.org/?tree=Try&rev=d2207cd3fc20
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-24 16:58:42 PDT
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 (⌚ UTC+1/+2) 2012-04-25 00:08:12 PDT
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] 2012-04-26 22:16:13 PDT
Right ... we didn't want to do all those all at once ...
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2012-04-27 14:35:24 PDT
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] 2012-04-30 00:22:05 PDT
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] 2012-05-25 20:12:35 PDT
FYI, returning this one to the wild until interest returns...
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2012-05-26 13:30:05 PDT
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] 2012-07-08 02:19:02 PDT
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
Comment 15 Mark Capella [:capella] 2012-07-08 03:19:50 PDT
TRY:
https://tbpl.mozilla.org/?tree=Try&rev=14a50688c871
Comment 16 Mark Capella [:capella] 2012-07-09 05:38:44 PDT
On to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4da377ae2e2f
Comment 17 Ryan VanderMeulen [:RyanVM] 2012-07-09 18:04:57 PDT
https://hg.mozilla.org/mozilla-central/rev/4da377ae2e2f

Note You need to log in before you can comment on or make changes to this bug.