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

Pipe dwrite rendering options to rasterize_glyphs #770

Merged
merged 1 commit into from Jan 26, 2017

Conversation

@changm
Copy link
Contributor

changm commented Jan 23, 2017

Essentially replicating this from gecko - http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#1390

I contemplated various ways to pass glyph rendering options to rasterize_glyphs either through traits or some struct, but we don't do anything special for linux or mac, so using a option seemed like the best thing, even though it exposes "dwrite" and forces the other platforms to use it. Nor could I find a clean way to pass derived types with data only as traits seemed to only pass down method interfaces.


This change is Reviewable

@kvark kvark self-requested a review Jan 24, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Jan 24, 2017

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

@glennw
Copy link
Member

glennw commented Jan 24, 2017

Do these options need to be stored per text run dynamically? For instance, can they be set once per frame / font or at init time perhaps? How often do they change?

@changm
Copy link
Contributor Author

changm commented Jan 24, 2017

They can't be stored once per frame, it depends on the text run :(. Layout determines whether or not to enable these options for each text run. Even for the same font, depending on the font size, the options can change.

@changm changm force-pushed the changm:options branch from d71e5a5 to ba856a9 Jan 24, 2017
@glennw
Copy link
Member

glennw commented Jan 24, 2017

OK, in that case, how about we rename the struct to just be GlyphOptions, and then we can specify which platforms each field applies to. This will allow us to use the same struct for other platform-specific options in the future if we need to.

@changm
Copy link
Contributor Author

changm commented Jan 24, 2017

How do you specify how each field applies to which platform? I couldn't find something like that. I thought about going the GlyphOptions route but we don't use that for CG/Unix and I'd rather keep it very explicit what it's for. That's also why I passed it in as an Option rather than the full struct.

@glennw
Copy link
Member

glennw commented Jan 24, 2017

You can use cfg() on each field to make it only available on that platform - but I was actually just suggesting a comment above each field specifying that this applies to Windows only. So I think if we just rename DWriteGlyphOptions -> GlyphOptions and add a comment for each of the two fields saying they only apply to Windows, that would be fine for now.

Copy link
Member

kvark left a comment

I agree that DWriteGlyphOptions needs to be renamed, otherwise looks good.

@changm changm force-pushed the changm:options branch from ba856a9 to fd59942 Jan 24, 2017
@changm
Copy link
Contributor Author

changm commented Jan 24, 2017

Thanks for the review! Renamed and a comment added saying that GlyphOptions are only used on windows for now.

@changm changm force-pushed the changm:options branch from fd59942 to c5d6838 Jan 25, 2017
@glennw
Copy link
Member

glennw commented Jan 25, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2017

📌 Commit c5d6838 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jan 25, 2017

Testing commit c5d6838 with merge 38e57d8...

bors-servo added a commit that referenced this pull request Jan 25, 2017
Pipe dwrite rendering options to rasterize_glyphs

Essentially replicating this from gecko - http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetSkia.cpp#1390

I contemplated various ways to pass glyph rendering options to rasterize_glyphs either through traits or some struct, but we don't do anything special for linux or mac, so using a option seemed like the best thing, even though it exposes "dwrite" and forces the other platforms to use it. Nor could I find a clean way to pass derived types with data only as traits seemed to only pass down method interfaces.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/770)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 26, 2017

☀️ Test successful - status-travis

@bors-servo bors-servo merged commit c5d6838 into servo:master Jan 26, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@changm changm deleted the changm:options branch Jan 26, 2017
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

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