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)
Toolkit
Places
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)
11.90 KB,
patch
|
sgautherie
:
feedback+
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(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 :(
Reporter | ||
Comment 1•12 years ago
|
||
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 | ||
Updated•12 years ago
|
Assignee: nobody → mak77
Comment 2•12 years ago
|
||
Looks like this could cause a fair bit of user confusion if separator is missing, tracking to make sure this gets in for 14.
Assignee | ||
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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)
Assignee | ||
Comment 5•12 years ago
|
||
moves the import tests to toolkit, adds back importing separators and tests for that.
Attachment #622689 -
Flags: review?(dietrich)
Comment 6•12 years ago
|
||
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+
Reporter | ||
Comment 7•12 years ago
|
||
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-
Assignee | ||
Comment 8•12 years ago
|
||
(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.
Assignee | ||
Comment 9•12 years ago
|
||
removed the double because, and the bogus 6-2 comment
Attachment #622689 -
Attachment is obsolete: true
Reporter | ||
Comment 10•12 years ago
|
||
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+
Reporter | ||
Updated•12 years ago
|
Attachment #622232 -
Attachment description: (Av1) bookmarks.preplaces.html: Add a 'HR' → (Av1) bookmarks.preplaces.html: Add a 'HR'
[Merged into next patch]
Assignee | ||
Comment 11•12 years ago
|
||
(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.
Reporter | ||
Updated•12 years ago
|
Summary: bookmarks import ignores separators → bookmarks import ignores separators. ([SeaMonkey] "test_bookmarks_html.js | 3 == 4")
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/904364bcee0e
Flags: in-testsuite+
Comment 13•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/904364bcee0e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•12 years ago
|
||
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?
Updated•12 years ago
|
Attachment #622810 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Comment 15•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c49bf9402c8a
Assignee | ||
Updated•12 years ago
|
status-firefox13:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•