WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
163406
Add test and infrastructure for link popover
https://bugs.webkit.org/show_bug.cgi?id=163406
Summary
Add test and infrastructure for link popover
Megan Gardner
Reported
2016-10-13 15:11:34 PDT
Add test and infrastructure for link popover
Attachments
Patch
(19.98 KB, patch)
2016-10-13 15:30 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(19.80 KB, patch)
2016-10-17 15:07 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(20.14 KB, patch)
2016-10-17 17:17 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Patch
(20.16 KB, patch)
2016-10-17 17:39 PDT
,
Megan Gardner
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Megan Gardner
Comment 1
2016-10-13 15:30:45 PDT
Created
attachment 291526
[details]
Patch
Simon Fraser (smfr)
Comment 2
2016-10-13 16:15:59 PDT
Comment on
attachment 291526
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=291526&action=review
> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:26 > +#pragma once
Don't add this, since Obj-C's #import already takes care of this.
> Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:274 > +- (void)didShowForcePressPreview WK_API_AVAILABLE(ios(WK_IOS_TBA)); > +- (void)didDismissForcePressPreview WK_API_AVAILABLE(ios(WK_IOS_TBA));
These should have leading underscores ("_didShowForcePressPreview") - the others need fixing too.
> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3851 > + if ([userInterfaceItem isEqualToString:@"popoverContents"]) {
Blank line above please.
> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:125 > + // Preview form handling
Maybe "Force press preview handling"
> Tools/WebKitTestRunner/cocoa/TestRunnerWKWebView.h:26 > +#pragma once
No need.
> LayoutTests/ChangeLog:9 > + * fast/events/touch/ios/iphone7/force-press-on-link-expected.txt: Added. > + * fast/events/touch/ios/iphone7/force-press-on-link.html: Added.
Does the popover go away when this test is complete?
> LayoutTests/fast/events/touch/ios/iphone7/force-press-on-link.html:96 > + <a href="
http://www.apple.com
">Link Test</a>
This should be a local URL, rather than loading a live web site.
Megan Gardner
Comment 3
2016-10-17 15:07:54 PDT
Created
attachment 291882
[details]
Patch
WebKit Commit Bot
Comment 4
2016-10-17 15:10:18 PDT
Attachment 291882
[details]
did not pass style-queue: ERROR: Tools/WebKitTestRunner/cocoa/TestRunnerWKWebView.h:0: Use #pragma once header guard. [build/header_guard] [5] ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:0: Use #pragma once header guard. [build/header_guard] [5] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Simon Fraser (smfr)
Comment 5
2016-10-17 15:14:15 PDT
Comment on
attachment 291882
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=291882&action=review
Good patch, but the test needs tweaking.
> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3852 > + if ([userInterfaceItem isEqualToString:@"popoverContents"]) {
@"popoverContents" seems a bit too generic; I think it should be @"linkPreviewPopoverURL" or something.
> LayoutTests/fast/events/touch/ios/iphone7/force-press-on-link.html:56 > + uiController.uiScriptComplete(JSON.stringify(uiController.contentsOfUserInterfaceItem('popoverContents'))); > + uiController.liftUpAtPoint(20,40,1,function() {});
This is racey. In the uiController.uiScriptComplete() callback, the web process does testRunner.notifyDone(), so may start moving onto the next test. What you should do instead is have a second testRunner.runUIScript() below, have that fire the uiController.liftUpAtPoint(), and only notifyDone() when that second one completes.
Megan Gardner
Comment 6
2016-10-17 17:17:52 PDT
Created
attachment 291900
[details]
Patch
WebKit Commit Bot
Comment 7
2016-10-17 17:19:01 PDT
Attachment 291900
[details]
did not pass style-queue: ERROR: Tools/WebKitTestRunner/cocoa/TestRunnerWKWebView.h:0: Use #pragma once header guard. [build/header_guard] [5] ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:0: Use #pragma once header guard. [build/header_guard] [5] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Simon Fraser (smfr)
Comment 8
2016-10-17 17:28:56 PDT
Comment on
attachment 291900
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=291900&action=review
> Source/WebKit2/UIProcess/ios/WKContentViewInteraction.mm:3854 > + return @{ userInterfaceItem: @{@"pageURL": url}};
Perferred spacing here would be: @{ userInterfaceItem: @{ @"pageURL": url } };
> LayoutTests/fast/events/touch/ios/iphone7/force-press-on-link.html:65 > + uiController.liftUpAtPoint(20,40,1,function() {
Spaces after commas please.
> LayoutTests/fast/events/touch/ios/iphone7/force-press-on-link.html:87 > + document.getElementById('target').innerHTML = output;
I think it's OK to do this before this testRunner.runUIScript() invocation.
Megan Gardner
Comment 9
2016-10-17 17:39:09 PDT
Created
attachment 291904
[details]
Patch
WebKit Commit Bot
Comment 10
2016-10-17 17:41:28 PDT
Attachment 291904
[details]
did not pass style-queue: ERROR: Tools/WebKitTestRunner/cocoa/TestRunnerWKWebView.h:0: Use #pragma once header guard. [build/header_guard] [5] ERROR: Source/WebKit2/UIProcess/API/Cocoa/WKWebViewPrivate.h:0: Use #pragma once header guard. [build/header_guard] [5] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 11
2016-10-17 18:11:07 PDT
Comment on
attachment 291904
[details]
Patch Clearing flags on attachment: 291904 Committed
r207447
: <
http://trac.webkit.org/changeset/207447
>
WebKit Commit Bot
Comment 12
2016-10-17 18:11:11 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug