344059 - Don't write search prefs. on shutdown unless something has changed

Status: RESOLVED FIXED

(Firefox :: Search, defect)

Description

[Adam Guthrie]

As of now, we always write the search engine metadata using mozStroage on shutdown. We should only do this if this metadata changed.

See bug 341850 comment 9.

Comment 1

[Adam Guthrie]

Attachment: patch

Comment 2

[Régis Caspar]

(from patch)
    if (this._needToSetOrderPrefs) {
      engineMetadataService.setAttr(engines[i], "order", i+1);
      dump("into saveSortedengineList\n");
      var engines = this._getSortedEngines(false);
      // here ----->
      for (var i = 0; i < engines.length; ++i)
        engineMetadataService.setAttr(engines[i], "order", i+1);
      // -----------
    }

If we expand engineMetadataService.setAttr(engines[i], "order", i+1) we get something like :
for ....
  begin transaction
  query
  end transaction

and that's the problem in bug 341850 (i.e. use of transactions in a bad way). 

Comment 3

[:Gavin Sharp [email: [e-mail]]]

This is meant to be applied in addition to the patch in that bug - it's just moving the current code.

Comment 4

[Régis Caspar]

(In reply to comment #3)
> This is meant to be applied in addition to the patch in that bug - it's just
> moving the current code.
OH ok :) So mv 'comment #2' /dev/null

Comment 5

[:Gavin Sharp [email: [e-mail]]]

This patch needs to be updated now that bug 341850 landed.

Comment 6

[Adam Guthrie]

Attachment: patch v2

Comment 7

[Adam Guthrie]

Attachment: patch v3

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 230769 [details] [diff] [review]
patch v3

>Index: browser/components/search/nsSearchService.js

>+    // We only need to write the prefs. if something has changed.
>+    if (this._needToSetOrderPrefs) {

Just return early if (!_needToSetOrderPrefs), and save the indentation.

Also, I think you should set _needToSetOrderPrefs in an else block after http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/search/nsSearchService.js&rev=1.63#2329 .

Comment 9

[Adam Guthrie]

Attachment: patch v4

Comment 10

[Adam Guthrie]

Attachment: patch v4 (for real)

OK. Think I got it this time...

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 230791 [details] [diff] [review]
patch v4 (for real)

Set _needToSetOrderPrefs to true inside the if() block at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/search/nsSearchService.js&rev=1.64&mark=2220#2220
and r=me. Might also want to add something like "this needs to happen anytime _sortedEngines is modified after initial startup" to the comment near _needToSetOrderPrefs's declaration.

Comment 12

[Adam Guthrie]

Attachment: patch v4 (for checkin)

Comment 13

[Adam Guthrie]

Attachment: patch v4 (really for checkin)

Comment 14

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/nsSearchService.js 	1.65

Comment 15

[Adam Guthrie]

Comment on attachment 230810 [details] [diff] [review]
patch v4 (really for checkin)

This patch has been on trunk since Thursday with no known regressions. It makes writing the order of search engines on shutdown only take place if the order has actually changed.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

mozilla/browser/components/search/nsSearchService.js 	1.1.2.54