Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1904453003:
Change finding common ancestor from O(n^2) to O(n) (Closed)
|
Created:
4 years, 8 months ago by Navid Zolghadr Modified:
4 years, 8 months ago CC:
chromium-reviews, blink-reviews, dtapuska+blinkwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange finding common ancestor from O(n^2) to O(n)
Changing the method of finding the common ancestor
of two nodes in the boundary transition events from
O(n^2) to O(n).
BUG=602264
Committed: https://crrev.com/c49fdc54f02eb0d4fe3ca0faf620ad8d45ac77f7
Cr-Commit-Position: refs/heads/master@{#389600}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : #
Total comments: 9
Patch Set 4 : Applying the comments #
Total comments: 27
Patch Set 5 : Applying comments #Patch Set 6 : Rebased #Messages
Total messages: 27 (8 generated)
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, mustaq@chromium.org
Great catch, Navid! We should have seen this simple solution before. LGTM % renaming etc. https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (left): https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:183: HeapVector<Member<Node>, 32> exitedAncestors; Please add a comment that httparchive=>97% cases have depth<20, and cut down the initial size to, say, 20. https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:82: void findCommonAncestor( s/findCommonAncestor/buildAncestorChainsAndFindCommonAncestors/? Didn't find a shorter alternative! https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:103: exitedAncestorsCommonParentIndex = exitedAncestors.size(); Add DCHECKs to emphasize that we are not subtracting 1 from 0U. https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:105: for (size_t i = exitedAncestors.size(), j = enteredAncestors.size(); Nit: I think a |while| loop modifying the |*Index| vars directly would be cleaner than the 2-line |for| loop.
https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:82: void findCommonAncestor( Can we add some comments around this in terms of the ordering of the arrays. When I first looked at it I would have expected the arrays to be reversed like enteredAncestors[0] is the root; but the arrays are actually inverted.
ptal https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (left): https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:183: HeapVector<Member<Node>, 32> exitedAncestors; On 2016/04/20 16:09:05, mustaq wrote: > Please add a comment that httparchive=>97% cases have depth<20, and cut down the > initial size to, say, 20. Done. https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:82: void findCommonAncestor( On 2016/04/20 19:42:28, dtapuska wrote: > Can we add some comments around this in terms of the ordering of the arrays. > > When I first looked at it I would have expected the arrays to be reversed > > like enteredAncestors[0] is the root; but the arrays are actually inverted. Done. https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:103: exitedAncestorsCommonParentIndex = exitedAncestors.size(); On 2016/04/20 16:09:05, mustaq wrote: > Add DCHECKs to emphasize that we are not subtracting 1 from 0U. Why is that needed though? The for/while condition prevents that from happening anyway. Also I believe these arrays may be empty as the target might have been removed from its document. https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:105: for (size_t i = exitedAncestors.size(), j = enteredAncestors.size(); On 2016/04/20 16:09:06, mustaq wrote: > Nit: I think a |while| loop modifying the |*Index| vars directly would be > cleaner than the 2-line |for| loop. Done.
On 2016/04/21 14:46:41, Navid Zolghadr wrote: > ptal > > https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (left): > > https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:183: > HeapVector<Member<Node>, 32> exitedAncestors; > On 2016/04/20 16:09:05, mustaq wrote: > > Please add a comment that httparchive=>97% cases have depth<20, and cut down > the > > initial size to, say, 20. > > Done. > > https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): > > https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:82: void > findCommonAncestor( > On 2016/04/20 19:42:28, dtapuska wrote: > > Can we add some comments around this in terms of the ordering of the arrays. > > > > When I first looked at it I would have expected the arrays to be reversed > > > > like enteredAncestors[0] is the root; but the arrays are actually inverted. > > Done. > > https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:103: > exitedAncestorsCommonParentIndex = exitedAncestors.size(); > On 2016/04/20 16:09:05, mustaq wrote: > > Add DCHECKs to emphasize that we are not subtracting 1 from 0U. > > Why is that needed though? The for/while condition prevents that from happening > anyway. Also I believe these arrays may be empty as the target might have been > removed from its document. > > https://codereview.chromium.org/1904453003/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/PointerEventManager.cpp:105: for (size_t i > = exitedAncestors.size(), j = enteredAncestors.size(); > On 2016/04/20 16:09:06, mustaq wrote: > > Nit: I think a |while| loop modifying the |*Index| vars directly would be > > cleaner than the 2-line |for| loop. > > Done. lgtm
Still LGTM.
nzolghadr@chromium.org changed reviewers: + bokan@chromium.org
bokan@chromium.org: Please review changes in third_party/WebKit/Source/core/input/PointerEventManager.cpp
Looks good overall, just some style and suggest sprinkling in some DCHECKs https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:83: EventTarget* exitedTarget, EventTarget* enteredTarget, Seems like these can't be null? If that's true, make them references, preferably const ref if possible. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:87: size_t& enteredAncestorsCommonParentIndex) Output parameters should be passed by pointer. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:91: I see you check if exitedTarget == enteredTarget before calling here, but adding an DCHECK in here wouldn't hurt. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:93: Node* exitedNode = exitedTarget->toNode(); Either DCHECK that exitedNode != nullptr (if indeed it must be a Node) or handle the case where it's not a Node. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:101: enteredNode->updateDistribution(); Question: what does updateDistribution do? https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:107: enteredAncestorsCommonParentIndex = enteredAncestors.size(); One more suggested DCHECK here :), exitedAncestors[exitedAncestors.size() - 1] == enteredAncestors[enteredAncestors.size() - 1], that is, they should have a common root. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:219: // Based on httparchive, in more than 97% cases the depth of DOM is less Doesn't this mean that in 3% of cases we'll incur a heap allocation in here? That seems a bit excessive. any reason not to leave it at 32? Or at least a a number that's a couple of nines worth of percent?
https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:83: EventTarget* exitedTarget, EventTarget* enteredTarget, On 2016/04/21 21:38:29, bokan wrote: > Seems like these can't be null? If that's true, make them references, preferably > const ref if possible. No. They can be null. The isInDocument checks for the nullity and returns false. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:87: size_t& enteredAncestorsCommonParentIndex) On 2016/04/21 21:38:28, bokan wrote: > Output parameters should be passed by pointer. You mean like size_t* and HeapVector<Member<Node>, 20>*? Are you sure we do that here in Blink? That seems to be a C way of doing this. Then do I need to add another check to make sure those pointer are not null? https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:91: On 2016/04/21 21:38:29, bokan wrote: > I see you check if exitedTarget == enteredTarget before calling here, but adding > an DCHECK in here wouldn't hurt. I don't think that is needed. This function doesn't make any assumption whether those nodes are the same or not. It just fills up the ancestor array and if they happen to be the same node that single node will be the only member of those two arrays which is correct. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:93: Node* exitedNode = exitedTarget->toNode(); On 2016/04/21 21:38:28, bokan wrote: > Either DCHECK that exitedNode != nullptr (if indeed it must be a Node) or handle > the case where it's not a Node. Neither the DCHECK nor the behavior of handling when it is not a node was there before (as I just moved this code here in this function). So I assume it means it is always a node. So I will just add a DCHECK here. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:101: enteredNode->updateDistribution(); On 2016/04/21 21:38:29, bokan wrote: > Question: what does updateDistribution do? To be honest with you, I'm not sure myself. From the looks of it and where it's used around the code it seems whenever we want to play with dom like take parents of the nodes if there was a call previously to js we call updateDistribution to make sure we restructure/re-style or something like that if js had changed the dom as the result of the previous call. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:107: enteredAncestorsCommonParentIndex = enteredAncestors.size(); On 2016/04/21 21:38:29, bokan wrote: > One more suggested DCHECK here :), exitedAncestors[exitedAncestors.size() - 1] > == enteredAncestors[enteredAncestors.size() - 1], that is, they should have a > common root. That's not right. They might be from different documents and in that case they don't have a common root at all. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:219: // Based on httparchive, in more than 97% cases the depth of DOM is less On 2016/04/21 21:38:29, bokan wrote: > Doesn't this mean that in 3% of cases we'll incur a heap allocation in here? > That seems a bit excessive. any reason not to leave it at 32? Or at least a a > number that's a couple of nines worth of percent? I don't have a strong answer here. I had it at 32. But Mustaq suggested to make it 20. Either way I don't think that matters at all 20 or 32. By creating 20 we are already allocating that whole array in the memory. If we add more, the array just expands (and reallocate that 20 that was filled). So in 3% of the cases we reallocate that 20. Either way 12 less pointer (in the case of 20) doesn't really change the memory or reallocating/copying 20 pointer (in the case of 32) shouldn't be time consuming here at all.
https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:83: EventTarget* exitedTarget, EventTarget* enteredTarget, On 2016/04/21 23:40:14, Navid Zolghadr wrote: > On 2016/04/21 21:38:29, bokan wrote: > > Seems like these can't be null? If that's true, make them references, > preferably > > const ref if possible. > > No. They can be null. The isInDocument checks for the nullity and returns false. Got it, ok, fine as they are (if possible though, make them pointer to const) https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:87: size_t& enteredAncestorsCommonParentIndex) On 2016/04/21 23:40:14, Navid Zolghadr wrote: > On 2016/04/21 21:38:28, bokan wrote: > > Output parameters should be passed by pointer. > > You mean like size_t* and HeapVector<Member<Node>, 20>*? Are you sure we do > that here in Blink? That seems to be a C way of doing this. > Then do I need to add another check to make sure those pointer are not null? Yup, see the bit about output params here: https://google.github.io/styleguide/cppguide.html#Reference_Arguments (when Blink and Chromium style guide doesn't have a specific rule, we use Google style). Reference output probably exists in some places in Blink, but we use pointer so it's clearer from the call site the param will be output. You can put DCHECKs for nullity but also put 'out' in the param names. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:91: On 2016/04/21 23:40:14, Navid Zolghadr wrote: > On 2016/04/21 21:38:29, bokan wrote: > > I see you check if exitedTarget == enteredTarget before calling here, but > adding > > an DCHECK in here wouldn't hurt. > > I don't think that is needed. This function doesn't make any assumption whether > those nodes are the same or not. It just fills up the ancestor array and if they > happen to be the same node that single node will be the only member of those two > arrays which is correct. Whoops, yah, you're right. That's fine. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:93: Node* exitedNode = exitedTarget->toNode(); On 2016/04/21 23:40:14, Navid Zolghadr wrote: > On 2016/04/21 21:38:28, bokan wrote: > > Either DCHECK that exitedNode != nullptr (if indeed it must be a Node) or > handle > > the case where it's not a Node. > > Neither the DCHECK nor the behavior of handling when it is not a node was there > before (as I just moved this code here in this function). So I assume it means > it is always a node. So I will just add a DCHECK here. Acknowledged. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:101: enteredNode->updateDistribution(); On 2016/04/21 23:40:14, Navid Zolghadr wrote: > On 2016/04/21 21:38:29, bokan wrote: > > Question: what does updateDistribution do? > > To be honest with you, I'm not sure myself. From the looks of it and where it's > used around the code it seems whenever we want to play with dom like take > parents of the nodes if there was a call previously to js we call > updateDistribution to make sure we restructure/re-style or something like that > if js had changed the dom as the result of the previous call. Hmm, I've never seen it before. But I can see it was there before, just curious what it meant :) https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:107: enteredAncestorsCommonParentIndex = enteredAncestors.size(); On 2016/04/21 23:40:14, Navid Zolghadr wrote: > On 2016/04/21 21:38:29, bokan wrote: > > One more suggested DCHECK here :), exitedAncestors[exitedAncestors.size() - 1] > > == enteredAncestors[enteredAncestors.size() - 1], that is, they should have a > > common root. > > That's not right. They might be from different documents and in that case they > don't have a common root at all. That's what I'm getting at, you have no way of distinguishing that case from the return values here, it'll just return the first element in each vector (i.e. the targets themselves) as the common ancestor even if they're different nodes. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:219: // Based on httparchive, in more than 97% cases the depth of DOM is less On 2016/04/21 23:40:14, Navid Zolghadr wrote: > On 2016/04/21 21:38:29, bokan wrote: > > Doesn't this mean that in 3% of cases we'll incur a heap allocation in here? > > That seems a bit excessive. any reason not to leave it at 32? Or at least a a > > number that's a couple of nines worth of percent? > > I don't have a strong answer here. > I had it at 32. But Mustaq suggested to make it 20. Either way I don't think > that matters at all 20 or 32. By creating 20 we are already allocating that > whole array in the memory. If we add more, the array just expands (and > reallocate that 20 that was filled). So in 3% of the cases we reallocate that > 20. Either way 12 less pointer (in the case of 20) doesn't really change the > memory or reallocating/copying 20 pointer (in the case of 32) shouldn't be time > consuming here at all. Heap allocations can be fairly expensive and unpredictable, at least (IMO) compared to saving 12 WORDs, and this would be especially important in event delivery where we care about latency. Though I agree in this case it's small and unlikely to make a difference in practice, just more "death by a thousand paper cuts". Anyway, I'll leave the decision up to you guys :)
https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:107: enteredAncestorsCommonParentIndex = enteredAncestors.size(); On 2016/04/22 00:52:54, bokan wrote: > On 2016/04/21 23:40:14, Navid Zolghadr wrote: > > On 2016/04/21 21:38:29, bokan wrote: > > > One more suggested DCHECK here :), exitedAncestors[exitedAncestors.size() - > 1] > > > == enteredAncestors[enteredAncestors.size() - 1], that is, they should have > a > > > common root. > > > > That's not right. They might be from different documents and in that case they > > don't have a common root at all. > > That's what I'm getting at, you have no way of distinguishing that case from the > return values here, it'll just return the first element in each vector (i.e. the > targets themselves) as the common ancestor even if they're different nodes. No. That is not the case. Here is an example if we call buildAncestorChainsAndFindCommonAncestors(A,B) If C is the root and and happens to be parent of both A and B A->C B->C these indices will be 1. But if they are in two different documents A->D B->C Then the indices are 2. If the two targets happen to be the same (i.e. A==B) then the indices are 0. Is that right? Or maybe I'm not quite understanding your case.
Ok, lgtm with the changes (that actually made sense) https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:107: enteredAncestorsCommonParentIndex = enteredAncestors.size(); On 2016/04/22 01:46:13, Navid Zolghadr wrote: > On 2016/04/22 00:52:54, bokan wrote: > > On 2016/04/21 23:40:14, Navid Zolghadr wrote: > > > On 2016/04/21 21:38:29, bokan wrote: > > > > One more suggested DCHECK here :), exitedAncestors[exitedAncestors.size() > - > > 1] > > > > == enteredAncestors[enteredAncestors.size() - 1], that is, they should > have > > a > > > > common root. > > > > > > That's not right. They might be from different documents and in that case > they > > > don't have a common root at all. > > > > That's what I'm getting at, you have no way of distinguishing that case from > the > > return values here, it'll just return the first element in each vector (i.e. > the > > targets themselves) as the common ancestor even if they're different nodes. > > No. That is not the case. Here is an example > if we call buildAncestorChainsAndFindCommonAncestors(A,B) > > If C is the root and and happens to be parent of both A and B > A->C > B->C > > these indices will be 1. But if they are in two different documents > A->D > B->C > > Then the indices are 2. If the two targets happen to be the same (i.e. A==B) > then the indices are 0. Is that right? Or maybe I'm not quite understanding your > case. > Bah, I'm not having a good day...you're totally right.
ptal. Particularly regarding the const parameters. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:83: EventTarget* exitedTarget, EventTarget* enteredTarget, On 2016/04/22 00:52:54, bokan wrote: > On 2016/04/21 23:40:14, Navid Zolghadr wrote: > > On 2016/04/21 21:38:29, bokan wrote: > > > Seems like these can't be null? If that's true, make them references, > > preferably > > > const ref if possible. > > > > No. They can be null. The isInDocument checks for the nullity and returns > false. > > Got it, ok, fine as they are (if possible though, make them pointer to This turned out to be harder than I thought. Marking these EventTargets as const make toNode function not callable as that function is not marked as const and changing that would need me to change quite a lot of things around the code. Are you okay with having these pointers as non-const or do you have a better solution like maybe const_cast which I'm not sure is as cleaner than what it is now? https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:87: size_t& enteredAncestorsCommonParentIndex) On 2016/04/22 00:52:54, bokan wrote: > On 2016/04/21 23:40:14, Navid Zolghadr wrote: > > On 2016/04/21 21:38:28, bokan wrote: > > > Output parameters should be passed by pointer. > > > > You mean like size_t* and HeapVector<Member<Node>, 20>*? Are you sure we do > > that here in Blink? That seems to be a C way of doing this. > > Then do I need to add another check to make sure those pointer are not null? > > Yup, see the bit about output params here: > https://google.github.io/styleguide/cppguide.html#Reference_Arguments (when > Blink and Chromium style guide doesn't have a specific rule, we use Google > style). Reference output probably exists in some places in Blink, but we use > pointer so it's clearer from the call site the param will be output. > > You can put DCHECKs for nullity but also put 'out' in the param names. Done. https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:93: Node* exitedNode = exitedTarget->toNode(); On 2016/04/22 00:52:54, bokan wrote: > On 2016/04/21 23:40:14, Navid Zolghadr wrote: > > On 2016/04/21 21:38:28, bokan wrote: > > > Either DCHECK that exitedNode != nullptr (if indeed it must be a Node) or > > handle > > > the case where it's not a Node. > > > > Neither the DCHECK nor the behavior of handling when it is not a node was > there > > before (as I just moved this code here in this function). So I assume it means > > it is always a node. So I will just add a DCHECK here. > > Acknowledged. Done.
https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/PointerEventManager.cpp (right): https://codereview.chromium.org/1904453003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/PointerEventManager.cpp:83: EventTarget* exitedTarget, EventTarget* enteredTarget, On 2016/04/22 17:04:12, Navid Zolghadr wrote: > On 2016/04/22 00:52:54, bokan wrote: > > On 2016/04/21 23:40:14, Navid Zolghadr wrote: > > > On 2016/04/21 21:38:29, bokan wrote: > > > > Seems like these can't be null? If that's true, make them references, > > > preferably > > > > const ref if possible. > > > > > > No. They can be null. The isInDocument checks for the nullity and returns > > false. > > > > Got it, ok, fine as they are (if possible though, make them pointer to > > This turned out to be harder than I thought. Marking these EventTargets as const > make toNode function not callable as that function is not marked as const and > changing that would need me to change quite a lot of things around the code. Are > you okay with having these pointers as non-const or do you have a better > solution like maybe const_cast which I'm not sure is as cleaner than what it is > now? Nah, if it's more trouble lgtm as-is. Our code is only mostly const-correct...
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, dtapuska@chromium.org Link to the patchset: https://codereview.chromium.org/1904453003/#ps80001 (title: "Applying comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904453003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904453003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mustaq@chromium.org, dtapuska@chromium.org, bokan@chromium.org Link to the patchset: https://codereview.chromium.org/1904453003/#ps100001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1904453003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1904453003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Change finding common ancestor from O(n^2) to O(n) Changing the method of finding the common ancestor of two nodes in the boundary transition events from O(n^2) to O(n). BUG=602264 ========== to ========== Change finding common ancestor from O(n^2) to O(n) Changing the method of finding the common ancestor of two nodes in the boundary transition events from O(n^2) to O(n). BUG=602264 Committed: https://crrev.com/c49fdc54f02eb0d4fe3ca0faf620ad8d45ac77f7 Cr-Commit-Position: refs/heads/master@{#389600} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/c49fdc54f02eb0d4fe3ca0faf620ad8d45ac77f7 Cr-Commit-Position: refs/heads/master@{#389600} |
