Last Comment Bug 696163 - Remove default livemark
: Remove default livemark
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: Firefox 10
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
: 697038 (view as bug list)
Depends on: 703227
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-20 12:01 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2013-12-27 14:24 PST (History)
17 users (show)
mak77: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove default livemark (1.43 KB, patch)
2011-10-20 12:01 PDT, Jeff Muizelaar [:jrmuizel]
mak77: review-
limi: ui‑review+
Details | Diff | Splinter Review
Remove default livemark w/ test changes (3.76 KB, patch)
2011-10-28 06:34 PDT, Jeff Muizelaar [:jrmuizel]
mak77: review+
Details | Diff | Splinter Review
Remove latest_headlines from extract-bookmarks.py template (734 bytes, patch)
2011-10-28 06:49 PDT, Jeff Muizelaar [:jrmuizel]
l10n: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2011-10-20 12:01:58 PDT
Created attachment 568464 [details] [diff] [review]
Remove default livemark

The livemark service does sqlite io on the main thread like the following:

0 anonymous() ["file:///Users/jrmuizel/source/obj-x86_64-apple-darwin10.8.0/dist/Nightly.app/Contents/MacOS/components/nsLivemarkService.js":664]
    this = [object BackstagePass]
1 LM_runBatched(aUserData = null) ["file:///Users/jrmuizel/source/obj-x86_64-apple-darwin10.8.0/dist/Nightly.app/Contents/MacOS/components/nsLivemarkService.js":660]
    this = [object Object]
2 LM_replaceChildren() ["file:///Users/jrmuizel/source/obj-x86_64-apple-darwin10.8.0/dist/Nightly.app/Contents/MacOS/components/nsLivemarkService.js":667]
    self = [object Object]
    this = [object Object]
3 LLL_handleResult(aResult = [xpconnect wrapped nsIFeedResult]) ["file:///Users/jrmuizel/source/obj-x86_64-apple-darwin10.8.0/dist/Nightly.app/Contents/MacOS/components/nsLivemarkService.js":755]
    this = [object Object]
4 FP_sendResult() ["file:///Users/jrmuizel/source/obj-x86_64-apple-darwin10.8.0/dist/Nightly.app/Contents/MacOS/components/FeedProcessor.js":1368]
    this = [object Object]
5 FP_endDocument() ["file:///Users/jrmuizel/source/obj-x86_64-apple-darwin10.8.0/dist/Nightly.app/Contents/MacOS/components/FeedProcessor.js":1441]
    this = [object Object]
6 FP_onStopRequest(statusCode = 0, context = null, request = [xpconnect wrapped (nsISupports, nsIRequest, nsIChannel)]) ["file:///Users/jrmuizel/source/obj-x86_64-apple-darwin10.8.0/dist/Nightly.app/Contents/MacOS/components/FeedProcessor.js":1407]
    this = [object Object]
7 LLL_onStopRequest(aStatus = 0, aContext = null, aRequest = [xpconnect wrapped (nsISupports, nsIRequest, nsIChannel)]) ["file:///Users/jrmuizel/source/obj-x86_64-apple-darwin10.8.0/dist/Nightly.app/Contents/MacOS/components/nsLivemarkService.js":811]
    this = [object Object]

The default livemark has become pretty hard to discover now that we have hidden the bookmarks toolbar, so there probably won't be a lot of new users using it. I suggest we get rid of it so that we're not paying these IO costs for a feature that new users are unlikely to use.

It may also be worth removing livemarks all together as they are very difficult to create anyways and would be better done with an add-on.
Comment 1 Justin Dolske [:Dolske] 2011-10-20 12:33:55 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #0)

> It may also be worth removing livemarks all together as they are very
> difficult to create anyways and would be better done with an add-on.

