Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2725953002:
Refactor LayoutTreeBuilderTraversal to expose a cleaner interface to layout sibling nodes. (Closed)
DescriptionRefactor LayoutTreeBuilderTraversal to expose a cleaner interface to layout sibling nodes.
This is the first step towards fixing the whitespace attachment bugs we have. My
plan is to use this from recalcDescendantStyles, and reattachWhitespaceSiblings
instead of the light tree.
BUG=657748
Review-Url: https://codereview.chromium.org/2725953002
Cr-Commit-Position: refs/heads/master@{#456105}
Committed: https://chromium.googlesource.com/chromium/src/+/15261753dcb96f302ca53757dcaefa442c4b2f0d
Patch Set 1 #Patch Set 2 : Refactor LayoutTreeBuilderTraversal to expose a cleaner interface to layout sibling nodes. #Patch Set 3 : Refactor LayoutTreeBuilderTraversal to expose a cleaner interface to layout sibling nodes. #
Total comments: 1
Patch Set 4 : Address review comments. #Patch Set 5 : Address review comments. #Patch Set 6 : Refactor LayoutTreeBuilderTraversal to expose a cleaner interface to layout sibling nodes. #
Messages
Total messages: 25 (17 generated)
The CQ bit was checked by ecobos@igalia.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by ecobos@igalia.com to run a CQ dry run
Dry run: 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
Dry run: Try jobs failed on following builders: linux_chromium_asan_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 ecobos@igalia.com to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
ecobos@igalia.com changed reviewers: + rune@opera.com
Ouch, I submitted this a few days ago and thought I had put it on your queue. Could you review this? It's cleanup mainly, but I plan to use this to get the proper text sibling on recalcStyle, etc. (note that we need to look at undisplayed siblings, so we can't just use the LayoutObject APIs).
https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h (right): https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h:41: static Node* previousLayoutSibling(const Node&, int32_t& limit); I'd put these in a private section below the public section instead. Or since these are private, couldn't they be static functions in the cpp file instead, like we do with other methods? Oh, you're using it below. Will it be a perf-issue if you implement nextLayoutSibling() in the cpp file instead? I'd guess that the compiler would inline a static nextLayoutSibling function into that implementation.
On 2017/03/10 12:32:42, rune wrote: > https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h (right): > > https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h:41: static Node* > previousLayoutSibling(const Node&, int32_t& limit); > I'd put these in a private section below the public section instead. Or since > these are private, couldn't they be static functions in the cpp file instead, > like we do with other methods? > > Oh, you're using it below. Will it be a perf-issue if you implement > nextLayoutSibling() in the cpp file instead? I'd guess that the compiler would > inline a static nextLayoutSibling function into that implementation. Sure, makes sense.
On 2017/03/10 13:31:00, emilio wrote: > On 2017/03/10 12:32:42, rune wrote: > > > https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h (right): > > > > > https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h:41: static > Node* > > previousLayoutSibling(const Node&, int32_t& limit); > > I'd put these in a private section below the public section instead. Or since > > these are private, couldn't they be static functions in the cpp file instead, > > like we do with other methods? > > > > Oh, you're using it below. Will it be a perf-issue if you implement > > nextLayoutSibling() in the cpp file instead? I'd guess that the compiler would > > inline a static nextLayoutSibling function into that implementation. > > Sure, makes sense. Done. I don't love the extra qualifications needed to avoid name conflicts, but if you're fine with it...
On 2017/03/10 13:40:50, emilio wrote: > On 2017/03/10 13:31:00, emilio wrote: > > On 2017/03/10 12:32:42, rune wrote: > > > > > > https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h > (right): > > > > > > > > > https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h:41: static > > Node* > > > previousLayoutSibling(const Node&, int32_t& limit); > > > I'd put these in a private section below the public section instead. Or > since > > > these are private, couldn't they be static functions in the cpp file > instead, > > > like we do with other methods? > > > > > > Oh, you're using it below. Will it be a perf-issue if you implement > > > nextLayoutSibling() in the cpp file instead? I'd guess that the compiler > would > > > inline a static nextLayoutSibling function into that implementation. > > > > Sure, makes sense. > > Done. I don't love the extra qualifications needed to avoid name conflicts, but > if you're fine with it... Huh, I didn't know that would be a problem. I'm fine with either, but if you go for the first approach, please move the methods into a private section at the bottom. lgtm.
The CQ bit was checked by ecobos@igalia.com
The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com Link to the patchset: https://codereview.chromium.org/2725953002/#ps100001 (title: "Refactor LayoutTreeBuilderTraversal to expose a cleaner interface to layout sibling nodes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/10 14:14:19, rune wrote: > On 2017/03/10 13:40:50, emilio wrote: > > On 2017/03/10 13:31:00, emilio wrote: > > > On 2017/03/10 12:32:42, rune wrote: > > > > > > > > > > https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2725953002/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/dom/LayoutTreeBuilderTraversal.h:41: static > > > Node* > > > > previousLayoutSibling(const Node&, int32_t& limit); > > > > I'd put these in a private section below the public section instead. Or > > since > > > > these are private, couldn't they be static functions in the cpp file > > instead, > > > > like we do with other methods? > > > > > > > > Oh, you're using it below. Will it be a perf-issue if you implement > > > > nextLayoutSibling() in the cpp file instead? I'd guess that the compiler > > would > > > > inline a static nextLayoutSibling function into that implementation. > > > > > > Sure, makes sense. > > > > Done. I don't love the extra qualifications needed to avoid name conflicts, > but > > if you're fine with it... > > Huh, I didn't know that would be a problem. I'm fine with either, but if you go > for the first approach, please move the methods into a private section at the > bottom. I ended up doing this, I think the code reads slightly better that way.
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489163912142580,
"parent_rev": "fe89c29e259964557dc8a1ff4e450accb47193e1", "commit_rev":
"15261753dcb96f302ca53757dcaefa442c4b2f0d"}
Message was sent while issue was closed.
Description was changed from ========== Refactor LayoutTreeBuilderTraversal to expose a cleaner interface to layout sibling nodes. This is the first step towards fixing the whitespace attachment bugs we have. My plan is to use this from recalcDescendantStyles, and reattachWhitespaceSiblings instead of the light tree. BUG=657748 ========== to ========== Refactor LayoutTreeBuilderTraversal to expose a cleaner interface to layout sibling nodes. This is the first step towards fixing the whitespace attachment bugs we have. My plan is to use this from recalcDescendantStyles, and reattachWhitespaceSiblings instead of the light tree. BUG=657748 Review-Url: https://codereview.chromium.org/2725953002 Cr-Commit-Position: refs/heads/master@{#456105} Committed: https://chromium.googlesource.com/chromium/src/+/15261753dcb96f302ca53757dcae... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/15261753dcb96f302ca53757dcae... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
