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

RESOLVED FIXED in Thunderbird 36.0

Status

defect
--
major
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: sgautherie, Assigned: mkmelin)

Tracking

(Blocks 1 bug)

Trunk
Thunderbird 36.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird34?, thunderbird36 fixed)

Details

Attachments

(2 attachments)

+++ 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.
Reporter

Comment 1

5 years ago
(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");
}}
Assignee

Updated

5 years ago
Depends on: 1056948
The patch for Bug 559505 causes some functionality to break in Thunderbird: see https://bugzilla.mozilla.org/show_bug.cgi?id=559505#c77
Reporter

Comment 3

5 years ago
(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.)
Assignee

Updated

5 years ago
Duplicate of this bug: 1057710
Reporter

Updated

5 years ago
Severity: normal → major
Assignee

Updated

5 years ago
Assignee: nobody → mkmelin+mozilla
Depends on: 1058873
QA Contact: mkmelin+mozilla
Assignee

Updated

5 years ago
QA Contact: mkmelin+mozilla
Assignee

Comment 5

5 years ago
Attachment #8481961 - Flags: review?(mconley)
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Duplicate of this bug: 1077959
Assignee

Comment 7

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

Richard, maybe you'd like to take this review? ;)
Attachment #8481961 - Flags: review?(richard.marti)
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.
Attachment #8481961 - Flags: review?(richard.marti) → review+
Reporter

Comment 9

5 years ago
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 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.
Attachment #8481961 - Flags: review?(mconley) → review+
Assignee

Comment 11

5 years ago
https://hg.mozilla.org/comm-central/rev/8d1c850ff12b -> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 36.0
Assignee

Comment 12

5 years ago
Will push this logic error that the tests caught
You need to log in before you can comment on or make changes to this bug.