Closed Bug 1446585 Opened 2 years ago Closed 2 years ago

Remove support for resource entries in bootstrapped chrome.manifest files

Categories

(Toolkit :: Add-ons Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(1 file)

Supporting these has a pretty big impact on startup and shutdown performance, and requires that we maintain a lot of otherwise unused code. Now that non-system legacy extensions are no longer supported, it's time for it to go away.
Blocks: 1446586
Attachment #8959756 - Flags: review?(khudson)
Attachment #8959756 - Flags: review?(MattN+bmo)
Adding Matt and Kate for the system add-on changes.  Kate for activity stream of course and Matt for formautofill and onboarding.
Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

https://reviewboard.mozilla.org/r/228606/#review234708

Sh

::: toolkit/mozapps/extensions/test/xpcshell/test_cacheflush.js:121
(Diff revision 2)
>      gExpectedFile = gProfD.clone();
>      gExpectedFile.append("extensions");
>      gExpectedFile.append("addon2@tests.mozilla.org.xpi");
>  
>      a2.uninstall();
> -    Assert.equal(gCacheFlushCount, 2);
> +    Assert.equal(gCacheFlushCount, 1);

huh?  was this a side effect of touching the resource: protocol handler?
Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

https://reviewboard.mozilla.org/r/228606/#review234708

wtf reviewboard.
Was trying to write: shouldn't the jar.mn resource entries be removed from the system add-ons you touched?
Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

https://reviewboard.mozilla.org/r/228606/#review234708

> huh?  was this a side effect of touching the resource: protocol handler?

No, it was a side-effect of removing the ChromeManifestParser call at shutdown, which did its own jar cache flush.
Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

https://reviewboard.mozilla.org/r/228606/#review234708

They should, but the all-files-referenced test unfortunately still depends on them to figure out where resource URLs from those extensions point :/ I need to update them to look up the URLs in the resource protocol handler instead, but I'm going to do it in a separate bug.
Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

https://reviewboard.mozilla.org/r/228606/#review234708

> No, it was a side-effect of removing the ChromeManifestParser call at shutdown, which did its own jar cache flush.

Ugh, thanks for making this slightly less terrible.
Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

https://reviewboard.mozilla.org/r/228606/#review234856

Thanks. LGTM for the two components (since a follow-up will be filed to remove the manifest entries).
Attachment #8959756 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

https://reviewboard.mozilla.org/r/228606/#review234858

Changes look fine (as far as I can tell), no broken links or tests when running locally for Activity Stream / Onboarding code
Attachment #8959756 - Flags: review?(khudson) → review+
Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

https://reviewboard.mozilla.org/r/228606/#review235262

sorry I thought I marked this r+ yesterday
Attachment #8959756 - Flags: review?(aswan) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f9a0b898e9367f2590685e683dbb2bcbe61ba6b
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files. r=aswan,MattN,k88hudson
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d6ff810b526a595ebd4e60a6e616afc336ecdd
Bug 1446585: Follow-up: Fix Windows file locking xpcshell bustage. r=bustage
https://hg.mozilla.org/mozilla-central/rev/6f9a0b898e93
https://hg.mozilla.org/mozilla-central/rev/d5d6ff810b52
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/cd70d329b53449d6f57ca504b0b906d121c12d05
chore(mc): Port Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files. r=aswan,MattN,k88hudson
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(kmaglione+bmo)
See Also: → 1474414
Flags: needinfo?(kmaglione+bmo) → qe-verify-
You need to log in before you can comment on or make changes to this bug.