1243559 - Remove unneeded casts for getting values from frame properties

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

In bug 1230034, we make frame property typesafe, thus the original static_cast<> thing is no longer needed for getting values from frame properties. The casts should be removed.

Comment 1

[David Baron :dbaron:]

Are there also some casts used in setting as well, or did bug 1230034 remove all of them?

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Hmmm, I don't think casts were needed for setting for pointers, since pointers can always be implicitly converted to void*. So if there is any, they are redundant even without bug 1230034.

Small values may use casts, but those should have been removed in bug 1230034 part 6, otherwise compiling would fail.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Basically you just need to check every callsite of FrameProperties::{Get,Set,Remove}.

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Removes static casts on received values from calls to FrameProperties::Get

Removes static casts from all the examples of FrameProperties::Get that I could find, except for an overloaded property descriptor in nsSVGEffects.cpp that needs more delicate attention.

I could not find any examples of casts in FrameProperties::Set other than the complicated cases in nsSVGEffects.cpp.

FrameProperties::Delete isn't passed a property point and therefore can't be using unnecessary static casts.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2143a923a035

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

I mentioned ::Remove, not ::Delete. That has at least one static_cast:
https://dxr.mozilla.org/mozilla-central/rev/5f95858f8ddf21ea2271a12810332efd09eff138/layout/generic/nsBlockFrame.cpp#5063

Comment 6

[Brad Werth [:bradwerth]]

Got it; sorry about that. I'll go after those in an updated patch.

Comment 7

[David Baron :dbaron:]

Comment on attachment 8763246 [details] [diff] [review]
Removes static casts on received values from calls to FrameProperties::Get

It would probably be nice to convert the first few changes in
nsFrame.cpp from "nsRect *rect" to "nsRect* rect" to match our
current style guidelines, although I won't insist that you mix
that change into this patch.

However, I do want you to change the three occurrences of
"auto positionedParts" in nsTableFrame.cpp to
"FrameTArray* positionedParts" since there's no longer an indication
of the type at the declaration.

r=dbaron with that, since these changes look good


