Closed Bug 798669 Opened 8 years ago Closed 8 years ago

nsImapMailFolder::GetOfflineMsgFolder should be made accessible to js

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 19.0

People

(Reporter: dlech, Assigned: atuljangra)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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=
Blocks: 798145
Do we really want to nsImapMailFolder::GetOfflineMsgFolder accessible to js?
( I am not against it or something, just confirming :) )
CC'ed 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.
Added a method nsMsgDBFolder::GetOfflineMsgFolder, so that things go well in general.
Also function is now accessible to idl file, thus exposed to extensions.
Attachment #675431 - Flags: review?(mbanner)
Confirming the bug based on c#2
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → atuljangra66
Status: NEW → UNCONFIRMED
Ever confirmed: false
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.
Attachment #675431 - Flags: superreview+
Attachment #675431 - Flags: review?(mbanner)
Attachment #675431 - Flags: review?(irving)
Attached patch Fixed the nits (obsolete) — Splinter Review
Fixed the nits pointed out by Standard8. Now waiting for Irving's review. :-)
Attachment #675431 - Attachment is obsolete: true
Attachment #675431 - Flags: review?(irving)
Attachment #676298 - Flags: review?(irving)
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
Attachment #676298 - Flags: review?(irving) → review+
Attached patch Everything fixed, good to go :) (obsolete) — Splinter Review
Fixed everything, already got sr+ by Standard8 and r+ by irving. Good to go. :)
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
For the person who will be landing this:
Please land the last attachment. Whitespace has been fixed in the last patch.
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 :)
Attachment #676298 - Attachment is obsolete: true
https://hg.mozilla.org/comm-central/rev/2cd71031e850

Also, for the future, bugs shouldn't be resolved until the patch actually lands.
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 19.0
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'
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Target Milestone: Thunderbird 19.0 → ---
Thanks for the corrections Ryan :). I will keep this in mind.
Attached patch Final PatchSplinter Review
Everything good to go. Tested on try server. Thanks Ryan for your help :-)
Attachment #679827 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/4c9774a9a2a7
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 19.0
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Why was I marked as the reviewer for this patch? :/
Mike, I don't know :-/  I just saw it. Maybe Ryan knows the answer. :)
You need to log in before you can comment on or make changes to this bug.