Closed Bug 415117 Opened 17 years ago Closed 17 years ago

get rid of chrome://browser in toolkit places

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: kairo, Assigned: kairo)

References

()

Details

(Keywords: late-l10n)

Attachments

(2 files, 1 obsolete file)

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.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → DUPLICATE
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.
Assignee: nobody → kairo
Status: RESOLVED → ASSIGNED
Attachment #300687 - Flags: review?(mano)
Resolution: DUPLICATE → ---
Comment on attachment 300687 [details] [diff] [review]
remove chrome://browser dependencies from toolkit places

the uri is chrome://places/locale/places.properties
Attachment #300687 - Flags: review?(mano) → review-
Sorry, this patch corrects the .properties URL.
Attachment #300687 - Attachment is obsolete: true
Attachment #300756 - Flags: review?(mano)
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.
That's possible, I don't understand the code well enough, I just understand what it needs to move it over.
Comment on attachment 300756 [details] [diff] [review]
use chrome://places/locale

r=mano
Attachment #300756 - Flags: review?(mano) → review+
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!
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.
Attachment #300777 - Flags: review?(mano)
Blocks: 382187
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.
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 on attachment 300777 [details] [diff] [review]
alternate patch: include some cleanup

ok!
Attachment #300777 - Flags: review?(mano) → review+
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.
Attachment #300777 - Flags: approval1.9?
Keywords: late-l10n
Attachment #300777 - Flags: approval1.9? → approval1.9+
Checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: