Implement a Hit Test Cache.

Implement a Hit Test Cache for same point hits.
    
Introduce a HitTestCache which can be used to return hit tests from the
cache. Currently each cache hit is verified against the true result and
a UMA metric recorded. Once no false positives have been seen in UMA short
circuiting the verify will be done.
    
Rect and list based hit tests aren't cached just yet;
again this will be a future improvement.
    
Add Internals APIs to return the hit test cache hits, the ability to
clear the hit test cache and make specialized requests.
    
This commit attempt fixes two issues that appeared to cause some flaky test
failures
1) List based requests were returned which it appears we can do non-rect
based list requests which I didn't anticipate.
2) LayoutPart incorrected allowed the HitTestResult to be cached on the
parent's frame causing a set of incorrect results. Add test to check this.
    
BUG=398920
TEST=Tools/Scripts/run-webkit-tests

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197233

[Rick Byers, 2015-05-25 19:11:30]

rbyers@chromium.org changed reviewers:
+ rbyers@chromium.org

[Rick Byers, 2015-05-25 19:11:30]

I'm impressed with how far you've gotten with a relatively small amount of
additional complexity.  Of course the devil is in the details here.

Can you update LayoutTests/fast/events/hit-test-counts.html (or add a similar
test) to demonstrate the benefit on simple GestureTap* cases?

Also, I know it's early but I'm anxious to see some sort of telemetry test that
quantifies the benefit (in terms of reduce CPU / power usage) on mouse move
cases.  I'm not sure what the best way to construct realistic test cases are. 
Maybe go through our top25 sites and record an event sequence of yourself moving
the mouse around in a realistic way?

[esprehn, 2015-05-25 21:01:44]

esprehn@chromium.org changed reviewers:
+ esprehn@chromium.org

[esprehn, 2015-05-25 21:01:45]

You don't need to worry about the compositor animations, the main thread is
sync'ed with them every frame.

https://codereview.chromium.org/1142283004/diff/1/Source/core/frame/FrameView...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/1142283004/diff/1/Source/core/frame/FrameView...
Source/core/frame/FrameView.cpp:962: layoutView()->clearHitTestCache();
This is not enough, you also need to invalidate on style, dom modification, and
scroll...

[dtapuska, 2015-05-28 17:27:00]

On 2015/05/25 21:01:45, esprehn wrote:
> You don't need to worry about the compositor animations, the main thread is
> sync'ed with them every frame.
> 
>
https://codereview.chromium.org/1142283004/diff/1/Source/core/frame/FrameView...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/1142283004/diff/1/Source/core/frame/FrameView...
> Source/core/frame/FrameView.cpp:962: layoutView()->clearHitTestCache();
> This is not enough, you also need to invalidate on style, dom modification,
and
> scroll...

PTAL. I think I have addressed invalidate on style, scroll. I believe any
relevant dom modifications will cause a layout (so I didn't explicitly cover
that).

[dtapuska, 2015-05-28 17:46:51]

dtapuska@chromium.org changed reviewers:
+ tdresser@chromium.org

[dtapuska, 2015-06-03 14:01:46]

On 2015/05/28 17:27:00, Dave Tapuska wrote:
> On 2015/05/25 21:01:45, esprehn wrote:
> > You don't need to worry about the compositor animations, the main thread is
> > sync'ed with them every frame.
> > 
> >
>
https://codereview.chromium.org/1142283004/diff/1/Source/core/frame/FrameView...
> > File Source/core/frame/FrameView.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1142283004/diff/1/Source/core/frame/FrameView...
> > Source/core/frame/FrameView.cpp:962: layoutView()->clearHitTestCache();
> > This is not enough, you also need to invalidate on style, dom modification,
> and
> > scroll...
> 
> PTAL. I think I have addressed invalidate on style, scroll. I believe any
> relevant dom modifications will cause a layout (so I didn't explicitly cover
> that).

Ping.

[Rick Byers, 2015-06-05 20:48:59]

Sorry for the long delay - finally getting through my review backlog after
vacation.  I haven't quite finished reviewing (need to look only at
HitTestCache.cpp now), but wanted to get you these before I have to leave.  Will
finish ASAP.

This is looking great - quite simple, not too intrusive and providing clear
benefit.

Have you done any perf measurement of the benefit, eg. on our key_hit_test_cases
test (cpu usage for a bunch of taps)?  Also, have you verified that this
addresses the PEP elementFromPoint shadowDom scenario?

Probably a good idea to add a link to your design doc to the CL description.

/cc pdr@ who may be interested in this too.

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
File LayoutTests/fast/events/hit-test-cache.html (right):

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache.html:73: debug('Hit test fixed div after
scroll');
please add at least one simple test case where the cache does get hit (just to
verify that the cache is working at all in this test setup).

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache.html:80: 
nit: debug('') here to make the output more readable

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache.html:85: reportElementAt(12, 12);
I'd prefer if the test code indicated what the expected behavior was (eg. with
shouldBe, etc.).  I know it's captured by the expected output, but I've seen
scenarios where people mis-understand the test intentions and update the
expected output and then future readers have no idea that the test is actually
broken and expecting failure.

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
File LayoutTests/fast/events/hit-test-counts-expected.txt (right):

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-counts-expected.txt:13: MouseDown: 3/2 [with
multiTargetTapNotification: 3/1]
multiTargetTapNotification really shouldn't be changing the results for non-tap
scenarios.  Presumably this is a side-effect of the state of the cache, right? 
Perhaps you should be flushing your cache before doing the
multiTargetTapNotification version to avoid such confusing side effects?

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
File LayoutTests/fast/events/hit-test-counts.html (right):

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-counts.html:63: countStr += ' ' +
hitTestCountDelta(documents[i]) + "/" + hitTestCacheHitsDelta(documents[i]);
nit: I'd find this more readable if it was "<misses>+<hits>" instead of
"total/hits".  It's really the misses number I care most about in this test -
would be nice to be able to scan without doing the math for each result.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1230: HitTestResult result =
hitTestInDocument(this, x, y, true);
This pattern is generally frowned upon in Google C++, see the true/false part of
http://google.github.io/styleguide/cppguide.html#Implementation_Comments

