334408 - Bookmark import/export should get/set last character set as annotation and use it when loading a page

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Joe Hughes]

Comment 1

[Anders Conbere]

I don't know which of these you meant to file but I'll dupe them to this one.

Comment 2

[Anders Conbere]

or not.  Joe... did you mean to file 3 bugs (334408, 334409, 334410) that are all almost exactly the same?

Comment 3

[Joe Hughes]

Basically, the work described here will define the annotation to use, and then bug 334409 and bug 334410 describe two different conditions in which that annotation needs to be updated.

Comment 4

[Phil Ringnalda (:philor)]

Does "use it when loading a page" encompass implementing nsICharsetResolver (bug 284660, bug 101995), or do we need another bug for that?

Comment 5

[Dan Mills [:thunder]]

Attachment: wip patch

Do we want to land this before turning bookmarks on?

I'm not sure whether (a) the anno string is at all appropriate, or (b) it should be a page anno vs an item anno.  But, heh, here's a start.

Note: only applies cleanly on top of "fix v5" patch from bug 376253.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 264984 [details] [diff] [review]
wip patch

In places we want to this to be set as a page-annotation, see also bug 317472.

I don't think we should block places-bookmarks for this FWIW.

Comment 7

[Dan Mills [:thunder]]

Attachment: patch v1

Fixes the anno string to match the one in bug 317472.

Comment 8

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

hey dan, I think something is wrong with that patch.

Comment 9

[Dan Mills [:thunder]]

Attachment: patch v1 (really)

I had a long battle with hg for windows (which I lost) today, trying to make it use 8 lines of context for diffs.  I guess I broke my patch during that time... sigh.

Thanks Seth :)

Comment 10

[Asaf Romano (gone)]

This should be set as a uri annotation, not item annotation (if the annotation is not set already).

Comment 11

[Asaf Romano (gone)]

Comment on attachment 271177 [details] [diff] [review]
patch v1 (really)

For the same reason i minus ed the former ;)

Comment 12

[Dan Mills [:thunder]]

Attachment: patch v2

That's... embarrassing.  Sorry.

Here's a new patch, I made these changes:

$ hg diff
diff -r c2674415e051 browser/components/places/src/nsPlacesImportExportService.cpp
--- a/browser/components/places/src/nsPlacesImportExportService.cpp     Thu Jul 05 22:25:18 2007 -0700
+++ b/browser/components/places/src/nsPlacesImportExportService.cpp     Fri Jul 06 13:45:08 2007 -0700
@@ -965,9 +965,11 @@ BookmarkContentSink::HandleLinkBegin(con
 
   // import last charset
   if (!lastCharset.IsEmpty()) {
-    mAnnotationService->SetItemAnnotationString(frame.mPreviousId, LAST_CHARSET_ANNO,
-                                                lastCharset, 0,
-                                                nsIAnnotationService::EXPIRE_NEVER);
+    nsCOMPtr<nsIURI> hrefObject;
+    if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(hrefObject), href)))
+      mAnnotationService->SetPageAnnotationString(hrefObject, LAST_CHARSET_ANNO,
+                                                  lastCharset, 0,
+                                                  nsIAnnotationService::EXPIRE_NEVER);
   }
 }
 
@@ -1922,12 +1924,12 @@ nsPlacesImportExportService::WriteItem(n
 
   // last charset
   PRBool hasLastCharset = PR_FALSE;
-  rv = mAnnotationService->ItemHasAnnotation(itemId, LAST_CHARSET_ANNO,
+  rv = mAnnotationService->PageHasAnnotation(pageURI, LAST_CHARSET_ANNO,
                                              &hasLastCharset);
   NS_ENSURE_SUCCESS(rv, rv);
   if (hasLastCharset) {
     nsAutoString lastCharset;
-    rv = mAnnotationService->GetItemAnnotationString(itemId, LAST_CHARSET_ANNO,
+    rv = mAnnotationService->GetPageAnnotationString(pageURI, LAST_CHARSET_ANNO,
                                                      lastCharset);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = aOutput->Write(kLastCharsetAttribute, sizeof(kLastCharsetAttribute)-1, &dummy);

Comment 13

[Asaf Romano (gone)]

Comment on attachment 271265 [details] [diff] [review]
patch v2


>+  if (!lastCharset.IsEmpty()) {
>+    nsCOMPtr<nsIURI> hrefObject;
>+    if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(hrefObject), href)))

You have it in frame.mPreviousLink.

>+      mAnnotationService->SetPageAnnotationString(hrefObject, LAST_CHARSET_ANNO,

I think we should only do this if the annotation is not already set.

Comment 14

[Dan Mills [:thunder]]

(In reply to comment #13)
> (From update of attachment 271265 [details] [diff] [review])
> 
> >+  if (!lastCharset.IsEmpty()) {
> >+    nsCOMPtr<nsIURI> hrefObject;
> >+    if (NS_SUCCEEDED(NS_NewURI(getter_AddRefs(hrefObject), href)))
> 
> You have it in frame.mPreviousLink.

Indeed.  I'll change the microsummary import to match as well, then.

> >+      mAnnotationService->SetPageAnnotationString(hrefObject, LAST_CHARSET_ANNO,
> 
> I think we should only do this if the annotation is not already set.

Can you explain why?  It makes more sense to me to have the imported value take precedence.

Comment 15

[Dan Mills [:thunder]]

Attachment: patch v3

Uses frame.mPreviousLink instead of making a new uri object.

Comment 16

[Asaf Romano (gone)]

If I've visited a page, then imported my ancient bookmarks file which happened to have that page in too, the already-set character-set should take precedence IMO. I think we better consider generated meta data like this as useful only if we don't have something better.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 271407 [details] [diff] [review]
patch v3

r=mano otherwise.

Comment 18

[Dan Mills [:thunder]]

Attachment: final patch

Here it is with the anno check.  I think you're right.  At least, now that we're appending bookmarks instead of merging them in.
I'll commit tomorrow when I'll be awake to watch tinderbox...

Comment 19

[Dan Mills [:thunder]]

Checked in:

Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.25; previous revision: 1.24
done

I forgot to update the tests for the item -> page & anno name changes, so I had to commit those twice.  Last commit was:

Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.10; previous revision: 1.9
done

Marking as fixed.

Comment 20

[Tracy Walker [:tracy]]

Is there a user method of verifying this bug? 
Also should this bug be flagged in-testsuite+

Comment 21

[Dan Mills [:thunder]]

I used the sqlite database browser (http://sqlitebrowser.sourceforge.net/) to open up places.sqlite and check its contents.

I don't think there's an easier way to check it, since the annotation is not currently used to choose the encoding.

Comment 22

[Tracy Walker [:tracy]]

Dan,  are there unit test case(s) running regularly that confirm it is fixed? It be fine with that as verification.  

Comment 23

[Dan Mills [:thunder]]

Yes, see here:

http://lxr.mozilla.org/mozilla/source/browser/components/places/tests/unit/test_bookmarks_html.js#261

Comment 24

[Phil Ringnalda (:philor)]

(In reply to comment #21)
> I don't think there's an easier way to check it, since the annotation is not
> currently used to choose the encoding.

So the answer to comment 4 was "yes, we need another bug about using it, the 'use it when loading a page' in the summary is just a tease"?

Comment 25

[Tracy Walker [:tracy]]

ok, verified per unit tests 

also added in-testsuite+

Comment 26

[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