Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2691093004:
Fix orthogonal mode legacy mismatch (Closed)
|
Created:
3 years, 10 months ago by atotic Modified:
3 years, 10 months ago Reviewers:
cbiesinger CC:
atotic+reviews_chromium.org, blink-reviews, blink-reviews-layout_chromium.org, cbiesinger, chromium-reviews, dgrogan+ng_chromium.org, 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. |
DescriptionFix orthogonal mode legacy mismatch
Orthogonal styles currently assert inside legacy layout, because width is -1.
Ex:
<div style="writing-mode:vertical-rl; border:1px solid red;">
<p style="writing-mode:horizontal-tb; border:1px solid blue;">Hello, world.</p>
</div>
These two routines did not handle orthogonal modes properly.
NGConstraintSpace::CreateFromLayoutObject
NGBlockNode::RunOldLayout
I am not sure if this fix is correct, it is my best guess. With the fix, assert
is avoided, but <p> is too tall. The constraint space used for layout looks correct,
bug might be something else.
Meta question: Should we bother with legacy orthogonal modes at all?
I've stumbled upon several other orthogonal mode bugs.
BUG=635619
[ng_orthogonal_legacy]
Review-Url: https://codereview.chromium.org/2691093004
Cr-Commit-Position: refs/heads/master@{#450878}
Committed: https://chromium.googlesource.com/chromium/src/+/283cef0d240cf6a118f605d396eee0fbee0278b7
Patch Set 1 #
Total comments: 16
Patch Set 2 : CR fixes #
Total comments: 2
Patch Set 3 : CR fixes, removed odd IsParallelWritingMode #
Messages
Total messages: 21 (10 generated)
Description was changed from ========== Fix ortohogonal mode legacy mismatch BUG=635619 ========== to ========== Fix ortohogonal mode legacy mismatch Orthogonal styles currently assert inside legacy layout, because width is -1. Ex: <div style="writing-mode:vertical-rl; border:1px solid red;"> <p style="writing-mode:horizontal-tb; border:1px solid blue;">Hello, world.</p> </div> These two routines did not handle orthogonal modes properly. NGConstraintSpace::CreateFromLayoutObject NGBlockNode::RunOldLayout I am not sure if this fix is correct, it is my best guess. With the fix, assert is avoided, but <p> is too tall. The constraint space used for layout looks correct, bug might be something else. Meta question: - Should we bother with legacy orthogonal modes at all? I've stumbled upon several other orthogonal mode bugs, seems like a bug minefield. BUG=635619 [ng_orthogonal_legacy] ==========
atotic@chromium.org changed reviewers: + cbiesinger@chromium.org
Description was changed from ========== Fix ortohogonal mode legacy mismatch Orthogonal styles currently assert inside legacy layout, because width is -1. Ex: <div style="writing-mode:vertical-rl; border:1px solid red;"> <p style="writing-mode:horizontal-tb; border:1px solid blue;">Hello, world.</p> </div> These two routines did not handle orthogonal modes properly. NGConstraintSpace::CreateFromLayoutObject NGBlockNode::RunOldLayout I am not sure if this fix is correct, it is my best guess. With the fix, assert is avoided, but <p> is too tall. The constraint space used for layout looks correct, bug might be something else. Meta question: - Should we bother with legacy orthogonal modes at all? I've stumbled upon several other orthogonal mode bugs, seems like a bug minefield. BUG=635619 [ng_orthogonal_legacy] ========== to ========== Fix ortohogonal mode legacy mismatch Orthogonal styles currently assert inside legacy layout, because width is -1. Ex: <div style="writing-mode:vertical-rl; border:1px solid red;"> <p style="writing-mode:horizontal-tb; border:1px solid blue;">Hello, world.</p> </div> These two routines did not handle orthogonal modes properly. NGConstraintSpace::CreateFromLayoutObject NGBlockNode::RunOldLayout I am not sure if this fix is correct, it is my best guess. With the fix, assert is avoided, but <p> is too tall. The constraint space used for layout looks correct, bug might be something else. Meta question: Should we bother with legacy orthogonal modes at all? I've stumbled upon several other orthogonal mode bugs. BUG=635619 [ng_orthogonal_legacy] ==========
https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:292: // OverrideContainingBlock should be in containing block writing mode. Huh, that seems to be true, I did not think it was. We really need to switch those to physical dimensions at some point. See TODO at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:56: writing_mode); If you like you can just do: bool parallel_containing_block = box.styleRef().isHorizontalWritingMode() == box.containingBlock()->styleRef().isHorizontalWritingMode() That said, are you sure that containingBlock() can't be null here? https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:59: // XXX for orthogonal writing mode this is not right I think you can remove this comment now? https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:71: if (parallel_containing_block) Please add {} for a multi-line if body (and also to the else branch) https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:23: CORE_EXPORT bool IsParallelWritingMode(NGWritingMode a, NGWritingMode b); If you agree with my suggestion above, you can remove this function. If you keep it, please switch the other code that does this check manually to use this function (in either this or a new cl) https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:25: CORE_EXPORT bool IsParallelWritingMode(WritingMode a, WritingMode b); I don't think you should have this function in NG code, since it purely operates on legacy writing modes. But you're not using it anyway, right?
ptal. Once the IsParallelWritingMode format is fixed, I'll convert other existing code. https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:292: // OverrideContainingBlock should be in containing block writing mode. On 2017/02/14 at 16:50:27, cbiesinger wrote: > Huh, that seems to be true, I did not think it was. We really need to switch those to physical dimensions at some point. See TODO at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... Saw that TODO. Who's lajava, and is this actively being worked on? https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:56: writing_mode); On 2017/02/14 at 16:50:27, cbiesinger wrote: > If you like you can just do: > > bool parallel_containing_block = box.styleRef().isHorizontalWritingMode() == box.containingBlock()->styleRef().isHorizontalWritingMode() > > That said, are you sure that containingBlock() can't be null here? You are right, containingBlock can be null. I've added another utility IsParallelWritingMode routine that takes care of all of this. https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:59: // XXX for orthogonal writing mode this is not right On 2017/02/14 at 16:50:27, cbiesinger wrote: > I think you can remove this comment now? done https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:71: if (parallel_containing_block) On 2017/02/14 at 16:50:27, cbiesinger wrote: > Please add {} for a multi-line if body (and also to the else branch) done https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:23: CORE_EXPORT bool IsParallelWritingMode(NGWritingMode a, NGWritingMode b); On 2017/02/14 at 16:50:27, cbiesinger wrote: > If you agree with my suggestion above, you can remove this function. > > If you keep it, please switch the other code that does this check manually to use this function (in either this or a new cl) Before doing this, I just want to get an ok on how IsParallelWritingMode defined. Since we are moving to class-based enums in blink, I'd love to redefine NGWritingMode as a class: class NGWritingMode { static const int kHorizontalTopBottom = 0; ..... static const FromPlatorm(....) static const IsParallel() } https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:25: CORE_EXPORT bool IsParallelWritingMode(WritingMode a, WritingMode b); On 2017/02/14 at 16:50:27, cbiesinger wrote: > I don't think you should have this function in NG code, since it purely operates on legacy writing modes. But you're not using it anyway, right? done. Removed.
ping
lgtm https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:292: // OverrideContainingBlock should be in containing block writing mode. On 2017/02/14 19:11:28, atotic wrote: > On 2017/02/14 at 16:50:27, cbiesinger wrote: > > Huh, that seems to be true, I did not think it was. We really need to switch > those to physical dimensions at some point. See TODO at > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > Saw that TODO. Who's lajava, and is this actively being worked on? He's jfernandez@igalia. I decided to work on that TODO now. https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:23: CORE_EXPORT bool IsParallelWritingMode(NGWritingMode a, NGWritingMode b); On 2017/02/14 19:11:28, atotic wrote: > On 2017/02/14 at 16:50:27, cbiesinger wrote: > > If you agree with my suggestion above, you can remove this function. > > > > If you keep it, please switch the other code that does this check manually to > use this function (in either this or a new cl) > > Before doing this, I just want to get an ok on how IsParallelWritingMode > defined. > > Since we are moving to class-based enums in blink, I'd love to redefine > NGWritingMode as a class: > > class NGWritingMode { > static const int kHorizontalTopBottom = 0; > ..... > static const FromPlatorm(....) > static const IsParallel() > } I would rather not do that. enums are much nicer when debugging code, and also this is conceptually an enum, not a class. https://codereview.chromium.org/2691093004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h (right): https://codereview.chromium.org/2691093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:28: bool IsParallelWritingMode(const LayoutObject* layoutObject, Please add a comment for what this function does. Also, this seems fairly specialized, I'd just move it over to ng_constraint_space.cc -- are there any other potential callers?
https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:292: // OverrideContainingBlock should be in containing block writing mode. On 2017/02/15 17:52:24, cbiesinger wrote: > On 2017/02/14 19:11:28, atotic wrote: > > On 2017/02/14 at 16:50:27, cbiesinger wrote: > > > Huh, that seems to be true, I did not think it was. We really need to switch > > those to physical dimensions at some point. See TODO at > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/La... > > > > Saw that TODO. Who's lajava, and is this actively being worked on? > > He's jfernandez@igalia. I decided to work on that TODO now. Actually, I will not work on this right now because this turned out to be too complicated to do quickly, so I'm working on higher-priority issues instead for now.
Description was changed from ========== Fix ortohogonal mode legacy mismatch Orthogonal styles currently assert inside legacy layout, because width is -1. Ex: <div style="writing-mode:vertical-rl; border:1px solid red;"> <p style="writing-mode:horizontal-tb; border:1px solid blue;">Hello, world.</p> </div> These two routines did not handle orthogonal modes properly. NGConstraintSpace::CreateFromLayoutObject NGBlockNode::RunOldLayout I am not sure if this fix is correct, it is my best guess. With the fix, assert is avoided, but <p> is too tall. The constraint space used for layout looks correct, bug might be something else. Meta question: Should we bother with legacy orthogonal modes at all? I've stumbled upon several other orthogonal mode bugs. BUG=635619 [ng_orthogonal_legacy] ========== to ========== Fix orthogonal mode legacy mismatch Orthogonal styles currently assert inside legacy layout, because width is -1. Ex: <div style="writing-mode:vertical-rl; border:1px solid red;"> <p style="writing-mode:horizontal-tb; border:1px solid blue;">Hello, world.</p> </div> These two routines did not handle orthogonal modes properly. NGConstraintSpace::CreateFromLayoutObject NGBlockNode::RunOldLayout I am not sure if this fix is correct, it is my best guess. With the fix, assert is avoided, but <p> is too tall. The constraint space used for layout looks correct, bug might be something else. Meta question: Should we bother with legacy orthogonal modes at all? I've stumbled upon several other orthogonal mode bugs. BUG=635619 [ng_orthogonal_legacy] ==========
The CQ bit was checked by atotic@chromium.org
thanks for the review, and sorry about IsPlatformWritingMode torture. I've removed the IsPlatformWritingMode(LayoutObject, Style), and commented remaining function. Also converted a couple of other places to use IsParallelWritingMode. https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h (right): https://codereview.chromium.org/2691093004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:23: CORE_EXPORT bool IsParallelWritingMode(NGWritingMode a, NGWritingMode b); On 2017/02/15 at 17:52:24, cbiesinger wrote: > On 2017/02/14 19:11:28, atotic wrote: > > On 2017/02/14 at 16:50:27, cbiesinger wrote: > > > If you agree with my suggestion above, you can remove this function. > > > > > > If you keep it, please switch the other code that does this check manually to > > use this function (in either this or a new cl) > > > > Before doing this, I just want to get an ok on how IsParallelWritingMode > > defined. > > > > Since we are moving to class-based enums in blink, I'd love to redefine > > NGWritingMode as a class: > > > > class NGWritingMode { > > static const int kHorizontalTopBottom = 0; > > ..... > > static const FromPlatorm(....) > > static const IsParallel() > > } > > I would rather not do that. enums are much nicer when debugging code, and also this is conceptually an enum, not a class. I agree. I'd just love to find a way to keep those functions from pollting blink namespace. https://codereview.chromium.org/2691093004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h (right): https://codereview.chromium.org/2691093004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_writing_mode.h:28: bool IsParallelWritingMode(const LayoutObject* layoutObject, On 2017/02/15 at 17:52:24, cbiesinger wrote: > Please add a comment for what this function does. Also, this seems fairly specialized, I'd just move it over to ng_constraint_space.cc -- are there any other potential callers? done. This funciton was removed due to lack of callers.
The patchset sent to the CQ was uploaded after l-g-t-m from cbiesinger@chromium.org Link to the patchset: https://codereview.chromium.org/2691093004/#ps40001 (title: "CR fixes, removed odd IsParallelWritingMode")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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": 40001, "attempt_start_ts": 1487222738886420,
"parent_rev": "1edc35f110d1a463f9809286478c3f0dee9e1761", "commit_rev":
"283cef0d240cf6a118f605d396eee0fbee0278b7"}
Message was sent while issue was closed.
Description was changed from ========== Fix orthogonal mode legacy mismatch Orthogonal styles currently assert inside legacy layout, because width is -1. Ex: <div style="writing-mode:vertical-rl; border:1px solid red;"> <p style="writing-mode:horizontal-tb; border:1px solid blue;">Hello, world.</p> </div> These two routines did not handle orthogonal modes properly. NGConstraintSpace::CreateFromLayoutObject NGBlockNode::RunOldLayout I am not sure if this fix is correct, it is my best guess. With the fix, assert is avoided, but <p> is too tall. The constraint space used for layout looks correct, bug might be something else. Meta question: Should we bother with legacy orthogonal modes at all? I've stumbled upon several other orthogonal mode bugs. BUG=635619 [ng_orthogonal_legacy] ========== to ========== Fix orthogonal mode legacy mismatch Orthogonal styles currently assert inside legacy layout, because width is -1. Ex: <div style="writing-mode:vertical-rl; border:1px solid red;"> <p style="writing-mode:horizontal-tb; border:1px solid blue;">Hello, world.</p> </div> These two routines did not handle orthogonal modes properly. NGConstraintSpace::CreateFromLayoutObject NGBlockNode::RunOldLayout I am not sure if this fix is correct, it is my best guess. With the fix, assert is avoided, but <p> is too tall. The constraint space used for layout looks correct, bug might be something else. Meta question: Should we bother with legacy orthogonal modes at all? I've stumbled upon several other orthogonal mode bugs. BUG=635619 [ng_orthogonal_legacy] Review-Url: https://codereview.chromium.org/2691093004 Cr-Commit-Position: refs/heads/master@{#450878} Committed: https://chromium.googlesource.com/chromium/src/+/283cef0d240cf6a118f605d396ee... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/283cef0d240cf6a118f605d396ee... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
