Dead Code in Layout

RESOLVED FIXED in mozilla1.9.3a4

Status

()

Core
Layout
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Ehren Metcalfe, Assigned: Ehren Metcalfe)

Tracking

Trunk
mozilla1.9.3a4
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 436391 [details] [diff] [review]
Patch v1

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
(Assignee)

Updated

7 years ago
Attachment #436391 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Blocks: 457262
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+
(Assignee)

Comment 2

7 years ago
Created attachment 436398 [details] [diff] [review]
Patch v2

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+
(Assignee)

Updated

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

Comment 7

7 years ago
Created attachment 436412 [details] [diff] [review]
patch v3

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.

Updated

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

Comment 11

7 years ago
Created attachment 436424 [details] [diff] [review]
patch v4

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)
Attachment #436424 - Flags: review?(roc) → review+
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?

Updated

7 years ago
Assignee: nobody → ehren.m

Comment 14

7 years ago
Comment on attachment 436424 [details] [diff] [review]
patch v4

Awesome work. :)

Is this one still good to land? I'll push it if so!
(Assignee)

Comment 15

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

Comment 17

7 years ago
Created attachment 436565 [details] [diff] [review]
patch v5
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+
(Assignee)

Updated

7 years ago
Whiteboard: [needs r=bz] → checkin-needed
http://hg.mozilla.org/mozilla-central/rev/13504a5fc074
Status: NEW → RESOLVED
Last Resolved: 7 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.