Closed Bug 1399389 Opened 2 years ago Closed 2 years ago

Integrate gfxCriticalNote/gfxCriticalError in WebRender

Categories

(Core :: Graphics: WebRender, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: nical, Assigned: jerry)

References

(Blocks 1 open bug)

Details

(Whiteboard: [wr-reserve] [gfx-noted])

Attachments

(2 files, 2 obsolete files)

Being able to attach some info to crash reports is useful in gecko and it would be useful to have access to that in WebRender code as well.

This should be fairly easy to do.

Relevant webrender issue: https://github.com/servo/webrender/issues/1677
Priority: -- → P3
Whiteboard: [gfx-noted][wr-mvp] [triage] → [wr-reserve] [gfx-noted]
MozReview-Commit-ID: Fxkz3Fq96Tb
Assignee: nobody → hshih
Status: NEW → ASSIGNED
Comment on attachment 8922247 [details] [diff] [review]
P1: redirect the warn/error message to gfx_critical_error/note in WR.

This patch try to use a custom log_handler which could redirect the error/warn message to gecko through the ffi.

I will get a strange call stack in crash report for a gfx_critical_error message. Maybe it's the problem as bug 1352475.
Attachment #8922247 - Flags: review?(nical.bugzilla)
Attachment #8922247 - Flags: review?(kvark)
Comment on attachment 8922248 [details] [diff] [review]
P2: update Cargo.lock and webrender_ffi_generated.h for webrender_bindings updating.

Just update the cargo.lock and ffi.h
Attachment #8922248 - Flags: review?(nical.bugzilla)
Comment on attachment 8922247 [details] [diff] [review]
P1: redirect the warn/error message to gfx_critical_error/note in WR.

Review of attachment 8922247 [details] [diff] [review]:
-----------------------------------------------------------------

Good start! A few fixes are needed

::: gfx/thebes/gfxPlatform.cpp
@@ +141,5 @@
>  #include "mozilla/gfx/GPUParent.h"
>  #include "mozilla/layers/MemoryReportingMLGPU.h"
>  #include "prsystem.h"
>  
> +#include "mozilla/webrender/webrender_ffi.h"

nit: any reason for this to be on a separate line?

::: gfx/webrender_bindings/Cargo.toml
@@ +12,5 @@
>  euclid = "0.15"
>  app_units = "0.5.6"
>  gleam = "0.4"
> +log = "0.3"
> +env_logger = "0.4"

we shouldn't use `env_logger` in FF at all
Attachment #8922247 - Flags: review?(kvark) → review-
Priority: P3 → P1
Attachment #8922248 - Flags: review?(nical.bugzilla) → review+
remove the env_logger.

MozReview-Commit-ID: Fxkz3Fq96Tb
Attachment #8922726 - Flags: review?(nical.bugzilla)
Attachment #8922726 - Flags: review?(kvark)
Attachment #8922247 - Attachment is obsolete: true
Attachment #8922247 - Flags: review?(nical.bugzilla)
Attachment #8922248 - Attachment is obsolete: true
Attachment #8922726 - Flags: review?(kvark) → review+
Depends on: 1412740
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee22692d9844
redirect the warn/error message to gfx_critical_error/note in WR. r=kvark
https://hg.mozilla.org/integration/mozilla-inbound/rev/2be892ca2def
update Cargo.lock and webrender_ffi_generated.h for webrender_bindings updating. r=nical
Attachment #8922726 - Flags: review?(nical.bugzilla)
https://hg.mozilla.org/mozilla-central/rev/ee22692d9844
https://hg.mozilla.org/mozilla-central/rev/2be892ca2def
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1413043
Comment on attachment 8922726 [details] [diff] [review]
P1: redirect the warn/error message to gfx_critical_error/note in WR. v2.

Review of attachment 8922726 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatform.cpp
@@ +2568,5 @@
>      }
> +
> +    // Redirect the webrender's log to gecko's log system.
> +    // The current log level is "warning".
> +    mozilla::wr::wr_init_external_log_handler(wr::LogLevelFilter::Warn);

I don't think this will do what you want on Windows. On Windows this code runs in the UI process, but the useful part of the webrender code lives in the GPU process.

Also it would be good to wrap this raw wr_* function in a WebRenderAPI static function so we don't have raw FFI calls littered all over the place.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #10)
> Comment on attachment 8922726 [details] [diff] [review]

> I don't think this will do what you want on Windows. On Windows this code
> runs in the UI process, but the useful part of the webrender code lives in
> the GPU process.
> 
> Also it would be good to wrap this raw wr_* function in a WebRenderAPI
> static function so we don't have raw FFI calls littered all over the place.

Thanks, I will create another bug to fix this.
Blocks: 1415816
Create bug 1415816 for comment 10.
Flags: needinfo?(hshih)
You need to log in before you can comment on or make changes to this bug.