Closed Bug 870516 Opened 7 years ago Closed 7 years ago

add more MOZ_OVERRIDE in layout

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Six, Assigned: Six)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Add ~200 MOZ_OVERRIDE in layout/ (obsolete) — Splinter Review
This patch adds ~200 more MOZ_OVERRIDE in layout/
Attachment #747565 - Flags: review?(dholbert)
Comment on attachment 747565 [details] [diff] [review]
Add ~200 MOZ_OVERRIDE in layout/

>diff --git a/layout/mathml/nsMathMLContainerFrame.h b/layout/mathml/nsMathMLContainerFrame.h
>--- a/layout/mathml/nsMathMLContainerFrame.h
>+++ b/layout/mathml/nsMathMLContainerFrame.h
>   NS_IMETHOD
>   DidReflow(nsPresContext*           aPresContext,
>             const nsHTMLReflowState*  aReflowState,
>             nsDidReflowStatus         aStatus)
>-
>+ MOZ_OVERRIDE
>   {
>     mPresentationData.flags &= ~NS_MATHML_STRETCH_DONE;
>     return nsContainerFrame::DidReflow(aPresContext, aReflowState, aStatus);
>   }


This one ^^ needs fixing.

The fact that there's a blank line there is odd, but the MOZ_OVERRIDE should go after the ")", not on the blank line.

(still reading, more comments / r+ coming soon)
(If you feel like it, feel free to delete that blank line while you're fixing that chunk.)
Comment on attachment 747565 [details] [diff] [review]
Add ~200 MOZ_OVERRIDE in layout/

Assuming your script is attempting to catch all of the cases...

>diff --git a/layout/mathml/nsMathMLmrowFrame.h b/layout/mathml/nsMathMLmrowFrame.h
>--- a/layout/mathml/nsMathMLmrowFrame.h
>+++ b/layout/mathml/nsMathMLmrowFrame.h
>@@ -21,17 +21,17 @@ public:
>   friend nsIFrame* NS_NewMathMLmrowFrame(nsIPresShell* aPresShell, nsStyleContext* aContext);
> 
>   NS_IMETHOD
>   AttributeChanged(int32_t  aNameSpaceID,
>                    nsIAtom* aAttribute,
>                    int32_t  aModType);

Why isn't this one ^^ marked as MOZ_OVERRIDE?

>diff --git a/layout/mathml/nsMathMLmstyleFrame.h b/layout/mathml/nsMathMLmstyleFrame.h
>--- a/layout/mathml/nsMathMLmstyleFrame.h
>+++ b/layout/mathml/nsMathMLmstyleFrame.h
>@@ -21,17 +21,17 @@ public:
>   friend nsIFrame* NS_NewMathMLmstyleFrame(nsIPresShell* aPresShell, nsStyleContext* aContext);
> 
>   NS_IMETHOD
>   AttributeChanged(int32_t         aNameSpaceID,
>                    nsIAtom*        aAttribute,
>                    int32_t         aModType);

or this one^^ ?

>diff --git a/layout/mathml/nsMathMLmsubsupFrame.h b/layout/mathml/nsMathMLmsubsupFrame.h
>   NS_IMETHOD
>   TransmitAutomaticData();

Or this one ^^

>diff --git a/layout/mathml/nsMathMLmsupFrame.h b/layout/mathml/nsMathMLmsupFrame.h
>--- a/layout/mathml/nsMathMLmsupFrame.h
>+++ b/layout/mathml/nsMathMLmsupFrame.h
>   NS_IMETHOD
>   TransmitAutomaticData();

Or this one ^^

>diff --git a/layout/mathml/nsMathMLmtableFrame.h b/layout/mathml/nsMathMLmtableFrame.h
>   NS_IMETHOD
>   SetInitialChildList(ChildListID  aListID,
>                       nsFrameList& aChildList);

Or this one^^

>   NS_IMETHOD
>   AttributeChanged(int32_t  aNameSpaceID,
>                    nsIAtom* aAttribute,
>                    int32_t  aModType);

or several of these ^^ in nsMathMLmtableFrame.h (for the classes nsMathMLmtrFrame and nsMathMLmtdFrame at least)

>@@ -242,17 +242,17 @@ public:
>   }
> 
>   NS_IMETHOD
>   Reflow(nsPresContext*          aPresContext,
>          nsHTMLReflowMetrics&     aDesiredSize,
>          const nsHTMLReflowState& aReflowState,
>          nsReflowStatus&          aStatus);

