1365585 - localstore.rdf is still packaged

Status: RESOLVED FIXED

(Thunderbird :: Installer, defect)

Description

[Richard Marti (:Paenglab)]

localstore.rdf is no more used since we use XULStore.json introduced with bug 559505.

We still package an empty (almost empty, it has only headers in it) localstore.rdf file in every build.

Shouldn't we stop doing this?

Comment 1

[Richard Marti (:Paenglab)]

Aceman, Magnus, what do you think?

Comment 2

[:aceman]

Yeah, I have this file in my profiles but it wasn't touched in years. It can probably go. There is also some localstore-safe.rdf. Is that the same case?

Comment 3

[Magnus Melin [:mkmelin]]

Yes, both that and localstore-safe.rdf should be ok to stop shipping.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: localstore.patch

This removes it. The localstore-safe.rdf is not in the tree.

Comment 5

[:aceman]

Comment on attachment 8871423 [details] [diff] [review]
localstore.patch

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

It seems this needs some more thought.
We still reference this file at: https://dxr.mozilla.org/comm-central/rev/d60f75d773252e42d92b614f9dab52b8988856c2/mail/base/content/safeMode.js#14
(see https://dxr.mozilla.org/comm-central/rev/f81bcc23d37d7bec48f08b19a9327e93c54d37b5/mozilla/toolkit/xre/nsXREDirProvider.cpp#520 ). So does Seamonkey.

Shouldn't we delete the new XULStore.json there too? Seamonkey does (https://dxr.mozilla.org/comm-central/rev/d60f75d773252e42d92b614f9dab52b8988856c2/suite/common/safeMode.js#44).

The handling of localStore-safe.rdf is strange: https://dxr.mozilla.org/comm-central/rev/f81bcc23d37d7bec48f08b19a9327e93c54d37b5/mozilla/toolkit/xre/nsXREDirProvider.cpp#525 . It is deleted when found?

Comment 6

[Richard Marti (:Paenglab)]

Attachment: localstore.patch v2

(In reply to :aceman from comment #5)
> Comment on attachment 8871423 [details] [diff] [review]
> localstore.patch
> 
> Review of attachment 8871423 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems this needs some more thought.
> We still reference this file at:
> https://dxr.mozilla.org/comm-central/rev/
> d60f75d773252e42d92b614f9dab52b8988856c2/mail/base/content/safeMode.js#14
> (see
> https://dxr.mozilla.org/comm-central/rev/
> f81bcc23d37d7bec48f08b19a9327e93c54d37b5/mozilla/toolkit/xre/
> nsXREDirProvider.cpp#520 ). So does Seamonkey.
> 
> Shouldn't we delete the new XULStore.json there too?

Yes, added. I leave the deletion of localstore in it as when we delete xulstore it could use at the next start localstore to convert to xulstore. Is this correct?

> The handling of localStore-safe.rdf is strange:
> https://dxr.mozilla.org/comm-central/rev/
> f81bcc23d37d7bec48f08b19a9327e93c54d37b5/mozilla/toolkit/xre/
> nsXREDirProvider.cpp#525 . It is deleted when found?

Yes, but this is in m-c and I won't touch it.

PS: I think we need to uplift this patch (or only the safeMode.js part) to 52 as now the "Reset toolbars and controls" doesn't work because it never touches xulstore.

Comment 7

[:aceman]

Comment on attachment 8871701 [details] [diff] [review]
localstore.patch v2

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

Yes, thanks.

::: mail/base/content/safeMode.js
@@ +14,5 @@
>    var localstoreFile = Services.dirsvc.get("LStoreS", Components.interfaces.nsIFile);
>    if (localstoreFile.exists())
>      localstoreFile.remove(false);
> +  // Delete the new xulstore file.
> +  localstoreFile.leafName = "xulstore.json";

I wonder why there isn't a constant into Services.dirsvc.get() for this file, but whatever. They want to kill XUL.

Comment 8

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #6)
> > Shouldn't we delete the new XULStore.json there too?
> Yes, added. I leave the deletion of localstore in it as when we delete
> xulstore it could use at the next start localstore to convert to xulstore.
> Is this correct?

Yes. We will not ship localstore.rdf from now on, but users still have it in their profiles, so we want to remove it on occasion.
 
> > The handling of localStore-safe.rdf is strange:
> > https://dxr.mozilla.org/comm-central/rev/
> > f81bcc23d37d7bec48f08b19a9327e93c54d37b5/mozilla/toolkit/xre/
> > nsXREDirProvider.cpp#525 . It is deleted when found?
> 
> Yes, but this is in m-c and I won't touch it.

Sure, I din't ask for that, I just wondered what that does.
 
> PS: I think we need to uplift this patch (or only the safeMode.js part) to
> 52 as now the "Reset toolbars and controls" doesn't work because it never
> touches xulstore.

I think so too. Resetting toolbars probably does nothing now. Seamonkey noticed the change 3 years ago in bug 1056635 and we didn't adapt TB.

Comment 9

[Richard Marti (:Paenglab)]

Thanks

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8871701 [details] [diff] [review]
localstore.patch v2

Let's land it first though. Sorry about the delay, but I had nicer patches to land ;-)

Comment 11

[Richard Marti (:Paenglab)]

Attachment: xulstoreDelete.patch

This is only the safeMode.js part for ESR without the localstore.rdf removal to only fix a regression.

Comment 12

[Jorg K (CEST = GMT+2)]

That's wrong with using the entire patch on ESR 52? Doesn't hurt, does it? As far as I know localstore.rdf is deprecated even in TB 52.

Comment 13

[Richard Marti (:Paenglab)]

In a PM you wrote, it was reported, too much is uplifted to ESR and only regressions should be uplifted. But you are the 52 maintainer. If you're okay with the full patch, then okay. :-)

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8871701 [details] [diff] [review]
localstore.patch v2

I don't listen to all complaints ;-)

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/4aeb9d455545b3654becf90c24ba45dfe1ca37c6

Comment 16

[Jorg K (CEST = GMT+2)]

Beta (TB 54):
https://hg.mozilla.org/releases/comm-beta/rev/d55124369888943e095aa0248817e1472c0c057e

Comment 17

[Jorg K (CEST = GMT+2)]

TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/f26bb7ef90a4c8b50ddbf08b56e08337778e1770