Closed Bug 1512436 Opened 6 years ago Closed 6 years ago

Add new install location for built in search web extensions

Categories

(WebExtensions :: General, enhancement, P1)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox67 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: daleharvey, Assigned: aswan)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1512436 Add new built-in addon location r=kmag
(deleted), text/x-phabricator-request
Details
0001-Testing.patch
(deleted), patch
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → aswan
Blocks: 1486820
Are we sure that this won't add any startup I/O before first paint?
We talked about performance worries. Andrew said the process shouldnt be expensive. We are going to be reading a bunch of json files from the omni jar instead of those xml files directly from disk afaik so it could be slower or faster, but will measure and if it causes any slowdown there are things we can do to make sure it is fast (like build the nssearchservice cache at build time for instance) Any pointers to tests I could use to verify / measure these issues would be very useful, thanks
(In reply to Dale Harvey (:daleharvey) from comment #2) > We talked about performance worries. Andrew said the process shouldnt be > expensive. > > We are going to be reading a bunch of json files from the omni jar instead > of those xml files directly from disk The xml files are in omni.ja too. My main question is if we are going to read more from the omni.ja before first paint of the browser UI. Currently the search service starts asynchronously after first paint, and we have a test ensuring that. Most of the add-on manager initialization happens before first paint. > Any pointers to tests I could use to verify / measure these issues would be > very useful, thanks The impact of I/O is hard to measure unfortunately, as it's not deterministic. Typically this would be more visible on cold startup on machines with slow rotating hard drives.
Priority: -- → P1
Blocks: 1513316
Hey Andrew whats your thoughts on this, especially with the plan of installing 'all' engines?
Flags: needinfo?(aswan)
Ping Andrew Hey so we are going to be determining the status of this in terms of 66 vs 67 today and it looks like we would need to be landed by 10th. Is this something you can pick up shortly or is it something I can take with some guidance? Cheers Dale
Flags: needinfo?(aswan)
Flags: needinfo?(aswan)
I started on this before the holidays and ran into a small snag. Its at the top of my list though, I expect to finish it in the next day or two. I'll have an update for you either way at the end of today (US west coast time)
Flags: needinfo?(aswan)
I've attached my work-in-progress. The new test and existing xpcshell tests are passing but it needs some polish as well as more thorough testing. I hope to have it ready for review tomorrow.
Attachment #9034111 - Attachment description: Bug 1512436 Add new built-in addon location → Bug 1512436 Add new built-in addon location r=kmag
Attached patch 0001-Testing.patch (deleted) — — Splinter Review

Hey Andrew

As per IRC chat, I am testing this with unzipped web extensions in the omni jar and finding that the engines seem to install, but the promise returned never seems to resolve for me

Cheers
Dale

This is possibly my problem, I wanted to test this on a fresh patch but I might need some fixes from my patch to avoid a race here, will report back tonight

Just to confirm, it was a problem on my end, basing my work on top of this patch and it looks to be working well at the moment

Pushed by aswan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8952c559acfb Add new built-in addon location r=kmag

I'll fix that test and re-land this after the soft freeze is over.

Flags: needinfo?(aswan)

Andrew asked me to leave a needinfo with some follow ups that would be very useful

  1. Add support for hidden: true so we can avoid showing these in about:addons
  2. Expose an isSystem or similiar property so we can distinguish built in addons from user installed ones

Many thanks

Flags: needinfo?(aswan)

https://hg.mozilla.org/mozilla-central/rev/e3e75697e98c

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

The patch above should cover the requests from comment 17, if anything got overlooked, lets open a new bug.

Flags: needinfo?(aswan)
Depends on: 1523535
Blocks: rm-lwthemes
No longer blocks: 1513316
Blocks: 1525762

Can you please add some STRs to this issue or mark it as "qe-verify- " if no manual testing is needed ?

Flags: needinfo?(aswan)

no manual testing needed, thanks.

Flags: needinfo?(aswan) → qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: