Bug 174233 – Add location to NavigationActionData

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Megan Gardner]

Add location to NavigationActionData

[Megan Gardner]

Created attachment 314792 [details]
Patch

[Wenson Hsieh]

Comment on attachment 314792 [details]
Patch

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

I think it shouldn't be too difficult to write a unit test for this, as well.

> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationAction.mm:128
> +    return [NSString stringWithFormat:@"<%@: %p; navigationType = %ld; syntheticClickType = %ld; position x = %ld y= %ld request = %@; sourceFrame = %@; targetFrame = %@>", NSStringFromClass(self.class), self,

Perhaps just position x = %.2f y = %.2f, instead of casting the floats to long

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:72
> +static WebCore::FloatPoint clickPointForMouseEvent(const MouseEvent* mouseEvent)

Since we're using namespace WebCore, you can omit the WebCore:: here.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:77
> +    return WebCore::FloatPoint(mouseEvent->screenX(), mouseEvent->screenY());

Are screenX() and screenY() in root view coordinates? IMO, we should make sure this point's coordinate space matches up with what -_requestActivatedElementAtPosition:completionBlock: accepts (which appears to be in root view coordinates). Or if not, then give the variables here more descriptive names so that it's clear what coordinate space everything is in.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:108
> +WebCore::FloatPoint InjectedBundleNavigationAction::clickPointForNavigationAction(const NavigationAction& navigationAction)

You can omit the WebCore:: here too.

[Simon Fraser (smfr)]

Comment on attachment 314792 [details]
Patch

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

> Source/WebKit2/Shared/NavigationActionData.h:52
> +    WebCore::FloatPoint clickPoint;

Would be good if the name said what coordinate space this point lives in. Also, we'd normally call this a "location".

> Source/WebKit2/UIProcess/API/APINavigationAction.h:66
> +    WebCore::FloatPoint clickPoint() const { return m_navigationActionData.clickPoint; }

Ditto.

> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationActionPrivate.h:54
> +@property (nonatomic, readonly) CGPoint _clickPoint WK_API_AVAILABLE(ios(11.0));

Ditto.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:75
> +        return WebCore::FloatPoint::zero();

This can just be "return { };"

[Megan Gardner]

Created attachment 314864 [details]
Patch

[Tim Horton]

Comment on attachment 314864 [details]
Patch

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

> Source/WebKit2/Shared/NavigationActionData.h:52
> +    WebCore::FloatPoint rootViewClickLocation;

I think WK2 convention is generally xInYCoordinates (clickLocationInRootViewCoordinates). The coordinate space info is secondary, shouldn’t come first

[Megan Gardner]

Created attachment 314871 [details]
Patch

[Wenson Hsieh]

Comment on attachment 314871 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationAction.mm:132
> +        0L, 0L, 0L,

This probably needs to be 0 instead of 0L to fix the Mac build.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:72
> +static WebCore::FloatPoint clickLocationInRootViewCoordinatesForMouseEvent(const MouseEvent* mouseEvent)

No need for WebCore:: here.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:76
> +    

I noticed that neighboring methods check if (!mouseEvent->buttonDown() || !mouseEvent->isTrusted()). Do we need this logic here too?

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:77
> +    return WebCore::FloatPoint(mouseEvent->clientX(), mouseEvent->clientY());

Ditto.

[Megan Gardner]

Created attachment 314884 [details]
Patch

[Wenson Hsieh]

Comment on attachment 314884 [details]
Patch

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

r=me with some minor tweaks, though this will need a wk2r+ too.

> Source/WebKit2/ChangeLog:11
> +        * Shared/NavigationActionData.cpp:

ChangeLog entry still references rootViewClickLocation instead of clickLocationInRootViewCoordinates.

> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationAction.mm:127
> +    return [NSString stringWithFormat:@"<%@: %p; navigationType = %ld; syntheticClickType = %ld; position x = %.2f y= %.2f request = %@; sourceFrame = %@; targetFrame = %@>", NSStringFromClass(self.class), self,

Nit - missing a space between y and =

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:111
> +WebCore::FloatPoint InjectedBundleNavigationAction::clickLocationInRootViewCoordinatesForNavigationAction(const NavigationAction& navigationAction)

Nit - no need for WebCore:: here.

[Megan Gardner]

Created attachment 314889 [details]
Patch

[Megan Gardner]

Created attachment 314894 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 314894 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:80
> +    return FloatPoint(mouseEvent->clientX(), mouseEvent->clientY());

I think this is probably wrong if the event occurred inside an iframe (clientX/clientY) will be relative to the iframe. clientX/clientY may also not map simply to root view coordinates because of page scale.

[Simon Fraser (smfr)]

(In reply to Simon Fraser (smfr) from comment #12)
> Comment on attachment 314894 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=314894&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.cpp:80
> > +    return FloatPoint(mouseEvent->clientX(), mouseEvent->clientY());
> 
> I think this is probably wrong if the event occurred inside an iframe
> (clientX/clientY) will be relative to the iframe. clientX/clientY may also
> not map simply to root view coordinates because of page scale.

I would use mouseEvent->absoluteLocation(), and call contentsToRootView() on the FrameView obtained from the event's "view" (actually a DOMWindow). We could maybe add a function to Mouse{Related}Event to do this.

[Megan Gardner]

Created attachment 314908 [details]
Patch

[Megan Gardner]

Ok, hopefully this is the right way to get the point.

[Wenson Hsieh]

Comment on attachment 314908 [details]
Patch

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

> Source/WebCore/dom/MouseRelatedEvent.cpp:170
> +int MouseRelatedEvent::absoluteLocationConvetedToRootViewX() const

This says "conveted"

> Source/WebCore/dom/MouseRelatedEvent.cpp:174
> +    return frameView()->contentsToRootView(flooredIntPoint(m_absoluteLocation)).x();

In most other places, where we perform absolute <=> root view conversion, we round instead of flooring (i.e. roundedIntPoint). Is there a reason we should prefer flooring here?

> Source/WebCore/dom/MouseRelatedEvent.cpp:181
> +    return frameView()->contentsToRootView(flooredIntPoint(m_absoluteLocation)).y();

Splitting these into separate methods means two calls to contentsToRootView when we need to compute one of these points, when only one is needed. Can we instead make this something like absoluteLocationInRootViewCoordinates() and have it return an IntPoint, and then have clickLocationInRootViewCoordinatesForMouseEvent return mouseEvent->absoluteLocationInRootViewCoordinates()?

> Source/WebKit2/ChangeLog:22
> +        (WebKit::InjectedBundleNavigationAction::rootViewClickLocationForNavigationAction):

These names still reference "rootViewClickLocation" instead of "clickLocationInRootViewCoordinates".

[Megan Gardner]

Created attachment 314996 [details]
Patch

[Build Bot]

Attachment 314996 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Megan Gardner]

Created attachment 314998 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 314998 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Add the root view location of a tap to a NavigationAction to vend to Safari.
> +
> +        Reviewed by NOBODY (OOPS!).

Swap these two lines around.

> Source/WebCore/ChangeLog:11
> +        Test: small enough change to not be tested alone.

But you could write an API test for it.

> Source/WebCore/dom/MouseRelatedEvent.h:60
> +    WEBCORE_EXPORT IntPoint absoluteLocationConvertedToRootView() const;

The fact that it's computed from absolute coords is not relevant to the caller; this should just be called locationInRootViewCoordinates(), and should return a FloatPoint (since converting to root view may go through transforms).

[Megan Gardner]

Created attachment 315004 [details]
Patch

Comment on attachment 315004 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKNavigationActionPrivate.h:54
> +@property (nonatomic, readonly) CGPoint _clickLocationInRootViewCoordinates WK_API_AVAILABLE(ios(11.0));

In the past, we had used WK_IOS_TBA for new API prior to the GM release of the SDK.

[Megan Gardner]

https://trac.webkit.org/changeset/219304/webkit

[Radar WebKit Bug Importer]

<rdar://problem/34782261>