Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2013693002:
[css-tables] Set table and cell widths dirty when section border changes (Closed)
|
Created:
4 years, 7 months ago by dgrogan Modified:
4 years, 5 months ago CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[css-tables] Set table and cell widths dirty when section border changes
When borders collapse, section borders affect both the table width and
cell widths. Previously, tables and cells would use the old width when a
section's border changed.
BUG=613728
Committed: https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1
Cr-Commit-Position: refs/heads/master@{#404994}
Patch Set 1 #
Total comments: 9
Patch Set 2 : refactor and remove some table->setPreferredLogicalWidthsDirty #
Total comments: 1
Patch Set 3 : now with layout test #
Total comments: 2
Patch Set 4 : add back table->setPreferredLogicalWidthsDirty #Patch Set 5 : Tot #Patch Set 6 : comments about setChildNeedsLayout #
Total comments: 1
Patch Set 7 : add optional param and remove new layout test #
Total comments: 8
Patch Set 8 : respond to comments #Patch Set 9 : ToT and corresponding update to cached-change-tbody-border-width-expected.txt #Patch Set 10 : re-add layout test #
Total comments: 2
Patch Set 11 : switched from bool to enum #Patch Set 12 : handle case where row is null #Messages
Total messages: 49 (14 generated)
dgrogan@chromium.org changed reviewers: + mstensho@opera.com, wangxianzhu@chromium.org
Morten, can you review the logic? And if you know the TODOs off the top of your head, that'd be great. If not I will ask eae@. Xianzhu, can you ensure that the paint diff is correct?
https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right): https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:80: // TODO(dgrogan): Do we need to setPreferredLogicalWidthsDirty even when !diff.needsFullLayout()? No? :) See my other comment. https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:119: // TODO(dgrogan): Where should this go? It is duplicated in LayoutTableRow.cpp. There's a common LayoutTableBoxComponent base class now, so maybe there? https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:138: // TODO(dgrogan): Do we need to setPreferredLogicalWidthsDirty even when !diff.needsFullLayout()? Can we really schedule for layout as part of preferred logical widths computations? If we can't (and I really hope we can't), I cannot imagine how that could be necessary. Doing a full layout without recalculating preferred logical widths could in theory be sensible in some cases, but the opposite seems strange to me. https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:148: childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis); Why MarkOnlyThis? If that's not necessary (or even wrong), I'd say that this code also looks very similar to what's in TableLayoutAlgorithmFixed::willChangeTableLayout(). Maybe we need a LayoutTableSection::setPreferredLogicalWidthsDirtyOnAllCells() or something like that?
On 2016/05/25 01:12:18, dgrogan wrote: > Morten, can you review the logic? And if you know the TODOs off the top of your > head, that'd be great. If not I will ask eae@. > > Xianzhu, can you ensure that the paint diff is correct? The paint diff looks good. About the code, I have a concern: change of collapsed border width affects not only the contained cells, but also the adjacent cells. Can setting preferredLogicalWidthsDirty of contained cells ensure correct layout of affected adjacent cells? (Also it seems not necessary to set preferredLogicalWidthsDirty for inner cells that are not affected by the collapsed border width, but this is not a big deal.)
https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:119: // TODO(dgrogan): Where should this go? It is duplicated in LayoutTableRow.cpp. On 2016/05/25 12:18:28, mstensho wrote: > There's a common LayoutTableBoxComponent base class now, so maybe there? Yes, good call. https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:138: // TODO(dgrogan): Do we need to setPreferredLogicalWidthsDirty even when !diff.needsFullLayout()? On 2016/05/25 12:18:28, mstensho wrote: > Can we really schedule for layout as part of preferred logical widths > computations? If we can't (and I really hope we can't), I cannot imagine how > that could be necessary. Doing a full layout without recalculating preferred > logical widths could in theory be sensible in some cases, but the opposite seems > strange to me. This reasoning makes sense to me. I just didn't know what cases caused needsFullLayout to be set. I now see that changed border widths sets needsFullLayout in ComputedStyle::diffNeedsFullLayoutAndPaintInvalidation. https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:148: childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis); On 2016/05/25 12:18:28, mstensho wrote: > Why MarkOnlyThis? The idea was that it was an optimization: parents wouldn't need their width recalculated. But that's wrong for table on line 140. But with this patch (including MarkOnlyThis) putting the table in a shrink-to-fit floating div somehow works. I don't yet know why. https://jsfiddle.net/dgrogan/3p02m578/ > If that's not necessary (or even wrong), I'd say that this > code also looks very similar to what's in > TableLayoutAlgorithmFixed::willChangeTableLayout(). > > Maybe we need a LayoutTableSection::setPreferredLogicalWidthsDirtyOnAllCells() > or something like that? I like this idea.
Note that I'm going on paternity leave at EOD tomorrow, but I suppose you could get others to take over the review if you need more time. https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:148: childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis); On 2016/05/25 20:01:09, dgrogan wrote: > On 2016/05/25 12:18:28, mstensho wrote: > > Why MarkOnlyThis? > > The idea was that it was an optimization: parents wouldn't need their width > recalculated. But that's wrong for table on line 140. But with this patch > (including MarkOnlyThis) putting the table in a shrink-to-fit floating div > somehow works. I don't yet know why. > https://jsfiddle.net/dgrogan/3p02m578/ Yes, I really think the parents DO need their preferred widths recalculated, or your fiddle would fail. Maybe the style change itself automatically marks the entire ancestry? Maybe that's why it works, even though you use MarkOnlyThis?
New patch uploaded, please take a look. Xianzhu, I think an updated border width on a table section can only affect the heights of cells in adjacent sections (if they share a bottom/top border). Widths aren't affected. When I tackle columns and column groups I'll need to ensure the code marks cells in adjacent columns. Does that make sense? And is it right as far as you know? https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:148: childBox->setPreferredLogicalWidthsDirty(MarkOnlyThis); You're right, LayoutObject::styleDidChange marks the container chain's widths as dirty when diff.needsFullLayout(). So this new code I'm working on only has to worry about affected descendants, not ancestors. https://codereview.chromium.org/2013693002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:130: // TODO(dgrogan): Do we need to call cell->setChildNeedsLayout() like LayoutTableRow::styleDidChange does? I'll dig into this tomorrow if you don't know off the top of your head.
https://codereview.chromium.org/2013693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:130: // TODO(dgrogan): Do we need to call cell->setChildNeedsLayout() like LayoutTableRow::styleDidChange does? I'll dig into this tomorrow if you don't know off the top of your head.
On 2016/05/27 00:21:38, dgrogan wrote: > New patch uploaded, please take a look. > > Xianzhu, I think an updated border width on a table section can only affect the > heights of cells in adjacent sections (if they share a bottom/top border). > Widths aren't affected. When I tackle columns and column groups I'll need to > ensure the code marks cells in adjacent columns. > > Does that make sense? And is it right as far as you know? > This sounds right. Thanks for explanation.
https://codereview.chromium.org/2013693002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:130: // TODO(dgrogan): Do we need to call cell->setChildNeedsLayout() like LayoutTableRow::styleDidChange does? On 2016/05/27 00:23:53, dgrogan wrote: > I'll dig into this tomorrow if you don't know off the top of your head. My guess: yes, you need to do that. I'm not aware of any mechanism that detects preferred width changes and marks for layout based on that. But please dig. :) I'm not super-familiar with the preferred widths calculation machinery.
dgrogan@chromium.org changed reviewers: + eae@chromium.org
Emil, I've stalled working on this, could you look it over and then we can talk about it in person either in Munich or after? https://codereview.chromium.org/2013693002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right): https://codereview.chromium.org/2013693002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:79: // TODO(dgrogan) Changing this to setNeedsLayout doesn't cause eae@, I'm getting diminishing returns trying to understand how our various needsLayout bits work -- perhaps we could get together in person to discuss...
On 2016/06/10 22:51:11, dgrogan wrote: > Emil, I've stalled working on this, could you look it over and then we can talk > about it in person either in Munich or after? > > https://codereview.chromium.org/2013693002/diff/100001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right): > > https://codereview.chromium.org/2013693002/diff/100001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:79: // TODO(dgrogan) > Changing this to setNeedsLayout doesn't cause > eae@, I'm getting diminishing returns trying to understand how our various > needsLayout bits work -- perhaps we could get together in person to discuss... Understandable. I'll set aside some time for a meeting where we can go through these things in person. Thanks for sticking iwth it!
Emil, this is ready for a real look. https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right): https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:79: // TODO(dgrogan) Add a layout test showing that setChildNeedsLayout is needed instead of setNeedsLayout. I failed to generate one :( But this patch can land without it.
Looking good but have a few questions. https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp (right): https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:30: return oldStyle->borderLeftWidth() != newStyle->borderLeftWidth() "return oldStyle->border() != newStyle->border()" will compare the BorderData objects. It will return true if the style changes in addition to the width but triggering layout in that case doesn't seem too bad. Especially not if it simplifies the logic. Another option would be to add a sizeEquals method to BorderData and using that. return return oldStyle->border().sizeEquals(newStyle->border()); https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableRow.cpp (right): https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableRow.cpp:79: // TODO(dgrogan) Add a layout test showing that setChildNeedsLayout is needed instead of setNeedsLayout. On 2016/07/01 20:02:26, dgrogan wrote: > I failed to generate one :( But this patch can land without it. Sadness. Let's try to create one together at some point. https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1227: if (!row) When would row ever be null? Sparse tables?
eae: ready for another look trchen: could you take a look at the comment in LayoutTableSection.cpp? https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp (right): https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:30: return oldStyle->borderLeftWidth() != newStyle->borderLeftWidth() On 2016/07/01 22:20:25, eae wrote: > "return oldStyle->border() != newStyle->border()" will compare the BorderData > objects. It will return true if the style changes in addition to the width but > triggering layout in that case doesn't seem too bad. Especially not if it > simplifies the logic. > > Another option would be to add a sizeEquals method to BorderData and using that. > > return return oldStyle->border().sizeEquals(newStyle->border()); I like this option... keep the logic about borders in the BorderData class. https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1227: if (!row) On 2016/07/01 22:20:25, eae wrote: > When would row ever be null? Sparse tables? Good question. I don't think it will be. I cargo culted the code from TableLayoutAlgorithmFixed.cpp. It was added in https://crrev.com/27262002 (Need to set preferred logical widths dirty bits when switching table layout). Adding trchen@ to see if he remembers what it was for. Removing the conditional doesn't cause any layout tests to fail -- what do you think of the strategy hinted at by the comments? Meaning: (1) collect crash reports for at most 1 release (2) if there are none replace the CHECK with DCHECK (3) if there are some re-add the conditional at least with comments about why it's there, hopefully with a layout test that exercises it
On 2016/07/06 23:10:29, dgrogan wrote: > eae: ready for another look > trchen: could you take a look at the comment in LayoutTableSection.cpp? > > https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp (right): > > https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/LayoutTableBoxComponent.cpp:30: return > oldStyle->borderLeftWidth() != newStyle->borderLeftWidth() > On 2016/07/01 22:20:25, eae wrote: > > "return oldStyle->border() != newStyle->border()" will compare the BorderData > > objects. It will return true if the style changes in addition to the width but > > triggering layout in that case doesn't seem too bad. Especially not if it > > simplifies the logic. > > > > Another option would be to add a sizeEquals method to BorderData and using > that. > > > > return return oldStyle->border().sizeEquals(newStyle->border()); > > I like this option... keep the logic about borders in the BorderData class. Yay! > > https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): > > https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1227: if (!row) > On 2016/07/01 22:20:25, eae wrote: > > When would row ever be null? Sparse tables? > > Good question. I don't think it will be. I cargo culted the code from > TableLayoutAlgorithmFixed.cpp. It was added in https://crrev.com/27262002 (Need > to set preferred logical widths dirty bits when switching table layout). Adding > trchen@ to see if he remembers what it was for. Thanks, scanning through the code didn't immidiately identify any cases where it would be null. > > Removing the conditional doesn't cause any layout tests to fail -- what do you > think of the strategy hinted at by the comments? Meaning: > (1) collect crash reports for at most 1 release > (2) if there are none replace the CHECK with DCHECK > (3) if there are some re-add the conditional at least with comments about why > it's there, hopefully with a layout test that exercises it Sounds like a good plan to me! LGTM based on this discussion.
dgrogan@chromium.org changed reviewers: + trchen@chromium.org
Actually +trchen this time trchen, could you look at the comments in https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou...
dgrogan@chromium.org changed required reviewers: + eae@chromium.org, trchen@chromium.org, wangxianzhu@chromium.org
I decided to readd a layout test that checks only layout stuff, not paint stuff. And the paint golden file changed since it was last looked at. I'm waiting for 3 more LGTM's: eae (again): just for the layout test trchen: the comments in https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... Xianzhu: updated paint golden file
What do you mean the "paint golden file"? https://codereview.chromium.org/2013693002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:131: markAllCellsWidthsDirtyAndOrNeedsLayout(true /*markNeedsLayout*/); This is a good example that we prefer enum to bool as a parameter: the meaning at call site would be clear without additional comments. https://codereview.chromium.org/2013693002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1228: void LayoutTableSection::markAllCellsWidthsDirtyAndOrNeedsLayout(bool markNeedsLayout) Please change the parameter to an enum (or you can keep the bool if you would like to do the following soon). In the future you can consider to add template <Function> forEachCell(Function f) { ... f(*cell); ... } in LayoutTableSection, and the call site at line 131 would be: forEachCell([](LayoutTableCell& cell) { cell.setPreferredLogicalWidthsDirty(); cell.setChildNeedsLayout(); });
By "paint golden file" I meant border-collapsing/cached-change-tbody-border-width-expected.txt. I don't know how to interpret the diff so would like you to check it. I switched the naked bool param to an enum. I dislike that style rule, but alas, you are correct, it IS the rule.
On 2016/07/08 17:18:08, dgrogan wrote: > By "paint golden file" I meant > border-collapsing/cached-change-tbody-border-width-expected.txt. I don't know > how to interpret the diff so would like you to check it. > We are using the "golden file" to check two things: - under-invalidation: whether the invalidated rects cover all changed part (for correctness) and we invalidate all changed objects. Now we have code to check for under-invalidation so if the test doesn't output any underInvalidations or crash at object-based under-invalidation checking, it's good. - over-invalidation: whether we invalidate too more than needed (for performance). This can be checked using the 'overlay' link after the test in the test result. Green and red boxes visualize the actual and expected repaint rects in the test results before the test is rebaselined. It's good if the green ones cover the same area as the red ones. If green ones cover more area we should check if the performance impact is acceptable. > I switched the naked bool param to an enum. I dislike that style rule, but alas, > you are correct, it IS the rule. lgtm.
On 2016/07/08 17:39:02, Xianzhu wrote: > On 2016/07/08 17:18:08, dgrogan wrote: > > By "paint golden file" I meant > > border-collapsing/cached-change-tbody-border-width-expected.txt. I don't know > > how to interpret the diff so would like you to check it. > > > > We are using the "golden file" to check two things: > - under-invalidation: whether the invalidated rects cover all changed part (for > correctness) and we invalidate all changed objects. Now we have code to check > for under-invalidation so if the test doesn't output any underInvalidations or > crash at object-based under-invalidation checking, it's good. > - over-invalidation: whether we invalidate too more than needed (for > performance). This can be checked using the 'overlay' link after the test in the > test result. Green and red boxes visualize the actual and expected repaint rects > in the test results before the test is rebaselined. It's good if the green ones > cover the same area as the red ones. If green ones cover more area we should > check if the performance impact is acceptable. Thanks for the explanation. In this case, there is a new sibling to "paintInvalidations": "objectPaintInvalidations". What is the difference between those two? (Also, thanks for the lambda/template code in the previous round, if that function grows more options that's what I'll switch to.)
On 2016/07/08 18:30:32, dgrogan wrote: > On 2016/07/08 17:39:02, Xianzhu wrote: > > On 2016/07/08 17:18:08, dgrogan wrote: > > > By "paint golden file" I meant > > > border-collapsing/cached-change-tbody-border-width-expected.txt. I don't > know > > > how to interpret the diff so would like you to check it. > > > > > > > We are using the "golden file" to check two things: > > - under-invalidation: whether the invalidated rects cover all changed part > (for > > correctness) and we invalidate all changed objects. Now we have code to check > > for under-invalidation so if the test doesn't output any underInvalidations or > > crash at object-based under-invalidation checking, it's good. > > - over-invalidation: whether we invalidate too more than needed (for > > performance). This can be checked using the 'overlay' link after the test in > the > > test result. Green and red boxes visualize the actual and expected repaint > rects > > in the test results before the test is rebaselined. It's good if the green > ones > > cover the same area as the red ones. If green ones cover more area we should > > check if the performance impact is acceptable. > > Thanks for the explanation. In this case, there is a new sibling to > "paintInvalidations": "objectPaintInvalidations". What is the difference between > those two? Previously they are mixed in paintInvalidations. The invalidation entries without rects were object invalidations. We changed the method of object invalidations which no longer attached to particular compositing layers, so we moved them out to the top level. Rect invaldiations are still attached on compositing layers. Object invalidations are for blink painting. PaintController caches blink painting results by objects. For each frame, we need to repaint all changed objects, and PaintController will reuse the previous results of unchanged objects. Rect invalidations are for chromium compositor to know which parts of the compositing layers need re-rasterization (basically the bits of bitmap will change on the screen). These things will keep changing in the next months when we are implementing slimming paint v2. > > (Also, thanks for the lambda/template code in the previous round, if that > function grows more options that's what I'll switch to.)
https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1227: if (!row) On 2016/07/06 23:10:29, dgrogan wrote: > On 2016/07/01 22:20:25, eae wrote: > > When would row ever be null? Sparse tables? > > Good question. I don't think it will be. I cargo culted the code from > TableLayoutAlgorithmFixed.cpp. It was added in https://crrev.com/27262002 (Need > to set preferred logical widths dirty bits when switching table layout). Adding > trchen@ to see if he remembers what it was for. > > Removing the conditional doesn't cause any layout tests to fail -- what do you > think of the strategy hinted at by the comments? Meaning: > (1) collect crash reports for at most 1 release > (2) if there are none replace the CHECK with DCHECK > (3) if there are some re-add the conditional at least with comments about why > it's there, hopefully with a layout test that exercises it I don't remember, but other functions in LayoutTableSection does the null check as well. I think it may have something to do with cells with rowspan that exceeded the last row.
https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right): https://codereview.chromium.org/2013693002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1227: if (!row) You nailed it; cells with rowspan that exceeded the last row have null LayoutTableRow objects.
The CQ bit was checked by dgrogan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org, wangxianzhu@chromium.org Link to the patchset: https://codereview.chromium.org/2013693002/#ps220001 (title: "handle case where row is null")
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
All required reviewers (with asterisk prefixes) have not yet approved this CL. No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
dgrogan@chromium.org changed required reviewers: - trchen@chromium.org
Removing trchen as required reviewer
The CQ bit was checked by dgrogan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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 dgrogan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 dgrogan@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 #12 (id:220001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [css-tables] Set table and cell widths dirty when section border changes When borders collapse, section borders affect both the table width and cell widths. Previously, tables and cells would use the old width when a section's border changed. BUG=613728 ========== to ========== [css-tables] Set table and cell widths dirty when section border changes When borders collapse, section borders affect both the table width and cell widths. Previously, tables and cells would use the old width when a section's border changed. BUG=613728 Committed: https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1 Cr-Commit-Position: refs/heads/master@{#404994} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/596c28c4ab1759719fe00b8319571fba398f48e1 Cr-Commit-Position: refs/heads/master@{#404994} |
