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

Support keyword values for 'cursor' in geckolib #11688

Merged
merged 1 commit into from Jun 10, 2016

Conversation

@mbrubeck
Copy link
Contributor

mbrubeck commented Jun 9, 2016

This has a big hard-coded match instead of a generated one because Servo's Cursor enum is not generated by Mako code; instead it's in style_traits where it can be shared with Servo's gfx and windowing code.

We could move this mapping into the macro that generates the Cursor enum, but it would either involve the same amount of code duplication, or make style_traits depend on gecko_bindings. I'm not really sure which is best; open to suggestions.

r? @emilio


This change is Reviewable

@emilio
Copy link
Member

emilio commented Jun 9, 2016

Looks good. I suppose we don't want to make a normal Servo build depend on gecko_bindings, and I can't think off-hand of a way that wouldn't have this match otherwise.

If we see this pattern repeating a lot I guess we can use a helper trait like ToGeckoEnum, and move the matches away from properties.mako.rs for legibility, at least until we align the binary representations for that. But I don't really have any other objection/idea :)

@bors-servo: r+

-S-awaiting-review +S-awaiting-merge

Previously, mbrubeck (Matt Brubeck) wrote…

Support keyword values for 'cursor' in geckolib

This has a big hard-coded match instead of a generated one because Servo's Cursor enum is not generated by Mako code; instead it's in style_traits where it can be shared with Servo's gfx and windowing code.

We could move this mapping into the macro that generates the Cursor enum, but it would either involve the same amount of code duplication, or make style_traits depend on gecko_bindings. I'm not really sure which is best; open to suggestions.

r? @emilio


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Jun 9, 2016

📌 Commit d4a84cc has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented Jun 9, 2016

Testing commit d4a84cc with merge 25f7b9e...

bors-servo added a commit that referenced this pull request Jun 9, 2016
Support keyword values for 'cursor' in geckolib

This has a big hard-coded `match` instead of a generated one because Servo's Cursor enum is not generated by Mako code; instead it's in `style_traits` where it can be shared with Servo's gfx and windowing code.

We *could* move this mapping into the macro that generates the Cursor enum, but it would either involve the same amount of code duplication, or make style_traits depend on gecko_bindings. I'm not really sure which is best; open to suggestions.

r? @emilio

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

bors-servo commented Jun 9, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 9, 2016

  ▶ FAIL [expected PASS] /_mozilla/css/iframe/positioning_margin.html
  └   → /_mozilla/css/iframe/positioning_margin.html d9d9e489bc5b4508c01b06c7ff92f19dcc2ece3c
/_mozilla/css/iframe/positioning_margin_ref.html 3a4684450cbc08c6e80ab78eb761e71881c74ab1
Testing d9d9e489bc5b4508c01b06c7ff92f19dcc2ece3c == 3a4684450cbc08c6e80ab78eb761e71881c74ab1
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jun 9, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

Testing commit d4a84cc with merge c6f9c81...

bors-servo added a commit that referenced this pull request Jun 10, 2016
Support keyword values for 'cursor' in geckolib

This has a big hard-coded `match` instead of a generated one because Servo's Cursor enum is not generated by Mako code; instead it's in `style_traits` where it can be shared with Servo's gfx and windowing code.

We *could* move this mapping into the macro that generates the Cursor enum, but it would either involve the same amount of code duplication, or make style_traits depend on gecko_bindings. I'm not really sure which is best; open to suggestions.

r? @emilio

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

bors-servo commented Jun 10, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 10, 2016

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-007.htm
  └   → /css-transforms-1_dev/html/transform-abspos-007.htm 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 78d197606924062e8dd2a773c977afcecf8940f8
Testing 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8 == 78d197606924062e8dd2a773c977afcecf8940f8
@mbrubeck
Copy link
Contributor Author

mbrubeck commented Jun 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

Testing commit d4a84cc with merge 2cfef13...

bors-servo added a commit that referenced this pull request Jun 10, 2016
Support keyword values for 'cursor' in geckolib

This has a big hard-coded `match` instead of a generated one because Servo's Cursor enum is not generated by Mako code; instead it's in `style_traits` where it can be shared with Servo's gfx and windowing code.

We *could* move this mapping into the macro that generates the Cursor enum, but it would either involve the same amount of code duplication, or make style_traits depend on gecko_bindings. I'm not really sure which is best; open to suggestions.

r? @emilio

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

bors-servo commented Jun 10, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Jun 10, 2016

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-abspos-002.htm
  └   → /css-transforms-1_dev/html/transform-abspos-002.htm 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8
/css-transforms-1_dev/html/reference/transform-abspos-ref.htm 78d197606924062e8dd2a773c977afcecf8940f8
Testing 71f0313eedfbfcce0b0fcc5ae55f34b1daa3b8d8 == 78d197606924062e8dd2a773c977afcecf8940f8
@cbrewster
Copy link
Member

cbrewster commented Jun 10, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2016

@bors-servo bors-servo merged commit d4a84cc into servo:master Jun 10, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@cbrewster cbrewster mentioned this pull request Jun 12, 2016
4 of 5 tasks complete
@mbrubeck mbrubeck deleted the mbrubeck:cursor-master branch May 2, 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

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