Closed
Bug 703068
Opened 13 years ago
Closed 11 years ago
Use MOZ_OVERRIDE in mailnews/
Categories
(MailNews Core :: Backend, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 25.0
People
(Reporter: rain1, Assigned: Six)
References
()
Details
Attachments
(9 files, 35 obsolete files)
5.65 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
20.74 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
50.97 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
60.62 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
46.56 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
34.22 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
12.21 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
10.84 KB,
patch
|
Six
:
review+
|
Details | Diff | Splinter Review |
7.19 KB,
patch
|
Six
:
review+
|
Details | Diff | Splinter Review |
This is going to help us catch bugs where the signature of an override doesn't match and so doesn't get called. bienvenu requested that this be done on top of bug 402392.
Comment 1•12 years ago
|
||
Sid ?
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #747590 -
Flags: review?(sid.bugzilla)
Reporter | ||
Comment 3•11 years ago
|
||
Comment on attachment 747590 [details] [diff] [review] Add ~200 MOZ_OVERRIDE in mailnews/ This patch is way too big for me to have the time to review. Please ask someone else, preferably individual module owners.
Attachment #747590 -
Flags: review?(sid.bugzilla)
Updated•11 years ago
|
Assignee: nobody → six.dsn
Status: NEW → ASSIGNED
Comment 4•11 years ago
|
||
Comment on attachment 747590 [details] [diff] [review] Add ~200 MOZ_OVERRIDE in mailnews/ Adding mconley for the ab part Standard8 -> for LDAP We should also have
Attachment #747590 -
Flags: review?(mconley)
Attachment #747590 -
Flags: feedback?(mbanner)
Assignee | ||
Comment 5•11 years ago
|
||
A new patch will be available soon, i have improved the script that generates it, cf Bug 870516 there must be some missing overrides that should be fixed in the next pass
Comment 6•11 years ago
|
||
(In reply to Arnaud Sourioux [:Six] from comment #5) > A new patch will be available soon, > i have improved the script that generates it, cf Bug 870516 > there must be some missing overrides > that should be fixed in the next pass Good can you split the patches by functional area (eg AB, Imap etc ...) so we can request reviews to proper peers (eg see https://wiki.mozilla.org/Modules/MailNews_Core) ?
Assignee | ||
Comment 7•11 years ago
|
||
Yes i guess it's the only way to make it right :) I can provide one patch by mailnews main subfolder which looks kind of what you expect So next patch will be done this way thanks for the advice
Comment 8•11 years ago
|
||
Comment on attachment 747590 [details] [diff] [review] Add ~200 MOZ_OVERRIDE in mailnews/ Cancelling review and feedback request as new patches are coming up.
Attachment #747590 -
Flags: review?(mconley)
Attachment #747590 -
Flags: feedback?(mbanner)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #749387 -
Flags: review?(mconley)
Assignee | ||
Updated•11 years ago
|
Attachment #747590 -
Attachment is obsolete: true
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #749388 -
Flags: review?
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #749391 -
Flags: review?
Assignee | ||
Updated•11 years ago
|
Attachment #749388 -
Flags: review? → review?(mozilla)
Assignee | ||
Updated•11 years ago
|
Attachment #749391 -
Flags: review? → review?(mozilla)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #749392 -
Flags: review?(mozilla)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #749393 -
Flags: review?(mozilla)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #749394 -
Flags: review?(mbanner)
Assignee | ||
Updated•11 years ago
|
Attachment #749387 -
Attachment description: Bug 703068: Annotate 22 methods with MOZ_OVERRIDE in mailnews/addrbook → Annotate 22 methods with MOZ_OVERRIDE in mailnews/addrbook
Assignee | ||
Updated•11 years ago
|
Attachment #749388 -
Attachment description: Bug 703068: Annotate 101 methods with MOZ_OVERRIDE in mailnews/base → Annotate 101 methods with MOZ_OVERRIDE in mailnews/base
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #749395 -
Flags: review?(mbanner)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #749396 -
Flags: review?
Assignee | ||
Updated•11 years ago
|
Attachment #749396 -
Flags: review? → review?(mozilla)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #749398 -
Flags: review?(Pidgeot18)
Assignee | ||
Comment 18•11 years ago
|
||
This is all the MOZ_OVERRIDE added to mailnews/ splitted by module and assigned for review to each module owner. Edit them if some are wrong.
Comment 19•11 years ago
|
||
Adding Irving in case he has some spare cycle for review ?
Assignee | ||
Comment 20•11 years ago
|
||
Attachment #749387 -
Attachment is obsolete: true
Attachment #749387 -
Flags: review?(mconley)
Attachment #752903 -
Flags: review?(mconley)
Assignee | ||
Comment 21•11 years ago
|
||
Attachment #749388 -
Attachment is obsolete: true
Attachment #749388 -
Flags: review?(mozilla)
Attachment #752905 -
Flags: review?(mozilla)
Assignee | ||
Comment 22•11 years ago
|
||
Attachment #749391 -
Attachment is obsolete: true
Attachment #749391 -
Flags: review?(mozilla)
Attachment #752907 -
Flags: review?(mozilla)
Assignee | ||
Comment 23•11 years ago
|
||
Attachment #749392 -
Attachment is obsolete: true
Attachment #749392 -
Flags: review?(mozilla)
Attachment #752908 -
Flags: review?(mozilla)
Assignee | ||
Comment 24•11 years ago
|
||
Attachment #749393 -
Attachment is obsolete: true
Attachment #749393 -
Flags: review?(mozilla)
Attachment #752911 -
Flags: review?(mozilla)
Assignee | ||
Comment 25•11 years ago
|
||
Attachment #749394 -
Attachment is obsolete: true
Attachment #749394 -
Flags: review?(mbanner)
Attachment #752912 -
Flags: review?(mbanner)
Assignee | ||
Comment 26•11 years ago
|
||
Attachment #749395 -
Attachment is obsolete: true
Attachment #749395 -
Flags: review?(mbanner)
Attachment #752913 -
Flags: review?(mbanner)
Assignee | ||
Comment 27•11 years ago
|
||
Attachment #749396 -
Attachment is obsolete: true
Attachment #749396 -
Flags: review?(mozilla)
Attachment #752915 -
Flags: review?(mozilla)
Assignee | ||
Comment 28•11 years ago
|
||
Attachment #749398 -
Attachment is obsolete: true
Attachment #749398 -
Flags: review?(Pidgeot18)
Attachment #752916 -
Flags: review?(Pidgeot18)
Comment 29•11 years ago
|
||
Comment on attachment 752903 [details] [diff] [review] Annotate 51 methods with MOZ_OVERRIDE in mailnews/addrbook Review of attachment 752903 [details] [diff] [review]: ----------------------------------------------------------------- Assuming http://whereswalden.com/2011/11/16/introducing-moz_override-to-annotate-virtual-functions-which-override-base-class-virtual-functions/ is an accurate description of what MOZ_OVERRIDE is supposed to do, this all looks sane to me.
Attachment #752903 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 30•11 years ago
|
||
Hi, thanks for your review, i'm working on it since one year, the script that generates it is safe, the review is mainly to see if there are some miss. There might be a new version in few days as we (with dholbert) improved little things that weren't caught as override. it may not affect your module.
Comment 31•11 years ago
|
||
Comment on attachment 752916 [details] [diff] [review] Annotate 5 methods with MOZ_OVERRIDE in mailnews/news Review of attachment 752916 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/news/src/nsNewsDownloader.h @@ +69,5 @@ > DownloadNewsArticlesToOfflineStore(nsIMsgWindow *window, nsIMsgDatabase *db, nsIUrlListener *listener); > virtual ~DownloadNewsArticlesToOfflineStore(); > > + NS_IMETHOD OnStartRunningUrl(nsIURI* url) MOZ_OVERRIDE; > + NS_IMETHOD OnStopRunningUrl(nsIURI* url, nsresult exitCode) MOZ_OVERRIDE; nits: change nsIURI* url to nsIURI aUrl while you're changing these lines, and exitCode to aExitCode @@ +74,2 @@ > protected: > + virtual int32_t StartDownload() MOZ_OVERRIDE; Nit: lose the extra space here.
Attachment #752916 -
Flags: review?(Pidgeot18) → review+
Assignee | ||
Comment 32•11 years ago
|
||
Ok i will change this by hand in the next patch,this is done by a script so i never change the content. thanks for the review
Assignee | ||
Comment 33•11 years ago
|
||
final version.
Attachment #752903 -
Attachment is obsolete: true
Attachment #754382 -
Flags: review?(mconley)
Assignee | ||
Comment 34•11 years ago
|
||
final version.
Attachment #752905 -
Attachment is obsolete: true
Attachment #752905 -
Flags: review?(mozilla)
Attachment #754383 -
Flags: review?(mozilla)
Assignee | ||
Comment 35•11 years ago
|
||
final version.
Attachment #752907 -
Attachment is obsolete: true
Attachment #752907 -
Flags: review?(mozilla)
Attachment #754384 -
Flags: review?
Assignee | ||
Comment 36•11 years ago
|
||
Attachment #752908 -
Attachment is obsolete: true
Attachment #752908 -
Flags: review?(mozilla)
Attachment #754385 -
Flags: review?(mozilla)
Assignee | ||
Comment 37•11 years ago
|
||
Attachment #752911 -
Attachment is obsolete: true
Attachment #752911 -
Flags: review?(mozilla)
Attachment #754386 -
Flags: review?(mozilla)
Assignee | ||
Comment 38•11 years ago
|
||
final version.
Attachment #752912 -
Attachment is obsolete: true
Attachment #752912 -
Flags: review?(mbanner)
Attachment #754387 -
Flags: review?(mbanner)
Assignee | ||
Comment 39•11 years ago
|
||
final version.
Attachment #752913 -
Attachment is obsolete: true
Attachment #752913 -
Flags: review?(mbanner)
Attachment #754388 -
Flags: review?(mbanner)
Assignee | ||
Comment 40•11 years ago
|
||
final version.
Attachment #752915 -
Attachment is obsolete: true
Attachment #752915 -
Flags: review?(mozilla)
Attachment #754389 -
Flags: review?(mozilla)
Assignee | ||
Comment 41•11 years ago
|
||
final version.
Attachment #752916 -
Attachment is obsolete: true
Attachment #754390 -
Flags: review?(Pidgeot18)
Assignee | ||
Updated•11 years ago
|
Attachment #754384 -
Flags: review? → review?(mozilla)
Assignee | ||
Comment 42•11 years ago
|
||
final version + 3 manual modifications asked in the last review.
Attachment #754390 -
Attachment is obsolete: true
Attachment #754390 -
Flags: review?(Pidgeot18)
Attachment #754391 -
Flags: review?(Pidgeot18)
Assignee | ||
Comment 43•11 years ago
|
||
Attachment #754382 -
Attachment is obsolete: true
Attachment #754382 -
Flags: review?(mconley)
Attachment #755330 -
Flags: review?(mconley)
Assignee | ||
Comment 44•11 years ago
|
||
Attachment #754383 -
Attachment is obsolete: true
Attachment #754383 -
Flags: review?(mozilla)
Attachment #755332 -
Flags: review?(mozilla)
Assignee | ||
Comment 45•11 years ago
|
||
Attachment #754385 -
Attachment is obsolete: true
Attachment #754385 -
Flags: review?(mozilla)
Attachment #755333 -
Flags: review?(mozilla)
Assignee | ||
Comment 46•11 years ago
|
||
Attachment #754386 -
Attachment is obsolete: true
Attachment #754386 -
Flags: review?(mozilla)
Attachment #755334 -
Flags: review?(mozilla)
Assignee | ||
Comment 47•11 years ago
|
||
Attachment #754388 -
Attachment is obsolete: true
Attachment #754388 -
Flags: review?(mbanner)
Attachment #755336 -
Flags: review?(mbanner)
Assignee | ||
Comment 48•11 years ago
|
||
Attachment #754391 -
Attachment is obsolete: true
Attachment #754391 -
Flags: review?(Pidgeot18)
Attachment #755339 -
Flags: review?(Pidgeot18)
Comment 49•11 years ago
|
||
Comment on attachment 755339 [details] [diff] [review] Annotate 73 methods with MOZ_OVERRIDE in mailnews/news + manual modifications Review of attachment 755339 [details] [diff] [review]: ----------------------------------------------------------------- It's fine, but a lot of associated cleanup should be done while you're touching these lines. ::: mailnews/news/src/nsNNTPNewsgroupPost.h @@ +84,1 @@ > Nit: clean up this trailing whitespace. ::: mailnews/news/src/nsNNTPProtocol.h @@ +140,5 @@ > nsIMsgWindow *aMsgWindow); > virtual ~nsNNTPProtocol(); > > // stop binding is a "notification" informing us that the stream associated with aURL is going away. > + NS_IMETHOD OnStopRequest(nsIRequest *request, nsISupports * aCtxt, nsresult aStatus) MOZ_OVERRIDE; Nit: 80-char line width violations here... @@ +146,5 @@ > char * m_ProxyServer; /* proxy server hostname */ > > + NS_IMETHOD Cancel(nsresult status) MOZ_OVERRIDE; // handle stop button > + NS_IMETHOD GetContentType(nsACString &aContentType) MOZ_OVERRIDE; > + NS_IMETHOD AsyncOpen(nsIStreamListener *listener, nsISupports *ctxt) MOZ_OVERRIDE; ...here... @@ +172,5 @@ > * the code will not read the input stream at all. Therefore, it is strongly > * advised to suspend the request before using this state. > */ > virtual nsresult ProcessProtocolState(nsIURI * url, nsIInputStream * inputStream, > + uint64_t sourceOffset, uint32_t length) MOZ_OVERRIDE; ...here... @@ +178,4 @@ > > // we have our own implementation of SendData which writes to the nntp log > // and then calls the base class to transmit the data > + nsresult SendData(const char * dataBuffer, bool aSuppressLogging = false) MOZ_OVERRIDE; ...and here. ::: mailnews/news/src/nsNewsDownloader.h @@ +74,2 @@ > protected: > + virtual int32_t StartDownload() MOZ_OVERRIDE; Nit: lose the extra space. ::: mailnews/news/src/nsNewsFolder.h @@ +40,2 @@ > > + NS_IMETHOD CreateSubfolder(const nsAString& folderName,nsIMsgWindow *msgWindow) MOZ_OVERRIDE; More 80-char violations here.. @@ +42,3 @@ > > + NS_IMETHOD Delete () MOZ_OVERRIDE; > + NS_IMETHOD Rename (const nsAString& newName, nsIMsgWindow *msgWindow) MOZ_OVERRIDE; ...here... @@ +49,3 @@ > > NS_IMETHOD GetExpungedBytesCount(uint32_t *count); > + NS_IMETHOD GetDeletable (bool *deletable) MOZ_OVERRIDE; (Extra space here) @@ +56,2 @@ > > + NS_IMETHOD GetDBFolderInfoAndDB(nsIDBFolderInfo **folderInfo, nsIMsgDatabase **db) MOZ_OVERRIDE; ...here... @@ +58,5 @@ > > NS_IMETHOD DeleteMessages(nsIArray *messages, > nsIMsgWindow *msgWindow, bool deleteStorage, bool isMove, > + nsIMsgCopyServiceListener* listener, bool allowUndo) MOZ_OVERRIDE; > + NS_IMETHOD GetNewMessages(nsIMsgWindow *aWindow, nsIUrlListener *aListener) MOZ_OVERRIDE; ...here as well... (Also, fix the bad indentation for the DeleteMessages continuation?) @@ +71,4 @@ > > + NS_IMETHOD DownloadMessagesForOffline(nsIArray *messages, nsIMsgWindow *window) MOZ_OVERRIDE; > + NS_IMETHOD Compact(nsIUrlListener *aListener, nsIMsgWindow *aMsgWindow) MOZ_OVERRIDE; > + NS_IMETHOD DownloadAllForOffline(nsIUrlListener *listener, nsIMsgWindow *msgWindow) MOZ_OVERRIDE; ...all three of these... @@ +79,3 @@ > > + NS_IMETHOD GetFilterList(nsIMsgWindow *aMsgWindow, nsIMsgFilterList **aFilterList) MOZ_OVERRIDE; > + NS_IMETHOD GetEditableFilterList(nsIMsgWindow *aMsgWindow, nsIMsgFilterList **aFilterList) MOZ_OVERRIDE; ...both of these... @@ +89,5 @@ > nsresult ParseFolder(nsIFile *path); > nsresult CreateSubFolders(nsIFile *path); > nsresult AddDirectorySeparator(nsIFile *path); > + nsresult GetDatabase() MOZ_OVERRIDE; > + virtual nsresult CreateChildFromURI(const nsCString &uri, nsIMsgFolder **folder) MOZ_OVERRIDE; ...here... @@ +98,5 @@ > nsresult ForgetLine(void); > nsresult GetNewsMessages(nsIMsgWindow *aMsgWindow, bool getOld, nsIUrlListener *aListener); > > int32_t HandleNewsrcLine(const char * line, uint32_t line_size); > + virtual void GetIncomingServerType(nsCString& serverType) MOZ_OVERRIDE { serverType.AssignLiteral("nntp");} ...and here. ::: mailnews/news/src/nsNntpIncomingServer.h @@ +66,5 @@ > + NS_IMETHOD GetCanSearchMessages(bool *canSearchMessages) MOZ_OVERRIDE; > + NS_IMETHOD GetOfflineSupportLevel(int32_t *aSupportLevel) MOZ_OVERRIDE; > + NS_IMETHOD GetDefaultCopiesAndFoldersPrefsToServer(bool *aCopiesAndFoldersOnServer) MOZ_OVERRIDE; > + NS_IMETHOD GetCanCreateFoldersOnServer(bool *aCanCreateFoldersOnServer) MOZ_OVERRIDE; > + NS_IMETHOD GetCanFileMessagesOnServer(bool *aCanFileMessagesOnServer) MOZ_OVERRIDE; Some more 80-char violations in this file. @@ +77,4 @@ > > protected: > virtual nsresult CreateRootFolderFromUri(const nsCString &serverUri, > + nsIMsgFolder **rootFolder) MOZ_OVERRIDE; Again. @@ +90,5 @@ > * Downloads the newsgroup headers. > */ > nsresult DownloadMail(nsIMsgWindow *aMsgWindow); > > + NS_IMETHOD GetServerRequiresPasswordForBiff(bool *aServerRequiresPasswordForBiff) MOZ_OVERRIDE; and again. Actually, for the last one, just changing the name to "retval" would fix the issue much better.
Attachment #755339 -
Flags: review?(Pidgeot18) → review+
Comment 50•11 years ago
|
||
Comment on attachment 754387 [details] [diff] [review] Annotate 49 methods with MOZ_OVERRIDE in mailnews/import Review of attachment 754387 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I would normally ask for whitespace cleanup, however as the import code is so badly formatted anyway, it'd end up changing whole files which would be a separate bug anyway. So lets take it as it is.
Attachment #754387 -
Flags: review?(mbanner) → review+
Comment 51•11 years ago
|
||
Comment on attachment 755330 [details] [diff] [review] Annotate 196 methods with MOZ_OVERRIDE in mailnews/addrbook Review of attachment 755330 [details] [diff] [review]: ----------------------------------------------------------------- Just one nit, but other than that, this looks fine to me. ::: mailnews/addrbook/src/nsAbBSDirectory.h @@ +22,5 @@ > virtual ~nsAbBSDirectory(); > > // nsIAbDirectory methods > + NS_IMETHOD Init(const char *aURI) MOZ_OVERRIDE; > + NS_IMETHOD GetChildNodes(nsISimpleEnumerator* *result) MOZ_OVERRIDE; While you're here - please fix the indentation on this line.
Attachment #755330 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 52•11 years ago
|
||
It tried to apply all your comments, it should be ok
::: mailnews/news/src/nsNntpIncomingServer.h
@@ +66,5 @@
> + NS_IMETHOD GetCanSearchMessages(bool *canSearchMessages) MOZ_OVERRIDE;
> + NS_IMETHOD GetOfflineSupportLevel(int32_t *aSupportLevel) MOZ_OVERRIDE;
> + NS_IMETHOD GetDefaultCopiesAndFoldersPrefsToServer(bool *aCopiesAndFoldersOnServer) MOZ_OVERRIDE;
> + NS_IMETHOD GetCanCreateFoldersOnServer(bool *aCanCreateFoldersOnServer) MOZ_OVERRIDE;
> + NS_IMETHOD GetCanFileMessagesOnServer(bool *aCanFileMessagesOnServer) MOZ_OVERRIDE;
This is looking ugly in the patch but not other choice except change the parameter name
Attachment #755339 -
Attachment is obsolete: true
Attachment #764033 -
Flags: review?(Pidgeot18)
Assignee | ||
Comment 53•11 years ago
|
||
The same patch with only the indentation fixed.
Attachment #755330 -
Attachment is obsolete: true
Attachment #764034 -
Flags: review?(mconley)
Updated•11 years ago
|
Attachment #754384 -
Flags: review?(mozilla) → review+
Comment 54•11 years ago
|
||
Comment on attachment 754389 [details] [diff] [review] Annotate 32 methods with MOZ_OVERRIDE in mailnews/mime Review of attachment 754389 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, r=Standard8 with the comments fixed. ::: mailnews/mime/emitters/nsMimeHtmlEmitter.h @@ +42,4 @@ > > + virtual nsresult WriteHeaderFieldHTMLPrefix(const nsACString &name) MOZ_OVERRIDE; > + virtual nsresult WriteHeaderFieldHTML(const char *field, const char *value) MOZ_OVERRIDE; > + virtual nsresult WriteHeaderFieldHTMLPostfix() MOZ_OVERRIDE; It'd be nice if you could remove the unnecessary blank whitespace from these lines and cut down the length a bit. ::: mailnews/mime/emitters/nsMimeRawEmitter.h @@ +18,5 @@ > public: > nsMimeRawEmitter (); > virtual ~nsMimeRawEmitter (void); > > + NS_IMETHOD WriteBody(const nsACString &buf, uint32_t *amountWritten) MOZ_OVERRIDE; nit: could you wrap this line a bit, maybe in between the two parameters. ::: mailnews/mime/src/nsMimeObjectClassAccess.h @@ +45,2 @@ > > + NS_IMETHOD MimeCreate(char *content_type, void * hdrs, void * opts, void**ptr) MOZ_OVERRIDE; nit: can you wrap this line a bit?
Attachment #754389 -
Flags: review?(mozilla) → review+
Comment 55•11 years ago
|
||
Comment on attachment 755332 [details] [diff] [review] Annotate 161 methods with MOZ_OVERRIDE in mailnews/base Review of attachment 755332 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I've commented on a few lines that need wrapping, but it'd be nice if you could tidy up some of the other long ones that this patch affects as well. ::: mailnews/base/src/nsMsgAccountManagerDS.h @@ +45,5 @@ > NS_DECL_NSIINCOMINGSERVERLISTENER > NS_DECL_NSIOBSERVER > // RDF datasource methods > > + /* nsIRDFNode GetTarget (in nsIRDFResource aSource, in nsIRDFResource property, in boolean aTruthValue) MOZ_OVERRIDE; */ nit: please just remove this comment, there's no actual use it in anyway. ::: mailnews/base/src/nsMsgFolderDataSource.h @@ +69,2 @@ > > + NS_IMETHOD HasArcOut(nsIRDFResource *aSource, nsIRDFResource *aArc, bool *result) MOZ_OVERRIDE; nit: could do with a little wrapping. @@ +79,3 @@ > > NS_IMETHOD GetAllCmds(nsIRDFResource* source, > + nsISimpleEnumerator/*<nsIRDFResource>*/** commands) MOZ_OVERRIDE; nit: wrapping @@ +294,2 @@ > protected: > + virtual nsresult GetFolderDisplayName(nsIMsgFolder *folder, nsString& folderName) MOZ_OVERRIDE; nit: wrapping @@ +296,4 @@ > virtual void EnsureFolders(); > virtual bool WantsThisFolder(nsIMsgFolder *folder); > bool ResourceIsOurRoot(nsIRDFResource *resource); > + virtual nsresult OnItemAddedOrRemoved(nsIMsgFolder *parentItem, nsISupports *item, bool added) MOZ_OVERRIDE; nit: wrapping ::: mailnews/base/src/nsMsgGroupThread.h @@ +71,5 @@ > protected: > + virtual void InsertMsgHdrAt(nsMsgViewIndex index, nsIMsgDBHdr *hdr) MOZ_OVERRIDE; > + virtual void SetMsgHdrAt(nsMsgViewIndex index, nsIMsgDBHdr *hdr) MOZ_OVERRIDE; > + virtual nsMsgViewIndex FindMsgHdr(nsIMsgDBHdr *hdr) MOZ_OVERRIDE; > + virtual nsMsgViewIndex AddMsgHdrInDateOrder(nsIMsgDBHdr *child, nsMsgDBView *view) MOZ_OVERRIDE; nit: wrapping
Attachment #755332 -
Flags: review?(mozilla) → review+
Comment 56•11 years ago
|
||
Comment on attachment 755333 [details] [diff] [review] Annotate 40 methods with MOZ_OVERRIDE in mailnews/db Review of attachment 755333 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/db/msgdb/public/nsMailDatabase.h @@ +22,5 @@ > public: > nsMailDatabase(); > virtual ~nsMailDatabase(); > + NS_IMETHOD ForceClosed() MOZ_OVERRIDE; > + NS_IMETHOD DeleteMessages(uint32_t aNumKeys, nsMsgKey* nsMsgKeys, nsIDBChangeListener *instigator) MOZ_OVERRIDE; nit: line length @@ +35,2 @@ > > + NS_IMETHOD GetOfflineOpForKey(nsMsgKey opKey, bool create, nsIMsgOfflineImapOperation **op) MOZ_OVERRIDE; nit: line length
Attachment #755333 -
Flags: review?(mozilla) → review+
Comment 57•11 years ago
|
||
Comment on attachment 755334 [details] [diff] [review] Annotate 163 methods with MOZ_OVERRIDE in mailnews/imap Review of attachment 755334 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/imap/src/nsIMAPBodyShell.h @@ +134,3 @@ > // Generates an HTML representation of this part. Returns content length generated, -1 if failed. > + virtual int32_t Generate(nsIMAPBodyShell *aShell, bool stream, bool prefetch) MOZ_OVERRIDE; > + virtual nsIMAPBodypart *FindPartWithNumber(const char *partNum) MOZ_OVERRIDE; // Returns the part object with the given number nit: could you fix the indentation, and put the comment on the previous line please? @@ +176,5 @@ > virtual ~nsIMAPBodypartMessage(); > + virtual int32_t Generate(nsIMAPBodyShell *aShell, bool stream, bool prefetch) MOZ_OVERRIDE; > + virtual bool ShouldFetchInline(nsIMAPBodyShell *aShell) MOZ_OVERRIDE; > + virtual bool PreflightCheckAllInline(nsIMAPBodyShell *aShell) MOZ_OVERRIDE; > + virtual nsIMAPBodypart *FindPartWithNumber(const char *partNum) MOZ_OVERRIDE; // Returns the part object with the given number nit: indentation & comment ::: mailnews/imap/src/nsImapIncomingServer.h @@ +66,3 @@ > NS_IMETHOD GetNumIdleConnections(int32_t *aNumIdleConnections); > + NS_IMETHOD ForgetSessionPassword() MOZ_OVERRIDE; > + NS_IMETHOD GetMsgFolderFromURI(nsIMsgFolder *aFolderResource, const nsACString& aURI, nsIMsgFolder **aFolder) MOZ_OVERRIDE; nit: line wrapping please
Attachment #755334 -
Flags: review?(mozilla) → review+
Comment 58•11 years ago
|
||
Comment on attachment 755336 [details] [diff] [review] Annotate 105 methods with MOZ_OVERRIDE in mailnews/local Review of attachment 755336 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/local/src/nsRssIncomingServer.h @@ +27,5 @@ > + NS_IMETHOD GetOfflineSupportLevel(int32_t *aSupportLevel) MOZ_OVERRIDE; > + NS_IMETHOD GetSupportsDiskSpace(bool *aSupportsDiskSpace) MOZ_OVERRIDE; > + NS_IMETHOD GetAccountManagerChrome(nsAString& aResult) MOZ_OVERRIDE; > + NS_IMETHOD PerformBiff(nsIMsgWindow *aMsgWindow) MOZ_OVERRIDE; > + NS_IMETHOD GetServerRequiresPasswordForBiff(bool *aServerRequiresPasswordForBiff) MOZ_OVERRIDE; nit: line length
Attachment #755336 -
Flags: review?(mbanner) → review+
Comment 59•11 years ago
|
||
Thanks for doing these Arnaud.
Assignee | ||
Comment 60•11 years ago
|
||
The same one including your comments
Attachment #755332 -
Attachment is obsolete: true
Attachment #765396 -
Flags: review?(mbanner)
Assignee | ||
Comment 61•11 years ago
|
||
The same one including your comments
Attachment #755333 -
Attachment is obsolete: true
Attachment #765398 -
Flags: review?(mbanner)
Assignee | ||
Comment 62•11 years ago
|
||
The same one including your comments
Attachment #755334 -
Attachment is obsolete: true
Attachment #765400 -
Flags: review?(mbanner)
Assignee | ||
Comment 63•11 years ago
|
||
The same one including your comments
Attachment #755336 -
Attachment is obsolete: true
Attachment #765401 -
Flags: review?(mbanner)
Assignee | ||
Comment 64•11 years ago
|
||
The last one with your previous comments My pleasure :)
Attachment #754389 -
Attachment is obsolete: true
Attachment #765403 -
Flags: review?(mbanner)
Updated•11 years ago
|
Attachment #765396 -
Flags: review?(mbanner) → review+
Updated•11 years ago
|
Attachment #765398 -
Flags: review?(mbanner) → review+
Updated•11 years ago
|
Attachment #765400 -
Flags: review?(mbanner) → review+
Updated•11 years ago
|
Attachment #765401 -
Flags: review?(mbanner) → review+
Updated•11 years ago
|
Attachment #765403 -
Flags: review?(mbanner) → review+
Updated•11 years ago
|
Attachment #764033 -
Flags: review?(Pidgeot18) → review+
Comment 65•11 years ago
|
||
Comment on attachment 764034 [details] [diff] [review] Annotate 196 methods with MOZ_OVERRIDE in mailnews/addrbook + fix indentation Review of attachment 764034 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK to me.
Attachment #764034 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 66•11 years ago
|
||
this patch is the same but we are able to merge it with last thunderbird repo version
Attachment #765398 -
Attachment is obsolete: true
Attachment #775326 -
Flags: review+
Assignee | ||
Comment 67•11 years ago
|
||
this patch is the same but we are able to merge it with last thunderbird repo version
Attachment #764033 -
Attachment is obsolete: true
Attachment #775327 -
Flags: review+
Assignee | ||
Comment 68•11 years ago
|
||
Tbpl is here, full green https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=c7d098928ea9
Keywords: checkin-needed
Comment 69•11 years ago
|
||
Seriously, mark old patches obsolete when you post newer ones. Not doing so makes it a nightmare for someone else trying to help land things for you.
Comment 70•11 years ago
|
||
Nevermind me, I was being dumb... https://hg.mozilla.org/comm-central/rev/b34d771ffae6 https://hg.mozilla.org/comm-central/rev/4b57451b4319 https://hg.mozilla.org/comm-central/rev/e6fdf34bd222 https://hg.mozilla.org/comm-central/rev/d3949b664af8 https://hg.mozilla.org/comm-central/rev/f4773fcc281d https://hg.mozilla.org/comm-central/rev/4da44efb7fde https://hg.mozilla.org/comm-central/rev/7b91af46ee27 https://hg.mozilla.org/comm-central/rev/5021826aed3e https://hg.mozilla.org/comm-central/rev/ca0d6b4f36da
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 25.0
You need to log in
before you can comment on or make changes to this bug.
Description
•