1697863 - Add preliminary Proton styles for browser UI menulists

Status: RESOLVED FIXED

(Firefox :: Theme, task)

Description

[Mike Conley (:mconley) (:⚙️)]

Similar to bug 1697315. For Proton, we're changing the look and feel of browser UI menulists. This patch introduces the new styling, with a legacy attribute that can be set on the menulist to fallback to the old styling if we need to.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1697863 - Add preliminary Proton styles for browser UI menulists. r?harry!

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1697863 - Use native menulist styling on some dialogs. r?harry!

Depends on D108061

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/64166dc7f85f
Add preliminary Proton styles for browser UI menulists. r=harry,desktop-theme-reviewers
https://hg.mozilla.org/integration/autoland/rev/f82846b236e3
Use native menulist styling on some dialogs. r=harry

Comment 4

[Natalia Csoregi [:nataliaCs]]

Backed out for valgrind failures

backout: https://hg.mozilla.org/integration/autoland/rev/6834c693610e056902f6729f8a0554994c0c1272

push: https://treeherder.mozilla.org/jobs?repo=autoland&searchStr=linux%2Cx64%2Copt%2Cvalgrind-linux64-valgrind%2Fopt%2Cv&revision=f82846b236e3e5056988793f66e5c69dc6127fee

failure log: https://treeherder.mozilla.org/logviewer?job_id=333450892&repo=autoland&lineNumber=60856