If skipping the cache is unusual, then I'd suggest having a
hitTestInDocumentNoCache method and leaving the 3 arg method alone.  Alternately
you could use an enum instead of a bool or (usually lowest preference) add a
comment at each call site saying what the true/false means.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:12: class HitTestCache {
Please add a brief comment describing the overall intention here (maybe making
specific reference to the issues like GestureTap where normally the target
doesn't change but occasionally it needs to - or just link to your doc for
details).

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:21: static void verify(const HitTestResult&,
const HitTestResult&);
the other two methods seem self explanatory here, but this one doesn't
necessarily - please add a comment.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:29: MISS_VALIDITY_RECT_MATCHES, // Region
matches by request type doesn't.
s/by/but/

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:56: // size of 1.
Do you think such ping-ponging of flags that affect the hit test could be fixed?
 Seems like it probably shouldn't happen? Maybe file a separate bug to track
that.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestResult.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestResult.h:67: void cacheValues(const HitTestResult&);
please add a brief comment describing when cacheValues is intended to be used
(not immediately obvious to me having not yet seen the callers).

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:1803: view()->clearHitTestCache();
I don't know enough about the style system to say for sure that this is the
right place.  Hopefully esprehn will chime in, but if not and manual testing
isn't showing any false cache hits, then go ahead and land it and we'll see what
we get from UMA :-)

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutView.cpp:130: bool cacheHit = cacheHit =
m_hitTestCache.check(cacheResult);
remove extra cacheHit

[Rick Byers, 2015-06-06 16:12:54]

