349523 - Generate bookmarks.html, default_places.html from properties

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Axel Hecht [:Pike]]

Now that bug 348165 made the links in bookmarks.html strict and only dependent on locale, we can generate that file via a simple set of strings, just like default_places.
This is going to cut down the amount of fileformats just as well as the numerous ways to break those files. And it going to make review of those files a lot easier, too.

Comment 1

[(not reading, please use [e-mail] instead)]

I believe default_places.html should be going away on the trunk.

Comment 2

[Axel Hecht [:Pike]]

Seth, I actually filed this bug already, [e-mail] is just another me.

Comment 3

[Dietrich Ayala (:dietrich)]

Attachment: wip patch for places defaults

Comment 4

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

Comment 5

[Dietrich Ayala (:dietrich)]

latest patch adds favicons, and removes the default_places.html file.

Comment 6

[Axel Hecht [:Pike]]

Comment on attachment 260613 [details] [diff] [review]
fix v1

>Index: browser/locales/en-US/chrome/browser/places/places.properties
>===================================================================
>RCS file: /cvsroot/mozilla/browser/locales/en-US/chrome/browser/places/places.properties,v
>retrieving revision 1.18
>diff -u -8 -p -r1.18 places.properties
>--- browser/locales/en-US/chrome/browser/places/places.properties	13 Mar 2007 01:21:32 -0000	1.18
>+++ browser/locales/en-US/chrome/browser/places/places.properties	4 Apr 2007 17:30:32 -0000
>@@ -62,8 +62,27 @@ tabs.openWarningTitle=Confirm open
> tabs.openWarningMultipleBranded=You are about to open %S tabs.  This might slow down %S while the pages are loading.  Are you sure you want to continue?
> tabs.openButtonMultiple=Open tabs
> tabs.openWarningPromptMeBranded=Warn me when opening multiple tabs might slow down %S
> 
> status_foldercount = %S object(s)
> 
> SelectImport=Import Bookmarks File
> EnterExport=Export Bookmarks File
>+
>+PlacesRootTitle=Bookmarks and History
>+
>+PlacesHistoryQueryURI=place:&beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&sort=4&type=1
>+PlacesHistoryQueryTitle=History
>+PlacesHistoryQueryDescription=Shows all browsing history
>+
>+PlacesBookmarksRootTitle=Bookmarks
>+PlacesBookmarksRootDescription=Add bookmarks to this folder to see them displayed on the Bookmarks Menu
>+PlacesBookmarksRootIconURI=chrome://browser/skin/places/bookmarksMenu.png
>+
>+PlacesBookmarksToolbarTitle=Bookmarks Toolbar Folder
>+PlacesBookmarksToolbarDescription=Add bookmarks to this folder to see them displayed on the Bookmarks Toolbar
>+PlacesBookmarksToolbarIconURI=chrome://browser/skin/places/bookmarksToolbar.png
>+
>+PlacesFeedsQueryURI=place:&annotation=livemark%2FfeedURI
>+PlacesFeedsQueryIconURI=chrome://browser/skin/places/livemarkItem.png
>+PlacesFeedsQueryTitle=Subscriptions
>+PlacesFeedsQueryDescription=Shows all Subscribed Feeds

Most of these shouldn't be in locales/en-US, like, the URIs definitely not, both the icon and query urls. 

How many of the titles are we using right now, and how many of those are waiting for the rewrite of the manager?

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: fix v2 - fixes axel's comments

