Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upstyle: Move cursor property out of mako #19798
Conversation
highfive
commented
Jan 17, 2018
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jan 17, 2018
|
error[E0609]: no field `cursor` on type `&properties::style_structs::Pointing`
--> components/style/servo/restyle_damage.rs:284:22
|
284 | get_pointing.cursor, get_pointing.pointer_events,
| ^^^^^^
error: aborting due to previous error
error: Could not compile `style`.Not sure what causes it. |
|
Looks reasonable! I need to take a closer look tomorrow, meanwhile the comments should allow you to get pass the build error. Thanks for working on this! |
| "Cursor", | ||
| "computed::Cursor::auto()", | ||
| initial_specified_value="specified::Cursor::auto()", | ||
| products="gecko", |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 17, 2018
Member
You need to remove this products="gecko". With it you just removed the property from servo! :)
This comment has been minimized.
This comment has been minimized.
|
|
||
| /// cursor: [auto | default | ...] | ||
| #[cfg(not(feature = "gecko"))] | ||
| pub fn parse<'i, 't>( |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 17, 2018
Member
This should be a Parse trait implementation instead, ditto for the other below.
| @@ -94,12 +93,13 @@ define_cursor! { | |||
| "all-scroll" => AllScroll = 32, | |||
| "zoom-in" => ZoomIn = 33, | |||
| "zoom-out" => ZoomOut = 34, | |||
| "auto" => Auto = 35, | |||
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
Author
Contributor
Are these values arbitrary or do they represent something else internally? Is it okay if we just move them by one like this?
This comment has been minimized.
This comment has been minimized.
emilio
Jan 18, 2018
Member
I don't see them used anywhere, so we could even remove them and simplify the macro, I'd think.
| @@ -1196,6 +1196,7 @@ impl WindowMethods for Window { | |||
| use glutin::MouseCursor; | |||
|
|
|||
| let glutin_cursor = match c { | |||
| CursorKind::Auto => MouseCursor::Default, | |||
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
•
Author
Contributor
Without this, we'll get a pattern not covered error.
I looked at the glutin crate and, if I understand correctly, it's used as the layer between Servo and the OS that lets changing the appearance of the mouse cursor, right? As there is no MouseCursor::Auto in that crate, I used the Default variant for the time being. Shall we implement it there or is it okay the way it is? Also, can the OS even have a Auto variant of the mouse cursor? To me it seems it always has to be some specific type.
This comment has been minimized.
This comment has been minimized.
CYBAI
Jan 18, 2018
Collaborator
About pattern not covered part, it's caused by removing the pattern of CursorKind::Auto.
When doing pattern matching, we need to cover all the patterns for the type so if you removed CursorKind::Auto in this pattern matching, compiler will complain about this pattern is not covered.
I don't understand the implementation part either so only reply to the pattern matching part!
| @@ -3161,12 +3161,12 @@ impl ComputedValuesCursorUtility for ComputedValues { | |||
| #[inline] | |||
| fn get_cursor(&self, default_cursor: CursorKind) -> Option<CursorKind> { | |||
| match ( | |||
| self.get_pointing().pointer_events, | |||
| self.get_pointing().cursor, | |||
| &self.get_pointing().pointer_events, | |||
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
•
Author
Contributor
I'm getting a cannot move out of borrowed content error here. We can solve it in two ways: by borrowing like here and then pattern matching the references or by copying like so:
self.get_pointing().pointer_events.clone()and leaving the rest the way it is. I went with borrowing, but let me know if we should copy instead.
| _context: &ParserContext, | ||
| input: &mut Parser<'i, 't> | ||
| ) -> Result<Self, ParseError<'i>> { | ||
| #[allow(unused_imports)] use std::ascii::AsciiExt; |
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
•
Author
Contributor
Can we remove it? It's wasn't used anywhere. Was there something planned to be implemented, maybe?
This comment has been minimized.
This comment has been minimized.
emilio
Jan 18, 2018
Member
Yeah, we can.
This was needed for the eq_ignore_ascii_case on older rustc, and to keep style building with stable rust since it was needed for ports/geckolib.
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
Author
Contributor
It appears it's still needed. ./mach build -d won't compile when it's removed.
| Cursor::Alias => return cef_cursor_type_t::CT_ALIAS, | ||
| Cursor::Text => return cef_cursor_type_t::CT_IBEAM, | ||
| Cursor::Grab | Cursor::AllScroll => | ||
| CursorKind::None => return cef_cursor_type_t::CT_NONE, |
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
Author
Contributor
Isn't it the same thing without the return's? I think we can remove them and simplify the function just a bit.
This comment has been minimized.
This comment has been minimized.
CYBAI
Jan 18, 2018
Collaborator
No, we don't need to use return if this pattern matching will just return the value!
IMO, it's more clear if we don't use return here!
This comment has been minimized.
This comment has been minimized.
| @@ -1190,47 +1190,48 @@ impl WindowMethods for Window { | |||
| } | |||
|
|
|||
| /// Has no effect on Android. | |||
| fn set_cursor(&self, c: Cursor) { | |||
| fn set_cursor(&self, c: CursorKind) { | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| (PointerEvents::Auto, cursor::Keyword::Cursor(cursor)) => Some(cursor), | ||
| (&PointerEvents::None, _) => None, | ||
| (&PointerEvents::Auto, &Cursor(CursorKind::Auto)) => Some(default_cursor), | ||
| (&PointerEvents::Auto, &Cursor(cursor)) => Some(cursor), |
This comment has been minimized.
This comment has been minimized.
CYBAI
Jan 18, 2018
•
Collaborator
Sorry, I think I'm not familiar with borrow checker enough but I'm just curious why we have to use reference here?
If we don't use it, the value will be moved?
| /// The computed value for the `cursor` property. | ||
| /// Servo variant. | ||
| /// https://drafts.csswg.org/css-ui/#cursor | ||
| #[cfg(not(feature = "gecko"))] |
This comment has been minimized.
This comment has been minimized.
CYBAI
Jan 18, 2018
Collaborator
How about just using #[cfg(feature = "servo")]?
Maybe we don't need the Servo variant comment with this config attribute.
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
•
Author
Contributor
It's a bit different.
When we build with ./mach build-geckolib we trigger the feature = "gecko" attribute and compile in all code marked with it. If we mark that part with #[cfg(feature = "servo")], then it will still be compiled in with ./mach build-geckolib, which we don't want. However, marking it with #[cfg(not(feature = "gecko"))] allows us to exclude it in when we compile with ./mach build-geckolib, which is what we want.
This comment has been minimized.
This comment has been minimized.
CYBAI
Jan 18, 2018
•
Collaborator
Oh, in my understanding, if we mark it as #[cfg(feature = "servo")], it will only be compiled when feature is servo so it will be ignored when feature is gecko.
If I'm wrong, please share with me and just ignore this comment :)
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
Author
Contributor
Oh, wait, I think you're right actually. I thought for some reason that it will be compiled when feature is servo, which it won't. I need to get some sleep haha.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 18, 2018
•
Author
Contributor
No problem :). I just thought there could be a possibility that when we compile with ./mach build-geckolib both servo and gecko are enabled. Let's wait what Emilio says :).
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
emilio
Jan 18, 2018
Member
Yeah, feature = "gecko" and feature = "servo" are mutually exclusive. So not(feature = "gecko") and feature = "servo" are effectively the same. Use whatever you please :)
|
|
|
Looks good! Just answered your questions and pointed out a nit that makes the code a bit cleaner. |
| /// Servo variant. | ||
| /// https://drafts.csswg.org/css-ui/#cursor | ||
| #[cfg(not(feature = "gecko"))] | ||
| #[derive(Clone, Debug, MallocSizeOf, PartialEq, ToComputedValue, ToCss)] |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 18, 2018
Member
Just derive Copy here, then you can avoid all the borrow vs. clone issues :)
| @@ -94,12 +93,13 @@ define_cursor! { | |||
| "all-scroll" => AllScroll = 32, | |||
| "zoom-in" => ZoomIn = 33, | |||
| "zoom-out" => ZoomOut = 34, | |||
| "auto" => Auto = 35, | |||
This comment has been minimized.
This comment has been minimized.
emilio
Jan 18, 2018
Member
I don't see them used anywhere, so we could even remove them and simplify the macro, I'd think.
| Cursor::Alias => return cef_cursor_type_t::CT_ALIAS, | ||
| Cursor::Text => return cef_cursor_type_t::CT_IBEAM, | ||
| Cursor::Grab | Cursor::AllScroll => | ||
| CursorKind::None => return cef_cursor_type_t::CT_NONE, |
This comment has been minimized.
This comment has been minimized.
| Cursor::AllScroll => MouseCursor::AllScroll, | ||
| Cursor::ZoomIn => MouseCursor::ZoomIn, | ||
| Cursor::ZoomOut => MouseCursor::ZoomOut, | ||
| CursorKind::Auto => MouseCursor::Default, |
This comment has been minimized.
This comment has been minimized.
|
Updated, rebased, and squashed. |
f8dab51
to
e331e3c
|
Looks good! I just noticed a tiny inconsistency, but we can land this and address it in a followup, just let me know. Thanks for working on this! |
| /// The specified value for the `cursor` property. | ||
| /// | ||
| /// https://drafts.csswg.org/css-ui/#cursor | ||
| pub use values::computed::pointing::Cursor; |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 19, 2018
Member
Usually we do it the other way around (define it in specified, re-export it from computed).
It also feels a bit weird to have ToCss in a different place than Parse for the same type.
Sorry for not having caught this before, the patch looks great otherwise. If you don't want to address it is fine, can be a followup or something.
This comment has been minimized.
This comment has been minimized.
gootorov
Jan 19, 2018
Author
Contributor
No problem. No need to open another issue, I'll change it in this patch today! :)
| #[derive(Clone, Debug, MallocSizeOf, PartialEq, ToComputedValue)] | ||
| pub struct Cursor { | ||
| /// The parsed images for the cursor. | ||
| pub images: Vec<CursorImage>, |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 19, 2018
Member
We should also file followups for:
- Converting
imagestoBox<[CursorImage]>. - Derive
ToCss(I think it can be done now using#[css(comma)]on theimagesfield.
Would you mind filing those issues? I can do that myself if not otherwise.
This comment has been minimized.
This comment has been minimized.
emilio
Jan 19, 2018
Member
After trying to do that, it apparently is not possible as of right now.
You'd need to tweak style_derive, and the code I had to add to handle #[css(iterable, comma)] on field was more than the serialization code itself, so there's no need to do the derive thing.
I'll make all necessary changes within this PR today, no need to open other issues. |
Awesome, thank you! |
c0f42b8
to
e9edc70
|
|
|
LGTM with the test fixed, and the nit. |
| "Cursor", | ||
| "computed::Cursor::auto()", | ||
| initial_specified_value="specified::Cursor::auto()", | ||
| boxed=product == "gecko", |
This comment has been minimized.
This comment has been minimized.
| _context: &ParserContext, | ||
| input: &mut Parser<'i, 't> | ||
| ) -> Result<Self, ParseError<'i>> { | ||
| #[allow(unused_imports)] use std::ascii::AsciiExt; |
This comment has been minimized.
This comment has been minimized.
| #[allow(unused_imports)] use std::ascii::AsciiExt; | ||
| let location = input.current_source_location(); | ||
| let ident = input.expect_ident()?; | ||
| if ident.eq_ignore_ascii_case("auto") { |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 19, 2018
Member
if you remove this case completely, which should also be handled by from_css_keyword now.
| ) -> Result<Self, ParseError<'i>> { | ||
| Ok(Self { | ||
| url: SpecifiedUrl::parse(context, input)?, | ||
| hotspot: match input.try(|input| input.expect_number()) { |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 19, 2018
Member
This probably should use Number::parse, to handle calc(), but let's do this in a followup, since it also needs tests and all that.
But could you leave a comment like:
// FIXME(emilio): Should use Number::parse to handle calc() correctly.right above hotspot:?
This comment has been minimized.
This comment has been minimized.
|
Passes the test now. |
13ce507
to
98a1655
|
@CYBAI Thanks. |
|
ports/embedding is only built by Anyway, still LGTM. Thanks a lot @CYBAI for the catch! @bors-servo r+ |
|
|
style: Move cursor property out of mako <!-- Please describe your changes on the following line: --> Sub-PR of #19015 r? emilio --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19775 (github issue number if applicable). <!-- Either: --> - [x] These changes do not require tests <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19798) <!-- Reviewable:end -->
|
|
|
From 2018-01-20 02:03:36.456 rustc[27188:428515753] Metadata.framework [Error]: couldn't get the client port
error[E0433]: failed to resolve. Use of undeclared type or module `Cursor`
--> ports/cef/window.rs:148:36
|
148 | CursorKind::Grab | Cursor::AllScroll =>
| ^^^^^^ Use of undeclared type or module `Cursor`
error[E0433]: failed to resolve. Use of undeclared type or module `Cursor`
--> ports/cef/window.rs:150:38
|
150 | CursorKind::NoDrop | Cursor::NotAllowed =>
| ^^^^^^ Use of undeclared type or module `Cursor`
error[E0433]: failed to resolve. Use of undeclared type or module `Cursor`
--> ports/cef/window.rs:155:40
|
155 | CursorKind::EwResize | Cursor::ColResize =>
| ^^^^^^ Use of undeclared type or module `Cursor`
error[E0433]: failed to resolve. Use of undeclared type or module `Cursor`
--> ports/cef/window.rs:159:40
|
159 | CursorKind::NsResize | Cursor::RowResize =>
| ^^^^^^ Use of undeclared type or module `Cursor`
error[E0412]: cannot find type `Cursor` in this scope
--> ports/cef/window.rs:136:48
|
136 | fn cursor_handle_for_cursor(&self, cursor: Cursor) -> cef_cursor_handle_t {
| ^^^^^^ not found in this scope
help: possible candidates are found in other modules, you can import them into scope
|
12 | use servo::style::values::computed::Cursor;
|
12 | use servo::style::values::computed::pointing::Cursor;
|
12 | use servo::style::values::specified::Cursor;
|
12 | use servo::style::values::specified::pointing::Cursor;
|
and 1 other candidates
error: aborting due to 5 previous errors
error: Could not compile `embedding`.
To learn more, run the command again with --verbose. |
|
@CYBAI Thanks again!
@emilio Wasn't aware of that, good to know! I'm still able to compile it with Anyway, I fixed the errors, so hopefully it'll pass the tests now. In the meanwhile, I'll try to figure out why it builds for me with the incorrect code. |
4d90f0e
to
4ee9eb8
|
Ok, let's try again! :) |
| CursorKind::NsResize => cef_cursor_type_t::CT_NORTHSOUTHRESIZE, | ||
| CursorKind::RowResize => cef_cursor_type_t::CT_ROWRESIZE, | ||
| CursorKind::VerticalText => cef_cursor_type_t::CT_VERTICALTEXT, | ||
| _ => cef_cursor_type_t::CT_POINTER, | ||
| } | ||
| } | ||
|
|
||
| /// Returns the Cocoa cursor for a CSS cursor. These match Firefox, except where Firefox | ||
| /// bundles custom resources (which we don't yet do). | ||
| #[cfg(target_os="macos")] |
This comment has been minimized.
This comment has been minimized.
emilio
Jan 20, 2018
Member
Yeah, this function is macos only, so if you're on Linux like I am you don't even build it.
|
@bors-servo r+ |
|
|
style: Move cursor property out of mako <!-- Please describe your changes on the following line: --> Sub-PR of #19015 r? emilio --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #19775 (github issue number if applicable). <!-- Either: --> - [x] These changes do not require tests <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19798) <!-- Reviewable:end -->
|
|
gootorov commentedJan 17, 2018
•
edited
Sub-PR of #19015
r? emilio
./mach build -ddoes not report any errors./mach build-geckolibdoes not report any errors./mach test-tidydoes not report any errorsThis change is