Bug 92367 - Move region from HitTestResult to HitTestPoint
Summary: Move region from HitTestResult to HitTestPoint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-26 04:42 PDT by Allan Sandfeld Jensen
Modified: 2012-07-26 09:15 PDT (History)
3 users (show)

See Also:


Attachments
Patch (16.42 KB, patch)
2012-07-26 04:45 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Allan Sandfeld Jensen 2012-07-26 04:42:00 PDT
HitTestResult currently keeps track of a region in which the current hit-test is being performed, but the region is not really specific to the result, and is more closely tied to the current point being tested, therefore it makes more sense to store the region in the HitTestPoint instead of the HitTestResult.

The also makes it possible to get rid of setRegion() since it was only used to override and restore the original region in HitTestResult, since a new HitTestPoint is made when descending into a new region, this save/restore mechanic is no longer necessary, and region can be made a constant in HitTestPoint.
Comment 1 Allan Sandfeld Jensen 2012-07-26 04:45:58 PDT
Created attachment 154610 [details]
Patch
Comment 2 Antonio Gomes 2012-07-26 07:33:06 PDT
Comment on attachment 154610 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154610&action=review

r=me

> Source/WebCore/rendering/HitTestResult.cpp:107
> +HitTestPoint::HitTestPoint(const HitTestPoint& other, const LayoutSize& offset, RenderRegion* region)
> +    : m_point(other.m_point)
> +    , m_boundingBox(other.m_boundingBox)
> +    , m_transformedPoint(other.m_transformedPoint)
> +    , m_transformedRect(other.m_transformedRect)
> +    , m_region(region)

I wonder who owns this point. Can it be deleted elsewhere, and we get garbage here?
Comment 3 Antonio Gomes 2012-07-26 07:34:00 PDT
> 
> I wonder who owns this point. Can it be deleted elsewhere, and we get garbage here?

s/point/pointer/g
Comment 4 Allan Sandfeld Jensen 2012-07-26 08:22:30 PDT
(In reply to comment #2)
> (From update of attachment 154610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154610&action=review
> 
> r=me
> 
> > Source/WebCore/rendering/HitTestResult.cpp:107
> > +HitTestPoint::HitTestPoint(const HitTestPoint& other, const LayoutSize& offset, RenderRegion* region)
> > +    : m_point(other.m_point)
> > +    , m_boundingBox(other.m_boundingBox)
> > +    , m_transformedPoint(other.m_transformedPoint)
> > +    , m_transformedRect(other.m_transformedRect)
> > +    , m_region(region)
> 
> I wonder who owns this point. Can it be deleted elsewhere, and we get garbage here?

It is owned by whoever created it, in this case in it just exists on the stack where it is created in renderflowthread, and is deleted automatically when that function returns.
Comment 5 WebKit Review Bot 2012-07-26 09:15:51 PDT
Comment on attachment 154610 [details]
Patch

Clearing flags on attachment: 154610

Committed r123754: <http://trac.webkit.org/changeset/123754>
Comment 6 WebKit Review Bot 2012-07-26 09:15:55 PDT
All reviewed patches have been landed.  Closing bug.