1254287 - Remove the Engineering Mode API

Status: RESOLVED FIXED

Description

[[:fabrice] Fabrice Desré]

Keeping the permission is ok, but everything that is gated by AvailableIn=CertifiedApps needs to be chrome only instead.

http://mxr.mozilla.org/mozilla-central/source/dom/webidl/EngineeringMode.webidl#7

Comment 1

[Gabriele Svelto [:gsvelto]]

I was considering removing the engineering mode API altogether. I don't think we'll ever use it anymore and it's a pretty trivial task to remove it (a few line changes in gaia and basically two patches to revert in gecko). This could be done directly in mozilla-central. Thoughts about this? Do we still need it for something else?

Comment 2

[Evelyn Hung]

It's fine to remove it; I believe no one uses it.

Comment 3

[Gabriele Svelto [:gsvelto]]

Created attachment 8740095 [details] [diff] [review]
[PATCH] Remove the EngineeringMode API

This patch removes the EngineeringMode API and changes the affected tests and components to reflect the removal.

Comment 4

[Evelyn Hung]

Hi Gabriele,
Sorry for my late reply, but I'm afraid that you attached a wrong patch?

Comment 5

[Gabriele Svelto [:gsvelto]]

Created attachment 8743164 [details] [diff] [review]
[PATCH] Remove the EngineeringMode API

Yes, I attached the wrong one, sorry. Here's the correct patch.

Comment 6

[Evelyn Hung]

Comment on attachment 8743164 [details] [diff] [review]
[PATCH] Remove the EngineeringMode API

Review of attachment 8743164 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me except the devtool part. We hope WebIDE still work for TV 2.6 releases which is using old permission model. Therefore, it's better to keep this part of implementation until Gecko 52 (at least). Thanks!

Comment 7

[Gabriele Svelto [:gsvelto]]

Created attachment 8745313 [details] [diff] [review]
[PATCH v2] Remove the EngineeringMode API

Thanks for the review Evelyn and sorry for the late reply, I've been sick for a while... This is an updated patch with the devtools changed amended. I've only removed the engineering-mode permission test and replaced it with one for another forbidden but still supported permission (embed-apps) so that it keeps working and testing that particular bit. I'm waiting for a try run to finish and then I'll land this. Carrying over the r+ flag.

Comment 8

[Gabriele Svelto [:gsvelto]]

Green try run, ready to land: https://treeherder.mozilla.org/#/jobs?repo=try&revision=23c83543a18a7700733351864184b56ae93717f0

Comment 9

[Gabriele Svelto [:gsvelto]]

Actually I realised I need this to be reviewed by a DOM peer too before being able to land since I'm getting rid of an webidl file.

Comment 10

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8745313 [details] [diff] [review]
[PATCH v2] Remove the EngineeringMode API

The contents of the patch were already reviewed by Evelyn but I need a DOM peer review for the webidl changes. This only removes dom/webidl/EngineeringMode.webidl and doesn't alter other DOM interfaces.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8745313 [details] [diff] [review]
[PATCH v2] Remove the EngineeringMode API

Review of attachment 8745313 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! (Though as always, sad to see old B2G stuff go)

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d30f9cddfa7c

Comment 13

[Gabriele Svelto [:gsvelto]]

(In reply to Jonas Sicking (:sicking) from comment #11)
> Thanks! (Though as always, sad to see old B2G stuff go)

Thanks for the quick review; we're getting rid of things we're sure we won't need in the hope we can maintain some of the more useful stuff in working order for B2G OS.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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