Update webrender to 0748e02d1be5f889fc17de2eb81c0c363ee3aa80

RESOLVED FIXED in Firefox 56



2 years ago
2 years ago


(Reporter: kats, Assigned: kats)


(Blocks 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)


(Whiteboard: [gfx-noted])


(5 attachments)

+++ This bug was initially created as a clone of Bug #1380645 +++

I'm filing this as a placeholder bug for the next webrender update. I may be running a cron script [1] that does try pushes with webrender update attempts, so that we can track build/test breakages introduced by webrender on a rolling basis. This bug will hold the try push links as well as dependencies filed for those breakages, so that we have a better idea going into the update of what needs fixing. I might abort the cron job because once things get too far out of sync it's hard to fully automate fixing all the breakages.

When we are ready to actually land the update, we can rename this bug and use it for the update, and then file a new bug for the next "future update".

[1] https://github.com/staktrace/moz-scripts/blob/master/try-latest-webrender.sh
Last good cset was b83c200c657f6b6fb17d09f329ba77803420b46a, but updating to cset 8fd634882111415a65da67e947f26eb170234f2f caused a bustage. Copying from bug 1380645 comment 11 and 12:

WR @ 8fd634882111415a65da67e947f26eb170234f2f


Bustage, regression range:

*   8fd6348 Auto merge of #1504 - glennw:lines, r=kvark
| * 1fd1eaa Basic implementation of line decoration display items.
| * bb8f891 Add LineDisplayItem to Wrench, add reftest
| * 88ac352 add LineDisplayItem
* dc746ed Auto merge of #1445 - kvark:namespace, r=glennw
* 6a2662c Cleaning resources on RenderApi::drop

The problem is that the FontKey struct in WR has been updated to include IdNamespace [1]. Both FontKey and IdNamespace have type aliases to deal with them being tuples with unnamed fields [2], but since FontKey contains IdNamespace and not WrIdNamespace, cbindgen doesn't use the alias in this case and generates a definition for IdNamespace [3] which doesn't compile.

Ryan, is there a good way to fix this (either in cbindgen, or in our bindings file)?

[1] https://hg.mozilla.org/try/rev/97063d8add4d5b22074727489f33c3d3011b297b#l26.26
[2] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/gfx/webrender_bindings/src/bindings.rs#44,51
[3] https://hg.mozilla.org/try/rev/1a09edf45c1589b7a5224148c3aa69292e4c6cdb#l1.12
Flags: needinfo?(rhunt)
There are a few options here.

1. Switch to generating typedef's for `type = ` instead of generating new types.

This matches up much closer with the rust definition of a type alias, and we could then have annotations be transfered from the Foo in `type Foo = Bar` to the Bar. Allowing us to specify annotations in webrender_bindings, and have them apply to the actual type in webrender.

/// cbindgen:field-names=[mHandle]
type WrIdNamespace = IdNamespace

