Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2725773002:
Remove NGBlockNode constructor, SetFirstChild, SetNextSibling methods (Closed)
|
Created:
3 years, 9 months ago by atotic Modified:
3 years, 9 months ago 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. |
DescriptionRemove NGBlockNode constructor, SetFirstChild, SetNextSibling methods
I've removed the NGBlockNode constructor, SetFirstChild, and SetNextSibling that were only used by tests as discussed on chat.
BUG=635619
[ng_blocknode_style]
Review-Url: https://codereview.chromium.org/2725773002
Cr-Commit-Position: refs/heads/master@{#454130}
Committed: https://chromium.googlesource.com/chromium/src/+/7f1b8e5ae10a9dc930aedfda4c51c47e9a0c2cc9
Patch Set 1 #
Total comments: 16
Patch Set 2 : CR fixes #
Total comments: 9
Patch Set 3 : CR fixes #
Total comments: 11
Patch Set 4 : git cl web #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== Remove NGBlockNode(Style) constructor BUG=635619 ========== to ========== Remove NGBlockNode(Style) constructor I've removed the NGBlockNode constructor that was only used by tests as discussed on chat. BUG=635619 ==========
atotic@chromium.org changed reviewers: + glebl@chromium.org, ikilpatrick@chromium.org
Description was changed from ========== Remove NGBlockNode(Style) constructor I've removed the NGBlockNode constructor that was only used by tests as discussed on chat. BUG=635619 ========== to ========== Remove NGBlockNode(Style) constructor I've removed the NGBlockNode constructor that was only used by tests as discussed on chat. BUG=635619 [ng_blocknode_style] ==========
https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:678: #div1 { margin-bottom: 10px; width: 10px; height: 60px; writing-mode: vertical-rl; } this shouldn't be longer than 80 symbols. https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:804: <div id="first" style="width:10px;margin-left:auto;margin-right:auto"></div> same here. https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1343: auto container = new NGBlockNode(getLayoutObjectByElementId("container")); auto* ? here and below. https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1456: <div id="parent" style="column-count: 2; column-fill: auto; can you move the style definition to <style> ? and below https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1792: width:320px; height:100px;"> wrong indentation and please move it to <style>
https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (left): https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:105: node->SetFirstChild(first_child); can we also remove/make private SetFirstChild now as well? (if not don't worry). https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:120: auto box = new NGBlockNode(getLayoutObjectByElementId("box")); auto* box = .... https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:144: auto container = new NGBlockNode(getLayoutObjectByElementId("container")); auto* (and elsewhere).
ptal https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (left): https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:105: node->SetFirstChild(first_child); On 2017/03/01 at 17:32:46, ikilpatrick wrote: > can we also remove/make private SetFirstChild now as well? (if not don't worry). done. SetFirstChild/SetNextSibling are gone https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:120: auto box = new NGBlockNode(getLayoutObjectByElementId("box")); On 2017/03/01 at 17:32:46, ikilpatrick wrote: > auto* box = .... done https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:144: auto container = new NGBlockNode(getLayoutObjectByElementId("container")); On 2017/03/01 at 17:32:46, ikilpatrick wrote: > auto* (and elsewhere). done https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:678: #div1 { margin-bottom: 10px; width: 10px; height: 60px; writing-mode: vertical-rl; } On 2017/03/01 at 17:25:22, Gleb Lanbin wrote: > this shouldn't be longer than 80 symbols. done https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:804: <div id="first" style="width:10px;margin-left:auto;margin-right:auto"></div> On 2017/03/01 at 17:25:22, Gleb Lanbin wrote: > same here. done https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1343: auto container = new NGBlockNode(getLayoutObjectByElementId("container")); On 2017/03/01 at 17:25:22, Gleb Lanbin wrote: > auto* ? here and below. done https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1456: <div id="parent" style="column-count: 2; column-fill: auto; On 2017/03/01 at 17:25:22, Gleb Lanbin wrote: > can you move the style definition to <style> ? and below done for long style defs. https://codereview.chromium.org/2725773002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1792: width:320px; height:100px;"> On 2017/03/01 at 17:25:22, Gleb Lanbin wrote: > wrong indentation and please move it to <style> done
https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (left): https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:105: node->SetFirstChild(first_child); can you update the description because it doesn't look that you are only deleting NGBlockNode anymore. https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:132: <div id="first_child" style="height: 20px"> ids: first_child, second_child etc. are not used anymore. Change const NGPhysicalFragment* child = frag->Children()[0].get(); to first_child_fragment = frag->Children()[0].get(); // 20 = #first_child's height EXPECT_EQ(LayoutUnit(20), first_child_fragment->Height()); https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:135: style="height: 30px;margin-top: 5px;margin-bottom: 20px"> please follow https://google.github.io/styleguide/htmlcssguide.html 1) a single space after ; 2) .nit closing divs for empty blocks can be placed on the previous line. https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:172: <div id="div2" style="width: 50px; the style definition takes more than 3 lines? move it to <style>? https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_node.cc (right): https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_node.cc:170: next_sibling_ = new NGInlineNode(next_sibling, &Style()); personally I prefer that all updates of internal variables are done via helper functions. That's because it's easy to do refactoring if needed or introduce a precondition check etc. Why are you removing SetNextChild ?
Description was changed from ========== Remove NGBlockNode(Style) constructor I've removed the NGBlockNode constructor that was only used by tests as discussed on chat. BUG=635619 [ng_blocknode_style] ========== to ========== Remove NGBlockNode(Style) constructor Also removes NGBlockNode::SetFirstChild, NGBlock I've removed the NGBlockNode constructor that was only used by tests as discussed on chat. BUG=635619 [ng_blocknode_style] ==========
Description was changed from ========== Remove NGBlockNode(Style) constructor Also removes NGBlockNode::SetFirstChild, NGBlock I've removed the NGBlockNode constructor that was only used by tests as discussed on chat. BUG=635619 [ng_blocknode_style] ========== to ========== Remove NGBlockNode constructor, SetFirstChild, SetNextSibling methods I've removed the NGBlockNode constructor, SetFirstChild, and SetNextSibling that were only used by tests as discussed on chat. BUG=635619 [ng_blocknode_style] ==========
ptal https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (left): https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:105: node->SetFirstChild(first_child); On 2017/03/01 at 19:47:44, Gleb Lanbin wrote: > can you update the description because it doesn't look that you are only deleting NGBlockNode anymore. done https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:132: <div id="first_child" style="height: 20px"> On 2017/03/01 at 19:47:44, Gleb Lanbin wrote: > ids: first_child, second_child etc. are not used anymore. > Change const NGPhysicalFragment* child = frag->Children()[0].get(); to > > first_child_fragment = frag->Children()[0].get(); > // 20 = #first_child's height > EXPECT_EQ(LayoutUnit(20), first_child_fragment->Height()); done https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:135: style="height: 30px;margin-top: 5px;margin-bottom: 20px"> On 2017/03/01 at 19:47:44, Gleb Lanbin wrote: > please follow https://google.github.io/styleguide/htmlcssguide.html > 1) a single space after ; > 2) .nit closing divs for empty blocks can be placed on the previous line. done. > .nit closing divs for empty blocks can be placed on the previous line. I prefer them on separate lines, because I've forgotten closing divs before. Separate lines make the error obvious. https://codereview.chromium.org/2725773002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:172: <div id="div2" style="width: 50px; On 2017/03/01 at 19:47:44, Gleb Lanbin wrote: > the style definition takes more than 3 lines? move it to <style>? done
lgtm https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:75: NGBlockNode* block) { NGBlockNode* node we use "node" in most cases. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:138: const int kWidth = 30; optional kContainerWidth kFirstChildHeight etc https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1254: <div id="first_child" style="width: 20px"></div> should be first-child, second-child, same below https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Naming https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1276: const int kWidthChild2 = 30; What is Child2? kSecondChildWidth? https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:106: DCHECK(initial_containing_block_size.width >= LayoutUnit()); 1) DCHECK(initial_containing_block_size >= NGPhysicalSize()) >> "ICB cannot be zero-sized by the spec."; 2) add operator >= to NGPhysicalSize
https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:106: DCHECK(initial_containing_block_size.width >= LayoutUnit()); On 2017/03/01 21:08:42, Gleb Lanbin wrote: > 1) DCHECK(initial_containing_block_size >= NGPhysicalSize()) > >> "ICB cannot be zero-sized by the spec."; > 2) add operator >= to NGPhysicalSize nit: DCHECK_GE?
The CQ bit was checked by atotic@chromium.org
https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc (right): https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:75: NGBlockNode* block) { On 2017/03/01 at 21:08:42, Gleb Lanbin wrote: > NGBlockNode* node > we use "node" in most cases. > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng... done. I went with node initially, but then thought that block better reflects the fact that it is block formatted node.... This will get interesting later, as number of node types grows. https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:138: const int kWidth = 30; On 2017/03/01 at 21:08:41, Gleb Lanbin wrote: > optional > > kContainerWidth > kFirstChildHeight > etc ack, but no. It is a fair amount of work, and I do not think it contributes to readability. https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1254: <div id="first_child" style="width: 20px"></div> On 2017/03/01 at 21:08:42, Gleb Lanbin wrote: > should be first-child, second-child, same below > https://google.github.io/styleguide/htmlcssguide.html#ID_and_Class_Naming done https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm_test.cc:1276: const int kWidthChild2 = 30; On 2017/03/01 at 21:08:41, Gleb Lanbin wrote: > What is Child2? kSecondChildWidth? done. It was existing code, now switched to kSecondChildWidth. https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc (right): https://codereview.chromium.org/2725773002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ng/ng_constraint_space.cc:106: DCHECK(initial_containing_block_size.width >= LayoutUnit()); On 2017/03/01 at 21:25:09, dgrogan wrote: > On 2017/03/01 21:08:42, Gleb Lanbin wrote: > > 1) DCHECK(initial_containing_block_size >= NGPhysicalSize()) > > >> "ICB cannot be zero-sized by the spec."; done. > > 2) add operator >= to NGPhysicalSize no, would run into conflicts, ng_units refactor is in flight. > nit: DCHECK_GE? done
The patchset sent to the CQ was uploaded after l-g-t-m from glebl@chromium.org Link to the patchset: https://codereview.chromium.org/2725773002/#ps60001 (title: "git cl web")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488409668460170,
"parent_rev": "765accc846f00255326c71f6c95709fd3df308d6", "commit_rev":
"7f1b8e5ae10a9dc930aedfda4c51c47e9a0c2cc9"}
Message was sent while issue was closed.
Description was changed from ========== Remove NGBlockNode constructor, SetFirstChild, SetNextSibling methods I've removed the NGBlockNode constructor, SetFirstChild, and SetNextSibling that were only used by tests as discussed on chat. BUG=635619 [ng_blocknode_style] ========== to ========== Remove NGBlockNode constructor, SetFirstChild, SetNextSibling methods I've removed the NGBlockNode constructor, SetFirstChild, and SetNextSibling that were only used by tests as discussed on chat. BUG=635619 [ng_blocknode_style] Review-Url: https://codereview.chromium.org/2725773002 Cr-Commit-Position: refs/heads/master@{#454130} Committed: https://chromium.googlesource.com/chromium/src/+/7f1b8e5ae10a9dc930aedfda4c51... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7f1b8e5ae10a9dc930aedfda4c51... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
