Bug 138220

Summary: Implement action menus for text
Product: WebKit Reporter: Beth Dakin <bdakin>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, conrad_shultz, enrica, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch thorton: review+

Description Beth Dakin 2014-10-30 12:10:49 PDT
Implement action menus for text

rdar://problem/18742297
Comment 1 Beth Dakin 2014-10-30 12:18:26 PDT
Created attachment 240685 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-30 12:20:03 PDT
Attachment 240685 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tim Horton 2014-10-30 12:26:10 PDT
Comment on attachment 240685 [details]
Patch

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

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:118
> +    if (!hitTestResult->isSelected())

Shouldn't this be "if we have no selection at all"? I'm not sure.

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:314
> +    if (!hitTestResult->isTextNode())
> +        return @[ ];

No need to check this here, the caller already checked. At most, ASSERT. Also, our _type system depends on these things actually returning the menu we ask for!

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:324
> +    _page->executeEditCommand(String("copy"));

Doesn't the literal->String conversion happen magically?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:532
> +PassRefPtr<Range> WebPage::rangeForDictionaryLookupAtHitTestResult(const WebCore::HitTestResult& hitTestResult, NSDictionary **options)

NSDictionary ** could be 'NSDictionary*& options' instead, I think?
Comment 4 Tim Horton 2014-10-30 12:26:25 PDT
Probably should have Enrica or someone look over this too.
Comment 5 Beth Dakin 2014-10-30 12:36:51 PDT
(In reply to comment #3)
> Comment on attachment 240685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240685&action=review
> 
> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:118
> > +    if (!hitTestResult->isSelected())
> 
> Shouldn't this be "if we have no selection at all"? I'm not sure.
> 
I did consider this, but I don't think so…I think it feels a lot more natural if invoking the action menu on text moves the selection there since it normally creates a selection. We can look at this in person if you like.

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:314
> > +    if (!hitTestResult->isTextNode())
> > +        return @[ ];
> 
> No need to check this here, the caller already checked. At most, ASSERT.
> Also, our _type system depends on these things actually returning the menu
> we ask for!
> 

Okay!

> > Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:324
> > +    _page->executeEditCommand(String("copy"));
> 
> Doesn't the literal->String conversion happen magically?
> 

Oh, yes! Fixed.

> > Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:532
> > +PassRefPtr<Range> WebPage::rangeForDictionaryLookupAtHitTestResult(const WebCore::HitTestResult& hitTestResult, NSDictionary **options)
> 
> NSDictionary ** could be 'NSDictionary*& options' instead, I think?

I don't think so? WebKitSystemInterface function wants a **, and I can't seem to make it work with a *& passed into the WebPage functions, but I'm falling down the pointer/reference rabbit hole a little bit, so yeah.
Comment 6 Beth Dakin 2014-10-30 12:38:41 PDT
Thanks Tim!