Bookmarks Manager prevents bookmarking of other tabs

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Chris Lawson (gone), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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.
Version: Trunk → unspecified
(Assignee)

Comment 1

11 years ago
I broke this in bug 188679.  Taking.
Assignee: nobody → stridey
(Assignee)

Comment 2

11 years ago
Created attachment 231900 [details] [diff] [review]
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.
Attachment #231900 - Flags: review?(stuart.morgan)

Comment 3

11 years ago
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.
Attachment #231900 - Flags: review?(stuart.morgan) → review-
(Assignee)

Updated

11 years ago
Blocks: 302614
(Assignee)

Comment 4

11 years ago
Created attachment 231911 [details] [diff] [review]
Gets rid of check entirely

Rips out mentioned code, and the method description (since it's now self-explanatory)
Attachment #231900 - Attachment is obsolete: true
Attachment #231911 - Flags: review?(stuart.morgan)

Updated

11 years ago
Attachment #231911 - Flags: review?(stuart.morgan) → review-

Comment 5

11 years ago
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.
(Assignee)

Comment 6

11 years ago
Created attachment 232278 [details] [diff] [review]
Slims it down

Yeah, I wasn't sure if assigning outparams directly was acceptable practice or not.
Attachment #231911 - Attachment is obsolete: true
Attachment #232278 - Flags: review?(stuart.morgan)
(Assignee)

Comment 7

11 years ago
*** Bug 347801 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 8

11 years ago
Created attachment 233427 [details] [diff] [review]
Super slims it down

per irc, getting rid of the method entirely.
Attachment #232278 - Attachment is obsolete: true
Attachment #233427 - Flags: review?(stuart.morgan)
Attachment #232278 - Flags: review?(stuart.morgan)
Putting this regression on the 1.1 list so it doesn't get lost (even though it'll hopefully be fixed soon).
Target Milestone: --- → Camino1.1

Comment 10

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

r=me
Attachment #233427 - Flags: superreview?(mikepinkerton)
Attachment #233427 - Flags: review?(stuart.morgan)
Attachment #233427 - Flags: review+
Comment on attachment 233427 [details] [diff] [review]
Super slims it down

sr=pink

i heart fixing bugs by removing code.
Attachment #233427 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [needs checkin]

Comment 12

11 years ago
Checked into trunk and MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: regression → fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.