347111 - Bookmarks Manager prevents bookmarking of other tabs

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Chris Lawson (gone)]

1) Open any URL.
2) Open a new tab with the Bookmarks Manager in it.
3) Switch to the other tab (with WWW content in it).
4) Cmd-D.
5) Wonder why the Bookmarks menu just flashed and nothing else happened.
6) Close the tab containing the Bookmarks Manager.
7) Repeat step 4 and marvel at the restored functionality.
8) Profit!!!

I haven't checked branch yet but I'm fairly certain this worked at one point, so I'm labeling it a regression for now.

Comment 1

[froodian (Ian Leue)]

I broke this in bug 188679.  Taking.

Comment 2

[froodian (Ian Leue)]

Attachment: Patch

The problem here is that we were writing over the outparams with nil, even though they had useful information when coming from |bookmarkableTitle| (though they should be overwritten if it's not nil).  Simply not writing over them if we don't find anything is more elegant than checking whether the results were nil in |bookmarkableTitle|, so that's what this does.

Comment 3

[Stuart Morgan]

Comment on attachment 231900 [details] [diff] [review]
Patch

r-, per IRC discussions. The right place to bullet-proof this would be in code that will be ripped out to fix bug 302614, so just do that.

Comment 4

[froodian (Ian Leue)]

Attachment: Gets rid of check entirely

Rips out mentioned code, and the method description (since it's now self-explanatory)

Comment 5

[Stuart Morgan]

Comment on attachment 231911 [details] [diff] [review]
Gets rid of check entirely

(oops, missed the text field).

You may as well remove curTitle and curURL, since they don't serve a purpose.

Comment 6

[froodian (Ian Leue)]

Attachment: Slims it down

Yeah, I wasn't sure if assigning outparams directly was acceptable practice or not.

Comment 7

[froodian (Ian Leue)]

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

Comment 8

[froodian (Ian Leue)]

Attachment: Super slims it down

per irc, getting rid of the method entirely.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Putting this regression on the 1.1 list so it doesn't get lost (even though it'll hopefully be fixed soon).

Comment 10

[Stuart Morgan]

Comment on attachment 233427 [details] [diff] [review]
Super slims it down

r=me

Comment 11

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 233427 [details] [diff] [review]
Super slims it down

sr=pink

i heart fixing bugs by removing code.

Comment 12

[Nick Kreeger]

Checked into trunk and MOZILLA_1_8_BRANCH