Closed
Bug 1383041
Opened 8 years ago
Closed 8 years ago
Update webrender to 0748e02d1be5f889fc17de2eb81c0c363ee3aa80
Categories
(Core :: Graphics: WebRender, enhancement, P3)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(5 files)
15.91 KB,
patch
|
Details | Diff | Splinter Review | |
59 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kvark
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mrobinson
:
review+
|
Details |
+++ 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
Assignee | ||
Comment 1•8 years ago
|
||
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)
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
(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.
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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.
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
WR @ 9ebb50b1e22cea566a7c0a8d2cdc6e446a90ea24
https://treeherder.mozilla.org/#/jobs?repo=try&revision=adcba6e60d57109e0dfe207f898587b8b3c3595c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3737389fcd3395ad87141446a9dbc2de78c5b4c
Bustage, looks like a minor API change.
Assignee | ||
Comment 12•8 years ago
|
||
WR @ 9ebb50b1e22cea566a7c0a8d2cdc6e446a90ea24, with fixup
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1f4d0f4f8c9682d7c1cce8072b2184b35a20cec
https://treeherder.mozilla.org/#/jobs?repo=try&revision=272715455bccaa02319d72e6b466d7f9c49813b3
Green
Assignee | ||
Comment 13•8 years ago
|
||
WR @ d88ea62991d66583fd411f160523b2de020fcbc9
https://treeherder.mozilla.org/#/jobs?repo=try&revision=941d9ebfde2538b7a2629ceb3534f7532502b9c4
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cb476dd3b8bc02049c70348fc65dbf7f53aa149
Bustage again
Assignee | ||
Comment 14•8 years ago
|
||
WR @ d88ea62991d66583fd411f160523b2de020fcbc9 with fixup:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b38182f927d4b749ae7aad2d8dc4f5006ce570d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=807e2b864bf4fdd550753496281d5cfc00d2b925
Green
Assignee | ||
Comment 15•8 years ago
|
||
WR @ 283192c41743a59da87b065cbc14c659d94c90b5
https://treeherder.mozilla.org/#/jobs?repo=try&revision=53347e6c8528e568e01457fe1acd304cb2462849
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e48c00231f7f438bd93f7fe523c7b43c75b8e93e
Green so far, windows still pending
Assignee | ||
Comment 16•8 years ago
|
||
WR @ 8cbc971fc2a61114ba007f7c7d11f8ed72e8317f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=91c44cbab59102722c495101d1c16378ee299dfc
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fcd15f7b93b359eecc37c3f76c2e3d5cf175ad9b
Green so far, windows still pending.
Assignee | ||
Comment 17•8 years ago
|
||
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 | ||
Comment 18•8 years ago
|
||
WR @ 0748e02d1be5f889fc17de2eb81c0c363ee3aa80
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea3ab5916eb7e2358e6b8878ed19b8779b25587
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf12d4833d07f1b4e85328b47ac31bad7e93319
Ditto
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → bugmail
Summary: Future webrender update bug → Update webrender to 0748e02d1be5f889fc17de2eb81c0c363ee3aa80
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•8 years ago
|
||
mozreview-review |
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 24•8 years ago
|
||
mozreview-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 25•8 years ago
|
||
mozreview-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 26•8 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 31•8 years ago
|
||
mozreview-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+
Comment 32•8 years ago
|
||
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
Comment 33•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f413861fc69a
https://hg.mozilla.org/mozilla-central/rev/caba97838c06
https://hg.mozilla.org/mozilla-central/rev/64e6e4ef4776
https://hg.mozilla.org/mozilla-central/rev/c8568609e10c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•