Closed Bug 875367 Opened 7 years ago Closed 7 years ago

even 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, 4 obsolete files)

Continue to add more MOZ_OVERRIDE in layout/
Attachment #753325 - Flags: review?(dholbert)
QA Contact: six.dsn
No longer blocks: 870516
Depends on: 870516
I'm tweaking this bug to have a slightly different name from the last one, for my sanity in distinguishing between them when reading bugmail. :)
Summary: add more MOZ_OVERRIDE in layout → even more MOZ_OVERRIDE in layout
Comment on attachment 753325 [details] [diff] [review]
Annotate ~250 more methods with MOZ_OVERRIDE in /layout

Thanks for the patch!  Looks great -- I just noticed a few methods in nsPresShell.h that probably want to be annotated.

>diff --git a/layout/base/nsPresShell.h b/layout/base/nsPresShell.h
>-  NS_IMETHOD SetDisplaySelection(int16_t aToggle);
>-  NS_IMETHOD GetDisplaySelection(int16_t *aToggle);
>+  NS_IMETHOD SetDisplaySelection(int16_t aToggle) MOZ_OVERRIDE;
>+  NS_IMETHOD GetDisplaySelection(int16_t *aToggle) MOZ_OVERRIDE;
>   NS_IMETHOD ScrollSelectionIntoView(SelectionType aType, SelectionRegion aRegion,
>                                      int16_t aFlags);
>   NS_IMETHOD RepaintSelection(SelectionType aType);

These last two methods (ScrollSelectionIntoView() and RepaintSelection()) should be annotated.

>   NS_IMETHOD CharacterExtendForDelete();
>   NS_IMETHOD CharacterExtendForBackspace();
[...]
>   NS_IMETHOD SelectAll();

...as should these three.

All five methods come from the same IDL file -- nsISelectionController.idl:
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsISelectionController.idl#89

...and I can add them in my local build and things still build correctly.

Do you know why those ones are getting missed?  And could you add them (manually if necessary) to the patch, or in a second patch here?

r=me with that, and with the patch's commit message updated to include the bug number.
Attachment #753325 - Flags: review?(dholbert) → review+
Hi daniel,

(In reply to Daniel Holbert [:dholbert] from comment #2)
>diff --git a/layout/base/nsPresShell.h b/layout/base/nsPresShell.h
>   NS_IMETHOD ScrollSelectionIntoView(SelectionType aType, SelectionRegion
> aRegion, int16_t aFlags);
>   NS_IMETHOD RepaintSelection(SelectionType aType);
> 
> Thoses last two methods (ScrollSelectionIntoView() and RepaintSelection())
> should be annotated.

I don't think so (or anyway can't do), they are using SelectionType as param when nsISelectionController uses uint16_t, maybe it's the same type, but i don't have any typedef in nsPresShell.h that could resolve it during parsing.

