Remove the Engineering Mode API

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fabrice, Assigned: gsvelto)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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
Blocks: 1261022
No longer blocks: 1252143
(Assignee)

Comment 1

2 years ago
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)

Comment 2

2 years ago
It's fine to remove it; I believe no one uses it.
Flags: needinfo?(ehung)
(Assignee)

Updated

2 years ago
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
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.
Flags: needinfo?(fabrice)
Attachment #8740095 - Flags: review?(ehung)

Comment 4

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

Updated

2 years ago
Attachment #8740095 - Flags: review?(ehung)
(Assignee)

Comment 5

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

Yes, I attached the wrong one, sorry. Here's the correct patch.
Attachment #8740095 - Attachment is obsolete: true
Attachment #8743164 - Flags: review?(ehung)

Comment 6

2 years ago
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
(Assignee)

Comment 7

2 years ago
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.
Attachment #8743164 - Attachment is obsolete: true
Attachment #8745313 - Flags: review+
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Comment 10

2 years ago
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+
(Assignee)

Comment 13

2 years ago
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d30f9cddfa7c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.