515435 - Remove "Get Bookmark Add-ons" from default bookmarks

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given by bug 498320 Beltzner pointed out that we should remove this entry from the default bookmarks.

Comment 1

[u88484]

I can create a patch if this is still wanted.

Comment 2

[Dietrich Ayala (:dietrich)]

Thanks Kurt, yes that'd be great!

Comment 3

[u88484]

Attachment: Patch

I didn't update the two tests because they state " <!-- This is an automatically generated file. It will be read and overwritten. DO NOT EDIT! -->"  I wasn't sure if that meant locally or not.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i guess this code could go away
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsPlacesImportExportService.cpp#1185

also the  " <!-- This is an
automatically generated file. It will be read and overwritten. DO NOT EDIT!
-->" should be part of any bookmarks.html, thus it's not the test file that is generated automatically, but any bookmarks.html.
I think test files should be edited and tests run to check they are passing.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 441203 [details]
Patch

Can you run this through the tryserver? I'm betting some unit tests in /browser depend on this being there, and will need updating.

Which are the tests you're referring to?

And please request review from l10n@moz as well, if the test run looks good.

Comment 6

[u88484]

Attachment: Patch v2

(In reply to comment #4)
> i guess this code could go away
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsPlacesImportExportService.cpp#1185
> 

Done

> also the  " <!-- This is an
> automatically generated file. It will be read and overwritten. DO NOT EDIT!
> -->" should be part of any bookmarks.html, thus it's not the test file that is
> generated automatically, but any bookmarks.html.
> I think test files should be edited and tests run to check they are passing.

Done.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i did a first push to the tryserver.
actually i think i have to ask KaiRo if Seamonkey could still need the <hr> removal code.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yeah there is some test in need of updating

TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_384370.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_384370.js | 2 == 4 - See following stack:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_457441-import-export-corrupt-bookmarks-html.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_457441-import-export-corrupt-bookmarks-html.js | 2 == 4 - See following stack:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_bookmarks_html.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_bookmarks_html.js | 2 == 4 - See following stack:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_browserGlue_smartBookmarks.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_browserGlue_smartBookmarks.js | 5 == 6 - See following stack:

Comment 9

[Robert Kaiser]

(In reply to comment #7)
> actually i think i have to ask KaiRo if Seamonkey could still need the <hr>
> removal code.

The default SeaMonkey 2.0 bookmarks.html files only have one single separator in the whole file, and that one is at the end of the list (I guess to separate personally added bookmarks from the defaults). I haven't checked older versions, but our prime interest is that import from SeaMonkey 2.0 works nicely once we introduce places-driven bookmarks in the 2.1 (1.9.3-based) series.

Comment 10

[u88484]

Attachment: Patch v3

(In reply to comment #8)
> yeah there is some test in need of updating
> 

Updated tests.  Thanks for your help!

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

looks goot at first glance, will push to try along the day.

Comment 12

[Axel Hecht [:Pike]]

Comment on attachment 441496 [details]
Patch v3

looks ok from an l10n pov.

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 441496 [details]
Patch v3

still some error:

TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_384370.js | test failed (with xpcshell return code: 3), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | ReferenceError: countFolderChildren is not defined - See following stack:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_457441-import-export-corrupt-bookmarks-html.js | test failed (with xpcshell return code: 3), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | ReferenceError: countFolderChildren is not defined - See following stack:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_bookmarks_html.js | test failed (with xpcshell return code: 3), see following log:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | ReferenceError: countFolderChildren is not defined - See following stack:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_browserGlue_smartBookmarks.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/sendchange-linux-unittest/mozilla/objdir/_tests/xpcshell/test_browser_places/unit/test_browserGlue_smartBookmarks.js | 5 == 4 - See following stack:

>diff --git a/browser/components/places/tests/unit/test_384370.js b/browser/components/places/tests/unit/test_384370.js
>--- a/browser/components/places/tests/unit/test_384370.js
>+++ b/browser/components/places/tests/unit/test_384370.js
>@@ -156,17 +156,18 @@ function testCanonicalBookmarks() {
>   var query = PlacesUtils.history.getNewQuery();
>   query.setFolders([PlacesUtils.bookmarks.bookmarksMenuFolder], 1);
>   var result = PlacesUtils.history.executeQuery(query, PlacesUtils.history.getNewQueryOptions());
>   var rootNode = result.root;
>   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, 4);
>+  do_check_eq(countFolderChildren(bs.bookmarksMenuFolder),
>+              DEFAULT_BOOKMARKS_ON_MENU + 1);

hm, i'm unsure why you changed this to countFolderChildren(), that method does not exist, you should not change rootNode.childCount

>diff --git a/browser/components/places/tests/unit/test_457441-import-export-corrupt-bookmarks-html.js b/browser/components/places/tests/unit/test_457441-import-export-corrupt-bookmarks-html.js
>-  do_check_eq(rootNode.childCount, 4);
>+  do_check_eq(countFolderChildren(bs.bookmarksMenuFolder),
>+              DEFAULT_BOOKMARKS_ON_MENU + 1);

same

>diff --git a/browser/components/places/tests/unit/test_bookmarks_html.js b/browser/components/places/tests/unit/test_bookmarks_html.js

>-  do_check_eq(rootNode.childCount, 4);
>+  do_check_eq(countFolderChildren(bs.bookmarksMenuFolder),
>+              DEFAULT_BOOKMARKS_ON_MENU + 1);

same

About smartBookmarks.js failure, looks like now there are 2 separators before Mozilla Firefox folder, one is the separator that is after Toolbar folder in the bookmarks.inc file, the other one is the separator created by smart bookmarks.

Since FX2 support has been dropped i guess we don't need anymore the <HR> separating toolbar from firefox-heading in bookmarks.html.in and we must remove it

->     <HR>
       <DT><H3 ID="rdf:#$ZvPhC3">@firefox_heading@</H3>

also this code in browserGlue should be smarter:
http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#1175

1173         if (smartBookmarksCurrentVersion == 0 &&
1174             smartBookmarkItemIds.length == 0)
1175           bmsvc.insertSeparator(bmsvc.bookmarksMenuFolder, bookmarksMenuIndex);

should be:

if (smartBookmarksCurrentVersion == 0 &&
    smartBookmarkItemIds.length == 0) {
  let id = bmsvc.getIdForItemAt(bmsvc.bookmarksMenuFolder,
                                bookmarksMenuIndex);
  // Don't add a separator if the menu was empty or there is one already.
  if (id != -1 && bmsvc.getItemType(id) != bmsvc.TYPE_SEPARATOR)
    bmsvc.insertSeparator(bmsvc.bookmarksMenuFolder, bookmarksMenuIndex);
}

Comment 14

[u88484]

Attachment: Patch v4

Addresses review comments

Comment 15

[u88484]

Attachment: Patch v5

Addresses reviews.

Last patch had tabs and a extra white line

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 441809 [details] [diff] [review]
Patch v5

>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js

>+            smartBookmarkItemIds.length == 0) {
>+          let id = bmsvc.getIdForItemAt(bmsvc.bookmarksMenuFolder,
>+                                        boomkarksMenuIndex);
>+        // Don't add a separator if the menu was empty or there is one already

wrong indentation, end comment with a period please.

>diff --git a/browser/components/places/tests/unit/test_384370.js b/browser/components/places/tests/unit/test_384370.js
>--- a/browser/components/places/tests/unit/test_384370.js
>+++ b/browser/components/places/tests/unit/test_384370.js
>@@ -155,17 +155,18 @@ function testCanonicalBookmarks() {
>   var query = PlacesUtils.history.getNewQuery();
>   query.setFolders([PlacesUtils.bookmarks.bookmarksMenuFolder], 1);
>   var result = PlacesUtils.history.executeQuery(query, PlacesUtils.history.getNewQueryOptions());
>   var rootNode = result.root;
>   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, 4);
>+  do_check_eq(rootNode.childCount(bs.bookmarksMenuFolder),

this should only be rootNode.childCount, it's a property not a method
so, it does not need to be changed from what it was
same for all tests

Comment 17

[u88484]

Attachment: Patch v6

Comment 18

[u88484]

Attachment: Patch v7

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 441831 [details] [diff] [review]
Patch v7

looks good, passes tests locally.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

there is a typo
boomkarksMenuIndex should be bookmarksMenuIndex

i've fixed this locally, and i'll take care of pushing.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v8

fixed typo.

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/95a6812fa815

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a6pre) Gecko/20100620 Minefield/3.7a6pre

Litmus test which has been updated:
https://litmus.mozilla.org/show_test.cgi?id=10033