I'm curious how you verified that this was all of the changes needed
to fix the bug.  (You probably do need to look at calls to Remove,
which you didn't mention in comment 4.)

Comment 8

[Brad Werth [:bradwerth]]

Attachment: Compiler-assisted removal of static casts on FrameProperties ::Get ::Set and ::Remove

The first patch found callsites via a regex, and missed some cases, as noted in the comments. For this patch, I enlisted the compiler and temporarily renamed the method signatures to force all callsites to be individually addressed.

These files USE STATIC CASTS and are updated in this patch:

layout/base/FrameLayerBuilder.cpp
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSRendering.cpp
layout/base/nsDisplayList.h
layout/base/nsLayoutUtils.cpp
layout/base/RestyleManager.cpp
layout/base/RestyleTracker.h
layout/forms/nsTextControlFrame.cpp
layout/generic/nsBlockFrame.h
layout/generic/nsBlockFrame.cpp
layout/generic/nsCanvasFrame.cpp
layout/generic/nsFlexContainerFrame.cpp
layout/generic/nsFloatManager.cpp
layout/generic/nsFontInflationData.cpp
layout/generic/nsFrame.cpp
layout/generic/nsHTMLReflowState.cpp
layout/generic/nsIFrame.h
layout/generic/nsGridContainerFrame.cpp
layout/generic/nsTextFrame.cpp
layout/generic/StickyScrollContainer.cpp
layout/mathml/nsMathMLContainerFrame.cpp
layout/svg/nsSVGIntegrationUtils.cpp
layout/svg/nsSVGUtils.cpp
layout/tables/nsTableFrame.cpp
layout/tables/nsTableRowFrame.cpp

These files contain callsites but HAVE NO STATIC CASTS and were left unchanged:

layout/base/ActiveLayerTracker.cpp
layout/base/nsBidi.h
layout/base/nsBidiPresUtils.cpp
layout/base/nsDisplayList.cpp
layout/forms/nsTextControlFrame.h
layout/generic/nsBulletFrame.cpp
layout/generic/nsCanvasFrame.h
layout/generic/nsContainerFrame.h
layout/generic/nsContainerFrame.cpp
layout/generic/nsGridContainerFrame.h
layout/generic/nsLineLayout.cpp
layout/generic/nsPlaceholderFrame.cpp
layout/generic/nsSelection.cpp
layout/generic/RubyUtils.cpp
layout/mathml/nsMathMLmtableFrame.cpp
layout/tables/nsTableRowGroupFrame.cpp
layout/xul/nsBox.cpp
layout/xul/nsMenuFrame.cpp

The following files STILL USE STATIC CASTS through some complex type relationships, which need more focused work to fix (possibly in another bug). These have been left unchanged:

layout/svg/nsSVGEffects.cpp
layout/svg/nsSVGFilterFrame.cpp
layout/svg/nsSVGGradientFrame.cpp
layout/svg/nsSVGPatternFrame.cpp
layout/svg/SVGTextFrame.cpp

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b3e5b3782fb

Comment 9

[David Baron :dbaron:]

Comment on attachment 8763712 [details] [diff] [review]
Compiler-assisted removal of static casts on FrameProperties ::Get ::Set and ::Remove

>diff --git a/layout/tables/nsTableRowFrame.cpp b/layout/tables/nsTableRowFrame.cpp
>-        nsMargin* computedOffsetProp = static_cast<nsMargin*>
>-          (kidFrame->Properties().Get(nsIFrame::ComputedOffsetProperty()));
>+        nsMargin* computedOffsetProp = kidFrame->Properties().Get(nsIFrame::ComputedOffsetProperty());

Please break after the = to avoid going past the 80th column.



(Also, in the future, probably better not to add in unrelated style cleanups, although there were only a few here.)



r=dbaron

Comment 10

[David Baron :dbaron:]

(In reply to Brad Werth [:bradwerth] from comment #8)
> The following files STILL USE STATIC CASTS through some complex type
> relationships, which need more focused work to fix (possibly in another
> bug). These have been left unchanged:
> 
> layout/svg/nsSVGEffects.cpp
> layout/svg/nsSVGFilterFrame.cpp
> layout/svg/nsSVGGradientFrame.cpp
> layout/svg/nsSVGPatternFrame.cpp
> layout/svg/SVGTextFrame.cpp

A bunch of these don't look difficult, but just require fixing some types in nsSVGEffects.h, I think.  GetEffectProperty in nsSVGEffects.cpp seems like the only difficult one.

Comment 11

[Brad Werth [:bradwerth]]

Attachment: Compiler-assisted removal of static casts on FrameProperties ::Get ::Set and ::Remove, updated with better line wrapping

Comment 12

[Brad Werth [:bradwerth]]

Attachment: Compiler-assisted removal of static casts on FrameProperties ::Get ::Set and ::Remove, updated with better line wrapping, updated with better line wrapping

Updated patch that was r+ dbaron, with additional formatting cleanup requested in comment 9.

Comment 13

[Brad Werth [:bradwerth]]

Attachment: Additive patch to rework SVG classes to replace generic nsISupports properties with correct types

This is additive to attachment 8763987 [details] [diff] [review].

Updates property definitions in nsSVGEffects.h to supply types for properties that were formerly nsISupports.

Comment 14

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c34fa9f9db32

Comment 15

[David Baron :dbaron:]

Comment on attachment 8763987 [details] [diff] [review]
Compiler-assisted removal of static casts on FrameProperties ::Get ::Set and ::Remove, updated with better line wrapping, updated with better line wrapping

You didn't actually need to request review again on this one.

Comment 16

[David Baron :dbaron:]

Comment on attachment 8763993 [details] [diff] [review]
Additive patch to rework SVG classes to replace generic nsISupports properties with correct types

Please rename:
  TextPathProperty -> HrefAsTextPathProperty
  PaintingProperty -> HrefAsPaintingProperty

Could you make GetEffectProperty into a function template instead
of manually inlining it three times into the three callers?  It seems
like it could be templatized over the property type.  (You still
don't need the create functions.)

Otherwise I think this looks good, but I'd like to look at the revisions.

Comment 17

[Brad Werth [:bradwerth]]

Attachment: Additive patch to rework SVG classes to replace generic nsISupports properties with correct types, with corrected property names and templatized access methods

This is additive to attachment 8763987 [details] [diff] [review] and addresses the issues raised in comment 16.

Comment 18

[David Baron :dbaron:]

Comment on attachment 8764023 [details] [diff] [review]
Additive patch to rework SVG classes to replace generic nsISupports properties with correct types, with corrected property names and templatized access methods

r=dbaron

(We should probably also clean up GetEffectProperty and its callers to use already_AddRefed<T> rather than returning AddRef'd raw pointers, but that would definitely be a separate patch.)

Comment 19

[Brad Werth [:bradwerth]]

Attachment: Comprehensive patch ready for checkin

All r+ patches rolled into this one, ready for checkin.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Doesn't apply cleanly to inbound tip.

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Brad Werth [:bradwerth] from comment #19)
> Created attachment 8764055 [details] [diff] [review]
> Comprehensive patch ready for checkin
> 
> All r+ patches rolled into this one, ready for checkin.

I would suggest you not to roll patches into one, unless you have good reason (e.g. the patch doesn't work on its own, was split just for easier review). Reason is that we may need to read commits again in the future, and a finer granularity would benefit not only the current review, but also future readers.

Comment 22

[Brad Werth [:bradwerth]]

Attachment: Comprehensive patch rebased to current tip

Rebased to current tip and it applies for me. If it fails for you, is there a way to share with me some info on the failure so I can address?

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8764072 [details] [diff] [review]
Comprehensive patch rebased to current tip

For future reference, just resetting checkin-needed would have sufficed. Also, this patch is lacking in a commit message and metadata such as your name. Please fix.
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 24

[Brad Werth [:bradwerth]]

Attachment: Comprehensive patch rebased to current tip, with appropriate headers

I have re-exported the patch via an hg export, which I believe produces the needed headers.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bb00282a4c2

Comment 26

[Phil Ringnalda (:philor)]

See all those orange debug jobs on your try run, the ones that failed from "Assertion failure: aFrame->GetType() == nsGkAtoms::svgPathGeometryFrame && static_cast<nsSVGPathGeometryElement*>(aFrame->GetContent())->IsMarkable() (Bad frame), at /home/worker/workspace/build/src/layout/svg/nsSVGEffects.cpp:529"?

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dd0771829dc90fdc6c9d19f4884ebc7c340f3beb

Comment 27

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c4a7f34e3c4

Comment 28

[Brad Werth [:bradwerth]]

Attachment: noStatic2.patch

Updated patch with assert issue resolved (moved to correct place), and a patch header noting r=dbaron.

Comment 29

[Carsten Book [:Tomcat]]

has problems to apply:

sing... done
adding 1243559 to series file
renamed 1243559 -> noStatic2.patch
applying noStatic2.patch
patching file layout/svg/nsSVGEffects.cpp
Hunk #1 FAILED at 479
Hunk #2 FAILED at 499
2 out of 2 hunks FAILED -- saving rejects to file layout/svg/nsSVGEffects.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh noStatic2.patch

Comment 30

[Brad Werth [:bradwerth]]

Attachment: noStatic3.patch

Built the patch to apply the changesets in the proper order; confirmed that this can import cleanly.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed713102408e
Removes static casts from calls to FrameProperties::Get, ::Set, and ::Remove, and forces callers to use the type associated with the property. r=dbaron

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed713102408e