Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1473363003:
Invalidate first line display item clients when first line style changes (Closed)
|
Created:
5 years ago by Xianzhu Modified:
5 years ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInvalidate first line display item clients when first line style changes
It's not enough to invalidate the object having the first line style
changed, but also the display item clients (e.g. line boxes) that will
use the changed style to paint changed display items.
BUG=548705
TEST=fast/css/first-line-hover-001.html
TEST=fast/css/first-line-change-color.html
TEST=fast/css/first-line-change-color-direct.html
Committed: https://crrev.com/f7eea74c2c1e37c1af0e98637713283b6df9a167
Cr-Commit-Position: refs/heads/master@{#363663}
Patch Set 1 #
Total comments: 2
Patch Set 2 : #
Total comments: 2
Patch Set 3 : Recurse into first block child #
Total comments: 2
Patch Set 4 : Fix a typo in test (yet another mixup of local versions) #Patch Set 5 : New method #
Total comments: 3
Patch Set 6 : Handle inherited firstline style #
Total comments: 13
Patch Set 7 : Invalidate the whole first line #
Total comments: 1
Patch Set 8 : Revert changes about early returns in Element::updatePseudoStyleCache(). Will address this later #Patch Set 9 : LayoutBlockFlow -> LayoutBlock #Patch Set 10 : #Patch Set 11 : Add fast/css/button-first-line-change-color.html #Patch Set 12 : Pure paint invalildation change (Element.cpp reverted) #
Total comments: 9
Patch Set 13 : Separate block and line code #
Total comments: 13
Patch Set 14 : Address mstensho's comments #
Total comments: 12
Patch Set 15 : Add comments #Patch Set 16 : Source/core/paint/README.md #
Total comments: 7
Patch Set 17 : #Patch Set 18 : Some modifications were not saved to README.md in the previous patch #Messages
Total messages: 77 (25 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, rune@opera.com
https://codereview.chromium.org/1473363003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1847: StyleDifference diff = adjustStyleDifference(oldStyle.visualInvalidationDiff(newStyle)); Is the change in this file strictly necessary?
https://codereview.chromium.org/1473363003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1847: StyleDifference diff = adjustStyleDifference(oldStyle.visualInvalidationDiff(newStyle)); On 2015/11/24 23:21:56, chrishtr wrote: > Is the change in this file strictly necessary? I think yes. The above line ensures that StyleDifference::textDecorationOrColorChanged etc. to be translated to needsPaintInvalidation and/or needsLayout.
On 2015/11/24 at 23:26:58, wangxianzhu wrote: > https://codereview.chromium.org/1473363003/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): > > https://codereview.chromium.org/1473363003/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1847: StyleDifference diff = adjustStyleDifference(oldStyle.visualInvalidationDiff(newStyle)); > On 2015/11/24 23:21:56, chrishtr wrote: > > Is the change in this file strictly necessary? > > I think yes. > > The above line ensures that StyleDifference::textDecorationOrColorChanged etc. to be translated to needsPaintInvalidation and/or needsLayout. So then there were two bugs? Not invalidating all display item clients for line boxes, and not adjusting the style before upgrading to layout or paint invalidation as necessary?
On 2015/11/24 23:30:00, chrishtr wrote: > On 2015/11/24 at 23:26:58, wangxianzhu wrote: > > > https://codereview.chromium.org/1473363003/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): > > > > > https://codereview.chromium.org/1473363003/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/LayoutObject.cpp:1847: StyleDifference > diff = adjustStyleDifference(oldStyle.visualInvalidationDiff(newStyle)); > > On 2015/11/24 23:21:56, chrishtr wrote: > > > Is the change in this file strictly necessary? > > > > I think yes. > > > > The above line ensures that StyleDifference::textDecorationOrColorChanged etc. > to be translated to needsPaintInvalidation and/or needsLayout. > > So then there were two bugs? Not invalidating all display item clients for line > boxes, and not adjusting the style before upgrading to layout or paint > invalidation as necessary? Previously we always setNeedsLayoutAndPrefWidthsRecalcAndFullPaintInvalidation which doesn't need adjusting the style diff. All context-sensitive changes are also counted as hasDifference().
The CQ bit was checked by chrishtr@chromium.org
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1467793002 Patch 20001). Please resolve the dependency and try again.
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473363003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473363003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1473363003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1852: toLayoutBlockFlow(this)->invalidateDisplayItemClientsOfFirstLine(); The implementation of invalidateDisplayItemClientsOfFirstLine() does not seem to take into consideration that there might be 'n' block level boxes between the block(s) which get ::first-line style and the line boxes for the ::first-line. I guess that's why you get the test regression. LayoutObject::firstLineStyleDidChange is called on the layout object for the elements which matches ::first-line, not the element whose LayoutObject for the block which has the first line.
https://codereview.chromium.org/1473363003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1852: toLayoutBlockFlow(this)->invalidateDisplayItemClientsOfFirstLine(); On 2015/11/26 00:24:05, rune wrote: > The implementation of invalidateDisplayItemClientsOfFirstLine() does not seem to > take into consideration that there might be 'n' block level boxes between the > block(s) which get ::first-line style and the line boxes for the ::first-line. I > guess that's why you get the test regression. > > LayoutObject::firstLineStyleDidChange is called on the layout object for the > elements which matches ::first-line, not the element whose LayoutObject for the > block which has the first line. You are right. The latest patch set recurs into the first block child. Actually I had that version before uploading the previous patch, but somehow uploaded a wrong version. Ptal.
https://codereview.chromium.org/1473363003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1852: toLayoutBlockFlow(this)->invalidateDisplayItemClientsOfFirstLine(); How about FIRST_LINE_INHERITED. firstLineStyleDidChange() will be called on inline elements on the first line, and necessarily so, even if the ::first-line style doesn't change. I reported 562418, which I suspect is also a slimming paint regression. I suspect this CL won't fix that. Could you check?
https://codereview.chromium.org/1473363003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1852: toLayoutBlockFlow(this)->invalidateDisplayItemClientsOfFirstLine(); On 2015/11/27 09:27:11, rune wrote: > How about FIRST_LINE_INHERITED. firstLineStyleDidChange() will be called on > inline elements on the first line, and necessarily so, even if the ::first-line > style doesn't change. I reported 562418, which I suspect is also a slimming > paint regression. I suspect this CL won't fix that. Could you check? For FIRST_LINE_INHERITED, we'll use the normal paint invalidation on style change flow: mark the LayoutInline shouldDoFullPaintInvalidation during setStyle and invalidate all line boxes during paint invalidation (LayoutInline::invalidateDisplayItemClients()). crbug.com/562418 seems not a slimming paint regression because it also reproduces on Safari. It seems a style priority issue or a painting issue. Will investigate it as a separate issue.
lgtm
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1473363003/#ps40001 (title: "Recurse into first block child")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473363003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473363003/40001
The CQ bit was unchecked by wangxianzhu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org, rune@opera.com Link to the patchset: https://codereview.chromium.org/1473363003/#ps60001 (title: "Fix a typo in test (yet another mixup of local versions)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473363003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473363003/60001
The CQ bit was unchecked by wangxianzhu@chromium.org
Description was changed from ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. BUG=548705 ========== to ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. BUG=548705,562418 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-child-color.html ==========
Description was changed from ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. BUG=548705,562418 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-child-color.html ========== to ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. Also fix a bug in Element::recalcOwnStyle() that pseudoStyleCacheInvalid() is not called in some cases when pseudo style changes. The case is that a LayoutInline's first-line style may change because the LayoutInline's own style change, without ::first-line style change in any containing block. BUG=548705,562418 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-child-color.html ==========
When investigating crbug.com/562418, I found that I need a combined solution for the two bugs. Rune is right about FIRST_LINE_INHERITED. The normal paint invalidation path doesn't work for LayoutInlines because the normal paint invalidation is issued by LayoutText instead of LayoutInline. Now also invalidate line boxes of LayoutInlines when its first-line style changes.
On 2015/12/01 01:18:41, Xianzhu wrote: > When investigating crbug.com/562418, I found that I need a combined solution for > the two bugs. Rune is right about FIRST_LINE_INHERITED. The normal paint > invalidation path doesn't work for LayoutInlines because the normal paint > invalidation is issued by LayoutText instead of LayoutInline. Now also > invalidate line boxes of LayoutInlines when its first-line style changes. Ptal the latest patch set.
https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1794: bool pseudoStyleCacheIsInvalid = this->pseudoStyleCacheIsInvalid(oldStyle.get(), newStyle.get()); this-> not necessary? Also, that pseudoStyleCacheIsInvalid has a side-effect is not obvious from its name. Rename it? https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:2072: else if (firstChild()->isLayoutBlockFlow()) Add a comment explaining each side of the if ... else branch. https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutInline.cpp:1439: child->setShouldDoFullPaintInvalidation(); Why this line? https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1851: invalidateDisplayItemClientsOfFirstLine(); Won't text decoration potentially affect all lines, not just first?
Paint invalidation would also benefit from a markdown file explaining it. Would you mind starting that file with invalidation of inlines and first line?
Someone with more knowledge about the layout tree structure should probably look at the invalidateDisplayItemClientsOfFirstLine(), since I'm not too familiar with what kind of box structures one might have. https://codereview.chromium.org/1473363003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1473363003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1795: if (localChange != NoChange || pseudoStyleCacheIsInvalid || svgFilterNeedsLayerUpdate()) { I'm a bit confused about the role of diffPseudoStyles(), which affects localChange here, and pseudoStyleCacheIsInvalid(). They seem to do similar things? I guess this is the reason for the increased invalidation numbers in the scrollbar tests? I suspect this change also fixes crbug.com/563943 which I just reported. Another thing - the early returns from the for-loop in pseudoStyleCacheIsInvalid(). If we have both first-line and a different pseudo element, and we encounter the other pseudo element first in the for-loop, it seems firstLineStyleDidChange() will not be called because of the early return. We can handle that in another CL.
mstensho@opera.com changed reviewers: + mstensho@opera.com
https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/first-line-change-child-color.html (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/first-line-change-child-color.html:9: <p>Red<span id="t">This text should be green.<span>Red</p> <span> -> </span> https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:2072: else if (firstChild()->isLayoutBlockFlow()) How do we know that we have children at all? Also, this will enter any first block flow child, including floats and out-of-flow positioned objects. That's unnecessary, since they never apply ::fist-line rules from ancestors. https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutInline.cpp:1438: for (LayoutObject* child = firstChild(); child; child = child->nextSibling()) { Looks like there's nothing that stops at the end of the first line here. <div> <span>[lots of text, creating hundreds of lines]</span> </div> You'd end up invalidating the entire block in that case, wouldn't you? Conversely, there doesn't seem to be any check whether this inline occurs on a first line at all. I don't think I understand why we want this part of the method at all. I think it would be better if the code above tried harder to locate line boxes (in case of !alwaysCreateLineBoxes()). If that's to much work, how about locating the containing block and call invalidateDisplayItemClientsOfFirstLine() on it instead? But I think we should ask a line layout expert.
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/1473363003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1473363003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1795: if (localChange != NoChange || pseudoStyleCacheIsInvalid || svgFilterNeedsLayerUpdate()) { On 2015/12/01 11:09:37, rune wrote: > I'm a bit confused about the role of diffPseudoStyles(), which affects > localChange here, and pseudoStyleCacheIsInvalid(). They seem to do similar > things? I see diffPseudoStyles() compares cached pseudo styles, but pseudoStyleCacheIsInvalid() compares old cached pseudo style and new unchached pseudo style. I'm not sure when a new style will have cached pseudo style, but the two functions seem to do different things. > > I guess this is the reason for the increased invalidation numbers in the > scrollbar tests? The numbers in the test is the numbers of calls to StyleResolver::styleForElement(), pseudoStyleForElement() and styleForPage(). There are one styleForElement() call for the element's own style and one pseudoStyleForElement call for each pseudo style. diffPseudoStyles() doesn't increase the number because it accesses cached pseudo style only. > I suspect this change also fixes crbug.com/563943 which I just reported. Thanks for reporting the bug. Unfortunately this CL doesn't fix the bug. Will investigate later. > Another thing - the early returns from the for-loop in > pseudoStyleCacheIsInvalid(). If we have both first-line and a different pseudo > element, and we encounter the other pseudo element first in the for-loop, it > seems firstLineStyleDidChange() will not be called because of the early return. > We can handle that in another CL. Done in this CL. https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/first-line-change-child-color.html (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/first-line-change-child-color.html:9: <p>Red<span id="t">This text should be green.<span>Red</p> On 2015/12/01 12:29:49, mstensho wrote: > <span> -> </span> Done. https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/Element.cpp:1794: bool pseudoStyleCacheIsInvalid = this->pseudoStyleCacheIsInvalid(oldStyle.get(), newStyle.get()); On 2015/12/01 01:43:11, chrishtr wrote: > this-> not necessary? > > Also, that pseudoStyleCacheIsInvalid has a side-effect is not obvious from its > name. Rename it? Done. https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:2072: else if (firstChild()->isLayoutBlockFlow()) On 2015/12/01 01:43:11, chrishtr wrote: > Add a comment explaining each side of the if ... else branch. Done. https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlowLine.cpp:2072: else if (firstChild()->isLayoutBlockFlow()) On 2015/12/01 12:29:49, mstensho wrote: > How do we know that we have children at all? It seems that if the block has at least one non-inline child, it will be !childrenInline(), otherwise it always has a root line box, so ASSERT(!firstRootBox() || firstChild()) However the above rule looks not explicit and I changed code to make it clearer. > Also, this will enter any first > block flow child, including floats and out-of-flow positioned objects. That's > unnecessary, since they never apply ::fist-line rules from ancestors. Fixed. https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutInline.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutInline.cpp:1438: for (LayoutObject* child = firstChild(); child; child = child->nextSibling()) { On 2015/12/01 12:29:49, mstensho wrote: > Looks like there's nothing that stops at the end of the first line here. > > <div> > <span>[lots of text, creating hundreds of lines]</span> > </div> > > You'd end up invalidating the entire block in that case, wouldn't you? > > Conversely, there doesn't seem to be any check whether this inline occurs on a > first line at all. > > I don't think I understand why we want this part of the method at all. I think > it would be better if the code above tried harder to locate line boxes (in case > of !alwaysCreateLineBoxes()). If that's to much work, how about locating the > containing block and call invalidateDisplayItemClientsOfFirstLine() on it > instead? But I think we should ask a line layout expert. I think invalidating the first line of containing block is a good idea. It may invalidate more but code is much simpler and the over-invalidation doesn't seem a big issue. https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1851: invalidateDisplayItemClientsOfFirstLine(); On 2015/12/01 01:43:11, chrishtr wrote: > Won't text decoration potentially affect all lines, not just first? Here is style change affecting the first line only. Style change affecting the whole LayoutObject is handled in normal paint invalidation mechanism in setStyle(). https://codereview.chromium.org/1473363003/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1861: block->setShouldDoFullPaintInvalidation(); This is needed for rect-based invalidation. Currently this over-invalidates. Will switch to use display item client bounds for slimming paint v2.
https://codereview.chromium.org/1473363003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Element.cpp (right): https://codereview.chromium.org/1473363003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Element.cpp:1795: if (localChange != NoChange || pseudoStyleCacheIsInvalid || svgFilterNeedsLayerUpdate()) { On 2015/12/01 20:06:25, Xianzhu wrote: > On 2015/12/01 11:09:37, rune wrote: > > > Another thing - the early returns from the for-loop in > > pseudoStyleCacheIsInvalid(). If we have both first-line and a different pseudo > > element, and we encounter the other pseudo element first in the for-loop, it > > seems firstLineStyleDidChange() will not be called because of the early > return. > > We can handle that in another CL. > > Done in this CL. I reverted the changes about early returns in the latest patch set. It was incorrect and further change for crbug.com/563943 would need to change that again.
The last patch set has the following changes: - Check firstChild only (instead of trying to find the 'first-line' after floated or out-of-flow-positioned child) according to discussions in crbug.com/564242; - Moved LayoutBlockFlow::invalidateDisplayItemClientsOfFirstLine() into LayoutBlock, because sometimes a non-flow object (e.g. a LayoutButton) can also have ::first-line applied to descendant inline.
Why are all the bots red?
On 2015/12/02 01:53:38, esprehn wrote: > Why are all the bots red? They will be green with patch set 12 which reverted Element.cpp. The pseudo style change logic needs a more complete solution.
Naming typo. Otherwise good to go from my side. https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2893: static void invalidteDisplayItemClientsOfInlineBoxRecursively(InlineBox& box) invalidte -> invalidate
https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2902: void LayoutBlock::invalidateDisplayItemClientsOfFirstLine() Lines are only interesting in block containers, i.e. LayoutBlockFlow, and ideally we shouldn't be dealing with lines in LayoutBlock at all [1]. I realize that LayoutBlock needs to do *something*, since we have to support ::first-line on LayoutButton (which is a LayoutFlexibleBox). Could you perhaps move the parts that deal with lines to LayoutBlockFlow, and just do the child block thing here? [1] http://crbug.com/302024 https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2913: if (!child->isFloatingOrOutOfFlowPositioned() && child->isLayoutBlock()) Should only allow LayoutBlockFlow here. It's okay to enter this machinery for non-LayoutBlockFlow objects (because of LayoutButton), but children should be true block containers. https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1854: if (toLayoutInline(this)->firstLineBoxIncludingCulling()->isFirstLineStyle()) Can't firstLineBoxIncludingCulling() return nullptr?
Patchset #13 (id:260001) has been deleted
https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2893: static void invalidteDisplayItemClientsOfInlineBoxRecursively(InlineBox& box) On 2015/12/02 09:00:49, rune wrote: > invalidte -> invalidate Done. https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2902: void LayoutBlock::invalidateDisplayItemClientsOfFirstLine() On 2015/12/02 10:09:31, mstensho wrote: > Lines are only interesting in block containers, i.e. LayoutBlockFlow, and > ideally we shouldn't be dealing with lines in LayoutBlock at all [1]. I realize > that LayoutBlock needs to do *something*, since we have to support ::first-line > on LayoutButton (which is a LayoutFlexibleBox). > > Could you perhaps move the parts that deal with lines to LayoutBlockFlow, and > just do the child block thing here? > > [1] http://crbug.com/302024 Done. Separated this method into LayoutBlock::firstLineContainer() and LayoutBlockFlow::invalidateDisplayItemClientsOfFirstLine(). BTW, merged https://codereview.chromium.org/1490013002/ and renamed LayoutBlock::firstLineBlock() to firstLineStyleBlock() to avoid ambiguity and conflict with this CL. https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2913: if (!child->isFloatingOrOutOfFlowPositioned() && child->isLayoutBlock()) On 2015/12/02 10:09:31, mstensho wrote: > Should only allow LayoutBlockFlow here. It's okay to enter this machinery for > non-LayoutBlockFlow objects (because of LayoutButton), but children should be > true block containers. Done. Now this is in LayoutBlock::firstLineContainer(). https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1854: if (toLayoutInline(this)->firstLineBoxIncludingCulling()->isFirstLineStyle()) On 2015/12/02 10:09:31, mstensho wrote: > Can't firstLineBoxIncludingCulling() return nullptr? Not sure though didn't encounter. Added nullptr checking.
mstensho@opera.com changed reviewers: + eae@chromium.org
@eae - could you comment on one particular change here (line layout), please? Just search for "eae". :) https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1473363003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2902: void LayoutBlock::invalidateDisplayItemClientsOfFirstLine() On 2015/12/02 17:41:57, Xianzhu wrote: > On 2015/12/02 10:09:31, mstensho wrote: > > Lines are only interesting in block containers, i.e. LayoutBlockFlow, and > > ideally we shouldn't be dealing with lines in LayoutBlock at all [1]. I > realize > > that LayoutBlock needs to do *something*, since we have to support > ::first-line > > on LayoutButton (which is a LayoutFlexibleBox). > > > > Could you perhaps move the parts that deal with lines to LayoutBlockFlow, and > > just do the child block thing here? > > > > [1] http://crbug.com/302024 > > Done. Separated this method into LayoutBlock::firstLineContainer() and > LayoutBlockFlow::invalidateDisplayItemClientsOfFirstLine(). > > BTW, merged https://codereview.chromium.org/1490013002/ and renamed > LayoutBlock::firstLineBlock() to firstLineStyleBlock() to avoid ambiguity and > conflict with this CL. Acknowledged. https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:279: // Obtains the nearest enclosing block (including this block) that contributes a first-line style to our inline children. "Returns the nearest enclosing block (including this block) that contributes a first-line style to our first line" https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:280: LayoutBlock* firstLineStyleBlock() const; enclosingFirstLineStyleBlock()? https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:281: // Returns the descendant block containing the actual first line. Shouldn't this also include this block? I see that the implementation doesn't include it, but shouldn't it? https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:282: LayoutBlockFlow* firstLineContainer() const; Not sure if I'm happy with the block vs. container distinction. nearestInnerBlockWithFirstLine()? https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:3115: static void invalidateDisplayItemClientsOfInlineBoxRecursively(InlineBox& box) All of this should go into LayoutBlockFlowLine.cpp, since we have that implementation separation. https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:3120: for (InlineBox* child = toInlineFlowBox(box).firstChild(); child; child = child->nextOnLine()) @eae: can you comment on this, please? I don't know the line code well enough. https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:753: // TODO(layout-team): Remove when buttons are implemented with align-items instead A reference to crbug.com/226252 here? Not sure if "layout-team" is right here. How about TODO(cbiesinger)? The FIXME was added by him in b981273f245e746020a7afa4580e2a85580c4e32
Description was changed from ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. Also fix a bug in Element::recalcOwnStyle() that pseudoStyleCacheInvalid() is not called in some cases when pseudo style changes. The case is that a LayoutInline's first-line style may change because the LayoutInline's own style change, without ::first-line style change in any containing block. BUG=548705,562418 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-child-color.html ========== to ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. BUG=548705 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-color-direct.html ==========
https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:279: // Obtains the nearest enclosing block (including this block) that contributes a first-line style to our inline children. On 2015/12/03 12:08:10, mstensho wrote: > "Returns the nearest enclosing block (including this block) that contributes a > first-line style to our first line" Done. https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:280: LayoutBlock* firstLineStyleBlock() const; On 2015/12/03 12:08:10, mstensho wrote: > enclosingFirstLineStyleBlock()? Done. https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:281: // Returns the descendant block containing the actual first line. On 2015/12/03 12:08:10, mstensho wrote: > Shouldn't this also include this block? I see that the implementation doesn't > include it, but shouldn't it? Yes, it should. Done. Also added a test to cover the case. https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:282: LayoutBlockFlow* firstLineContainer() const; On 2015/12/03 12:08:10, mstensho wrote: > Not sure if I'm happy with the block vs. container distinction. > nearestInnerBlockWithFirstLine()? Your suggestion sounds better. Done. https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right): https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:3115: static void invalidateDisplayItemClientsOfInlineBoxRecursively(InlineBox& box) On 2015/12/03 12:08:10, mstensho wrote: > All of this should go into LayoutBlockFlowLine.cpp, since we have that > implementation separation. Done. https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.h (right): https://codereview.chromium.org/1473363003/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.h:753: // TODO(layout-team): Remove when buttons are implemented with align-items instead On 2015/12/03 12:08:10, mstensho wrote: > A reference to crbug.com/226252 here? Not sure if "layout-team" is right here. > How about TODO(cbiesinger)? The FIXME was added by him in > b981273f245e746020a7afa4580e2a85580c4e32 Done.
Also, what do you think about the markdown file? Given the difficulty of this CL I don't want to necessary block things, but OTOH it's quite complicated. https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/button-first-line-change-color.html (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/button-first-line-change-color.html:2: <style> Can this invalidation change be unittested instead/also? https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1838: if (canHaveFirstLineOrFirstLetterStyle()) { It seems it would be cleaner to make firstLineStyleDidChange() virtual and implement this stuff in LayoutBlockFlow, LayoutButton and LayoutInline. https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/line/InlineBox.h (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/line/InlineBox.h:303: void invalidateDisplayItemClientsRecursively(); It's not super clear how far this recursion goes. For box subtree? A comment would also help.
On 2015/12/03 18:41:56, chrishtr wrote: > Also, what do you think about the markdown file? Given the difficulty of this CL > I don't want to necessary block things, but OTOH it's quite complicated. > I'm learning how to write a markdown :) https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/css/button-first-line-change-color.html (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/css/button-first-line-change-color.html:2: <style> On 2015/12/03 18:41:56, chrishtr wrote: > Can this invalidation change be unittested instead/also? Yes, but I feel a layout test provides better coverage from style invalidation, layout, paint invalidation and paint. We may have bugs about first-line at each stage. https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1838: if (canHaveFirstLineOrFirstLetterStyle()) { On 2015/12/03 18:41:56, chrishtr wrote: > It seems it would be cleaner to make firstLineStyleDidChange() virtual and > implement this stuff in LayoutBlockFlow, LayoutButton and LayoutInline. That would need to duplicate more than half of this method into three copies. How about making 'LayoutBlockFlow* firstLineContainerBlock()' virtual, so that line 1838-1846 can be one line: if (LayoutBlockFlow* firstLineContainerBlock = this->firstLineContainerBlock()) ? https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/line/InlineBox.h (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/line/InlineBox.h:303: void invalidateDisplayItemClientsRecursively(); On 2015/12/03 18:41:56, chrishtr wrote: > It's not super clear how far this recursion goes. For box subtree? A comment > would also help. Acknowledged.
On 2015/12/03 19:10:40, Xianzhu wrote: > On 2015/12/03 18:41:56, chrishtr wrote: > > Also, what do you think about the markdown file? Given the difficulty of this > CL > > I don't want to necessary block things, but OTOH it's quite complicated. > > > > I'm learning how to write a markdown :) I just read jchaffraix's "Documenting blink" slides on blinkon 5. For first-line paint invalidation an inline documentation (perhaps above LayoutObject::firstLineStyleDidChange()) seems more helpful. Wdyt?
https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1838: if (canHaveFirstLineOrFirstLetterStyle()) { On 2015/12/03 19:10:40, Xianzhu wrote: > On 2015/12/03 18:41:56, chrishtr wrote: > > It seems it would be cleaner to make firstLineStyleDidChange() virtual and > > implement this stuff in LayoutBlockFlow, LayoutButton and LayoutInline. > > That would need to duplicate more than half of this method into three copies. > How about making 'LayoutBlockFlow* firstLineContainerBlock()' virtual, so that > line 1838-1846 can be one line: > if (LayoutBlockFlow* firstLineContainerBlock = > this->firstLineContainerBlock()) > ? I think it might be be better to separate FIRST_LINE (for LayoutBlock) and FIRST_LINE_INHERITED (for LayoutInline) for firstLineStyleDidChange() from Element::pseudoStyleCacheIsValid(). Trying this.
https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1838: if (canHaveFirstLineOrFirstLetterStyle()) { On 2015/12/03 19:45:13, Xianzhu wrote: > On 2015/12/03 19:10:40, Xianzhu wrote: > > On 2015/12/03 18:41:56, chrishtr wrote: > > > It seems it would be cleaner to make firstLineStyleDidChange() virtual and > > > implement this stuff in LayoutBlockFlow, LayoutButton and LayoutInline. > > > > That would need to duplicate more than half of this method into three copies. > > How about making 'LayoutBlockFlow* firstLineContainerBlock()' virtual, so that > > line 1838-1846 can be one line: > > if (LayoutBlockFlow* firstLineContainerBlock = > > this->firstLineContainerBlock()) > > ? > > I think it might be be better to separate FIRST_LINE (for LayoutBlock) and > FIRST_LINE_INHERITED (for LayoutInline) for firstLineStyleDidChange() from > Element::pseudoStyleCacheIsValid(). Trying this. FWIW, I think what we already have here is good. https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/line/InlineBox.h (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/line/InlineBox.h:303: void invalidateDisplayItemClientsRecursively(); Why this change now? I don't recall having seen anyone request it in this review. I think it was better the way it was in the previous patch set. It carries out a very specific task, and doesn't deserve more than a static function (that's what it used to be). No need to make it look like an API that someone would want to call, IMHO. :)
Added some inline comments. https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/line/InlineBox.h (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/line/InlineBox.h:303: void invalidateDisplayItemClientsRecursively(); On 2015/12/03 19:58:02, mstensho wrote: > Why this change now? I don't recall having seen anyone request it in this > review. I think it was better the way it was in the previous patch set. It > carries out a very specific task, and doesn't deserve more than a static > function (that's what it used to be). No need to make it look like an API that > someone would want to call, IMHO. :) I'm also working on crbug.com/563943 and found this will be needed by it.
https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/line/InlineBox.h (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/line/InlineBox.h:303: void invalidateDisplayItemClientsRecursively(); On 2015/12/03 20:34:55, Xianzhu wrote: > On 2015/12/03 19:58:02, mstensho wrote: > > Why this change now? I don't recall having seen anyone request it in this > > review. I think it was better the way it was in the previous patch set. It > > carries out a very specific task, and doesn't deserve more than a static > > function (that's what it used to be). No need to make it look like an API that > > someone would want to call, IMHO. :) > > I'm also working on crbug.com/563943 and found this will be needed by it. OK, fair enough. Then ignore me, but chrishtr has a point about commenting this, though. :)
https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/line/InlineBox.h (right): https://codereview.chromium.org/1473363003/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/line/InlineBox.h:303: void invalidateDisplayItemClientsRecursively(); On 2015/12/03 18:41:56, chrishtr wrote: > It's not super clear how far this recursion goes. For box subtree? A comment > would also help. Done.
Re markdown: I'm suggesting we start a markdown file for paint invalidation in general, and begin with a brief skeleton and a note about first-line invalidation.
On 2015/12/03 22:52:57, chrishtr wrote: > Re markdown: I'm suggesting we start a markdown file for paint invalidation in > general, and begin with a brief skeleton and a note about first-line > invalidation. Is there any existing guideline (e.g. file naming convention, format style, etc.)?
On 2015/12/04 00:28:36, chrishtr wrote: > https://github.com/google/styleguide/tree/gh-pages/docguide Done.
lgtm. Not sure if it's optimal to add the markdown file (about some things that are not related to this commit) as part of this commit (in case it gets reverted), but I'll let it pass. :) https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutBlock.h (right): https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutBlock.h:279: // An example explaining layout tree structure about first-line style: Great!!
lgtm https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1846: // but we only need to invalidate only if it does. Nit: remove second "only" https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1854: if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) Add a comment explaining why this is conditional on v2. https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/README.md (right): https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/README.md:40: owing `LayoutInline`s and `LayoutText`s, respectively. Nit: "owning"
https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right): https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1846: // but we only need to invalidate only if it does. On 2015/12/07 18:06:03, chrishtr wrote: > Nit: remove second "only" Done. https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutObject.cpp:1854: if (!RuntimeEnabledFeatures::slimmingPaintV2Enabled()) On 2015/12/07 18:06:03, chrishtr wrote: > Add a comment explaining why this is conditional on v2. Done. https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/README.md (right): https://codereview.chromium.org/1473363003/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/README.md:40: owing `LayoutInline`s and `LayoutText`s, respectively. On 2015/12/07 18:06:04, chrishtr wrote: > Nit: "owning" Done.
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rune@opera.com, mstensho@opera.com, chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/1473363003/#ps360001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473363003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473363003/360001
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com, chrishtr@chromium.org, rune@opera.com Link to the patchset: https://codereview.chromium.org/1473363003/#ps380001 (title: "Some modifications were not saved to README.md in the previous patch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473363003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473363003/380001
Message was sent while issue was closed.
Description was changed from ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. BUG=548705 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-color-direct.html ========== to ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. BUG=548705 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-color-direct.html ==========
Message was sent while issue was closed.
Committed patchset #18 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. BUG=548705 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-color-direct.html ========== to ========== Invalidate first line display item clients when first line style changes It's not enough to invalidate the object having the first line style changed, but also the display item clients (e.g. line boxes) that will use the changed style to paint changed display items. BUG=548705 TEST=fast/css/first-line-hover-001.html TEST=fast/css/first-line-change-color.html TEST=fast/css/first-line-change-color-direct.html Committed: https://crrev.com/f7eea74c2c1e37c1af0e98637713283b6df9a167 Cr-Commit-Position: refs/heads/master@{#363663} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/f7eea74c2c1e37c1af0e98637713283b6df9a167 Cr-Commit-Position: refs/heads/master@{#363663} |
