Closed
Bug 1454878
Opened 6 years ago
Closed 6 years ago
Update WrExternalLogHandler as to use env_logger
Categories
(Core :: Graphics: WebRender, enhancement, P1)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
Attachments
(1 file, 1 obsolete file)
14.83 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
In gecko, wr::WrLogLevelFilter::Error is always used for WrLogLevelFilter in WebRenderAPI::InitExternalLogHandler(). It seems better to set GfxLogLevel to WrLogLevelFilter. https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_bindings/WebRenderAPI.cpp#257
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 1•6 years ago
|
||
Hmm, WrExternalLogHandler seems to work only when gpu process is enabled.
Assignee | ||
Comment 2•6 years ago
|
||
WrExternalLogHandler was set by calling log::set_logger(). But it works only when a logger is not set to rust log yet. https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_bindings/src/bindings.rs#2367 When WebRender was crated on parent process, a logger was already set to rust log. It was done under InitializeServo(). Its stack was like the following. xul.dll!log::set_logger_inner<closure>(closure make_logger) 行 1054 xul.dll!env_logger::Builder::try_init() 行 610 xul.dll!env_logger::Builder::init() 行 622 xul.dll!geckoservo::glue::Servo_Initialize(a) 行 18 xul.dll!InitializeServo() 行 2444 C++ xul.dll!nsLayoutStatics::Initialize() 行 286 C++ xul.dll!Initialize() 行 274 C++ xul.dll!nsComponentManagerImpl::KnownModule::Load() 行 727 C++ xul.dll!nsFactoryEntry::GetFactory() 行 1748 C++ xul.dll!nsComponentManagerImpl::CreateInstanceByContractID()
Assignee | ||
Comment 3•6 years ago
|
||
Like the following link, log::set_logger_inner() store a logger only when it is not initialized yet. https://dxr.mozilla.org/mozilla-central/source/third_party/rust/log/src/lib.rs#1049
Assignee | ||
Comment 4•6 years ago
|
||
The rust log is a global system. It seems not make sense to set graphics only logger WrExternalLogHandler to the rust log.
Assignee | ||
Comment 5•6 years ago
|
||
MDN already has a document for logging in rust. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Gecko_Logging#Rust
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #4) > The rust log is a global system. It seems not make sense to set graphics > only logger WrExternalLogHandler to the rust log. Then it seems better to remove WrExternalLogHandler and use env_logger instead to make the logging consistent to gecko.
Assignee | ||
Comment 7•6 years ago
|
||
I changed my mind, it seems better to keep WrExternalLogHandler for now. Even if WrExternalLogHandler works only with GPU process. It helps to analyze WebRender's bug on Windows. And WrExternalLogHandler could coexist with env_logger.
Assignee | ||
Updated•6 years ago
|
Summary: Connect GfxLogLevel to WrLogLevel → Update WrExternalLogHandler as to use env_logger
Assignee | ||
Comment 8•6 years ago
|
||
The patch also limit WrExternalLogHandler usage only in gpu process. On non-gpu process, InitializeServo() handles rust logger initialization and WrExternalLogHandler does not work already.
Assignee | ||
Updated•6 years ago
|
Attachment #8969180 -
Flags: review?(jmuizelaar)
Updated•6 years ago
|
Blocks: stage-wr-trains
Priority: -- → P1
Updated•6 years ago
|
Attachment #8969180 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 9•6 years ago
|
||
Rebased.
Attachment #8969180 -
Attachment is obsolete: true
Attachment #8970771 -
Flags: review+
Assignee | ||
Comment 10•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29da378e8457b311343dfbb7081ec9bfc1ecc4f9
Comment 11•6 years ago
|
||
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/be541e099174 Update WrExternalLogHandler as to use env_logger r=jrmuizel
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/be541e099174
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•