Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2293713002:
Made ElementRareData store ComputedStyle on LayoutObject if possible. (Closed)
DescriptionMade ElementRareData store ComputedStyle on LayoutObject if possible.
Made ElementRareData store ComputedStyle on LayoutObject of it's base
class NodeRareDataBase if it has a LayoutObject.
Otherwise it is stored on the ComputedStyle member of ElementRareData.
The reason for this is to make it consistent with storage of
ComputedStyle on Node, which stores a union that can be one of:
1) a ComputedStyle
2) a LayoutObject (ComputedStyle can be stored on this)
3) a NodeRareData (LayoutObject can be stored on this which stores
ComputedStyle or if no LayoutObject ComputedStyle can be stored
directly on ElementRareData)
This patch encapsulates the logic of where to store the ComputedStyle
on the ElementRareData so that it doesn't need to be done at the call
site (e.g. in new method Node::setComputedStyle from
https://codereview.chromium.org/2001453002)
The clearComputedStyle method is special and only clears the
ComputedStyle member as its current use is not intended to clear a
LayoutObject's ComputedStyle.
BUG=595137
Committed: https://crrev.com/c542d3744b905a2a00f5ac2dd2047a6a323ec181
Cr-Commit-Position: refs/heads/master@{#418453}
Patch Set 1 #Patch Set 2 : removed whitespace #Patch Set 3 : Added unit test #
Total comments: 4
Patch Set 4 : Addressed reviewer comments #Patch Set 5 : Added CORE_EXPORT to NodeRareDataBase #
Total comments: 2
Messages
Total messages: 51 (31 generated)
The CQ bit was checked by bugsnash@chromium.org 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.
Description was changed from ========== Made ElementRareData store ComputedStyle on LayoutObject if possible. Made ElementRareData store ComputedStyle on LayoutObject of it's base class NodeRareDataBase if it has a LayoutObject. Otherwise it is stored on the ComputedStyle member of ElementRareData. The clearComputedStyle method is special and only clears the ComputedStyle member. BUG: 595137 ========== to ========== Made ElementRareData store ComputedStyle on LayoutObject if possible. Made ElementRareData store ComputedStyle on LayoutObject of it's base class NodeRareDataBase if it has a LayoutObject. Otherwise it is stored on the ComputedStyle member of ElementRareData. The reason for this is to make it consistent with storage of ComputedStyle on Node, which stores a union that can be one of: 1) a ComputedStyle 2) a LayoutObject (ComputedStyle can be stored on this) 3) a NodeRareData (LayoutObject can be stored on this which stores ComputedStyle or if no LayoutObject ComputedStyle can be stored directly on ElementRareData) This patch encapsulates the logic of where to store the ComputedStyle on the ElementRareData so that it doesn't need to be done at the call site (e.g. in new method Node::setComputedStyle from https://codereview.chromium.org/2001453002) The clearComputedStyle method is special and only clears the ComputedStyle member as its current use is not intended to clear a LayoutObject's ComputedStyle. BUG: 595137 ==========
bugsnash@chromium.org changed reviewers: + meade@chromium.org
The change looks good. Would suggest adding a Unit Test that tests our ability to get/set a ComputedStyle from a LayoutObject on an ElementRareData. While you're at it also add a Unit Test that checks get/set a ComputedStyle on an ElementRareData. If you can find a way to make a LayoutTest work - that would do the trick too! Don't have enough knowledge on the ability to LayoutTest vs Unit Test this.
On 2016/08/30 at 04:59:43, nainar wrote: > The change looks good. > > Would suggest adding a Unit Test that tests our ability to get/set a ComputedStyle from a LayoutObject on an ElementRareData. While you're at it also add a Unit Test that checks get/set a ComputedStyle on an ElementRareData. > > If you can find a way to make a LayoutTest work - that would do the trick too! Don't have enough knowledge on the ability to LayoutTest vs Unit Test this. I'll add a unit test. As this isn't a layout affecting change I don't think a layout test is appropriate.
This seems fine to me, modulo questions about whether the structure should be simplified so there is a single canonical location where the computed style is stored. Do you think there is a single place it _could_ be kept and everything would still work?
On 2016/09/02 at 03:33:37, meade wrote: > This seems fine to me, modulo questions about whether the structure should be simplified so there is a single canonical location where the computed style is stored. Do you think there is a single place it _could_ be kept and everything would still work? The simplest place to canonically store ComputedStyle would be on a new member inside Node. But that makes all nodes 4 bytes larger. The current design of Squad to store Node's ComputedStyle on the existing union avoids making Node any bigger, storing LayoutObject and ComputedStyle on the rare data in the union if necessary. For cases where there is rare data you may or may not have a LayoutObject at various points in time. When you don't have a LayoutObject, you need a place to store the ComputedStyle (here it's on the ElementRareData member m_computedStyle). For cases where you do have a LayoutObject, that LayoutObject has a pointer to the ComputedStyle which is probably being used, so I don't see an obvious way to simplify here. Inside ElementRareData itself, we could leave it as is and always store the ComputedStyle on the member m_computedStyle and just ignore whatever the LayoutObject is pointing to (but this seems like it could be error prone).
lgtm
bugsnash@chromium.org changed reviewers: + timloh@chromium.org
The CQ bit was checked by bugsnash@chromium.org 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...
bugsnash@chromium.org changed reviewers: + nainar@chromium.org
Have added unit test
On 2016/09/05 at 05:41:14, bugsnash wrote: > Have added unit test lgtm :) thanks for adding the unit test!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
Description was changed from ========== Made ElementRareData store ComputedStyle on LayoutObject if possible. Made ElementRareData store ComputedStyle on LayoutObject of it's base class NodeRareDataBase if it has a LayoutObject. Otherwise it is stored on the ComputedStyle member of ElementRareData. The reason for this is to make it consistent with storage of ComputedStyle on Node, which stores a union that can be one of: 1) a ComputedStyle 2) a LayoutObject (ComputedStyle can be stored on this) 3) a NodeRareData (LayoutObject can be stored on this which stores ComputedStyle or if no LayoutObject ComputedStyle can be stored directly on ElementRareData) This patch encapsulates the logic of where to store the ComputedStyle on the ElementRareData so that it doesn't need to be done at the call site (e.g. in new method Node::setComputedStyle from https://codereview.chromium.org/2001453002) The clearComputedStyle method is special and only clears the ComputedStyle member as its current use is not intended to clear a LayoutObject's ComputedStyle. BUG: 595137 ========== to ========== Made ElementRareData store ComputedStyle on LayoutObject if possible. Made ElementRareData store ComputedStyle on LayoutObject of it's base class NodeRareDataBase if it has a LayoutObject. Otherwise it is stored on the ComputedStyle member of ElementRareData. The reason for this is to make it consistent with storage of ComputedStyle on Node, which stores a union that can be one of: 1) a ComputedStyle 2) a LayoutObject (ComputedStyle can be stored on this) 3) a NodeRareData (LayoutObject can be stored on this which stores ComputedStyle or if no LayoutObject ComputedStyle can be stored directly on ElementRareData) This patch encapsulates the logic of where to store the ComputedStyle on the ElementRareData so that it doesn't need to be done at the call site (e.g. in new method Node::setComputedStyle from https://codereview.chromium.org/2001453002) The clearComputedStyle method is special and only clears the ComputedStyle member as its current use is not intended to clear a LayoutObject's ComputedStyle. BUG:595137 ==========
Description was changed from ========== Made ElementRareData store ComputedStyle on LayoutObject if possible. Made ElementRareData store ComputedStyle on LayoutObject of it's base class NodeRareDataBase if it has a LayoutObject. Otherwise it is stored on the ComputedStyle member of ElementRareData. The reason for this is to make it consistent with storage of ComputedStyle on Node, which stores a union that can be one of: 1) a ComputedStyle 2) a LayoutObject (ComputedStyle can be stored on this) 3) a NodeRareData (LayoutObject can be stored on this which stores ComputedStyle or if no LayoutObject ComputedStyle can be stored directly on ElementRareData) This patch encapsulates the logic of where to store the ComputedStyle on the ElementRareData so that it doesn't need to be done at the call site (e.g. in new method Node::setComputedStyle from https://codereview.chromium.org/2001453002) The clearComputedStyle method is special and only clears the ComputedStyle member as its current use is not intended to clear a LayoutObject's ComputedStyle. BUG:595137 ========== to ========== Made ElementRareData store ComputedStyle on LayoutObject if possible. Made ElementRareData store ComputedStyle on LayoutObject of it's base class NodeRareDataBase if it has a LayoutObject. Otherwise it is stored on the ComputedStyle member of ElementRareData. The reason for this is to make it consistent with storage of ComputedStyle on Node, which stores a union that can be one of: 1) a ComputedStyle 2) a LayoutObject (ComputedStyle can be stored on this) 3) a NodeRareData (LayoutObject can be stored on this which stores ComputedStyle or if no LayoutObject ComputedStyle can be stored directly on ElementRareData) This patch encapsulates the logic of where to store the ComputedStyle on the ElementRareData so that it doesn't need to be done at the call site (e.g. in new method Node::setComputedStyle from https://codereview.chromium.org/2001453002) The clearComputedStyle method is special and only clears the ComputedStyle member as its current use is not intended to clear a LayoutObject's ComputedStyle. BUG=595137 ==========
Sorry for coming back to this again. Could you please add the fact that clearComputedStyle is only ever meant to clear the ComputedStyle on ElementRareData directly and never the one on LayoutObject in case people confuse it otherwise and add a callsite that uses it incorrectly. Thank you!
ok lgtm but you still have a red bot (maybe needs CORE_EXPORT on NodeRareDataBase?) https://codereview.chromium.org/2293713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2293713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementRareData.h:95: { Maybe DCHECK(!(layoutObject() && m_computedStyle)) here https://codereview.chromium.org/2293713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementRareData.h:107: void clearComputedStyle() { m_computedStyle = nullptr; } OK for now but at some point we should work out if we actually need this because it's a bit weird. Please add a comment because otherwise it looks like it's just doing the wrong thing.
The CQ bit was checked by bugsnash@chromium.org 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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by bugsnash@chromium.org 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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by bugsnash@chromium.org 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.
Tim can you please confirm you're ok with the clearComputedStyle solution before I commit? https://codereview.chromium.org/2293713002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2293713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementRareData.h:95: { On 2016/09/07 at 04:56:07, Timothy Loh wrote: > Maybe DCHECK(!(layoutObject() && m_computedStyle)) here Done. https://codereview.chromium.org/2293713002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementRareData.h:107: void clearComputedStyle() { m_computedStyle = nullptr; } On 2016/09/07 at 04:56:07, Timothy Loh wrote: > OK for now but at some point we should work out if we actually need this because it's a bit weird. Please add a comment because otherwise it looks like it's just doing the wrong thing. As per offline conversation the usage of this method only works when there is no LayoutObject. I did a dry run with all 3 call sites only calling this method if there is no LayoutObject and it passed. I the ntried removing the check for a LayoutObject in each of the 3 call sites 1 at at time and the bots failed for all 3 (see https://codereview.chromium.org/2318973003). Therefore I think the best solution is to keep the functionality in this patch and change the name of this method to clearComputedStyleIfNoLayoutObject to make it clear that this will not clear the ComputedStyle on the LayoutObject. I've also added a DCHECK that there isn't simultaneously a LayoutObject and a separate ComputedStyle as this may lead to unexpected behavior.
On 2016/09/13 21:46:55, Bugs Nash wrote: > Tim can you please confirm you're ok with the clearComputedStyle solution before > I commit? Looks fine
The CQ bit was checked by bugsnash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meade@chromium.org, nainar@chromium.org, timloh@chromium.org Link to the patchset: https://codereview.chromium.org/2293713002/#ps100001 (title: "Added CORE_EXPORT to NodeRareDataBase")
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 #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Made ElementRareData store ComputedStyle on LayoutObject if possible. Made ElementRareData store ComputedStyle on LayoutObject of it's base class NodeRareDataBase if it has a LayoutObject. Otherwise it is stored on the ComputedStyle member of ElementRareData. The reason for this is to make it consistent with storage of ComputedStyle on Node, which stores a union that can be one of: 1) a ComputedStyle 2) a LayoutObject (ComputedStyle can be stored on this) 3) a NodeRareData (LayoutObject can be stored on this which stores ComputedStyle or if no LayoutObject ComputedStyle can be stored directly on ElementRareData) This patch encapsulates the logic of where to store the ComputedStyle on the ElementRareData so that it doesn't need to be done at the call site (e.g. in new method Node::setComputedStyle from https://codereview.chromium.org/2001453002) The clearComputedStyle method is special and only clears the ComputedStyle member as its current use is not intended to clear a LayoutObject's ComputedStyle. BUG=595137 ========== to ========== Made ElementRareData store ComputedStyle on LayoutObject if possible. Made ElementRareData store ComputedStyle on LayoutObject of it's base class NodeRareDataBase if it has a LayoutObject. Otherwise it is stored on the ComputedStyle member of ElementRareData. The reason for this is to make it consistent with storage of ComputedStyle on Node, which stores a union that can be one of: 1) a ComputedStyle 2) a LayoutObject (ComputedStyle can be stored on this) 3) a NodeRareData (LayoutObject can be stored on this which stores ComputedStyle or if no LayoutObject ComputedStyle can be stored directly on ElementRareData) This patch encapsulates the logic of where to store the ComputedStyle on the ElementRareData so that it doesn't need to be done at the call site (e.g. in new method Node::setComputedStyle from https://codereview.chromium.org/2001453002) The clearComputedStyle method is special and only clears the ComputedStyle member as its current use is not intended to clear a LayoutObject's ComputedStyle. BUG=595137 Committed: https://crrev.com/c542d3744b905a2a00f5ac2dd2047a6a323ec181 Cr-Commit-Position: refs/heads/master@{#418453} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c542d3744b905a2a00f5ac2dd2047a6a323ec181 Cr-Commit-Position: refs/heads/master@{#418453}
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
So I think there's been some confusion here and in the other patch https://codereview.chromium.org/2001453002 Maybe we should have a meeting? I think what you might want is: - If there's ElementRareData, put the style there. - If there's a LayoutObject put the style in a HashMap<Element*, RefPtr<ComputedStyle>> - Otherwise put the style in Node::DataUnion. An alternative might be: - change your plan such that you leave layout tree destruction inside the recalcStyle phase, so you can always store the style in either the ElementRareData or the DataUnion. https://codereview.chromium.org/2293713002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementRareData.h (right): https://codereview.chromium.org/2293713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementRareData.h:104: layoutObject()->setStyleInternal(computedStyle); This case isn't reachable (outside your test which doesn't look right), you can't end up inside ElementRareData::setComputedStyle when there's a layoutObject. That code is only reached from Element::ensureComputedStyle https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Eleme... when mutableComputedStyle was null, which can only happen if layoutObject() was null, since layout objects always have a style except very briefly inside LayoutTreeBuilder between the create call and the setStyle call (we could probs fix that by passing the style into the constructor, WebKit did it a while ago, then LayoutObject::style could return a reference). Note also that using setStyleInternal here is bad since it potentially stuffs an invalid style into a layout object. This patch worked because the code was unreachable. setStyleInternal exists for dealing with some complex style sharing situations. It's also gotten used in a few other cases, but they're super subtle. Generally speaking you should never use setStyleInternal unless the newStyle you're setting would return true from ComputedStyle::operator== (which is expensive, so you don't want to check that yourself outside the recalc phase). I know there's a few other callers, like the TextAutosizer. Its usage of setStyleInternal is technically correct, even though it violates the operator== requirement, because of how it works inside layout updating styles recursively down the tree, but I'll admit it's not obvious. https://codereview.chromium.org/2293713002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementRareData.h:110: DCHECK(!(layoutObject() && m_computedStyle)); Plz run demorgans :D running it in my head is hard, this check is: DCHECK(!layoutObject() || !m_computedStyle) so we can end up in here when there's a layoutObject as long as m_computedStyle was already null. That's apparent inside Element::recalcStyle where we call this totally ignoring the state of if there's a layout object. I'd remove the long IfNoLayoutObject thing here and just leave this as clearComputedStyle(). This method shouldn't need to care about if there's a layout object.
Message was sent while issue was closed.
Seems like we need a meeting and another redesign of the squad project Bugs On 24 Sep. 2016 12:10 pm, <esprehn@chromium.org> wrote: > So I think there's been some confusion here and in the other patch > https://codereview.chromium.org/2001453002 > > Maybe we should have a meeting? > > I think what you might want is: > - If there's ElementRareData, put the style there. > - If there's a LayoutObject put the style in a HashMap<Element*, > RefPtr<ComputedStyle>> > - Otherwise put the style in Node::DataUnion. > > An alternative might be: > - change your plan such that you leave layout tree destruction inside the > recalcStyle phase, so you can always store the style in either the > ElementRareData or the DataUnion. > > > > https://codereview.chromium.org/2293713002/diff/100001/ > third_party/WebKit/Source/core/dom/ElementRareData.h > File third_party/WebKit/Source/core/dom/ElementRareData.h (right): > > https://codereview.chromium.org/2293713002/diff/100001/ > third_party/WebKit/Source/core/dom/ElementRareData.h#newcode104 > third_party/WebKit/Source/core/dom/ElementRareData.h:104: > layoutObject()->setStyleInternal(computedStyle); > This case isn't reachable (outside your test which doesn't look right), > you can't end up inside ElementRareData::setComputedStyle when there's a > layoutObject. That code is only reached from > Element::ensureComputedStyle > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/ > Element.cpp?q=ensureComputedStyle&sq=package:chromium&dr=CSs&l=2905 > > when mutableComputedStyle was null, which can only happen if > layoutObject() was null, since layout objects always have a style except > very briefly inside LayoutTreeBuilder between the create call and the > setStyle call (we could probs fix that by passing the style into the > constructor, WebKit did it a while ago, then LayoutObject::style could > return a reference). > > Note also that using setStyleInternal here is bad since it potentially > stuffs an invalid style into a layout object. This patch worked because > the code was unreachable. setStyleInternal exists for dealing with some > complex style sharing situations. It's also gotten used in a few other > cases, but they're super subtle. Generally speaking you should never use > setStyleInternal unless the newStyle you're setting would return true > from ComputedStyle::operator== (which is expensive, so you don't want to > check that yourself outside the recalc phase). > > I know there's a few other callers, like the TextAutosizer. Its usage of > setStyleInternal is technically correct, even though it violates the > operator== requirement, because of how it works inside layout updating > styles recursively down the tree, but I'll admit it's not obvious. > > https://codereview.chromium.org/2293713002/diff/100001/ > third_party/WebKit/Source/core/dom/ElementRareData.h#newcode110 > third_party/WebKit/Source/core/dom/ElementRareData.h:110: > DCHECK(!(layoutObject() && m_computedStyle)); > Plz run demorgans :D running it in my head is hard, this check is: > > DCHECK(!layoutObject() || !m_computedStyle) > > so we can end up in here when there's a layoutObject as long as > m_computedStyle was already null. That's apparent inside > Element::recalcStyle where we call this totally ignoring the state of if > there's a layout object. > > I'd remove the long IfNoLayoutObject thing here and just leave this as > clearComputedStyle(). This method shouldn't need to care about if > there's a layout object. > > https://codereview.chromium.org/2293713002/ > -- 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.
Message was sent while issue was closed.
Seems like we need a meeting and another redesign of the squad project Bugs On 24 Sep. 2016 12:10 pm, <esprehn@chromium.org> wrote: > So I think there's been some confusion here and in the other patch > https://codereview.chromium.org/2001453002 > > Maybe we should have a meeting? > > I think what you might want is: > - If there's ElementRareData, put the style there. > - If there's a LayoutObject put the style in a HashMap<Element*, > RefPtr<ComputedStyle>> > - Otherwise put the style in Node::DataUnion. > > An alternative might be: > - change your plan such that you leave layout tree destruction inside the > recalcStyle phase, so you can always store the style in either the > ElementRareData or the DataUnion. > > > > https://codereview.chromium.org/2293713002/diff/100001/ > third_party/WebKit/Source/core/dom/ElementRareData.h > File third_party/WebKit/Source/core/dom/ElementRareData.h (right): > > https://codereview.chromium.org/2293713002/diff/100001/ > third_party/WebKit/Source/core/dom/ElementRareData.h#newcode104 > third_party/WebKit/Source/core/dom/ElementRareData.h:104: > layoutObject()->setStyleInternal(computedStyle); > This case isn't reachable (outside your test which doesn't look right), > you can't end up inside ElementRareData::setComputedStyle when there's a > layoutObject. That code is only reached from > Element::ensureComputedStyle > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/ > Element.cpp?q=ensureComputedStyle&sq=package:chromium&dr=CSs&l=2905 > > when mutableComputedStyle was null, which can only happen if > layoutObject() was null, since layout objects always have a style except > very briefly inside LayoutTreeBuilder between the create call and the > setStyle call (we could probs fix that by passing the style into the > constructor, WebKit did it a while ago, then LayoutObject::style could > return a reference). > > Note also that using setStyleInternal here is bad since it potentially > stuffs an invalid style into a layout object. This patch worked because > the code was unreachable. setStyleInternal exists for dealing with some > complex style sharing situations. It's also gotten used in a few other > cases, but they're super subtle. Generally speaking you should never use > setStyleInternal unless the newStyle you're setting would return true > from ComputedStyle::operator== (which is expensive, so you don't want to > check that yourself outside the recalc phase). > > I know there's a few other callers, like the TextAutosizer. Its usage of > setStyleInternal is technically correct, even though it violates the > operator== requirement, because of how it works inside layout updating > styles recursively down the tree, but I'll admit it's not obvious. > > https://codereview.chromium.org/2293713002/diff/100001/ > third_party/WebKit/Source/core/dom/ElementRareData.h#newcode110 > third_party/WebKit/Source/core/dom/ElementRareData.h:110: > DCHECK(!(layoutObject() && m_computedStyle)); > Plz run demorgans :D running it in my head is hard, this check is: > > DCHECK(!layoutObject() || !m_computedStyle) > > so we can end up in here when there's a layoutObject as long as > m_computedStyle was already null. That's apparent inside > Element::recalcStyle where we call this totally ignoring the state of if > there's a layout object. > > I'd remove the long IfNoLayoutObject thing here and just leave this as > clearComputedStyle(). This method shouldn't need to care about if > there's a layout object. > > https://codereview.chromium.org/2293713002/ > -- 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.
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/2378573002/ by bugsnash@chromium.org. The reason for reverting is: Storage of ComputedStyle on LayoutObject causes invalid LayoutObject state. Also no longer required for consistency as Squad project requires redesign and will no longer store ComputedStyle on Node's data union.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
