1351453 - 1) Stop using prettiestName attribute of nsIMsgFolder 2) Remove prettiestName attribute of nsIMsgFolder

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[:aceman]

Is the prettiestName attribute on nsIMsgFolder used for anything? It is only defined in nsMsgDBFolder.cpp and falls back to prettyName or Name. This behaviour is not overridden in any server type.

There are about 80 uses of it in m-c (prettyName is used about 500 times).
There are about 170 hits in addons.

Does anybody know what this is good for?
The idl file has no documentation on it:
https://dxr.mozilla.org/comm-central/rev/8eeb5667420daba2148746b6b9e470d0d8ebc35d/mailnews/base/public/nsIMsgFolder.idl#49

Can it be removed?

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Got added in bug 218825. But prettyName just calls name: https://hg.mozilla.org/comm-central/annotate/2967d03cf5bc66717e3ff62ece9fc94faeae45e6/mailnews/base/util/nsMsgDBFolder.cpp#l3393

Comment 2

[:aceman]

Thanks.

But at least SetPrettyName does different stuff for different servers, so that one is useful. https://dxr.mozilla.org/comm-central/rev/2967d03cf5bc66717e3ff62ece9fc94faeae45e6/mailnews/base/util/nsMsgDBFolder.cpp#3398

But I don't know what prettiestName is good for.

If we remove it, we can check that prettyName also guarantees return of a name (falling back to name if needed).

Comment 3

[Kushal Singh]

Hello guys, 

If possible I'd like to work on this bug (as one of my starting bug). Based on the discussion it's still a bit unclear to me if we want the prettiestName to be refactored completely or not ?

Since the code contains https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.cpp#3536 
the GetPrettiestName method which does use the GetName method. So what exactly are we looking to fix here 

Any guidance would be appreciated :)

Thanks !
- Kushal

Comment 4

[:aceman]

