877746 - Mark overriding methods as MOZ_OVERRIDE in /dom

Status: RESOLVED FIXED

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

Description

[Arnaud Sourioux [:Six]]

See description of MOZ_OVERRIDE in bug 733186 comment 0.

Filing this bug on marking overriding most of methods as MOZ_OVERRIDE in /dom.

Comment 1

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~230 methods with MOZ_OVERRIDE in /dom

Has been build on my local Linux 64b correctly

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 756129 [details] [diff] [review]
Annotate ~230 methods with MOZ_OVERRIDE in /dom

Review of attachment 756129 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: dom/src/notification/DesktopNotification.h
@@ +177,5 @@
>     else
>       (void) Cancel();
>     return true;
>   }
> + void IPDLRelease() MOZ_OVERRIDE { Release(); }

Please mark those 'virtual' too

::: dom/src/storage/DOMStorage.h
@@ +30,2 @@
>    virtual DOMStorageManager* GetManager() const { return mManager; }
>    virtual const DOMStorageCache* GetCache() const { return mCache; }

Missed these two

Comment 3

[Arnaud Sourioux [:Six]]

Ok so to be sure:
you want me to annotate
>  NS_IMETHOD_(bool) Recv__delete__(const bool& allow)
>  NS_IMETHOD_(void) IPDLRelease()

about that:
(In reply to :Ms2ger from comment #2)
> ::: dom/src/storage/DOMStorage.h
> @@ +30,2 @@
> >    virtual DOMStorageManager* GetManager() const { return mManager; }
> >    virtual const DOMStorageCache* GetCache() const { return mCache; }

I dont catch them due to a silly thing:
in base class the return type is prefixed by namespace mozilla::dom::
and not in child class.

i will see if it's still safe to ignore namespaces in return types.

Comment 4

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~230 methods with MOZ_OVERRIDE in /dom (update with last comments)

diffs from precedent patch:
- class nsOfflineResourceListSH has been removed from moz-central, then nsOfflineResourceListSH ::GetStringAt is not marked MOZ_OVERRIDE anymore (inexistant)

- GetManager() and GetCache() are marked MOZ_OVERRIDE (directly by the script not manually)

- Recv__delete__() and IPDLRelease() are marked as NS_IMETHOD(retType) everywhere

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Arnaud Sourioux [:Six] from comment #4)
> Created attachment 757372 [details] [diff] [review]
> Annotate ~230 methods with MOZ_OVERRIDE in /dom (update with last comments)
> 
> diffs from precedent patch:
> - Recv__delete__() and IPDLRelease() are marked as NS_IMETHOD(retType)
> everywhere

Why's that? I don't think that's right.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 757372 [details] [diff] [review]
Annotate ~230 methods with MOZ_OVERRIDE in /dom (update with last comments)

r=me if you change the NS_IMETHOD_(foo)s to just 'virtual foo'.

Comment 7

[Arnaud Sourioux [:Six]]

Attachment: Annotate ~230 methods with MOZ_OVERRIDE in /dom (update 2nd version)

Corrected as you wish

Comment 8

[Arnaud Sourioux [:Six]]

Waiting for tpbl to finish before adding "checkin-needed"

https://tbpl.mozilla.org/?tree=Try&rev=c0973f1b77d9

Comment 9

[Arnaud Sourioux [:Six]]

Wrong push, good version is there:
https://tbpl.mozilla.org/?tree=Try&rev=57e3f99435fb

Comment 10

[Arnaud Sourioux [:Six]]

Tbpl builds are good

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14bd2ec730ae

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14bd2ec730ae