Migrate https://searchfox.org/mozilla-central/source/browser/base/content/browser.xhtml#39-73 to Fluent.

title_normal got introduced in bug 411929, and doesn't seem to be in use at all anymore.

Relevant C++ bits:

• https://searchfox.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#851-876
• https://searchfox.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#619-634

JS:

• https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#958-981

Gijs, Mossop - you may be able to help me understand how much of that I can actually cut out, and what bits I need to preserve.

The code that manipulates those bits in C++ and JS dates back to 2000, but some bits are updated as recently as 2013, so you know, hot stuff.

Can we simplify this?

I'm kind of afraid of touching it because in no world will I be able to test the possible permutations, but I'd like to try to limit the entanglement.

(I'd like to try to simplify the FTL as well, but first, simplify the XML+JS+CPP logic)

Ugh. This.

Are any of those C++ bits still used? From what I can tell neither of them are used unless you have a "contenttitlesetting" attribute on the window element, which only the layout debugger has. Looks to me like only the tabbrowser.js code matters and looks likely that that can be substantially tidied up by using a single fluent string with some arguments.

David - can you help us with the layout debugger contenttitlesetting? Is it necessary to maintain that?

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #8)

David - can you help us with the layout debugger contenttitlesetting? Is it necessary to maintain that?

Probably not necessary to maintain it. It was probably copied from whatever the Firefox browser.xul or some other XUL window did in 2003. As far as I can tell, today, the layout debugger's window title is just "Layout Debugger" and it doesn't appear to reflect the page's <title> (although I suspect it may have in the past).

Thanks! Filled bug 1591570 to remove that.

Mossop - bug 1591570 removes the "contenttitlesetting", but I think https://searchfox.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#619-634 is still possible in this scenario: https://searchfox.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#827

Can I remove that as well?

Ah, Mossop pointed out in the review that the method does in fact become a no-op. The Runnable is separate.

I think between other people's comments here and those on phab including mine, there's nothing left for me to add here, but please re-needinfo if I missed something.

Dave - is that also something we can remove now - https://searchfox.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#738-756 ?

I started moving the code to use your proposal from phabricator, and currently I have

  # This is the default window title in case where there is no custom
  # title to be displayed.
  #
  # Depending on the $mode, the string will look like this (in en-US):
  #
  # "default" - "Mozilla Firefox"
  # "private" - "Mozilla Firefox (Private Browsing)"
  #
  # Variables
  #   $mode (String) - "private" in case of a private browsing mode, "default" otherwise.
  main-window =
      .title = { $mode ->
          [private] { -brand-full-name } (Private Browsing)
         *[other] { -brand-full-name }
      }

  # This is the default window title in case where there is a custom
  # title to be displayed.
  #
  # Depending on the $mode, the string will look like this (in en-US):
  #
  # "default" - "Example Title - Mozilla Firefox"
  # "private" - "Example Title - Mozilla Firefox (Private Browsing)"
  #
  # Variables
  #   $mode (String) - "private" in case of a private browsing mode, "default" otherwise.
  main-window-content-title =
      .title = { $mode ->
          [private] { $title } - { -brand-full-name } (Private Browsing)
         *[other] { $title } - { -brand-full-name }
      }

Does it look like what you envisioned?

I'm now trying to figure out how to refactor https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#955 to fit the new model.

This function has two callers, one sets document.title directly, and the other assigns preview.title and preview.tooltip.

Assuming the document in the first case is the browser.xhtml window document, we could morph it into sth like:

function setCustomTitle(title) {
  ... logic around selecting the right title and filtering it.

   document.l10n.setAttributes(document.documentElement, "main-window-content-title", { title });
}

but I'm not sure what to do with the latter. What do you think?

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #15)

Does it look like what you envisioned?

Yep.

I'm now trying to figure out how to refactor https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#955 to fit the new model.

This function has two callers, one sets document.title directly, and the other assigns preview.title and preview.tooltip.

Assuming the document in the first case is the browser.xhtml window document, we could morph it into sth like:

function setCustomTitle(title) {
  ... logic around selecting the right title and filtering it.

   document.l10n.setAttributes(document.documentElement, "main-window-content-title", { title });
}

but I'm not sure what to do with the latter. What do you think?

I think we make getWindowTitleForBrowser return an object with properties that indicate the FTL identifier and the attributes, and the document title setter can call document.l10n.setAttributes(), and the preview code will need to call formatValue.

The latter is a bit annoying because the current call is sync. I don't know what to do about that - you'll need to test on Windows (possibly even win7, I dunno if we expose the title on win10 off-hand), with the windows previews-per-tab enabled (it's a checkbox under "Tabs" in the prefs). It may not make much difference if we set the title async. Though we'll need to somehow ensure that the latest title "wins" and/or that things are sane if the preview object goes away or becomes invalid between the point where we get the l10n info and where we try to set the property based on the result of formatValue.

Alternatively, for the preview code, I think we could just hardcode using the tab content title (or the brand name (which we can safely cache) if there's no title at all), which would remove the need for l10n to be used for that path at all.

(In reply to Zibi Braniecki [:zbraniecki][:gandalf] from comment #14)

Dave - is that also something we can remove now - https://searchfox.org/mozilla-central/source/xpfe/appshell/nsContentTreeOwner.cpp#738-756 ?

I don't see any callers, so yeah!

Ok, here's the full patch. I'm going to test it against Windows tomorrow

Depends on D53775

Green try https://treeherder.mozilla.org/#/jobs?repo=try&revision=f497ac6a2229097121504b3159b18085457f211e&selectedJob=276892873 on top of bug 1595925.

https://hg.mozilla.org/mozilla-central/rev/0bcc6c8444db
https://hg.mozilla.org/mozilla-central/rev/bff9eef8191f

https://hg.mozilla.org/mozilla-central/rev/475b4e9b25fc

Backed out 3 changesets (bug 1591328) for causing Bug 1602808

https://hg.mozilla.org/releases/mozilla-beta/rev/ebe06d4050d9c073aebc01e894c906c9b3e51230

This is still present in 73.

Backout improvement
== Change summary for alert #24517 (as of Wed, 18 Dec 2019 13:17:10 GMT) ==

Improvements:

6% tabswitch windows10-64-shippable-qr opt e10s stylo 7.35 -> 6.93
4% tabswitch windows10-64-shippable opt e10s stylo 9.92 -> 9.50

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24517

Marian, this was the regression landed initially on autoland; merged to central and then backed out on mozilla-beta. Please keep an eye on this, as the improvement should appear also on autoland.

(In reply to Alexandru Ionescu :alexandrui from comment #30)

Backout improvement
== Change summary for alert #24517 (as of Wed, 18 Dec 2019 13:17:10 GMT) ==

Improvements:

6% tabswitch windows10-64-shippable-qr opt e10s stylo 7.35 -> 6.93
4% tabswitch windows10-64-shippable opt e10s stylo 9.92 -> 9.50

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=24517

Marian, this was the regression landed initially on autoland; merged to central and then backed out on mozilla-beta. Please keep an eye on this, as the improvement should appear also on autoland.

I'm confused; it won't appear on autoland, we deliberately backed this out of beta only, because of functional and performance regressions. The regression is staying on central until we fix it in bug 1602808...

Thanks for clarifying this, Gijs. AFAIK, there's no workflow of backing up on the same repo the regression was introduced, so it's a bit difficult to understand the entire context everytime.
Marian, no need to follow-up on this.

I keep not being able to find bug 1626842 from here, so marking see also.