Adds the boilerplate for the extension. mythmon: Check out privileged/FirefoxMonitor.jsm. The init function in that module is the entry point of the feature and has the pref code - let me know if it looks ok. Thanks!

Forgot to hg add a file. Please see the previous comment for details about the patch.

Comment on attachment 8997968 [details] [diff] [review] Patch Review of attachment 8997968 [details] [diff] [review]: ----------------------------------------------------------------- The pref handling here is almost too little to be sure. It looks like it is going in the right direction, since you're using a pref observer. I think this will be fine for Normandy compatibility.

Fixed some minor style inconsistencies.

Comment on attachment 8997972 [details] [diff] [review] Patch v1.1 Review of attachment 8997972 [details] [diff] [review]: ----------------------------------------------------------------- This looks good overall but we should make sure we're not leaking pref observers (maybe this works for some reason because it's a webextension but we should still unregister explicitly). ::: browser/extensions/fxmonitor/privileged/FirefoxMonitor.jsm @@ +6,5 @@ > + extension: null, > + init(aExtension) { > + this.extension = aExtension; > + > + Preferences.observe(this.kEnabledPref, () => { This needs a Preferences.ignore equivalent. https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/toolkit/modules/Preferences.jsm#281 Does this pass on try? :)

Try run on beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79b6ae1e2c95b5e53455edef373b07d74c44412b Try run on central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43210417658efcaded41765e5040ddbdb321f207

Comment on attachment 8998205 [details] [diff] [review] Patch v2 Review of attachment 8998205 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

Approval Request Comment [Feature/Bug causing the regression]: Not a regression. Feature is bug 1478521 [User impact if declined]: This is for a new feature, intended to be released in the September marketing season. [Is this code covered by automated tests?]: It introduces boilerplate for a new system add-on; no functionality; try push links in comment 6. [Has the fix been verified in Nightly?]: No [Needs manual test from QE? If yes, steps to reproduce]: No, but QA will be testing this thoroughly as the feature evolves. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Adds boilerplate. No functionality. Introduces a pref (off by default) behind which future functionality additions will be added. [String changes made/needed]: None.

Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ad28f117cf Implement Firefox Monitor add-on stub. r=johannh

Backed out changeset b1ad28f117cf (Bug 1480610) for ESlint failure on /browser/extensions/fxmonitor/background.js:1:1. CLO Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9dfaada5a1c278bfb1e6651b5f32e98cc7e4620c Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b1ad28f117cfb929d4a133000fd3b41b3cfc6888&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&selectedJob=192573816 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=192573816&repo=mozilla-inbound&lineNumber=306

Please note that these "SEGV on unknown address failures" are also from your push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b1ad28f117cfb929d4a133000fd3b41b3cfc6888&filter-searchStr=9ec81768c0017df379e1c297d30a961f02367fde&selectedJob=192596138 https://treeherder.mozilla.org/logviewer.html#?job_id=192596140&repo=mozilla-inbound&lineNumber=1388

Comment on attachment 8998225 [details] [diff] [review] Patch v2 for beta Clearing approval request while I figure out the issues that led to backout.

Stubs this patch even more, fixes eslint failures.

Luca, as I mentioned on IRC, adding this stub system add-on is causing an asan failure in browser_ext_addon_debugging_netmonitor.js. You're the author of that file, so I was wondering if you could help me figure out why my patch is causing an asan failure there and what we could do to fix it. Reference try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b14fbd7a62db6cf7576741e2639f65e78edc6346&selectedJob=192800506 Log of the error: https://treeherder.mozilla.org/logviewer.html#?job_id=192800506&repo=try&lineNumber=1419 Thanks!

Wennie, is this something that still need to land in 62 beta for this feature to be rolled out during 62 release?

As an effort to move this along, I've stripped the patch down even further to the bare minimum we need to uplift in order to be able to push updates to the add-on. I've pushed it to try to see if the asan failure goes away. https://treeherder.mozilla.org/#/jobs?repo=try&revision=67b4a2eb1a88f002bb954db98c3ff504738aa8e5

Johann, this strips down the patch to the bare minimum required to register the add-on. This eliminates the asan failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67b4a2eb1a88f002bb954db98c3ff504738aa8e5&selectedJob=192828813.

Comment on attachment 8998583 [details] [diff] [review] Ultra minimal patch Review of attachment 8998583 [details] [diff] [review]: ----------------------------------------------------------------- I also would have been fine with filing a bug and disabling the test for now :)

This simply rebases browser/extensions/moz.build on top of the beta head. Carrying the r+.

Pushed by aciure@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3e23fa2eb65d Add minimal stub for fxmonitor add-on into build system. r=johannh

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #15) > Wennie, is this something that still need to land in 62 beta for this > feature to be rolled out during 62 release? Liz, not sure if you clarified this with Wennie already outside of Bugzilla, but yes, this patch is the minimum that needs to be uplifted to beta in order to enable shipping Firefox Monitor during 62. Please let me know if there's any other info I can provide. I'm taking the liberty to clear Wennie's needinfo? request, please re-request if necessary.

OK, please request uplift once it lands. Thanks!

Comment on attachment 8998585 [details] [diff] [review] Ultra minimal patch for beta Approval Request Comment [Feature/Bug causing the regression]: Not a regression. Feature is bug 1478521 [User impact if declined]: This is for a new feature, intended to be released in the September marketing season. [Is this code covered by automated tests?]: It introduces boilerplate for a new system add-on; no functionality. [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No, but QA will be testing this thoroughly as the feature evolves. [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No [Why is the change risky/not risky?]: Adds boilerplate. No functionality. Just an add-on manifest and the relevant moz.build changes. [String changes made/needed]: None.

Comment on attachment 8998585 [details] [diff] [review] Ultra minimal patch for beta Let's uplift for beta 17 as it's planned for this feature in 62/63. As I understand it this will be off by default when we ship 62 but then will be rolled out after 62 release (so there is a bit of extra runway for testing).

(In reply to Nihanth Subramanya [:nhnt11] from comment #14) > Luca, as I mentioned on IRC, adding this stub system add-on is causing an > asan failure in browser_ext_addon_debugging_netmonitor.js. You're the author > of that file, so I was wondering if you could help me figure out why my > patch is causing an asan failure there and what we could do to fix it. Hi Nihanth, Sorry I forgot to reply to this needinfo, we met a similar issue in Bug 1449055 and we figured out how to prevent those asan failures, the fix landed as part of Bug 1449055 in: https://hg.mozilla.org/mozilla-central/rev/774f0b7aba00