Bug 138220 – Implement action menus for text

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

[Beth Dakin]

Implement action menus for text

rdar://problem/18742297

[Beth Dakin]

Created attachment 240685 [details]
Patch

[WebKit Commit Bot]

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.

[Tim Horton]

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?

[Tim Horton]

Probably should have Enrica or someone look over this too.

[Beth Dakin]

(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.

[Beth Dakin]

Thanks Tim!