servo / servo

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

Properly account for relative CSS units in borders, margins, padding, an... #602

Merged

bors-servo merged 5 commits into servo:master from metajack:relative-bpm

Jul 19, 2013

Contributor

metajack commented Jul 19, 2013

...d widths.


        Properly account for relative CSS units in borders, margins, padding,…

9a1d4d5

… and widths.

pcwalton reviewed

View changes

src/components/main/layout/box.rs Outdated

  
                        let br = base.model.compute_border_width(self.style().border_right_width());

                        let style = self.style();

                        let font_size = style.font_size();

                        let w = MaybeAuto::from_width(style.width(),

pcwalton Jul 19, 2013

Contributor

I'm not really a fan of these one- or two-letter variable names, especially in layout code which needs to be clear.

pcwalton reviewed

View changes

src/components/main/layout/box.rs Outdated

  
                        let font_size = style.font_size();

                        let w = MaybeAuto::from_width(style.width(),

                                                      Au(0),

                                                      font_size).spec_or_default(Au(0));

pcwalton Jul 19, 2013

Contributor

Maybe it's worth having a spec_or_zero() here that uses the Zero trait? (I don't actually know what spec means: specified I guess? Another naming nit.)

pcwalton reviewed

View changes

src/components/main/layout/model.rs Outdated

  
                    }

                    CSSPaddingLength(Px(v)) => Au::from_frac_px(v),

                    CSSPaddingLength(Pt(v)) => Au::from_pt(v),

                    CSSPaddingLength(Em(em)) => match font_size {

pcwalton Jul 19, 2013

Contributor

nit: brace all multi-line pattern arms.

pcwalton reviewed

View changes

src/components/main/layout/model.rs Outdated

  
                    }

                    CSSBorderWidthLength(Px(v)) => Au::from_frac_px(v),

                    CSSBorderWidthLength(Pt(v)) => Au::from_pt(v),

                    CSSBorderWidthLength(Em(em)) => match font_size {

pcwalton Jul 19, 2013

Contributor

nit: brace all multi-line pattern arms.

pcwalton reviewed

View changes

src/components/main/layout/model.rs Outdated

  
                self.padding.right = self.compute_padding_length(style.padding_right(), cb_width);

                self.padding.bottom = self.compute_padding_length(style.padding_bottom(), cb_width);

                self.padding.left = self.compute_padding_length(style.padding_left(), cb_width);

            pub fn compute_padding(&mut self, style: CompleteStyle, cb_width: Au) {

pcwalton Jul 19, 2013

Contributor

nit: what is cb here? Containing box? Abbreviations in layout code are confusing :(

pcwalton reviewed

View changes

src/components/main/layout/model.rs Outdated

  
                    CSSWidthLength(Em(v)) => Specified(Au::from_frac_px(v)),

                    CSSWidthLength(Px(v)) => Specified(Au::from_frac_px(v)),

                    CSSWidthLength(Pt(v)) => Specified(Au::from_pt(v)),

                    CSSWidthLength(Em(em)) => match font_size {

pcwalton Jul 19, 2013

Contributor

nit: brace all multi-line pattern arms.

pcwalton reviewed

View changes

src/components/main/layout/model.rs Outdated

  
                }

            }

            pub fn from_width(width: CSSWidth, cb_width: Au) -> MaybeAuto{

                match width{

            pub fn from_width(width: CSSWidth, cb_width: Au, font_size: CSSFontSize) -> MaybeAuto {

pcwalton Jul 19, 2013

Contributor

nit: as below I think cb_width is confusing. I guess it's "containing box", not "content box"?

metajack added 4 commits


        Renaming and refactoring `spec_or_default`.

4d1e21b

`spec_or_default` is now `specified_or_default` and `specified_or_zero` was
added to handle the most common case.


        Brace multi-line match patterns per coding style.

b2c042d


        Rename `cb_width` to `containing_width`.

e10b362


        Rename variables for clarity.

11af5ff

pcwalton commented on `11af5ff` Jul 19, 2013

r+

Contributor

bors-servo commented on `11af5ff` Jul 19, 2013

saw approval from pcwalton
at metajack@11af5ff

Contributor

bors-servo replied Jul 19, 2013

merging metajack/servo/relative-bpm = 11af5ff into auto

Contributor

bors-servo replied Jul 19, 2013

metajack/servo/relative-bpm = 11af5ff merged ok, testing candidate = b68b573

Contributor

bors-servo replied Jul 19, 2013

all tests pass:
http://servo-buildbot.pub.build.mozilla.org/builders/linux/builds/144
http://servo-buildbot.pub.build.mozilla.org/builders/mac/builds/99

Contributor

bors-servo replied Jul 19, 2013

fast-forwarding master to auto = b68b573

bors-servo pushed a commit that referenced this pull request


        auto merge of #602 : metajack/servo/relative-bpm, r=pcwalton

b68b573

...d widths.

bors-servo merged commit 11af5ff into servo:master

1 check passed

1 check passed

default all tests passed

glennw pushed a commit to glennw/servo that referenced this pull request


        Auto merge of servo#602 - nical:more-units-api, r=glennw

b5f63b3

Use strongly typed geometry - Part 2

Use strongly typed geometry in the public API. This changeset adds the LayoutPixel unit which is basically an alias to LayerPixel.
I think that it's best to keep the "Layer" terminology internal to webrender, and since the 1-1 correspondence between the api's layout pixels and internal layer pixels is somewhat coincidental, it will help to have separate names if things like async zoom introduce an actual difference between the two coordinate spaces (as it does in Gecko). Using an alias instead of a separate type comes from a mix of laziness (not having to cast from layout t layer pixels all over frame.rs) and the fact that currently layer and layout pixels are the same thing, but I'll add a separate unit if there is a preference for it.

I did not introduce ParentLayoutPixel. I don't know the API well enough yet to be sure whether some geometry is passed in a stacking context's parent coordinate space, but if so we should consider introducing a special unit for it, if only for the sake of proper documentation.

This PR is a lot easier to rebase than part 1 and is a breaking change to the public API, so it's fine to wait a bit if there are cross-crates changes that we want to coordinate before having to adapt servo to this (although it should be easy to do).

<!-- 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/602)
<!-- Reviewable:end -->

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.