Bug 138284 – Implement action menus for editable text

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Beth Dakin]

Implement action menus for editable text.

rdar://problem/18742323

[Beth Dakin]

Created attachment 240797 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:340:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 5 files


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

[Tim Horton]

Comment on attachment 240797 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:339
> +-(void)_paste:(id)sender

"copyText", but "paste"? copy can copy things that aren't text; can we go with _copy and _paste or _performCopy and _performPaste or something?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:496
> +    return hitTestResult;

.release() here, I think

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:503
> +        _type = kWKActionMenuNone;

unfortunate that the 'none' case is duplicated now.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:518
> +        if (DDActionContext *actionContext = _hitTestResult.actionContext.get()) {

We should shuffle this out into a "defaultMenuItemsForDataDetectedItem" or something, so that these all look the same.

[Beth Dakin]

(In reply to comment #3)
> Comment on attachment 240797 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240797&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:339
> > +-(void)_paste:(id)sender
> 
> "copyText", but "paste"? copy can copy things that aren't text; can we go
> with _copy and _paste or _performCopy and _performPaste or something?
> 

They are inconsistent. I do think that copy needs a more specific name because right now it will just copy the current selection. So perhaps copySelection is right for copy. paste still seems like the most accurate way to describe paste, but I suppose we could go with the somewhat redundant pasteFromPasteboard?! I'll think about it, but fix copy for sure.

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:496
> > +    return hitTestResult;
> 
> .release() here, I think
> 

Ah yes.

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:503
> > +        _type = kWKActionMenuNone;
> 
> unfortunate that the 'none' case is duplicated now.
> 

I know. It seems so crazy to have all the rest of it nested in an if though.

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:518
> > +        if (DDActionContext *actionContext = _hitTestResult.actionContext.get()) {
> 
> We should shuffle this out into a "defaultMenuItemsForDataDetectedItem" or
> something, so that these all look the same.

Will do.

Thanks you!

[Beth Dakin]

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