Last Comment Bug 736476 - Title page for New tab is about:newtab after restart
: Title page for New tab is about:newtab after restart
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: 13 Branch
: All All
: -- normal (vote)
: Firefox 14
Assigned To: Bellindira Castillo [:bellindira]
:
:
Mentors:
Depends on: 745712
Blocks: 455553
  Show dependency treegraph
 
Reported: 2012-03-16 08:30 PDT by Virgil Dicu [:virgil] [QA]
Modified: 2012-04-24 08:10 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
verified


Attachments
Patch (1.27 KB, patch)
2012-04-12 18:40 PDT, Bellindira Castillo [:bellindira]
no flags Details | Diff | Splinter Review
Changed the default namespace to "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" (1.70 KB, patch)
2012-04-13 21:10 PDT, Bellindira Castillo [:bellindira]
no flags Details | Diff | Splinter Review
v3 (806 bytes, patch)
2012-04-19 02:04 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (1.07 KB, patch)
2012-04-19 02:14 PDT, Raymond Lee [:raymondlee]
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Virgil Dicu [:virgil] [QA] 2012-03-16 08:30:04 PDT
Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120315 Firefox/13.0a2

1. Start firefox.
2. Select "show windows from last time" and "don't load tabs until selected".
3. Open two new tabs.
4. Restart.

Actual results: "about:newtab" in the title page instead of "New tab" for the two  tabs opened at step 3.
Expected: "new tab" in the title page. remember the title name before shutdown, as any regular tab or as about:blank
Comment 1 Bellindira Castillo [:bellindira] 2012-04-12 18:40:14 PDT
Created attachment 614649 [details] [diff] [review]
Patch

Applied the same logic used for about:blank to set title page.
Comment 2 Tim Taubert [:ttaubert] 2012-04-13 03:56:28 PDT
Comment on attachment 614649 [details] [diff] [review]
Patch

Review of attachment 614649 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for tackling this, Bellindira. I had a look at it and actually we shouldn't need to hard-code this in the session store service. The tab has a title and should be saved no matter what the actual URL behind this is:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1989

