Closed Bug 1254287 Opened 4 years ago Closed 3 years ago

Remove the Engineering Mode API

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox49 fixed)

RESOLVED FIXED
Tracking Status
firefox49 --- fixed

People

(Reporter: fabrice, Assigned: gsvelto)

References

Details

Attachments

(1 file, 2 obsolete files)

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
No longer blocks: 1252143
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?
Flags: needinfo?(fabrice)
Flags: needinfo?(ehung)
It's fine to remove it; I believe no one uses it.
Flags: needinfo?(ehung)
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
This patch removes the EngineeringMode API and changes the affected tests and components to reflect the removal.
Flags: needinfo?(fabrice)
Attachment #8740095 - Flags: review?(ehung)
Hi Gabriele,
Sorry for my late reply, but I'm afraid that you attached a wrong patch?
Attachment #8740095 - Flags: review?(ehung)
Yes, I attached the wrong one, sorry. Here's the correct patch.
Attachment #8740095 - Attachment is obsolete: true
Attachment #8743164 - Flags: review?(ehung)
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!
Attachment #8743164 - Flags: review?(ehung) → review+
Summary: Remove the dependency on apps status in the EngineeringMode api code → Remove the Engineering Mode 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.
Attachment #8743164 - Attachment is obsolete: true
Attachment #8745313 - Flags: review+
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 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.
Attachment #8745313 - Flags: review+ → review?(jonas)
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)
Attachment #8745313 - Flags: review?(jonas) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d30f9cddfa7c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.