Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't skip SetGradientStops #2329

Closed
wants to merge 1 commit into from
Closed

Don't skip SetGradientStops #2329

wants to merge 1 commit into from

Conversation

@kvark
Copy link
Member

kvark commented Jan 22, 2018

Fixes #2328
r? @gankro


This change is Reviewable

@kvark
Copy link
Member Author

kvark commented Jan 22, 2018

Note: WR frame building is already happy skipping this:

            // Do nothing; these are dummy items for the display list parser
            SpecificDisplayItem::SetGradientStops => {}
@pyfisch
Copy link
Contributor

pyfisch commented Jan 22, 2018

Not sure why but with this PR servo crashes for me because of Option::unwrap. https://gist.github.com/pyfisch/8ff4ff4a476152aeb644a340141dc1ca

Maybe because webrender_api and webrender do not match. Even with matching versions servo crashes. On master this crash does not happen.

@Gankra
Copy link
Contributor

Gankra commented Jan 22, 2018

This isn't correct; we clear gradient stops at the start of next. as-is this patch just makes it so gradient-stops never make it to the backend.

@kvark
Copy link
Member Author

kvark commented Jan 22, 2018

@gankro what would be your recommendation then? having that continue gated by a run-time boolean flag that we enable only when debug-serializing?

@Gankra
Copy link
Contributor

Gankra commented Jan 22, 2018

I'm not familiar with the debug-serializing feature, but one option would be to split next into two methods: a "raw" one, and a wrapper that consumes the raw one and behaves like the current next. Then anyone who needs to reproduce the input can just use the raw one?

@kvark
Copy link
Member Author

kvark commented Jan 22, 2018

@gankro how about we don't hack thing as low as traversing items and instead put the gradient stop logic in the context flattening? That would make all the low-level things like debug serialization robust. Please take a look at the code in this PR now.

@kvark kvark force-pushed the kvark-gradient-stops branch from 5bfe9d1 to 6eb062b Jan 22, 2018
@Gankra
Copy link
Contributor

Gankra commented Jan 22, 2018

Presumably the same logic would need to be used in the DL->yaml/json writers? (I don't think we have tests for that)

@kvark kvark force-pushed the kvark-gradient-stops branch from 6eb062b to 2d653ce Jan 22, 2018
@kvark
Copy link
Member Author

kvark commented Jan 22, 2018

@gankro good point! I fixed the YAML writer to take those into account and tested it by saving/loading a thing. Appears to work fine.
As to JSON/RON - these folks use debug serialization, and they don't need to be loaded (1) afterwards, so not only they don't require extra fixes, but they are also already improved :)

(1) actually, the capture infrastructure does need RON to be loaded, and with gradient stops we can just have better capturing of gradients on the scene level.

@glennw
Copy link
Member

glennw commented Jan 23, 2018

@gankro @kvark Is this one ready to go?

@glennw
Copy link
Member

glennw commented Jan 29, 2018

@kvark
Copy link
Member Author

kvark commented Jan 29, 2018

Marking as blocked, decided to re-think gradient stops more fundamentally with @gankro .

@bors-servo
Copy link
Contributor

bors-servo commented Jan 31, 2018

The latest upstream changes (presumably #2315) made this pull request unmergeable. Please resolve the merge conflicts.

@pyfisch
Copy link
Contributor

pyfisch commented Feb 11, 2018

Are there now concrete plans for improved gradient stops? If not what are the open questions?

@kvark
Copy link
Member Author

kvark commented Feb 12, 2018

@pyfisch last time I heard @gankro was looking into it

@glennw
Copy link
Member

glennw commented Mar 14, 2018

@gankro Should this be kept open?

@Gankra
Copy link
Contributor

Gankra commented Mar 14, 2018

Let's close this PR in favour of an issue (sorry for letting this slip!)

@kvark kvark closed this Mar 14, 2018
@kvark kvark deleted the kvark-gradient-stops branch Jul 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.