Closed Bug 1365585 Opened 4 years ago Closed 4 years ago

localstore.rdf is still packaged

Categories

(Thunderbird :: Installer, defect)

defect
Not set
normal

Tracking

(thunderbird_esr5254+ fixed, thunderbird54 fixed, thunderbird55 fixed)

RESOLVED FIXED
Thunderbird 55.0
Tracking Status
thunderbird_esr52 54+ fixed
thunderbird54 --- fixed
thunderbird55 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(1 file, 2 obsolete files)

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?
Aceman, Magnus, what do you think?
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
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?
Flags: needinfo?(acelists)
Yes, both that and localstore-safe.rdf should be ok to stop shipping.
Flags: needinfo?(mkmelin+mozilla)
Attached patch localstore.patch (obsolete) — Splinter Review
This removes it. The localstore-safe.rdf is not in the tree.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8871423 - Flags: review?(acelists)
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?
Attachment #8871423 - Flags: feedback+
(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.
Attachment #8871423 - Attachment is obsolete: true
Attachment #8871423 - Flags: review?(acelists)
Attachment #8871701 - Flags: review?(acelists)
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.
Attachment #8871701 - Flags: review?(acelists) → review+
(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.
Thanks
Keywords: checkin-needed
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 ;-)
Attachment #8871701 - Flags: approval-comm-esr52?
Attachment #8871701 - Flags: approval-comm-beta+
Attached patch xulstoreDelete.patch (obsolete) — Splinter Review
This is only the safeMode.js part for ESR without the localstore.rdf removal to only fix a regression.
Attachment #8872131 - Flags: review+
Attachment #8872131 - Flags: approval-comm-esr52?
Attachment #8871701 - Flags: approval-comm-esr52?
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.
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. :-)
Attachment #8872131 - Attachment is obsolete: true
Attachment #8872131 - Flags: approval-comm-esr52?
Comment on attachment 8871701 [details] [diff] [review]
localstore.patch v2

I don't listen to all complaints ;-)
Attachment #8871701 - Flags: approval-comm-esr52?
Target Milestone: --- → Thunderbird 55.0
Attachment #8871701 - Flags: approval-comm-esr52? → approval-comm-esr52+
You need to log in before you can comment on or make changes to this bug.