Closed Bug 1484372 Opened Last year Closed Last year

Port Bug 1484109 Modernize nsRelativeFilePref

Categories

(MailNews Core :: General, defect, blocker)

defect
Not set
blocker

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 63.0

People

(Reporter: jorgk, Assigned: jorgk)

References

Details

Attachments

(3 files, 4 obsolete files)

mailnews/base/util/nsMsgIncomingServer.cpp:493:10: error: use of undeclared identifier 'NS_NewRelativeFilePref'

Removed here:
https://hg.mozilla.org/mozilla-central/rev/0da5607c48f6#l2.27
I tried the attached patch but that doesn't compile since nsRelativeFilePref is not visible outside modules/libpref/Preferences.cpp:

https://hg.mozilla.org/mozilla-central/rev/0b2b4da68f04#l1.13

So what's the way forward here for TB?
Flags: needinfo?(n.nethercote)
Flags: needinfo?(ehsan)
Severity: normal → blocker
Never mind the copy paste errors, but the problem remains:
 0:08.12 c:/mozilla-source/comm-central/comm/mailnews/base/util/nsMsgIncomingServer.cpp(515,51):  error: unknown type name 'nsRelativeFilePref'; did you mean 'nsIRelativeFilePref'?
Attachment #9002133 - Attachment is obsolete: true
Keywords: leave-open
Since it's the weekend and we don't want our volunteers to sit idle or juggle local M-C backouts, here's something that works.

As far as I can see, the class is 100% stand-alone, so this fork should work for the time being. At least it compiles ;-) I can't really see what this class does other than store two member variables
Assignee: nobody → jorgk
Attachment #9002137 - Attachment is obsolete: true
Attachment #9002146 - Flags: feedback?(n.nethercote)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/da15c07bba7b
Port bug 1484109: replace use of NS_NewRelativeFilePref() and fork nsRelativeFilePref. rs=bustage-fix CLOSED TREE
Looks like the Linux and Mac linkers didn't like the forked class. So let's change it's name.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f70c7f479a97
Follow-up: rename forked class nsRelativeFilePref. rs=bustage-fix CLOSED TREE
Hmm, without the rename, the Linux/Mac linker failed with errors like:
https://taskcluster-artifacts.net/EAWc0h7tTj2cee55XpJ69Q/0/public/logs/live_backing.log
multiple definition of `nsRelativeFilePref::QueryInterface(nsID const&, void**)'
multiple definition of `nsRelativeFilePref::GetRelativeToKey(nsTSubstring<char>&)' etc. down to
multiple definition of `vtable for nsRelativeFilePref'

With the rename these changed to:
multiple definition of `nsRelativeFilePref2::QueryInterface(nsID const&, void**)'
multiple definition of `nsRelativeFilePref2::GetRelativeToKey(nsTSubstring<char>&)' etc. down to
multiple definition of `vtable for nsRelativeFilePref2'

Looks like I forgot to rename at:
https://hg.mozilla.org/comm-central/rev/da15c07bba7b#l3.15
+  NS_DECL_NSIRELATIVEFILEPREF
Nope, NS_DECL_NSIRELATIVEFILEPREF2 does not exist and NS_DECL_NSIRELATIVEFILEPREF is simply:
#define NS_DECL_NSIRELATIVEFILEPREF \
  NS_IMETHOD GetFile(nsIFile **aFile) override; \
  NS_IMETHOD SetFile(nsIFile *aFile) override; \
  NS_IMETHOD GetRelativeToKey(nsACString& aRelativeToKey) override; \
  NS_IMETHOD SetRelativeToKey(const nsACString& aRelativeToKey) override;

So some C++ magic somewhere is resisting this fork :-(
The Mac compiler gives a somewhat clearer message:
[snip]
00:30:56     INFO -  duplicate symbol __ZN19nsRelativeFilePref216SetRelativeToKeyERK12nsTSubstringIcE in:
00:30:56     INFO -      ../../comm/mailnews/base/util/nsMsgIncomingServer.o
00:30:56     INFO -      ../../comm/mailnews/base/util/nsMsgUtils.o
00:30:56     INFO -  duplicate symbol __ZTV19nsRelativeFilePref2 in:
00:30:56     INFO -      ../../comm/mailnews/base/util/nsMsgIncomingServer.o
00:30:56     INFO -      ../../comm/mailnews/base/util/nsMsgUtils.o
00:30:56     INFO -  ld: 13 duplicate symbols for architecture x86_64

Looks like both nsMsgIncomingServer and nsMsgUtils set up the class.
OK, let's backout the first follow-up and try with this one instead.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a6910b695ac3
Backed out follow-up (changeset f70c7f479a97). a=jorgk
https://hg.mozilla.org/comm-central/rev/dfb69d1bfa57
Follow-up, take 2: Avoid name clash of nsRelativeFilePref classes. rs=bustage-fix CLOSED TREE DONTBUILD
Attachment #9002183 - Flags: feedback?(n.nethercote)
Attachment #9002159 - Attachment description: 1484372-fup.patch → 1484372-fup.patch [landed and backed out]
Attachment #9002159 - Attachment is obsolete: true
Comment on attachment 9002146 [details] [diff] [review]
1484372-NS_NewRelativeFilePref.patch (v2)

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

Thanks, r+ if it works.
Attachment #9002146 - Flags: review+
Comment on attachment 9002183 [details] [diff] [review]
1484372-fup2.patch

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

Some ugly games here ;)
(In reply to :aceman from comment #14)
> Thanks, r+ if it works.
That's the entire point, it DOES NOT work. You need the "ugly games" from part two.

I think M-C should make class nsRelativeFilePref visible again and then we can get rid of the "ugly games". But to get the show back on the road, there was no option.
Comment on attachment 9002183 [details] [diff] [review]
1484372-fup2.patch

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

::: mailnews/base/util/nsRelativeFilePref.h
@@ +17,5 @@
>    nsCString mRelativeToKey;
>  };
>  
> +NS_IMPL_ISUPPORTS(NSRELATIVEFILEPREF_CLASS, nsIRelativeFilePref)
> +NSRELATIVEFILEPREF_CLASS::NSRELATIVEFILEPREF_CLASS() = default;

