Bug 143895 – [Mac] Extend action menus to support PDF

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

There are no action menu options for embedded PDF documents. We should hook up some basic support.

[Brent Fulgham]

<rdar://problem/19003333>

[Brent Fulgham]

Created attachment 251046 [details]
Patch

[Tim Horton]

Comment on attachment 251046 [details]
Patch

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

> Source/WebKit2/ChangeLog:13
> +        If no text is selected, select the word underneath the mouse. This is consistent with how
> +        normal text is treated in pure HTML views. If some text is already selected, and the mouse

This is not consistent with how normal text is treated in pure HTML views. We use Lookup to determine what to select, we don't just select the word under the cursor.

[Brent Fulgham]

Created attachment 251239 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:457:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:741:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:815:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1875:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 25 files


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

[Brent Fulgham]

Created attachment 251244 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:457:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:741:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:815:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1875:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 25 files


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

[Tim Horton]

Comment on attachment 251244 [details]
Patch

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

> Source/WebCore/Configurations/Base.xcconfig:116
> +OTHER_CFLAGS = -iframework $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Frameworks $(ASAN_OTHER_CFLAGS);
> +OTHER_CPLUSPLUSFLAGS = -iframework $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Frameworks $(ASAN_OTHER_CPLUSPLUSFLAGS);

This should probably be in WebCore.xcconfig.

> Source/WebCore/editing/mac/DictionaryLookup.h:52
> +WEBCORE_EXPORT NSString* dictionaryLookupForPDFSelection(PDFSelection*, NSDictionary **options);

Lots of stars on the wrong side.

> Source/WebKit2/Shared/API/c/WKActionMenuTypes.h:47
> +    kWKActionMenuPDF

This is very specifically for read only text in a PDF, right? (links will be -Link and images will be -Image). It should have a better name. kWKActionMenuPDFReadOnlyText, just like the one above?

> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:494
> +- (NSArray *)_defaultMenuItemsForPDF

This, too, should have a better name.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h:98
> +- (NSArray*) rectsForSelectionInLayoutSpace: (PDFSelection*) sel;

the spacing in this line is all wrong

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:172
> +    String lookupTextAtLocation(const WebCore::FloatPoint&, PDFSelection**, NSDictionary**) const override;

stars on the wrong side

> Source/WebKit2/WebProcess/WebPage/WebPage.h:1096
> +    String selectedTextAtLocation(PluginView&, WebCore::FloatPoint);

This should probably have plugin in its name somewhere?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1118
> +                    pluginDocument.setFocusedElement(element);

This is fairly surprising. What code does this for e.g. subframes? I assume it's not here.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1125
> +                if (WebCore::protocolIsInHTTPFamily(selectedText)) {

It doesn't seem like this will work if the title of the link and the URL of the link are different (Lookup will select the title, or part of the title...) Is there not a way to use the PDFAnnotation API to get the link URL?

> Tools/TestWebKitAPI/Tests/WebKit2/action-menu-targets.html:12
> +    width:100%; height:100%;
> +    overflow:hidden;

spaces after colons

> Tools/TestWebKitAPI/Tests/WebKit2/action-menu-targets.html:113
> +<div style="top: 250px; left: 400px; height:350px"><embed src="test.pdf"></embed></div><br/>

ditto

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:456
> +        [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate distantPast]];

shouldn't this use Util::run?

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:740
> +    // Click in the PDF to make sure focus is in the view:

Isn't this somewhat contradictory with your explicit focus earlier?

[Brent Fulgham]

Comment on attachment 251244 [details]
Patch

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

>> Source/WebCore/Configurations/Base.xcconfig:116
>> +OTHER_CPLUSPLUSFLAGS = -iframework $(SDKROOT)$(SYSTEM_LIBRARY_DIR)/Frameworks/Quartz.framework/Frameworks $(ASAN_OTHER_CPLUSPLUSFLAGS);
> 
> This should probably be in WebCore.xcconfig.

OK!

>> Source/WebCore/editing/mac/DictionaryLookup.h:52
>> +WEBCORE_EXPORT NSString* dictionaryLookupForPDFSelection(PDFSelection*, NSDictionary **options);
> 
> Lots of stars on the wrong side.

OK!

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h:98
>> +- (NSArray*) rectsForSelectionInLayoutSpace: (PDFSelection*) sel;
> 
> the spacing in this line is all wrong

