Closed Bug 373501 Opened 17 years ago Closed 17 years ago

Places does not import microsummaries set on legacy-bookmarks

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 alpha5

People

(Reporter: asaf, Assigned: hello)

References

Details

(Keywords: dataloss)

Attachments

(1 file, 4 obsolete files)

Places does not import microsummaries set on legacy-bookmarks.
on this topic, does Fx 2 properly handle import / export of microsummaries (I think so, but I haven't checked.)
dietrich tells me that it does.

so this is about fixing the Fx 3 import of Fx2 microsummaries in legacy bookmarks.html
Blocks: 375108
Attached patch wip patch (obsolete) — Splinter Review
This should be done using the microsummaries service, don't set annotations directly.

This bug depends on moving the import & export code to browser/.
Assignee: nobody → thunder
Depends on: 379913
Attached patch wip patch mk II (obsolete) — Splinter Review
Updated for moved importer/exporter code to browser/, changed to use the microsummary service (requires patch, see bug 379913).
Attachment #259746 - Attachment is obsolete: true
Attachment #263962 - Flags: review?(dietrich)
Comment on attachment 263962 [details] [diff] [review]
wip patch mk II


>+    // import microsummary
>+    // Note: expiration and generated title are ignored, and will be
>+    // recalculated by the microsummary service

in that case, do we even need to bother importing/exporting these?

hm, i wonder if a bookmarks.html w/o those fields is imported correctly into pre-places bookmarks. if so, we shouldn't bother with them at all.
Comment on attachment 263962 [details] [diff] [review]
wip patch mk II


>+
>+    // write out expiration
>+    nsAutoString micsumExpiration;
>+    rv = mAnnotationService->GetAnnotationString(placeURI, ANNO_MICSUM_EXPIRATION, micsumExpiration);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = aOutput->Write(kMicsumExpirationEquals, sizeof(kMicsumExpirationEquals)-1, &dummy);
>+    if (NS_FAILED(rv)) return rv;
>+    char* utf8Expiration = ToNewUTF8String(micsumExpiration); 
>+    aOutput->Write(utf8Expiration, sizeof(utf8Expiration), &dummy);
>+    rv = aOutput->Write(kQuoteStr, sizeof(kQuoteStr)-1, &dummy);
>+    if (NS_FAILED(rv)) return rv;
>+    nsMemory::Free(utf8Expiration);

this fails, needs to be GetAnnotationInt64, per the microsummary service.

also, can you please add tests for these changes here:

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

a microsummarized bookmark is already in the test data, just needs the anno checks to be done.
Attachment #263962 - Flags: review?(dietrich) → review-
Attached patch wip patch mk III (obsolete) — Splinter Review
* Updates for latest changes in places/microsummaries interfaces.
* Adds tests
* Removes export of expiration and generated title

Expiration and generated title change every 1/2 hour, so they don't make sense to import/export.

There is a crasher bug that is preventing the tests from running correctly.  I'll file a separate bug for that.
Attachment #263962 - Attachment is obsolete: true
Attachment #264545 - Flags: review?
Attachment #264545 - Flags: review? → review?(mano)
Depends on: 380468
Crasher bug filed as bug 380468.
Just to clarify why I removed import/export of expiration and generated titles:

Expiration is variable, but defaults to 30 mins and we probably won't see a whole lot of change in that (imo).  That means that any exports/imports farther apart than 30 mins will have most microsummaries already expired anyway.  And even in cases when export/import are close together (e.g., profile migrations), it is harmless--the microsummary service just fetches the summary and that's it.

As for the generated title, the same logic as for expiration follows.  I'm not as sure about this one, though, because it might be useful for anyone looking at bookmarks.html with an editor.

Note that Firefox 2 only requires the generator uri to be set (I tested it with an exported bookmarks.html from minefield+this patch).
I should also add...

There is currently no interface in nsMicrosummaryService.idl to get or set the expiration.  That would need to be added, or we'd need to munge the annos directly.

There is an interface to get (but not to set) the summarized content, but it is asynchronous only, which would be sort of messy to deal with during export.
Attached patch wip patch mk IV (obsolete) — Splinter Review
This version imports the generated title.  It does not export it, however.
Attachment #264545 - Attachment is obsolete: true
Attachment #264819 - Flags: review?(mano)
Attachment #264545 - Flags: review?(mano)
Er, you don't seem to export the generator URI (you do have kMicsumGenURIEqual defined though).
Comment on attachment 264819 [details] [diff] [review]
wip patch mk IV

>@@ -831,21 +847,41 @@ BookmarkContentSink::HandleLinkBegin(con

>+  // recalculated by the microsummary service
>+  if (!micsumGenURI.IsEmpty()) {
>+    nsCOMPtr<nsIURI> micsumGenURIObject;
>+    NS_NewURI(getter_AddRefs(micsumGenURIObject), micsumGenURI);
>+    nsCOMPtr<nsIURI> hrefObject;
>+    NS_NewURI(getter_AddRefs(hrefObject), href);

wrap the code in NS_SUCCEEDED() for each uri creation, NS_NewURI will fail for bogus input.

>+    nsCOMPtr<nsIMicrosummary> microsummary;
>+    mMicrosummaryService->CreateMicrosummary(hrefObject, micsumGenURIObject,
>+                                             getter_AddRefs(microsummary));
>+    mMicrosummaryService->SetMicrosummary(frame.mPreviousId, microsummary);
>+ 	
>+    // create generated title anno
>+    rv = mAnnotationService->SetItemAnnotationString(frame.mPreviousId, GENERATED_TITLE_ANNO,
>+                                                     generatedTitle, 0,
>+                                                     nsIAnnotationService::EXPIRE_NEVER);
>+    NS_ASSERTION(NS_SUCCEEDED(rv), "Creating microsummary generated title failed");
>+  }

>@@ -1595,16 +1632,40 @@ nsPlacesImportExportService::WriteItem(n

>+  // microsummary
>+  nsCOMPtr<nsIMicrosummary> microsummary;
>+  rv = mMicrosummaryService->GetMicrosummary(itemId, getter_AddRefs(microsummary));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (microsummary) {
>+    nsCOMPtr<nsIMicrosummaryGenerator> generator;
>+    rv = microsummary->GetGenerator(getter_AddRefs(generator));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCOMPtr<nsIURI> genURI;
>+    rv = generator->GetUri(getter_AddRefs(genURI));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    nsCAutoString spec;
>+    rv = genURI->GetSpec(spec);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // write out generator URI
>+    rv = aOutput->Write(kMicsumGenURIEquals, sizeof(kMicsumGenURIEquals)-1, &dummy);
>+    if (NS_FAILED(rv)) return rv;
>+    rv = WriteEscapedUrl(spec, aOutput);
>+    if (NS_FAILED(rv)) return rv;
>+    rv = aOutput->Write(kQuoteStr, sizeof(kQuoteStr)-1, &dummy);
>+    if (NS_FAILED(rv)) return rv;

prefer NS_ENSURE_SUCCESS

>diff -r 9307bf9f27cf browser/components/places/src/nsPlacesImportExportService.h
>--- a/browser/components/places/src/nsPlacesImportExportService.h	Mon May 14 16:03:22 2007 -0700
>+++ b/browser/components/places/src/nsPlacesImportExportService.h	Mon May 14 20:39:45 2007 -0700
>@@ -23,16 +24,17 @@ class nsPlacesImportExportService : publ
>     virtual ~nsPlacesImportExportService();
> 
>   protected:
>     nsCOMPtr<nsIFaviconService> mFaviconService;
>     nsCOMPtr<nsIAnnotationService> mAnnotationService;
>     nsCOMPtr<nsINavBookmarksService> mBookmarksService;
>     nsCOMPtr<nsINavHistoryService> mHistoryService;
>     nsCOMPtr<nsILivemarkService> mLivemarkService;
>+    nsCOMPtr<nsIMicrosummaryService> mMicrosummaryService;

Ensure its state in EnsureServiceState()

r=mano otherwise.
Attachment #264819 - Flags: review?(mano) → review+
This fixes Mano's comments above, updates the patch for the latest changes on the trunk, and fixes tests so they don't fail.
The microsummary tests are commented out due to bug 380468.
Attachment #264819 - Attachment is obsolete: true
Committed:

Checking in browser/components/build/Makefile.in;
/cvsroot/mozilla/browser/components/build/Makefile.in,v  <--  Makefile.in
new revision: 1.55; previous revision: 1.54
done
Checking in browser/components/places/src/Makefile.in;
/cvsroot/mozilla/browser/components/places/src/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
Checking in browser/components/places/src/nsPlacesImportExportService.cpp;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.cpp,v  <--  nsPlacesImportExportService.cpp
new revision: 1.9; previous revision: 1.8
done
Checking in browser/components/places/src/nsPlacesImportExportService.h;
/cvsroot/mozilla/browser/components/places/src/nsPlacesImportExportService.h,v  <--  nsPlacesImportExportService.h
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/places/tests/unit/bookmarks.preplaces.html;
/cvsroot/mozilla/browser/components/places/tests/unit/bookmarks.preplaces.html,v  <--  bookmarks.preplaces.html
new revision: 1.3; previous revision: 1.2
done
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.4; previous revision: 1.3
done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 alpha5
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.