Closed Bug 1383041 Opened 7 years ago Closed 7 years ago

Update webrender to 0748e02d1be5f889fc17de2eb81c0c363ee3aa80

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files)

+++ 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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=136279f480a9763e5728931b4cecf9a6850c27cf
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b435efae5f8ef50f513bde9f37896ed42d77c59f

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.

So,
/// 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.

So,
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.

Yes.

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:

https://treeherder.mozilla.org/#/jobs?repo=try&author=kgupta@mozilla.com&fromchange=26e6310b80bebc254592946636976b5712945ff3&group_state=expanded&bugfiler&tochange=a14f6b88081ed1f18301c9313859801c85e9d7e6

Things are green.
WR @ 82ac4fbbeff9602027fa972009be7f7c5693f901

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c75ce32b88d21913da913dd4b971a5d5f64cdfa
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5f15626d80757120854b9c96578af81f79db54e

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.

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

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

https://reviewboard.mozilla.org/r/162142/#review167426

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

https://reviewboard.mozilla.org/r/162146/#review167686

::: 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.

https://reviewboard.mozilla.org/r/162146/#review167774
Attachment #8890966 - Flags: review?(mrobinson) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f413861fc69a
Update WR to cset 0748e02d1be5f889fc17de2eb81c0c363ee3aa80. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/caba97838c06
Update bindings for IdNamespace changes in WR cset 6a2662c. r=kvark
https://hg.mozilla.org/integration/autoland/rev/64e6e4ef4776
Update bindings for API change in WR cset 9868ef4. r=jrmuizel
https://hg.mozilla.org/integration/autoland/rev/c8568609e10c
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.