1501886 - Migrate browser-menubar to Fluent

Status: RESOLVED FIXED

(Firefox :: Menus, enhancement, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

As part of bug 1501881 we'll want to migrate the browser-menubar.inc to Fluent.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

the idea here is to introduce a lazy JSM component in toolkit, which will get constructed at the startup with the root element and the path to JSON file with the data.

Then, on MacOS we would initialized right after the browser is interactive, and on Windows/Linux by default we'd only initialize it when the user unhides the toolbar.

The initialization will trigger an async I/O for the JSON which will then get populated to DOM or sent to MacOS.


For live changes, I'd like to allow users to register promises to be resolved when the menu is initialized before the content is dumped into DOM, and some event to be registered every time the menu changes (to allow code to inspect it if needed).

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

In line with bug 1501881, I'm considering first moving the current DOM-driven data structure to Fluent but localize it lazily after first paint.

Initial talos numbers look promising: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=87c3839e6336&newProject=try&newRevision=cf3eda5ae8bc&framework=1

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Since you're the triage owner, I assume it's your call whether we can approach this strategy. I haven't yet test it on mac, but I did select the "View>Toolbars>Menu Bar" checkbox and restarted and the browser loaded normally with the top menubar appearing after the newtab is loaded.

I can see us going in one of the three directions:
 - make us eagerly localize it if the menu is visible by default, otherwise lazily
 - accept that it shows up late both in visible and hidden scenario
 - delay transitioning to Fluent until performance is on par (bug 1441035)

I'd like to verify that you're ok with me pursuing this direction before I invest more time in writing the full patch.

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Screencast_11-01-2018_03:38:04 PM.webm

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: mac os with lazy menubar

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

The triage owner isn't the same as the submodule owner, so it's not really "my call". It's likely a mix of UX and Engineering people that should make this call (tradeoffs on user experience, performance, maintainability, etc).

I doubt we would be fine with "accept that it shows up late both in visible and hidden scenario" since we've done a fair amount of work to reduce the various number of paints during startup.

So I see us going with one of: a. "make us eagerly localize it if the menu is visible by default, otherwise lazily" or b. "delay transitioning to Fluent until performance is on par (bug 1441035)". I'm guessing eventually we could do both a. and b.

Stephen and Florian would be good people from both orgs to get feedback from, as they have both spent a good amount of time looking at startup.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

After looking at the other comments from bug 1501881 I'm okay with the menubar text appearing late as long as it's not shifting content around in position. Also, I think Mike's feedback on bug 1501881 is sufficient so I'll cancel Florian's needinfo.

Comment 8

[Stephen Horlander [:shorlander]]

(In reply to Jared Wein [:jaws] (Regression Engineering Owner for 65) (please needinfo? me) from comment #6)
> The triage owner isn't the same as the submodule owner, so it's not really
> "my call". It's likely a mix of UX and Engineering people that should make
> this call (tradeoffs on user experience, performance, maintainability, etc).
> 
> I doubt we would be fine with "accept that it shows up late both in visible
> and hidden scenario" since we've done a fair amount of work to reduce the
> various number of paints during startup.

A lot of the work around improving perceived performance has been in showing primary UI surfaces as soon as possible. This has mostly focused on the main window, but this would also apply to a visible menubar.

The video of the lag on the macOS menubar is not a regression we should take. 

Lazy loading for a hidden menubar is probably fine since it's not visible at startup.

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1501886 - Move menubar localization to happen at idle.

Depends on D7591

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1501886 - Migrate browser menubar to Fluent.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

> A lot of the work around improving perceived performance has been in showing primary UI surfaces as soon as possible. This has mostly focused on the main window, but this would also apply to a visible menubar.
> The video of the lag on the macOS menubar is not a regression we should take. 

Just to double check. This means that if we have a tradeoff (not necessarily in this bug, just in general in my area of work) where l10n takes 50ms, and we can show the UI without strings 50ms earlier, or 50ms later with strings, the better perceived performance is to always go for the latter?

Comment 12

[Zibi Braniecki [:zbraniecki][:gandalf]]

Perf regression as of today - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=861ac65a0a24c1c2fdf662cd423bfa7803b60052&newProject=try&newRevision=8fe6a0d871760230065cc5184300d7b9f1dc12e5&framework=1

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

It seems that bug 1552714 fixed it for us.

Single FTL string vs central - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c1a9623c2c56f38cfcef106d2a41b1c8fa058f3d&newProject=try&newRevision=2b5aaba72a5856b507b5741faf4c4ac2ce28ec98
Menubar migrated vs. central - https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=c1a9623c2c56f38cfcef106d2a41b1c8fa058f3d&newProject=try&newRevision=e71d091aa4b5ec691a50b8bdc13fe34b6b310377

I'm open to continue exploring bug 1441035 for potential gains, but I'd like to first migrate this.

Comment 14

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1501886 - Migrate menubar to Fluent.

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1501886 - Migrate menubar to Fluent.

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

With the landing of bug 1517880 this patch now has no talos impact: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a3a6c44eef1c9e2c5b66a787fab860e4122815b9&newProject=try&newRevision=374c6906f4166581ebd8e6942b40f25c25d0e40e

We can finalize and land it!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a06c08bf267
Migrate menubar to Fluent. r=fluent-reviewers,Pike,flod

Comment 18

[Andreea Pavel [:apavel]]

Backed out for failing bc at browser_window_menu_list.js and browser_bookmarkMenu_hiddenWindow.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=7a06c08bf26762fabba8806bb87c2ef31ee62510&selectedJob=259503275

Failure logs: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=259503275&repo=autoland&lineNumber=9909
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=259490689&repo=autoland&lineNumber=1095

Backout: https://hg.mozilla.org/integration/autoland/rev/22bda3da7ce3439a1ccafce4753ad59ad098bddc

Comment 19

[Zibi Braniecki [:zbraniecki][:gandalf]]

Brendan - so, the backout happened because I added

<linkset>
  <html:link rel="localization" href="branding/brand.ftl"/>
  <html:link rel="localization" href="browser/branding/sync-brand.ftl"/>
  <html:link rel="localization" href="browser/menubar.ftl"/>
</linkset>

to browser/base/content/macWindow.inc.xul which gets included all around, and in one place, there was no html namespace. It was browser/base/content/hiddenWindowMac.xhtml.

When I added it, the test passes, but the browser doesn't start on Mac with:

0:02.86 GECKO(20102) JavaScript error: resource://gre/modules/Fluent.jsm, line 636: TypeError: res.body is undefined

I assume this is because of https://searchfox.org/mozilla-central/source/dom/base/Document.cpp#5701 ?

Does it mean that we treat browser.xhtml specially, but not browser/base/content/hiddenWindowMac.xhtml ?

Can you help me get the body in there as well?

Comment 20

[Zibi Braniecki [:zbraniecki][:gandalf]]

The bug is reproducible with this patch on MacOS.

Comment 21

[Zibi Braniecki [:zbraniecki][:gandalf]]

d'uh, I really shouldn't be allowed to set NI in the evening...

The reason it failed was because hiddenWindowMac.xhtml was loaded before browser.xhtml, and it triggered async loading of the 3 FTL files, and then browser.xhtml tried to load the same files but sync.

I added a warning in L10nRegistry to not return from cache if the cached value is a Promise and we want sync - this should help us discover the cases better than with a meaningless body.res is undefined.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f622156e9ea
Migrate menubar to Fluent. r=fluent-reviewers,Pike,flod

Comment 23

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/1f622156e9ea

Comment 25

[Jorg K (CEST = GMT+2)]

Zibi, next time you do Fluent changes that remove stuff we use and cause a "burning tree" on a Saturday morning (bug 1571181), could you please let us know. Thanks in advance.