(In reply to Kushal Singh from comment #3)
> Hello guys, 
> 
> If possible I'd like to work on this bug (as one of my starting bug). Based
> on the discussion it's still a bit unclear to me if we want the
> prettiestName to be refactored completely or not ?

I wanted to remove it as it seems to have no purpose/difference to prettyName attribute.

> Since the code contains
> https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgDBFolder.
> cpp#3536 
> the GetPrettiestName method which does use the GetName method. So what
> exactly are we looking to fix here 

First step would be to check if every implementation of GetPrettyName() falls back to GetName() if it can't provide a 'pretty name'. Then replace all calls of (nsIMsgFolder).prettiestName with .prettyName in Javacsript, and GetPrettiestName() with GetPrettyName() in C++, in the whole comm-central tree.

At the last step we can decide what to do with the addons, if to remove the attribute or not.

Comment 5

[Kushal Singh]

Attachment: firstDraft.diff

Removes the GetPrettiestName and .prettiest from the C++ and JS code respectively (for entire comm-central)

Comment 6

[Kushal Singh]

I tried to address the points you suggested in the comment #4 and made a patch. Kindly have a look.
Thanks !

Comment 7

[:aceman]

Comment on attachment 8872095 [details] [diff] [review]
firstDraft.diff

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

Thanks! I will check/build this more thoroughly later. It goes in the right direction.
Just split the actual removal of the attribute in a second patch.

It is interesting there were no uses in Seamonkey (/suite).

::: mailnews/base/public/nsIMsgFolder.idl
@@ -46,5 @@
>    /* get new headers for db */
>    void updateFolder(in nsIMsgWindow aWindow);
>  
> -  readonly attribute AString prettiestName;
> -

Please do not remove this yet. Split it off into a separate patch that you also upload here. Addons still use this attribute and we need to decide what to do with it.

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ -3537,5 @@
> -{
> -  if (NS_SUCCEEDED(GetPrettyName(name)))
> -    return NS_OK;
> -  return GetName(name);
> -}

Split this into the second patch too.

Comment 8

[Kushal Singh]

Attachment: mainPatch.diff

Updating the patch earlier to the new patch which excludes the removal of attribute [ As suggested in the feedback earlier ]

Comment 9

[Kushal Singh]

Attachment: attributeRemoved.diff

This patch [Part #2] deals with only the removal of the attribute , which is still being used (as suggested by @aceman

Comment 10

[:aceman]

Comment on attachment 8872140 [details] [diff] [review]
mainPatch.diff

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

Thanks, this seems to work. I haven't found any remaining occurences of prettiestName and all tests have passed:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ff4508ae0ee9aec40837539063f1a7463a0fbda1

Please change the commit message from "Remove the prettiestName attribute" to "Do not use prettiestName attribute..." as you are not removing it in this patch yet.

Comment 11

[:aceman]

(In reply to :aceman from comment #10)
> Please change the commit message from "Remove the prettiestName attribute"
> to "Do not use prettiestName attribute..." as you are not removing it in
> this patch yet.

Actually we may change the message when checking it in so you do not need to upload new version.

Comment 12

[:aceman]

Comment on attachment 8872141 [details] [diff] [review]
attributeRemoved.diff

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

This looks good to me. But the attribute is still used about 180 times in addons. So I'll forward to Magnus to decide when to land this.

Comment 13

[Jorg K (CEST = GMT+2)]

I think I'll land part 1 now before it rots. I'll change the commit message as per comment #11.

Thank you for your contribution.

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8872141 [details] [diff] [review]
attributeRemoved.diff

If it's used in add-ons, we can't remove it. We have a bad fame of breaking add-ons and here we'd break them for no good reason.

All we can do here is add [deprecated] to the IDL like here:
https://dxr.mozilla.org/comm-central/rev/2a593ea93f6637df49eedc998b1f5ae4781a0f56/mozilla/storage/mozIStorageBaseStatement.idl#55

and something like this to the C++ code like here:
https://dxr.mozilla.org/comm-central/rev/2a593ea93f6637df49eedc998b1f5ae4781a0f56/mozilla/storage/mozIStorageBaseStatement.idl#55
or like in attachment 8805914 [details] [diff] [review].

Nothing to review here right now.

Comment 15

[Kushal Singh]

(In reply to Jorg K (GMT+2) from comment #14)
> Comment on attachment 8872141 [details] [diff] [review]
> attributeRemoved.diff
> 
> If it's used in add-ons, we can't remove it. We have a bad fame of breaking
> add-ons and here we'd break them for no good reason.
> 
> All we can do here is add [deprecated] to the IDL like here:
> https://dxr.mozilla.org/comm-central/rev/
> 2a593ea93f6637df49eedc998b1f5ae4781a0f56/mozilla/storage/
> mozIStorageBaseStatement.idl#55
> 
> and something like this to the C++ code like here:
> https://dxr.mozilla.org/comm-central/rev/
> 2a593ea93f6637df49eedc998b1f5ae4781a0f56/mozilla/storage/
> mozIStorageBaseStatement.idl#55
> or like in attachment 8805914 [details] [diff] [review].
> 
> Nothing to review here right now.

I am a bit new , so I'll just make sure I understand what I need to work on next specifically.
For the .idl file I just need to add [deprecated] in front of the readonly attribute 
And for the .cpp file , do I need to issue the browser console error message just like shown in https://bugzilla.mozilla.org/attachment.cgi?id=8805914&action=diff , for the .cpp file ?

Thanks

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Kushal Singh from comment #15)
> I am a bit new , so I'll just make sure I understand what I need to work on
> next specifically.
> For the .idl file I just need to add [deprecated] in front of the readonly
> attribute 
That's what I said ;-)

> And for the .cpp file , do I need to issue the browser console error message
> just like shown in
> https://bugzilla.mozilla.org/attachment.cgi?id=8805914&action=diff , for the
> .cpp file ?
Yes, either like in attachment 8805914 [details] [diff] [review] or in fact more aggressively like here.
Damn, I posted the wrong link, here's the right one:
https://hg.mozilla.org/mozilla-central/rev/c2f7bd2c69db#l3.32
Just copy that without making it a macro.

Comment 17

[:aceman]

Yes, good idea Jorg.

(In reply to Kushal Singh from comment #15)
> I am a bit new , so I'll just make sure I understand what I need to work on
> next specifically.
> For the .idl file I just need to add [deprecated] in front of the readonly
> attribute 

Yes.

> And for the .cpp file , do I need to issue the browser console error message
> just like shown in
> https://bugzilla.mozilla.org/attachment.cgi?id=8805914&action=diff , for the
> .cpp file ?

Yes, just make the message say "...prettiestName is deprecated, use prettyName instead...".

Those will be the only changes replacing the current patch attributeRemoved.diff .

Thanks.

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #17)
> Yes, just make the message say "...prettiestName is deprecated, use
> prettyName instead...".
Actually, no, use the code from
https://hg.mozilla.org/mozilla-central/rev/c2f7bd2c69db#l3.32

Comment 19

[Kushal Singh]

Attachment: updatedPatchForDepreciating.diff

Based on the suggestions earlier , adding the warning and the deprecated tag.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8872183 [details] [diff] [review]
updatedPatchForDepreciating.diff

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

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3551,5 @@
> +        cs->LogMessage(e);
> +      }
> +    }
> +  }
> +  MOZ_ASSERT(false, "You are trying to use a deprecated mozStorage method. "

mozStorage? Cut&paste is nice, but at times you need to read and adapt ;-)

How about:
You are trying to use the deprecated attribute 'prettiestName'.

Comment 21

[Kushal Singh]

Attachment: updatedPatchForDepreciating.diff [#3]

Fixed the error :P pointed out by Jork [in the error message] and updated the patch , hopefully it'll work now.

Comment 22

[Kushal Singh]

Jorg K, have a look I hope it's okay now :)

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8872187 [details] [diff] [review]
updatedPatchForDepreciating.diff [#3]

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

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3552,5 @@
> +      }
> +    }
> +  }
> +  MOZ_ASSERT(false, "You are trying to use the deprecated attribute 'prettiestName'"
> +                    "Check error message in console to identify the method name.");

You know how strings work in C/C++, right?

"A" "B" gives "AB". So your message will read:
You are trying to use the deprecated attribute 'prettiestName'Check error message in console to identify the method name.

Ugly, huh? Just lose the "Check error message ...".

Comment 24

[Kushal Singh]

Attachment: updatedPatchForDepreciating.diff [Version #4]

Taken the string comment into consideration, kindly review.

Comment 25

[Kushal Singh]

Dropped the second part of the string "Check error message ...", as suggested in Comment #23

Comment 26

[Jorg K (CEST = GMT+2)]

Landed part 1:
https://hg.mozilla.org/comm-central/rev/5804118778fbcc679020ec4303996c27896ad4e5

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: updatedPatchForDepreciating.diff (v5).

Turns out that [deprecated] is not available for attributes, see:
https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Deprecatedtag
"This annotation can be added to interface methods or interfaces."

So I turned this into a comment since it didn't compile for me.

Locally backing out "Part 1" I get this in the error console as expected:
nsMsgDBFolder::GetPrettiestName is deprecated and will be removed soon.

So as far as I can see, this works as desired.

Comment 28

[:aceman]

Comment on attachment 8872297 [details] [diff] [review]
updatedPatchForDepreciating.diff (v5).

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

Thanks

::: mailnews/base/util/nsMsgDBFolder.cpp
@@ +3546,5 @@
> +
> +      nsCOMPtr<nsIScriptError> e = do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
> +      if (e && NS_SUCCEEDED(e->Init(NS_ConvertUTF8toUTF16(msg), EmptyString(),
> +                                    EmptyString(), 0, 0,
> +                                    nsIScriptError::errorFlag, "Storage"))) {

warningFlag should be enough

@@ +3551,5 @@
> +        cs->LogMessage(e);
> +      }
> +    }
> +  }
> +  MOZ_ASSERT(false, "You are trying to use the deprecated attribute 'prettiestName'.");

