Cleanup and organize lazy module/service getters

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Dolske, Assigned: Dolske)

Tracking

Trunk
Firefox 51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
The top of browser.js and nsBrowserGlue.js is kind of a disorganized mess of of various lazy getters. I'd like to clean this up by organizing them a bit, and especially shorten the verbose definition for each lazy module getter.
(Assignee)

Comment 1

2 years ago
Created attachment 8777494 [details]
Bug 1291833 - Cleanup and organize lazy module/service getters.

Review commit: https://reviewboard.mozilla.org/r/69024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69024/
Attachment #8777494 - Flags: review?(MattN+bmo)
(Assignee)

Comment 2

2 years ago
In case it's not obvious from the diffs, this should be a basic mechanical change. The only exception (unless I fucked something up) is changing the Cu.import of RecentWindow.jsm to a lazy getter. I don't see anything being evaluated upon import, so I'm not sure why that wasn't just made lazy in the first place.
Comment on attachment 8777494 [details]
Bug 1291833 - Cleanup and organize lazy module/service getters.

https://reviewboard.mozilla.org/r/69024/#review66622
Attachment #8777494 - Flags: review?(MattN+bmo) → review+

Comment 5

2 years ago
Pushed by jdolske@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89c4afd55323
Cleanup and organize lazy module/service getters. r=MattN
Backed out in https://hg.mozilla.org/integration/autoland/rev/679159ab1be9, "TypeError: gWebRTCUI is undefined" in browser_devices_get_user_media.js, https://treeherder.mozilla.org/logviewer.html#?job_id=1521552&repo=autoland
OS X only, but debug as well as opt, so at least it isn't something from universal build smooshing.
(In reply to Phil Ringnalda (:philor) from comment #6)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/679159ab1be9,
> "TypeError: gWebRTCUI is undefined" in browser_devices_get_user_media.js,
> https://treeherder.mozilla.org/logviewer.html#?job_id=1521552&repo=autoland
I think that's happening because it no longer passes the "webrtcUI" symbol in

XPCOMUtils.defineLazyModuleGetter(this, "gWebRTCUI",
  "resource:///modules/webrtcUI.jsm", "webrtcUI");

vs.

[
  ...
  ["gWebRTCUI", "resource:///modules/webrtcUI.jsm", "webrtcUI"],
].forEach(([name, resource]) => XPCOMUtils.defineLazyModuleGetter(this, name, resource));

I'd think either
.forEach(([name, resource, symbol]) => XPCOMUtils.defineLazyModuleGetter(this, name, resource, symbol));
or keeping it separate would fix it.
(Assignee)

Comment 10

2 years ago
(In reply to Ian Moody [:Kwan] from comment #8)

> I think that's happening because it no longer passes the "webrtcUI" symbol in

Yeah, I missed that the defineLazyModuleGetter() call for this had the extra argument to rename the imported symbol.

As it turns out, "gWebRTCUI" is only used in a handful of places, so I'm just going to change these places to instead use "webrtcUI" symbol that's naturally exported from the module. Florian added this back in bug 1037415, and he OK'd the change in IRC.


(In reply to Phil Ringnalda (:philor) from comment #7)
> OS X only, but debug as well as opt, so at least it isn't something from
> universal build smooshing.

In further clownshoes, I did a try push of a slightly earlier version of this patch (https://treeherder.mozilla.org/#/jobs?repo=try&revision=806781c10b05), but infra only managed to build it on Linux (pretty sure that wasn't my problem, though?). I'd also pushed the final version to try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=33b13705df44), but forgot to, y'know, enable tests.
(Assignee)

Comment 11

2 years ago
And now MozReview is broken again, fantastic. abort: reviewboard error: One or more fields had errors (HTTP 400, API Error 105); identifier: Parent review request is submitted or discarded
(Assignee)

Updated

2 years ago
Attachment #8777494 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Created attachment 8779020 [details] [diff] [review]
Patch part 1 - fix webrtc symbols
Attachment #8779020 - Flags: review?(MattN+bmo)
(Assignee)

Comment 13

2 years ago
Created attachment 8779022 [details] [diff] [review]
Patch part 2 - cleanup getters

(This is the same as the previously-reviewed patch, just removing the unused arg that got into the condensed table of module getters.)
Attachment #8779022 - Flags: review+
Attachment #8779020 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 14

2 years ago
(In reply to Justin Dolske [:Dolske] from comment #9)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1b5f439ad55

Still no test results on OS X, but I tested locally on OS X and the test that failed before is now passing. So, relanding.
(Assignee)

Comment 15

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/b18acdf2c363a0df9150ce3d708fcb697709103e
Bug 1291833 - remove little-used gWebRTCUI symbol in favor of natural module export. r=MattN

https://hg.mozilla.org/integration/fx-team/rev/6290767b211dc902629c99ac08be894b1c63d765
Bug 1291833 - Cleanup and organize lazy module/service getters. r=mattn
Comment hidden (obsolete)

Comment 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b18acdf2c363
https://hg.mozilla.org/mozilla-central/rev/6290767b211d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.