Bug 186217 - [LFC] Merge width and horizontal margin computation
Summary: [LFC] Merge width and horizontal margin computation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-01 18:20 PDT by zalan
Modified: 2018-06-04 07:45 PDT (History)
6 users (show)

See Also:


Attachments
Patch (31.52 KB, patch)
2018-06-01 18:30 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (31.56 KB, patch)
2018-06-01 18:31 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (36.33 KB, patch)
2018-06-01 20:40 PDT, zalan
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2018-06-01 18:20:41 PDT
to be able to closely follow the steps defined at https://www.w3.org/TR/CSS22/visudet.html
Comment 1 zalan 2018-06-01 18:30:14 PDT
Created attachment 341813 [details]
Patch
Comment 2 zalan 2018-06-01 18:31:57 PDT
Created attachment 341814 [details]
Patch
Comment 3 zalan 2018-06-01 18:43:29 PDT
Comment on attachment 341814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341814&action=review

> Source/WebCore/layout/displaytree/DisplayBox.h:175
> +    struct EdgePair {
> +        LayoutUnit first;
> +        LayoutUnit second;
> +    };
> +    using HorizontalEdges = EdgePair;
> +    using VerticalEdges = EdgePair;
>  
> +    struct Edges {
>          LayoutUnit top;
>          LayoutUnit left;
>          LayoutUnit bottom;
>          LayoutUnit right;
>      };

Edges could be defined like this 
struct Edges {
    HorizontalEdges horizontal;
    VerticalEdges vertical;
}; 
but then top would read like edges.vertical.first instead of edges.top. I find edges.top more readable.
Comment 4 Sam Weinig 2018-06-01 19:43:22 PDT
Comment on attachment 341814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341814&action=review

> Source/WebCore/ChangeLog:9
> +        at https://www.w3.org/TR/CSS22/visudet.html, we should just merge width and horizontal maring computations

s/maring/margin/

> Source/WebCore/layout/displaytree/DisplayBox.h:167
> +    struct EdgePair {
> +        LayoutUnit first;
> +        LayoutUnit second;
> +    };
> +    using HorizontalEdges = EdgePair;

Could EdgePair be defined as:

using EdgePair = std::pair<LayoutUnit, LayoutUnit>;

Though, actually, I think what might be the cleanest is the really verbose:

struct HorizontalEdges {
    LayoutUnit left;
    LayoutUnit right;
};
struct VerticalEdges {
    LayoutUnit top;
    LayoutUnit bottom;
};
struct Edges {
    HorizontalEdges horizontal;
    VerticalEdges vertical;
};
Comment 5 zalan 2018-06-01 19:48:56 PDT
(In reply to Sam Weinig from comment #4)
> Comment on attachment 341814 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341814&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        at https://www.w3.org/TR/CSS22/visudet.html, we should just merge width and horizontal maring computations
> 
> s/maring/margin/
> 
> > Source/WebCore/layout/displaytree/DisplayBox.h:167
> > +    struct EdgePair {
> > +        LayoutUnit first;
> > +        LayoutUnit second;
> > +    };
> > +    using HorizontalEdges = EdgePair;
> 
> Could EdgePair be defined as:
> 
> using EdgePair = std::pair<LayoutUnit, LayoutUnit>;
> 
> Though, actually, I think what might be the cleanest is the really verbose:
> 
> struct HorizontalEdges {
>     LayoutUnit left;
>     LayoutUnit right;
> };
> struct VerticalEdges {
>     LayoutUnit top;
>     LayoutUnit bottom;
> };
> struct Edges {
>     HorizontalEdges horizontal;
>     VerticalEdges vertical;
> };
That's exactly what I had as my first version but I though some people might find it too verbose. Thanks for the comment! I will gladly make this change!
Comment 6 zalan 2018-06-01 20:40:27 PDT
Created attachment 341823 [details]
Patch
Comment 7 Antti Koivisto 2018-06-04 07:35:57 PDT
Comment on attachment 341823 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341823&action=review

> Source/WebCore/ChangeLog:11
> +        Use 0 computed marings for now.

margins
Comment 8 zalan 2018-06-04 07:44:05 PDT
Committed r232465: <https://trac.webkit.org/changeset/232465>
Comment 9 Radar WebKit Bug Importer 2018-06-04 07:45:21 PDT
<rdar://problem/40771393>