798669 - nsImapMailFolder::GetOfflineMsgFolder should be made accessible to js

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David Lechner (:dlech)]

Followup bug to item 9 in https://bugzilla.mozilla.org/show_bug.cgi?id=721316#c60

nsImapMailFolder::GetOfflineMsgFolder is not exposed in nsIMsgImapMailFolder.idl
As a result, we don't have a way to write xpcshell tests to test if it is working right.

Related source code:
https://mxr.mozilla.org/comm-central/ident?i=GetOfflineMsgFolder&tree=comm-central&filter=

Comment 1

[Atul Jangra [:atuljangra]]

Do we really want to nsImapMailFolder::GetOfflineMsgFolder accessible to js?
( I am not against it or something, just confirming :) )
CC'ed Standard8.

Comment 2

[Mark Banner (:standard8)]

Hmm, Atul has an interesting question. In some ways I could see this being tested via HasMsgOffline and GetOfflineFileStream - we can easily have test files for the profile, and then the unit test can try and get the stream, if the stream doesn't succeed, then obviously its not picking it up.

On the other hand, I don't think it'd hurt to expose, as GetOfflineFileStream is exposed, and it may even be useful to extensions, though I can't think of a use case at the moment.

Comment 3

[Atul Jangra [:atuljangra]]

Attachment: First Version, just to get started

Added a method nsMsgDBFolder::GetOfflineMsgFolder, so that things go well in general.
Also function is now accessible to idl file, thus exposed to extensions.

Comment 4

[Atul Jangra [:atuljangra]]

Confirming the bug based on c#2

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 675431 [details] [diff] [review]
First Version, just to get started

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

The general concept is fine for me, so sr=me. Passing the actual review to irving.

::: mailnews/base/public/nsIMsgFolder.idl
@@ +532,5 @@
>                                        out long long aOffset,
>                                        out unsigned long aSize);
>  
>    /**
> +   * Get the folder where the msg could be present. 

nit: trailing whitespaces in several places (use the splinter review to see where these show up).

@@ +534,5 @@
>  
>    /**
> +   * Get the folder where the msg could be present. 
> +   * @param msgKey  key of the msg for which we are trying to get the folder;
> +   * @param[out] aMsgFolder  required folder;   

We normally use @returns (as per the function below)

@@ +537,5 @@
> +   * @param msgKey  key of the msg for which we are trying to get the folder;
> +   * @param[out] aMsgFolder  required folder;   
> +   *
> +   */
> +  nsIMsgFolder GetOfflineMsgFolder(in nsMsgKey msgKey);

You need to change the uuid for nsIMsgFolder.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +5969,5 @@
> +      NS_IF_ADDREF(*aMsgFolder = this);
> +      return NS_OK;
> +    }
> +  }
> +  return NS_OK;  

nit: whitespace at end of line.

Comment 6

[Atul Jangra [:atuljangra]]

Attachment: Fixed the nits

Fixed the nits pointed out by Standard8. Now waiting for Irving's review. :-)

Comment 7

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 676298 [details] [diff] [review]
Fixed the nits

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

::: mailnews/base/public/nsIMsgFolder.idl
@@ +538,5 @@
> +   * @returns aMsgFolder  required folder;
> +   *
> +   */
> +  nsIMsgFolder GetOfflineMsgFolder(in nsMsgKey msgKey);
> +  

Trailing white space

Comment 8

[Atul Jangra [:atuljangra]]

Attachment: Everything fixed, good to go :)

Fixed everything, already got sr+ by Standard8 and r+ by irving. Good to go. :)

Comment 9

[Atul Jangra [:atuljangra]]

For the person who will be landing this:
Please land the last attachment. Whitespace has been fixed in the last patch.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 676298 [details] [diff] [review]
Fixed the nits

Please mark obsolete patches as such. It'll save you having to make comments like the last one :)

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/2cd71031e850

Also, for the future, bugs shouldn't be resolved until the patch actually lands.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Building on Windows is also a good thing to do. Backed out.
https://hg.mozilla.org/comm-central/rev/7b51fd24e5bb

https://tbpl.mozilla.org/php/getParsedLog.php?id=16874288&tree=Thunderbird-Trunk

nsImapIncomingServer.cpp

e:\builds\moz2_slave\tb-c-cen-w32\build\mailnews\imap\src\nsImapMailFolder.h(321) : error C2695: 'nsImapMailFolder::GetOfflineMsgFolder': overriding virtual function differs from 'nsMsgDBFolder::GetOfflineMsgFolder' only by calling convention

        e:\builds\moz2_slave\tb-c-cen-w32\build\objdir-tb\mozilla\dist\include\nsMsgDBFolder.h(57) : see declaration of 'nsMsgDBFolder::GetOfflineMsgFolder'

Comment 13

[Atul Jangra [:atuljangra]]

Thanks for the corrections Ryan :). I will keep this in mind.

Comment 14

[Atul Jangra [:atuljangra]]

Attachment: Final Patch

Everything good to go. Tested on try server. Thanks Ryan for your help :-)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/4c9774a9a2a7

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Why was I marked as the reviewer for this patch? :/

Comment 17

[Atul Jangra [:atuljangra]]

Mike, I don't know :-/  I just saw it. Maybe Ryan knows the answer. :)