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

style: Cleanup the cascade a good bit. #17788

Merged
merged 1 commit into from Jul 20, 2017

Conversation

Projects
None yet
5 participants
@emilio
Copy link
Member

commented Jul 20, 2017

Was about the time.


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Jul 20, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/servo/media_queries.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/gecko.mako.rs, components/style/values/specified/length.rs, components/style/stylesheets/viewport_rule.rs and 13 more
  • @canaltinova: components/style/servo/media_queries.rs, components/style/properties/longhand/inherited_text.mako.rs, components/style/properties/gecko.mako.rs, components/style/values/specified/length.rs, components/style/stylesheets/viewport_rule.rs and 13 more
@emilio

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@highfive highfive assigned heycam and unassigned cbrewster Jul 20, 2017

@emilio

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

⌛️ Trying commit 77f5f26 with merge 229481c...

bors-servo added a commit that referenced this pull request Jul 20, 2017

Auto merge of #17788 - emilio:clean-cascade, r=<try>
style: Cleanup the cascade a good bit.

Was about the time.

<!-- 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/17788)
<!-- Reviewable:end -->
@emilio

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

As a followup, we should really get that font-size stuff on Gecko cleaned up...

@emilio

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@heycam
Copy link
Member

left a comment

These changes look good! Just a question about the animation-related FFI function changes.

reset_style: &'a ComputedValues,

/// The style we're inheriting from explicitly, or none if we're the root of
/// a subtree.

This comment has been minimized.

Copy link
@heycam

heycam Jul 20, 2017

Member

I think the comments here don't really explain the difference between inherited_style and parent_style. Can you expand on the comments?

This comment has been minimized.

Copy link
@emilio

emilio Jul 20, 2017

Author Member

Will do.

}

/// Returns the style we're getting reset properties from.
pub fn reset_style(&self) -> &'a ComputedValues {

This comment has been minimized.

Copy link
@heycam

heycam Jul 20, 2017

Member

reset_style and reset_[prop] have similar name but do different things. Can we rename one, maybe reset_style can be initial_style or default_style?

This comment has been minimized.

Copy link
@emilio

emilio Jul 20, 2017

Author Member

Agreed, I used default_style for consistency with computed::Context.

flags
);
StyleAdjuster::new(&mut builder)
.adjust(layout_parent_style, flags);

This comment has been minimized.

Copy link
@heycam

heycam Jul 20, 2017

Member

I guess this doesn't need to be in a block.

let pseudo = style.pseudo();
let mut context = create_context(&data, &metrics, style, parent_style, pseudo.as_ref());
let mut context = create_context(&data, &metrics, style, pseudo.as_ref());

This comment has been minimized.

Copy link
@heycam

heycam Jul 20, 2017

Member

Are you sure these animation-related function changes are right? For Servo_GetAnimationValue, at least, which is used for SMIL, it seems like Gecko will indeed use the element's parent style if inherit was given.

This comment has been minimized.

Copy link
@emilio

emilio Jul 20, 2017

Author Member

Yeah, this is quite fishy indeed, will fix.

@@ -2908,7 +2903,7 @@ fn create_context<'a>(per_doc_data: &'a PerDocumentStyleDataImpl,

#[no_mangle]
pub extern "C" fn Servo_GetComputedKeyframeValues(keyframes: RawGeckoKeyframeListBorrowed,
element: RawGeckoElementBorrowed,
_element: RawGeckoElementBorrowed,

This comment has been minimized.

Copy link
@heycam

heycam Jul 20, 2017

Member

Remove the argument (maybe in a followup given this means you need to touch C++ too)?

@emilio emilio force-pushed the emilio:clean-cascade branch from 77f5f26 to e374a54 Jul 20, 2017

@emilio

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2017

@bors-servo r=heycam p=1

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

📌 Commit e374a54 has been approved by heycam

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

⌛️ Testing commit e374a54 with merge f594ae5...

bors-servo added a commit that referenced this pull request Jul 20, 2017

Auto merge of #17788 - emilio:clean-cascade, r=heycam
style: Cleanup the cascade a good bit.

Was about the time.

<!-- 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/17788)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2017

@bors-servo bors-servo merged commit e374a54 into servo:master Jul 20, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@emilio emilio deleted the emilio:clean-cascade branch Jul 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.