Closed Bug 753205 Opened 12 years ago Closed 12 years ago

bookmarks import ignores separators. ([SeaMonkey] "test_bookmarks_html.js | 3 == 4")

Categories

(Toolkit :: Places, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox13 --- unaffected
firefox14 + fixed

People

(Reporter: sgautherie, Assigned: mak)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

(In reply to Marco Bonardo [:mak] from bug 739041 comment #35)
> (In reply to Serge Gautherie (:sgautherie) from bug 739041 comment #34)
> > Marco, can you confirm that it is expected that the separator is not
> > imported (anymore?)?
> 
> The separators should be imported afaik, it's their names we don't import
> anymore, but this one doesn't even have a name...
> The firefox bookmarks preplaces doesn't have a separator, so there may even
> be an actual regression here, indeed the old code had an handleSeparator,
> the new code is missing it :(
Add this testcase, that SeaMonkey already have and which detected this regression.

test_bookmarks_html.js should be updated when fixing the Toolkit regression.
Attachment #622232 - Flags: review?(mak77)
Assignee: nobody → mak77
Looks like this could cause a fair bit of user confusion if separator is missing, tracking to make sure this gets in for 14.
Serge, I think I'm going to move these import tests to toolkit, so you may make a patch to remove them from seamonkey, this should simplify future support:

-[test_384370.js]
-[test_398914.js]
-[test_457441-import-export-corrupt-bookmarks-html.js]
-[test_bookmarksRestoreNotification.js]
-[test_bookmarks_html.js]
- bookmarks.corrupt.html
- bookmarks.preplaces.html
Comment on attachment 622232 [details] [diff] [review]
(Av1) bookmarks.preplaces.html: Add a 'HR'
[Merged into next patch]

I'm merging this change in my patch
Attachment #622232 - Attachment is obsolete: true
Attachment #622232 - Flags: review?(mak77)
Attached patch patch v1.0 (obsolete) — Splinter Review
moves the import tests to toolkit, adds back importing separators and tests for that.
Attachment #622689 - Flags: review?(dietrich)
Comment on attachment 622689 [details] [diff] [review]
patch v1.0

Review of attachment 622689 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thanks
Attachment #622689 - Flags: review?(dietrich) → review+
Comment on attachment 622689 [details] [diff] [review]
patch v1.0

Review of attachment 622689 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/BookmarkHTMLUtils.jsm
@@ +198,5 @@
> +   * Handles <hr> as a separator.
> +   *
> +   * @note Separators may have a title in old html files, though Places dropped
> +   *       support for them.
> +   *       We also don't import ADD_DATE or LAST_MODIFIED for separators because

Nit: doubled 'because'.

::: browser/components/places/tests/unit/test_384370.js
@@ +166,5 @@
>    rootNode.containerOpen = true;
>  
>    // 6-2: the toolbar contents are imported to the places-toolbar folder,
>    // the separator above it is removed.
> +  do_check_eq(rootNode.childCount, 3);

(Here and in the rest of this (and other) file),
Could you document all the "magic numbers"?
Also this '6-2' doesn't seem to be up-to-date...

This is one of the diff with SeaMonkey test, which I was loath to try and figure out :-/

::: toolkit/components/places/tests/unit/xpcshell.ini
@@ +7,5 @@
>  [test_317472.js]
>  # Bug 676989: test hangs consistently on Android
>  skip-if = os == "android"
>  [test_331487.js]
> +[test_384370.js]

Could this be postponed?
See above.

@@ +12,4 @@
>  [test_385397.js]
>  # Bug 676989: test fails consistently on Android
>  fail-if = os == "android"
> +[test_398914.js]

Ready to merge :-)

@@ +54,5 @@
>  skip-if = os == "android"
>  [test_async_history_api.js]
>  [test_autocomplete_stopSearch_no_throw.js]
>  [test_bookmark_catobs.js]
> +[test_bookmarks_html.js]

Could this be postponed?
I haven't "fully" resync'ed SM test yet.

Also FF and SM "let test_bookmarks" contents are very different.
This doesn't seem as trivial to merge as this...

@@ +55,5 @@
>  [test_async_history_api.js]
>  [test_autocomplete_stopSearch_no_throw.js]
>  [test_bookmark_catobs.js]
> +[test_bookmarks_html.js]
> +[test_bookmarks_html_corrupt.js]

Could this be postponed?
I haven't "fully" resync'ed SM test yet.

@@ +56,5 @@
>  [test_autocomplete_stopSearch_no_throw.js]
>  [test_bookmark_catobs.js]
> +[test_bookmarks_html.js]
> +[test_bookmarks_html_corrupt.js]
> +[test_bookmarks_restore_notification.js]

Ready to merge :-)
Attachment #622689 - Flags: feedback-
(In reply to Serge Gautherie (:sgautherie) from comment #7)
> ::: browser/components/places/tests/unit/test_384370.js
> @@ +166,5 @@
> >    rootNode.containerOpen = true;
> >  
> >    // 6-2: the toolbar contents are imported to the places-toolbar folder,
> >    // the separator above it is removed.
> > +  do_check_eq(rootNode.childCount, 3);
> 
> (Here and in the rest of this (and other) file),
> Could you document all the "magic numbers"?

Nope, the scope of this bug is not to cleanup this crappy test, we will do in future.  Btw, these magic numbers are just "count how many are there in the test html file you are importing", the previous consts were just wrong.

> This is one of the diff with SeaMonkey test, which I was loath to try and
> figure out :-/

You should just remove the test and rely on toolkit testing, or update it if you wish to keep it, I don't think there's a reason to not move these tests to toolkit, they are basically staying the same.
Imo we should just proceed here, you have all the time you want to sync your tests, but you are already getting toolkit coverage in the meanwhile.
Attached patch patch v1.1Splinter Review
removed the double because, and the bogus 6-2 comment
Attachment #622689 - Attachment is obsolete: true
Comment on attachment 622810 [details] [diff] [review]
patch v1.1

Review of attachment 622810 [details] [diff] [review]:
-----------------------------------------------------------------

SeaMonkey tests resync'ed, except for the "wrong" consts: ready to land this patch.

NB: I will do a follow-up patch to merge some differences from SeaMonkey files ;-)

::: browser/components/places/tests/unit/test_bookmarks_html.js
@@ +38,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
> +const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
> +const DESCRIPTION_ANNO = "bookmarkProperties/description";
> +const POST_DATA_ANNO = "bookmarkProperties/POSTData";

Looks like POST_DATA_ANNO is unused...
Attachment #622810 - Flags: feedback+
Attachment #622232 - Attachment description: (Av1) bookmarks.preplaces.html: Add a 'HR' → (Av1) bookmarks.preplaces.html: Add a 'HR' [Merged into next patch]
(In reply to Serge Gautherie (:sgautherie) from comment #10)
> > +const LOAD_IN_SIDEBAR_ANNO = "bookmarkProperties/loadInSidebar";
> > +const DESCRIPTION_ANNO = "bookmarkProperties/description";
> > +const POST_DATA_ANNO = "bookmarkProperties/POSTData";
> 
> Looks like POST_DATA_ANNO is unused...

ah, it's likely, I just copied the block from browser, will remove it on push.
Summary: bookmarks import ignores separators → bookmarks import ignores separators. ([SeaMonkey] "test_bookmarks_html.js | 3 == 4")
https://hg.mozilla.org/mozilla-central/rev/904364bcee0e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment on attachment 622810 [details] [diff] [review]
patch v1.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 482911
User impact if declined: We were not importing anymore separators from bookmarks.html
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): the code change in itself is limited (just adding support for <hr>), the worst thing may happen is html import breaks, but tests are there to ensure it doesn't happen.
String or UUID changes made by this patch: none
Attachment #622810 - Flags: approval-mozilla-aurora?
Attachment #622810 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Blocks: 767776
You need to log in before you can comment on or make changes to this bug.