We could warn into the console, but not crash the app. If our developer is using one of the addons using this, why make life hard for him. We are not able to fix addons quickly.

Or can we distinguish whether the function is called from c++ core or from JS addon?

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #28)
> > +                                    nsIScriptError::errorFlag, "Storage"))) {
> warningFlag should be enough
Right. And what do we put instead of "Storage"?

> Or can we distinguish whether the function is called from c++ core or from
> JS addon?
Not that I know.

Comment 30

[Kushal Singh]

> We could warn into the console, but not crash the app. If our developer is
> using one of the addons using this, why make life hard for him. We are not
> able to fix addons quickly.

I did get the part about removing the MOZ_ASSERT so that the app doesn't crash, but the part about removing the warning flag was a bit unclear to me. Does that mean that we remove the warning displayed at the log ?

Comment 31

[Jorg K (CEST = GMT+2)]

Aceman is suggesting to use: nsIScriptError::warningFlag.

Since you copied this from
https://hg.mozilla.org/mozilla-central/rev/c2f7bd2c69db#l3.45
we should replace the word "Storage" with, hmm, "nsMsgDBFolder".

MOZ_ASSERT() should become NS_WARNING().

Comment 32

[:aceman]

(In reply to Jorg K (GMT+2) from comment #31)
> Since you copied this from
> https://hg.mozilla.org/mozilla-central/rev/c2f7bd2c69db#l3.45
> we should replace the word "Storage" with, hmm, "nsMsgDBFolder".

Yes, that would be the case if we had tons of similar warnings.
But currently there are a handful of them I'd suggest putting this new one into the already existing "mailnews" identifier.

Comment 33

[Jorg K (CEST = GMT+2)]

Attachment: updatedPatchForDepreciating.diff (v6).

Made the necessary adjustments.

Comment 34

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/5804118778fbcc679020ec4303996c27896ad4e5 - Part 1, comment #26
https://hg.mozilla.org/comm-central/rev/069cc93a3e3bd88d682e7899dc5b53722a174d2b - Part 2

Thanks for your contribution!

Comment 35

[Jorg K (CEST = GMT+2)]

This caused 15 Xpcshell failures:

TEST-UNEXPECTED-FAIL | mail/base/test/unit/test_emptyTrash_dbViewWrapper.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/db/gloda/test/unit/test_cleanup_msf_databases.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_copyThenMove.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_gmailOfflineMsgStore.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapAttachmentSaves.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapFolderCopy.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapAutoSync.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapStatusCloseDBs.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapUndo.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_copyThenMove.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_gmailOfflineMsgStore.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapFolderCopy.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapAutoSync.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapStatusCloseDBs.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | mailnews/imap/test/unit/test_imapUndo.js | xpcshell return code: 0 [log…]

Here a sample log:

https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-win32-debug/1496211376/comm-central_win7_ix-debug_test-xpcshell-bm112-tests1-windows-build37.txt.gz

00:51:37     INFO -  PID 10884 | [10884] WARNING: You are trying to use the deprecated attribute 'prettiestName'.: file c:/builds/moz2_slave/tb-c-cen-w32-d-000000000000000/build/mailnews/base/util/nsMsgDBFolder.cpp, line 3555
00:51:37     INFO -  PID 10884 | [10884] WARNING: You are trying to use the deprecated attribute 'prettiestName'.: file c:/builds/moz2_slave/tb-c-cen-w32-d-000000000000000/build/mailnews/base/util/nsMsgDBFolder.cpp, line 3555
00:51:37     INFO -  TEST-PASS | mail/base/test/unit/test_emptyTrash_dbViewWrapper.js | test_empty_trash - [test_empty_trash : 35] null != [object XPCWrappedNative_NoHelper]
00:51:37     INFO -  (xpcshell/head.js) | test _async_driver pending (3)
00:51:37     INFO -  (xpcshell/head.js) | test _async_driver finished (3)
00:51:37     INFO -  Error console says [stackFrame nsMsgDBFolder::GetPrettiestName is deprecated and will be removed soon.]
00:51:37     INFO -  ../../../../mailnews/resources/logHelper.js:observe:95
00:51:37     INFO -  C:\slave\test\build\tests\xpcshell\head.js:_do_main:222
00:51:37     INFO -  C:\slave\test\build\tests\xpcshell\head.js:_execute_test:550
00:51:37     INFO -  -e:null:1
00:51:37     INFO -  exiting test
00:51:37     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "nsMsgDBFolder::GetPrettiestName is deprecated and will be removed soon."]"
00:51:37     INFO -  Error console says [stackFrame nsMsgDBFolder::GetPrettiestName is deprecated and will be removed soon.]

And those warnings make the tests fail. 

I'll have to check in a local debug build to see where this is still called since I can't see it in DXR.

Comment 36

[Kushal Singh]

Thank you so much for all the help :aceman and Jorg :) , I feel slightly more confident now. Where can I find more mentored bugs that would have a higher difficulty compared to this one [for the next steps] .

