|
|
Created:
5 years, 6 months ago by changseok Modified:
5 years, 4 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, yoichio Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDo not traverse nodes not having a layout object while searching editable visible position.
This is a merge of http://trac.webkit.org/changeset/185287
Typing is slow in Gmail on iPads by Ryosuke Niwa <rniwa@webkit.org>
Performance drop in GMail was caused by nextCandidate and nextVisuallyDistinctCandidate
traversing through each character in a text node without a layout object. Skip any node
that doesn't have a layout object in both of those functions and corresponding previous* functions.
It's fine to skip unrendered nodes in PositionIterator because only other clients of PositionIterator
are Position::upstream and Position::downstream and they don't care about un-rendered nodes either.
BUG=497525
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200236
Patch Set 1 #Patch Set 2 : Added a perf test #Patch Set 3 : Split out the test #
Total comments: 2
Patch Set 4 : Replace FIXMEs with TODOs #Patch Set 5 : Rebased #Patch Set 6 : Fixed a build failure #Patch Set 7 : Fix a loop issue caused by document-fragment #
Total comments: 3
Patch Set 8 : Removed conditions, !node->isDocumentFragment() #Patch Set 9 : Resolve conflicts on PositionIterator.cpp #Patch Set 10 : Resolved conflicts. #
Total comments: 4
Patch Set 11 : Rebased #Patch Set 12 : Fix infinite loop cuased by accessing shadow dom. #Patch Set 13 : Udated cl #Patch Set 14 : Fix a crash caused by m_anchorNode being null #
Total comments: 2
Patch Set 15 : Updated patch #
Messages
Total messages: 72 (19 generated)
shivamidow@gmail.com changed reviewers: + eae@chromium.org, leviw@chromium.org, mstensho@opera.com, pdr@chromium.org
Is there a test for this?
This is a reasonable change. Can you update the description? We don't run Blink on iPads.
On 2015/06/15 15:26:04, eae wrote: > Is there a test for this? No. There is no test attached in the original patch. I think it's enough that this patch doesn't make a regression for existing tests.
On 2015/06/15 19:00:46, leviw wrote: > This is a reasonable change. Can you update the description? We don't run Blink > on iPads. Don't we need to keep the same name with the original patch? ok.. let me do that.
On 2015/06/16 at 03:21:38, shivamidow wrote: > On 2015/06/15 19:00:46, leviw wrote: > > This is a reasonable change. Can you update the description? We don't run Blink > > on iPads. > > Don't we need to keep the same name with the original patch? > ok.. let me do that. You can keep the old subject line in the description and still give it a better one here. You've also got a broken unicode marker in your description "<<U+200B>rniwa@webkit.org>". "Do not traverse any node that doesn't have a layout object." This still doesn't give someone scanning the subjects an idea of what part of the code you're touching. It's probable that we don't have good perf coverage for this code. It'd be nice if we could get a perf test in before this to prove its usefulness, and to prevent ourselves from regressing it later.
On 2015/06/16 17:09:51, leviw wrote: > You can keep the old subject line in the description and still give it a better > one here. You've also got a broken unicode marker in your description > mailto:"<<U+200B>rniwa@webkit.org>". The description is updated. > "Do not traverse any node that doesn't have a layout object." > This still doesn't give someone scanning the subjects an idea of what part of > the code you're touching. Ditto. > It's probable that we don't have good perf coverage for this code. It'd be nice > if we could get a perf test in before this to prove its usefulness, and to > prevent ourselves from regressing it later. Ah.. performance test. I didn't think that. At a glance, there is ./PerformanceTests/DOM/textarea-edit.html Isn't that relevant with this cl?
On 2015/06/17 06:52:03, changseok wrote: > On 2015/06/16 17:09:51, leviw wrote: > > You can keep the old subject line in the description and still give it a > better > > one here. You've also got a broken unicode marker in your description > > mailto:"<<U+200B>rniwa@webkit.org>". > The description is updated. > > > "Do not traverse any node that doesn't have a layout object." > > This still doesn't give someone scanning the subjects an idea of what part of > > the code you're touching. > Ditto. > > > It's probable that we don't have good perf coverage for this code. It'd be > nice > > if we could get a perf test in before this to prove its usefulness, and to > > prevent ourselves from regressing it later. > Ah.. performance test. I didn't think that. At a glance, there is > ./PerformanceTests/DOM/textarea-edit.html > Isn't that relevant with this cl? O.K I just added a perf test for this cl. I could see 8-10% performance gain from the test.
Would you comment on the updated cl?
Let's pull the perf test out and land it separately and in advance so we can see the benefit of this patch as compared against the previous baseline :)
On 2015/06/22 20:14:46, leviw wrote: > Let's pull the perf test out and land it separately and in advance so we can see > the benefit of this patch as compared against the previous baseline :) O.K. I split out the cl into two. You can find the perf test at http://crrev.com/1200183002.
LGTM with nit addressed now that the perf test has been landed and had time to get baselines. https://codereview.chromium.org/1189543002/diff/40001/Source/core/editing/htm... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/1189543002/diff/40001/Source/core/editing/htm... Source/core/editing/htmlediting.cpp:233: // FIXME: Use PositionIterator instead. We use TODO(name) syntax in Blink now.
Thanks! https://codereview.chromium.org/1189543002/diff/40001/Source/core/editing/htm... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/1189543002/diff/40001/Source/core/editing/htm... Source/core/editing/htmlediting.cpp:233: // FIXME: Use PositionIterator instead. On 2015/06/25 00:15:20, leviw wrote: > We use TODO(name) syntax in Blink now. Done.
The CQ bit was checked by shivamidow@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1189543002/#ps60001 (title: "Replace FIXMEs with TODOs")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by shivamidow@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1189543002/#ps80001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by shivamidow@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1189543002/#ps100001 (title: "Fixed a build failure")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
On 2015/06/25 09:02:46, commit-bot: I haz the power wrote: > Exceeded global retry quota There was a loop issue caused by DocumentFragment node in nextVisuallyDistinctCandidate(). This didn't happen before few days ago. Maybe something has been changed in the meantime.. Would you see the updated cl if it makes sense?
https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... Source/core/editing/htmlediting.cpp:261: if (!node->isDocumentFragment() && !node->layoutObject()) Can you help me understand why this is needed?
https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... Source/core/editing/htmlediting.cpp:261: if (!node->isDocumentFragment() && !node->layoutObject()) On 2015/06/29 20:17:56, leviw wrote: > Can you help me understand why this is needed? I'm not sure if I correctly understand the root cause on this though, let me try my best to elaborate the situation here. After recent commits on htmlediting.cpp (since https://codereview.chromium.org/1174183004 maybe?) document-fragment nodes have come in this function. As my understanding, the document-fragment is an invisible document because it's a kind of offscreen document tree and is not appended to DOM tree yet. (https://developer.mozilla.org/en-US/docs/Web/API/Document/createDocumentFragment) According to a normal flow before my cl, the p.containerNode() for the last iteration in this while loop is a documentFragment node and its p.atEndOfTree() at the moment is true. So we don't go over the next iteration. After my cl, however node->layoutObject() is false for the documentFragment node, so we find a new p through lastPositionInOrAfterNode(node) at the following line and p.atEndOfTree() of the new p is false now. Thus we go into an infinite loop. To avoid this loop, I added a condition, !node->isDocumentFragment() so that we don't find a new position for documentFragment even though it has no layoutObject.
leviw@chromium.org changed reviewers: + tkent@chromium.org, yosin@chromium.org
+yosin@ and tkent@ The CL you mention shouldn't have had any behavior changes.
I suggest to this CL to split into - htmlediting.cpp - PositionIterator.cpp Checking layout object for position in |nextVisuallyDistinctCandidate()| and |prviousVisuallyDistinctCandidate()| isn't enough. We should consider distribution in shadow DOM. We don't want to handle shadow DOM details in editing functions, rather we make editing functions to work on composed tree. https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... File Source/core/editing/htmlediting.cpp (right): https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... Source/core/editing/htmlediting.cpp:261: if (!node->isDocumentFragment() && !node->layoutObject()) On 2015/06/30 05:50:58, changseok wrote: > On 2015/06/29 20:17:56, leviw wrote: > > Can you help me understand why this is needed? > > I'm not sure if I correctly understand the root cause on this though, let me try > my best to elaborate the situation here. After recent commits on htmlediting.cpp > (since https://codereview.chromium.org/1174183004 maybe?) document-fragment > nodes have come in this function. As my understanding, the document-fragment is > an invisible document because it's a kind of offscreen document tree and is not > appended to DOM tree yet. > (https://developer.mozilla.org/en-US/docs/Web/API/Document/createDocumentFragment) > According to a normal flow before my cl, the p.containerNode() for the last > iteration in this while loop is a documentFragment node and its p.atEndOfTree() > at the moment is true. So we don't go over the next iteration. After my cl, > however node->layoutObject() is false for the documentFragment node, so we find > a new p through lastPositionInOrAfterNode(node) at the following line and > p.atEndOfTree() of the new p is false now. Thus we go into an infinite loop. > To avoid this loop, I added a condition, !node->isDocumentFragment() so that we > don't find a new position for documentFragment even though it has no > layoutObject. Bottom line: You don't need to check |!node->isDocumentFragment()| here, since it is shadow root and we don't define behavior on this function on shadow tree. Background: I'm working on make this function to work on composed tree for selection for composed tree, aka selection on shadow DOM tree, by templatizing this function as |previousVisuallyDistinctCandidate()|. Once it is done, we don't see shadow root here. In other words, functions for |Position| should not handle shadow tree related things, e.g. shadow root, active insertion point (CONTENT/SHADOW elements). These function will be worked on composed tree.
On 2015/07/01 01:50:12, Yosi_UTC9 wrote: > I suggest to this CL to split into > - htmlediting.cpp > - PositionIterator.cpp > > Checking layout object for position in |nextVisuallyDistinctCandidate()| and > |prviousVisuallyDistinctCandidate()| isn't enough. We should consider > distribution in shadow DOM. We don't want to handle shadow DOM details in > editing functions, rather we make editing functions to work on composed tree. > > https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... > File Source/core/editing/htmlediting.cpp (right): > > https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... > Source/core/editing/htmlediting.cpp:261: if (!node->isDocumentFragment() && > !node->layoutObject()) > On 2015/06/30 05:50:58, changseok wrote: > > On 2015/06/29 20:17:56, leviw wrote: > > > Can you help me understand why this is needed? > > > > I'm not sure if I correctly understand the root cause on this though, let me > try > > my best to elaborate the situation here. After recent commits on > htmlediting.cpp > > (since https://codereview.chromium.org/1174183004 maybe?) document-fragment > > nodes have come in this function. As my understanding, the document-fragment > is > > an invisible document because it's a kind of offscreen document tree and is > not > > appended to DOM tree yet. > > > (https://developer.mozilla.org/en-US/docs/Web/API/Document/createDocumentFragment) > > According to a normal flow before my cl, the p.containerNode() for the last > > iteration in this while loop is a documentFragment node and its > p.atEndOfTree() > > at the moment is true. So we don't go over the next iteration. After my cl, > > however node->layoutObject() is false for the documentFragment node, so we > find > > a new p through lastPositionInOrAfterNode(node) at the following line and > > p.atEndOfTree() of the new p is false now. Thus we go into an infinite loop. > > To avoid this loop, I added a condition, !node->isDocumentFragment() so that > we > > don't find a new position for documentFragment even though it has no > > layoutObject. > > Bottom line: > You don't need to check |!node->isDocumentFragment()| here, since it is shadow > root and we don't define behavior on this function on shadow tree. > > > Background: > I'm working on make this function to work on composed tree for selection for > composed tree, aka selection on shadow DOM tree, by templatizing this function > as |previousVisuallyDistinctCandidate()|. Once it is done, we don't see shadow > root here. > > In other words, functions for |Position| should not handle shadow tree related > things, e.g. shadow root, active insertion point (CONTENT/SHADOW elements). > These function will be worked on composed tree. It seems that it is better to use |PositionIterator| in |nextVisuallyDistinctCandidate()| and |previousVisuallyDistinctCandidate()| as TODO comment.
On 2015/07/01 02:12:00, Yosi_UTC9 wrote: > On 2015/07/01 01:50:12, Yosi_UTC9 wrote: > > I suggest to this CL to split into > > - htmlediting.cpp > > - PositionIterator.cpp > > > > Checking layout object for position in |nextVisuallyDistinctCandidate()| and > > |prviousVisuallyDistinctCandidate()| isn't enough. We should consider > > distribution in shadow DOM. We don't want to handle shadow DOM details in > > editing functions, rather we make editing functions to work on composed tree. > > > > > https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... > > File Source/core/editing/htmlediting.cpp (right): > > > > > https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... > > Source/core/editing/htmlediting.cpp:261: if (!node->isDocumentFragment() && > > !node->layoutObject()) > > On 2015/06/30 05:50:58, changseok wrote: > > > On 2015/06/29 20:17:56, leviw wrote: > > > > Can you help me understand why this is needed? > > > > > > I'm not sure if I correctly understand the root cause on this though, let me > > try > > > my best to elaborate the situation here. After recent commits on > > htmlediting.cpp > > > (since https://codereview.chromium.org/1174183004 maybe?) document-fragment > > > nodes have come in this function. As my understanding, the document-fragment > > is > > > an invisible document because it's a kind of offscreen document tree and is > > not > > > appended to DOM tree yet. > > > > > > (https://developer.mozilla.org/en-US/docs/Web/API/Document/createDocumentFragment) > > > According to a normal flow before my cl, the p.containerNode() for the last > > > iteration in this while loop is a documentFragment node and its > > p.atEndOfTree() > > > at the moment is true. So we don't go over the next iteration. After my cl, > > > however node->layoutObject() is false for the documentFragment node, so we > > find > > > a new p through lastPositionInOrAfterNode(node) at the following line and > > > p.atEndOfTree() of the new p is false now. Thus we go into an infinite loop. > > > To avoid this loop, I added a condition, !node->isDocumentFragment() so that > > we > > > don't find a new position for documentFragment even though it has no > > > layoutObject. > > > > Bottom line: > > You don't need to check |!node->isDocumentFragment()| here, since it is shadow > > root and we don't define behavior on this function on shadow tree. > > > > > > Background: > > I'm working on make this function to work on composed tree for selection for > > composed tree, aka selection on shadow DOM tree, by templatizing this function > > as |previousVisuallyDistinctCandidate()|. Once it is done, we don't see shadow > > root here. > > > > In other words, functions for |Position| should not handle shadow tree related > > things, e.g. shadow root, active insertion point (CONTENT/SHADOW elements). > > These function will be worked on composed tree. Do you mean removing !node->isDocumentFragment() will just be fine here and we need to wait until your work, selection on shadow dom, will be done? > > It seems that it is better to use |PositionIterator| in > |nextVisuallyDistinctCandidate()| and |previousVisuallyDistinctCandidate()| as > TODO comment. This cl has been a little lagging. My first intention was simply merging the webkit patch into blink. How about dealing with the TODO comment in a different bug? I can follow it up of course. =)
On 2015/07/01 07:55:28, changseok wrote: > On 2015/07/01 02:12:00, Yosi_UTC9 wrote: > > On 2015/07/01 01:50:12, Yosi_UTC9 wrote: > > > I suggest to this CL to split into > > > - htmlediting.cpp > > > - PositionIterator.cpp > > > > > > Checking layout object for position in |nextVisuallyDistinctCandidate()| and > > > |prviousVisuallyDistinctCandidate()| isn't enough. We should consider > > > distribution in shadow DOM. We don't want to handle shadow DOM details in > > > editing functions, rather we make editing functions to work on composed > tree. > > > > > > > > > https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... > > > File Source/core/editing/htmlediting.cpp (right): > > > > > > > > > https://codereview.chromium.org/1189543002/diff/120001/Source/core/editing/ht... > > > Source/core/editing/htmlediting.cpp:261: if (!node->isDocumentFragment() && > > > !node->layoutObject()) > > > On 2015/06/30 05:50:58, changseok wrote: > > > > On 2015/06/29 20:17:56, leviw wrote: > > > > > Can you help me understand why this is needed? > > > > > > > > I'm not sure if I correctly understand the root cause on this though, let > me > > > try > > > > my best to elaborate the situation here. After recent commits on > > > htmlediting.cpp > > > > (since https://codereview.chromium.org/1174183004 maybe?) > document-fragment > > > > nodes have come in this function. As my understanding, the > document-fragment > > > is > > > > an invisible document because it's a kind of offscreen document tree and > is > > > not > > > > appended to DOM tree yet. > > > > > > > > > > (https://developer.mozilla.org/en-US/docs/Web/API/Document/createDocumentFragment) > > > > According to a normal flow before my cl, the p.containerNode() for the > last > > > > iteration in this while loop is a documentFragment node and its > > > p.atEndOfTree() > > > > at the moment is true. So we don't go over the next iteration. After my > cl, > > > > however node->layoutObject() is false for the documentFragment node, so we > > > find > > > > a new p through lastPositionInOrAfterNode(node) at the following line and > > > > p.atEndOfTree() of the new p is false now. Thus we go into an infinite > loop. > > > > To avoid this loop, I added a condition, !node->isDocumentFragment() so > that > > > we > > > > don't find a new position for documentFragment even though it has no > > > > layoutObject. > > > > > > Bottom line: > > > You don't need to check |!node->isDocumentFragment()| here, since it is > shadow > > > root and we don't define behavior on this function on shadow tree. > > > > > > > > > Background: > > > I'm working on make this function to work on composed tree for selection for > > > composed tree, aka selection on shadow DOM tree, by templatizing this > function > > > as |previousVisuallyDistinctCandidate()|. Once it is done, we don't see > shadow > > > root here. > > > > > > In other words, functions for |Position| should not handle shadow tree > related > > > things, e.g. shadow root, active insertion point (CONTENT/SHADOW elements). > > > These function will be worked on composed tree. > > Do you mean removing !node->isDocumentFragment() will just be fine here and we > need to wait until your work, selection on shadow dom, will be done? > Yes. Please leave |{next,previous}VisuallyDistinctCandidate()| as is. I'm in infinite loop as you meet in different reason. So, I should fix it. > > > > It seems that it is better to use |PositionIterator| in > > |nextVisuallyDistinctCandidate()| and |previousVisuallyDistinctCandidate()| as > > TODO comment. > > This cl has been a little lagging. My first intention was simply merging the > webkit patch into blink. > How about dealing with the TODO comment in a different bug? I can follow it up > of course. =) Please do so. It makes my life easier.
On 2015/07/01 08:53:52, Yosi_UTC9 wrote: > > Do you mean removing !node->isDocumentFragment() will just be fine here and we > > need to wait until your work, selection on shadow dom, will be done? > > > Yes. Please leave |{next,previous}VisuallyDistinctCandidate()| as is. > I'm in infinite loop as you meet in different reason. So, I should fix it. I just updated this cl again. But it should be put back until Yosi will finish his work on selection on shadow dom. @Yosi please let me know if once you're done. Then I'll try to land this. =)
On 2015/07/03 09:08:01, changseok wrote: > On 2015/07/01 08:53:52, Yosi_UTC9 wrote: > > > Do you mean removing !node->isDocumentFragment() will just be fine here and > we > > > need to wait until your work, selection on shadow dom, will be done? > > > > > Yes. Please leave |{next,previous}VisuallyDistinctCandidate()| as is. > > I'm in infinite loop as you meet in different reason. So, I should fix it. > > I just updated this cl again. But it should be put back until Yosi will finish > his work on selection on shadow dom. > @Yosi please let me know if once you're done. Then I'll try to land this. =) The cl is rebased. Would you trigger the bots again?
The CQ bit was checked by leviw@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1189543002/#ps180001 (title: "Resolved conflicts.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/69592) (exceeded global retry quota)
> Still waiting for the following processes to finish: > /b/build/slave/linux_layout/build/src/out/Release/webkit_unit_tests --gtest_filter=StyledMarkupSerializerTest.AcrossShadow --single-process-tests --test-launcher-output=/tmp/.org.chromium.Chromium.LuPXcC/test_results.xml > [ RUN ] StyledMarkupSerializerTest.AcrossShadow > [2319/2319] StyledMarkupSerializerTest.AcrossShadow (TIMED OUT) > 1 test timed out: > StyledMarkupSerializerTest.AcrossShadow I looked into this and found the infinite loop is still there. Following lines in StyledMarkupSerializerTest.AcrossShadow cause the loop. > PositionInComposedTree startICT(toText(one->firstChild()), 0); > PositionInComposedTree endICT(toText(two->firstChild()), 2); > const std::string& serializedICT = serializePart<EditingInComposedTreeStrategy>(startICT, endICT, AnnotateForInterchange); When I re-added the removed like, if (!node->isDocumentFragment() && !node->layoutObject())' to nextVisuallyDistinctCandidate(), StyledMarkupSerializerTest.AcrossShadow was finally happy. Do we need those lines? or are those correct? If so, would you guide me for the loop?
On 2015/07/17 09:09:35, changseok wrote: > > Still waiting for the following processes to finish: > > /b/build/slave/linux_layout/build/src/out/Release/webkit_unit_tests > --gtest_filter=StyledMarkupSerializerTest.AcrossShadow --single-process-tests > --test-launcher-output=/tmp/.org.chromium.Chromium.LuPXcC/test_results.xml > > [ RUN ] StyledMarkupSerializerTest.AcrossShadow > > [2319/2319] StyledMarkupSerializerTest.AcrossShadow (TIMED OUT) > > 1 test timed out: > > StyledMarkupSerializerTest.AcrossShadow > > I looked into this and found the infinite loop is still there. > Following lines in StyledMarkupSerializerTest.AcrossShadow cause the loop. > > > PositionInComposedTree startICT(toText(one->firstChild()), 0); > > > > PositionInComposedTree endICT(toText(two->firstChild()), 2); > > const std::string& serializedICT = > serializePart<EditingInComposedTreeStrategy>(startICT, endICT, > AnnotateForInterchange); > > When I re-added the removed like, if (!node->isDocumentFragment() && > !node->layoutObject())' to nextVisuallyDistinctCandidate(), > StyledMarkupSerializerTest.AcrossShadow was finally happy. > Do we need those lines? or are those correct? > If so, would you guide me for the loop? Since, purpose of this optimization is skipping unrendered text node, it should be no infinite loop when we skip node which is offsetInCharacters(), e.g. if (auto* node = p.containerNode()) { if (node->offsetIncharters() && node->offsetInCharacters()) p = lastPositionInOrAfterNode(node); // firstPositionInOrBefore(node) } For non-text node, lastPositionInOrAfterNode() is as same as Position.next(). So, we aren't be involved shadow DOM tree boundary issue.
I'm thinking to get rid of DOM tree version of PositionIterator and next/previousXXX and use composed tree version only. When they work on composed tree, we don't need to handle shadow root specially. https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... File Source/core/dom/PositionIterator.cpp (right): https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... Source/core/dom/PositionIterator.cpp:80: if (m_anchorNode->layoutObject() && !Strategy::hasChildren(*m_anchorNode) && m_offsetInAnchor < Strategy::lastOffsetForEditing(m_anchorNode)) { Please check layoutObject() only for nodes which return true for offsetInCharacter(). https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... Source/core/dom/PositionIterator.cpp:113: if (m_offsetInAnchor && m_anchorNode->layoutObject()) { Same as L80
On 2015/07/17 11:48:52, Yosi_UTC9 wrote: > On 2015/07/17 09:09:35, changseok wrote: > > > Still waiting for the following processes to finish: > > > /b/build/slave/linux_layout/build/src/out/Release/webkit_unit_tests > > --gtest_filter=StyledMarkupSerializerTest.AcrossShadow --single-process-tests > > --test-launcher-output=/tmp/.org.chromium.Chromium.LuPXcC/test_results.xml > > > [ RUN ] StyledMarkupSerializerTest.AcrossShadow > > > [2319/2319] StyledMarkupSerializerTest.AcrossShadow (TIMED OUT) > > > 1 test timed out: > > > StyledMarkupSerializerTest.AcrossShadow > > > > I looked into this and found the infinite loop is still there. > > Following lines in StyledMarkupSerializerTest.AcrossShadow cause the loop. > > > > > PositionInComposedTree startICT(toText(one->firstChild()), 0); > > > > > > > > > PositionInComposedTree endICT(toText(two->firstChild()), 2); > > > const std::string& serializedICT = > > serializePart<EditingInComposedTreeStrategy>(startICT, endICT, > > AnnotateForInterchange); > > > > When I re-added the removed like, if (!node->isDocumentFragment() && > > !node->layoutObject())' to nextVisuallyDistinctCandidate(), > > StyledMarkupSerializerTest.AcrossShadow was finally happy. > > Do we need those lines? or are those correct? > > If so, would you guide me for the loop? > > Since, purpose of this optimization is skipping unrendered text node, > it should be no infinite loop when we skip node which is offsetInCharacters(), > e.g. > if (auto* node = p.containerNode()) { > if (node->offsetIncharters() && node->offsetInCharacters()) > p = lastPositionInOrAfterNode(node); // firstPositionInOrBefore(node) > } > > For non-text node, lastPositionInOrAfterNode() is as same as Position.next(). > So, we aren't be involved shadow DOM tree boundary issue. Sorry for late response. I was busy with my daily job for a while. You're right I saw the loop has gone with the changes you guided me. Thank you.
> I'm thinking to get rid of DOM tree version of PositionIterator and > next/previousXXX and use composed tree version only. When they work on composed > tree, we don't need to handle shadow root specially. I currently don't know much details on this, but it sounds interesting. Thanks for sharing your insight. :) https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... File Source/core/dom/PositionIterator.cpp (right): https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... Source/core/dom/PositionIterator.cpp:80: if (m_anchorNode->layoutObject() && !Strategy::hasChildren(*m_anchorNode) && m_offsetInAnchor < Strategy::lastOffsetForEditing(m_anchorNode)) { On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > Please check layoutObject() only for nodes which return true for > offsetInCharacter(). Done. https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... Source/core/dom/PositionIterator.cpp:113: if (m_offsetInAnchor && m_anchorNode->layoutObject()) { On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > Same as L80 Done.
On 2015/07/28 05:05:38, changseok wrote: > > I'm thinking to get rid of DOM tree version of PositionIterator and > > next/previousXXX and use composed tree version only. When they work on > composed > > tree, we don't need to handle shadow root specially. > > I currently don't know much details on this, but it sounds interesting. > Thanks for sharing your insight. :) > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > File Source/core/dom/PositionIterator.cpp (right): > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > Source/core/dom/PositionIterator.cpp:80: if (m_anchorNode->layoutObject() && > !Strategy::hasChildren(*m_anchorNode) && m_offsetInAnchor < > Strategy::lastOffsetForEditing(m_anchorNode)) { > On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > > Please check layoutObject() only for nodes which return true for > > offsetInCharacter(). > > Done. > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > Source/core/dom/PositionIterator.cpp:113: if (m_offsetInAnchor && > m_anchorNode->layoutObject()) { > On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > > Same as L80 > > Done. Can I land this?
On 2015/07/29 04:24:27, changseok wrote: > On 2015/07/28 05:05:38, changseok wrote: > > > I'm thinking to get rid of DOM tree version of PositionIterator and > > > next/previousXXX and use composed tree version only. When they work on > > composed > > > tree, we don't need to handle shadow root specially. > > > > I currently don't know much details on this, but it sounds interesting. > > Thanks for sharing your insight. :) > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > File Source/core/dom/PositionIterator.cpp (right): > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > Source/core/dom/PositionIterator.cpp:80: if (m_anchorNode->layoutObject() && > > !Strategy::hasChildren(*m_anchorNode) && m_offsetInAnchor < > > Strategy::lastOffsetForEditing(m_anchorNode)) { > > On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > > > Please check layoutObject() only for nodes which return true for > > > offsetInCharacter(). > > > > Done. > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > Source/core/dom/PositionIterator.cpp:113: if (m_offsetInAnchor && > > m_anchorNode->layoutObject()) { > > On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > > > Same as L80 > > > > Done. > > Can I land this? Could you rebase your patch and uplodad? I think we don't need to change htmlediting.cpp anymore since {next,previous}VisuallyDistinctCandidate() use PositionIterator now.
On 2015/07/29 05:55:31, Yosi_UTC9 wrote: > On 2015/07/29 04:24:27, changseok wrote: > > On 2015/07/28 05:05:38, changseok wrote: > > > > I'm thinking to get rid of DOM tree version of PositionIterator and > > > > next/previousXXX and use composed tree version only. When they work on > > > composed > > > > tree, we don't need to handle shadow root specially. > > > > > > I currently don't know much details on this, but it sounds interesting. > > > Thanks for sharing your insight. :) > > > > > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > > File Source/core/dom/PositionIterator.cpp (right): > > > > > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > > Source/core/dom/PositionIterator.cpp:80: if (m_anchorNode->layoutObject() && > > > !Strategy::hasChildren(*m_anchorNode) && m_offsetInAnchor < > > > Strategy::lastOffsetForEditing(m_anchorNode)) { > > > On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > > > > Please check layoutObject() only for nodes which return true for > > > > offsetInCharacter(). > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > > Source/core/dom/PositionIterator.cpp:113: if (m_offsetInAnchor && > > > m_anchorNode->layoutObject()) { > > > On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > > > > Same as L80 > > > > > > Done. > > > > Can I land this? > > Could you rebase your patch and uplodad? > I think we don't need to change htmlediting.cpp anymore since > {next,previous}VisuallyDistinctCandidate() use PositionIterator now. Interestingly, this cl is no longer useful. I saw applying this just resulted in about 17% performance regression. It seems that using PositionIterator covers the improvement this cl intended. Great work. =) If there is no other opinion, we can discard this ticket.
On 2015/07/30 07:36:05, changseok wrote: > On 2015/07/29 05:55:31, Yosi_UTC9 wrote: > > On 2015/07/29 04:24:27, changseok wrote: > > > On 2015/07/28 05:05:38, changseok wrote: > > > > > I'm thinking to get rid of DOM tree version of PositionIterator and > > > > > next/previousXXX and use composed tree version only. When they work on > > > > composed > > > > > tree, we don't need to handle shadow root specially. > > > > > > > > I currently don't know much details on this, but it sounds interesting. > > > > Thanks for sharing your insight. :) > > > > > > > > > > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > > > File Source/core/dom/PositionIterator.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > > > Source/core/dom/PositionIterator.cpp:80: if (m_anchorNode->layoutObject() > && > > > > !Strategy::hasChildren(*m_anchorNode) && m_offsetInAnchor < > > > > Strategy::lastOffsetForEditing(m_anchorNode)) { > > > > On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > > > > > Please check layoutObject() only for nodes which return true for > > > > > offsetInCharacter(). > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1189543002/diff/180001/Source/core/dom/Positi... > > > > Source/core/dom/PositionIterator.cpp:113: if (m_offsetInAnchor && > > > > m_anchorNode->layoutObject()) { > > > > On 2015/07/17 12:00:21, Yosi_UTC9 wrote: > > > > > Same as L80 > > > > > > > > Done. > > > > > > Can I land this? > > > > Could you rebase your patch and uplodad? > > I think we don't need to change htmlediting.cpp anymore since > > {next,previous}VisuallyDistinctCandidate() use PositionIterator now. > > Interestingly, this cl is no longer useful. I saw applying this just resulted in > about 17% performance regression. > It seems that using PositionIterator covers the improvement this cl intended. > Great work. =) > If there is no other opinion, we can discard this ticket. No.. my bad. This cl is still meaningful. After removing offsetInCharacters() from PositionIterator.cpp, I could see obvious 3% around performance gain. Please have a look again.
LGTM Thanks so much!
The CQ bit was checked by yosin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1189543002/#ps240001 (title: "Udated cl")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1189543002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/72059)
> editing/selection/DOMSelection-DocumentType.html > editing/undo/orphaned-selection-crash-bug32823-3.html > fast/dom/shadow/mouse-click-mouseup-listener-update-distribution-crash.html > fast/dom/shadow/gesture-tap-mouseup-listener-update-distribution-crash.html > editing/pasteboard/smart-paste-003-trailing-whitespace.html There was a crash caused by m_anchorNode being null for above tests. The backtrace is like following. * thread #26: tid = 0x229804, 0x0000000105e0a621 Content Shell Framework`blink::Node::getFlag(this=0x0000000000000000, mask=IsContainerFlag) const + 33 at Node.h:724, name = 'Chrome_InProcRendererThread', stop reason = EXC_BAD_ACCESS (code=1, address=0x18) * frame #0: 0x0000000105e0a621 Content Shell Framework`blink::Node::getFlag(this=0x0000000000000000, mask=IsContainerFlag) const + 33 at Node.h:724 frame #1: 0x0000000104b567c8 Content Shell Framework`blink::Node::isContainerNode(this=0x0000000000000000) const + 40 at Node.h:244 frame #2: 0x0000000104b578ea Content Shell Framework`blink::Node::firstChild(this=0x0000000000000000) const + 42 at ContainerNode.h:338 frame #3: 0x0000000104b91933 Content Shell Framework`blink::NodeTraversal::firstChild(parent=0x0000000000000000) + 35 at NodeTraversal.h:91 frame #4: 0x0000000104b92a43 Content Shell Framework`blink::NodeTraversal::hasChildren(parent=0x0000000000000000) + 35 at NodeTraversal.h:89 frame #5: 0x0000000104b9fc2c Content Shell Framework`blink::PositionIteratorAlgorithm<blink::EditingAlgorithm<blink::NodeTraversal> >::computePosition(this=0x0000000132d070d0) const + 236 at PositionIterator.cpp:74 frame #6: 0x0000000105612647 Content Shell Framework`blink::PositionAlgorithm<blink::EditingAlgorithm<blink::NodeTraversal> > blink::nextCandidateAlgorithm<blink::EditingAlgorithm<blink::NodeTraversal> >(position=0x0000000132d07278) + 103 at htmlediting.cpp:339 frame #7: 0x000000010560c6fa Content Shell Framework`blink::nextCandidate(position=0x0000000132d07278) + 42 at htmlediting.cpp:348 frame #8: 0x00000001055f2db6 Content Shell Framework`blink::PositionAlgorithm<blink::EditingAlgorithm<blink::NodeTraversal> > blink::canonicalPosition<blink::PositionAlgorithm<blink::EditingAlgorithm<blink::NodeTraversal> > >(passedPosition=0x0000000132d074d0) + 614 at VisiblePosition.cpp:590 frame #9: 0x00000001055f48e1 Content Shell Framework`void blink::VisiblePosition::init<blink::PositionAlgorithm<blink::EditingAlgorithm<blink::NodeTraversal> > >(this=0x0000000132d074e8, position=0x0000000132d074d0, affinity=DOWNSTREAM) + 81 at VisiblePosition.cpp:643 frame #10: 0x00000001055effd1 Content Shell Framework`blink::VisiblePosition::VisiblePosition(this=0x0000000132d074e8, pos=0x0000000132d074d0, affinity=DOWNSTREAM) + 65 at VisiblePosition.cpp:53 frame #11: 0x00000001055f0021 Content Shell Framework`blink::VisiblePosition::VisiblePosition(this=0x0000000132d074e8, pos=0x0000000132d074d0, affinity=DOWNSTREAM) + 49 at VisiblePosition.cpp:52 frame #12: 0x000000010552ba1a Content Shell Framework`blink::DOMSelection::setBaseAndExtent(this=0x0000057941a04de0, baseNode=0x00000c97fb21c090, baseOffset=0, extentNode=0x0000000000000000, extentOffset=0, exceptionState=0x0000000132d07910) + 602 at DOMSelection.cpp:280 frame #13: 0x000000010631f2e0 Content Shell Framework`blink::DOMSelectionV8Internal::setBaseAndExtentMethod(info=0x0000000132d07a30) + 2576 at V8Selection.cpp:396 frame #14: 0x000000010631c415 Content Shell Framework`blink::DOMSelectionV8Internal::setBaseAndExtentMethodCallback(info=0x0000000132d07a30) + 117 at V8Selection.cpp:407 frame #15: 0x000000010364a65a Content Shell Framework`v8::internal::FunctionCallbackArguments::Call(this=0x0000000132d07af0, f=0x000000010631c3a0)(v8::FunctionCallbackInfo<v8::Value> const&)) + 186 at arguments.cc:33 frame #16: 0x000000010368ad7a Content Shell Framework`v8::internal::MaybeHandle<v8::internal::Object> v8::internal::HandleApiCallHelper<false>(isolate=0x000000011e04a600, args=<unavailable>)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>&) + 1530 at builtins.cc:1092 frame #17: 0x000000010368e845 Content Shell Framework`v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [inlined] v8::internal::Builtin_Impl_HandleApiCall(args=<unavailable>)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*) + 55 at builtins.cc:1115 frame #18: 0x000000010368e80e Content Shell Framework`v8::internal::Builtin_HandleApiCall(args_length=<unavailable>, args_object=<unavailable>, isolate=0x000000011e04a600) + 126 at builtins.cc:1111 Long story short, since we insert m_anchorNode->layoutObject() in PositionIteratorAlgorithm<Strategy>::increment(), the m_anchorNode could be a null pointer by' m_anchorNode = Strategy::parent(*m_nodeAfterPositionInAnchor); Unfortunately it was accessed without null-checking in PositionIteratorAlgorithm<Strategy>::deprecatedComputePosition(). So I think we can avoid the crash by inserting null check before using it. Does this make sense to you, Yosi?
> PositionIteratorAlgorithm<Strategy>::deprecatedComputePosition(). Err, it should be PositionIteratorAlgorithm<Strategy>::computePosition().
On 2015/07/31 05:33:53, changseok wrote: > > PositionIteratorAlgorithm<Strategy>::deprecatedComputePosition(). > > Err, it should be PositionIteratorAlgorithm<Strategy>::computePosition(). It sounds strange that since this patch is optimization, we don't expect change behavior.
https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... File Source/core/dom/PositionIterator.cpp (right): https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... Source/core/dom/PositionIterator.cpp:74: if (m_anchorNode && Strategy::hasChildren(*m_anchorNode)) Could you remove |m_anchorNode|? |computePosition()| should be identical to |deprecatedComputePositon()| except for calling |createLegacyEditing()|. https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... Source/core/dom/PositionIterator.cpp:76: if (m_anchorNode && m_anchorNode->isTextNode()) Could you remove |m_anchorNode|? |computePosition()| should be identical to |deprecatedComputePositon()| except for calling |createLegacyEditing()|.
On 2015/07/31 06:37:01, Yosi_UTC9 wrote: > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > File Source/core/dom/PositionIterator.cpp (right): > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > Source/core/dom/PositionIterator.cpp:74: if (m_anchorNode && > Strategy::hasChildren(*m_anchorNode)) > Could you remove |m_anchorNode|? |computePosition()| should be identical to > |deprecatedComputePositon()| except for calling |createLegacyEditing()|. > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > Source/core/dom/PositionIterator.cpp:76: if (m_anchorNode && > m_anchorNode->isTextNode()) > Could you remove |m_anchorNode|? |computePosition()| should be identical to > |deprecatedComputePositon()| except for calling |createLegacyEditing()|. Hrm.. I am not sure if I understand your point correctly. It sounds the m_anchorNode should not be null here. Then do we need to insert null-checking somewhere else? If the anchorNode has neither layoutObject nor a parent in PositionIteratorAlgorithm<Strategy>::increment(), what happens?
On 2015/07/31 08:40:11, changseok wrote: > On 2015/07/31 06:37:01, Yosi_UTC9 wrote: > > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > > File Source/core/dom/PositionIterator.cpp (right): > > > > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > > Source/core/dom/PositionIterator.cpp:74: if (m_anchorNode && > > Strategy::hasChildren(*m_anchorNode)) > > Could you remove |m_anchorNode|? |computePosition()| should be identical to > > |deprecatedComputePositon()| except for calling |createLegacyEditing()|. > > > > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > > Source/core/dom/PositionIterator.cpp:76: if (m_anchorNode && > > m_anchorNode->isTextNode()) > > Could you remove |m_anchorNode|? |computePosition()| should be identical to > > |deprecatedComputePositon()| except for calling |createLegacyEditing()|. > > Hrm.. I am not sure if I understand your point correctly. It sounds the > m_anchorNode should not be null here. > Then do we need to insert null-checking somewhere else? > If the anchorNode has neither layoutObject nor a parent in > PositionIteratorAlgorithm<Strategy>::increment(), what happens? I think null anchorNode is brought by caller rather than PositionIterator. PositionIterator::computePosition() should be called anchorNode != nullptr. So, caller pattern: while (!iterator.atEnd()) { iterator.increment(); auto positon = iterator.computePosition(); if (use position) ... } is dangerous, because iterator.incremeant() can move iterator to end. Above is similar to while (str < end) { ++str; if (use(*str)) ... } I proposed to change call sites of PositionIterator as http://crrev.com/1273583003
On 2015/08/06 02:01:53, Yosi_UTC9 wrote: > On 2015/07/31 08:40:11, changseok wrote: > > On 2015/07/31 06:37:01, Yosi_UTC9 wrote: > > > > > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > > > File Source/core/dom/PositionIterator.cpp (right): > > > > > > > > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > > > Source/core/dom/PositionIterator.cpp:74: if (m_anchorNode && > > > Strategy::hasChildren(*m_anchorNode)) > > > Could you remove |m_anchorNode|? |computePosition()| should be identical to > > > |deprecatedComputePositon()| except for calling |createLegacyEditing()|. > > > > > > > > > https://codereview.chromium.org/1189543002/diff/260001/Source/core/dom/Positi... > > > Source/core/dom/PositionIterator.cpp:76: if (m_anchorNode && > > > m_anchorNode->isTextNode()) > > > Could you remove |m_anchorNode|? |computePosition()| should be identical to > > > |deprecatedComputePositon()| except for calling |createLegacyEditing()|. > > > > Hrm.. I am not sure if I understand your point correctly. It sounds the > > m_anchorNode should not be null here. > > Then do we need to insert null-checking somewhere else? > > If the anchorNode has neither layoutObject nor a parent in > > PositionIteratorAlgorithm<Strategy>::increment(), what happens? > > I think null anchorNode is brought by caller rather than PositionIterator. > PositionIterator::computePosition() should be called anchorNode != nullptr. > > So, caller pattern: > while (!iterator.atEnd()) { > iterator.increment(); > auto positon = iterator.computePosition(); > if (use position) ... > } > > is dangerous, because iterator.incremeant() can move iterator to end. > > Above is similar to > > while (str < end) { > ++str; > if (use(*str)) ... > } > > I proposed to change call sites of PositionIterator as > http://crrev.com/1273583003 Thanks for your detailed guide. In fact, I tried a similar approach to yours. At that time, I thought inserting null-checking in computePosition() was better because I could reduce code changes. But I don't mind moving the null-check to caller's places. Instead, I improved your approach a little bit(but it should work same.), please have a look. If you don't have an objection, I'd like to land this soon.
lgtm Thanks for long iteration!
The CQ bit was checked by yosin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from leviw@chromium.org Link to the patchset: https://codereview.chromium.org/1189543002/#ps280001 (title: "Updated patch")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1189543002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1189543002/280001
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200236 |