Closed Bug 754623 Opened 12 years ago Closed 8 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: MattN, Assigned: scottwu)

References

Details

Attachments

(1 file)

+++ 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
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.
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/
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: nobody → scwwu
Attachment #8754230 - Flags: review?(mak77)
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.
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)
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
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+
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;
Thanks a lot Marco! I've just added r=mak to the commit.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/6763cbc32e3e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: