1446585 - Remove support for resource entries in bootstrapped chrome.manifest files

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Kris Maglione [:kmag]]

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.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

Review commit: https://reviewboard.mozilla.org/r/228606/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228606/

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8959756 [details]
Bug 1446585: Remove support for resource entries in bootstrapped chrome.manifest files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228606/diff/1-2/

Comment 3

[Andrew Swan [:aswan]]

Adding Matt and Kate for the system add-on changes.  Kate for activity stream of course and Matt for formautofill and onboarding.

Comment 4

[Andrew Swan [:aswan]]

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("[e-mail]");
>  
>      a2.uninstall();
> -    Assert.equal(gCacheFlushCount, 2);
> +    Assert.equal(gCacheFlushCount, 1);

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

Comment 5

[Andrew Swan [:aswan]]

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 6

[Kris Maglione [:kmag]]

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 7

[Kris Maglione [:kmag]]

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 8

[Andrew Swan [:aswan]]

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 9

[Matthew N. [:MattN]]

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).

Comment 10

[Kate Hudson :k88hudson]

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

Comment 11

[Andrew Swan [:aswan]]

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

Comment 12

[Kris Maglione [:kmag]]

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

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5d6ff810b526a595ebd4e60a6e616afc336ecdd
Bug 1446585: Follow-up: Fix Windows file locking xpcshell bustage. r=bustage

Comment 14

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/6f9a0b898e93
https://hg.mozilla.org/mozilla-central/rev/d5d6ff810b52

Comment 15

[[github robot]]

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

Comment 16

[CosminB]

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!