Name field of bookmarks saved via "Bookmark All Tabs" is empty when loading the tab is deferred

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: MattN, Assigned: scottwu)

Tracking

Trunk
Firefox 50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

+++ This bug was initially created as a clone of Bug #446171 +++

If "Bookmarks - Bookmark All Tabs..." is applied to several tabs, the particular bookmarks do not get any page title. Part of the URL is shown in the bookmarks folder instead.

Reproducible: Always

Steps to Reproduce:
1. Open several tabs
2. Open http://www.mozilla.org/community/
3. Set tabs to only load when selected after startup (deferred tab loading).
4. Restart Firefox and see the titles for all the tabs are present even before loading them.
5. Select "Bookmarks => Bookmark All Tabs..."
6. Enter a Folder name, e.g., "foo"
7. Select "Bookmarks => foo"
Actual Results:  
In the subfolder "foo" of the bookmark folder, the corresponding bookmark to http://www.mozilla.org/community/ appears not as "Mozilla Community" but as "http://www.mozilla.org/community/" instead.

Expected Results:  
I expect Firefox to fill in the page title ("Mozilla Community" in the example) into the name field of each particular bookmark generated by "Bookmark All Tabs..." since the title is already present on the tabs themselves.

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:15.0) Gecko/15.0 Firefox/15.0a1
Hm, strange, the title is fetched from history, since you loaded the pages in the session 1, they should be available in session 2, unless session 1 was a PB session.
(In reply to Marco Bonardo [:mak] from comment #1)
> Hm, strange, the title is fetched from history, since you loaded the pages
> in the session 1, they should be available in session 2, unless session 1
> was a PB session.

It definitely wasn't PB but since you mention history, this was a tab group which I hadn't used in months (over 6 maybe) and so perhaps the history entries expired already?
(In reply to Matthew N. [:MattN] from comment #2)
> It definitely wasn't PB but since you mention history, this was a tab group
> which I hadn't used in months (over 6 maybe) and so perhaps the history
> entries expired already?

it's possible, and when reloading the page that way we don't add history iirc

Comment 4

2 years ago
I was wondering why Bug 446171 didn't appear to be fixed and it was due to this bug.

STR:
1. Create a session with several tabs pointing to different websites.
2. Restart browser and resume last session.
3. Bookmark All Tabs.

The bookmarks for unloaded tabs have no name value.

Updated

2 years ago
Duplicate of this bug: 1248906
(Assignee)

Comment 6

2 years ago
Created attachment 8754230 [details]
Bug 754623 - Get page title from tab label;

Review commit: https://reviewboard.mozilla.org/r/53852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53852/
(Assignee)

Comment 7

2 years ago
Comment on attachment 8754230 [details]
Bug 754623 - Get page title from tab label;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53852/diff/1-2/
(Assignee)

Comment 8

2 years ago
It appears that when the tabs are resumed, they are not loaded until focused, so the contentTitle would be empty. Seems that we can just use the label attribute from tabs to get the page titles.

Waiting for try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb795cd32e7d
(Assignee)

Updated

2 years ago
Assignee: nobody → scwwu
(Assignee)

Updated

2 years ago
Attachment #8754230 - Flags: review?(mak77)
(Assignee)

Comment 9

2 years ago
Hello Marco, to fix this bug I grabbed the tab label attribute rather than using the contentTitle. However I am seeing a lot of oranges in the try, and I'm not sure what happened there.

I rebased my patch, but the result was still the same. What I find strange is that when I pushed the same changeset from central (supposedly all passing) to try, the result returned a lot of oranges too.

My patch try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6174af828a70
My Central try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7836cb0dd09d
Rebased from: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=46fe2115d46a5bb40523b8466341d8f9a26e1bdf

Could it be the try command I put in? I used "./mach try -b do -p linux,linux64,macosx64,win32,win64 -u xpcshell,marionette,marionette-e10s,mochitests -t none".

Thank a lot!
Flags: needinfo?(mak77)
(In reply to Scott Wu [:scottwu] from comment #9)
> Hello Marco, to fix this bug I grabbed the tab label attribute rather than
> using the contentTitle. However I am seeing a lot of oranges in the try, and
> I'm not sure what happened there.

I think you're seeing https://mail.mozilla.org/pipermail/firefox-dev/2016-May/004304.html

> I rebased my patch, but the result was still the same. What I find strange
> is that when I pushed the same changeset from central (supposedly all
> passing) to try, the result returned a lot of oranges too.

Try rebase again as it should be fixed now with bug 1270962.
(Assignee)

Comment 11

2 years ago
Thanks Matthrew! I'll give it a shot.
hat about using tab.label only if contentTitle is empty? off-hand looks like contentTitle would in-general be close to what we want, the tab label could be modified by an add-on or such.
Flags: needinfo?(mak77)
(Assignee)

Comment 13

2 years ago
Comment on attachment 8754230 [details]
Bug 754623 - Get page title from tab label;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53852/diff/2-3/
Attachment #8754230 - Attachment description: MozReview Request: Bug 754623 - Get page title from tab label → Bug 754623 - Get page title from tab label
(Assignee)

Comment 14

2 years ago
Thanks Marco! I've changed tab.label to be a fallback if contentTitle is absent.

Waiting for try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=67433943caa5
Comment on attachment 8754230 [details]
Bug 754623 - Get page title from tab label;

https://reviewboard.mozilla.org/r/53852/#review55356

Thank you!
Attachment #8754230 - Flags: review?(mak77) → review+
(Assignee)

Comment 16

2 years ago
Comment on attachment 8754230 [details]
Bug 754623 - Get page title from tab label;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53852/diff/3-4/
Attachment #8754230 - Attachment description: Bug 754623 - Get page title from tab label → Bug 754623 - Get page title from tab label;
(Assignee)

Comment 17

2 years ago
Thanks a lot Marco! I've just added r=mak to the commit.
Keywords: checkin-needed

Comment 18

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/6763cbc32e3e
Get page title from tab label; r=mak
Keywords: checkin-needed

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6763cbc32e3e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.