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

RESOLVED FIXED in Firefox 2 beta2

Status

()

Firefox
Search
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Adam Guthrie, Assigned: Adam Guthrie)

Tracking

({fixed1.8.1, perf})

2.0 Branch
Firefox 2 beta2
fixed1.8.1, perf
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

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

Comment 1

12 years ago
Created attachment 228633 [details] [diff] [review]
patch
Assignee: nobody → ispiked
Status: NEW → ASSIGNED
Attachment #228633 - Flags: review?(gavin.sharp)

Comment 2

12 years ago
(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). 
This is meant to be applied in addition to the patch in that bug - it's just moving the current code.

Comment 4

12 years ago
(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

Updated

12 years ago
Flags: blocking-firefox2+
This patch needs to be updated now that bug 341850 landed.
(Assignee)

Comment 6

12 years ago
Created attachment 229337 [details] [diff] [review]
patch v2
Attachment #228633 - Attachment is obsolete: true
Attachment #229337 - Flags: review?(gavin.sharp)
Attachment #228633 - Flags: review?(gavin.sharp)

Updated

12 years ago
Whiteboard: [has patch][needs review gavin]
(Assignee)

Comment 7

12 years ago
Created attachment 230769 [details] [diff] [review]
patch v3
Attachment #229337 - Attachment is obsolete: true
Attachment #230769 - Flags: review?(gavin.sharp)
Attachment #229337 - Flags: review?(gavin.sharp)
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 .
(Assignee)

Comment 9

12 years ago
Created attachment 230790 [details] [diff] [review]
patch v4
Attachment #230769 - Attachment is obsolete: true
Attachment #230790 - Flags: review?(gavin.sharp)
Attachment #230769 - Flags: review?(gavin.sharp)
(Assignee)

Comment 10

12 years ago
Created attachment 230791 [details] [diff] [review]
patch v4 (for real)

OK. Think I got it this time...
Attachment #230790 - Attachment is obsolete: true
Attachment #230791 - Flags: review?(gavin.sharp)
Attachment #230790 - Flags: review?(gavin.sharp)
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.
Attachment #230791 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 12

12 years ago
Created attachment 230805 [details] [diff] [review]
patch v4 (for checkin)
Attachment #230791 - Attachment is obsolete: true
Attachment #230805 - Flags: review+
(Assignee)

Updated

12 years ago
Whiteboard: [has patch][needs review gavin] → [checkin needed]
(Assignee)

Comment 13

12 years ago
Created attachment 230810 [details] [diff] [review]
patch v4 (really for checkin)
Attachment #230805 - Attachment is obsolete: true
Attachment #230810 - Flags: review+
mozilla/browser/components/search/nsSearchService.js 	1.65
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → Firefox 2 beta2

Updated

12 years ago
Whiteboard: [has patch][baking]
(Assignee)

Comment 15

12 years ago
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.
Attachment #230810 - Flags: approval1.8.1?

Updated

12 years ago
Attachment #230810 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Updated

12 years ago
Whiteboard: [has patch][baking] → [checkin needed (1.8 branch)]
mozilla/browser/components/search/nsSearchService.js 	1.1.2.54
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
You need to log in before you can comment on or make changes to this bug.