420729 - ITEM_ID neither exported nor imported from bookmarks.html

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ondrej Brablc]

nsPlacesImportExportService.cpp contains code that handles ITEM_ID imported from a file. However, this value is never exported nor imported by this module. It should either be implemented or deleted from the code.

Decision please!

Comment 1

[Dietrich Ayala (:dietrich)]

delete it.

Comment 2

[Ondrej Brablc]

Attachment: Remove unused code

The first hunk removes tabs from unrelated code. I have problems with those tabs anytime I'm doing patch with this module. I have fixed style too.

Note that this code does not remove support for ITEM_ID as such, it removes code which could never get executed.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 308406 [details] [diff] [review]
Remove unused code

r=me, thanks for cleanup.

Comment 4

[Ondrej Brablc]

Comment on attachment 308406 [details] [diff] [review]
Remove unused code

I have found more lines to be removed.

Comment 5

[Ondrej Brablc]

Attachment: Remove unused code v2

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 308489 [details] [diff] [review]
Remove unused code v2

r=me, thanks

Comment 7

[Nochum Sossonko [:Natch]]

Comment on attachment 308489 [details] [diff] [review]
Remove unused code v2

Any reason this didn't land? Does this still apply to tip?

Comment 8

[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

Comment 9

[Marco Bonardo [:mak]]

Attachment: patch v2.0

since i was in need to make error checking in this component more solid (to debug some oranges), i included these changes (plus error checking and some cleanup).

But i think this component should be converted in js, moved to toolkit, and be able to handle both html and json import/export (replacing the backup/restore functionalities in PlacesUtils). Especially now that FX2 is discontinued and we can stop being blocked by its issues (like crashes if we add properties to the exported html file).

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 419424 [details] [diff] [review]
patch v2.0

ohr, this is actually bad:
+  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "InsertBookmark failed");
should return on failure, since we can't continue setting properties on a not existant item. Looks like i should do a second pass on this patch.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v2.1

this one is better.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 421343 [details] [diff] [review]
patch v2.1


>-  href.Trim(kWhitespace);
>-  feedUrl.Trim(kWhitespace);
>-  icon.Trim(kWhitespace);
>-  iconUri.Trim(kWhitespace);
>-  lastCharset.Trim(kWhitespace);
>-  keyword.Trim(kWhitespace);
>-  postData.Trim(kWhitespace);
>-  webPanel.Trim(kWhitespace);
>-  itemId.Trim(kWhitespace);
>-  micsumGenURI.Trim(kWhitespace);
>-  generatedTitle.Trim(kWhitespace);
>-  dateAdded.Trim(kWhitespace);
>-  lastModified.Trim(kWhitespace);

why?

>diff --git a/browser/components/places/tests/unit/test_bookmarks_html.js b/browser/components/places/tests/unit/test_bookmarks_html.js
>--- a/browser/components/places/tests/unit/test_bookmarks_html.js
>+++ b/browser/components/places/tests/unit/test_bookmarks_html.js
>@@ -314,6 +314,9 @@ function testCanonicalBookmarks(aFolder)
>   result = histsvc.executeQuery(query, histsvc.getNewQueryOptions());
>   var unfiledBookmarks = result.root;
>   unfiledBookmarks.containerOpen = true;
>+  for (var i = 0; i < unfiledBookmarks.childCount; i++) {
>+    print(unfiledBookmarks.getChild(i).title);
>+  }
>   do_check_eq(unfiledBookmarks.childCount, 1);
>   unfiledBookmarks.containerOpen = false;

ditto

r=me otherwise, thanks for the cleanup.

Comment 13

[Marco Bonardo [:mak]]

(In reply to comment #12)
> (From update of attachment 421343 [details] [diff] [review])
> 
> >-  href.Trim(kWhitespace);
> >-  feedUrl.Trim(kWhitespace);
> >-  icon.Trim(kWhitespace);
> >-  iconUri.Trim(kWhitespace);
> >-  lastCharset.Trim(kWhitespace);
> >-  keyword.Trim(kWhitespace);
> >-  postData.Trim(kWhitespace);
> >-  webPanel.Trim(kWhitespace);
> >-  itemId.Trim(kWhitespace);
> >-  micsumGenURI.Trim(kWhitespace);
> >-  generatedTitle.Trim(kWhitespace);
> >-  dateAdded.Trim(kWhitespace);
> >-  lastModified.Trim(kWhitespace);
> 
> why?

i directly trim value in the for loop before assigning it
+    nsAutoString value(node.GetValueAt(i));
+    value.Trim(kWhitespace);

> >diff --git a/browser/components/places/tests/unit/test_bookmarks_html.js b/browser/components/places/tests/unit/test_bookmarks_html.js
> >--- a/browser/components/places/tests/unit/test_bookmarks_html.js
> >+++ b/browser/components/places/tests/unit/test_bookmarks_html.js
> >@@ -314,6 +314,9 @@ function testCanonicalBookmarks(aFolder)
> >   result = histsvc.executeQuery(query, histsvc.getNewQueryOptions());
> >   var unfiledBookmarks = result.root;
> >   unfiledBookmarks.containerOpen = true;
> >+  for (var i = 0; i < unfiledBookmarks.childCount; i++) {
> >+    print(unfiledBookmarks.getChild(i).title);
> >+  }
> >   do_check_eq(unfiledBookmarks.childCount, 1);
> >   unfiledBookmarks.containerOpen = false;
> 
> ditto

oh sorry, i forgot this debug print, i had to add it to debug a wrong change, will remove.

Comment 14

[Marco Bonardo [:mak]]

removed the dump and pushed.
http://hg.mozilla.org/mozilla-central/rev/19cbd4b19925

Comment 15

[Marco Bonardo [:mak]]

pushed a bustage fix:
http://hg.mozilla.org/mozilla-central/rev/2a1ef305a435