Closed Bug 1243559 Opened 5 years ago Closed 4 years ago

Remove unneeded casts for getting values from frame properties

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: xidorn, Assigned: bradwerth)

References

Details

Attachments

(1 file, 10 obsolete files)

86.38 KB, patch
cbook
: checkin+
Details | Diff | Splinter Review
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.
Summary: Remove unneeded casts of values got from frame properties → Remove unneeded casts for getting values from frame properties
Are there also some casts used in setting as well, or did bug 1230034 remove all of them?
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.
Assignee: nobody → bugzilla
Flags: needinfo?(bugzilla)
Assignee: bugzilla → nobody
Flags: needinfo?(bugzilla)
Assignee: nobody → bwerth
Basically you just need to check every callsite of FrameProperties::{Get,Set,Remove}.
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
Attachment #8763246 - Flags: review?(dbaron)
Got it; sorry about that. I'll go after those in an updated patch.
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.)
Attachment #8763246 - Flags: review?(dbaron) → review+
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
Attachment #8763246 - Attachment is obsolete: true
Attachment #8763712 - Flags: review?(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
Attachment #8763712 - Flags: review?(dbaron) → review+
(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.
Updated patch that was r+ dbaron, with additional formatting cleanup requested in comment 9.
Attachment #8763986 - Attachment is obsolete: true
Attachment #8763986 - Flags: review?(dbaron)
Attachment #8763987 - Flags: review?(dbaron)
This is additive to attachment 8763987 [details] [diff] [review].

Updates property definitions in nsSVGEffects.h to supply types for properties that were formerly nsISupports.
Attachment #8763993 - Flags: review?(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.
Attachment #8763987 - Flags: review?(dbaron) → review+
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.
Attachment #8763993 - Flags: review?(dbaron) → review-
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.)
Attachment #8764023 - Flags: review?(dbaron) → review+
All r+ patches rolled into this one, ready for checkin.
Attachment #8763987 - Attachment is obsolete: true
Attachment #8764023 - Attachment is obsolete: true
Doesn't apply cleanly to inbound tip.
Flags: needinfo?(bwerth)
Keywords: checkin-needed
(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.
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?
Attachment #8764055 - Attachment is obsolete: true
Flags: needinfo?(bwerth) → needinfo?(ryanvm)
Attachment #8764072 - Flags: checkin?(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
Flags: needinfo?(ryanvm) → needinfo?(bwerth)
Attachment #8764072 - Flags: checkin?(ryanvm)
I have re-exported the patch via an hg export, which I believe produces the needed headers.
Attachment #8764072 - Attachment is obsolete: true
Flags: needinfo?(bwerth)
Keywords: checkin-needed
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
Flags: needinfo?(bwerth)
Attached patch noStatic2.patch (obsolete) — Splinter Review
Updated patch with assert issue resolved (moved to correct place), and a patch header noting r=dbaron.
Attachment #8764347 - Attachment is obsolete: true
Flags: needinfo?(bwerth)
Attachment #8765064 - Flags: checkin?
Attachment #8765064 - Flags: checkin?
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
Flags: needinfo?(bwerth)
Attached patch noStatic3.patchSplinter Review
Built the patch to apply the changesets in the proper order; confirmed that this can import cleanly.
Attachment #8765064 - Attachment is obsolete: true
Flags: needinfo?(bwerth)
Attachment #8765927 - Flags: checkin?
Pushed by cbook@mozilla.com:
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
Attachment #8765927 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/ed713102408e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.