856822 - Mark overriding methods as MOZ_OVERRIDE in /content

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

See description of MOZ_OVERRIDE in bug 733186 comment 0.

Filing this bug on marking overriding methods in nsHTMLDocument and XMLDocument as MOZ_OVERRIDE.  (And maybe doing the same in other related classes, too, while we're at it.)

Comment 1

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~1000 methods with MOZ_OVERRIDE in /content

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I don't understand why only some of the nsIFormControl and nsITextControlElement methods have
MOZ_OVERRIDE in HTMLInputElement.

Comment 3

[Arnaud Sourioux [:Six]]

Hi,

thanks for the review, we found some upgrades to do on the script that generates the patch.
we're working on it, still an issue on the CppParser when some macro are juste before a methods declaration.
I will check if the missing MOZ_OVERRIDE you mentionned are now found with the last version and i will push a new patch when everything will be ok.

Comment 4

[Arnaud Sourioux [:Six]]

Quick update on where we are:

-tyepdef fooClass BarClass is not handled so it breaks the inheritance tree:
    This may be fixed very soon

- some methods are not marked MOZ_OVERRIDE because the method they override is in an .idl file:
    we don't actually parse .idl files but it looks easy to adapt the parser to get relevant informations.
    I guess i just have to look for "interface" keyword instead of "class"

- we can't MOZ_OVERRIDE all NS_IMETHOD methods as some of them are defining top level interface method that will be overrided in child classes.

Comment 5

[Arnaud Sourioux [:Six]]

Quick update:
- Typedef should know work with the parser, litle tests looks fine
- I added an option to the script to specify a folder where it can find the .idl files compiled in .h and they are included into the heritage tree

So i guess tomorrow afternoon it should be ok

Comment 6

[Arnaud Sourioux [:Six]]

(In reply to Olli Pettay [:smaug] from comment #2)
> I don't understand why only some of the nsIFormControl and
> nsITextControlElement methods have
> MOZ_OVERRIDE in HTMLInputElement.
This is all good except:

NS_IMETHOD_(uint32_t) GetType() const because of its return type that his really poorly handled by the parser and i don't think i could fix this issue...

i found 1500 MOZ_OVERRIDE to add instead of the previous ~1000

i tried it here with some others: https://tbpl.mozilla.org/?tree=Try&rev=85ec02b04d1e
it's not the final patch i will push tonight, in this test the previous comments haven't been fixed do to: 
PR_STATIC_ASSERT((uint32_t)eFormControlsWithoutSubTypesMax < (uint32_t)NS_FORM_BUTTON_ELEMENT);

in nsIFormControl.h which was exploding the parser...

Comment 7

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~1500 methods with MOZ_OVERRIDE in content/

idl files are parsed, typedefs are handled, only specific macros could obfuscate detection of an override.
Type NS_IMETHOD_(Foo) is still an issue

Comment 8

[Arnaud Sourioux [:Six]]

Build on linux x86_64, earlier tbpl was ok too.
it only needs your review and then a full tbpl build.

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 752889 [details] [diff] [review]
Annotate ~1500 methods with MOZ_OVERRIDE in content/

>diff --git a/content/base/src/nsDocument.h b/content/base/src/nsDocument.h
[...]
>   // nsIDOMEventTarget
>   virtual nsresult PreHandleEvent(nsEventChainPreVisitor& aVisitor);
>   virtual nsEventListenerManager*
>-    GetListenerManager(bool aCreateIfNotFound);
>+    GetListenerManager(bool aCreateIfNotFound) MOZ_OVERRIDE;
> 
>   // nsIScriptObjectPrincipal
>   virtual nsIPrincipal* GetPrincipal();
> 
>   // nsIApplicationCacheContainer
>   NS_DECL_NSIAPPLICATIONCACHECONTAINER

Drive-by comment: do you know why PreHandleEvent() and GetPrincipal() weren't detected as MOZ_OVERRIDE candidates there?  (They appear to be, and when I mark them as MOZ_OVERRIDE I can still build successfully.)

Comment 10

[Arnaud Sourioux [:Six]]

ok so now i'm tired of thoses f*cking macro on top of methods declarations... :)
so i added a function to my script to remove thoses macros from return type.
i tested it on content/ and all ret types modified were good.
it as added ~80 more moz_override including thoses you pointed out.

i'm gonna push the new patch to have a relevant review.

thank you Daniel,

Comment 11

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~1500 methods with MOZ_OVERRIDE in /content

I add to parse the whole moz-central as some headers are included from other modules (DOM and others), idl files are included.
Maximum of annoying macros avoided.
There still can be some misses due to specific macro but not so much i guess.
All the misses in previous comments have been corrected.
Compilation hasn't been tested, waiting the review.

Comment 12

[Arnaud Sourioux [:Six]]

*Annotate ~1600 methods, but patch message is good

Comment 13

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~1600 methods with MOZ_OVERRIDE in /content (2nd version)

This one is better as it also catches methods where param type is defined with namespace (mozilla::Foo) in class and defined without namespace (Foo) in the other class.
It gets 20 more override than the last one.

Comment 14

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~1600 methods with MOZ_OVERRIDE in /content (3rd version)

This is the final version,
every reviews comments are corrected.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 754380 [details] [diff] [review]
Annotate ~1600 methods with MOZ_OVERRIDE in /content (3rd version)

MOZ_MUST_OVERRIDE MOZ_OVERRIDE starts to look ugly, but not your fault.

For some reason NS_IMETHOD WillParse(void) { return NS_OK; } is missing 
MOZ_OVERRIDE, yet other methods overriding nsIContentSink have it.

Comment 16

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~1700 methods with MOZ_OVERRIDE in /content

Ok,
I did a fix on my script, it got ~20 more MOZ_OVERRIDE including thoses you find out.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Could you perhaps create an interdiff of the patch I reviewed and the latest patch?

Comment 18

[Daniel Holbert [:dholbert]]

I'm not sure if Arnaud has used interdiff before, so I gave it a shot in case he needed a hand.  Unfortunately, I get:
> 1 out of 3 hunks FAILED -- saving rejects to file /tmp/interdiff-1.CRUkse.rej
> interdiff: Error applying patch1 to reconstructed file

So it looks like the old patch no longer applies cleanly to trunk, and that thwarts interdiff attempts.

smaug: perhaps you could review the new patch by comparing it against the old patch in a merge tool, and just skipping through the changes? (That's how I reviewed Arnaud's various patch-updates over on bug 875367, incidentally).

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Ok, I can do that.

Comment 20

[Daniel Holbert [:dholbert]]

This built successfully for me on my Linux64 box, so I went ahead and pushed it:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a966aadd9ae3

(I don't think this got a cross-the-board Try run, so there's a very remote chance that there might be some platform-specific bustage in here -- if so, sorry about that.  I consider that very unlikely, given that it builds locally (which means it's correct, for a patch like this) & given my confidence in Arnaud's script's behavior.  Plus, the longer we wait, the more likely this is to bitrot, since it's a mega-patch, so landing it sooner saves bitrot-fixing pain later.)

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> This built successfully for me on my Linux64 box, so I went ahead and pushed
> it:
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/a966aadd9ae3
> 
> (I don't think this got a cross-the-board Try run, so there's a very remote
> chance that there might be some platform-specific bustage in here -- if so,
> sorry about that.  I consider that very unlikely, given that it builds
> locally (which means it's correct, for a patch like this) & given my
> confidence in Arnaud's script's behavior.  Plus, the longer we wait, the
> more likely this is to bitrot, since it's a mega-patch, so landing it sooner
> saves bitrot-fixing pain later.)

Funny story...
https://tbpl.mozilla.org/php/getParsedLog.php?id=23547256&tree=Mozilla-Inbound

Backed out.
http://hg.mozilla.org/integration/mozilla-inbound/rev/813bcead7cf6

Comment 22

[Daniel Holbert [:dholbert]]

D'oh. Thanks, RyanVM.

So, the bustage was:
> nsHTMLFormElement.h(222) : error C3668: 'nsHTMLFormElement::GetClassInfoW' :
>  method with override specifier 'override' did not override any base class methods

There is no GetClassInfoW method declared there; I suspect this is a (de facto) special/reserved method-name on MSVC, so it gets renamed out from under us, or something. This is probably what this comment...
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsINode.h?rev=c41885d5aa3d&mark=22-27#22
...is about.

I don't know exactly what's going on -- clearly the method is *expecting* to override something, since the MOZ_OVERRIDE works on other platforms. I'm not sure whether it's really not overriding on Windows (which seems like a huge bug), or if does actually override but MSVC just can't tell when it evaluates MOZ_OVERRIDE.

For now, it'd probably be safest to exclude GetClassInfo() methods from the patch entirely. (There are a lot; I suspect this is just the first one that we hit.)  Arnaud, do you have a way of flagging a particular function-name as not-to-be-annotated? (I seem to remember you mentioning being able to filter out certain keywords).  If so, could you try filtering out GetClassInfo and give that an all-platforms Try run? (without unit tests, as usual)

There's surely a better fix, but it'd need to land before this, and without an up-to-date windows build environment at the moment and with several other things on my plate, I can't really drive that right now.  And I'd like for the rest of this patch to land before it bitrots too much.

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> So, the bustage was:
> > nsHTMLFormElement.h(222) : error C3668: 'nsHTMLFormElement::GetClassInfoW' :
> >  method with override specifier 'override' did not override any base class methods
> 
> There is no GetClassInfoW method declared there; I suspect this is a (de
> facto) special/reserved method-name on MSVC, so it gets renamed out from
> under us, or something.

In particular, the line of code pointed to by the build error is:
> 222  virtual nsXPCClassInfo* GetClassInfo() MOZ_OVERRIDE;
https://hg.mozilla.org/integration/mozilla-inbound/annotate/a966aadd9ae3/content/html/content/src/nsHTMLFormElement.h#l222

Comment 24

[Daniel Holbert [:dholbert]]

More info from http://trac.wxwidgets.org/ticket/10950 :
>  c:\Program Files\Microsoft SDKs\Windows\v6.0A\Include\WinUser.h @ 3766
>  #define GetClassInfo  GetClassInfoW

And from http://msdn.microsoft.com/en-us/library/windows/desktop/ms633578%28v=vs.85%29.aspx :
> GetClassInfo function
> Retrieves information about a window class.
> [...]
> Unicode and ANSI names    GetClassInfoW (Unicode) and GetClassInfoA (ANSI)

Comment 25

[Daniel Holbert [:dholbert]]

...and from http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.h?rev=eaaa2d2ce8eb&mark=98-101#98 :
>  98   // Whaaaaa! I wanted to name this method GetClassInfo, but nooo,
>  99   // some of Microsoft devstudio's headers #defines GetClassInfo to
> 100   // GetClassInfoA so I can't, those $%#@^! bastards!!! What gives
> 101   // them the right to do that?

:)

Comment 26

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #22)
> For now, it'd probably be safest to exclude GetClassInfo() methods from the
> patch entirely. (There are a lot; I suspect this is just the first one that
> we hit.)

Actually, I didn't look closely enough - turns out there are only two instances of GetClassInfo in the patch, so I'll just revert those locally and give the result a Try run.

Comment 27

[Daniel Holbert [:dholbert]]

Try run on all platforms with GetClassInfo un-annotated in nsHTMLFormElement.h:
  https://tbpl.mozilla.org/?tree=Try&rev=961b0042a028
I suspect (but am not sure) that we'll need to un-annotate the other one (nsXMLElement) too, so here's another push (windows-only) that has that as well:
  https://tbpl.mozilla.org/?tree=Try&rev=21608bc75ad0

Comment 28

[Daniel Holbert [:dholbert]]

Apparently the GetClassInfo in nsHTMLFormElement.h is the only problematic one.  I'll re-push with that MOZ_OVERRIDE removed, and I'll file a followup on figuring out what's going on there.

Comment 29

[Daniel Holbert [:dholbert]]

(Tree's closed at the moment, but I filed bug 877510 on the windows-only nsHTMLFormElement GetClassInfo issue.)

Comment 30

[Daniel Holbert [:dholbert]]

Re-landed, with the annotation removed on GetClassInfo in nsHTMLFormElement.h, though I added a comment above it:
> // XXXdholbert This should be MOZ_OVERRIDE, or maybe dropped (see bug 877510)

The new landing cset: https://hg.mozilla.org/integration/mozilla-inbound/rev/fab554d11298

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fab554d11298