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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Dietrich Ayala (:dietrich)]

Attachment: v1

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.

Comment 1

[Asaf Romano (gone)]

Comment on attachment 291023 [details] [diff] [review]
v1

ugh, whatever, r=mano.

Comment 2

[Dietrich Ayala (:dietrich)]

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.

Comment 3

[Gervase Markham [:gerv]]

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

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Justin Dolske [:Dolske]]

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.

Comment 6

[Dietrich Ayala (:dietrich)]

(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

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[Justin Dolske [:Dolske]]

Filed bug 767640.

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Marco Bonardo [:mak]]

Attachment: Bug 406371 - Make PlacesUtils a lazy module getter in placesOverlay.xul.

Review commit: https://reviewboard.mozilla.org/r/218350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218350/

Comment 11

[Marco Bonardo [:mak]]

most of the work has been done already!
Just this tiny bit left.

Comment 12

[Marco Bonardo [:mak]]

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...

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8948927 [details]
Bug 406371 - Make PlacesUtils a lazy module getter in placesOverlay.xul.

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

Yay!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/61c43c2713d7
Make PlacesUtils a lazy module getter in placesOverlay.xul. r=standard8

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/61c43c2713d7