Summary: | Implement action menus for text | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Beth Dakin <bdakin> | ||||
Component: | WebKit2 | Assignee: | 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
Beth Dakin
2014-10-30 12:10:49 PDT
Created attachment 240685 [details]
Patch
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 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? Probably should have Enrica or someone look over this too. (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. Thanks Tim! |