870516 - add more MOZ_OVERRIDE in layout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Arnaud Sourioux [:Six]]

Attachment: Add ~200 MOZ_OVERRIDE in layout/

This patch adds ~200 more MOZ_OVERRIDE in layout/

Comment 1

[Daniel Holbert [: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)

Comment 2

[Daniel Holbert [:dholbert]]

(If you feel like it, feel free to delete that blank line while you're fixing that chunk.)

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

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)

Comment 5

[Daniel Holbert [:dholbert]]

* 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

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Arnaud Sourioux [:Six]]

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.

Comment 8

[Arnaud Sourioux [:Six]]

Attachment: Adds ~600 MOZ_OVERRIDE to layout/ with last fix

Comment 9

[Arnaud Sourioux [:Six]]

Attachment: Adds ~600 MOZ_OVERRIDE to layout/ with all last fix

This one is good and take away all last comments

Comment 10

[Daniel Holbert [:dholbert]]

Comment on attachment 748424 [details] [diff] [review]
Adds ~600 MOZ_OVERRIDE to layout/ with all last fix

Looks great!

r=me

Comment 11

[Arnaud Sourioux [:Six]]

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.

Comment 12

[Daniel Holbert [:dholbert]]

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?)

Comment 13

[Daniel Holbert [:dholbert]]

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).

Comment 14

[Daniel Holbert [:dholbert]]

(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.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: same patch (with ~600 MOZ_OVERRIDE), bitrot-fixed

Comment 16

[Daniel Holbert [:dholbert]]

Try run, with bitrot-fixed patch applied on top of current mozilla-central tip:
  https://tbpl.mozilla.org/?tree=Try&rev=9f92342c1a48

Comment 17

[Arnaud Sourioux [:Six]]

Looks pretty green :)

Comment 18

[Daniel Holbert [:dholbert]]

Yup! Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/233ee2f1efbc

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/233ee2f1efbc