Closed Bug 556446 Opened 14 years ago Closed 14 years ago

Dead Code in Layout

Categories

(Core :: Layout, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4

People

(Reporter: ehren.m, Assigned: ehren.m)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Patch v1 (obsolete) — Splinter Review
These functions are dead (I can refile based on submodule if this is too much for one patch):

In layout/xul:

void nsBoxFrame::FireDOMEventSynch(const nsAString_internal&,nsIContent*) 
void nsGridRow::MarkDirty(nsBoxLayoutState&)
void nsBoxLayout::GetParentLayout(nsIBox*,nsIBoxLayout**)
nsGridRow* nsGrid::GetRows()
nsGridRow* nsGrid::GetColumns()
nsresult nsMenuFrame::IsActive(PRBool&)
void nsBox::AddMargin(nsSize&)

layout/base:

nsIFrame* nsLayoutUtils::GetClosestCommonAncestorViaPlaceholders(nsIFrame*,nsIFrame*,nsIFrame*)
nsBidi::nsBidi(PRUint32,PRUint32)
nsIFrame* nsFrameManager::GetCanvasFrame()
nsresult nsBidiPresUtils::GetBidiEngine(nsBidi**)
void nsDisplayList::DeleteBottom()
void nsFrameManager::ClearUndisplayedContentMap()
PRInt32 nsIPresShell::GetVerifyReflowFlags()
PRBool PresShell::nsDelayedEvent::Equals(nsPIDOMEventTarget*,PRUint32)

layout/style:

nsresult CSSLoaderImpl::StopLoadingSheet(nsIURI*)
StopLoadingSheetsByURIClosure::StopLoadingSheetsByURIClosure(nsIURI*,nsTArray&)
PRBool langMatches(RuleProcessorData&,PRBool,nsPseudoClassList*)
void nsCSSCornerSizes::SetAllCornersTo(const nsCSSValue&)
nsresult CSSLoaderImpl::GetPreferredSheet(nsAString_internal&)
nsCSSValue::nsCSSValue(nsCSSValueGradient*)
nsCSSValue::nsCSSValue(Image*)
nsCSSValue::nsCSSValue(URL*)
nsCSSValue::nsCSSValue(Array*,nsCSSUnit)
nsCSSValue::nsCSSValue(float,nsCSSUnit)
nsresult nsHTMLStyleSheet::GetLinkColor(nscolor&)
nsresult nsHTMLStyleSheet::GetActiveLinkColor(nscolor&)
nsresult nsHTMLStyleSheet::GetVisitedLinkColor(nscolor&)
PRBool nsCSSPseudoClasses::IsPseudoClass(nsIAtom*)
void nsCSSValue::SetRectIsAutoValue()
nsresult nsCSSDeclaration::AppendComment(const nsAString_internal&)

layout/generic:

void nsFrameList::SortByContentOrder()
nsIFrame* nsBlockFrame::GetTopBlockChild(nsPresContext*)
PRInt32 nsILineIterator::FindLineAt(nscoord)
PRInt32 nsLineIterator::FindLineAt(nscoord)
PRInt32 nsTableRowGroupFrame::FindLineAt(nscoord)
nsLineBox* nsLineBox::FindLineContaining(nsLineList&,nsIFrame*,PRInt32*)
PRBool nsFrameList::ContainsFrameBefore(const nsIFrame*,const nsIFrame*)
nsFramesetDrag::nsFramesetDrag(PRBool,PRInt32,PRInt32,nsHTMLFramesetFrame*)
void Area::GetHREF(nsAString_internal&)
nsRect nsIFrame::GetMarginRect()
PRBool nsIntervalSet::HasPoint(nscoord)
nsresult nsIFrame::Clip(nsDisplayListBuilder*,const nsDisplayListSet&,const nsDisplayListSet&,const nsRect&)
nsRect nsIFrame::GetScreenRectInAppUnitsExternal()

layout/tables:

void nsTableOuterFrame::BalanceLeftRightCaption(PRUint8,const nsMargin&,const nsMargin&,nscoord&,nscoord&)
PRBool nsCellMap::ColHasSpanningCells(PRInt32)
PRBool nsTableFrame::ColHasSpanningCells(PRInt32)
PRBool nsTableCellMap::ColHasSpanningCells(PRInt32)
PRBool nsTableCellMap::ColIsSpannedInto(PRInt32)
PRBool nsTableFrame::ColIsSpannedInto(PRInt32)
PRBool nsCellMap::IsZeroColSpan(PRInt32,PRInt32)
void nsTableFrame::SetColumnWidth(PRInt32,nscoord)
PRBool nsTableFrame::IsAutoWidth(PRBool*)
PRInt32 nsTableFrame::GetEffectiveCOLSAttribute()
PRBool nsTableFrame::IsPctHeight(nsStyleContext*)

layout/forms:

nsresult nsComboboxControlFrame::GetOptionSelected(PRInt32,PRBool*)
nsresult nsListControlFrame::GetOptionSelected(PRInt32,PRBool*)
nsresult nsISelectControlFrame::GetOptionSelected(PRInt32,PRBool*)
nsMargin nsButtonFrameRenderer::GetFullButtonBorderAndPadding()
nsMargin nsButtonFrameRenderer::GetButtonOutlineBorderAndPadding()

layout/mathml:

nsresult nsMathMLFrame::SetEmbellishData(const nsEmbellishData&)
nsresult nsMathMLFrame::SetEmbellishData(const nsEmbellishData&)
nsresult nsMathMLFrame::GetReference(nsPoint&)
nsresult nsIMathMLFrame::GetReference(nsPoint&)
nsresult nsMathMLFrame::SetPresentationData(const nsPresentationData&)
nsresult nsIMathMLFrame::SetPresentationData(const nsPresentationData&)

This wiki page has links to MXR searched for these functions: http://zenit.senecac.on.ca/wiki/index.php/User:Egmetcalfe/Dead_Code#layout.2Fxul
Attachment #436391 - Flags: review?(roc)
Blocks: deadcode
Comment on attachment 436391 [details] [diff] [review]
Patch v1

Let's keep GetScreenRectInAppUnitsExternal. Someone outside our tree might use it :-(. r+ if you put it back.

Is StopLoadingSheetsByURIClosure used at all now? I suppose you're not detecting unused types, variables or fields yet?
Attachment #436391 - Flags: review?(roc) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
Ok, GetScreenRectInAppUnitsExternal is back in. I'm not really looking for types atm but StopLoadingSheetsByURIClosure is still used by StopLoadingSheetByURICallback: http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSLoader.cpp#2362
Attachment #436391 - Attachment is obsolete: true
Attachment #436398 - Flags: review+
Attachment #436398 - Flags: review+ → review?(roc)
I'd sort of like to keep the nsCSSValue methods being removed so that it always has both a constructor and a setter for each type, even if we're currently only using one of them.
Oh, and if nsCSSPseudoClasses::IsPseudoClass isn't being used, that's a bug, and I think a recently-introduced one.  It should stay, and we need to figure out why it's not being called.
(In reply to comment #4)
> Oh, and if nsCSSPseudoClasses::IsPseudoClass isn't being used, that's a bug,
> and I think a recently-introduced one.  It should stay, and we need to figure
> out why it's not being called.

Oh, actually, never mind... the caller is now using nsCSSPseudoClasses::GetPseudoType.
Comment on attachment 436398 [details] [diff] [review]
Patch v2

Better address comment #3.

Maybe we should have an attribute for static analysis that says "treat this as used", so that we can reanalyze later and we won't try to remove those methods again?
Attachment #436398 - Flags: review?(roc) → review-
Attached patch patch v3 (obsolete) — Splinter Review
The constructors are back in.

Also it turned out that StopLoadingSheetByURICallback is dead too (not sure how my script missed it). That means StopLoadingSheetsByURIClosure can go as well.
Attachment #436398 - Attachment is obsolete: true
Attachment #436412 - Flags: review?(roc)
Comment on attachment 436412 [details] [diff] [review]
patch v3

very nice!!!
Attachment #436412 - Flags: review?(roc) → review+
(In reply to comment #6)
> Maybe we should have an attribute for static analysis that says "treat this as
> used", so that we can reanalyze later and we won't try to remove those methods
> again?

In that case, what I'd really want for nsCSSValue would be something that says "if one of these two functions is used, treat both as used", since I'd like to maintain the correspondence between setters and constructors, but I'd still want to know if an entire value type had become unused.  But that sounds like a lot of work for something pretty small.
Keywords: checkin-needed
> nsresult CSSLoaderImpl::StopLoadingSheet(nsIURI*)

I believe we want this method around; we need to stop some sheet loads and just aren't doing it yet.  We have bugs on that....
Attached patch patch v4 (obsolete) — Splinter Review
StopLoadingSheetsByURIClosure, StopLoadingSheet, and StopLoadingSheetByURICallback are back in.

Also while we're at it I wonder if I can sneak in a tiny bug fix:

In layout/xul, void nsBox::CoordNeedsRecalc(PRInt32&) and void nsListBoxBodyFrame::SetRowHeight(nscoord) both register as dead. In reality their parameter types are incorrect. CoordNeedsRecalc is declared as taking an ncoord& parameter but defined as PRInt32&. SetRowHeight is defined with nscoord but declared with PRInt32. This will break the build if NS_COORD_IS_FLOAT is defined.
Attachment #436412 - Attachment is obsolete: true
Attachment #436424 - Flags: review?(roc)
Personally, I'm comfortable with removing code that might be used in the future but currently isn't. We can always get it out of mozilla-central when the time comes.
We can if we know to look for it....  hg's support for "find me the revision that had the code that isn't here anymore" is not so great.

I suppose we could remove it and replace with a comment that says that it used to be there; the blame on that comment will then point to the right thing?
Assignee: nobody → ehren.m
Comment on attachment 436424 [details] [diff] [review]
patch v4

Awesome work. :)

Is this one still good to land? I'll push it if so!
regarding comment #13, I could post another patch with the Sheet stuff replaced with something like: 

/* nsresult Loader::StopLoadingSheet(nsIURI* aURL), which notifies the nsICSSLoaderObserver with NS_BINDING_ABORTED, was removed in Bug 556446. It can be found in revision xxxxx */

Or we could just push this and leave that for another bug if it is desirable.
(In reply to comment #15)
> /* nsresult Loader::StopLoadingSheet(nsIURI* aURL), which notifies the
> nsICSSLoaderObserver with NS_BINDING_ABORTED, was removed in Bug 556446. It can
> be found in revision xxxxx */

Sounds great to me.
Attached patch patch v5Splinter Review
Attachment #436424 - Attachment is obsolete: true
Attachment #436565 - Flags: review?(bzbarsky)
Keywords: checkin-needed
Whiteboard: [needs r=bz]
Comment on attachment 436565 [details] [diff] [review]
patch v5

r=me on cssloader changes.  Didn't look at the rest
Attachment #436565 - Flags: review?(bzbarsky) → review+
Whiteboard: [needs r=bz] → checkin-needed
http://hg.mozilla.org/mozilla-central/rev/13504a5fc074
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed
Target Milestone: --- → mozilla1.9.3a4
Blocks: 738705
You need to log in before you can comment on or make changes to this bug.