Ok, all done.  Looks great, just a couple more minor suggestions.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:9: #include <base/macros.h>
What do you need this for?  It's still very rare for blink to depend on base
(outside of tests).  Not sure if it really matters, just curious.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:19: // For now we don't support rect based
hit results.
But list-based tests (like elementsFromPoint) work OK?  Please add a basic test
that covers that.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:21: for (size_t i = 0; m_items[i].m_valid &&
i < arraysize(m_items); ++i) {
i < arraysize(m_items) should come before the m_valid check to avoid reading
past the end of m_items.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:18: bool check(HitTestResult& /*result*/);
nit: no need for /*result*/

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:21: static void verify(const HitTestResult&,
const HitTestResult&);
Since the order of the arguments are important (at least to the metric
generated), please include the argument names here since they do communicate
some extra value.  See #9 at
https://www.chromium.org/blink/coding-style#TOC-Names for a good rule of thumb.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestRequest.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestRequest.h:80: static const HitTestRequestType
CacheabilityBits = ReadOnly | Active | Move | Release | TouchEvent;
this probably deserves a comment saying these bits don't affect the result of
hit tests themselves (and a TODO saying they should be moved out of the enum?).

[esprehn, 2015-06-06 21:14:30]

You need to also clear the cache if there's a DOM mutation, we can do a mutation
and not actually do layout (ex. an abs pos element is removed).

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1230: HitTestResult result =
hitTestInDocument(this, x, y, true);
Yes, please no bools like this.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:277: Element* TreeScope::elementFromPoint(int x,
int y) const
elementFromPoint(int x, int y, HitTestCacheType = UseHitTestCache)

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:282: Element*
TreeScope::elementFromPointNoCache(int x, int y) const
Use default args instead, no need for two methods.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:181: Element* hitTestPoint(int x, int y, bool
useCache) const;
Can we use an IntPoint?

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:9: #include <base/macros.h>
On 2015/06/06 at 16:12:54, Rick Byers wrote:
> What do you need this for?  It's still very rare for blink to depend on base
(outside of tests).  Not sure if it really matters, just curious.

I think you want WTF_ARRAY_SIZE, lets not use base yet.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:13: bool HitTestCache::check(HitTestResult&
hitResult)
verify() I think is better than check.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:57: void HitTestCache::cacheValue(const
HitTestResult& result)
addCacheValue, in blink cacheValue() is a getter (I know cache is also a verb,
but  it's confusing here)

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:47: bool m_valid = false;
no m_ prefix on public fields.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:57: CacheItem m_items[2];
We're probably going to need to hook this up to memory pressure notifications,
you're keeping DOM trees alive inside the CacheItems.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestResult.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestResult.h:67: void cacheValues(const HitTestResult&);
On 2015/06/05 at 20:48:59, Rick Byers wrote:
> please add a brief comment describing when cacheValues is intended to be used
(not immediately obvious to me having not yet seen the callers).

addCachedResult() I think is what you want.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:1803: view()->clearHitTestCache();
On 2015/06/05 at 20:48:59, Rick Byers wrote:
> I don't know enough about the style system to say for sure that this is the
right place.  Hopefully esprehn will chime in, but if not and manual testing
isn't showing any false cache hits, then go ahead and land it and we'll see what
we get from UMA :-)

You don't want to be here, this puts code inside setStyle() for every element on
the page, you want to be in Document::updateLayoutTree.

[Rick Byers, 2015-06-07 04:34:46]

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:57: CacheItem m_items[2];
On 2015/06/06 21:14:30, esprehn wrote:
> We're probably going to need to hook this up to memory pressure notifications,
> you're keeping DOM trees alive inside the CacheItems.

I was concerned about this at first too, but thought it was unlikely to be a
real problem because the only way we could hold the only ref here is if it was
just removed from a document, and so we should be just about to flush the cache.
 Right?  Can you think of a real scenario where we could actually prolong
lifetime non-trivially?

[Rick Byers, 2015-06-07 04:44:57]

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:9: #include <base/macros.h>
On 2015/06/06 21:14:30, esprehn wrote:
> On 2015/06/06 at 16:12:54, Rick Byers wrote:
> > What do you need this for?  It's still very rare for blink to depend on base
> (outside of tests).  Not sure if it really matters, just curious.
> 
> I think you want WTF_ARRAY_SIZE, lets not use base yet.

Note that there is at least one non-test use of base in blink (in
WebPluginContainerImpl.cpp).  If this matters, maybe we need to enforce it, or
at least mention it in the style guide?

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:13: bool HitTestCache::check(HitTestResult&
hitResult)
On 2015/06/06 21:14:29, esprehn wrote:
> verify() I think is better than check.

Verify is below.  Maybe call this lookup?

[dtapuska, 2015-06-09 18:21:21]

dtapuska@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[dtapuska, 2015-06-09 18:21:25]

Adding oil-pan reviews to comment on the use of the HitTestCache object off of
the LayoutView.

Ideally if the LayoutTree was gc'd then I'd trace the HitTestCache and move the
Node members on the HitTestResults to be WeakMembers. But the LayoutTree isn't
GC'd yet; so I'm just adding them to indicate whether what I've done here is
sane as esprehn wanted the HitTestResult's members to be WeakMembers.

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
File LayoutTests/fast/events/hit-test-cache.html (right):

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache.html:73: debug('Hit test fixed div after
scroll');
On 2015/06/05 20:48:58, Rick Byers wrote:
> please add at least one simple test case where the cache does get hit (just to
> verify that the cache is working at all in this test setup).

Done.

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache.html:80: 
On 2015/06/05 20:48:59, Rick Byers wrote:
> nit: debug('') here to make the output more readable

Done.

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-cache.html:85: reportElementAt(12, 12);
On 2015/06/05 20:48:58, Rick Byers wrote:
> I'd prefer if the test code indicated what the expected behavior was (eg. with
> shouldBe, etc.).  I know it's captured by the expected output, but I've seen
> scenarios where people mis-understand the test intentions and update the
> expected output and then future readers have no idea that the test is actually
> broken and expecting failure.

Done.

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
File LayoutTests/fast/events/hit-test-counts-expected.txt (right):

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-counts-expected.txt:13: MouseDown: 3/2 [with
multiTargetTapNotification: 3/1]
On 2015/06/05 20:48:59, Rick Byers wrote:
> multiTargetTapNotification really shouldn't be changing the results for
non-tap
> scenarios.  Presumably this is a side-effect of the state of the cache, right?

> Perhaps you should be flushing your cache before doing the
> multiTargetTapNotification version to avoid such confusing side effects?

Done.

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
File LayoutTests/fast/events/hit-test-counts.html (right):

https://codereview.chromium.org/1142283004/diff/60001/LayoutTests/fast/events...
LayoutTests/fast/events/hit-test-counts.html:63: countStr += ' ' +
hitTestCountDelta(documents[i]) + "/" + hitTestCacheHitsDelta(documents[i]);
On 2015/06/05 20:48:59, Rick Byers wrote:
> nit: I'd find this more readable if it was "<misses>+<hits>" instead of
> "total/hits".  It's really the misses number I care most about in this test -
> would be nice to be able to scan without doing the math for each result.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1230: HitTestResult result =
hitTestInDocument(this, x, y, true);
On 2015/06/05 20:48:59, Rick Byers wrote:
> This pattern is generally frowned upon in Google C++, see the true/false part
of
> http://google.github.io/styleguide/cppguide.html#Implementation_Comments
> 
> If skipping the cache is unusual, then I'd suggest having a
> hitTestInDocumentNoCache method and leaving the 3 arg method alone. 
Alternately
> you could use an enum instead of a bool or (usually lowest preference) add a
> comment at each call site saying what the true/false means.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:1230: HitTestResult result =
hitTestInDocument(this, x, y, true);
On 2015/06/06 21:14:29, esprehn wrote:
> Yes, please no bools like this.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:277: Element* TreeScope::elementFromPoint(int x,
int y) const
On 2015/06/06 21:14:29, esprehn wrote:
> elementFromPoint(int x, int y, HitTestCacheType = UseHitTestCache)

I'm just going to allow the HitTestRequest to be passed in... Seems better than
defining another enum; since this is all internal to this class.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.cpp:282: Element*
TreeScope::elementFromPointNoCache(int x, int y) const
On 2015/06/06 21:14:29, esprehn wrote:
> Use default args instead, no need for two methods.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/dom/TreeSco...
Source/core/dom/TreeScope.h:181: Element* hitTestPoint(int x, int y, bool
useCache) const;
On 2015/06/06 21:14:29, esprehn wrote:
> Can we use an IntPoint?

We could; but I think this would be overkill. Since the implementation of this
method does in fact create a IntPoint it would make sense at first; but the
implementation actually modifies the IntPoint; so we'd still need to make a copy
of the IntPoint that was passed in anyways. So passing it intermediately as a
IntPoint just seems like overkill. Let me know if you think otherwise.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:9: #include <base/macros.h>
On 2015/06/06 21:14:30, esprehn wrote:
> On 2015/06/06 at 16:12:54, Rick Byers wrote:
> > What do you need this for?  It's still very rare for blink to depend on base
> (outside of tests).  Not sure if it really matters, just curious.
> 
> I think you want WTF_ARRAY_SIZE, lets not use base yet.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:9: #include <base/macros.h>
On 2015/06/06 16:12:54, Rick Byers wrote:
> What do you need this for?  It's still very rare for blink to depend on base
> (outside of tests).  Not sure if it really matters, just curious.

it was used for arraysize(...) macro; esprehn pointed out there is
WTF_ARRAY_LENGTH.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:9: #include <base/macros.h>
On 2015/06/06 21:14:30, esprehn wrote:
> On 2015/06/06 at 16:12:54, Rick Byers wrote:
> > What do you need this for?  It's still very rare for blink to depend on base
> (outside of tests).  Not sure if it really matters, just curious.
> 
> I think you want WTF_ARRAY_SIZE, lets not use base yet.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:9: #include <base/macros.h>
On 2015/06/07 04:44:57, Rick Byers wrote:
> On 2015/06/06 21:14:30, esprehn wrote:
> > On 2015/06/06 at 16:12:54, Rick Byers wrote:
> > > What do you need this for?  It's still very rare for blink to depend on
base
> > (outside of tests).  Not sure if it really matters, just curious.
> > 
> > I think you want WTF_ARRAY_SIZE, lets not use base yet.
> 
> Note that there is at least one non-test use of base in blink (in
> WebPluginContainerImpl.cpp).  If this matters, maybe we need to enforce it, or
> at least mention it in the style guide?

Acknowledged.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:13: bool HitTestCache::check(HitTestResult&
hitResult)
On 2015/06/06 21:14:29, esprehn wrote:
> verify() I think is better than check.

Acknowledged.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:13: bool HitTestCache::check(HitTestResult&
hitResult)
On 2015/06/07 04:44:57, Rick Byers wrote:
> On 2015/06/06 21:14:29, esprehn wrote:
> > verify() I think is better than check.
> 
> Verify is below.  Maybe call this lookup?

Changed to lookupCacheValue... to model after the addCacheValue method name;
esprehn requested below.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:21: for (size_t i = 0; m_items[i].m_valid &&
i < arraysize(m_items); ++i) {
On 2015/06/06 16:12:54, Rick Byers wrote:
> i < arraysize(m_items) should come before the m_valid check to avoid reading
> past the end of m_items.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.cpp:57: void HitTestCache::cacheValue(const
HitTestResult& result)
On 2015/06/06 21:14:30, esprehn wrote:
> addCacheValue, in blink cacheValue() is a getter (I know cache is also a verb,
> but  it's confusing here)

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:12: class HitTestCache {
On 2015/06/05 20:48:59, Rick Byers wrote:
> Please add a brief comment describing the overall intention here (maybe making
> specific reference to the issues like GestureTap where normally the target
> doesn't change but occasionally it needs to - or just link to your doc for
> details).

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:18: bool check(HitTestResult& /*result*/);
On 2015/06/06 16:12:54, Rick Byers wrote:
> nit: no need for /*result*/

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:21: static void verify(const HitTestResult&,
const HitTestResult&);
On 2015/06/06 16:12:54, Rick Byers wrote:
> Since the order of the arguments are important (at least to the metric
> generated), please include the argument names here since they do communicate
> some extra value.  See #9 at
> https://www.chromium.org/blink/coding-style#TOC-Names for a good rule of
thumb.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:21: static void verify(const HitTestResult&,
const HitTestResult&);
On 2015/06/05 20:48:59, Rick Byers wrote:
> the other two methods seem self explanatory here, but this one doesn't
> necessarily - please add a comment.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:29: MISS_VALIDITY_RECT_MATCHES, // Region
matches by request type doesn't.
On 2015/06/05 20:48:59, Rick Byers wrote:
> s/by/but/

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:47: bool m_valid = false;
On 2015/06/06 21:14:30, esprehn wrote:
> no m_ prefix on public fields.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:56: // size of 1.
On 2015/06/05 20:48:59, Rick Byers wrote:
> Do you think such ping-ponging of flags that affect the hit test could be
fixed?
>  Seems like it probably shouldn't happen? Maybe file a separate bug to track
> that.

I don't think the ping pong can be changed. I believe there is a penetration
testing; looking inside iframes on the hover happens in between the processing.
We might be able to move it to the end of the flow; but then you'd loose the
cache if there was a similar event next with slightly different flags.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:57: CacheItem m_items[2];
On 2015/06/06 21:14:30, esprehn wrote:
> We're probably going to need to hook this up to memory pressure notifications,
> you're keeping DOM trees alive inside the CacheItems.

The refs are explicitly purged on clear; I wondered about this as well as
tracing with oilpan..

But the fields should be ref'd elsewhere; and really this is just a weak cache.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestCache.h:57: CacheItem m_items[2];
On 2015/06/07 04:34:46, Rick Byers wrote:
> On 2015/06/06 21:14:30, esprehn wrote:
> > We're probably going to need to hook this up to memory pressure
notifications,
> > you're keeping DOM trees alive inside the CacheItems.
> 
> I was concerned about this at first too, but thought it was unlikely to be a
> real problem because the only way we could hold the only ref here is if it was
> just removed from a document, and so we should be just about to flush the
cache.
>  Right?  Can you think of a real scenario where we could actually prolong
> lifetime non-trivially?

I couldn't think of any; since the fields are cleared on dom modifications.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestRequest.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestRequest.h:80: static const HitTestRequestType
CacheabilityBits = ReadOnly | Active | Move | Release | TouchEvent;
On 2015/06/06 16:12:54, Rick Byers wrote:
> this probably deserves a comment saying these bits don't affect the result of
> hit tests themselves (and a TODO saying they should be moved out of the
enum?).

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
File Source/core/layout/HitTestResult.h (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestResult.h:67: void cacheValues(const HitTestResult&);
On 2015/06/06 21:14:30, esprehn wrote:
> On 2015/06/05 at 20:48:59, Rick Byers wrote:
> > please add a brief comment describing when cacheValues is intended to be
used
> (not immediately obvious to me having not yet seen the callers).
> 
> addCachedResult() I think is what you want.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/HitT...
Source/core/layout/HitTestResult.h:67: void cacheValues(const HitTestResult&);
On 2015/06/05 20:48:59, Rick Byers wrote:
> please add a brief comment describing when cacheValues is intended to be used
> (not immediately obvious to me having not yet seen the callers).

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:1803: view()->clearHitTestCache();
On 2015/06/06 21:14:30, esprehn wrote:
> On 2015/06/05 at 20:48:59, Rick Byers wrote:
> > I don't know enough about the style system to say for sure that this is the
> right place.  Hopefully esprehn will chime in, but if not and manual testing
> isn't showing any false cache hits, then go ahead and land it and we'll see
what
> we get from UMA :-)
> 
> You don't want to be here, this puts code inside setStyle() for every element
on
> the page, you want to be in Document::updateLayoutTree.

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutObject.cpp:1803: view()->clearHitTestCache();
On 2015/06/05 20:48:59, Rick Byers wrote:
> I don't know enough about the style system to say for sure that this is the
> right place.  Hopefully esprehn will chime in, but if not and manual testing
> isn't showing any false cache hits, then go ahead and land it and we'll see
what
> we get from UMA :-)

Done.

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
File Source/core/layout/LayoutView.cpp (right):

https://codereview.chromium.org/1142283004/diff/60001/Source/core/layout/Layo...
Source/core/layout/LayoutView.cpp:130: bool cacheHit = cacheHit =
m_hitTestCache.check(cacheResult);
On 2015/06/05 20:48:59, Rick Byers wrote:
> remove extra cacheHit

Done.

[sof, 2015-06-09 18:59:18]

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Lay...
File Source/core/layout/LayoutView.h (right):

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutView.h:219: HitTestCache m_hitTestCache;
Oilpan-wise, to make this work on LayoutView, it needs to be handled&represented
like PendingSelection below. i.e., HitTestCache needs to be on the heap. (If
LayoutView had been on the heap itself, keeping it as a part object would have
been fine.)

[dtapuska, 2015-06-09 19:39:15]

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Lay...
File Source/core/layout/LayoutView.h (right):

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Lay...
Source/core/layout/LayoutView.h:219: HitTestCache m_hitTestCache;
On 2015/06/09 18:59:18, sof wrote:
> Oilpan-wise, to make this work on LayoutView, it needs to be
handled&represented
> like PendingSelection below. i.e., HitTestCache needs to be on the heap. (If
> LayoutView had been on the heap itself, keeping it as a part object would have
> been fine.)

In the posted version:
The pointers inside the HitTestResult are in fact raw ptrs which are really weak
because and are checked based on the dom tree version; if it doesn't match they
aren't used. So stale pointers won't get used.

Does turning HitTestCache into a GarbageCollected object imply that I need to
turn HitTestResult into a GarbageCollected object as well; that way I could
trace them and have the WeakMembers inside that class? 

If HitTestResult is created on the stack in a few places.. does it imply that I
need to turn all those stack allocations into heap allocations too?

[sof, 2015-06-09 20:17:46]

On 2015/06/09 19:39:15, Dave Tapuska wrote:
>
https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Lay...
> File Source/core/layout/LayoutView.h (right):
> 
>
https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Lay...
> Source/core/layout/LayoutView.h:219: HitTestCache m_hitTestCache;
> On 2015/06/09 18:59:18, sof wrote:
> > Oilpan-wise, to make this work on LayoutView, it needs to be
> handled&represented
> > like PendingSelection below. i.e., HitTestCache needs to be on the heap. (If
> > LayoutView had been on the heap itself, keeping it as a part object would
have
> > been fine.)
> 
> In the posted version:
> The pointers inside the HitTestResult are in fact raw ptrs which are really
weak
> because and are checked based on the dom tree version; if it doesn't match
they
> aren't used. So stale pointers won't get used.
> 

If you wanted to you could process these weak references manually with a weak
callback that checks the cached version number against the dom tree version. And
clears them out on a mismatch -- would that be the weak lifetime you're after?

> Does turning HitTestCache into a GarbageCollected object imply that I need to
> turn HitTestResult into a GarbageCollected object as well; that way I could
> trace them and have the WeakMembers inside that class? 
> 

It would be slightly unusual to have a stack allocated HitTestResult containing
WeakMembers, but it should work just fine. But if you use a weak callback for
HitTestCache, they can remain Member<>s.

> If HitTestResult is created on the stack in a few places.. does it imply that
I
> need to turn all those stack allocations into heap allocations too?

HitTestResult can still be stack allocated; it stays unchanged (% weak members
or not.)

[dtapuska, 2015-06-09 20:45:44]

> If you wanted to you could process these weak references manually with a weak
> callback that checks the cached version number against the dom tree version.
And
> clears them out on a mismatch -- would that be the weak lifetime you're after?

I don't think anything is wrong with the current code am just wondering what the
preferred implementation should be.

I hold onto a raw Node* matched with a dom_version. If when I get a call to use
the Node* I only return it if the
dom_version matches; otherwise I toss the cache. I want to do this lazily; ie
every dom manipulation I don't want
to clear my cache; it's not necessary.. Just lazily hold onto raw ptrs; and that
is what I am doing.

I believe esprehn had concerns that this would trip up some heuristics and
tooling with oilpan; but these
objects aren't even traced so I didn't think so. He suggested WeakMembers; but I
didn't think it bought me
anything as I was testing the dom version already; and if the dom version is the
same then it means the Node* is
still alive.

[Rick Byers, 2015-06-09 21:08:21]

This LGTM from my perspective now (modulo wrapping up the outstanding
questions/issues with esprehn/sof of course).

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:12: // This object implements a cache for
storing successfuly hit tests to DOM nodes
nit: successful

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:25: //
Nice comment, thank you!

[dtapuska, 2015-06-09 21:13:01]

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/100001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:12: // This object implements a cache for
storing successfuly hit tests to DOM nodes
On 2015/06/09 21:08:20, Rick Byers wrote:
> nit: successful

Done.

[sof, 2015-06-09 21:32:55]

On 2015/06/09 20:45:44, Dave Tapuska wrote:
> > If you wanted to you could process these weak references manually with a
weak
> > callback that checks the cached version number against the dom tree version.
> And
> > clears them out on a mismatch -- would that be the weak lifetime you're
after?
> 
> I don't think anything is wrong with the current code am just wondering what
the
> preferred implementation should be.
> 

Does it need to be better/more precise? If it is sufficient to clear the cache
by the first/next lookup operation that hits a version mismatch, then the above
isn't needed. But if you want to force a version check during GCs, then the
above suggestion is one way to do it.

[tdresser, 2015-06-10 13:35:22]

https://codereview.chromium.org/1142283004/diff/120001/Source/core/dom/TreeSc...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/1142283004/diff/120001/Source/core/dom/TreeSc...
Source/core/dom/TreeScope.h:214: HitTestResult hitTestInDocument(const
Document*, int x, int y, const HitTestRequest& = HitTestRequest::ReadOnly |
HitTestRequest::Active);
Why take a reference instead of a value here?

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.cpp (right):

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.cpp:48: // ASSERT that the cache hit is the same
as the actual result
nit: period at end of comment.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.cpp:49: ASSERT(false);
Use ASSERT_NOT_REACHED().

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:36: // Verify that the |actual| object matches
the |exepcted| object; otherwise
exepcted -> expected

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:37: // log UMA metrics indicating failure.
This is a bit misleading, in that we log UMA metrics either way.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:44: // These values are reported in UMA. Try
not to reorder/modify.
Don't you need a few more constraints here (and below)?
Shouldn't this be kept in sync with histograms.xml?
Also, it wouldn't hurt to mention that new values should be added to the end.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:74: // Mouse/Touch/Pointer events can generate
incrased cache misses with
incrased -> increased

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:76: CacheItem m_items[2];
Once this is in, it would be interesting to run an experiment to tune this. A
finch trial where we bumped this up to 3/4 could generate some interesting data.

[dtapuska, 2015-06-10 14:04:49]

https://codereview.chromium.org/1142283004/diff/120001/Source/core/dom/TreeSc...
File Source/core/dom/TreeScope.h (right):

https://codereview.chromium.org/1142283004/diff/120001/Source/core/dom/TreeSc...
Source/core/dom/TreeScope.h:214: HitTestResult hitTestInDocument(const
Document*, int x, int y, const HitTestRequest& = HitTestRequest::ReadOnly |
HitTestRequest::Active);
On 2015/06/10 13:35:21, tdresser wrote:
> Why take a reference instead of a value here?

HitTestRequest is a class; so I prefer to pass classes by const reference as I
don't want to imply the sizing of the class itself and what the compiler will
do.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.cpp (right):

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.cpp:48: // ASSERT that the cache hit is the same
as the actual result
On 2015/06/10 13:35:21, tdresser wrote:
> nit: period at end of comment.

Done.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.cpp:49: ASSERT(false);
On 2015/06/10 13:35:21, tdresser wrote:
> Use ASSERT_NOT_REACHED().

Done.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:36: // Verify that the |actual| object matches
the |exepcted| object; otherwise
On 2015/06/10 13:35:21, tdresser wrote:
> exepcted -> expected

Done.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:37: // log UMA metrics indicating failure.
On 2015/06/10 13:35:21, tdresser wrote:
> This is a bit misleading, in that we log UMA metrics either way.

Done.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:44: // These values are reported in UMA. Try
not to reorder/modify.
On 2015/06/10 13:35:21, tdresser wrote:
> Don't you need a few more constraints here (and below)?
> Shouldn't this be kept in sync with histograms.xml?
> Also, it wouldn't hurt to mention that new values should be added to the end.

Done.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:74: // Mouse/Touch/Pointer events can generate
incrased cache misses with
On 2015/06/10 13:35:21, tdresser wrote:
> incrased -> increased

Done.

https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:76: CacheItem m_items[2];
On 2015/06/10 13:35:21, tdresser wrote:
> Once this is in, it would be interesting to run an experiment to tune this. A
> finch trial where we bumped this up to 3/4 could generate some interesting
data.

Acknowledged.

[tdresser, 2015-06-10 14:11:04]

On 2015/06/10 14:04:49, Dave Tapuska wrote:
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/dom/TreeSc...
> File Source/core/dom/TreeScope.h (right):
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/dom/TreeSc...
> Source/core/dom/TreeScope.h:214: HitTestResult hitTestInDocument(const
> Document*, int x, int y, const HitTestRequest& = HitTestRequest::ReadOnly |
> HitTestRequest::Active);
> On 2015/06/10 13:35:21, tdresser wrote:
> > Why take a reference instead of a value here?
> 
> HitTestRequest is a class; so I prefer to pass classes by const reference as I
> don't want to imply the sizing of the class itself and what the compiler will
> do.
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> File Source/core/layout/HitTestCache.cpp (right):
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.cpp:48: // ASSERT that the cache hit is the
same
> as the actual result
> On 2015/06/10 13:35:21, tdresser wrote:
> > nit: period at end of comment.
> 
> Done.
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.cpp:49: ASSERT(false);
> On 2015/06/10 13:35:21, tdresser wrote:
> > Use ASSERT_NOT_REACHED().
> 
> Done.
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> File Source/core/layout/HitTestCache.h (right):
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.h:36: // Verify that the |actual| object
matches
> the |exepcted| object; otherwise
> On 2015/06/10 13:35:21, tdresser wrote:
> > exepcted -> expected
> 
> Done.
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.h:37: // log UMA metrics indicating failure.
> On 2015/06/10 13:35:21, tdresser wrote:
> > This is a bit misleading, in that we log UMA metrics either way.
> 
> Done.
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.h:44: // These values are reported in UMA. Try
> not to reorder/modify.
> On 2015/06/10 13:35:21, tdresser wrote:
> > Don't you need a few more constraints here (and below)?
> > Shouldn't this be kept in sync with histograms.xml?
> > Also, it wouldn't hurt to mention that new values should be added to the
end.
> 
> Done.
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.h:74: // Mouse/Touch/Pointer events can
generate
> incrased cache misses with
> On 2015/06/10 13:35:21, tdresser wrote:
> > incrased -> increased
> 
> Done.
> 
>
https://codereview.chromium.org/1142283004/diff/120001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.h:76: CacheItem m_items[2];
> On 2015/06/10 13:35:21, tdresser wrote:
> > Once this is in, it would be interesting to run an experiment to tune this.
A
> > finch trial where we bumped this up to 3/4 could generate some interesting
> data.
> 
> Acknowledged.

LGTM % others comments.

[dtapuska, 2015-06-10 21:08:19]

On 2015/06/09 21:32:55, sof wrote:
> On 2015/06/09 20:45:44, Dave Tapuska wrote:
> > > If you wanted to you could process these weak references manually with a
> weak
> > > callback that checks the cached version number against the dom tree
version.
> > And
> > > clears them out on a mismatch -- would that be the weak lifetime you're
> after?
> > 
> > I don't think anything is wrong with the current code am just wondering what
> the
> > preferred implementation should be.
> > 
> 
> Does it need to be better/more precise? If it is sufficient to clear the cache
> by the first/next lookup operation that hits a version mismatch, then the
above
> isn't needed. But if you want to force a version check during GCs, then the
> above suggestion is one way to do it.

No it doesn't need to be better/more precise. Its sufficient to clear it on the
next lookup...
So it sounds like what I am doing is fine.

[sof, 2015-06-10 21:43:05]

On 2015/06/10 21:08:19, Dave Tapuska wrote:
> On 2015/06/09 21:32:55, sof wrote:
> > On 2015/06/09 20:45:44, Dave Tapuska wrote:
> > > > If you wanted to you could process these weak references manually with a
> > weak
> > > > callback that checks the cached version number against the dom tree
> version.
> > > And
> > > > clears them out on a mismatch -- would that be the weak lifetime you're
> > after?
> > > 
> > > I don't think anything is wrong with the current code am just wondering
what
> > the
> > > preferred implementation should be.
> > > 
> > 
> > Does it need to be better/more precise? If it is sufficient to clear the
cache
> > by the first/next lookup operation that hits a version mismatch, then the
> above
> > isn't needed. But if you want to force a version check during GCs, then the
> > above suggestion is one way to do it.
> 
> No it doesn't need to be better/more precise. Its sufficient to clear it on
the
> next lookup...
> So it sounds like what I am doing is fine.

ok, under that assumption, then yes - no need to introduce WeakMember. But
HitTestCache needs to move to the heap, so that LayoutView can keep a Persistent
ref to it.

It's up to you if you want to address this here or not -- Oilpan breakages are
(still) oilpan-reviews' responsibility to fix.

[esprehn, 2015-06-12 05:09:30]

Can you explain how the rect is computed? It seems right now you just create a
rect (x, y, width: 1, height: 1) but how does that work if I'm at the very edge
of a box and move one pixel? Also why is the cache just down and to the right of
the current position?

https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.cpp (right):

https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.cpp:78: for (size_t i = 0; i <
WTF_ARRAY_LENGTH(m_items); ++i) {
We could use a Vector with inline capacity of 2 for m_items instead. Then you
can use range loops and this method can just be m_items.clear()

https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:24: //
https://docs.google.com/document/d/1b0NYAD4S9BJIpHGa4JD2HLmW28f2rUh1jlqrgpU3zVU/
Comments! You're a hero

https://codereview.chromium.org/1142283004/diff/140001/public/platform/WebLay...
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/1142283004/diff/140001/public/platform/WebLay...
public/platform/WebLayer.h:236: virtual void setHitCacheRect(const WebRect&) {
};
I'd probably call this HitTestCache since that's also what the class is called.

[dtapuska, 2015-06-12 13:09:42]

On 2015/06/12 05:09:30, esprehn wrote:
> Can you explain how the rect is computed? It seems right now you just create a
> rect (x, y, width: 1, height: 1) but how does that work if I'm at the very
edge
> of a box and move one pixel? Also why is the cache just down and to the right
of
> the current position?
> 
>
https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
> File Source/core/layout/HitTestCache.cpp (right):
> 
>
https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.cpp:78: for (size_t i = 0; i <
> WTF_ARRAY_LENGTH(m_items); ++i) {
> We could use a Vector with inline capacity of 2 for m_items instead. Then you
> can use range loops and this method can just be m_items.clear()
> 
>
https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
> File Source/core/layout/HitTestCache.h (right):
> 
>
https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
> Source/core/layout/HitTestCache.h:24: //
>
https://docs.google.com/document/d/1b0NYAD4S9BJIpHGa4JD2HLmW28f2rUh1jlqrgpU3zVU/
> Comments! You're a hero
> 
>
https://codereview.chromium.org/1142283004/diff/140001/public/platform/WebLay...
> File public/platform/WebLayer.h (right):
> 
>
https://codereview.chromium.org/1142283004/diff/140001/public/platform/WebLay...
> public/platform/WebLayer.h:236: virtual void setHitCacheRect(const WebRect&) {
> };
> I'd probably call this HitTestCache since that's also what the class is
called.

As described in the change list the validity rect is as you say x, y, x +
epsilon, y + epsilon. And contains(...) is rect is inclusive on the begin point
and exclusive on the max points.  So that means it is a direct hit (exact
point).

The motivation behind this was to break out the work I did to have a larger
validity region but only commit what we are confident in and run it in test mode
for a few weeks and collect uma metrics. 

Once we know that single point is 100% correct we will just enable returning the
cached node by default and perhaps occasionally do the validation (which means
to actually do the hit test and compare it against what the cached result was).
This will get the main case that we are after; as you can see in the test
results; the GestureTap gets a good hit rate... 1+3 (or 1+4) (So 3 out of 4 or 4
out of 5 hit tests are in fact hit with the cache). Ideally I'd like to get this
into trunk as soon as possible; collect some uma metrics; and then just enable
the fast path once we have a reasonable set of data. 

Once that is in hand; we can then start trials/reviews with the larger validity
rect that will build on the code that is here. Make sense?

[esprehn, 2015-06-12 15:12:49]

That makes sense, can you explain in your change description that this just does
exact match for now?

Lgtm w/ nits fixed

[dtapuska, 2015-06-12 15:30:38]

https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.cpp (right):

https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.cpp:78: for (size_t i = 0; i <
WTF_ARRAY_LENGTH(m_items); ++i) {
On 2015/06/12 05:09:29, esprehn wrote:
> We could use a Vector with inline capacity of 2 for m_items instead. Then you
> can use range loops and this method can just be m_items.clear()

Done.

https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
File Source/core/layout/HitTestCache.h (right):

https://codereview.chromium.org/1142283004/diff/140001/Source/core/layout/Hit...
Source/core/layout/HitTestCache.h:24: //
https://docs.google.com/document/d/1b0NYAD4S9BJIpHGa4JD2HLmW28f2rUh1jlqrgpU3zVU/
On 2015/06/12 05:09:30, esprehn wrote:
> Comments! You're a hero

Acknowledged.

https://codereview.chromium.org/1142283004/diff/140001/public/platform/WebLay...
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/1142283004/diff/140001/public/platform/WebLay...
public/platform/WebLayer.h:236: virtual void setHitCacheRect(const WebRect&) {
};
On 2015/06/12 05:09:30, esprehn wrote:
> I'd probably call this HitTestCache since that's also what the class is
called.

Done.

[dtapuska, 2015-06-12 16:43:10]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-06-12 16:43:10]

The patchset sent to the CQ was uploaded after l-g-t-m from rbyers@chromium.org,
esprehn@chromium.org, tdresser@chromium.org
Link to the patchset: https://codereview.chromium.org/1142283004/#ps160001
(title: "Use HeapVector with inline capacity")

[commit-bot: I haz the power, 2015-06-12 16:43:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142283004/160001

[commit-bot: I haz the power, 2015-06-12 16:47:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-12 16:47:09]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/58741)

[dtapuska, 2015-06-12 17:58:09]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-06-12 17:58:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, tdresser@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1142283004/#ps180001
(title: "Rebase against ToT")

[commit-bot: I haz the power, 2015-06-12 17:58:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142283004/180001

[commit-bot: I haz the power, 2015-06-12 18:09:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-12 18:09:28]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[dtapuska, 2015-06-12 19:58:51]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-06-12 19:58:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, tdresser@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1142283004/#ps200001
(title: "Seems verify is a macro on MacOSX; change name.")

[commit-bot: I haz the power, 2015-06-12 19:59:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142283004/200001

[commit-bot: I haz the power, 2015-06-12 21:56:17]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197063

[esprehn, 2015-06-16 22:55:23]

lgtm

[dtapuska, 2015-06-17 00:22:54]

The CQ bit was checked by dtapuska@chromium.org

[dtapuska, 2015-06-17 00:22:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdresser@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1142283004/#ps260001
(title: "Remove validity rect as per Elliott's request")

[commit-bot: I haz the power, 2015-06-17 00:23:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1142283004/260001

[commit-bot: I haz the power, 2015-06-17 03:05:50]

Committed patchset #14 (id:260001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197233