Closed
Bug 1405670
Opened 7 years ago
Closed 7 years ago
Stop importing old search settings from search-metadata.json
Categories
(Firefox :: Search, defect, P1)
Firefox
Search
Tracking
()
RESOLVED
FIXED
Firefox 59
People
(Reporter: florian, Assigned: florian)
References
Details
(Whiteboard: [fxsearch])
Attachments
(6 files)
11.58 KB,
patch
|
adw
:
review+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
4.16 KB,
patch
|
adw
:
review+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
5.36 KB,
patch
|
adw
:
review+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
12.09 KB,
patch
|
adw
:
review+
|
Details | Diff | Splinter Review |
28.69 KB,
patch
|
adw
:
review+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
34.39 KB,
patch
|
adw
:
review+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
search-metadata.json files were created by Firefox up to 44. Bug 1203167 changed the storage format for 45. We are still attempting to import search settings from this old file, which we could stop doing.
2 ESR cycles (45 and 52) happened since that. And the few users upgrading from a 44 or older to a current release will be forced to go through 48 anyway (https://chuttenblog.wordpress.com/2017/02/14/the-most-satisfying-graph/).
Updated•7 years ago
|
status-firefox57:
--- → wontfix
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
This could be folded into part 1, but part 1 was already big enough.
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
This breaks 25 xpcshell tests of the search service; these tests rely on dropping a test engine in the profile folder before initializing the search service.
Comment 6•7 years ago
|
||
> This breaks 25 xpcshell tests of the search service; these tests rely on dropping a test engine in the profile folder before initializing the search service.
What is the plan for that?
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #6)
> > This breaks 25 xpcshell tests of the search service; these tests rely on dropping a test engine in the profile folder before initializing the search service.
>
> What is the plan for that?
Assuming the other patches I attached here are wanted, I'll need to fixup the tests. Likely use addEngineWithDetails instead.
Assignee | ||
Comment 8•7 years ago
|
||
This fixes the otherwise still relevant tests that were relying on:
- us importing automatically engines from .xml files dropped in the searchplugins/ folder of the profile.
- directory service constants I'm removing.
Assignee | ||
Comment 9•7 years ago
|
||
These tests are obsolete. I also removed lots of attempts to remove the cache file at the very beginning or very end of an xpcshell test. This was just doing a lot of pointless main thread I/O while running the tests. Each xpcshell test starts with a new process and profile folder anyway.
Updated•7 years ago
|
Attachment #8928529 -
Flags: review?(adw)
Updated•7 years ago
|
Attachment #8928530 -
Flags: review?(adw)
Updated•7 years ago
|
Attachment #8928531 -
Flags: review?(adw)
Updated•7 years ago
|
Attachment #8928532 -
Flags: review?(adw)
Updated•7 years ago
|
Attachment #8929068 -
Flags: review?(adw)
Updated•7 years ago
|
Attachment #8929069 -
Flags: review?(adw)
Assignee | ||
Comment 10•7 years ago
|
||
Comment 11•7 years ago
|
||
This looks straightforward to me -- shutting off avenues for installing/using engines that we don't want to be available anymore -- but I'm not super familiar with this code. Most of this is code removal, which makes sense, and I don't see any problems in any of the other changes, not even any nits.
Updated•7 years ago
|
Attachment #8928529 -
Flags: review?(adw) → review+
Updated•7 years ago
|
Attachment #8928530 -
Flags: review?(adw) → review+
Updated•7 years ago
|
Attachment #8928531 -
Flags: review?(adw) → review+
Updated•7 years ago
|
Attachment #8928532 -
Flags: review?(adw) → review+
Updated•7 years ago
|
Attachment #8929068 -
Flags: review?(adw) → review+
Updated•7 years ago
|
Attachment #8929069 -
Flags: review?(adw) → review+
Comment 12•7 years ago
|
||
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74f28655378e
stop importing old search plugins from <profile>/searchplugins/*.xml when the cache file is missing, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3beeb64fb0da
stop saving the last modification date of directories we loaded engines from, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e05f84b0effd
stop importing old search metadata from search-metadata.json, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/297b54c04fb1
remove support for NS_APP_SEARCH_DIR_LIST and NS_APP_SEARCH_DIR from the directory service, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/12152163c058
fix search service tests relying on dropping .xml files in the profile folder, r=adw.
https://hg.mozilla.org/integration/mozilla-inbound/rev/160a2d1e4540
remove tests for search-metadata.json migration, r=adw.
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/74f28655378e
https://hg.mozilla.org/mozilla-central/rev/3beeb64fb0da
https://hg.mozilla.org/mozilla-central/rev/e05f84b0effd
https://hg.mozilla.org/mozilla-central/rev/297b54c04fb1
https://hg.mozilla.org/mozilla-central/rev/12152163c058
https://hg.mozilla.org/mozilla-central/rev/160a2d1e4540
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 14•7 years ago
|
||
Comment on attachment 8928529 [details] [diff] [review]
part 1 - stop importing old search plugins from <profile>/searchplugins/*.xml
Approval Request Comment
[Feature/Bug causing the regression]: not a regression, this is long overdue follow-up cleanup after the changes from bug 1203167. But this is more relevant now that we dropped legacy add-ons in 57, as we want to avoid carrying forward customizations that were done in the pre-WebExtension world.
[User impact if declined]: Easier to get search plugins injected against the user's will by dropping files in the user profile.
[Is this code covered by automated tests?]: Yes. But it's mostly a code removal, so also removing tests.
[Has the fix been verified in Nightly?]: It has baked on Nightly for a couple days without us hearing complaints, but I don't think QA verified.
[Needs manual test from QE? If yes, steps to reproduce]: currently being discussed with QA directly
[List of other uplifts needed for the feature/fix]: most other patches in this bug. I would like to let "part 4" ride the trains without uplifting it though, to avoid needlessly breaking eg. Thunderbird.
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: it's a code removal.
[String changes made/needed]: none.
Attachment #8928529 -
Flags: approval-mozilla-beta?
Comment 15•7 years ago
|
||
Comment on attachment 8928529 [details] [diff] [review]
part 1 - stop importing old search plugins from <profile>/searchplugins/*.xml
Let's take that.
Sheriff, please don't land part 4.
Attachment #8928529 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8928530 -
Flags: approval-mozilla-release+
Attachment #8928530 -
Flags: approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8928531 -
Flags: approval-mozilla-release+
Attachment #8928531 -
Flags: approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8929068 -
Flags: approval-mozilla-release+
Attachment #8929068 -
Flags: approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8929069 -
Flags: approval-mozilla-release+
Attachment #8929069 -
Flags: approval-mozilla-beta+
Updated•7 years ago
|
Attachment #8928529 -
Flags: approval-mozilla-release+
Updated•7 years ago
|
tracking-firefox57:
--- → +
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: [fxsearch]
Comment 16•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0abc56d5a1b1
https://hg.mozilla.org/releases/mozilla-beta/rev/fe0befa1aa9d
https://hg.mozilla.org/releases/mozilla-beta/rev/6ad58e06da8c
https://hg.mozilla.org/releases/mozilla-beta/rev/e4307e3d7f1f
https://hg.mozilla.org/releases/mozilla-beta/rev/1922cbee6cf4
https://hg.mozilla.org/releases/mozilla-beta/rev/827c49cd8d40
Comment 17•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/17e3d0eb9cdf
https://hg.mozilla.org/releases/mozilla-release/rev/1f25e81939ea
https://hg.mozilla.org/releases/mozilla-release/rev/037f3ba3a7b6
https://hg.mozilla.org/releases/mozilla-release/rev/12f0d3bf63bd
https://hg.mozilla.org/releases/mozilla-release/rev/61902e3ade32
https://hg.mozilla.org/releases/mozilla-release/rev/7ea28f7a42fe
Comment 18•7 years ago
|
||
Basic QA was indirectly covered on bug 1419941 for the reset search testing.
Marking this issue as a qe- after consulting with :florian about what extra QA should be done for this issue in particular.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•