Summary: | Add location to NavigationActionData | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Megan Gardner <megan_gardner> | ||||||||||||||||||||||
Component: | New Bugs | Assignee: | Megan Gardner <megan_gardner> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | bdakin, buildbot, cdumez, dbates, esprehn+autocc, kangil.han, mitz, simon.fraser, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
Megan Gardner
2017-07-06 19:07:07 PDT
Created attachment 314792 [details]
Patch
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. 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 { };" Created attachment 314864 [details]
Patch
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 Created attachment 314871 [details]
Patch
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. Created attachment 314884 [details]
Patch
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. Created attachment 314889 [details]
Patch
Created attachment 314894 [details]
Patch
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. (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. Created attachment 314908 [details]
Patch
Ok, hopefully this is the right way to get the point. 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". Created attachment 314996 [details]
Patch
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.
Created attachment 314998 [details]
Patch
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). 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. |