415117 - get rid of chrome://browser in toolkit places

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Robert Kaiser]

If we want other apps than Firefox to be able to use toolkit's places support, we need to get rid of chrome://browser referencesin that code, as that is not defined in other apps. This should use chrome://mozapps instead.

See http://mxr.mozilla.org/mozilla/search?string=chrome%3A%2F%2Fbrowser&find=%2Ftoolkit%2Fcomponents%2Fplaces%2F&findi=&filter=&tree=mozilla for the places (!) we need to fix.

Comment 2

[Robert Kaiser]

Attachment: remove chrome://browser dependencies from toolkit places

This patch moves all chrome://browser references we have in toolkit places code to chrome://mozapps URIs.

It looks like finduri-Hostname-is- is also unused in the browser places.properties file, but Mano might know better. I could not find any references to the strings in browser/ so I just moved them over.

The icon is unused in browser/ for winstripe and gnomestripe (and the same for both), moving it to toolkit winstripe makes it being included for both of them.
The pinstripe theme references the icon in browser/ so I did not remove it there, but also added a copy in toolkit, as otherwise we wouldn't be able to get rid of that dependency.

Comment 4

[Asaf Romano (gone)]

Comment on attachment 300687 [details] [diff] [review]
remove chrome://browser dependencies from toolkit places

the uri is chrome://places/locale/places.properties

Comment 5

[Robert Kaiser]

Attachment: use chrome://places/locale

Sorry, this patch corrects the .properties URL.

Comment 6

[Phil Ringnalda (:philor)]

Isn't the icon unused in toolkit/? Seems to just be set to a const that initializes lms._iconURI which isn't ever used again.

Comment 7

[Robert Kaiser]

That's possible, I don't understand the code well enough, I just understand what it needs to move it over.

Comment 8

[Asaf Romano (gone)]

Comment on attachment 300756 [details] [diff] [review]
use chrome://places/locale

r=mano

Comment 9

[Phil Ringnalda (:philor)]

For that matter, that icon's only used in browser/ in what looks to be an abandoned way from some previous design, where in Windows and Linux live bookmark items get the generic bookmark favicon until they've been visited, and in OS X they get livemarkItem.png. Mmm, cruft!

Comment 10

[Robert Kaiser]

Attachment: alternate patch: include some cleanup

Here's an alternate patch - as we are in freeze anyways and this will surely not land before unfreeze, I have added some cleanup: removal of livemarkItem.png and removal of the apparently unused finduri-Hostname-is- string.

Mano, I'll leave it up to you which one of those you want landed - I need some testing by FF people though to ensure this doesn't break functionality. I usually don't use or test Minefield, so I don't know the functionality well enough to see what could/would break because of this patch.

I'll request approval for whatever variant of the patch Mano want to see landed.

Comment 11

[Asaf Romano (gone)]

Yes, the icon should no longer bet set through the livemarkservice (did you actually remove all the references to _iconURI?). That said, Pinstripe is still using this icon, so i would still bother moving the images and tweaking jar.mns. Just make sure the service no longer refers to that.

Or you could even leave the icons in their place, and just dereference in in the service code. We can move the icons to browser/ later.

Comment 12

[Robert Kaiser]

1) From what I saw, _iconURI is only set but never used anywhere nowadays.

2) The icon _is_ already in browser/, I just went and removed it from everywhere else than pinstripe in that second patch. As you stated, that's the only place it's still used in, I won't touch that.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 300777 [details] [diff] [review]
alternate patch: include some cleanup

ok!

Comment 14

[Robert Kaiser]

Comment on attachment 300777 [details] [diff] [review]
alternate patch: include some cleanup

Requesting 1.9 approval, this fixes browser/-dependency in toolkit, so that other apps than FF can use places.

Comment 15

[Robert Kaiser]

Checked in.

Comment 16

[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