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

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
12 years ago
a year ago

People

(Reporter: dietrich, Assigned: mak)

Tracking

(Blocks 1 bug)

Trunk
Firefox 60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

12 years ago
Posted 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+
Reporter

Comment 2

12 years ago
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.
Reporter

Updated

12 years ago
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
Assignee

Updated

9 years ago
Depends on: 556739
Assignee

Updated

9 years ago
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!
Assignee

Comment 4

8 years ago
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.
Reporter

Comment 6

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

Comment 7

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

Updated

6 years ago
Depends on: 767640
Assignee

Updated

5 years ago
Whiteboard: [places-next-wanted]
Blocks: 759260
Assignee

Updated

a year ago
Depends on: 1432992
Assignee

Comment 9

a year ago
Bug 1432992 will remove the definitions, we must still remove the obsolete comment, and make PlacesUtils a defineModuleGetter
Assignee

Updated

a year ago
Attachment #291023 - Attachment is obsolete: true
Assignee

Comment 11

a year ago
most of the work has been done already!
Just this tiny bit left.
Assignee

Comment 12

a year ago
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

Updated

a year ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED

Comment 13

a year ago
mozreview-review
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+

Comment 14

a year ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/61c43c2713d7
Make PlacesUtils a lazy module getter in placesOverlay.xul. r=standard8
Assignee

Updated

a year ago
Blocks: 1436347

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61c43c2713d7
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60

Updated

a year ago
Depends on: 1437242
Assignee

Updated

a year ago
Blocks: 1434245
Assignee

Updated

a year ago
Whiteboard: [fxsearch]
You need to log in before you can comment on or make changes to this bug.