Remove unneeded casts for getting values from frame properties

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: xidorn, Assigned: bradwerth)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 10 obsolete attachments)

86.38 KB, patch
cbook
: checkin+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
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?
Reporter

Comment 2

3 years ago
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.
Reporter

Updated

3 years ago
Assignee: nobody → bugzilla
Reporter

Updated

3 years ago
Flags: needinfo?(bugzilla)
Reporter

Updated

3 years ago
Assignee: bugzilla → nobody
Flags: needinfo?(bugzilla)
Assignee

Updated

3 years ago
Assignee: nobody → bwerth
Reporter

Comment 3

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

Comment 4

3 years ago
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)
Assignee

Comment 6

3 years ago
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+
Assignee

Comment 8

3 years ago
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.
Assignee

Comment 12

3 years ago
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)
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Comment 13

3 years ago
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+
Assignee

Comment 19

3 years ago
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
Reporter

Comment 21

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

Comment 22

3 years ago
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)
Assignee

Comment 24

3 years ago
I have re-exported the patch via an hg export, which I believe produces the needed headers.
Attachment #8764072 - Attachment is obsolete: true
Assignee

Updated

3 years ago
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
Reporter

Updated

3 years ago
Flags: needinfo?(bwerth)
Assignee

Comment 28

3 years ago
Posted 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?
Assignee

Updated

3 years ago
Keywords: checkin-needed
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)
Assignee

Comment 30

3 years ago
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?

Comment 31

3 years ago
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+

Comment 32

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ed713102408e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.