always more MOZ_OVERRIDE in layout

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Six, Assigned: Six)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

There are about 300 methods in layout/ that aren't yet defined as MOZ_OVERRIDE.
Assignee: nobody → six.dsn
Status: NEW → ASSIGNED
Depends on: 733186
Duplicate of this bug: 974562
Posted patch always_override_layout.patch (obsolete) — Splinter Review
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
Attachment #8379111 - Flags: review?(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?
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
(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.)
Posted patch always_override_layout.patch (obsolete) — Splinter Review
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.
Attachment #8379111 - Attachment is obsolete: true
Attachment #8379111 - Flags: review?(dholbert)
Attachment #8379236 - Flags: review?(dholbert)
This is the 80col fix patch.
Part 2/2
Attachment #8379256 - Flags: review?(dholbert)
Comment on attachment 8379236 [details] [diff] [review]
always_override_layout.patch

># HG changeset patch
># Parent cf9666cc3f36ec029d906a62168e22cff368c13d
># User Arnaud Sourioux <six.dsn@gmail.com>
>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 on attachment 8379256 [details] [diff] [review]
always_override_layout_indent.patch

80-col fixes look good. Thanks!
Attachment #8379256 - Flags: review?(dholbert) → review+
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.
Attachment #8379236 - Attachment is obsolete: true
Attachment #8379236 - Flags: review?(dholbert)
Attachment #8379925 - Flags: review?(dholbert)
Comment on attachment 8379925 [details] [diff] [review]
always_override_layout.patch

Looks good to me - thanks!
Attachment #8379925 - Flags: review?(dholbert) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5c277d91fcd6
https://hg.mozilla.org/mozilla-central/rev/ac6b0bcdea0b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.