- Kushal

Comment 37

[Jorg K (CEST = GMT+2)]

I ran
mozilla/mach xpcshell-test mail/base/test/unit/test_emptyTrash_dbViewWrapper.js
and got the JS warning.

For the record, you need to use --interactive to run it interactively so attach the debugger.

OK, I got a stack dump, nothing on the C++ stack, here's the JS stack:

[5500] WARNING: You are trying to use the deprecated attribute 'prettiestName'.: file c:/mozilla-source/comm-central/mailnews/base/util/nsMsgDBFolder.cpp, line 3555
0 getMessage(error = AssertionError, prefix = undefined) ["resource://testing-common/Assert.jsm":74]
1 Assert.AssertionError(options = [object Object]) ["resource://testing-common/Assert.jsm":107]
    this = AssertionError
2 proto.report(failed = false, actual = null, expected = [xpconnect wrapped nsIMsgFolder @ 0xa9adb50 (native @ 0xafbbc00)], message = undefined, operator = "!=") ["resource://testing-common/Assert.jsm":183]
    this = [object Object]
3 notEqual(actual = null, expected = [xpconnect wrapped nsIMsgFolder @ 0xa9adb50 (native @ 0xafbbc00)]) ["resource://testing-common/Assert.jsm":242]
    this = [object Object]