Whoops! Fixed.

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:172
>> +    String lookupTextAtLocation(const WebCore::FloatPoint&, PDFSelection**, NSDictionary**) const override;
> 
> stars on the wrong side

ok

>> Source/WebKit2/WebProcess/WebPage/WebPage.h:1096
>> +    String selectedTextAtLocation(PluginView&, WebCore::FloatPoint);
> 
> This should probably have plugin in its name somewhere?

Actually, this method doesn't even exist! I'll remove this cruft form an earlier attempt at this patch.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1118
>> +                    pluginDocument.setFocusedElement(element);
> 
> This is fairly surprising. What code does this for e.g. subframes? I assume it's not here.

I was finding that the PDFPlugin was not reliably taking focus when invoking an action menu. This would cause the text to be selected, but when a 'copy' command was run through the page editor plumbing, it would decide the plugin wasn't active and ignore the copy command.

Subframes have the same problem (flakiness). I'm not sure who is responsible for placing focus on the subframe/plugin/etc.

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:1125
>> +                if (WebCore::protocolIsInHTTPFamily(selectedText)) {
> 
> It doesn't seem like this will work if the title of the link and the URL of the link are different (Lookup will select the title, or part of the title...) Is there not a way to use the PDFAnnotation API to get the link URL?

I'll see if I can figure out how to do that.

>> Tools/TestWebKitAPI/Tests/WebKit2/action-menu-targets.html:12
>> +    overflow:hidden;
> 
> spaces after colons

OK!

>> Tools/TestWebKitAPI/Tests/WebKit2/action-menu-targets.html:113
>> +<div style="top: 250px; left: 400px; height:350px"><embed src="test.pdf"></embed></div><br/>
> 
> ditto

OK!

>> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:456
>> +        [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate distantPast]];
> 
> shouldn't this use Util::run?

OK!

>> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:740
>> +    // Click in the PDF to make sure focus is in the view:
> 
> Isn't this somewhat contradictory with your explicit focus earlier?

That comment isn't true any more. I'll remove it. I don't perform an explicit click in the view here anymore.

[Brent Fulgham]

Comment on attachment 251244 [details]
Patch

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

>> Source/WebKit2/Shared/API/c/WKActionMenuTypes.h:47
>> +    kWKActionMenuPDF
> 
> This is very specifically for read only text in a PDF, right? (links will be -Link and images will be -Image). It should have a better name. kWKActionMenuPDFReadOnlyText, just like the one above?

I've decided to remove this. I never actually hit this case, because the PDF processing causes the relevant content to be flagged as "ReadOnlyText", or "Link", etc.

>> Source/WebKit2/UIProcess/mac/WKActionMenuController.mm:494
>> +- (NSArray *)_defaultMenuItemsForPDF
> 
> This, too, should have a better name.

Also gone!

[Brent Fulgham]

Created attachment 251300 [details]
Patch v4 (Working test cases!)

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:454:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:738:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:802:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1884:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 29 files


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

[Brent Fulgham]

Comment on attachment 251300 [details]
Patch v4 (Working test cases!)

Updated for the following:
1. Tim's suggestions resolved.
2. Links are now found by PDFAnnotation, rather than string matching.
3. The cause of the perpetual-resetting PDFSelection in TestWebKitAPI was fixed.

[Brent Fulgham]

The mysterious "perpetual reset" of the PDFSelection in TestWebKitAPI was triggered by my misguided attempt to force focus to the PDFView by performing a mouse click.

I'm not sure if the "platformWebView.simulateButtonClick(kWKEventMouseButtonLeftButton, ...);" maybe leaves the button in the pressed state, or if there is some other bug in that implementation.

Happily, the button press wasn't needed and once it was removed everything worked as expected.

[Tim Horton]

Comment on attachment 251300 [details]
Patch v4 (Working test cases!)

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

> Source/WebCore/editing/mac/DictionaryLookup.mm:177
> +NSString *dictionaryLookupForPDFSelection(PDFSelection *selection, NSDictionary **options)

I'm not sure "dictionaryLookup for PDFSelection" really captures what this function returns. It's not far off, but it's not quite right.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h:30
> +@class PDFTileCache;

Why in the world does WebKit need to know about PDFTileCache? That is an implementation detail.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1883
> +    *selection = [m_pdfLayerController currentSelection];

I think it looks strange to keep using *selection everywhere below. Maybe we can have a local too?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1892
> +    PDFTileCache *tileCache = (PDFTileCache *)[m_pdfLayerController valueForKey:@"_tileCache"];

