Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2001453002:
Set ComputedStyle on Node and use that in buildOwnLayout() (Closed)
|
Created:
4 years, 7 months ago by nainar Modified:
4 years, 2 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-dom_chromium.org, blink-reviews-layout_chromium.org, blink-reviews-style_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, rwlbuis, sof, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@storage Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet ComputedStyle on Node and use that in buildOwnLayout()
This patch builds on: https://codereview.chromium.org/1962953002 to
contain Layout Tree Construction code in its own function ->
Element::buildOwnLayout(). We also add a function Node::setComputedStyle
to allow us to store the ComputedStyle on Node in case
localChange == Reattach and we need to perform a Reattach.
This patch also changes the if conditions which call layoutObject()
to check if one exists to use hasLayoutObject() instead for efficiency.
With lots of help from @bugsnash
Design doc: http://bit.ly/29MBWvf
BUG=595137
Patch Set 1 #Patch Set 2 #Patch Set 3 #
Total comments: 4
Patch Set 4 #Patch Set 5 #Patch Set 6 #Patch Set 7 : For Sasha to review #Patch Set 8 : Initial review #
Total comments: 16
Patch Set 9 : make sure ensureRareData() only uses layoutObject if flag is set #Patch Set 10 #
Total comments: 3
Patch Set 11 : setStyleInternal -> setStyle #Patch Set 12 : send out for review to test the waters #Patch Set 13 : add commit and replace ASSERT with early exit #Patch Set 14 : add commit and replace ASSERT with early exit #Patch Set 15 : setStyleInternal -> setStyle #Patch Set 16 : Rebased #Patch Set 17 : deal with Pseudo Elements and TableElement #Patch Set 18 : deal with Pseudo Elements and TableElement #Patch Set 19 : Handle pseudo and table subtype elements #Patch Set 20 : test #Patch Set 21 : Images should not crash #Patch Set 22 : Images shouldn't crash #Patch Set 23 : Images shouldn't crash #Patch Set 24 : Sending for Bugs' input #
Total comments: 4
Patch Set 25 : Send over for review #
Total comments: 1
Patch Set 26 : Remove any cases that are forcing the newStyle on the old LayoutObject #Patch Set 27 : Remove any cases that are forcing the newStyle on the old LayoutObject #Patch Set 28 : Remove any cases that are forcing the newStyle on the old LayoutObject #Patch Set 29 : Remove any cases that are forcing the newStyle on the old LayoutObject #Messages
Total messages: 83 (63 generated)
Description was changed from ========== change Remove m_computedStyle from ElementRareData and store newStyle on either elementRareData or the LayoutObject in Element::recalcOwnStyle Add Node::setComputedStyle - TODO: Should be private? Split up code in if (localChange == Reattach) branch in Element::recalcOwnStyle into new function - Element::buildOwnLayout 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 patch from issue 1962953002 at patchset 40001 (http://crrev.com/1962953002#ps40001) ========== to ========== change Remove m_computedStyle from ElementRareData and store newStyle on either elementRareData or the LayoutObject in Element::recalcOwnStyle Add Node::setComputedStyle - TODO: Should be private? Split up code in if (localChange == Reattach) branch in Element::recalcOwnStyle into new function - Element::buildOwnLayout WIP: Divorce the code So far this is how the code goes: In the case of where the display of an object is set to none, there should be no LayoutObject for sure. The ComputedStyle should either be stored on the DataUnion or on the NodeRareDataBase itself. In this case the mutableComputedStyle is returning a value that has garbage stored on it. Here is the code details. In ComputedStyleCSSValueMapping::get DIV id='zoomed_and_hidden' class='test_div' styledNode 0xe39448236e8 layoutObject (nil) style passed to function here 0x3b371ef0ca10 style from styledNode 0x3b371ef0ca10 Both of these point to the same thing hasRareData() 1 hasLayoutObject() 0 length 0xcdcdcdcdcdcdcde5 (GARBAGE) the display value is garbage too. Should be display: none - but it isn't. WHY? BUG=595137 patch from issue 1962953002 at patchset 40001 (http://crrev.com/1962953002#ps40001) ==========
Description was changed from ========== change Remove m_computedStyle from ElementRareData and store newStyle on either elementRareData or the LayoutObject in Element::recalcOwnStyle Add Node::setComputedStyle - TODO: Should be private? Split up code in if (localChange == Reattach) branch in Element::recalcOwnStyle into new function - Element::buildOwnLayout WIP: Divorce the code So far this is how the code goes: In the case of where the display of an object is set to none, there should be no LayoutObject for sure. The ComputedStyle should either be stored on the DataUnion or on the NodeRareDataBase itself. In this case the mutableComputedStyle is returning a value that has garbage stored on it. Here is the code details. In ComputedStyleCSSValueMapping::get DIV id='zoomed_and_hidden' class='test_div' styledNode 0xe39448236e8 layoutObject (nil) style passed to function here 0x3b371ef0ca10 style from styledNode 0x3b371ef0ca10 Both of these point to the same thing hasRareData() 1 hasLayoutObject() 0 length 0xcdcdcdcdcdcdcde5 (GARBAGE) the display value is garbage too. Should be display: none - but it isn't. WHY? BUG=595137 patch from issue 1962953002 at patchset 40001 (http://crrev.com/1962953002#ps40001) ========== to ========== change Remove m_computedStyle from ElementRareData and store newStyle on either elementRareData or the LayoutObject in Element::recalcOwnStyle Add Node::setComputedStyle - TODO: Should be private? Split up code in if (localChange == Reattach) branch in Element::recalcOwnStyle into new function - Element::buildOwnLayout WIP: Divorce the code So far this is how the code goes: In the case of where the display of an object is set to none, there should be no LayoutObject for sure. The ComputedStyle should either be stored on the DataUnion or on the NodeRareDataBase itself. In this case the mutableComputedStyle is returning a value that has garbage stored on it. Here is the code details. In ComputedStyleCSSValueMapping::get DIV id='zoomed_and_hidden' class='test_div' styledNode 0xe39448236e8 layoutObject (nil) style passed to function here 0x3b371ef0ca10 style from styledNode 0x3b371ef0ca10 Both of these point to the same thing hasRareData() 1 hasLayoutObject() 0 length 0xcdcdcdcdcdcdcde5 (GARBAGE) the display value is garbage too. Should be display: none - but it isn't. WHY? The test case here is: <html> <style> #zoomed_and_hidden { display: none; background: orange; } </style> <div id="zoomed_and_hidden" class="test_div"> This div is has a zoom value of "2" and is hidden. It has a width of 300px. </div> <div id="test"></div> <script> var zoomedAndHidden = document.getElementById("zoomed_and_hidden"); var computedWidthHidden = parseFloat(getComputedStyle(zoomedAndHidden).background); test.innerHTML = computedWidthHidden; </script> </html> The issue is essentially getting the computedStyle on an element that has display set to none BUG=595137 patch from issue 1962953002 at patchset 40001 (http://crrev.com/1962953002#ps40001) ==========
Description was changed from
==========
change
Remove m_computedStyle from ElementRareData and store newStyle on either
elementRareData or the LayoutObject in Element::recalcOwnStyle
Add Node::setComputedStyle - TODO: Should be private?
Split up code in if (localChange == Reattach) branch in Element::recalcOwnStyle
into new function - Element::buildOwnLayout
WIP: Divorce the code
So far this is how the code goes:
In the case of where the display of an object is set to none, there should be no
LayoutObject for sure. The ComputedStyle should either be stored on the
DataUnion or on the NodeRareDataBase itself. In this case the
mutableComputedStyle is returning a value that has garbage stored on it.
Here is the code details.
In ComputedStyleCSSValueMapping::get
DIV id='zoomed_and_hidden' class='test_div'
styledNode 0xe39448236e8
layoutObject (nil)
style passed to function here 0x3b371ef0ca10
style from styledNode 0x3b371ef0ca10
Both of these point to the same thing
hasRareData() 1 hasLayoutObject() 0
length 0xcdcdcdcdcdcdcde5 (GARBAGE)
the display value is garbage too. Should be display: none - but it isn't.
WHY?
The test case here is:
<html>
<style>
#zoomed_and_hidden {
display: none;
background: orange;
}
</style>
<div id="zoomed_and_hidden" class="test_div">
This div is has a zoom value of "2" and is hidden. It has a width of 300px.
</div>
<div id="test"></div>
<script>
var zoomedAndHidden = document.getElementById("zoomed_and_hidden");
var computedWidthHidden =
parseFloat(getComputedStyle(zoomedAndHidden).background);
test.innerHTML = computedWidthHidden;
</script>
</html>
The issue is essentially getting the computedStyle on an element that has
display set to none
BUG=595137
patch from issue 1962953002 at patchset 40001
(http://crrev.com/1962953002#ps40001)
==========
to
==========
change
Remove m_computedStyle from ElementRareData and store newStyle on either
elementRareData or the LayoutObject in Element::recalcOwnStyle
Add Node::setComputedStyle - TODO: Should be private?
Split up code in if (localChange == Reattach) branch in Element::recalcOwnStyle
into new function - Element::buildOwnLayout
WIP: Divorce the code
So far this is how the code goes:
In the case of where the display of an object is set to none, there should be no
LayoutObject for sure. The ComputedStyle should either be stored on the
DataUnion or on the NodeRareDataBase itself. In this case the
mutableComputedStyle is returning a value that has garbage stored on it.
Here is the code details.
In ComputedStyleCSSValueMapping::get
DIV id='zoomed_and_hidden' class='test_div'
styledNode 0xe39448236e8
layoutObject (nil)
style passed to function here 0x3b371ef0ca10
style from styledNode 0x3b371ef0ca10
Both of these point to the same thing
hasRareData() 1 hasLayoutObject() 0
length 0xcdcdcdcdcdcdcde5 (GARBAGE)
the display value is garbage too. Should be display: none - but it isn't.
WHY?
The test case here is:
<html>
<style>
#test {
display: none;
}
</style>
<div id="test"></div>
<script>
var computedWidthHidden = getComputedStyle(test).background;
</script>
The issue is essentially getting the computedStyle on an element that has
display set to none
BUG=595137
patch from issue 1962953002 at patchset 40001
(http://crrev.com/1962953002#ps40001)
==========
Description was changed from
==========
change
Remove m_computedStyle from ElementRareData and store newStyle on either
elementRareData or the LayoutObject in Element::recalcOwnStyle
Add Node::setComputedStyle - TODO: Should be private?
Split up code in if (localChange == Reattach) branch in Element::recalcOwnStyle
into new function - Element::buildOwnLayout
WIP: Divorce the code
So far this is how the code goes:
In the case of where the display of an object is set to none, there should be no
LayoutObject for sure. The ComputedStyle should either be stored on the
DataUnion or on the NodeRareDataBase itself. In this case the
mutableComputedStyle is returning a value that has garbage stored on it.
Here is the code details.
In ComputedStyleCSSValueMapping::get
DIV id='zoomed_and_hidden' class='test_div'
styledNode 0xe39448236e8
layoutObject (nil)
style passed to function here 0x3b371ef0ca10
style from styledNode 0x3b371ef0ca10
Both of these point to the same thing
hasRareData() 1 hasLayoutObject() 0
length 0xcdcdcdcdcdcdcde5 (GARBAGE)
the display value is garbage too. Should be display: none - but it isn't.
WHY?
The test case here is:
<html>
<style>
#test {
display: none;
}
</style>
<div id="test"></div>
<script>
var computedWidthHidden = getComputedStyle(test).background;
</script>
The issue is essentially getting the computedStyle on an element that has
display set to none
BUG=595137
patch from issue 1962953002 at patchset 40001
(http://crrev.com/1962953002#ps40001)
==========
to
==========
Reorganize Layout Tree Construction code to be its own function
This patch builds on: https://codereview.chromium.org/1962953002 to
contain Layout Tree Construction code in its own function ->
Element::buildOwnLayout(). We also add a function Node::setComputedStyle
to allow us to store the ComputedStyle on Node in case
localChange == Reattach and we need to perform a Reattach.
This patch also changes the if conditions which call layoutObject()
to check if one exists to use hasLayoutObject() instead for efficiency.
BUG=595137
==========
nainar@chromium.org changed reviewers: + bugsnash@chromium.org, sashab@chromium.org
Hi Sasha! Can you take a look at this patch to see if I am headed in the right direction? Thanks!
Description was changed from ========== Reorganize Layout Tree Construction code to be its own function This patch builds on: https://codereview.chromium.org/1962953002 to contain Layout Tree Construction code in its own function -> Element::buildOwnLayout(). We also add a function Node::setComputedStyle to allow us to store the ComputedStyle on Node in case localChange == Reattach and we need to perform a Reattach. This patch also changes the if conditions which call layoutObject() to check if one exists to use hasLayoutObject() instead for efficiency. BUG=595137 ========== to ========== Reorganize Layout Tree Construction code to be its own function This patch builds on: https://codereview.chromium.org/1962953002 to contain Layout Tree Construction code in its own function -> Element::buildOwnLayout(). We also add a function Node::setComputedStyle to allow us to store the ComputedStyle on Node in case localChange == Reattach and we need to perform a Reattach. This patch also changes the if conditions which call layoutObject() to check if one exists to use hasLayoutObject() instead for efficiency. With lots of help from @bugsnash BUG=595137 ==========
Some suggestions about how to approach this change that might make landing it a little easier. https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1773: return buildOwnLayout(); A suggestion that might make this a little easier to understand/track bugs for/test: 1. Land a mechanical change patch that just calls buildOwnLayout(newStyle) here, with a TODO to say //TODO(nainar): Remove the style parameter from this function 2. Make a separate patch that splits buildOwnLayout() into the two functions, an actual behavioural change From my experience it's a ton easier to land mechanical changes - they are less likely to break things, get reverted, etc. Also if anything is wrong with them you know the patch is wrong. If we can move all the significant changes for this into a tiny change at the end it'll be much easier to see and understand. :) https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:612: if (hasLayoutObject()) I think you forgot to upload the code for this function :^) Also, I would make this a separate patch, and say that you used find-and-replace to make the change :)
@sashab, Just a progress update. Will get back to you with a clean CL that builds. Thanks! https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1773: return buildOwnLayout(); In progress update: Made a new CL with the mechanical change and the TODO as you suggested here: https://codereview.chromium.org/2118393002 https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Node.cpp:612: if (hasLayoutObject()) Took a look at this. The code for hasLayoutObject is meant to be in Bugs' patch. CodeReview sees it as landed and is hence running my code on master - which doesn't have the code change I need since the CL got reverted. The only way I see of overriding this setting the dependant patch to be a copy CL of Bugs' code. And then changing that once Bugs' patch is landed.
On 2016/07/05 at 00:26:56, nainar wrote: > @sashab, > > Just a progress update. > Will get back to you with a clean CL that builds. > > Thanks! > > https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Element.cpp (right): > > https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Element.cpp:1773: return buildOwnLayout(); > In progress update: Made a new CL with the mechanical change and the TODO as you suggested here: https://codereview.chromium.org/2118393002 > > https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/Node.cpp (right): > > https://codereview.chromium.org/2001453002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/Node.cpp:612: if (hasLayoutObject()) > Took a look at this. The code for hasLayoutObject is meant to be in Bugs' patch. CodeReview sees it as landed and is hence running my code on master - which doesn't have the code change I need since the CL got reverted. The only way I see of overriding this setting the dependant patch to be a copy CL of Bugs' code. And then changing that once Bugs' patch is landed. Yup, you'll need to make a new branch off master with bugs' patch on it and rebase your patch onto it. Here's what I would do: - Make a new branch off master e.g. bugs_patch - git cl patch NNNN (where NNNN is the patch number) will apply bugs' patch to that branch - Change to your patch branch and git branch --set-upstream-to=bugs_patch - git rebase to apply bugs' patch onto yours Then re-upload and it should detect the patch correctly :)
Hi! There is currently still one failing test -> fast/shapes/shape-outside-floats/shape-outside-no-image-crash.html While I poke around that, can you PTAL? And make sure that I am on the correct path. Thanks!
Description was changed from ========== Reorganize Layout Tree Construction code to be its own function This patch builds on: https://codereview.chromium.org/1962953002 to contain Layout Tree Construction code in its own function -> Element::buildOwnLayout(). We also add a function Node::setComputedStyle to allow us to store the ComputedStyle on Node in case localChange == Reattach and we need to perform a Reattach. This patch also changes the if conditions which call layoutObject() to check if one exists to use hasLayoutObject() instead for efficiency. With lots of help from @bugsnash BUG=595137 ========== to ========== Reorganize Layout Tree Construction code to be its own function This patch builds on: https://codereview.chromium.org/1962953002 to contain Layout Tree Construction code in its own function -> Element::buildOwnLayout(). We also add a function Node::setComputedStyle to allow us to store the ComputedStyle on Node in case localChange == Reattach and we need to perform a Reattach. This patch also changes the if conditions which call layoutObject() to check if one exists to use hasLayoutObject() instead for efficiency. With lots of help from @bugsnash Design doc: http://bit.ly/29MBWvf BUG=595137 ==========
Some questions mainly. :) https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:525: if (hasRareData() && hasLayoutObject()) Why the additional hasLayoutObject() here? https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:63: if (hasRareData()) { Is hasRareData() the check you want here? Maybe you want isElement(). Did you copy this code from somewhere or is this your own logic? https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:980: if (layoutObject->node()->isPseudoElement()) Why this?
Maybe give this patch a different title to the previous one, it's confusing which is which https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:525: if (hasRareData() && hasLayoutObject()) this isn't necessary https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:64: if (hasLayoutObject()) { this isn't necessary https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:980: if (layoutObject->node()->isPseudoElement()) why is this necessary? maybe add to description
Description was changed from ========== Reorganize Layout Tree Construction code to be its own function This patch builds on: https://codereview.chromium.org/1962953002 to contain Layout Tree Construction code in its own function -> Element::buildOwnLayout(). We also add a function Node::setComputedStyle to allow us to store the ComputedStyle on Node in case localChange == Reattach and we need to perform a Reattach. This patch also changes the if conditions which call layoutObject() to check if one exists to use hasLayoutObject() instead for efficiency. With lots of help from @bugsnash Design doc: http://bit.ly/29MBWvf BUG=595137 ========== to ========== Move Layout Tree Construction code to buildOwnLayout() This patch builds on: https://codereview.chromium.org/1962953002 to contain Layout Tree Construction code in its own function -> Element::buildOwnLayout(). We also add a function Node::setComputedStyle to allow us to store the ComputedStyle on Node in case localChange == Reattach and we need to perform a Reattach. This patch also changes the if conditions which call layoutObject() to check if one exists to use hasLayoutObject() instead for efficiency. With lots of help from @bugsnash Design doc: http://bit.ly/29MBWvf BUG=595137 ==========
Description was changed from ========== Move Layout Tree Construction code to buildOwnLayout() This patch builds on: https://codereview.chromium.org/1962953002 to contain Layout Tree Construction code in its own function -> Element::buildOwnLayout(). We also add a function Node::setComputedStyle to allow us to store the ComputedStyle on Node in case localChange == Reattach and we need to perform a Reattach. This patch also changes the if conditions which call layoutObject() to check if one exists to use hasLayoutObject() instead for efficiency. With lots of help from @bugsnash Design doc: http://bit.ly/29MBWvf BUG=595137 ========== to ========== Set ComputedStyle on Node and use that in buildOwnLayout() This patch builds on: https://codereview.chromium.org/1962953002 to contain Layout Tree Construction code in its own function -> Element::buildOwnLayout(). We also add a function Node::setComputedStyle to allow us to store the ComputedStyle on Node in case localChange == Reattach and we need to perform a Reattach. This patch also changes the if conditions which call layoutObject() to check if one exists to use hasLayoutObject() instead for efficiency. With lots of help from @bugsnash Design doc: http://bit.ly/29MBWvf BUG=595137 ==========
https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:525: if (hasRareData() && hasLayoutObject()) To make sure that we only read the LayoutObject in case we have specifically set the LayoutObject and it does not contain some garbage value. Two tests are failing here fast/forms/datalist/update-range-with-datalist.html svg/custom/animate-target-removed-from-document.svg https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:525: if (hasRareData() && hasLayoutObject()) See above. https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:63: if (hasRareData()) { Yup. The ComputedStyle on Node can be stored in the following ways: If the DataUnion is a NodeRareDataBase and has a LayoutObject - set the ComputedStyle on that LayoutObject If the DataUnion is an NodeRareDataBase (specifically an ElementRareData) - set the ComputedStyle on that LayoutObject If the DataUnion is a LayoutObject - set the ComputedStyle on that LayoutObject If the DataUnion is a ComputedStyle - make it point to the new ComputedStyle passed in. Bugs' had written up a draft of this code in his CL - however didn't land it as it wasn't needed as part of that CL. I modified it a bit. https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:64: if (hasLayoutObject()) { Agreed in a screen-to-screen conversation that: Quoting the design doc here: "If NodeRareDataBase exists, the ComputedStyle will be stored within the LayoutObject, which is stored within the NodeRareDataBase on the DataUnion. If there is no LayoutObject stored on NodeRareDataBase we directly store the ComputedStyle on the NodeRareDataBase (specifically the ElementRareData)." https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp:980: if (layoutObject->node()->isPseudoElement()) Sorry this was a hack I had made to force a test to pass and investigate the issue. Yeah add one more test to the failing test list! fast/css-generated-content/before-content-with-list-marker-in-anon-block-crash.html
https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:525: if (hasRareData() && hasLayoutObject()) On 2016/07/06 at 04:51:53, nainar wrote: > To make sure that we only read the LayoutObject in case we have specifically set the LayoutObject and it does not contain some garbage value. > > Two tests are failing here > > fast/forms/datalist/update-range-with-datalist.html > svg/custom/animate-target-removed-from-document.svg it shouldn't ever return garbage anyway - it should return nullptr if there is no LayoutObject on the rare data use rareData() instead of m_data.m_rareData
https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:525: if (hasRareData() && hasLayoutObject()) On 2016/07/06 at 05:00:21, Bugs Nash wrote: > On 2016/07/06 at 04:51:53, nainar wrote: > > To make sure that we only read the LayoutObject in case we have specifically set the LayoutObject and it does not contain some garbage value. > > > > Two tests are failing here > > > > fast/forms/datalist/update-range-with-datalist.html > > svg/custom/animate-target-removed-from-document.svg > > it shouldn't ever return garbage anyway - it should return nullptr if there is no LayoutObject on the rare data > > use rareData() instead of m_data.m_rareData oh nvm about the last comment - that's not part of this patch
Some comments for how to make this a little easier to follow :) https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:525: if (hasRareData() && hasLayoutObject()) On 2016/07/06 04:51:53, nainar wrote: > See above. I think you want if (hasRareData() && m_data.m_rareData->haslayoutObject())? https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:63: if (hasRareData()) { On 2016/07/06 04:51:53, nainar wrote: > Yup. The ComputedStyle on Node can be stored in the following ways: > > If the DataUnion is a NodeRareDataBase and has a LayoutObject - set the > ComputedStyle on that LayoutObject > If the DataUnion is an NodeRareDataBase (specifically an ElementRareData) - set > the ComputedStyle on that LayoutObject > If the DataUnion is a LayoutObject - set the ComputedStyle on that LayoutObject > If the DataUnion is a ComputedStyle - make it point to the new ComputedStyle > passed in. > > Bugs' had written up a draft of this code in his CL - however didn't land it as > it wasn't needed as part of that CL. I modified it a bit. Could you re-write this as a single if-statement with 4 branches? Might be a little easier to understand. Put what you've explained above in comments if you can, in each branch. :)
https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.h (right): https://codereview.chromium.org/2001453002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.h:525: if (hasRareData() && hasLayoutObject()) On 2016/07/06 at 05:10:40, sashab wrote: > On 2016/07/06 04:51:53, nainar wrote: > > See above. > > I think you want if (hasRareData() && m_data.m_rareData->haslayoutObject())? I think just hasRareData() is sufficient
Hi! Still got two failing tests: fast/css-generated-content/before-content-with-list-marker-in-anon-block-crash.html fast/shapes/shape-outside-floats/shape-outside-no-image-crash.html But PTAL? https://codereview.chromium.org/2001453002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (left): https://codereview.chromium.org/2001453002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:283: m_data.m_rareData = ElementRareData::create(m_data.m_layoutObject); @bugsnash, These two lines should be changed in your patch - since this is caused by adding ComputedStyle on the DataUnion.
https://codereview.chromium.org/2001453002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Node.cpp (left): https://codereview.chromium.org/2001453002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Node.cpp:283: m_data.m_rareData = ElementRareData::create(m_data.m_layoutObject); On 2016/07/06 at 07:09:03, nainar wrote: > @bugsnash, > > These two lines should be changed in your patch - since this is caused by adding ComputedStyle on the DataUnion. Good spot
Definitely looks good & on the right, track, just gotta fix those failing tests. :) https://codereview.chromium.org/2001453002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/2001453002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:61: inline void Node::setComputedStyle(PassRefPtr<ComputedStyle> computedStyle) This is SO much easier to read now, thx.
The CQ bit was checked by nainar@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by nainar@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by nainar@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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nainar@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by nainar@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nainar@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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nainar@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 checked by nainar@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Sending to Bugs for input: The current crashing tests in fast land are fast/css-generated-content/before-content-with-list-marker-in-anon-block-crash.html fast/shapes/shape-outside-floats/shape-outside-no-image-crash.html The stack trace is as follows: Crash log for renderer (pid <unknown>): STDOUT: PASS if no assert or crash on debug STDOUT: STDERR: ASSERTION FAILED: layoutObject->isBoxModelObject() && layoutObject->hasLayer() STDERR: ../../third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp(977) : bool blink::ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects(blink::FrameView *) const STDERR: 1 0x7f33d1cc2004 blink::ScrollingCoordinator::mainThreadScrollingReasons() const STDERR: 2 0x7f33d1cc0f13 blink::ScrollingCoordinator::updateAfterCompositingChangeIfNeeded() STDERR: 3 0x7f33d1b34e10 blink::FrameView::updateLifecyclePhasesInternal(blink::DocumentLifecycle::LifecycleState) STDERR: 4 0x7f33d1cae3fa blink::PageAnimator::updateAllLifecyclePhases(blink::LocalFrame&) STDERR: 5 0x7f33d310ef2f blink::WebViewImpl::updateAllLifecyclePhases() STDERR: 6 0x7f33d2f9803a STDERR: 7 0x7f33d34e3d39 STDERR: 8 0x7f33d7537596 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) STDERR: 9 0x7f33d34d6197 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(blink::scheduler::internal::WorkQueue*, blink::scheduler::internal::TaskQueueImpl::Task*) STDERR: 10 0x7f33d34d4d49 blink::scheduler::TaskQueueManager::DoWork(base::TimeTicks, bool) STDERR: 11 0x7f33d7537596 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) STDERR: 12 0x7f33d7561f05 base::MessageLoop::RunTask(base::PendingTask const&) STDERR: 13 0x7f33d7562268 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) STDERR: 14 0x7f33d756261b base::MessageLoop::DoWork() STDERR: 15 0x7f33d7563dee base::MessagePumpDefault::Run(base::MessagePump::Delegate*) STDERR: 16 0x7f33d7561a01 base::MessageLoop::RunHandler() STDERR: 17 0x7f33d758fe20 base::RunLoop::Run() STDERR: 18 0x7f33d6a3cab8
https://codereview.chromium.org/2001453002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/2001453002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:64: // printf("In Node::setComputedStyle nodeName %s hasRareData %d hasLayoutObject %d computedStyle %p\n", nodeName().ascii().data(), hasRareData(), hasLayoutObject(), mutableComputedStyle()); forgot to delete this? https://codereview.chromium.org/2001453002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:72: // If the DataUnion is an NodeRareDataBase (specifically an ElementRareData) without a LayoutObject- set the ComputedStyle on that LayoutObject. should be 'a NodeRareDataBase' and 'set the ComputedStyle on the rare data', also missing space before dash https://codereview.chromium.org/2001453002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:74: if (rareData->isElementRareData()) This should be a DCHECK. Since setComputedStyle is only ever called from Element::recalcOwnStyle, it will never be called on a non-Element Node.
https://codereview.chromium.org/2001453002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/NodeComputedStyle.h (right): https://codereview.chromium.org/2001453002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/NodeComputedStyle.h:83: // If the DataUnion is a ComputedStyle - make it point to the new ComputedStyle passed in. this case also covers the DataUnion being null
The CQ bit was checked by nainar@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 nainar@chromium.org
Sending for input:
The current crashing tests in fast land are
fast/css-generated-content/before-content-with-list-marker-in-anon-block-crash.html
I have narrowed it down to the case where in this code path
citeNode = document.createElement('cite');
citeNode.setAttribute('class', 'c19');
document.documentElement.appendChild(citeNode);
// cite has a display of table-row making it's LayoutObject a
LayoutTableRow.
parentDivListItem = document.createElement('div');
parentDivListItem.setAttribute('class', 'c9');
// cite should no longer be a LayoutTableRow but is still being interpreted
as one due to which we are going down a different code path than before causing
us to lose information about PaintLayers.
Stack trace is here:
ASSERTION FAILED: layoutObject->isBoxModelObject() && layoutObject->hasLayer()
../../third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp(978)
: bool
blink::ScrollingCoordinator::hasVisibleSlowRepaintViewportConstrainedObjects(blink::FrameView
*) const
1 0x7f1a4bf198f6 blink::ScrollingCoordinator::mainThreadScrollingReasons()
const
2 0x7f1a4bf18803
blink::ScrollingCoordinator::updateAfterCompositingChangeIfNeeded()
3 0x7f1a4ba2d9ca
blink::FrameView::updateLifecyclePhasesInternal(blink::DocumentLifecycle::LifecycleState)
4 0x7f1a4bf0468a
blink::PageAnimator::updateAllLifecyclePhases(blink::LocalFrame&)
5 0x7f1a4d3fb61f blink::WebViewImpl::updateAllLifecyclePhases()
6 0x7f1a4f2ecac1
cc::ProxyMain::BeginMainFrame(std::unique_ptr<cc::BeginMainFrameAndCommitState,
std::default_delete<cc::BeginMainFrameAndCommitState> >)
7 0x7f1a4f300663
8 0x7f1a4fbdc294 base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask const&)
9 0x7f1a4dfaf27c
blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue(blink::scheduler::internal::WorkQueue*)
10 0x7f1a4dfadb24 blink::scheduler::TaskQueueManager::DoWork(base::TimeTicks,
bool)
11 0x7f1a4fbdc294 base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask const&)
12 0x7f1a4fc07325 base::MessageLoop::RunTask(base::PendingTask const&)
13 0x7f1a4fc076f8 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask)
14 0x7f1a4fc07b1b base::MessageLoop::DoWork()
15 0x7f1a4fc092ba base::MessagePumpDefault::Run(base::MessagePump::Delegate*)
16 0x7f1a4fc06e21 base::MessageLoop::RunHandler()
17 0x7f1a4fc340e0 base::RunLoop::Run()
18 0x7f1a50df1aa8
19 0x7f1a50f1262f
20 0x7f1a50f12f0f
21 0x7f1a50f13993
22 0x7f1a50f121f0 content::ContentMain(content::ContentMainParams const&)
23 0x46716b
24 0x7f1a47edcf45 __libc_start_main
25 0x467061
https://codereview.chromium.org/2001453002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/2001453002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1566: void LayoutObject::setStyleFromNodeOrElement(PassRefPtr<ComputedStyle> computedStyle) method name should describe what it does, not where it is called from could this logic should be added to the setStyle method?
The CQ bit was checked by nainar@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nainar@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nainar@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 nainar@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nainar@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) |