Maybe there's already a bug on this? Sounds like a good idea, especially if it helps make Places saner. Livemarks aren't a very good way to read RSS feeds anyway.
Comment 2 Marco Bonardo [::mak] 2011-10-20 12:50:23 PDT
(In reply to Justin Dolske [:Dolske] from comment #1)
> Maybe there's already a bug on this?

bug 622049. I approve the removal of the default live bookmark here, that other bug has some more implication (as you can see from the comments there).

> Sounds like a good idea, especially if
> it helps make Places saner.

Nope, they are a really minor "problem". practically the only issue is that they are stored in the bookmarks table, once that is solved we have no issues with them.

> Livemarks aren't a very good way to read RSS
> feeds anyway.

I disagree, or better, surely they may be largely improved but we are not a RSS feed reader, we never wanted to be. We have minimal support since this has become a pretty common standard on the Web. I also still disagree with the removal of the feed icon, but as I did not impose my vision on that removal, if there's a shared will to remove the feature, I'll cope with it.
Comment 3 Alex Limi (:limi) — Firefox UX Team 2011-10-20 16:02:15 PDT
Comment on attachment 568464 [details] [diff] [review]
Remove default livemark

I have no problem with removing this. It's not visible in the default UI anymore.
Comment 4 Marco Bonardo [::mak] 2011-10-21 04:59:01 PDT
I'm quite sure this change is going to fail tests that rely on the number of bookmarks in the toolbar.
You should, at a minimum, decrement DEFAULT_BOOKMARKS_ON_TOOLBAR http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/unit/head_bookmarks.js#110 and run the result on try to catch tests in need of asjustements (most likely there may be some importExport test)
Comment 5 Marco Bonardo [::mak] 2011-10-21 05:00:34 PDT
PS: to save on resources and time you can reduce the tryrun to Linux, I think there should be no difference from other platforms in this case.
Comment 6 Axel Hecht [:Pike] 2011-10-21 09:15:06 PDT
Just to clarify, removing the default livemark from the code does't remove it from users that we got in the past few years, as that's just copied over on profile creation and never touched again.

I don't mind one item less to arbitrate about with new locales, though, clearly.
Comment 7 Kev Needham [:kev] 2011-10-24 08:21:33 PDT
Concur with removing it from the defaults, and I'll make sure there's no requirement to keep it in place. I think our agreement with the BBC is simply permission/notice of using it, because it does drive a fair bit of traffic. If we remove the link, we should notify them so their metrics team doesn't get confused.
Comment 8 Marco Bonardo [::mak] 2011-10-24 08:26:50 PDT
Comment on attachment 568464 [details] [diff] [review]
Remove default livemark

btw, this is r- for comment 4
Comment 9 Kev Needham [:kev] 2011-10-24 08:29:08 PDT
When things are good to go per comment 4, Asa should also be flagged to review for product defaults.
Comment 10 Asa Dotzler [:asa] 2011-10-24 10:56:38 PDT
No problems from my POV.
Comment 11 Axel Hecht [:Pike] 2011-10-25 03:24:51 PDT
*** Bug 697038 has been marked as a duplicate of this bug. ***
Comment 12 Jeff Muizelaar [:jrmuizel] 2011-10-26 05:24:54 PDT
(In reply to Marco Bonardo [:mak] from comment #4)
> I'm quite sure this change is going to fail tests that rely on the number of
> bookmarks in the toolbar.
> You should, at a minimum, decrement DEFAULT_BOOKMARKS_ON_TOOLBAR
> http://mxr.mozilla.org/mozilla-central/source/browser/components/places/
> tests/unit/head_bookmarks.js#110 and run the result on try to catch tests in
> need of asjustements (most likely there may be some importExport test)

Even with DEFAULT_BOOKMARKS_ON_TOOLBAR decremented, I get the following failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=7027825&tree=Try#error0

Any suggestions on how to fix it?
Comment 13 Marco Bonardo [::mak] 2011-10-26 05:36:42 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #12)
> (In reply to Marco Bonardo [:mak] from comment #4)
> Even with DEFAULT_BOOKMARKS_ON_TOOLBAR decremented, I get the following
> failure:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=7027825&tree=Try#error0
> 
> Any suggestions on how to fix it?

So the test is checking that default bookmarks are imported, this is done by checking that after smart bookmarks there is a default bookmark.
But looks like the test is doing it wrongly (or better, not that correctly):

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/unit/test_browserGlue_prefs.js#208
207       PlacesUtils.bookmarks.getIdForItemAt(PlacesUtils.toolbarFolderId,
208                                            SMART_BOOKMARKS_ON_TOOLBAR + 1);

it is checking 2 bookmarks after the smart bookmark, rather than the immediately next one. Since you removed the second default bookmark, the test complains that it can't find anything (it is looking at position 3, but now we have 2 bookmarks).
I think you should just remove that "+ 1" and everything will be fine.

It's likely you should also fix the same bug here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/unit/test_browserGlue_prefs.js#239
Comment 14 Jeff Muizelaar [:jrmuizel] 2011-10-28 06:34:23 PDT
Created attachment 570238 [details] [diff] [review]
Remove default livemark w/ test changes

This adds the test changes necessary to make this pass on try.
Comment 15 Marco Bonardo [::mak] 2011-10-28 06:43:07 PDT
Comment on attachment 570238 [details] [diff] [review]
Remove default livemark w/ test changes

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

All of this is fine from my point of view, but I still see a reference at http://mxr.mozilla.org/mozilla-central/source/browser/locales/generic/extract-bookmarks.py#71
I don't know the purpose of that script though, so please fix it, but get review from Pike on that change.
Comment 16 Jeff Muizelaar [:jrmuizel] 2011-10-28 06:49:57 PDT
Created attachment 570245 [details] [diff] [review]
Remove latest_headlines from extract-bookmarks.py template
Comment 17 Marco Bonardo [::mak] 2011-10-29 01:43:34 PDT
https://hg.mozilla.org/mozilla-central/rev/b4137b3ad5af
Comment 18 Francesco Lodolo [:flod] 2011-10-29 05:25:44 PDT
(In reply to Axel Hecht [:Pike] from comment #6)
> I don't mind one item less to arbitrate about with new locales, though,
> clearly.

How are we going to manage this in each locale considering that bookmark.inc is a productization file? Is enough to remove latest_headlines from our file and use this bug as a reference in the commit comment?
Comment 19 Axel Hecht [:Pike] 2011-10-29 17:03:09 PDT
bookmarks.inc is just the titles of the bookmarks, and hasn't undergone review ever since it was that.

The old bookmarks.html used to be under review, but just because the links themselves were in the localization.

Thus, this is just a string change as any other.
Comment 20 Francesco Lodolo [:flod] 2011-10-29 22:18:23 PDT
(In reply to Axel Hecht [:Pike] from comment #19)
> Thus, this is just a string change as any other.

Thanks Axel, faulty memory on my side ;-)

Note You need to log in before you can comment on or make changes to this bug.