Closed Bug 696163 Opened 13 years ago Closed 13 years ago

Remove default livemark

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch Remove default livemark (obsolete) — Splinter Review
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.
Attachment #568464 - Flags: ui-review?(limi)
Attachment #568464 - Flags: review?
Component: General → Bookmarks & History
QA Contact: general → bookmarks
OS: Mac OS X → All
Hardware: x86 → All
(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.
(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 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.
Attachment #568464 - Flags: ui-review?(limi) → ui-review+
Attachment #568464 - Flags: review? → review?(gavin.sharp)
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)
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.
Assignee: nobody → jmuizelaar
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.
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 on attachment 568464 [details] [diff] [review]
Remove default livemark

btw, this is r- for comment 4
Attachment #568464 - Flags: review-
When things are good to go per comment 4, Asa should also be flagged to review for product defaults.
No problems from my POV.
Attachment #568464 - Flags: review?(gavin.sharp)
(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?
(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
This adds the test changes necessary to make this pass on try.
Attachment #568464 - Attachment is obsolete: true
Attachment #570238 - Flags: review?(mak77)
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.
Attachment #570238 - Flags: review?(mak77) → review+
Attachment #570245 - Flags: review?(l10n) → review+
https://hg.mozilla.org/mozilla-central/rev/b4137b3ad5af
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
(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?
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.
(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 ;-)
You need to log in before you can comment on or make changes to this bug.