(In reply to Daniel Holbert [:dholbert] from comment #2)
>   NS_IMETHOD CharacterExtendForDelete();
>   NS_IMETHOD CharacterExtendForBackspace();
> [...]
>   NS_IMETHOD SelectAll();
> ...as should these three.
they where declared with "void" as param in base class so not matching "" in chid class, this is solved, only "void" as parameter is ignored (as it's irrelevant).

(In reply to Daniel Holbert [:dholbert] from comment #2)
> r=me with that, and with the patch's commit message updated to include the
> bug number.

I'm sorry about the bug number, failed the paste :)
but i assigned you with r=dholbert in the bug message.

I will push a new patch as soon as you will confirm/infirm the first 2 methods

Thanks,
(In reply to Arnaud Sourioux [:Six] from comment #3)
> (In reply to Daniel Holbert [:dholbert] from comment #2)
> >diff --git a/layout/base/nsPresShell.h b/layout/base/nsPresShell.h
> >   NS_IMETHOD ScrollSelectionIntoView(SelectionType aType, SelectionRegion
> > aRegion, int16_t aFlags);
> >   NS_IMETHOD RepaintSelection(SelectionType aType);
> > 
> > Thoses last two methods (ScrollSelectionIntoView() and RepaintSelection())
> > should be annotated.
> 
> I don't think so (or anyway can't do), they are using SelectionType as param
> when nsISelectionController uses uint16_t, maybe it's the same type, but i
> don't have any typedef in nsPresShell.h that could resolve it during parsing.

There's a typedef in nsIPresShell.h (note the "I"), which is included by nsPresShell.h:
> 89 typedef short SelectionType;
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsIPresShell.h?rev=3ee7ee6ad391#89
...and that must end up being the same as the type that nsISelectionController uses (int16_t) under the hood, because (at least on my system) I can manually add MOZ_OVERRIDE and it's fine.

So -- if you can get your script to handle this, then great -- and if not, it'd be great if you could manually add it for those two methods (either as part of the same patch, or as a second patch here, if you want to keep the main patch automated-stuff-only).

Thanks!
Hi,
yep i was guessing that there was a typedef somewhere,
it looks complicated and even if i can do it, it will expand the number of iterations in the script.

i'm gonna push the new patch with thoses two methods moz_overrid'ed by hand.

you didn't identify other misses?
Sounds good. Those were the only missed methods that stood out to me. Thanks!

(To be clear, by "i'm gonna push the new patch" you're talking about pushing *to try server*, right? for future reference, "pushing a patch" generally implies *actually* checking it in.   I think I know what you mean, but I figured I'd give you a heads-up on that terminology, so you know for other bugs.)

Thanks!
Yes i guess you learned how to understand me since one year :)
Thanks for the heads up.
I will do a tbpl with the new version this week end and put the link here
This is the final version,
all reviews comments are corrected, plus the two methods added manually
Attachment #753325 - Attachment is obsolete: true
Attachment #754381 - Flags: review?(dholbert)
I know there is a mistake in layout/base/nsPresContext.h,
Revoke shouldn't be override, but Run should instead, it is coming from the parser, i will see what makes this wrong.
It's pretty simple,
the CSSPointsToAppUnits declaration breaks the line counter, due to '/' at the end of line.
Waiting for a fix.
Comment on attachment 754381 [details] [diff] [review]
Annotate ~280 more methods with MOZ_OVERRIDE in /layout

Looks good, modulo comment 9, and with bitrot fixed in layout/svg/nsSVGGradientFrame.h.  (That file changed* in the last day such that this patch isn't applying there currently.)

* in bug 876175
Attachment #754381 - Flags: review?(dholbert) → review+
Fix the error in comment 9,
applied on the last rev so the bixtrot from comment 11 is ok
Thoses are the only diff with previous patch
Attachment #754381 - Attachment is obsolete: true
Attachment #754723 - Flags: review?(dholbert)
Tbpl on going:
https://tbpl.mozilla.org/?tree=Try&rev=a9143bbb2537

If it builds correctly, we could close this bug.
maybe a All Platform build before?
Comment on attachment 754723 [details] [diff] [review]
Annotate ~280 more methods with MOZ_OVERRIDE in /layout (2nd version)

Looks good!

It's just missing the two manual changes (ScrollSelectionIntoView and RepaintSelection, in nsPresShell.h).

r=me with those (possibly in their own patch, if you like, so if there's some bitrot somewhere else you can re-generate the automated patch without having to re-do the manual tweaks) 

Could you give it an all-platform test run? Assuming that's green, I can go ahead and check this in for you.
Attachment #754723 - Flags: review?(dholbert) → review+
Adds 3 more moz_override...
push to try of both last patches: https://tbpl.mozilla.org/?tree=Try&rev=d63e3f55a7e7
Attachment #754723 - Attachment is obsolete: true
Attachment #755234 - Flags: review?(dholbert)
Add 4 MOZ_OVERRIDE with fix from Bug 856822
This time it should be finished.
Tbpl went fine except two issues due to Build Infrastcruture.
Attachment #755234 - Attachment is obsolete: true
Attachment #755234 - Flags: review?(dholbert)
Attachment #755305 - Flags: review?(dholbert)
Comment on attachment 755305 [details] [diff] [review]
Annotate ~280 more methods with MOZ_OVERRIDE in /layout (4th version)

># HG changeset patch
># Parent e58336e813950e8d8573accc65906ef1207127d1
># User Arnaud Sourioux <six.dsn@gmail.com>
>Bug 875367: Annotate ~280 more methods with MOZ_OVERRIDE in /layout (4th version) r=dholbert

Nit: Don't bother putting things like "(3rd version)" "(4th version)" etc. in the commit message, except maybe in a situation where we checked in and backed out the patch 3 times (yikes!), and this is the 4th version that we're hoping will finally stay in. :)

It *is* useful to have labels like "4th version" in the *attachment description field* on the bug page, to distinguish between different patch versions attached on the bug.  But the commit message is different -- it's for people examining the source code & the commit history, and there's no reason those people would need to know (or care to know) that this happens to be the 4th version of this patch that you wrote before checking it in. :)

Anyway -- thanks very much for the patch! I'll land this at some point today (with an edited commit message ;)).
Attachment #755305 - Flags: review?(dholbert) → review+
Gret thank you :)
Btw i totally agree and understansd your hint about the commit message, i take note of it.
https://hg.mozilla.org/mozilla-central/rev/761fa2e9b325
https://hg.mozilla.org/mozilla-central/rev/ace94c40f23a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.