Closed Bug 877746 Opened 7 years ago Closed 6 years ago

Mark overriding methods as MOZ_OVERRIDE in /dom

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: Six, Assigned: Six)

Details

Attachments

(1 file, 2 obsolete files)

See description of MOZ_OVERRIDE in bug 733186 comment 0.

Filing this bug on marking overriding most of methods as MOZ_OVERRIDE in /dom.
Has been build on my local Linux 64b correctly
Attachment #756129 - Flags: review?(Ms2ger)
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
Attachment #756129 - Flags: review?(Ms2ger) → review+
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.
Status: NEW → ASSIGNED
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
Attachment #756129 - Attachment is obsolete: true
Attachment #757372 - Flags: review?(Ms2ger)
(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 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'.
Attachment #757372 - Flags: review?(Ms2ger) → review+
Corrected as you wish
Attachment #757372 - Attachment is obsolete: true
Attachment #757379 - Flags: review?(Ms2ger)
Attachment #757379 - Flags: review?(Ms2ger) → review+
Status: ASSIGNED → UNCONFIRMED
Ever confirmed: false
Keywords: checkin-needed
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Waiting for tpbl to finish before adding "checkin-needed"

https://tbpl.mozilla.org/?tree=Try&rev=c0973f1b77d9
Keywords: checkin-needed
Wrong push, good version is there:
https://tbpl.mozilla.org/?tree=Try&rev=57e3f99435fb
Tbpl builds are good
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/14bd2ec730ae
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.