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: Document and force documentation in a big chunk of the style crate. #14801

Merged
merged 19 commits into from Dec 31, 2016

Conversation

@emilio
Copy link
Member

commented Dec 31, 2016

Style no forced docs for the properties code and similar, but I ran out of time, and I think it's a nice improvement.

I'd appreciate a fast-ish turn-around time because this is pretty much prone to bitrot.


This change is Reviewable

emilio added 3 commits Dec 31, 2016
Since they do the same.
@highfive

This comment has been minimized.

Copy link

commented Dec 31, 2016

Heads up! This PR modifies the following files:

  • @bholley: components/style/values/computed/position.rs, components/style/values/specified/basic_shape.rs, components/style/data.rs, components/style/properties/longhand/background.mako.rs, components/style/values/computed/mod.rs, components/style/servo/selector_parser.rs, components/style/traversal.rs, components/style/values/specified/length.rs, components/style/properties/properties.mako.rs, components/style/animation.rs, components/style/lib.rs, components/style/values/specified/image.rs, components/style/str.rs, components/style/values/computed/basic_shape.rs, components/style/rule_tree/mod.rs, components/style/properties/longhand/font.mako.rs, components/style/viewport.rs, components/style/values/mod.rs, components/style/cache.rs, components/style/values/specified/url.rs, components/style/values/computed/length.rs, components/style/values/specified/mod.rs, components/style/values/computed/image.rs, components/style/thread_state.rs, components/style/properties/declaration_block.rs, components/style/dom.rs, components/style/properties/longhand/box.mako.rs, components/style/timer.rs, components/style/parallel.rs, components/style/properties/shorthand/serialize.mako.rs, components/style/stylesheets.rs, components/style/sequential.rs, components/style/properties/helpers.mako.rs, components/style/properties/helpers/animated_properties.mako.rs, components/style/values/specified/position.rs, components/style/values/specified/grid.rs
@highfive

This comment has been minimized.

Copy link

commented Dec 31, 2016

warning Warning warning

  • These commits modify style and layout code, but no tests are modified. Please consider adding a test!
@emilio

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2016

Mat said he was able to review this, thanks!

@bors-servo r? @mbrubeck

@highfive highfive assigned mbrubeck and unassigned metajack Dec 31, 2016
Copy link
Contributor

left a comment

Awesome work! See the comments for some very minor copy-editing. Can land with r=mbrubeck.

pub fn importance(&self) -> Importance {
self.get().importance
}

/// Get an iterator for this rule node and its ancestors including.

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Dec 31, 2016

Contributor

"including" is dangling here. Should maybe be removed or changed to "inclusive."

/// This is organized as a tree of rules. When a node matches a set of rules,
/// they're inserted in order in the tree, starting with the less specific one.
///
/// When a rule is inserted in the tree, other element may share the path up to

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Dec 31, 2016

Contributor

s/element/elements/

/// them.
///
/// When the rule node refcount drops to zero, it doesn't get freed. It gets
/// instead being put into a free list, and it is potentially GC'd after a

This comment has been minimized.

Copy link
@mbrubeck

mbrubeck Dec 31, 2016

Contributor

"being" can be removed

Copy link
Member

left a comment

reviewed up till (not including) "style: Document a bit better the rule tree code." may pick up later if someone else doesn't.

pub struct InnerMatrix2D {
pub m11: CSSFloat, pub m12: CSSFloat,
pub m21: CSSFloat, pub m22: CSSFloat,
}

/// A translation function.

This comment has been minimized.

Copy link
@Manishearth
@@ -158,7 +158,13 @@ pub mod shorthands {
<%include file="/shorthand/text.mako.rs" />
}

/// A module with all the code related to animated properties.
///
/// This needs to be loaded at least after all longhand modules, given they

This comment has been minimized.

Copy link
@Manishearth

Manishearth Dec 31, 2016

Member

might want to clarify that "loaded" has to do with mako eval order

@@ -405,7 +413,11 @@
}

impl<'a> LonghandsToSerialize<'a> {
pub fn from_iter<I: Iterator<Item=&'a PropertyDeclaration>>(iter: I) -> Result<Self, ()> {
/// Maybe gets a serializable set of longhands given a set of

This comment has been minimized.

Copy link
@Manishearth

Manishearth Dec 31, 2016

Member

"Tries to get"

@@ -477,7 +491,10 @@
}


pub fn parse(context: &ParserContext, input: &mut Parser,
/// Parse the given shorthand and return the result in the

This comment has been minimized.

Copy link
@Manishearth

Manishearth Dec 31, 2016

Member

"fill the result into"

impl ComputedValueAsSpecified for SpecifiedValue {}
/// The specified and computed value for overflow-y is a wrapper on top of
/// `overflow-x`, so we re-use the logic, but prevent errors from mistakenly
/// assign one to other.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Dec 31, 2016

Member

IMO we should actually be using the same value here. There's precedent for having typedefs.

///
/// Also, it gets a new display value, which is honored except when it's
/// `inline`.
/// values reseted new display, and also

This comment has been minimized.

Copy link
@Manishearth

Manishearth Dec 31, 2016

Member

and also...?

pub const FONT_MEDIUM_PX: i32 = 16;

/// A trait used to represent whether this value has viewport units.

This comment has been minimized.

Copy link
@Manishearth

Manishearth Dec 31, 2016

Member

s/represent/query

fn has_viewport_percentage(&self) -> bool;
}

/// A trait used as a marker to represent that a given value has no viewport

This comment has been minimized.

Copy link
@Manishearth

Manishearth Dec 31, 2016

Member

"given type may never contain viewport units"

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

@bors-servo p=12917

@@ -55,8 +68,8 @@ impl FontRelativeLength {
None
}

// NB: The use_inherited flag is used to special-case the computation of
// font-family.
/// Computes the font-relative length. We use the use_inherited flag is used

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Dec 31, 2016

Member

"We use the use_inherited flag is used" ?

///
/// TODO(emilio): Tell bholley to document this more accurately. I can try (and
/// the fields are certainly mostly self-explanatory), but it's better if you
/// do, to avoid any misconception.

This comment has been minimized.

Copy link
@wafflespeanut

wafflespeanut Dec 31, 2016

Member

TODO(emilio): Tell bholley to document this more accurately. I can try (and fields are certainly mostly self-explanatory), but it's better if you do, to avoid any misconception.

Maybe change this to,

TODO(emilio, bholley): This should be documented more accurately. Though the fields are self-explanatory, we should avoid any misconception.

emilio added 16 commits Dec 31, 2016
@emilio emilio force-pushed the emilio:no-missing-docs branch from bb5bee5 to a847f26 Dec 31, 2016
@emilio

This comment has been minimized.

Copy link
Member Author

commented Dec 31, 2016

Thanks for the review! I've addressed the copy-editing comments.

Going to land this as-is, document the pending stuff, then make missing_docs by default, probably leaving the properties module out for now.

@bors-servo r=mbrubeck,Manishearth,Wafflespeanut

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2016

📌 Commit a847f26 has been approved by mbrubeck,Manishearth,Wafflespeanut

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2016

⌛️ Testing commit a847f26 with merge bd67163...

bors-servo added a commit that referenced this pull request Dec 31, 2016
…,Wafflespeanut

style: Document and force documentation in a big chunk of the style crate.

Style no forced docs for the properties code and similar, but I ran out of time, and I think it's a nice improvement.

I'd appreciate a fast-ish turn-around time because this is pretty much prone to bitrot.

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

This comment has been minimized.

@bors-servo bors-servo merged commit a847f26 into servo:master Dec 31, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.