Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1962953002:
Storage of ComputedStyle separate from LayoutObject. (Closed)
|
Created:
4 years, 7 months ago by Bugs Nash Modified:
4 years, 4 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, nainar, rwlbuis, rune, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStorage of ComputedStyle separate from LayoutObject.
Part of stage 1 in separation of style resolution and
layout tree construction
Design doc: bit.ly/24nQ9UQ
Changes the Node::m_data DataUnion to now allow a ComputedStyle.
Updates Node flags with new flag HasLayoutObject to indicate whether
the DataUnion is a LayoutObject or a ComputedStyle. Adds new
hasLayoutObject method to check flag.
Updates ComputedStyle and Layoutobject getters and setters accordingly.
Written with nainar@
Originally landed as:
https://crrev.com/11af5b8769875ef50b16cb94d7eb758def0d9b08
but was reverted.
BUG=595137
Committed: https://crrev.com/7f405ec2b6914d482d081d2cb5d80bdf5226bd57
Cr-Commit-Position: refs/heads/master@{#411933}
Patch Set 1 #Patch Set 2 : Added clearLayoutObject method and continued steps #Patch Set 3 : Completed Node::setLayoutObject and flipped default DataUnion type from LayoutObject to ComputedSty… #Patch Set 4 : compressed Node::setLayoutObject and added Node::setComputedStyle method #
Total comments: 2
Patch Set 5 : Addressed reviewer comments #Patch Set 6 : Added TODOs to remove redundant ComputedStyle members #
Total comments: 3
Patch Set 7 : Addressed reviewer comments #Patch Set 8 : Removed ComputedStyle from ElementRareData #
Total comments: 1
Patch Set 9 : Removed Node::setComputedStyle which is not yet needed #Patch Set 10 : Removed ComputedStyle from NodeRareData, instead use existing ComputedStyle from ElementRareData #
Total comments: 3
Patch Set 11 : Addressed reviewer comments #
Total comments: 1
Patch Set 12 : Fixed LayoutObject access in Node::ensureRareData #
Total comments: 2
Patch Set 13 : Rebased #
Total comments: 11
Patch Set 14 : Addressed Elliott's comments #Messages
Total messages: 76 (28 generated)
Description was changed from ========== WIP storage of ComputedStyle. List of commits follows. WIP Node::setLayoutObject added some ComputedStyle saving WIP Node::setLayoutObject. Updated Node::mutableComputedStyle to check new HasLayoutObjectFlag flag and return ComputedStyle from the data union depending on whether or not it has rare data. Updated Node::layoutObject() to check new HasLayoutObjectFlag flag. Set and clear HasLayoutObjectFlag in Node::setLayoutObject appropriately. Moved Node::setLayoutObject from .h to .cpp file so we can later access LayoutObject member functions like ComputedStyle setter. Added ComputedStyle option to DataUnion and HasLayoutObjectFlag inidicating if the ComputedStyle is used with new getter for HasLayoutObjectFlag. Added ComputedStyle* member to NodeRareDataBase. BUG=595137 ========== to ========== WIP Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Changes the Node::m_data DataUnion to now allow a ComputedStyle, which is now the default. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Adds ComputedStyle pointer to NodeRareDataBase. Written with nainar@ Design doc: bit.ly/24nQ9UQ BUG=595137 ==========
bugsnash@chromium.org changed reviewers: + timloh@chromium.org
Tim could you please eyeball this WIP to make sure we're on the right track? Things that I plan on changing about this patch: 1. Remove ComputedStyle members that are no longer necessary, i.e. those in: a) HTMLOptionElement b) HTMLOptGroupElement c) ElementRareData 2. Compress the code in Node::setLayoutObject NB the new Node::setComputedStyle method is for HTMLOptionElement and HTMLOptGroupElement to use, so that their ComputedStyle members can be removed
On 2016/05/26 at 01:10:47, Bugs Nash wrote: > Tim could you please eyeball this WIP to make sure we're on the right track? > > Things that I plan on changing about this patch: > 1. Remove ComputedStyle members that are no longer necessary, i.e. those in: > a) HTMLOptionElement > b) HTMLOptGroupElement > c) ElementRareData > 2. Compress the code in Node::setLayoutObject > > NB the new Node::setComputedStyle method is for HTMLOptionElement and HTMLOptGroupElement to use, so that their ComputedStyle members can be removed One question in particular: In the current state of the code setLayoutObject(nullptr) is called after destroying the LayoutObject (which also destroys the ComputedStyle attached to it). This patch changes it so that calling setLayoutObject(nullptr) clears only the LayoutObject and saves the ComputedStyle, as it is my understanding that they should now be treated separately (i.e. it should be possible to clear the LayoutObject and not the ComputedStyle). However this means that setLayoutObject(nullptr) cannot be called after destroying the LayoutObject as it currently is, as the ComputedStyle member is garbage at that point. For this reason calls to setLayoutObject(nullptr) have been replaced in this patch with calls to a new method clearStyleandLayoutObject(), and nowhere is setLayoutObject(nullptr) used. For this patch I see 3 options: 1. The current state of the patch a) setLayoutObject(nullptr) keeps the ComputedStyle and clears the LayoutObject b) code that clears both the ComputedStyle and LayoutObject by calling setLayoutObject(nullptr) is changed to call clearStyleandLayoutObject() c) setLayoutObject(nullptr) is never used, but may be used in future. When it is used it should never be called after the LayoutObject is destroyed 2. a) setLayoutObject(nullptr) keeps the ComputedStyle and clears the LayoutObject b) code that clears both the ComputedStyle and LayoutObject by calling setLayoutObject(nullptr) is changed to keep the ComputedStyle (LayoutObject cannot be destroyed before calling setLayoutObject(nullptr)) c) clearStyleandLayoutObject() method not necessary 3. a) setLayoutObject(nullptr) is not allowed, instead clearStyleandLayoutObject() is used b) It is not possible to clear the Layoutobject and keep the ComputedStyle Which of these options do you think is best? Or is there another option that is better?
This patch is not intended to change behavior, right? So the current code which clears the LayoutObject (and thus also the ComputedStyle) should continue to clear both. No need to add functionality for only clearing the LayoutObject if it isn't going to be used yet. https://codereview.chromium.org/1962953002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1962953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:124: ComputedStyle* m_computedStyle; This should be a RefPtr<ComputedStyle> and should probably be on NodeRareData instead of NodeRareDataBase (afaict, this class exists only for inlining of the layoutObject() function, see wkb.ug/100057 for context) https://codereview.chromium.org/1962953002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.h:838: ComputedStyle* m_computedStyle; This probably needs lifetime management (i.e. refs and derefs when setting/clearing). The finalizer should probably also check that we don't have a computed style.
Description was changed from ========== WIP Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Changes the Node::m_data DataUnion to now allow a ComputedStyle, which is now the default. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Adds ComputedStyle pointer to NodeRareDataBase. Written with nainar@ Design doc: bit.ly/24nQ9UQ BUG=595137 ========== to ========== WIP Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Changes the Node::m_data DataUnion to now allow a ComputedStyle, which is now the default. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Adds ComputedStyle member to NodeRareDataBase so that ComputedStyle can be stored with rare data without a LayoutObject. Written with nainar@ Design doc: bit.ly/24nQ9UQ BUG=595137 ==========
Description was changed from ========== WIP Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Changes the Node::m_data DataUnion to now allow a ComputedStyle, which is now the default. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Adds ComputedStyle member to NodeRareDataBase so that ComputedStyle can be stored with rare data without a LayoutObject. Written with nainar@ Design doc: bit.ly/24nQ9UQ BUG=595137 ========== to ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle, which is now the default. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Adds ComputedStyle member to NodeRareDataBase so that ComputedStyle can be stored with rare data without a LayoutObject. Written with nainar@ BUG=595137 ==========
Patchset #6 (id:100001) has been deleted
Patchset #6 (id:120001) has been deleted
Patchset #5 (id:80001) has been deleted
On 2016/05/26 at 07:06:37, timloh wrote: > This patch is not intended to change behavior, right? So the current code which clears the LayoutObject (and thus also the ComputedStyle) should continue to clear both. No need to add functionality for only clearing the LayoutObject if it isn't going to be used yet. > > https://codereview.chromium.org/1962953002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Node.h (right): > > https://codereview.chromium.org/1962953002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Node.h:124: ComputedStyle* m_computedStyle; > This should be a RefPtr<ComputedStyle> and should probably be on NodeRareData instead of NodeRareDataBase (afaict, this class exists only for inlining of the layoutObject() function, see wkb.ug/100057 for context) > > https://codereview.chromium.org/1962953002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Node.h:838: ComputedStyle* m_computedStyle; > This probably needs lifetime management (i.e. refs and derefs when setting/clearing). The finalizer should probably also check that we don't have a computed style. Done
Just to confirm, are we moving the ComputedStyle up from ElementRareData to NodeRareData because we need these for Text nodes? The ComputedStyle on ElementRareData should be removed in this patch -- right now it has the exact same name as the one on its parent and has its own setters/getters which is really confusing. https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:745: V0CustomElementFlag = 1 << 28, don't these need to change too? https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:751: // 3 bits remaining. needs to be updated? https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:57: inline void Node::setComputedStyle(ComputedStyle* computedStyle) should take PassRefPtr<ComputedStyle>
On 2016/06/01 at 07:36:38, timloh wrote: > Just to confirm, are we moving the ComputedStyle up from ElementRareData to NodeRareData because we need these for Text nodes? We need a ComputedStyle on NodeRareData to handle the case where a Node has rare data and a ComputedStyle and does not have a LayoutObject > > The ComputedStyle on ElementRareData should be removed in this patch -- right now it has the exact same name as the one on its parent and has its own setters/getters which is really confusing. Will work on this. > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Node.h (right): > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Node.h:745: V0CustomElementFlag = 1 << 28, > don't these need to change too? Yes. Must have been a rebase mistake... > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Node.h:751: // 3 bits remaining. > needs to be updated? Done. > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/NodeComputedStyle.h:57: inline void Node::setComputedStyle(ComputedStyle* computedStyle) > should take PassRefPtr<ComputedStyle> Done.
On 2016/06/01 at 08:39:29, Bugs Nash wrote: > On 2016/06/01 at 07:36:38, timloh wrote: > > Just to confirm, are we moving the ComputedStyle up from ElementRareData to NodeRareData because we need these for Text nodes? > > We need a ComputedStyle on NodeRareData to handle the case where a Node has rare data and a ComputedStyle and does not have a LayoutObject > > > > > The ComputedStyle on ElementRareData should be removed in this patch -- right now it has the exact same name as the one on its parent and has its own setters/getters which is really confusing. > > Will work on this. > > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/dom/Node.h (right): > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/dom/Node.h:745: V0CustomElementFlag = 1 << 28, > > don't these need to change too? > > Yes. Must have been a rebase mistake... > > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/dom/Node.h:751: // 3 bits remaining. > > needs to be updated? > > Done. > > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/dom/NodeComputedStyle.h:57: inline void Node::setComputedStyle(ComputedStyle* computedStyle) > > should take PassRefPtr<ComputedStyle> > > Done. Having a problem when changing the ElementRareData to use the NodeRareData's ComputedStyle and not sure of the best solution The problem is: - Currently ElementRareData has both a LayoutObject (from NodeRareDataBase) and a ComputedStyle, which are treated separately. - The method ElementRareData::clearComputedStyle() method is used to clear the ComputedStyle only, leaving the LayoutObject (which has its own ComputedStyle that has not been cleared). - There are methods in LayoutObject that assume there is a ComputedStyle attached to it and crash if there is not, e.g. LayoutObject::isColumnSpanAll() - My new ComputedStyle on NodeRareDataBase is only used if there is no LayoutObject, otherwise the ComputedStyle on the LayoutObject is used - If the ComputedStyle on the LayoutObject is cleared and the LayoutObject is not cleared, crashes can happen because it's assumed that this is not the case Potential solutions I see are: 1. Keep LayoutObject and ComputedStyle separate in NodeRareData as they have been in ElementRareData 2. Remove the assumption that all LayoutObjects are associated with a ComputedStyle - i.e. check there is a ComputedStyle before accessing it from within LayoutObject 3. Clear style AND layout instead of just style where ElementRareData::clearComputedStyle() is called I'm leaning towards solution 1 because it seems the simplest and closest to the current behaviour. Your thoughts?
On 2016/06/01 at 09:22:01, Bugs Nash wrote: > On 2016/06/01 at 08:39:29, Bugs Nash wrote: > > On 2016/06/01 at 07:36:38, timloh wrote: > > > Just to confirm, are we moving the ComputedStyle up from ElementRareData to NodeRareData because we need these for Text nodes? > > > > We need a ComputedStyle on NodeRareData to handle the case where a Node has rare data and a ComputedStyle and does not have a LayoutObject > > > > > > > > The ComputedStyle on ElementRareData should be removed in this patch -- right now it has the exact same name as the one on its parent and has its own setters/getters which is really confusing. > > > > Will work on this. > > > > > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/dom/Node.h (right): > > > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/dom/Node.h:745: V0CustomElementFlag = 1 << 28, > > > don't these need to change too? > > > > Yes. Must have been a rebase mistake... > > > > > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/dom/Node.h:751: // 3 bits remaining. > > > needs to be updated? > > > > Done. > > > > > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): > > > > > > https://codereview.chromium.org/1962953002/diff/160001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/dom/NodeComputedStyle.h:57: inline void Node::setComputedStyle(ComputedStyle* computedStyle) > > > should take PassRefPtr<ComputedStyle> > > > > Done. > > Having a problem when changing the ElementRareData to use the NodeRareData's ComputedStyle and not sure of the best solution > > The problem is: > - Currently ElementRareData has both a LayoutObject (from NodeRareDataBase) and a ComputedStyle, which are treated separately. > - The method ElementRareData::clearComputedStyle() method is used to clear the ComputedStyle only, leaving the LayoutObject (which has its own ComputedStyle that has not been cleared). > - There are methods in LayoutObject that assume there is a ComputedStyle attached to it and crash if there is not, e.g. LayoutObject::isColumnSpanAll() > - My new ComputedStyle on NodeRareDataBase is only used if there is no LayoutObject, otherwise the ComputedStyle on the LayoutObject is used > - If the ComputedStyle on the LayoutObject is cleared and the LayoutObject is not cleared, crashes can happen because it's assumed that this is not the case > > Potential solutions I see are: > 1. Keep LayoutObject and ComputedStyle separate in NodeRareData as they have been in ElementRareData > 2. Remove the assumption that all LayoutObjects are associated with a ComputedStyle - i.e. check there is a ComputedStyle before accessing it from within LayoutObject > 3. Clear style AND layout instead of just style where ElementRareData::clearComputedStyle() is called > > I'm leaning towards solution 1 because it seems the simplest and closest to the current behaviour. Your thoughts? Have updated the patch using this solution
On 2016/06/01 08:39:29, Bugs Nash wrote: > On 2016/06/01 at 07:36:38, timloh wrote: > > Just to confirm, are we moving the ComputedStyle up from ElementRareData to > NodeRareData because we need these for Text nodes? > > We need a ComputedStyle on NodeRareData to handle the case where a Node has rare > data and a ComputedStyle and does not have a LayoutObject I'm asking about having this on *Node*RareData instead of *Element*RareData. I understand that we need to have the ComputedStyle on Node so we can union with the rare data and layout object, but are we actually using this on Node or is it sufficient to be an Element concept?
On 2016/06/01 09:22:01, Bugs Nash wrote: > Having a problem when changing the ElementRareData to use the NodeRareData's > ComputedStyle and not sure of the best solution > > The problem is: > - Currently ElementRareData has both a LayoutObject (from NodeRareDataBase) and > a ComputedStyle, which are treated separately. > - The method ElementRareData::clearComputedStyle() method is used to clear the > ComputedStyle only, leaving the LayoutObject (which has its own ComputedStyle > that has not been cleared). > - There are methods in LayoutObject that assume there is a ComputedStyle > attached to it and crash if there is not, e.g. LayoutObject::isColumnSpanAll() > - My new ComputedStyle on NodeRareDataBase is only used if there is no > LayoutObject, otherwise the ComputedStyle on the LayoutObject is used > - If the ComputedStyle on the LayoutObject is cleared and the LayoutObject is > not cleared, crashes can happen because it's assumed that this is not the case > > Potential solutions I see are: > 1. Keep LayoutObject and ComputedStyle separate in NodeRareData as they have > been in ElementRareData > 2. Remove the assumption that all LayoutObjects are associated with a > ComputedStyle - i.e. check there is a ComputedStyle before accessing it from > within LayoutObject > 3. Clear style AND layout instead of just style where > ElementRareData::clearComputedStyle() is called > > I'm leaning towards solution 1 because it seems the simplest and closest to the > current behaviour. Your thoughts? The only usage of (setting) ElementRareData's ComputedStyle looks like in Element::ensureComputedStyle(). These should be updated to use Node's functions so we don't have to create a rare data if not needed. We should prefer to actually maintain the current behavior, so for now we can probably make it clear the ComputedStyle if there's no LayoutObject (probably this can be simplified or removed by looking at the individual callsites).
https://codereview.chromium.org/1962953002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:68: m_data.m_computedStyle = computedStyle.get(); Actually here you can write computedStyle.leakRef() and then you don't need to explicitly call ref() above.
On 2016/06/02 at 04:49:35, timloh wrote: > On 2016/06/01 08:39:29, Bugs Nash wrote: > > On 2016/06/01 at 07:36:38, timloh wrote: > > > Just to confirm, are we moving the ComputedStyle up from ElementRareData to > > NodeRareData because we need these for Text nodes? > > > > We need a ComputedStyle on NodeRareData to handle the case where a Node has rare > > data and a ComputedStyle and does not have a LayoutObject > > I'm asking about having this on *Node*RareData instead of *Element*RareData. I understand that we need to have the ComputedStyle on Node so we can union with the rare data and layout object, but are we actually using this on Node or is it sufficient to be an Element concept? Are Elements the only Node objects that have a ComputedStyle? If so then we should be adding the new ComputedStyle storage to Element instead of Node. If not then doesn't the ComputedStyle need to be on NodeRareData rather than ElementRareData?
On 2016/06/02 05:26:56, Bugs Nash wrote: > Are Elements the only Node objects that have a ComputedStyle? That is what I was asking you :) The WK patch stores a map from Element to ComputedStyle (plus some extra metadata), and stores a set of Text objects. Do we need to store ComputedStyle objects for Text nodes?
On 2016/06/02 at 05:10:32, timloh wrote: > https://codereview.chromium.org/1962953002/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): > > https://codereview.chromium.org/1962953002/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/NodeComputedStyle.h:68: m_data.m_computedStyle = computedStyle.get(); > Actually here you can write computedStyle.leakRef() and then you don't need to explicitly call ref() above. Have removed the Node::setComputedStyle method for this patch because it's not yet needed.
On 2016/06/02 at 05:39:16, timloh wrote: > On 2016/06/02 05:26:56, Bugs Nash wrote: > > Are Elements the only Node objects that have a ComputedStyle? > > That is what I was asking you :) > > The WK patch stores a map from Element to ComputedStyle (plus some extra metadata), and stores a set of Text objects. Do we need to store ComputedStyle objects for Text nodes? Changed to use ElementRareData's ComputedStyle
On 2016/06/22 at 04:27:34, Bugs Nash wrote: > On 2016/06/02 at 05:39:16, timloh wrote: > > On 2016/06/02 05:26:56, Bugs Nash wrote: > > > Are Elements the only Node objects that have a ComputedStyle? > > > > That is what I was asking you :) > > > > The WK patch stores a map from Element to ComputedStyle (plus some extra metadata), and stores a set of Text objects. Do we need to store ComputedStyle objects for Text nodes? > > Changed to use ElementRareData's ComputedStyle IRL conversation about Text Nodes - they use a ComputedStyle in core/editing method computedStyleOfEnclosingTextNode, but this is unlikely to be required for this work. Decided to leave the ComputedStyle on ElementRareData for now and move it if required later
lgtm but description needs updating since we're not touching NodeRareDataBase any more https://codereview.chromium.org/1962953002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1962953002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:846: DataUnion() : m_computedStyle(nullptr) { } I'd leave this as initializing the LayoutObject (it makes no difference to what the compiler will do, but the main thing stored here will be the LayoutObject) https://codereview.chromium.org/1962953002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeRareData.cpp (right): https://codereview.chromium.org/1962953002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeRareData.cpp:37: #include "core/layout/LayoutObject.h" not needed? https://codereview.chromium.org/1962953002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/1962953002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeRareData.h:27: #include "core/style/ComputedStyle.h" not needed?
Description was changed from ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle, which is now the default. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Adds ComputedStyle member to NodeRareDataBase so that ComputedStyle can be stored with rare data without a LayoutObject. Written with nainar@ BUG=595137 ========== to ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle, which is now the default. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ BUG=595137 ==========
The CQ bit was checked by bugsnash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org Link to the patchset: https://codereview.chromium.org/1962953002/#ps260001 (title: "Addressed reviewer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1962953002/260001
Description was changed from ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle, which is now the default. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ BUG=595137 ========== to ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ BUG=595137 ==========
Description was changed from ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in divorce of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ BUG=595137 ========== to ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in separation of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ BUG=595137 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in separation of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ BUG=595137 ========== to ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in separation of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ BUG=595137 Committed: https://crrev.com/11af5b8769875ef50b16cb94d7eb758def0d9b08 Cr-Commit-Position: refs/heads/master@{#401821} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/11af5b8769875ef50b16cb94d7eb758def0d9b08 Cr-Commit-Position: refs/heads/master@{#401821}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:260001) has been created in https://codereview.chromium.org/2093143002/ by rnephew@chromium.org. The reason for reverting is: Bisect is saying this is causing android per test failures. https://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5X... crbug.com/623087.
Message was sent while issue was closed.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1962953002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1962953002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:847: ComputedStyle* m_computedStyle; You'll want to replace ElementRareDatas member with this too. Otherwise you're just increasing memory usage with this patch.
Message was sent while issue was closed.
On 2016/06/24 at 17:51:14, esprehn wrote: > https://codereview.chromium.org/1962953002/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/Node.h (right): > > https://codereview.chromium.org/1962953002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/Node.h:847: ComputedStyle* m_computedStyle; > You'll want to replace ElementRareDatas member with this too. Otherwise you're just increasing memory usage with this patch. We needed to keep the ComputedStyle on ElementRareData as well for the case where you have rare data and a ComputedStyle but no LayoutObject. Since ElementRareData is rare it shouldn't have too much of an impact.
Want to reland this patch. It was reverted because it was crashing android perf bots (see https://bugs.chromium.org/p/chromium/issues/detail?id=623087) Want to try to reland because: 1) I ran the Android perf bots locally and there was no crash 2) I can no longer test the bots remotely because they are throwing an exception on master 3) Changes have been made to the bots as per bug log 4) Non perf Android bots run fine I have made a small change to this patch since lgtm in Node::ensureRareData so Tim or Elliot can you ptal?
https://codereview.chromium.org/1962953002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:56: return static_cast<ElementRareData*>(rareData)->ensureComputedStyle(); didn't you rename this recently?
Description was changed from ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in separation of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ BUG=595137 Committed: https://crrev.com/11af5b8769875ef50b16cb94d7eb758def0d9b08 Cr-Commit-Position: refs/heads/master@{#401821} ========== to ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in separation of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ Originally landed as: https://crrev.com/11af5b8769875ef50b16cb94d7eb758def0d9b08 but was reverted. BUG=595137 ==========
https://codereview.chromium.org/1962953002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:56: return static_cast<ElementRareData*>(rareData)->ensureComputedStyle(); On 2016/07/18 at 20:58:52, esprehn wrote: > didn't you rename this recently? yes, whoops forgot to rebase! Fixed
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...
https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:53: if (hasRareData()) { Alternatively I could land the patch without this section, just leave it as 'return 0' for now (no change to functionality). This definitely doesn't cause the bots to crash. This section would however need to be added before any further progress can be made, as there's no point storing ComputedStyle on Node's m_data union if it an't be accessed that way.
https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:53: if (hasRareData()) { On 2016/07/19 at 01:20:36, Bugs Nash wrote: > Alternatively I could land the patch without this section, just leave it as 'return 0' for now (no change to functionality). This definitely doesn't cause the bots to crash. This section would however need to be added before any further progress can be made, as there's no point storing ComputedStyle on Node's m_data union if it an't be accessed that way. Crashing sounds like bug... Why does it crash?
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_...)
On 2016/07/19 at 01:59:10, esprehn wrote: > https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): > > https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/NodeComputedStyle.h:53: if (hasRareData()) { > On 2016/07/19 at 01:20:36, Bugs Nash wrote: > > Alternatively I could land the patch without this section, just leave it as 'return 0' for now (no change to functionality). This definitely doesn't cause the bots to crash. This section would however need to be added before any further progress can be made, as there's no point storing ComputedStyle on Node's m_data union if it an't be accessed that way. > > Crashing sounds like bug... Why does it crash? No idea. Nothing obvious from the patch/problem line. Progress seems to be being made on bug: https://bugs.chromium.org/p/chromium/issues/detail?id=623087
Hmm, why not set the hasLayoutObject flag correctly in this patch? https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:55: DCHECK(rareData->isElementRareData()); this check is not right, ex. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... callers this on a Document, if you were correctly setting the hasLayoutObject flag in this patch you'd hit the branch above since a Document should always have a layout object, but you don't, so this would crash if the document had rare data.
On 2016/07/19 at 03:25:03, esprehn wrote: > Hmm, why not set the hasLayoutObject flag correctly in this patch? > > https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): > > https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/dom/NodeComputedStyle.h:55: DCHECK(rareData->isElementRareData()); > this check is not right, ex. > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... > > callers this on a Document, if you were correctly setting the hasLayoutObject flag in this patch you'd hit the branch above since a Document should always have a layout object, but you don't, so this would crash if the document had rare data. Elliot how is the hasLayoutObject flag not set correctly in this patch? For Node, the only place the LayoutObject is ever set is in Node::setLayoutObject, which sets the flag. In the case of Document, a LayoutView is used (and stored on the Node LayoutObject pointer). The only place this LayoutView is set is in Document::attachLayoutTree and Document::detachLayoutTree, both of which call Node::setLayoutObject which would set the flag correctly. You say that a Document should always have a LayoutObject, but I don't see evidence of that in Document's constructor.
https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:267: if (!hasRareData() && !hasLayoutObject() && m_data.m_computedStyle) !hasLayoutObject() doesn't make sense here, you can't have a LayoutObject when ~Node runs, you'd fail the RELEASE_ASSERT above and crash https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:527: setFlag(HasLayoutObjectFlag); setFlag(static_cast<bool>(layoutObject), HasLayoutObjectFlag); then no branch. :) https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:749: // 1 bit remaining. thanks for fixing this :) https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:55: DCHECK(rareData->isElementRareData()); Yeah I see how the flag is set now, I'm still nervous about this assert, calling mutableComputedStyle() on a detached document or a Text node with RareData should not crash. What makes you think we never come in here for a Text node? https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeRareData.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeRareData.h:96: bool isElementRareData() const { return m_isElementRareData; } you don't need this if you move computedStyle()/ setComputedStyle/clearComputedStyle to NodeRareData and RefPtr<ComputedStyle> m_computedStyle; there as well.
Tim PTAL at Elliott's comments re: moving ComputedStyle from ElementRareData to back to NodeRareData https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:267: if (!hasRareData() && !hasLayoutObject() && m_data.m_computedStyle) On 2016/08/05 at 21:26:26, esprehn wrote: > !hasLayoutObject() doesn't make sense here, you can't have a LayoutObject when ~Node runs, you'd fail the RELEASE_ASSERT above and crash Done https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:527: setFlag(HasLayoutObjectFlag); On 2016/08/05 at 21:26:26, esprehn wrote: > setFlag(static_cast<bool>(layoutObject), HasLayoutObjectFlag); > > then no branch. :) Done https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:55: DCHECK(rareData->isElementRareData()); On 2016/08/05 at 21:26:26, esprehn wrote: > Yeah I see how the flag is set now, I'm still nervous about this assert, calling mutableComputedStyle() on a detached document or a Text node with RareData should not crash. > > What makes you think we never come in here for a Text node? Patch 9 of this CL moved the ComputedStyle from ElementRareData to NodeRareData like you suggested. Tim suggested moving it back because Element seemed to be the only type of Node that has a ComputedStyle. Tim, any more info on that?
Tim PTAL at Elliott's comments re: moving ComputedStyle from ElementRareData to back to NodeRareData https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:267: if (!hasRareData() && !hasLayoutObject() && m_data.m_computedStyle) On 2016/08/05 at 21:26:26, esprehn wrote: > !hasLayoutObject() doesn't make sense here, you can't have a LayoutObject when ~Node runs, you'd fail the RELEASE_ASSERT above and crash Done https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:527: setFlag(HasLayoutObjectFlag); On 2016/08/05 at 21:26:26, esprehn wrote: > setFlag(static_cast<bool>(layoutObject), HasLayoutObjectFlag); > > then no branch. :) Done https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/1962953002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:55: DCHECK(rareData->isElementRareData()); On 2016/08/05 at 21:26:26, esprehn wrote: > Yeah I see how the flag is set now, I'm still nervous about this assert, calling mutableComputedStyle() on a detached document or a Text node with RareData should not crash. > > What makes you think we never come in here for a Text node? Patch 9 of this CL moved the ComputedStyle from ElementRareData to NodeRareData like you suggested. Tim suggested moving it back because Element seemed to be the only type of Node that has a ComputedStyle. Tim, any more info on that?
Text also has a computedStyle, but I don't think it ever really has rare data. I studied this more and I think it's probably right with it in ElementRareData, but your assert still scares me because calling that method on a detached document should probably not crash? -- 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.
Text also has a computedStyle, but I don't think it ever really has rare data. I studied this more and I think it's probably right with it in ElementRareData, but your assert still scares me because calling that method on a detached document should probably not crash? -- 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.
What do you think the behavior should be instead? return nullptr for ComputedStyle? On Fri, Aug 12, 2016 at 2:10 PM, Elliott Sprehn <esprehn@chromium.org> wrote: > Text also has a computedStyle, but I don't think it ever really has rare > data. I studied this more and I think it's probably right with it in > ElementRareData, but your assert still scares me because calling that > method on a detached document should probably not crash? > -- 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.
What do you think the behavior should be instead? return nullptr for ComputedStyle? On Fri, Aug 12, 2016 at 2:10 PM, Elliott Sprehn <esprehn@chromium.org> wrote: > Text also has a computedStyle, but I don't think it ever really has rare > data. I studied this more and I think it's probably right with it in > ElementRareData, but your assert still scares me because calling that > method on a detached document should probably not crash? > -- 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.
I think so? That or you can assert inActiveDocument at the top the whole function? -- 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.
I think so? That or you can assert inActiveDocument at the top the whole function? -- 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.
Lgtm let's figure this out as we go. I don't want you stuck for so long there's bigger problems to solve.
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...
On 2016/08/12 at 05:00:16, esprehn wrote: > I think so? That or you can assert inActiveDocument at the top the whole > function? > > -- > 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. > Wouldn't an assert like that cause the same problem? Crashing when this is called on a detached document?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bugsnash@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org Link to the patchset: https://codereview.chromium.org/1962953002/#ps320001 (title: "Addressed Elliott's comments")
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
Exceeded global retry quota
The CQ bit was checked by bugsnash@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 #14 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in separation of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ Originally landed as: https://crrev.com/11af5b8769875ef50b16cb94d7eb758def0d9b08 but was reverted. BUG=595137 ========== to ========== Storage of ComputedStyle separate from LayoutObject. Part of stage 1 in separation of style resolution and layout tree construction Design doc: bit.ly/24nQ9UQ Changes the Node::m_data DataUnion to now allow a ComputedStyle. Updates Node flags with new flag HasLayoutObject to indicate whether the DataUnion is a LayoutObject or a ComputedStyle. Adds new hasLayoutObject method to check flag. Updates ComputedStyle and Layoutobject getters and setters accordingly. Written with nainar@ Originally landed as: https://crrev.com/11af5b8769875ef50b16cb94d7eb758def0d9b08 but was reverted. BUG=595137 Committed: https://crrev.com/7f405ec2b6914d482d081d2cb5d80bdf5226bd57 Cr-Commit-Position: refs/heads/master@{#411933} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/7f405ec2b6914d482d081d2cb5d80bdf5226bd57 Cr-Commit-Position: refs/heads/master@{#411933}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:320001) has been created in https://codereview.chromium.org/2380503002/ by bugsnash@chromium.org. The reason for reverting is: Squad project needs redesign, no longer will use ComputedStyle on Node's data union. |
