nsImapMailFolder::GetOfflineMsgFolder should be made accessible to js

RESOLVED FIXED in Thunderbird 19.0

Status

MailNews Core
Networking: IMAP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dlech, Assigned: atuljangra)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 19.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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=
(Reporter)

Updated

5 years ago
Blocks: 798145
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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.
Attachment #675431 - Flags: review?(mbanner)
(Assignee)

Comment 4

5 years ago
Confirming the bug based on c#2
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 6

5 years ago
Created attachment 676298 [details] [diff] [review]
Fixed the nits

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+
(Assignee)

Comment 8

5 years ago
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. :)
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(Assignee)

Comment 9

5 years ago
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 → ---
(Assignee)

Comment 13

5 years ago
Thanks for the corrections Ryan :). I will keep this in mind.
(Assignee)

Comment 14

5 years ago
Created attachment 680395 [details] [diff] [review]
Final Patch

Everything good to go. Tested on try server. Thanks Ryan for your help :-)
Attachment #679827 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/4c9774a9a2a7
Keywords: checkin-needed
Target Milestone: --- → Thunderbird 19.0
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Why was I marked as the reviewer for this patch? :/
(Assignee)

Comment 17

5 years ago
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.