Closed Bug 406371 Opened 13 years ago Closed 2 years ago

Browser code relies on Components.* (Cc, Ci, Cr) defined in PlacesOverlay.xul, move them to a proper shared file.

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: dietrich, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
utils.js is currently included via the places overlay. however, it also needs to be included inside components and other js contexts. also, the Cc/Ci declared in utils.js have dependent consumers in other files included in browser.js, which is a fragile.
Attachment #291023 - Flags: review?(mano)
Comment on attachment 291023 [details] [diff] [review]
v1

ugh, whatever, r=mano.
Attachment #291023 - Flags: review?(mano) → review+
Sorry for not removing the review request here. Over IRC we talked about it a while back, and you suggested that I shouldn't be including utils.js in components to begin with, so that's the approach I took.

I do think that we should have some centrally located place for these oft-used constants, as the current situation kinda sucks.
No longer blocks: 384370
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Depends on: 556739
Summary: Components.* vars in places' utils.js conflict with pre-existing consts when the file is included → Browser code relies on Components.* (Cc, Ci, Cr) defined in PlacesOverlay.xul. Hunt the lazy devs!
Sounds like the idea to fix this, is just to move them to a most proper position, handling all code to remove their use would be impossible.
Summary: Browser code relies on Components.* (Cc, Ci, Cr) defined in PlacesOverlay.xul. Hunt the lazy devs! → Browser code relies on Components.* (Cc, Ci, Cr) defined in PlacesOverlay.xul, move them to a proper shared file.
Whiteboard: [places-next-wanted]
Hmm, I wondering how we should fix this.

If we remove Cc/Ci/Cr from placesOverlay.xul, we'll need to define them in a few new scopes (I count ~8 places loading this overlay). I'm not sure if all of those spots actually reply on the implicit creation of these shortcut vars, but it's probably safer to just assume so to avoid addon compat issues.

There's also the slight oddity that since code pulled in by placesOverlay.xul (eg treeView.js) makes use of Cc/Ci/Cr, there would not be an implicit requirement that anything wanting to use placesOverlay.xul must also provide these variables. Are we ok with that? It's not crazy, but it a weird slightly-hidden dependency.

I wonder if instead of trying to figure out where to define these, we should instead just hack xpconnect to automatically prove these shortcuts everywhere that Components exists. (I suppose that might be a little tricky given it's still sometimes exposed to web content, but maybe that's easy to workaround?).

If we did that, we could simply _remove_ all the "var Cc = ..." cruft scattered around in our code.
(In reply to Justin Dolske [:Dolske] from comment #5)
> I wonder if instead of trying to figure out where to define these, we should
> instead just hack xpconnect to automatically prove these shortcuts
> everywhere that Components exists. (I suppose that might be a little tricky
> given it's still sometimes exposed to web content, but maybe that's easy to
> workaround?).
> 
> If we did that, we could simply _remove_ all the "var Cc = ..." cruft
> scattered around in our code.

+1
(In reply to Justin Dolske [:Dolske] from comment #5)
> There's also the slight oddity that since code pulled in by
> placesOverlay.xul (eg treeView.js) makes use of Cc/Ci/Cr, there would not be
> an implicit requirement that anything wanting to use placesOverlay.xul must
> also provide these variables. Are we ok with that?

Yes.

> I wonder if instead of trying to figure out where to define these, we should
> instead just hack xpconnect to automatically prove these shortcuts

that would be awesome.
Depends on: 767640
Whiteboard: [places-next-wanted]
Blocks: 759260
Depends on: 1432992
Bug 1432992 will remove the definitions, we must still remove the obsolete comment, and make PlacesUtils a defineModuleGetter
Attachment #291023 - Attachment is obsolete: true
most of the work has been done already!
Just this tiny bit left.
Comment on attachment 8948927 [details]
Bug 406371 - Make PlacesUtils a lazy module getter in placesOverlay.xul.

Setting the review flag here, mozreview is apparently broken...
Attachment #8948927 - Flags: review?(standard8)
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Comment on attachment 8948927 [details]
Bug 406371 - Make PlacesUtils a lazy module getter in placesOverlay.xul.

https://reviewboard.mozilla.org/r/218350/#review224100

Yay!
Attachment #8948927 - Flags: review?(standard8) → review+
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/61c43c2713d7
Make PlacesUtils a lazy module getter in placesOverlay.xul. r=standard8
Blocks: 1436347
https://hg.mozilla.org/mozilla-central/rev/61c43c2713d7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1437242
Blocks: 1434245
Whiteboard: [fxsearch]
You need to log in before you can comment on or make changes to this bug.