974687 - always more MOZ_OVERRIDE in layout

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Arnaud Sourioux [:Six]]

There are about 300 methods in layout/ that aren't yet defined as MOZ_OVERRIDE.

Comment 2

[Arnaud Sourioux [:Six]]

Attachment: always_override_layout.patch

Adds 297 MOZ_OVERRIDE in layout/
build in progress: https://tbpl.mozilla.org/?tree=Try&rev=40d4f8d72763
went fine earlier on linux64,win32 -do
If needed a whitespace patch will be filed as part2

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8379111 [details] [diff] [review]
always_override_layout.patch

>Bug 974687: adds about 300 MOZ_OVERRIDE in layout/ r=dholbert

s/adds/add/

>+++ b/layout/svg/SVGTextFrame.cpp
>-  void NotifyBeforeText(nscolor aColor);
>+  void NotifyBeforeText(nscolor aColor) MOZ_OVERRIDE;
>   void NotifyGlyphPathEmitted();
>   void NotifyBeforeSVGGlyphPainted();
>   void NotifyAfterSVGGlyphPainted();
>-  void NotifyAfterText();
>-  void NotifyBeforeSelectionBackground(nscolor aColor);
>-  void NotifySelectionBackgroundPathEmitted();

So there are three methods in the middle that stand out as being left untouched there -- NotifyGlyphPathEmitted, NotifyBeforeSVGGlyphPainted, NotifyAfterSVGGlyphPainted.  I believe those all should be MOZ_OVERRIDE as well.

I'll bet your script didn't catch them because the superclass version of those methods is declared in gfx/thebes, outside of layout:
 http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.h#2704

(This is in  SVGTextDrawPathCallbacks, which inherits from DrawPathCallbacks, which inherits from gfxTextRunDrawCallbacks, defined in gfxFont.h)

Could you either
 (a) just annotate these 3 methods as MOZ_OVERRIDE by hand
...or...
 (b) run your script in way that covers /gfx and /layout simultaneously, and see if it catches these (and maybe other stuff)

(not sure if (b) is too tricky to be worth it, just for these 3 methods. I'll bet it'd find another few opportunities, though.)

>+++ b/layout/xul/nsTextBoxFrame.cpp
[...]
>   virtual nsRect GetComponentAlphaBounds(nsDisplayListBuilder* aBuilder);
> 
>-  virtual void DisableComponentAlpha() { mDisableSubpixelAA = true; }
>+  virtual void DisableComponentAlpha() MOZ_OVERRIDE { mDisableSubpixelAA = true; }

I think GetComponentAlphaBounds should be annotated, too.

This is:
 http://mxr.mozilla.org/mozilla-central/source/layout/xul/nsTextBoxFrame.cpp#300
which overrides:
 http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.h#1208

Do you know why that didn't get caught?

Comment 4

[Arnaud Sourioux [:Six]]

I don't think it hasn't been caught because the superclass is in another folder as i parse all headers present in m-c (including idl files as i do a ./mach build export) and only modify thoses in layout/

maybe my build export wasn't up-to-date, i will look if it changes something.

my guess i that the superclass header parsing failed because of something that isn't recognised, or by another macro that the parser doesn't handle correctly.

Whatever what makes thoses methods untouched i will take a look at it and if i don't find a way i will fix them manually but it won't be ideal as there might be a few more missing.

thanks for the comments

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Arnaud Sourioux [:Six] from comment #4)
> if i don't find a way i will fix them manually but it won't be ideal as there
> might be a few more missing.

Thanks!

Yeah, ideally it'd be great to catch absolutely everything. :) But that's probably not going to happen; and in the meantime, it's at least nice to have *contiguous* blocks of overridding methods annotated consistently. (And these were the only instances I saw in the patch where that wasn't happening.)

Comment 6

[Arnaud Sourioux [:Six]]

Attachment: always_override_layout.patch

The same patch but including missing MOZ_OVERRIDE pointed out by Daniel.
A second patch will come to fix lines superior to 80col due to MOZ_OVERRIDE annotation.

About this:

>+++ b/layout/xul/nsTextBoxFrame.cpp
[...]
>   virtual nsRect GetComponentAlphaBounds(nsDisplayListBuilder* aBuilder);
> 
>-  virtual void DisableComponentAlpha() { mDisableSubpixelAA = true; }
>+  virtual void DisableComponentAlpha() MOZ_OVERRIDE { mDisableSubpixelAA = true; }

it was the macro NS_DISPLAY_DECL_NAME("XULTextBox", TYPE_XUL_TEXT_BOX) just above that made the parser failed (as usual) so the script undestands this macro now.

about this:
>   void NotifyGlyphPathEmitted();
>   void NotifyBeforeSVGGlyphPainted();
>   void NotifyAfterSVGGlyphPainted();

I added them manually as they are not automatically MOZ_OVERRIDED.
There still are many files that the cppheaderparser fails to parse correctly so the heritage tree is incomplete and i miss some MOZ_OVERRIDE.

Comment 7

[Arnaud Sourioux [:Six]]

Attachment: always_override_layout_indent.patch

This is the 80col fix patch.
Part 2/2

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8379236 [details] [diff] [review]
always_override_layout.patch

># HG changeset patch
># Parent cf9666cc3f36ec029d906a62168e22cff368c13d
># User Arnaud Sourioux <[e-mail]>
>Bug 974687: Part 1/2 Add about 300 MOZ_OVERRIDE in layout/ r=dholbert

This looks good! Looks like you picked up a lot of methods after the NS_DISPLAY_DECL_NAME() macro. :)

However, there are a number of methods that were annotated in the first version that aren't annotated in this new version. 

In particular, these methods (from the original patch) are no longer annotated:

 nsDocumentViewer.cpp:  Run()
 nsPresContext.cpp:     Run()
 nsPresShell.cpp:       Run()
 nsComboboxControlFrame.cpp: HandleEvent()
 nsSelection.cpp:       Notify()
 nsVideoFrame.cpp:      Run()
 nsPrintEngine.cpp:     Run()
 nsTableFrame.cpp:      Run()
 nsTreeBodyFrame.cpp:   Run()
 nsImageBoxFrame.cpp:   Run()
 nsMenuBarFrame.cpp:    Run()
 nsMenuFrame.cpp:       Run() (two different Run() methods, actually)
 nsMenuPopupFrame.cpp:  Run()

You can see these on bugzilla's "differences between [] and this patch" view, here:
 https://bugzilla.mozilla.org/attachment.cgi?oldid=8379111&action=interdiff&newid=8379236&headers=1

Ideally it'd be nice to keep all those annotations that you had in the original patch -- could you fix things to get those back in?

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8379256 [details] [diff] [review]
always_override_layout_indent.patch

80-col fixes look good. Thanks!

Comment 10

[Arnaud Sourioux [:Six]]

Attachment: always_override_layout.patch

This one looks good, it looks like it includes all your remarks.
i don't know what happend on the last one as i just add to launch again my script and the Run() methods were annotate MOZ_OVERRIDE.

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8379925 [details] [diff] [review]
always_override_layout.patch

Looks good to me - thanks!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c277d91fcd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac6b0bcdea0b

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c277d91fcd6
https://hg.mozilla.org/mozilla-central/rev/ac6b0bcdea0b