4 _do_check_neq(left = null, right = [xpconnect wrapped nsIMsgFolder @ 0xaaa6dc0 (native @ 0xafbbc00)], stack = JS frame :: c:/mozilla-source/comm-central/obj-i686-pc-mingw32/_tests/xpcshell/mail/base/test/unit/test_emptyTrash_dbViewWrapper.js :: test_empty_trash :: line 35, todo = false) ["c:\mozilla-source\comm-central\mozilla\testing\xpcshell\head.js":828]
    this = [object BackstagePass @ 0x71d7c10 (native @ 0x98f6454)]
5 do_check_neq(left = null, right = [xpconnect wrapped nsIMsgFolder @ 0xaaa6dc0 (native @ 0xafbbc00)]) ["c:\mozilla-source\comm-central\mozilla\testing\xpcshell\head.js":835]
    this = [object BackstagePass @ 0x71d7c10 (native @ 0x98f6454)]
6 test_empty_trash() ["c:/mozilla-source/comm-central/obj-i686-pc-mingw32/_tests/xpcshell/mail/base/test/unit/test_emptyTrash_dbViewWrapper.js":35]
    this = [object BackstagePass @ 0x71d7c10 (native @ 0x98f6454)]
7 InterpretGeneratorResume(gen = [object Generator], val = undefined, kind = "next") ["self-hosted":1250]
8 next(val = undefined) ["self-hosted":1157]
    this = [object Generator]
9 _async_driver() ["../../../../mailnews/resources/asyncTestUtils.js":158]
    this = [object BackstagePass @ 0x71d7c10 (native @ 0x98f6454)]