or this ^^, still in nsMathMLmtableFrame.h (for the class nsMathMLmtdInnerFrame)

>diff --git a/layout/mathml/nsMathMLTokenFrame.h b/layout/mathml/nsMathMLTokenFrame.h
>--- a/layout/mathml/nsMathMLTokenFrame.h
>+++ b/layout/mathml/nsMathMLTokenFrame.h
>   NS_IMETHOD
>   InheritAutomaticData(nsIFrame* aParent);

or this ^^

>diff --git a/layout/mathml/nsMathMLmunderoverFrame.h b/layout/mathml/nsMathMLmunderoverFrame.h
>--- a/layout/mathml/nsMathMLmunderoverFrame.h
>+++ b/layout/mathml/nsMathMLmunderoverFrame.h
>@@ -21,17 +21,17 @@ public:
>   friend nsIFrame* NS_NewMathMLmunderoverFrame(nsIPresShell* aPresShell, nsStyleContext* aContext);
> 
>   virtual nsresult
>   Place(nsRenderingContext& aRenderingContext,
>         bool                 aPlaceOrigin,
>         nsHTMLReflowMetrics& aDesiredSize);

or this ^^ (which is marked as MOZ_OVERRIDE in other classes in your patch)


Generally: anything with NS_IMETHOD *should* be MOZ_OVERRIDE.  If adding MOZ_OVERRIDE on any of those make it fail to build, then something's wrong -- that would mean the method has been orphaned -- and we'd definitely want to track that in a followup.

