Migrate browser-menubar to Fluent
Categories
(Firefox :: Menus, enhancement, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: zbraniecki, Assigned: zbraniecki)
References
Details
Attachments
(3 files, 3 obsolete files)
As part of bug 1501881 we'll want to migrate the browser-menubar.inc to Fluent.
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 1•5 years ago
|
||
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).
Assignee | ||
Comment 2•5 years ago
|
||
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
Assignee | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Comment 5•5 years ago
|
||
Comment 6•5 years ago
|
||
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•5 years ago
|
||
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•5 years ago
|
||
(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.
Assignee | ||
Comment 9•5 years ago
|
||
Depends on D7591
Assignee | ||
Comment 10•5 years ago
|
||
Assignee | ||
Comment 11•5 years ago
|
||
> 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?
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 13•5 years ago
|
||
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.
Assignee | ||
Comment 14•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 15•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 16•5 years ago
|
||
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•5 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7a06c08bf267 Migrate menubar to Fluent. r=fluent-reviewers,Pike,flod
Comment 18•5 years ago
|
||
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
Assignee | ||
Comment 19•5 years ago
|
||
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?
Assignee | ||
Comment 20•5 years ago
|
||
The bug is reproducible with this patch on MacOS.
Assignee | ||
Comment 21•5 years ago
|
||
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•5 years ago
|
||
Pushed by zbraniecki@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1f622156e9ea Migrate menubar to Fluent. r=fluent-reviewers,Pike,flod
Comment 23•5 years ago
|
||
bugherder |
Comment 25•5 years ago
|
||
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.
Description
•