389987 - stop exporting favicon to bookmark.html

Status: RESOLVED INVALID

(Firefox :: Bookmarks & History, defect)

Description

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

stop exporting favicon to bookmark.html

nsPlacesImportExportService::WriteItem() we call WriteFaviconAttribute() in order to export the favicon as the ICON attribute.

I think this is legacy code from fx 2, where we our on disk format for bookmarks is bookmarks.html.  

I claim (like with certain aspects of microsummaries) we don't need export this to disk, because we will re-generate them automatically.  

this should make shutdown faster, but I'm not sure by how much.  I noticed this because shutdown got very slow when I accidentally removed the moz_places_faviconindex (of the favicon_id column of the moz_places table).

note, there will be cases (like #380362) where we may want to continue to export the favicon.  perhaps conintue to export for chrome or data urls, but no for http / https?

dietrich / thunder / mano, what do you think?

Comment 1

[Lech Deregowski [:lech]]

Saw your message in bug #257791 pointing here, Seth. But my question is: if you're regenerating it each time then wouldn't that make startups slower by sending dozens even hundreds or thousands of queries to retrieve all those favicons just for that new session? Or do you mean simply retrieving favicons as the user clicks through their bookmarks and storing everything afterward? I'm guessing the latter.

In bug #257791 I simply proposed why trimming down the file size upon export, and optionally when backups are made might be a good idea. By stripping the icons from the file without changing how displaying bookmarks currently works you wind up with some smaller files to import when they're needed again without breaking any of the current functionality.

Alternately, if there's a way to store the icons in a separate file which could be flushed might be more ideal but would probably create unwanted overhead. Just a thought.

Comment 2

[Dietrich Ayala (:dietrich)]

(In reply to comment #1)
> Saw your message in bug #257791 pointing here, Seth. But my question is: if
> you're regenerating it each time then wouldn't that make startups slower by
> sending dozens even hundreds or thousands of queries to retrieve all those
> favicons just for that new session? Or do you mean simply retrieving favicons
> as the user clicks through their bookmarks and storing everything afterward?
> I'm guessing the latter.

bookmarks.html is not imported on every startup, only when migrating from Fx2 (or manually by the user). However, it *is* exported at every shutdown, which is the optimization this bug is targeting.

Comment 3

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

recently, thunder implemented backup and restore.

I think that for the user driven backup we might want to export favicons, but for automatic backups on shutdown, we should not (for the reasons listed above).

as with all our perf bugs, some real numbers are needed to warrant the risk.

Comment 4

[Lech Deregowski [:lech]]

Could there be a benefit of not backing up at all if the current bookmarks and backups haven't been modified at all for the current session? While closing out doesn't take long to run through necessary tasks to close down the application properly, if bookmarks aren't modified to begin with then why bother making another backup at all? If the file isn't corrupt and hasn't been modified there shouldn't really be a need to make yet another backup if you're constantly opening and closing browser sessions.

- Just mulling over the options here.

Comment 5

[Przemyslaw Bialik]

(In reply to comment #3)
> I think that for the user driven backup we might want to export favicons
There could be additional dialog for user to decide if he wants to export with favicons or without them.
or as Lech suggested save favicons in separate file (this would also fix bug 257791 and partially Bug 257474 - they would be saved but at least in different file ).

Comment 6

[Lech Deregowski [:lech]]

Seth, from my understanding FX3 will be storing bookmarks and URLdata within an SQLite database (places?). If this is the case, perhaps a switch could be implemented that shows up in about:config or the bookmarks manager to give users an option to prune favicons from either backups, exports or both.

Something like: browser.bookmarks.prune_favicons (true/false) boolean value with the default being false so people don't freak out over the expected behavior.

Comment 7

[Ondrej Brablc]

This bug is probably invalid because with bug 384370 we will have automatic backup only once a day and in JSON format. 

About the export, at the moment we export both ICON_URI and ICON. We import favicons only if they are chrome: URI or if they contain both ICON_URI and ICON.

Comment 8

[Lech Deregowski [:lech]]

(In reply to comment #3)
> recently, thunder implemented backup and restore.
> 
> I think that for the user driven backup we might want to export favicons, but
> for automatic backups on shutdown, we should not (for the reasons listed
> above).
> 
> as with all our perf bugs, some real numbers are needed to warrant the risk.
> 

Do we have the numbers for this which may favor which export option is better in regards to exporting with or without icon data?

Comment 9

[Ondrej Brablc]

(In reply to comment #8)
> Do we have the numbers for this which may favor which export option is better
> in regards to exporting with or without icon data?

JSON backups (bug 384370) contain neither favicon data nor favicon uri. So if JSON backups land, this bug will really be invalid. So the numbers are probably irrelevant (but if someone really needs them for .html format, I can get them).

Comment 10

[Lech Deregowski [:lech]]

Yeah, I was just curious what the numbers looked like for exporting to .html format, since I think that's what most general users will wind up doing.

Ondrej, will we be able to export between JSON and .html or would JSON simply be for backups?

Comment 11

[Ondrej Brablc]

(In reply to comment #10)
> Yeah, I was just curious what the numbers looked like for exporting to .html
> format, since I think that's what most general users will wind up doing.
> 
> Ondrej, will we be able to export between JSON and .html or would JSON simply
> be for backups?
> 

From what I have seen you will not be able to choose the export format (JSON would hardly be usefull for common users). Favicons were imported in Fx2 as well, so there is now really no reason why stopping export of favicons.

Comment 12

[Lech Deregowski [:lech]]

> From what I have seen you will not be able to choose the export format (JSON
> would hardly be usefull for common users). Favicons were imported in Fx2 as
> well, so there is now really no reason why stopping export of favicons.
> 

I understand that you won't be able to choose the export format and that JSON wouldn't be useful for common users. I'm not arguing against that at all.

One of the side discussions for this was actually the manual exporting of bookmarks via the places manager for common users. dumping the ICON and ICON_URI data would greatly reduce file size just for portability should the user require it. But perhaps this is for another bug, since what I was discussing no longer applies the scope of the usage described here.

Comment 13

[Ondrej Brablc]

The backup format has just been changed to JSON and favicons are not included in it. This makes this bug invalid.

Comment 14

[Asaf Romano (gone)]

We still support exporting to the legacy format.

Comment 15

[Ondrej Brablc]

(In reply to comment #14)
> We still support exporting to the legacy format.

This bug was about slow shutdown, the problem was .html file with icons and backup performed always on shutdown. Now we backup only once a day and do not include the favicons. So I believe this bug should be closed.

If someone wants to propose, that it is necessary to remove favicons from .html file for good, then he should have good reasons for this. I personally (as author of SiteBar Bookmark Manager) would vote against this, because I can import the favicons from the file.

If we want to have it optional, then it should be part of bigger export dialog redesign (may be part of bug 254292), we may want to fine-tune there if we want to export descriptions or last modified time and so on.

Comment 16

[mmortal03]

Yeah, at least have an option of cleaning them up from the file in the context of the Clear Private Data option.

Comment 17

[Ondrej Brablc]

(In reply to comment #16)
> Yeah, at least have an option of cleaning them up from the file in the context
> of the Clear Private Data option.

Some people may prefer not to have favicons at all, but this would deserve a new bug too. This one is IMHO really invalid. I'm trying to mark it again, if I'm not supposed to do this, please tell me. I feel that having a bug opened where it is unclear what should be done is not useful. Nobody can pick up such bug and implement.

Comment 18

[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