Would generate,
struct IdNamespace {
  uint64_t mHandle
typedef IdNamespace WrIdNamespace;

I have patches that do this in cbindgen and I can confirm it doesn't break Gecko, although there are a few slight changes needed.

2. Handle tuple structs better

Eventually I think we should get rid of all of those aliases, so we should provide a way without annotations to have tuple structs work.

I'm thinking a rename rule that only applies to tuple structs.

struct IdNamespace(u64);

Would generate,
struct IdNamespace {
  uint64_t m0;

And that would at least work and match exactly how Rust handles it.

I don't have patches for that, but it wouldn't be too hard.
Flags: needinfo?(rhunt)
With approach #2, we'd lose the field names, right? So we'd have to update any Gecko code that now uses e.g. WrWindowId::mHandle to instead refer to wr::WindowId::m0 instead? If so I think I prefer approach #1.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3)
> With approach #2, we'd lose the field names, right? So we'd have to update
> any Gecko code that now uses e.g. WrWindowId::mHandle to instead refer to
> wr::WindowId::m0 instead? If so I think I prefer approach #1.


Rust refers to tuple struct members by number too, but they can use match and if let() to make it not so confusing.

I just pushed a new cbindgen with #1. (version 0.1.19). Let me know if there are any issues.

I'll also upload a patch with the changes I had to make to Gecko.
Here is the full patch of the new output of cbindgen and the Gecko changes necessary.

One gross part is in LayersLogging due to inability to forward declare typedefs. I'm not sure what a great solution for that is. I don't know what still uses LayersLogging with WR types.
So I updated to cbindgen-0.1.19, applied your patch, and ran cbindgen again on top of that. It generates a bunch of changes, like changing "struct VecU8;" to "struct Vec_u8;" and other things. Is that expected? I'm using current m-c tip as my base revision.
Oh, I think it's because of bug 1381949 which just landed. I have it in my tree and I'm guessing you don't have it in yours. I'll see if I can get it building with that.
Yes that seems like it's from bug 1381949. Now that things are typedef'ed, we end up generating more monomorphisations which are mangled. Vec_u8 will be one of those. Let me know if it's too much bustage, we can try to find another solution.
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26e6310b80bebc254592946636976b5712945ff3

The "WIP switch to using typedefs in bindings file" patch is the one you attached above, and "[mq]: binding-fix2" is what I need to apply on top of it to effectively rebase on top of bug 1381949. And "Re-generate FFI header" and "[mq]: wr-fixit" are what I'll need as part of this bug to deal with the upstream WR changes. It builds locally, let's see how the try push turns out.
Above try push had OS X build errors which I fixed in subsequent pushes. Here's all the pushes with WR @ 8fd634882111415a65da67e947f26eb170234f2f from over the weekend:


Things are green.
WR @ 82ac4fbbeff9602027fa972009be7f7c5693f901


Green on linux64, the windows reftest jobs didn't run for some reason (maybe the taskcluster switchover?)
Assignee: nobody → bugmail
Summary: Future webrender update bug → Update webrender to 0748e02d1be5f889fc17de2eb81c0c363ee3aa80
Comment on attachment 8890963 [details]
Bug 1383041 - Update WR to cset 0748e02d1be5f889fc17de2eb81c0c363ee3aa80.

Attachment #8890963 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8890965 [details]
Bug 1383041 - Update bindings for API change in WR cset 9868ef4.

Attachment #8890965 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8890964 [details]
Bug 1383041 - Update bindings for IdNamespace changes in WR cset 6a2662c.


looks reasonable
Attachment #8890964 - Flags: review?(kvark) → review+
Comment on attachment 8890966 [details]
Bug 1383041 - Update bindings for API change in WR cset 9f66b56.


::: gfx/webrender_bindings/src/bindings.rs:1017
(Diff revision 1)
>          let content_rect: LayoutRect = content_rect.into();
>          let clip_rect: LayoutRect = clip_rect.into();
> -        state.frame_builder.dl_builder.define_scroll_frame(Some(clip_id), content_rect, clip_rect, vec![], None);
> +        state.frame_builder.dl_builder.define_scroll_frame(
> +            Some(clip_id), content_rect, clip_rect, vec![], None,
> +            ScrollSensitivity::ScriptAndInputEvents);

As far I know Gecko only sets scroll offsets in WebRender and never uses the WebRender scroll API. If that's the case ScrollSensitivity::Script may be more appropriate here.
Attachment #8890966 - Flags: review?(mrobinson) → review-
Comment on attachment 8890966 [details]
Bug 1383041 - Update bindings for API change in WR cset 9f66b56.

Attachment #8890966 - Flags: review?(mrobinson) → review+
Pushed by kgupta@mozilla.com:
Update WR to cset 0748e02d1be5f889fc17de2eb81c0c363ee3aa80. r=jrmuizel
Update bindings for IdNamespace changes in WR cset 6a2662c. r=kvark
Update bindings for API change in WR cset 9868ef4. r=jrmuizel
Update bindings for API change in WR cset 9f66b56. r=mrobinson
You need to log in before you can comment on or make changes to this bug.