Bug 143916 – Implement immediate action support for tel: and mailto: URLs

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

[Tim Horton]

Implement immediate action support for tel: and mailto: URLs

[Tim Horton]

Created attachment 251086 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:415:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:416:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:418:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:437:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:438:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:441:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 6 in 6 files


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

[Tim Horton]

Created attachment 251087 [details]
Patch

[Tim Horton]

<rdar://problem/19721711>

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:415:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:416:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit/mac/WebView/WebImmediateActionController.mm:418:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:437:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:438:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:441:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 6 in 6 files


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

[Darin Adler]

Comment on attachment 251087 [details]
Patch

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

> Source/WebKit/mac/WebView/WebImmediateActionController.mm:232
> +            RefPtr<TextIndicator> linkTextIndicator = TextIndicator::createWithRange(*linkRange, TextIndicatorPresentationTransition::FadeIn);

Code you just indented: I would call this just indicator or textIndicator; local variables don’t need globally clear names.

> Source/WebKit/mac/WebView/WebImmediateActionController.mm:235
> +            RetainPtr<QLPreviewMenuItem> qlPreviewLinkItem = [NSMenuItem standardQuickLookMenuItem];

Code you just indented: Why does this use a RetainPtr? Does not need one. I would call this just item or menuItem. Local variables don’t need globally clear names.

> Source/WebKit/mac/WebView/WebImmediateActionController.mm:245
> +        if (id<NSImmediateActionAnimationController> animationController = [self _animationControllerForDataDetectedText]) {

Can this use auto?

> Source/WebKit/mac/WebView/WebImmediateActionController.mm:413
> +    RefPtr<TextIndicator> linkTextIndicator = TextIndicator::createWithRange(*linkRange, TextIndicatorPresentationTransition::FadeIn);

Shorter name as I suggested above?

> Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:294
> +            RetainPtr<QLPreviewMenuItem> qlPreviewLinkItem = [NSMenuItem standardQuickLookMenuItem];
> +            [qlPreviewLinkItem setPreviewStyle:QLPreviewStylePopover];
> +            [qlPreviewLinkItem setDelegate:self];
> +            _currentQLPreviewMenuItem = qlPreviewLinkItem.get();

Really lame that we simply have this duplicated for legacy and modern WebKit. Need a code sharing strategy to avoid doing more and more of this over time for features like this one. All my comments above apply here as well, given that!

> Source/WebKit2/UIProcess/mac/WKImmediateActionController.mm:428
> +    RetainPtr<DDActionContext> actionContext = adoptNS([allocDDActionContextInstance() init]);

Ditto.

[Tim Horton]

(In reply to comment #6)
> Comment on attachment 251087 [details]
> Patch
> 

All the other comments sound good to me!

> Really lame that we simply have this duplicated for legacy and modern
> WebKit. Need a code sharing strategy to avoid doing more and more of this
> over time for features like this one. All my comments above apply here as
> well, given that!

I wholeheartedly agree; the maintenance burden has already been significant. I have a plan for a merger, but haven't had a chance to enact it, and now we've spent months making changes in two places.

[Tim Horton]

http://trac.webkit.org/changeset/183019