Closed Bug 947619 Opened 7 years ago Closed 7 years ago

Tabs bookmarked in private windows have no titles

Categories

(SeaMonkey :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.26

People

(Reporter: neil, Assigned: neil)

Details

Attachments

(1 file)

Steps to reproduce problem:
1. Open some tabs in a private window
2. Bookmark group of tabs

Expected results: Bookmarks have titles

Actual result: Bookmarks have no title

This is because creating a group of bookmarks does not give you a way of specifying the titles, and tabs in private windows have no history so their titles can't be used.
Attached patch Proposed patchSplinter Review
The sync part is untested.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #8344211 - Flags: review?(bugzilla)
Attachment #8344211 - Flags: feedback?(jh)
Comment on attachment 8344211 [details] [diff] [review]
Proposed patch

AFAICS, tabs in private windows don't get synced with or without the patch. Since I don't know whether that's OK or not [I'd say yes, but don't know what you expected], I'm clearing the feedback request rather than giving f+ or f-.
Attachment #8344211 - Flags: feedback?(jh)
Comment on attachment 8344211 [details] [diff] [review]
Proposed patch

The code for the "Tabs From Other Computers" screen suggests some bookmarking capability. I don't have a profile with working sync set up to test this. The changes seem pretty mechanical though...
Attachment #8344211 - Flags: feedback?(jh)
Comment on attachment 8344211 [details] [diff] [review]
Proposed patch

(In reply to neil@parkwaycc.co.uk from comment #3)
> The code for the "Tabs From Other Computers" screen suggests some
> bookmarking capability.

Well yes there is; you can open a context menu for any tabs that appear on that list. With and without your patch, a dialog comes up then which includes the title of the tab filled in. I thought your code had the title optional in the API in such a way that it is still being tried to retrieve some other way if not provided explicitly, so I thought the change wouldn't be necessary. But I have to admit I didn't check that, so f=me either way.

NB: You're missing an "l" after "Optiona" in the comment in PlacesUIUtils.jsm.
Attachment #8344211 - Flags: feedback?(jh) → feedback+
(In reply to Jens Hatlak from comment #4)
> (In reply to comment #3)
> With and without your patch, a dialog comes up then which
> includes the title of the tab filled in.
You should notice the difference if you look at the bookmarks themselves; without the patch they may not get a title if you haven't visited the page yet.

> NB: You're missing an "l" after "Optiona" in the comment in
> PlacesUIUtils.jsm.
Thanks.
(In reply to Jens Hatlak from comment #4)
> With and without your patch, a dialog comes up then which
> includes the title of the tab filled in.
Oh, and in case it wasn't clear, this is about bookmarking groups of tabs, so you have to select multiple tabs first.
(In reply to neil@parkwaycc.co.uk from comment #5)
> You should notice the difference if you look at the bookmarks themselves;
> without the patch they may not get a title if you haven't visited the page
> yet.

So now my test case was creating a bookmark group in SM from two or more tabs provided by a FF nightly profile connected using the same Sync profile. For me, with or without the patch, the bookmarks that got created had titles as exptected.
(In reply to Jens Hatlak from comment #7)
> So now my test case was creating a bookmark group in SM from two or more
> tabs provided by a FF nightly profile connected using the same Sync profile.
> For me, with or without the patch, the bookmarks that got created had titles
> as exptected.
Well, maybe you were also syncing history and getting the titles that way or something. At least nothing's broken, which is really want I wanted to know ;-)
Hrm, somehow I cannot reproduce this problem. I open a new private window, open two tabs, click on bookmark group of tabs and then look at the bookmarks: It used the page title as bookmark title.
The problem seems to by sync-dependent (not sure why): When I switch a new profile without sync, I can reproduce the problem. In contrast when testing with my normal profile (which has sync enabled, not for history though) I cannot reproduce this.
Ah ok, now I see why this works. If you have already opened the page from the private browsing window in a normal browsing window before, then the Places code/history knows about the page title (of course? :), even in the private browsing session.
Comment on attachment 8344211 [details] [diff] [review]
Proposed patch

Wouldn't this line
+      var title = this._titles[i] || this._getURITitleFromHistory(uri);

do the wrong thing when dealing with an empty page title ("")? Actually this would set no title when we've not seen the page before and set an empty title, too, if we have seen that page before (browser history). Maybe for consistency do an explicit "var title; if (typeof this._titles[i] != 'undefined') title = this._titles[i] else title = this._getURITitleFromHistory(uri);". But then, r+ with and without an fix for that, this nit is not that important :)
Attachment #8344211 - Flags: review?(bugzilla) → review+
It's possible that we're bookmarking a page whose title hasn't loaded yet, so I thought I would keep the fallback of looking up the title in history.
Pushed comm-central changeset ff2f997e1b32.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.26
You need to log in before you can comment on or make changes to this bug.