Last Comment Bug 798669 - nsImapMailFolder::GetOfflineMsgFolder should be made accessible to js
: nsImapMailFolder::GetOfflineMsgFolder should be made accessible to js
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: IMAP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 19.0
Assigned To: Atul Jangra [:atuljangra]
:
Mentors:
Depends on:
Blocks: 798145
  Show dependency treegraph
 
Reported: 2012-10-05 17:41 PDT by David Lechner (:dlech)
Modified: 2012-11-18 05:12 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
First Version, just to get started (3.47 KB, patch)
2012-10-25 20:37 PDT, Atul Jangra [:atuljangra]
standard8: superreview+
Details | Diff | Splinter Review
Fixed the nits (4.27 KB, patch)
2012-10-29 13:59 PDT, Atul Jangra [:atuljangra]
irving: review+
Details | Diff | Splinter Review
Everything fixed, good to go :) (4.27 KB, patch)
2012-11-08 14:06 PST, Atul Jangra [:atuljangra]
no flags Details | Diff | Splinter Review
Final Patch (5.55 KB, patch)
2012-11-10 16:28 PST, Atul Jangra [:atuljangra]
no flags Details | Diff | Splinter Review

Description David Lechner (:dlech) 2012-10-05 17:41:30 PDT
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] 2012-10-23 19:16:09 PDT
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) (afk until 26th July) 2012-10-24 01:25:56 PDT
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] 2012-10-25 20:37:41 PDT
Created attachment 675431 [details] [diff] [review]
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] 2012-10-25 20:38:36 PDT
Confirming the bug based on c#2
Comment 5 Mark Banner (:standard8) (afk until 26th July) 2012-10-29 12:23:36 PDT
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] 2012-10-29 13:59:56 PDT
Created attachment 676298 [details] [diff] [review]
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) 2012-11-08 13:56:23 PST
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] 2012-11-08 14:06:14 PST
Created attachment 679827 [details] [diff] [review]
Everything fixed, good to go :)

Fixed everything, already got sr+ by Standard8 and r+ by irving. Good to go. :)
Comment 9 Atul Jangra [:atuljangra] 2012-11-08 14:14:48 PST
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] 2012-11-08 14:56:32 PST
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] 2012-11-08 14:58:21 PST
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] 2012-11-08 16:07:52 PST
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] 2012-11-08 21:53:41 PST
Thanks for the corrections Ryan :). I will keep this in mind.
Comment 14 Atul Jangra [:atuljangra] 2012-11-10 16:28:02 PST
Created attachment 680395 [details] [diff] [review]
Final Patch

Everything good to go. Tested on try server. Thanks Ryan for your help :-)
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-11-11 04:05:01 PST
https://hg.mozilla.org/comm-central/rev/4c9774a9a2a7
Comment 16 Mike Conley (:mconley) - (Needinfo me!) 2012-11-11 09:51:39 PST
Why was I marked as the reviewer for this patch? :/
Comment 17 Atul Jangra [:atuljangra] 2012-11-18 05:12:35 PST
Mike, I don't know :-/  I just saw it. Maybe Ryan knows the answer. :)

Note You need to log in before you can comment on or make changes to this bug.