Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2605093002:
Negative margin lengths are legal (Closed)
|
Created:
3 years, 11 months ago by atotic Modified:
3 years, 11 months ago CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, glebl+reviews_chromium.org, jchaffraix+rendering, leviw+renderwatch, ojan+watch_chromium.org, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNegative margin lengths are legal
Fix for cbiesinger comment:
https://codereview.chromium.org/2568743005#msg16
Tnx to ikilpatrick for making me put these asserts for all code paths,
this bug got caught early.
BUG=635619
[ng_negmargin]
Committed: https://crrev.com/c31508114a74d26be7dd38bd08a047d1533f34df
Cr-Commit-Position: refs/heads/master@{#441268}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Remove DCHECKS #
Total comments: 2
Patch Set 3 : Reinstate size checks #Messages
Total messages: 24 (11 generated)
atotic@chromium.org changed reviewers: + cbiesinger@chromium.org
ptal
Description was changed from ========== Negative margin lengths are legal Fix for cbiesinger comment: https://codereview.chromium.org/2568743005#msg16 BUG=635619 ========== to ========== Negative margin lengths are legal Fix for cbiesinger comment: https://codereview.chromium.org/2568743005#msg16 Tnx to ikilpatrick for making me put these asserts for all code paths, this bug got caught early. BUG=635619 ==========
Fyi I won't be able to review this until the new year On Dec 28, 2016 10:31, <atotic@chromium.org> wrote: > Reviewers: cbiesinger > CL: https://codereview.chromium.org/2605093002/ > > Message: > ptal > > > > Description: > Negative margin lengths are legal > > Fix for cbiesinger comment: > https://codereview.chromium.org/2568743005#msg16 > > BUG=635619 > > Affected files (+4, -4 lines): > M third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > M third_party/WebKit/Source/core/layout/ng/ng_units.h > > > Index: third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > diff --git a/third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > b/third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > index a53ba2e33cf2f8f8af0686efcecad24061194b2e.. > b413f077431d206662778ac712681255836675fd 100644 > --- a/third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > +++ b/third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > @@ -27,8 +27,6 @@ enum class LengthResolveType { > kMarginBorderPaddingSize > }; > > -#define NGSizeIndefinite LayoutUnit(-1) > - > // Whether the caller needs to compute min-content and max-content sizes to > // pass them to ResolveInlineLength / ComputeInlineSizeForFragment. > // If this function returns false, it is safe to pass an empty > Index: third_party/WebKit/Source/core/layout/ng/ng_units.h > diff --git a/third_party/WebKit/Source/core/layout/ng/ng_units.h > b/third_party/WebKit/Source/core/layout/ng/ng_units.h > index 235bf802d0040eec8001b2adecbef7c87dfcccdb.. > 6bfc80892a1d42da72125102a516830c68e90751 100644 > --- a/third_party/WebKit/Source/core/layout/ng/ng_units.h > +++ b/third_party/WebKit/Source/core/layout/ng/ng_units.h > @@ -18,6 +18,8 @@ struct NGPhysicalOffset; > struct NGPhysicalSize; > struct NGBoxStrut; > > +#define NGSizeIndefinite LayoutUnit(-1) > + > struct CORE_EXPORT MinAndMaxContentSizes { > LayoutUnit min_content; > LayoutUnit max_content; > @@ -348,8 +350,8 @@ struct CORE_EXPORT NGStaticPosition { > LayoutUnit margin_end) const { > DCHECK_GE(container_size, LayoutUnit()); > DCHECK_GE(length, LayoutUnit()); > - DCHECK_GE(margin_start, LayoutUnit()); > - DCHECK_GE(margin_end, LayoutUnit()); > + DCHECK_NE(margin_start, NGSizeIndefinite); > + DCHECK_NE(margin_end, NGSizeIndefinite); > if (position_matches) > return position; > else > > > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Fyi I won't be able to review this until the new year On Dec 28, 2016 10:31, <atotic@chromium.org> wrote: > Reviewers: cbiesinger > CL: https://codereview.chromium.org/2605093002/ > > Message: > ptal > > > > Description: > Negative margin lengths are legal > > Fix for cbiesinger comment: > https://codereview.chromium.org/2568743005#msg16 > > BUG=635619 > > Affected files (+4, -4 lines): > M third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > M third_party/WebKit/Source/core/layout/ng/ng_units.h > > > Index: third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > diff --git a/third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > b/third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > index a53ba2e33cf2f8f8af0686efcecad24061194b2e.. > b413f077431d206662778ac712681255836675fd 100644 > --- a/third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > +++ b/third_party/WebKit/Source/core/layout/ng/ng_length_utils.h > @@ -27,8 +27,6 @@ enum class LengthResolveType { > kMarginBorderPaddingSize > }; > > -#define NGSizeIndefinite LayoutUnit(-1) > - > // Whether the caller needs to compute min-content and max-content sizes to > // pass them to ResolveInlineLength / ComputeInlineSizeForFragment. > // If this function returns false, it is safe to pass an empty > Index: third_party/WebKit/Source/core/layout/ng/ng_units.h > diff --git a/third_party/WebKit/Source/core/layout/ng/ng_units.h > b/third_party/WebKit/Source/core/layout/ng/ng_units.h > index 235bf802d0040eec8001b2adecbef7c87dfcccdb.. > 6bfc80892a1d42da72125102a516830c68e90751 100644 > --- a/third_party/WebKit/Source/core/layout/ng/ng_units.h > +++ b/third_party/WebKit/Source/core/layout/ng/ng_units.h > @@ -18,6 +18,8 @@ struct NGPhysicalOffset; > struct NGPhysicalSize; > struct NGBoxStrut; > > +#define NGSizeIndefinite LayoutUnit(-1) > + > struct CORE_EXPORT MinAndMaxContentSizes { > LayoutUnit min_content; > LayoutUnit max_content; > @@ -348,8 +350,8 @@ struct CORE_EXPORT NGStaticPosition { > LayoutUnit margin_end) const { > DCHECK_GE(container_size, LayoutUnit()); > DCHECK_GE(length, LayoutUnit()); > - DCHECK_GE(margin_start, LayoutUnit()); > - DCHECK_GE(margin_end, LayoutUnit()); > + DCHECK_NE(margin_start, NGSizeIndefinite); > + DCHECK_NE(margin_end, NGSizeIndefinite); > if (position_matches) > return position; > else > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
atotic@chromium.org changed reviewers: + ikilpatrick@chromium.org
Adding ian as reviewer, just in case you are around.
mstensho@opera.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/2605093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_units.h (right): https://codereview.chromium.org/2605093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_units.h:353: DCHECK_NE(margin_start, NGSizeIndefinite); But -1px is also a valid margin value. Maybe NGSizeIndefinite should be defined as LayoutUnit::min() or something like that instead?
Description was changed from ========== Negative margin lengths are legal Fix for cbiesinger comment: https://codereview.chromium.org/2568743005#msg16 Tnx to ikilpatrick for making me put these asserts for all code paths, this bug got caught early. BUG=635619 ========== to ========== Negative margin lengths are legal Fix for cbiesinger comment: https://codereview.chromium.org/2568743005#msg16 Tnx to ikilpatrick for making me put these asserts for all code paths, this bug got caught early. BUG=635619 [ng_negmargin] ==========
https://codereview.chromium.org/2605093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_units.h (right): https://codereview.chromium.org/2605093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_units.h:353: DCHECK_NE(margin_start, NGSizeIndefinite); On 2016/12/29 10:28:39, mstensho wrote: > But -1px is also a valid margin value. Maybe NGSizeIndefinite should be defined > as LayoutUnit::min() or something like that instead? More to the point -- do we ever use -1 / NGSizeIndefinite for margin values? Maybe these two margin DCHECKs are not that useful? I would hesitate to make NGSizeIndefinite different from the legacy-layout practice of using -1 for invalid.
ptal. Removed DCHECK completely, since all LayoutUnits for margins are valid. https://codereview.chromium.org/2605093002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_units.h (right): https://codereview.chromium.org/2605093002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_units.h:353: DCHECK_NE(margin_start, NGSizeIndefinite); On 2016/12/31 at 17:29:49, cbiesinger wrote: > On 2016/12/29 10:28:39, mstensho wrote: > > But -1px is also a valid margin value. Maybe NGSizeIndefinite should be defined > > as LayoutUnit::min() or something like that instead? > > More to the point -- do we ever use -1 / NGSizeIndefinite for margin values? Maybe these two margin DCHECKs are not that useful? > > I would hesitate to make NGSizeIndefinite different from the legacy-layout practice of using -1 for invalid. I had a bad assumption that NGSizeIndefinite was equivaled to 0xFFFF. I'll remove the DCHECKs here because these checks are not useful if there is no obvious "uninitialized" value for LayoutUnit. In other abspos code, I will only use NGSizeIndefinite to indicate unitialized for size variables. Others will have to use Optional<LayoutUnit> instead. I would have found it useful to have a way to specify uninitialized LayoutUnit. Unfortunatelly, there is not way to do this.
> I had a bad assumption that NGSizeIndefinite was equivaled to 0xFFFF. Technically it is :) https://codereview.chromium.org/2605093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.h (left): https://codereview.chromium.org/2605093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_units.h:350: DCHECK_GE(length, LayoutUnit()); You can keep the container_size and length DCHECKs, no? Those should not be negative/NGSizeIndefinite, right?
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
ptal https://codereview.chromium.org/2605093002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_units.h (left): https://codereview.chromium.org/2605093002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_units.h:350: DCHECK_GE(length, LayoutUnit()); On 2017/01/03 at 20:35:19, cbiesinger wrote: > You can keep the container_size and length DCHECKs, no? Those should not be negative/NGSizeIndefinite, right? done.
lgtm
The CQ bit was checked by atotic@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1483478973155060,
"parent_rev": "119edfba17fe044fc5545a18b9691bb917a2f11b", "commit_rev":
"b109d727ca85e767b954dea542e2800ad606fba7"}
Message was sent while issue was closed.
Description was changed from ========== Negative margin lengths are legal Fix for cbiesinger comment: https://codereview.chromium.org/2568743005#msg16 Tnx to ikilpatrick for making me put these asserts for all code paths, this bug got caught early. BUG=635619 [ng_negmargin] ========== to ========== Negative margin lengths are legal Fix for cbiesinger comment: https://codereview.chromium.org/2568743005#msg16 Tnx to ikilpatrick for making me put these asserts for all code paths, this bug got caught early. BUG=635619 [ng_negmargin] Review-Url: https://codereview.chromium.org/2605093002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Negative margin lengths are legal Fix for cbiesinger comment: https://codereview.chromium.org/2568743005#msg16 Tnx to ikilpatrick for making me put these asserts for all code paths, this bug got caught early. BUG=635619 [ng_negmargin] Review-Url: https://codereview.chromium.org/2605093002 ========== to ========== Negative margin lengths are legal Fix for cbiesinger comment: https://codereview.chromium.org/2568743005#msg16 Tnx to ikilpatrick for making me put these asserts for all code paths, this bug got caught early. BUG=635619 [ng_negmargin] Committed: https://crrev.com/c31508114a74d26be7dd38bd08a047d1533f34df Cr-Commit-Position: refs/heads/master@{#441268} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c31508114a74d26be7dd38bd08a047d1533f34df Cr-Commit-Position: refs/heads/master@{#441268} |