Interestingly, applying this little change fixes the problem (but yields other side-effects and shouldn't be needed):

> diff --git a/browser/base/content/newtab/newTab.xul b/browser/base/content/newtab/newTab.xul
> --- a/browser/base/content/newtab/newTab.xul
> +++ b/browser/base/content/newtab/newTab.xul
>  
>  <xul:window id="newtab-window" xmlns="http://www.w3.org/1999/xhtml"
>              xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> -            xul:disablefastfind="true" xul:title="&newtab.pageTitle;">
> +            xul:disablefastfind="true" title="&newtab.pageTitle;">

I'm not exactly sure why using html:title="" is any different from using xul:title="" but hopefully this helps as a little hint.
Comment 3 Bellindira Castillo [:bellindira] 2012-04-13 08:28:20 PDT
Thanks! I'll check what's the difference because my first approach was trying to save all titles however in the aEntry parameter in  _serializeHistoryEntry, title was always empty on xul pages cases.
Comment 4 Bellindira Castillo [:bellindira] 2012-04-13 21:10:25 PDT
Created attachment 614999 [details] [diff] [review]
Changed the default namespace to "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

The title is set using the default namespace.
Comment 5 Tim Taubert [:ttaubert] 2012-04-16 03:46:56 PDT
Ok, so I took a look at this again and the problem lies here:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#5205

This reads only "title" attributes without a namespace from XUL elements. So it doesn't read "xul:title". According to http://www.w3.org/TR/REC-xml-names/#defaulting

"A default namespace declaration applies to all unprefixed element names within its scope. Default namespace declarations do not apply directly to attribute names; the interpretation of unprefixed attributes is determined by the element on which they appear."

That means we don't really need the "xul:" prefix for attributes on the #newtab-window element.
Comment 6 Tim Taubert [:ttaubert] 2012-04-16 03:49:21 PDT
Comment on attachment 614999 [details] [diff] [review]
Changed the default namespace to "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

Review of attachment 614999 [details] [diff] [review]:
-----------------------------------------------------------------

After investigating this a bit further I think we should remove the "xul:" prefix from attributes of the #newtab-window element. Additionally, we need to add |title=" "| to the #newtab-scrollbox element or else we'd introduce a tooltip that is shown all the time no matter where the cursor is on the newtab page.
Comment 7 Dão Gottwald [:dao] 2012-04-16 03:57:42 PDT
(In reply to Tim Taubert [:ttaubert] from comment #6)
> After investigating this a bit further I think we should remove the "xul:"
> prefix from attributes of the #newtab-window element. Additionally, we need
> to add |title=" "| to the #newtab-scrollbox element or else we'd introduce a
> tooltip that is shown all the time no matter where the cursor is on the
> newtab page.

What you suggest is obviously a hack that shouldn't be needed, because the root element shouldn't get a tooltip in the first place.
Comment 8 Tim Taubert [:ttaubert] 2012-04-16 04:25:48 PDT
(In reply to Dão Gottwald [:dao] from comment #7)
> What you suggest is obviously a hack that shouldn't be needed, because the
> root element shouldn't get a tooltip in the first place.

Indeed. Let's fix this in another bug blocking this one.

(In reply to Tim Taubert [:ttaubert] from comment #6)
> Additionally, we need to add |title=" "| to the #newtab-scrollbox element

So, Bellindira, please don't do this. I'll file a bug.
Comment 9 Tim Taubert [:ttaubert] 2012-04-19 01:32:15 PDT
Hey, Bellindira, can you please attach a patch that removes the "xul:" prefixes from the <xul:window>'s attributes? That's an easy fix we should include in Fx 14. Thanks!
Comment 10 Raymond Lee [:raymondlee] 2012-04-19 02:04:18 PDT
Created attachment 616497 [details] [diff] [review]
v3

Removed the xul: for attributes
Comment 11 Tim Taubert [:ttaubert] 2012-04-19 02:11:17 PDT
Comment on attachment 616497 [details] [diff] [review]
v3

Review of attachment 616497 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Comment 12 Raymond Lee [:raymondlee] 2012-04-19 02:14:42 PDT
Created attachment 616500 [details] [diff] [review]
Patch for checkin
Comment 13 Tim Taubert [:ttaubert] 2012-04-19 02:18:24 PDT
Comment on attachment 616500 [details] [diff] [review]
Patch for checkin

[Approval Request Comment]
User impact if declined: having newtab loaded and restarting the browser causes the title of those tabs to show as 'about:newtab'. it's a small glitch but also a very small fix.
Risk to taking this patch (and alternatives if risky): very small risk and patch
String changes made by this patch: none

We need bug 745712 approved as well, as a prerequisite for this patch.
Comment 14 Tim Taubert [:ttaubert] 2012-04-19 02:24:41 PDT
(In reply to Tim Taubert [:ttaubert] from comment #13)
> User impact if declined: having newtab loaded and restarting the browser
> causes the title of those tabs to show as 'about:newtab'.

I wanted to say: if you open some new tabs and restart the browser those will be restored. Instead of having the correct title 'New Tab' they'll have their url shown (about:newtab).

Additionally, session restore is now enabled by default which is why I think we should backport this to Aurora.
Comment 15 Tim Taubert [:ttaubert] 2012-04-19 02:25:05 PDT
https://hg.mozilla.org/integration/fx-team/rev/fe5bf2fca92e
Comment 16 Tim Taubert [:ttaubert] 2012-04-20 01:58:04 PDT
https://hg.mozilla.org/mozilla-central/rev/fe5bf2fca92e
Comment 17 Alex Keybl [:akeybl] 2012-04-20 16:01:44 PDT
Comment on attachment 616500 [details] [diff] [review]
Patch for checkin

[Triage Comment]
Approved for Aurora 13.
Comment 18 Tim Taubert [:ttaubert] 2012-04-20 23:27:59 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/757b166b71fc
Comment 19 Virgil Dicu [:virgil] [QA] 2012-04-24 08:10:32 PDT
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120424 Firefox/13.0a2
Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120424 Firefox/14.0a1

Verified on mac OS 10.6, Ubuntu 11.10 and Windows 7 with the steps in comment 0 on Firefox 13 Aurora and Nightly 14.

Note You need to log in before you can comment on or make changes to this bug.