10 run() ["c:\mozilla-source\comm-central\mozilla\testing\xpcshell\head.js":710]
    this = [object Object]
11 _do_main() ["c:\mozilla-source\comm-central\mozilla\testing\xpcshell\head.js":222]
    this =

Sorry, I can't see this now in a hurry.

Comment 38

[Jorg K (CEST = GMT+2)]

Sorry, I had to back the second part out:
https://hg.mozilla.org/comm-central/rev/68f3ff4e37c98cef894272dff35e851d3dbcb772

Kushal, this is your bug, can you please investigate where this attribute is still used. You can run the tests as described in comment #37:
mozilla/mach xpcshell-test --interactive mail/base/test/unit/test_emptyTrash_dbViewWrapper.js

So sadly, we're not done here yet.

Comment 39

[Jorg K (CEST = GMT+2)]

Aceman said in a PM:
===
In test_emptyTrash_dbViewWrapper.js, the line "do_check_neq(null, viewWrapper.displayedFolder);" Does it do a comparision of objects (one null and one nsIMsgFolder)? May it do some enumerating of all attributes of the folder object? In that case it may hit also prettiestName.
===

Also, I don't like the way nsIMsgFolder.idl looks now:
  /* old nsIFolder properties and methods */
  readonly attribute ACString URI;
  attribute AString name;
  attribute AString prettyName;
  readonly attribute AString abbreviatedName;

So why are the "old"? I think we need a follow-up here. I'll leave it to the assignee and his mentor ;-)

Comment 40

[Jorg K (CEST = GMT+2)]

I guess Part 2 doesn't work like this. We might consider the approach from attachment 8805914 [details] [diff] [review], but then we'd still get that ugly warning during the tests.

Comment 41

[:aceman]

Attachment: patchForDepreciating v7

Could you live with this?
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7859e51290af36b556e6f916d24796434ea11b2a
Also the last failure is fixed in this version of the patch.

Comment 42

[Jorg K (CEST = GMT+2)]

So the warning doesn't get printed, right?

Can you fix nsIMsgFolder.idl as per comment #39. Maybe move |attribute AString prettyName;| up close to the deprecated one.

Comment 43

[:aceman]

Attachment: patchForDepreciating v7.1

Yes, the comparing of the object to null is done in a way that does not visit all properties.

I've grouped the 'name' attributes to the top of the idl.

Comment 44

[Jorg K (CEST = GMT+2)]

Comment on attachment 8873186 [details] [diff] [review]
patchForDepreciating v7.1

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

::: mailnews/base/public/nsIMsgFolder.idl
@@ +44,5 @@
> +  readonly attribute AString abbreviatedName;
> +  // [deprecated]
> +  readonly attribute AString prettiestName;
> +
> +  attribute nsIMsgFolder parent;

Why has this also moved?

@@ -615,5 @@
>  
>    /* does not persist across sessions */
>    attribute nsMsgKey lastMessageLoaded;
>  
> -  /* old nsIFolder properties and methods */

So what was old here?

Comment 45

[:aceman]

(In reply to Jorg K (GMT+2) from comment #44)
> > +  attribute nsIMsgFolder parent;
> Why has this also moved?

It appeared to me it would logically belong to the names.
 
> @@ -615,5 @@
> >    /* does not persist across sessions */
> >    attribute nsMsgKey lastMessageLoaded;
> >  
> > -  /* old nsIFolder properties and methods */
> So what was old here?

The current idl absorbed nsIFolder in bug 218825 so probably the 'old' was a reference to the origin of the attributes.

Comment 46

[Jorg K (CEST = GMT+2)]

Comment on attachment 8873186 [details] [diff] [review]
patchForDepreciating v7.1

OK, thanks.

Comment 47

[:aceman]

Thanks.

Comment 48

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/5804118778fbcc679020ec4303996c27896ad4e5 - Part 1, comment #26
https://hg.mozilla.org/comm-central/rev/6431fecfdf99c9cf927373c0de46a26c20428f32 - Part 2