Closed Bug 420729 Opened 13 years ago Closed 11 years ago

ITEM_ID neither exported nor imported from bookmarks.html

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.7a1

People

(Reporter: ondrej, Assigned: mak)

References

Details

Attachments

(1 file, 3 obsolete files)

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!
delete it.
Attached patch Remove unused code (obsolete) — Splinter Review
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.
Attachment #308406 - Flags: review?(dietrich)
Status: NEW → ASSIGNED
Blocks: 392193
Comment on attachment 308406 [details] [diff] [review]
Remove unused code

r=me, thanks for cleanup.
Attachment #308406 - Flags: review?(dietrich) → review+
Keywords: checkin-needed
Attachment #308406 - Flags: approval1.9?
OS: Windows XP → All
Hardware: PC → All
Comment on attachment 308406 [details] [diff] [review]
Remove unused code

I have found more lines to be removed.
Attachment #308406 - Attachment is obsolete: true
Attachment #308406 - Flags: review+
Attachment #308406 - Flags: approval1.9?
Attached patch Remove unused code v2 (obsolete) — Splinter Review
Attachment #308489 - Flags: review?(dietrich)
Comment on attachment 308489 [details] [diff] [review]
Remove unused code v2

r=me, thanks
Attachment #308489 - Flags: review?(dietrich) → review+
Assignee: ondrej → nobody
Comment on attachment 308489 [details] [diff] [review]
Remove unused code v2

Any reason this didn't land? Does this still apply to tip?
Status: ASSIGNED → NEW
No longer blocks: 392193
Blocks: 492384
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
Attached patch patch v2.0 (obsolete) — Splinter Review
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).
Assignee: nobody → mak77
Attachment #308489 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #419424 - Flags: review?(dietrich)
Attachment #419424 - Flags: review?(dietrich)
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.
Attached patch patch v2.1Splinter Review
this one is better.
Attachment #419424 - Attachment is obsolete: true
Attachment #421343 - Flags: review?(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.
Attachment #421343 - Flags: review?(dietrich) → review+
(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.
Summary: ITEM_ID neither exported nor imported → ITEM_ID neither exported nor imported from bookmarks.html
removed the dump and pushed.
http://hg.mozilla.org/mozilla-central/rev/19cbd4b19925
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.7a1
Depends on: 593566
You need to log in before you can comment on or make changes to this bug.