1056649 - Port |Bug 559505 - localstore.rdf kills ponies| to Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: Build Config, defect)

Description

[Serge Gautherie (:sgautherie)]

+++ This bug was initially created as a clone of Bug #1056635 +++

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from bug 559505 comment #74)
> https://hg.mozilla.org/mozilla-central/rev/25c918c5f3e1

1) Trivial config part is to copy 'browser/installer/package-manifest.in' changes.
2) Probably port some of 'browser/components/*' changes.
   i.e. http://mxr.mozilla.org/comm-central/search?string=rdf%3Alocal-store&case=1&find=%2Fmail%2F
3) Check to see if anything else needs to be done too.

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #0)
> 2) Probably port some of 'browser/components/*' changes.
>    i.e.
> http://mxr.mozilla.org/comm-central/search?string=rdf%3Alocal-
> store&case=1&find=%2Fmail%2F

http://mxr.mozilla.org/comm-central/search?string=rdf%3Alocal-store&case=1&find=%2Fmail
{{
Found 3 matching lines in 3 files

/mail/base/modules/mailMigrator.js
    * line 114 -- this._dataSource = this._rdf.GetDataSource("rdf:local-store");
/mail/test/mozmill/migration-to-rdf-ui-2/test-migrate-to-rdf-ui-2.js
    * line 35 -- let datasource = rdf.GetDataSource("rdf:local-store");
/mailnews/base/prefs/content/AccountManager.js
    * line 1468 -- this._rdfDataSource = this._rdf.GetDataSource("rdf:local-store");
}}

Comment 2

[Roberto Agostino Vitillo (:rvitillo)]

The patch for Bug 559505 causes some functionality to break in Thunderbird: see https://bugzilla.mozilla.org/show_bug.cgi?id=559505#c77

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to Roberto Agostino Vitillo (:rvitillo) from comment #2)
> The patch for Bug 559505 causes some functionality to break in Thunderbird:
> see https://bugzilla.mozilla.org/show_bug.cgi?id=559505#c77

(In reply to Thomas D. from bug 559505 comment #77)
> > - In 08-21, several things are broken at once: the thread summary no longer
> > works, tabs are not saved, the state of the folder pane (unified vs. all vs.
> > ...) is not saved either. Do you have any hints as to what bugs could be
> > causing this?

Yes, that breakage is very likely (due to) this bug.
(I'm not sure whether/how_much bug 1056948 will help by itself.)

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1056649_tb_localstore_rdf.patch

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8481961 [details] [diff] [review]
bug1056649_tb_localstore_rdf.patch

Richard, maybe you'd like to take this review? ;)

Comment 8

[Richard Marti (:Paenglab)]

Comment on attachment 8481961 [details] [diff] [review]
bug1056649_tb_localstore_rdf.patch

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

Looks good to me with my limited knowledge. My mailMigrator.js patch in dupe bug looked the same except your patch is more elegant :).

r+ with the comments addressed.

::: mail/base/modules/mailMigrator.js
@@ +132,5 @@
>          // We want to remove the throbber from the menubar on Linux and
>          // Windows, and from the mail-toolbar on OSX.
> +        let mailBarId = (Services.appinfo.OS == "Darwin") ?
> +          "mail-bar3" : "mail-toolbar-menubar2";
> +        let cs = xulStore.getValue(MESSENGER_DOCURL, mailBarId, "currentset");

Here and in UI version 3 you are using cs.

@@ +166,5 @@
>        }
>  
>        // In UI version 4, we add the chat button to the mail toolbar.
>        if (currentUIVersion < 4) {
> +        let currentset = xulStore.getValue(MESSENGER_DOCURL, "mail-bar3", "currentset");

And here (and also in UI version 5) currentset. Wouldn't it be more consistent to use always the same variable name to make it easier for future changes?

@@ +224,5 @@
>      } catch(e) {
>        Cu.reportError("Migrating from UI version " + currentUIVersion + " to "
>                       + UI_VERSION + " failed. Error message was: " + e + " -- "
>                       + "Will reattempt on next start.");
> +    } 

Nit: trailing white space.

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 8481961 [details] [diff] [review]
bug1056649_tb_localstore_rdf.patch

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

AccountManager.js already has a (new) patch in bug 1058873: maybe leave it out in the current bug.

Comment 10

[Mike Conley (:mconley) (:⚙️) (PTO Aug 24-28)]

Comment on attachment 8481961 [details] [diff] [review]
bug1056649_tb_localstore_rdf.patch

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

Woo! Glad to get rid of localstore.rdf. :) Thanks Magnus!

::: mail/base/modules/mailMigrator.js
@@ +166,5 @@
>        }
>  
>        // In UI version 4, we add the chat button to the mail toolbar.
>        if (currentUIVersion < 4) {
> +        let currentset = xulStore.getValue(MESSENGER_DOCURL, "mail-bar3", "currentset");

I agree - consistency would be better.

::: mail/test/mozmill/migration-to-rdf-ui-2/test-migrate-to-rdf-ui-2.js
@@ +40,5 @@
>  
>  /**
>   * Test that the "width" property of the folderPaneBox resource was persisted.
>   * We do this simply be checking that the width of the folderPaneBox matches
> + * the width defined in localstore.rdf (->XULStore.json) (which, in this case, is 500px).

I think we should make this comment more clear about what ->XULStore.json means. I think just a quick remark that localstore.rdf has since converted to using XULStore.json was made possible by bug 559505 is probably sufficient.

Comment 11

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/8d1c850ff12b -> FIXED

Comment 12

[Magnus Melin [:mkmelin]]

Attachment: bug1056649_rdf_followup.patch

Will push this logic error that the tests caught

Comment 13

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/cfb70d10971f