[task 2021-03-16T22:34:04.603Z] 22:34:04     INFO -   3:12.21 TEST-UNEXPECTED-FAIL | valgrind-test | Conditional jump or move depends on uninitialised value(s) at selectors::parser::parse_one_simple_selector / selectors::parser::parse_selector / selectors::parser::parse_functional_pseudo_class / selectors::parser::parse_one_simple_selector
[task 2021-03-16T22:34:04.603Z] 22:34:04     INFO -   3:12.21 ==12371== Conditional jump or move depends on uninitialised value(s)
[task 2021-03-16T22:34:04.603Z] 22:34:04     INFO -   3:12.21 ==12371==    at 0x15D35769: selectors::parser::parse_one_simple_selector+7817 (parser.rs:2023)
[task 2021-03-16T22:34:04.603Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15D303D7: selectors::parser::parse_selector+1175 (parser.rs:2130)
[task 2021-03-16T22:34:04.603Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15D39257: selectors::parser::parse_functional_pseudo_class+1399 (parser.rs:417)
[task 2021-03-16T22:34:04.604Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15D35031: selectors::parser::parse_one_simple_selector+5969 (parser.rs:2417)
[task 2021-03-16T22:34:04.604Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15D303D7: selectors::parser::parse_selector+1175 (parser.rs:2130)
[task 2021-03-16T22:34:04.604Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15D2F7A8: selectors::parser::SelectorList<Impl>::parse_with_state+120 (parser.rs:377)
[task 2021-03-16T22:34:04.604Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15E355D3: style::stylesheets::rule_parser::NestedRuleParser::parse_nested_rules+2483 (parser.rs:356)
[task 2021-03-16T22:34:04.604Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15E3240A: <style::stylesheets::rule_parser::NestedRuleParser as cssparser::rules_and_declarations::AtRuleParser>::parse_block+490 (servo/components/style/stylesheets/rule_parser.rs:525)
[task 2021-03-16T22:34:04.604Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15E30DF0: cssparser::rules_and_declarations::parse_at_rule+5280 (servo/components/style/stylesheets/rule_parser.rs:257)
[task 2021-03-16T22:34:04.605Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x15E4B00C: style::stylesheets::stylesheet::StylesheetContents::from_str+1980 (rules_and_declarations.rs:391)
[task 2021-03-16T22:34:04.605Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x157B6A56: Servo_StyleSheet_FromUTF8Bytes+454 (servo/ports/geckolib/glue.rs:1565)
[task 2021-03-16T22:34:04.605Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x13A31AF8: mozilla::StyleSheet::ParseSheet(mozilla::css::Loader&, nsTSubstring<char> const&, mozilla::css::SheetLoadData&)+472 (checkouts/gecko/layout/style/StyleSheet.cpp:1187)
[task 2021-03-16T22:34:04.605Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x13A1F971: mozilla::css::Loader::ParseSheet(nsTSubstring<char> const&, mozilla::css::SheetLoadData&, mozilla::css::Loader::AllowAsyncParse)+321 (checkouts/gecko/layout/style/Loader.cpp:1522)
[task 2021-03-16T22:34:04.605Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x13A2DEC2: mozilla::css::StreamLoader::OnStopRequest(nsIRequest*, nsresult)+562 (checkouts/gecko/layout/style/StreamLoader.cpp:143)
[task 2021-03-16T22:34:04.606Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x110EBAF6: nsBaseChannel::OnStopRequest(nsIRequest*, nsresult)+118 (checkouts/gecko/netwerk/base/nsBaseChannel.cpp:849)
[task 2021-03-16T22:34:04.606Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x110EBBBC: non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsresult)+12 (checkouts/gecko/netwerk/base/nsBaseChannel.cpp:0)
[task 2021-03-16T22:34:04.606Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11104398: nsInputStreamPump::OnStateStop()+328 (checkouts/gecko/netwerk/base/nsInputStreamPump.cpp:649)
[task 2021-03-16T22:34:04.606Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11103C0A: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)+426 (checkouts/gecko/netwerk/base/nsInputStreamPump.cpp:397)
[task 2021-03-16T22:34:04.606Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x10FF8363: nsInputStreamReadyEvent::Run()+35 (checkouts/gecko/xpcom/io/nsStreamUtils.cpp:94)
[task 2021-03-16T22:34:04.606Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11017C03: mozilla::RunnableTask::Run()+195 (checkouts/gecko/xpcom/threads/TaskController.cpp:472)
[task 2021-03-16T22:34:04.607Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11016B41: mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&)+1617 (checkouts/gecko/xpcom/threads/TaskController.cpp:760)
[task 2021-03-16T22:34:04.607Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11015E34: mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&)+36 (checkouts/gecko/xpcom/threads/TaskController.cpp:611)
[task 2021-03-16T22:34:04.607Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11015FC7: mozilla::TaskController::ProcessPendingMTTask(bool)+55 (checkouts/gecko/xpcom/threads/TaskController.cpp:395)
[task 2021-03-16T22:34:04.607Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11019771: operator() (checkouts/gecko/xpcom/threads/TaskController.cpp:133)
[task 2021-03-16T22:34:04.607Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11019771: mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_3>::Run()+17 (checkouts/gecko/xpcom/threads/nsThreadUtils.h:534)
[task 2021-03-16T22:34:04.608Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11023DF7: nsThread::ProcessNextEvent(bool, bool*)+1623 (checkouts/gecko/xpcom/threads/nsThread.cpp:1158)
[task 2021-03-16T22:34:04.608Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11027D87: NS_ProcessNextEvent(nsIThread*, bool)+71 (checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:548)
[task 2021-03-16T22:34:04.608Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x115CF987: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+135 (checkouts/gecko/ipc/glue/MessagePump.cpp:87)
[task 2021-03-16T22:34:04.608Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11589B35: RunInternal (checkouts/gecko/ipc/chromium/src/base/message_loop.cc:335)
[task 2021-03-16T22:34:04.608Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11589B35: RunHandler (checkouts/gecko/ipc/chromium/src/base/message_loop.cc:328)
[task 2021-03-16T22:34:04.608Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x11589B35: MessageLoop::Run()+85 (checkouts/gecko/ipc/chromium/src/base/message_loop.cc:310)
[task 2021-03-16T22:34:04.609Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x1388E618: nsBaseAppShell::Run()+40 (checkouts/gecko/widget/nsBaseAppShell.cpp:137)
[task 2021-03-16T22:34:04.609Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x14A19716: nsAppStartup::Run()+118 (checkouts/gecko/toolkit/components/startup/nsAppStartup.cpp:273)
[task 2021-03-16T22:34:04.609Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x14AE904B: XREMain::XRE_mainRun()+3051 (checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5352)
[task 2021-03-16T22:34:04.609Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x14AE998C: XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&)+1164 (checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5543)
[task 2021-03-16T22:34:04.609Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x14AE9D4E: XRE_main(int, char**, mozilla::BootstrapConfig const&)+158 (checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5606)
[task 2021-03-16T22:34:04.609Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x12ACB3: do_main (checkouts/gecko/browser/app/nsBrowserApp.cpp:220)
[task 2021-03-16T22:34:04.610Z] 22:34:04     INFO -   3:12.21 ==12371==    by 0x12ACB3: main+1203 (checkouts/gecko/browser/app/nsBrowserApp.cpp:347)
[task 2021-03-16T22:34:04.610Z] 22:34:04     INFO -   3:12.21 ==12371==  Uninitialised value was created by a stack allocation
[task 2021-03-16T22:34:04.610Z] 22:34:04     INFO -   3:12.21 ==12371==    at 0x15D338ED: selectors::parser::parse_one_simple_selector+13 (parser.rs:2306)
[task 2021-03-16T22:34:04.610Z] 22:34:04     INFO -   3:12.21 ==12371==

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Huh... I'm kind of at a loss to explain this one... emilio, any idea what I've tripped here?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

This is a known Valgrind false-positive.

This seems a version of this suppression just with an slightly different stack.

In particular, it looks like this suppression but without the parse_until_before stack frame.

I don't know if valgrind uses PGO builds. If it does, you probably made a code-path frequent enough that the compiler decided to inline parse_until_before.

In any case what needs to happen is adjusting the Valgrind suppression list, either by removing the line linked above, or adding a suppression for a stack without that line.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1697863 - Add another Stylo valgrind suppression. r?emilio!

Depends on D108101

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Here's a trybuild with the new suppression, which valgrind seems to like: https://treeherder.mozilla.org/jobs?repo=try&revision=008f1d9868960fc3606c6fc6c17b0d9069cada64

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6f19f39e7c5
Add preliminary Proton styles for browser UI menulists. r=harry,desktop-theme-reviewers
https://hg.mozilla.org/integration/autoland/rev/bad811dda7ec
Use native menulist styling on some dialogs. r=harry
https://hg.mozilla.org/integration/autoland/rev/42f6412b7bc1
Add another Stylo valgrind suppression. r=emilio

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d6f19f39e7c5
https://hg.mozilla.org/mozilla-central/rev/bad811dda7ec
https://hg.mozilla.org/mozilla-central/rev/42f6412b7bc1