(In reply to comment #6)
> 
> Most of these shouldn't be in locales/en-US, like, the URIs definitely not,
> both the icon and query urls. 

i moved all of the new properties into chrome and out of locale.

> 
> How many of the titles are we using right now, and how many of those are
> waiting for the rewrite of the manager?
> 

don't know, because the manager requirements for fx3 are not solid, so i've removed them from locale for now.

Comment 8

[Axel Hecht [:Pike]]

Comment on attachment 260723 [details] [diff] [review]
fix v2 - fixes axel's comments

r=me. Could you add a comment to places.properties that only the title strings should move over to locales/en-US?

If you want to keep the query and icon urls in a properties files, you could split the l10n and functionality part already, that's up to you.

Thanks for getting me on the hook here.

Comment 9

[(not reading, please use [e-mail] instead)]

Comment on attachment 260723 [details] [diff] [review]
fix v2 - fixes axel's comments

r=sspitzer

Comment 10

[(not reading, please use [e-mail] instead)]

note to dietrich:

I'm very excited that we have:

PlacesHistoryQueryURI=place:&beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&sort=4&type=1

If you lxr in our code, we use that same value in several places:

/browser/components/places/content/controller.js, line 46 -- const ORGANIZER_ROOT_HISTORY_UNSORTED = "place:&beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&type=1"
/browser/components/places/content/history-panel.xul, line 122 -- place="place:&beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&type=1"

Can you log a spin off bug about fixing our code to use the property?

note, we also use something similar, but slightly different, here:

/browser/base/content/browser-menubar.inc, line 334 -- place="place:&beginTime=-2592000000000&beginTimeRef=1&endTime=7200000000&endTimeRef=2&type=0&sort=4&maxResults=10">

For future extension developers, having one place to get PlacesHistoryQueryURI, instead of following our copy-and-paste approach, would be good.

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

mano pointed out that the subscriptions folder is obsolete. this patch doesn't create the folder. it does leave the un-localized properties there in case we use it during upcoming manager changes. we can remove it later if necessary.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: fix v4

per mano, the history query is unnecessary as well. leaving in the strings for now, just not creating the bookmark when initializing the datastore. we can remove the strings if necessary when we move them to locale.

Comment 13

[Asaf Romano (gone)]

Comment on attachment 261271 [details] [diff] [review]
fix v4

>Index: browser/components/places/content/places.properties
>===================================================================

So this should probably go to toolkit/

>+// nsNavBookmarks::InitDefaults
>+//
>+// Initializes default bookmarks and containers.
>+// Pulls from places.propertes for l10n.
>+// Replaces the old default_places.html file.
>+nsresult
>+nsNavBookmarks::InitDefaults()
>+{
>+  // give bookmarks root folder a title "Bookmarks"
>+  nsXPIDLString bookmarksTitle, bookmarksIcon;
>+  nsresult rv = mBundle->GetStringFromName(NS_LITERAL_STRING("PlacesBookmarksRootTitle").get(), getter_Copies(bookmarksTitle));

please try to keep code lines not that long (in browser/ I would mumble something about 80 character).

>+// BookmarkContentSink::SetItemFavicon
>+//
>+//    This sets the given favicon URI for the given folder. It is used to
>+//    initialize the favicons for the bookmarks menu and toolbar. We don't
>+//    actually set any data here because we assume the URI is a chrome: URI.
>+//    These do not have to contain any data for them to work.
>+
>+nsresult
>+nsNavBookmarks::SetItemFavicon(PRInt64 aId, const nsAString& aFavicon,
>+                               PRBool aIsFolder)
>+{
>+  nsFaviconService* faviconService = nsFaviconService::GetFaviconService();
>+  NS_ENSURE_TRUE(faviconService, NS_ERROR_OUT_OF_MEMORY);
>+
>+  nsresult rv;
>+  nsCOMPtr<nsIURI> itemURI;
>+  if (aIsFolder) {
>+    rv = GetFolderURI(aId, getter_AddRefs(itemURI));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+  }
>+  else {
>+    rv = GetItemURI(aId, getter_AddRefs(itemURI));

Hrm, we don't set favicons on bookmark items directly (the bookmarked uri is annotated).

I would rather though stop setting these icons at all for now.

Comment 14

[Dietrich Ayala (:dietrich)]

Attachment: fix v5

addresses mano's comments. xpcshell didn't like the properties file in a places/content subdir, so i put it in the global content.

Comment 15

[Dietrich Ayala (:dietrich)]

Attachment: fix v6 - w/ string file in the right place

Comment 16

[Asaf Romano (gone)]

Comment on attachment 261412 [details] [diff] [review]
fix v6 - w/ string file in the right place

r=mano.

Comment 17

[Dietrich Ayala (:dietrich)]

RCS file: /cvsroot/mozilla/toolkit/components/places/jar.mn,v
done
Checking in toolkit/components/places/jar.mn;
/cvsroot/mozilla/toolkit/components/places/jar.mn,v  <--  jar.mn
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/places/content/places.properties,v
done
Checking in toolkit/components/places/content/places.properties;
/cvsroot/mozilla/toolkit/components/places/content/places.properties,v  <--  places.properties
initial revision: 1.1
done
Checking in toolkit/components/places/src/nsNavBookmarks.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.cpp,v  <--  nsNavBookmarks.cpp
new revision: 1.80; previous revision: 1.79
done
Checking in toolkit/components/places/src/nsNavBookmarks.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavBookmarks.h,v  <--  nsNavBookmarks.h
new revision: 1.37; previous revision: 1.36
done