I suspect Ehsan won't be sympathetic about backing out his change. If that's the case, I think what you've done is reasonable, but I would do two things differently.

- Don't play games with the preprocessor, just give this class a different name and use it throughout. (`nsRelativeFilePref2` is one possibility. You could also do something like `nsRelativeToKeyFilePref`.)

- Pass the two arguments to the constructor and call SetFile() and SetRelativeToKey() within it.
Attachment #9002183 - Flags: feedback?(n.nethercote) → feedback+
Attachment #9002146 - Flags: feedback?(n.nethercote) → feedback+
Flags: needinfo?(n.nethercote)
Thanks for the comment.

(In reply to Nicholas Nethercote [:njn] from comment #17)
> - Don't play games with the preprocessor, just give this class a different
> name and use it throughout. (`nsRelativeFilePref2` is one possibility. You
> could also do something like `nsRelativeToKeyFilePref`.)
Did you miss comment #8 and comment #10 and
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=f70c7f479a97f733c4d40924e276fee99d76fdb2
https://hg.mozilla.org/comm-central/rev/f70c7f479a97f733c4d40924e276fee99d76fdb2
I think I did what you suggested, but since nsMsgIncomingServer.cpp and nsMsgUtils.cpp are in the same "compile unit" on Linux/Mac (works on Windows with clang-cl), so it clashes at link time, hence the pre-processor games.

> - Pass the two arguments to the constructor and call SetFile() and
> SetRelativeToKey() within it.
I don't understand that comment.

Or should be nsRelativeFilePref2 moved to its own .cpp file so it only compiles once?

> I suspect Ehsan won't be sympathetic about backing out his change.
Well, is there no way to make the class accessible outside modules/libpref/Preferences.cpp?
Flags: needinfo?(n.nethercote)
Attached patch 1484372-fup-take3.patch (obsolete) — Splinter Review
This needs to be applied on a backout of "follow-up, take 2". This should also build on Mac/Linux, but let's make sure:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b5192da4ddb3188635554d0483401b0886908b4f
Flags: needinfo?(n.nethercote)
Attachment #9002396 - Flags: review?(n.nethercote)
Hmm, I feel like there would be very little if any downsides to m-c exporting a header containing the declaration of nsRelativeFilePref, if Nick would accept such a patch.  Basically put this class into its own header in modules/libpref <https://searchfox.org/mozilla-central/rev/83562422ecb0f5683da7a9a26ce05ce62bc0c882/modules/libpref/Preferences.cpp#2312>, add the include guards and the license/#include boilerplates, and export the header here: https://searchfox.org/mozilla-central/rev/83562422ecb0f5683da7a9a26ce05ce62bc0c882/modules/libpref/moz.build#27, and #include it in Preferences.cpp.  That way, c-c should be able to just #include it and create a new instance from nsRelativeFilePref using operator new similar to m-c...
Flags: needinfo?(ehsan)
Depends on: 1484809
Comment on attachment 9002396 [details] [diff] [review]
1484372-fup-take3.patch

(In reply to :Ehsan Akhgari from comment #20)
> Hmm, I feel like there would be very little if any downsides to m-c
> exporting a header containing the declaration of nsRelativeFilePref, ...
Thanks Ehsan, will do, bug 1484809.
Attachment #9002396 - Attachment is obsolete: true
Attachment #9002396 - Flags: review?(n.nethercote)
This works together with the patch from bug 1484809.
Attachment #9002582 - Flags: review?(n.nethercote)
Attachment #9002582 - Flags: review?(n.nethercote) → review+
Keywords: leave-open
Target Milestone: --- → Thunderbird 63.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5a3262e69fc7
Follow-up, take 3: Use nsRelativeFilePref.h provided by M-C in bug 1484809. r=njn
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.