Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118393002:
Reorganize Layout Tree Construction code to be its own function (Closed)
|
Created:
4 years, 5 months ago by nainar Modified:
4 years, 5 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReorganize Layout Tree Construction code to be its own function
This patch trivially encapsulates Layout Tree Construction
code in its own function -> Element::buildOwnLayout().
No behaviour change is expected from this patch since it is
merely moving code around.
With lots of help from @bugsnash
Design doc: http://bit.ly/29MBWvf
BUG=595137
Committed: https://crrev.com/617ae6ff6a6664b776e2f8617ef3f51bef639a2b
Cr-Commit-Position: refs/heads/master@{#404584}
Patch Set 1 #
Total comments: 4
Patch Set 2 #Patch Set 3 : post Sasha's comments #
Total comments: 7
Patch Set 4 : CL #Patch Set 5 : For Tim's review #
Total comments: 2
Patch Set 6 : Final patch for landinh #
Messages
Total messages: 27 (9 generated)
Description was changed from ========== move code to buildOwnLayout() BUG= ========== to ========== Reorganize Layout Tree Construction code to be its own function This patch trivially encapsulates Layout Tree Construction code in its own function -> Element::buildOwnLayout(). No behaviour change is expected from this patch since it is merely moving code around. With lots of help from @bugsnash BUG=595137 ==========
nainar@chromium.org changed reviewers: + bugsnash@chromium.org, sashab@chromium.org
@sashab, Could you PTAL? This follows on from our discussion here: https://codereview.chromium.org/2001453002 Thank you!
LGTM with nits https://codereview.chromium.org/2118393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2118393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:1774: // using mutableComputedStyle() Nit: full stops at the end of each sentence :) https://codereview.chromium.org/2118393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2118393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.h:653: StyleRecalcChange buildOwnLayout(ComputedStyle*); const ComputedStyle*
nainar@chromium.org changed reviewers: + timloh@chromium.org
@timloh, PTAL? Thanks! https://codereview.chromium.org/2118393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2118393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.cpp:1774: // using mutableComputedStyle() Done. https://codereview.chromium.org/2118393002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2118393002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Element.h:653: StyleRecalcChange buildOwnLayout(ComputedStyle*); Done.
Yup, making it const is difficult but I think it's the right thing to do. The style should not be modified past once your separation of style/layout is complete. So a TODO makes sense. :) https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1815: { DCHECK(newstyle) here https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1815: { DCHECK(newstyle) here https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:653: StyleRecalcChange buildOwnLayout(ComputedStyle*); TODO(nainar): Make this const ComputedStyle*.
lgtm
On 2016/07/06 at 00:15:25, Bugs Nash wrote: > lgtm Add link to design doc in summary.
Description was changed from ========== Reorganize Layout Tree Construction code to be its own function This patch trivially encapsulates Layout Tree Construction code in its own function -> Element::buildOwnLayout(). No behaviour change is expected from this patch since it is merely moving code around. With lots of help from @bugsnash BUG=595137 ========== to ========== Reorganize Layout Tree Construction code to be its own function This patch trivially encapsulates Layout Tree Construction code in its own function -> Element::buildOwnLayout(). No behaviour change is expected from this patch since it is merely moving code around. With lots of help from @bugsnash Design doc: http://bit.ly/29MBWvf BUG=595137 ==========
https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:653: StyleRecalcChange buildOwnLayout(ComputedStyle*); On 2016/07/05 at 23:28:55, sashab wrote: > TODO(nainar): Make this const ComputedStyle*. Would PassRefPtr be better than raw ptr?
Made the changes Sasha has asked for. Waiting to hear about her opinion on Bugs' question of PassRefPtr vs raw ptr. https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1815: { Done. https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:653: StyleRecalcChange buildOwnLayout(ComputedStyle*); Done.
https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2118393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:653: StyleRecalcChange buildOwnLayout(ComputedStyle*); On 2016/07/06 at 00:52:09, nainar wrote: > Done. From https://webkit.org/blog/5381/refptr-basics/: "Later, in 2013, we noticed that our use of pointers in general, and smart pointers in particular, was causing a proliferation of null checks and uncertainty about what can be null. We started using references rather than pointers wherever possible in WebKit code." So, actually, it should be a reference! :) Make it a reference, remove the DCHECK and then use &newStyle to assign it to reattachContext, since that can actually be null. Technically this isn't great but ReattachContext can actually store a null ComputedStyle. Let's investigate making it const another day 8^)
Made the changes. @timloh, PTAL? Thanks!
lgtm https://codereview.chromium.org/2118393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2118393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:653: // TODO(nainar): Make this const ComputedStyle*. * -> &?
Thanks Tim! https://codereview.chromium.org/2118393002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.h (right): https://codereview.chromium.org/2118393002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.h:653: // TODO(nainar): Make this const ComputedStyle*. Done.
The CQ bit was checked by nainar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sashab@chromium.org, bugsnash@chromium.org, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2118393002/#ps100001 (title: "Final patch for landinh")
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nainar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Reorganize Layout Tree Construction code to be its own function This patch trivially encapsulates Layout Tree Construction code in its own function -> Element::buildOwnLayout(). No behaviour change is expected from this patch since it is merely moving code around. With lots of help from @bugsnash Design doc: http://bit.ly/29MBWvf BUG=595137 ========== to ========== Reorganize Layout Tree Construction code to be its own function This patch trivially encapsulates Layout Tree Construction code in its own function -> Element::buildOwnLayout(). No behaviour change is expected from this patch since it is merely moving code around. With lots of help from @bugsnash Design doc: http://bit.ly/29MBWvf BUG=595137 Committed: https://crrev.com/617ae6ff6a6664b776e2f8617ef3f51bef639a2b Cr-Commit-Position: refs/heads/master@{#404584} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/617ae6ff6a6664b776e2f8617ef3f51bef639a2b Cr-Commit-Position: refs/heads/master@{#404584} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