Having said that: this doesn't necessarily *have* to catch all of the above cases -- some stuff marked as MOZ_OVERRIDE is still a step in the right direction.  But it seems like you're trying to tailor your script such that it would catch most-or-all cases (assuming they aren't orphaned methods), and I'm curious why it's not catching these ones.
A few more things that should be MOZ_OVERRIDE that I ran across:

* In http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSRules.h , I'd expect that the methods under...
   // Rule methods
...and...
   // nsIStyleRule methods
...would be safe to mark as MOZ_OVERRIDE (but the patch doesn't currently do so).

* In nsTableCellFrame.h, I think "NS_IMETHOD GetCellIndexes()" should be safe to mark as MOZ_OVERRIDE
 (it's declared here:
 http://mxr.mozilla.org/mozilla-central/source/layout/tables/nsITableCellLayout.h#23
)

* In nsTableOuterFrame.h, GetContentInsertionFrame should be MOZ_OVERRIDE

* In nsTableRowGroupFrame.h, GetUsedBorder() and GetUsedMargin() and GetUsedPadding() should be MOZ_OVERRIDE.  (also in nsTableCellFrame.h)
* In layout/xul/base/src/nsMenuPopupFrame.h, GetCurrentMenuItem should be MOZ_OVERRIDE.

* In layout/xul/grid/nsGridRowGroupLayout.h, GetMinSize and GetPrefSize should be MOZ_OVERRIDE (looks like they override methods in nsSprocketLayout, in the grandparent class)

* ...and in that same file: CastToRowGroupLayout should be MOZ_OVERRIDE (it overrides a method in the parent class)

* In layout/xul/grid/nsGridRowLeafLayout.h: GetPrefSize, GetMinSize, ChildAddedOrRemoved should be MOZ_OVERRIDE
Having said all that: The attached patch could also be fine on its own, with comment 1 addressed. I'll leave it up to you whether you want to investigate the rest of my comments in a followup patch vs. all in the same patch.
I resolved all the missing other than thoses:

(In reply to Daniel Holbert [:dholbert] from comment #4)

> 
> * In nsTableCellFrame.h, I think "NS_IMETHOD GetCellIndexes()" should be
> safe to mark as MOZ_OVERRIDE
>  (it's declared here:
>  http://mxr.mozilla.org/mozilla-central/source/layout/tables/
> nsITableCellLayout.h#23
> )
> 
> * In nsTableRowGroupFrame.h, GetUsedMargin() should be MOZ_OVERRIDE.  (also in nsTableCellFrame.h)

They are due to things like that in class declarations:
>public:
>  NS_DECL_QUERYFRAME_TARGET(nsTableCellFrame)
>  NS_DECL_QUERYFRAME
>  NS_DECL_FRAMEARENA_HELPERS

And it looks complicated to ignore thoses macros.
I will send thoses errors to senex who develop the cppheaderparser to see if he can help again.
Attachment #747565 - Attachment is obsolete: true
Attachment #747565 - Flags: review?(dholbert)
Attachment #748422 - Flags: review?(dholbert)
This one is good and take away all last comments
Attachment #748422 - Attachment is obsolete: true
Attachment #748422 - Flags: review?(dholbert)
Attachment #748424 - Flags: review?(dholbert)
Assignee: nobody → six.dsn
Status: NEW → ASSIGNED
Summary: add ~200 MOZ_OVERRIDE in layout → add more MOZ_OVERRIDE in layout
Comment on attachment 748424 [details] [diff] [review]
Adds ~600 MOZ_OVERRIDE to layout/ with all last fix

Looks great!

r=me
Attachment #748424 - Flags: review?(dholbert) → review+
Thanks :)

The last issue is based on macro and needs manual intervention,

if there is something like NS_DECL_FRAMEARENA_HELPERS
in front of a method, we must add this macro to the to_ignore keyword list in my script.
Then this macro won't be taken as a part of the method signature.
Cool. Could we track part that in a followup bug? (And is it all right with you if I land your existing patch as-is, with a few minor bitrot fixes?)
To clarify [after talking to Arnaud on IRC]: he was saying that his script can be thwarted by macros that happen to fall before function signatures like...
> NS_DECL_FRAMEARENA_HELPERS
>
> nsIAtom* SomeMethodThatOverrides();
...where it can't tell whether the macro is part of the method-signature or not. (e.g. NS_IMETHOD in a similar position *would* be part of the signature, but NS_DECL_FRAMEARENA_HELPERS is *not*).  And this could prevent it from recognizing MOZ_OVERRIDE opportunities.

He added NS_DECL_FRAMEARENA_HELPERS to a blacklist, so his script will ignore that particular macro and it won't cause a problem. But -- he was saying that there may be *other* macros that cause the same problem which he didn't blacklist.

Anyway - if we run across any such latent MOZ_OVERRIDE opportunities, we can fix them in a followup bug. For now, I'm going to go ahead and land this (after giving it a sanity-check cross-platform Try run).
(In reply to Daniel Holbert [:dholbert] from comment #12)
> (And is it all right with
> you if I land your existing patch as-is, with a few minor bitrot fixes?)

For the record, the bitrot fixes are from functions that your patch marks as MOZ_OVERRIDE being removed out from under you.

In particular, this cset:
 http://hg.mozilla.org/mozilla-central/rev/9c42b49f5cac
removed StyleChangeReflow() from nsPresShell.h. So we don't need to mark that as MOZ_OVERRIDE now.

And this cset:
  https://hg.mozilla.org/mozilla-central/rev/9354202c68f0
did the following things:
 a) copied nsMathMLmactionFrame.h to nsMathMLSelectedFrame.h and marked TransmitAutomaticData as MOZ_OVERRIDE in the new copy (good)
 b) Removed TransmitAutomaticData from nsMathMLmactionFrame.h entirely
 c) Removed TransmitAutomaticData from nsMathMLsemanticsFrame.h entirely
So the chunks of this patch that tweaked nsMathMLmactionFrame.h and nsMathMLsemanticsFrame.h are no longer relevant and can be removed, since they were just marking that now-removed method as MOZ_OVERRIDE.
Try run, with bitrot-fixed patch applied on top of current mozilla-central tip:
  https://tbpl.mozilla.org/?tree=Try&rev=9f92342c1a48
Looks pretty green :)
https://hg.mozilla.org/mozilla-central/rev/233ee2f1efbc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 875367
Blocks: 875367
No longer depends on: 875367
You need to log in before you can comment on or make changes to this bug.