Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680463003:
Remove branch on childNeedsDistributionRecalc (Closed)
DescriptionRemove branch on childNeedsDistributionRecalc
This patch moves a branch on childNeedsDistributionRecalc() in
Text::textLayoutObjectIsNeeded() to shouldUpdateLayoutByReattaching()
and adds comments so that it is clearer that the branch is used for the
optimization purpose.
BUG=673967
Review-Url: https://codereview.chromium.org/2680463003
Cr-Commit-Position: refs/heads/master@{#449266}
Committed: https://chromium.googlesource.com/chromium/src/+/884c1d0ef960ac2bfcc0839b9767fad82f6ef668
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add updateDistribution to CharacterData::setDataAndUpdate, and its test #
Total comments: 2
Patch Set 3 : hayato review: document().updateDistribution() to updateDistribution() #
Total comments: 1
Patch Set 4 : Restricted only when isTextNode and not from parser #Patch Set 5 : rune review, hayato/tkent offline disucssions #
Total comments: 1
Patch Set 6 : rune nit: Fix typo in the comment #
Messages
Total messages: 46 (28 generated)
The CQ bit was checked by kojii@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...
Description was changed from ========== WIP: distribution BUG=673967 ========== to ========== Remove branch on childNeedsDistributionRecalc This pathc removes a branch on childNeedsDistributionRecalc in Text::textLayoutObjectIsNeeded(). The branch was added in crrev.com/845453006 to suppress assertion failure in fast/images/element-gcd-while-generating-alt-content.html. The proper fix at that point was probably to update the distribution in the entry of surroundContents(). The fix wasn't proper and the test no longer fails without this branch. BUG=673967 ==========
Description was changed from ========== Remove branch on childNeedsDistributionRecalc This pathc removes a branch on childNeedsDistributionRecalc in Text::textLayoutObjectIsNeeded(). The branch was added in crrev.com/845453006 to suppress assertion failure in fast/images/element-gcd-while-generating-alt-content.html. The proper fix at that point was probably to update the distribution in the entry of surroundContents(). The fix wasn't proper and the test no longer fails without this branch. BUG=673967 ========== to ========== Remove branch on childNeedsDistributionRecalc This pathc removes a branch on childNeedsDistributionRecalc in Text::textLayoutObjectIsNeeded(). The branch was added in crrev.com/845453006 to suppress an assertion failure in fast/images/element-gcd-while-generating-alt-content.html. The proper fix at that point was probably to update the distribution in the entry of surroundContents(). The fix wasn't proper and the test no longer fails without this branch. BUG=673967 ==========
kojii@chromium.org changed reviewers: + hayato@chromium.org
PTAL.
rune@opera.com changed reviewers: + rune@opera.com
https://codereview.chromium.org/2680463003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/Text.cpp (left): https://codereview.chromium.org/2680463003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/Text.cpp:286: return true; It looks like this could still happen, looking at the code. updateTextLayoutObject() -> shouldUpdateLayoutByReattaching() -> textLayoutObjectIsNeeded() in Text.cpp looks dubious because it may be triggered by setting characterdata from a script at a point where distribution/style/layout is dirty, and textLayoutObjectIsNeeded() seems to be expected to be called during layout (re-)attachment. (disclaimer: haven't tested/debugged to confirm this)
On 2017/02/06 22:58:52, rune wrote:
> (disclaimer: haven't tested/debugged to confirm this)
Here's a repro:
<!DOCTYPE html>
<div id="host">XXX</div>
<script>
document.body.offsetTop;
var root = host.attachShadow({mode:"open"});
root.innerHTML = "<slot></slot>";
host.firstChild.data = " ";
</script>
Thank you Rune for your detailed analysis as always. I think the intention is not to suppress the lifecycle failure in deep stack but update at as high stack as possible. I'll check your test and try to find appropriate place to udpate style/distribution. So much thank you for catching this!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. The same test was failing in PS1...were I dreaming to think it turned to green??? Anyway, the same change fixed it too, and Rune's test looks perfect. Thank you so much.
https://codereview.chromium.org/2680463003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CharacterData.cpp (right): https://codereview.chromium.org/2680463003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CharacterData.cpp:179: document().updateDistribution(); This should be (this->)updateDistribution() (Node::updateDistribution()), rather than document().updteDistribution().
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2680463003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CharacterData.cpp (right): https://codereview.chromium.org/2680463003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CharacterData.cpp:179: document().updateDistribution(); On 2017/02/07 at 07:46:24, hayato wrote: > This should be (this->)updateDistribution() (Node::updateDistribution()), rather than document().updteDistribution(). Ah, thank you, done in PS3.
Description was changed from ========== Remove branch on childNeedsDistributionRecalc This pathc removes a branch on childNeedsDistributionRecalc in Text::textLayoutObjectIsNeeded(). The branch was added in crrev.com/845453006 to suppress an assertion failure in fast/images/element-gcd-while-generating-alt-content.html. The proper fix at that point was probably to update the distribution in the entry of surroundContents(). The fix wasn't proper and the test no longer fails without this branch. BUG=673967 ========== to ========== Remove branch on childNeedsDistributionRecalc This patch removes a branch on childNeedsDistributionRecalc in Text::textLayoutObjectIsNeeded(). The branch was added in crrev.com/845453006 to suppress an assertion failure in fast/images/element-gcd-while-generating-alt-content.html. The proper fix is to update the distribution when setting CharacterData.data. BUG=673967 ==========
https://codereview.chromium.org/2680463003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CharacterData.cpp (right): https://codereview.chromium.org/2680463003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CharacterData.cpp:179: updateDistribution(); I am not sure this place is a good place to insert updateDistribution(). Can we insert updateDistribution() in surroundContents() (?), if that is the only trigger to make it dirty, to avoid potential perf regression? Is there any other caller of setDataAndUpdate()?
On 2017/02/07 at 09:00:01, hayato wrote: > https://codereview.chromium.org/2680463003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/dom/CharacterData.cpp (right): > > https://codereview.chromium.org/2680463003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/dom/CharacterData.cpp:179: updateDistribution(); > I am not sure this place is a good place to insert updateDistribution(). > Can we insert updateDistribution() in surroundContents() (?), if that is the only trigger to make it dirty, to avoid potential perf regression? > > Is there any other caller of setDataAndUpdate()? Yeah, rune's test case doesn't involve surroundContents() but directly to CharacterData::setData() https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Chara... which calls setDataAndUpdate(). I can move this to CharacterData::setData(), which is the entry function from V8, but by looking at CharacterData.cpp, it's easy to create other cases such as insert/delete. Maybe I should add to each function, and avoid parserAppendData? Oh, I can probably use UpdateFromNonParser to avoid updating from the parser. I'll try that.
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/07 at 09:07:51, kojii wrote: > On 2017/02/07 at 09:00:01, hayato wrote: > > Is there any other caller of setDataAndUpdate()? Callers are: setData appendData insertData deleteData replaceData -- these use UpdateFromNonParser. They are bound to V8, though some our internal functions also call them. surroundContents() is one of such. parserAppendData -- this uses UpdateFromParser. This is a private function, so no other classes call this.
On 2017/02/07 09:07:51, kojii wrote: > On 2017/02/07 at 09:00:01, hayato wrote: > > > https://codereview.chromium.org/2680463003/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/dom/CharacterData.cpp (right): > > > > > https://codereview.chromium.org/2680463003/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/dom/CharacterData.cpp:179: > updateDistribution(); > > I am not sure this place is a good place to insert updateDistribution(). > > Can we insert updateDistribution() in surroundContents() (?), if that is the > only trigger to make it dirty, to avoid potential perf regression? > > > > Is there any other caller of setDataAndUpdate()? > > Yeah, rune's test case doesn't involve surroundContents() but directly to > CharacterData::setData() > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Chara... > which calls setDataAndUpdate(). > > I can move this to CharacterData::setData(), which is the entry function from > V8, but by looking at CharacterData.cpp, it's easy to create other cases such as > insert/delete. > > Maybe I should add to each function, and avoid parserAppendData? Oh, I can > probably use UpdateFromNonParser to avoid updating from the parser. I'll try > that. I don't think we should do synchronous distribution updates in this case at all. I think the existing code was actually correct, but the distribution check hard to understand. At the point where we change the characterdata, not just the distribution, but also the style and layout tree may be dirty. Even if the distribution is clean, the LayoutTreeTraversal in textLayoutObjectIsNeeded() may give you an incorrect result for what would be required for the next lifecycle update as the display type of the neighbor element may have changed. What if we use textLayoutObjectIsNeeded() solely for layout tree building and add DCHECK()s to ensure that. Then make shouldUpdateLayoutByReattaching() check for combinations of layoutObject() and text content instead (not checking style as it may be out-of-date). Or, take the possibly out-of-date style into consideration and just split out the part before the if-test as a separate method which could then be called from both shouldUpdateLayoutByReattaching() and textLayoutObjectIsNeeded().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, talked with hayato and tkent, it looks like all in consensus that we should not do synchronous distribution updates here. I'll try updating comments to make the intention of using childNeedsDistributionRecalc() and see what Elliot thinks.
The CQ bit was checked by kojii@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...
Description was changed from ========== Remove branch on childNeedsDistributionRecalc This patch removes a branch on childNeedsDistributionRecalc in Text::textLayoutObjectIsNeeded(). The branch was added in crrev.com/845453006 to suppress an assertion failure in fast/images/element-gcd-while-generating-alt-content.html. The proper fix is to update the distribution when setting CharacterData.data. BUG=673967 ========== to ========== Remove branch on childNeedsDistributionRecalc This patch moves a branch on childNeedsDistributionRecalc() in Text::textLayoutObjectIsNeeded() to shouldUpdateLayoutByReattaching() and adds comments so that it is clearer that the branch is used for the optimization purpose. BUG=673967 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL. After a bit more reading to understand the code around here, PS5 is half of rune's and hayato/tkent suggestions, IIUC what rune suggested -- sorry if I didn't.
LGTM. That looks better. I think you would need Rune's review.
Wasn't exactly what I proposed, but lgtm with nit. https://codereview.chromium.org/2680463003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Text.cpp (right): https://codereview.chromium.org/2680463003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Text.cpp:428: *textLayoutObject->parent())) { genenral -> general
Thank you. Yeah, so "half" of the two suggestions. and thank you for catching the typo.
The CQ bit was checked by kojii@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org, rune@opera.com Link to the patchset: https://codereview.chromium.org/2680463003/#ps100001 (title: "rune nit: Fix typo in the comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486637807436180,
"parent_rev": "a9b551dfda8c2231f7ba4733cdee33bc9747979b", "commit_rev":
"884c1d0ef960ac2bfcc0839b9767fad82f6ef668"}
Message was sent while issue was closed.
Description was changed from ========== Remove branch on childNeedsDistributionRecalc This patch moves a branch on childNeedsDistributionRecalc() in Text::textLayoutObjectIsNeeded() to shouldUpdateLayoutByReattaching() and adds comments so that it is clearer that the branch is used for the optimization purpose. BUG=673967 ========== to ========== Remove branch on childNeedsDistributionRecalc This patch moves a branch on childNeedsDistributionRecalc() in Text::textLayoutObjectIsNeeded() to shouldUpdateLayoutByReattaching() and adds comments so that it is clearer that the branch is used for the optimization purpose. BUG=673967 Review-Url: https://codereview.chromium.org/2680463003 Cr-Commit-Position: refs/heads/master@{#449266} Committed: https://chromium.googlesource.com/chromium/src/+/884c1d0ef960ac2bfcc0839b9767... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/884c1d0ef960ac2bfcc0839b9767... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
