321973 - context menu "Bookmark this link..." is broken

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Phil Ringnalda (:philor)]

Currently, right clicking and trying to bookmark a link just brings up a broken and heightless dialog.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

-> annie. Annie, I think it's time we stop using the shims and start using #ifdef MOZ_PLACES. 

Comment 2

[Phil Ringnalda (:philor)]

*** Bug 329108 has been marked as a duplicate of this bug. ***

Comment 3

[Phil Ringnalda (:philor)]

*** Bug 329868 has been marked as a duplicate of this bug. ***

Comment 4

[u88484]

Same here, no bookmark but I don't have the blank dialog. I get this in the javascript console:

Error: BookmarksUtils is not defined
Source file: chrome://browser/content/browser.xul
Line: 1

Comment 5

[Phil Ringnalda (:philor)]

*** Bug 331860 has been marked as a duplicate of this bug. ***

Comment 6

[Spencer Selander [greenknight]]

When I click "Bookmark this link", absolutely nothing happens. JavaScript console shows nothing.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060330 Firefox/2.0a1 ID:2006033004

Same with my Trunk build.

Comment 7

[Peter Kasting]

I have a local fix for this

Comment 8

[Peter Kasting]

Attachment: patch v1

This patch does several things:
* Allows callers that pop up the add bookmarks dialog to provide a desired title.  If no title is provided, fall back to querying the history service.  This is the main part I'd like Joe to review.
* Shows the add bookmarks dialog for a link, with the title set to the link text.
* Adds relevant bug numbers and cleans up some unused functions for other stuff I ran across while fixing this.  These changes are basically all in browser.js.  These last two bullet points are the main things I'd like Ben to review.

Comment 9

[Joe Hughes]

Comment on attachment 235485 [details] [diff] [review]
patch v1

My main critique is that the explicit passing of undefined is a bit awkward.  It'd be better to (1) change the test to "if (param)" instead of "if (param === undefined)" and change the parameter order so that the title parameter goes last and thus can be omitted in the calls.

Comment 10

[Peter Kasting]

(In reply to comment #9)
> (From update of attachment 235485 [details] [diff] [review] [edit])
> It'd be better to (1) change the test to "if (param)" instead of "if (param ===
> undefined)"

I did this purposefully so that callers can provide an empty string as their desired title.  If I don't use "=== undefined" I can't get that behavior.

I can reorder the params if you wish; I was mainly trying to keep things logically grouped.

Comment 11

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 235485 [details] [diff] [review]
patch v1

aside from joe's comments, this looks ok to me from a sr perspective. 

sr=[e-mail] with those changes.

Comment 12

[Peter Kasting]

Attachment: patch v2

This moves the URI/title params to the end of the list so the title can be omitted in most calls.  It preserves the "=== undefined" bit, but with a comment noting why that's used.

Preserving sr+ from Ben.

Comment 13

[Joe Hughes]

Comment on attachment 235987 [details] [diff] [review]
patch v2

Good for the most part, but structurally, I think it's too much logic in the init function.  How about just always setting this._bookmarkTitle = title in the init and then surround the existing block in _populateProperties with the (this._bookmarkTitle === undefined) test.  (You'd also want to change the initial declaration of this._bookmarkTitle for consistency.)

Comment 14

[Peter Kasting]

Attachment: patch v3

Like this, then?

Comment 15

[Joe Hughes]

Comment on attachment 236337 [details] [diff] [review]
patch v3

Yep!

Comment 16

[Peter Kasting]

Checked in.

/mozilla/browser/base/content/browser-context.inc 1.26
/mozilla/browser/base/content/browser-places.js 1.9
/mozilla/browser/base/content/browser-sets.inc 1.81
/mozilla/browser/base/content/browser.js 1.703
/mozilla/browser/components/places/content/bookmarkProperties.js 1.23
/mozilla/browser/components/places/content/bookmarkProperties.xul 1.14
/mozilla/browser/components/places/content/controller.js 1.97

Clearing blocking request.

Comment 17

[Peter Kasting]

Followup fix for small copy-and-paste error:
/browser/components/places/content/bookmarkProperties.js 1.24

Comment 18

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv