Closed
Bug 1405670
Opened 6 years ago
Closed 6 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•6 years ago
|
status-firefox57:
--- → wontfix
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → florian
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•6 years ago
|
||
This could be folded into part 1, but part 1 was already big enough.
Assignee | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 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•6 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•6 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•6 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•6 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•6 years ago
|
Attachment #8928529 -
Flags: review?(adw)
Updated•6 years ago
|
Attachment #8928530 -
Flags: review?(adw)
Updated•6 years ago
|
Attachment #8928531 -
Flags: review?(adw)
Updated•6 years ago
|
Attachment #8928532 -
Flags: review?(adw)
Updated•6 years ago
|
Attachment #8929068 -
Flags: review?(adw)
Updated•6 years ago
|
Attachment #8929069 -
Flags: review?(adw)
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2faea31c76109e5032f0f138f461637ae1791e53
Comment 11•6 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•6 years ago
|
Attachment #8928529 -
Flags: review?(adw) → review+
Updated•6 years ago
|
Attachment #8928530 -
Flags: review?(adw) → review+
Updated•6 years ago
|
Attachment #8928531 -
Flags: review?(adw) → review+
Updated•6 years ago
|
Attachment #8928532 -
Flags: review?(adw) → review+
Updated•6 years ago
|
Attachment #8929068 -
Flags: review?(adw) → review+
Updated•6 years ago
|
Attachment #8929069 -
Flags: review?(adw) → review+
Comment 12•6 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•6 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: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 14•6 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•6 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•6 years ago
|
Attachment #8928530 -
Flags: approval-mozilla-release+
Attachment #8928530 -
Flags: approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8928531 -
Flags: approval-mozilla-release+
Attachment #8928531 -
Flags: approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8929068 -
Flags: approval-mozilla-release+
Attachment #8929068 -
Flags: approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8929069 -
Flags: approval-mozilla-release+
Attachment #8929069 -
Flags: approval-mozilla-beta+
Updated•6 years ago
|
Attachment #8928529 -
Flags: approval-mozilla-release+
Updated•6 years ago
|
tracking-firefox57:
--- → +
Updated•6 years ago
|
Priority: -- → P1
Whiteboard: [fxsearch]
Comment 16•6 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•6 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•6 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
•