Remove dom/settings and related code

RESOLVED FIXED in Firefox 53

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: aryx, Assigned: mds)

Tracking

(Blocks 1 bug, {dev-doc-complete})

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment)

B2G-related code gets removed from mozilla-central, see bug 1306391.
Component: General → DOM
Priority: -- → P3
Assignee: nobody → mdesimone
Duplicate of this bug: 1319861
Comment hidden (mozreview-request)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8823917 [details]
Bug 1313155 - Remove dom/settings and related code.

https://reviewboard.mozilla.org/r/102384/#review103508

::: accessible/jsat/Utils.jsm
(Diff revision 1)
> -  let settings = Utils.win.navigator.mozSettings;
> -  if (!settings) {
> -    if (aOptions.callbackNow) {
> +  if (aOptions.callbackNow) {
> -      runCallback();
> +    runCallback();
> -    }
> +  }
> -    return;

You're changing the semantics of this function.  Right now we return after this block because |settings| is always null.  Now we're continuting to execute the rest of the function.

::: b2g/app/b2g.js
(Diff revision 1)
>  
> -// Enable TV Manager API
> -pref("dom.tv.enabled", true);
> -
> -// Enable Inputport Manager API
> -pref("dom.inputport.enabled", true);

Why these?

::: devtools/shared/discovery/discovery.js
(Diff revision 1)
>  
>  /**
>   * Manages the local device's name.  The name can be generated in serveral
>   * platform-specific ways (see |_generate|).  The aim is for each device on the
> - * same local network to have a unique name.  If the Settings API is available,
> + * same local network to have a unique name.
> - * the name is saved there to persist across reboots.

I don't dare review devtools code.  :-)  Can you get someone else to look into these parts?  jryans would probably be a good candidate.

::: dom/system/NetworkGeolocationProvider.js
(Diff revision 1)
>    listener: null,
>  
>    observe: function(aSubject, aTopic, aData) {
> -    if (aTopic != SETTINGS_CHANGED_TOPIC) {
> -      return;
> -    }

Looks like the whole observe method (and probably the respective addObserver call) can go away, no?

::: testing/profiles/prefs_general.js
(Diff revision 1)
>  // Enable Media Source Extensions for testing
>  user_pref("media.mediasource.mp4.enabled", true);
>  user_pref("media.mediasource.webm.enabled", true);
>  
> -// Enable mozContacts
> -user_pref("dom.mozContacts.enabled", true);

Why this?
Attachment #8823917 - Flags: review?(ehsan) → review-
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8823917 [details]
Bug 1313155 - Remove dom/settings and related code.

https://reviewboard.mozilla.org/r/102384/#review103508

> You're changing the semantics of this function.  Right now we return after this block because |settings| is always null.  Now we're continuting to execute the rest of the function.

As this function seems to have no caller I've removed it altogether. I don't think WinMouseScrollHandler is using that but I may be wrong.

> Why these?

I'm following up to your comment in https://bugzilla.mozilla.org/show_bug.cgi?id=1309030#c4 by having good hygiene (inputport, TV and contacts are not on m-c anymore).

> Why this?

Ditto.
:jryans would you please give us a feedback on this, as per comment #3?
Flags: needinfo?(jryans)

Comment 7

2 years ago
mozreview-review
Comment on attachment 8823917 [details]
Bug 1313155 - Remove dom/settings and related code.

https://reviewboard.mozilla.org/r/102384/#review104348

::: devtools/server/main.js
(Diff revision 2)
>          constructor: "ActorRegistryActor",
>          type: { global: true }
>        });
>      }
> -    if (Services.prefs.getBoolPref("dom.mozSettings.enabled")) {
> -      this.registerModule("devtools/server/actors/settings", {

Here are some additional file related to settings that should also be removed:

devtools/server/actor/settings.js
devtools/shared/specs/settings.js
devtools/shared/fronts/settings.js
devtools/client/webide/content/devicesettings.js
devtools/client/webide/content/devicesettings.xhtml

Also, these references to devicesettings in webide need updates:

http://searchfox.org/mozilla-central/search?q=devicesettings&case=true&regexp=false&path=
Attachment #8823917 - Flags: review-
Flags: needinfo?(jryans)
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8823917 [details]
Bug 1313155 - Remove dom/settings and related code.

https://reviewboard.mozilla.org/r/102384/#review104832

Thanks, looks great!
Attachment #8823917 - Flags: review?(ehsan) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 years ago
Pushed by mdesimone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5566e83980ed
Remove dom/settings and related code. r=Ehsan
I had to back this out for mass build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=68564288&repo=autoland&lineNumber=2654

Looks like at least one moz.build file still needs updated?

https://hg.mozilla.org/integration/autoland/rev/251ca9a04afc
Flags: needinfo?(mdesimone)
Comment hidden (mozreview-request)
(In reply to Wes Kocher (:KWierso) from comment #13)

> Looks like at least one moz.build file still needs updated?

Ouch, I'm so sorry for that.
I've updated it and pushed to try again.
Flags: needinfo?(mdesimone)
Comment hidden (mozreview-request)
:jryans I've been trying to pinpoint where this WebIDE failure [1] is coming from...
Do you have any feedback about that?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=304f2977a15d&selectedJob=68603303
Flags: needinfo?(jryans)
(In reply to Michelangelo De Simone [:mds] from comment #17)
> :jryans I've been trying to pinpoint where this WebIDE failure [1] is coming
> from...
> Do you have any feedback about that?

Looks like you need to additionally remove:

* Import of getSettingsFront[1]
* Code paths that reference settingsFront[2]

[1]: http://searchfox.org/mozilla-central/source/devtools/client/webide/modules/app-manager.js#20
[2]: http://searchfox.org/mozilla-central/search?q=settingsFront&case=true&path=
Flags: needinfo?(jryans)
Comment hidden (mozreview-request)

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b172f492f29f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
:Ehsan and :jryans: thank you very much for your help!
I've archived all the B2G OS API stuff here (you'll see the settings interfaces listed):

https://developer.mozilla.org/en-US/docs/Archive/B2G_OS/API/

I've also added a note to the Fx53 release notes to make it clear that the API has been removed:

https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM_HTML_DOM
No longer blocks: 1369194
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.