Please no! Add something to PDFLayerController if you need it. Or just reimplement it here (it doesn't look very complicated). Anything that doesn't cause us to depend on these internals.

> Source/WebKit/mac/WebView/WebDocument.h:90
> +- (WebFrame *)_frame;

This is WK1 SPI. Are you sure this is what you meant?

> Source/WebKit/mac/WebView/WebHTMLView.h:69
> +- (WebFrame *)_frame;

I really think this belonged in an Internal header, not in the SPI headers. I don't know what's going on here, but it doesn't seem right.

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:462
> -TEST(WebKit2, DISABLED_ActionMenusTest)
> +TEST(WebKit2, ActionMenusTest)

Yayayay! Thank you!

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:743
> +        WKStringGetUTF8CString(wkLookupText.get(), lookupText, length);

Please use EXPECT_WK_STREQ instead of all of this stuff. It will magic the C string out of the WKStringRef and compare it with your other string regardless of type (well, for any combination of WKStringRef, WKRetainPtr<WKStringRef>, NSString *, char *, etc.)

> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:807
> +        WKStringGetUTF8CString(wkLookupText.get(), lookupText, length);

Ditto on the EXPECT_WK_STREQ.

[Brent Fulgham]

Created attachment 251325 [details]
Patch v5 (Fix iOS Build Error)

[WebKit Commit Bot]

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


ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:454:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:738:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:802:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1884:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 30 files


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

[Brent Fulgham]

Comment on attachment 251300 [details]
Patch v4 (Working test cases!)

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

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h:30
>> +@class PDFTileCache;
> 
> Why in the world does WebKit need to know about PDFTileCache? That is an implementation detail.

It needs this for 'pointInLayoutSpaceForPointInWindowSpace', which is the only way I could get the right coordinates. :-(

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1883
>> +    *selection = [m_pdfLayerController currentSelection];
> 
> I think it looks strange to keep using *selection everywhere below. Maybe we can have a local too?

Sure!

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1892
>> +    PDFTileCache *tileCache = (PDFTileCache *)[m_pdfLayerController valueForKey:@"_tileCache"];
> 
> Please no! Add something to PDFLayerController if you need it. Or just reimplement it here (it doesn't look very complicated). Anything that doesn't cause us to depend on these internals.

OK. I think I have something that will work.

>> Source/WebKit/mac/WebView/WebDocument.h:90
>> +- (WebFrame *)_frame;
> 
> This is WK1 SPI. Are you sure this is what you meant?

OK. I'll revise this.

>> Source/WebKit/mac/WebView/WebHTMLView.h:69
>> +- (WebFrame *)_frame;
> 
> I really think this belonged in an Internal header, not in the SPI headers. I don't know what's going on here, but it doesn't seem right.

Ditto.

>> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:462
>> +TEST(WebKit2, ActionMenusTest)
> 
> Yayayay! Thank you!

:-)

>> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:743
>> +        WKStringGetUTF8CString(wkLookupText.get(), lookupText, length);
> 
> Please use EXPECT_WK_STREQ instead of all of this stuff. It will magic the C string out of the WKStringRef and compare it with your other string regardless of type (well, for any combination of WKStringRef, WKRetainPtr<WKStringRef>, NSString *, char *, etc.)

Oh, cool! Will Do!

>> Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:807
>> +        WKStringGetUTF8CString(wkLookupText.get(), lookupText, length);
> 
> Ditto on the EXPECT_WK_STREQ.

OK!

[Brent Fulgham]

Created attachment 251364 [details]
Patch v6 (Fix more Tim comments)

[Brent Fulgham]

Comment on attachment 251364 [details]
Patch v6 (Fix more Tim comments)

Get rid of PDFTileCache, and the questionable _frame access changes.

[WebKit Commit Bot]

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


ERROR: Source/WebKit/mac/WebView/WebActionMenuController.mm:100:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:454:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:738:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKit2ObjC/ActionMenus.mm:797:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1896:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 5 in 24 files


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

[Tim Horton]

Comment on attachment 251364 [details]
Patch v6 (Fix more Tim comments)

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

> Source/WebCore/editing/mac/DictionaryLookup.mm:168
> +    size_t originalLength = [[selection string] length];

Dot notation for many of these things. (Lots of places)

[Brent Fulgham]

Committed r183136: <http://trac.webkit.org/changeset/183136>