Bug 143952 – Immediate action not functional for embedded PDFs

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

There is currently no immediate action support for embedded PDFs.

[Brent Fulgham]

Created attachment 251500 [details]
Patch

[Brent Fulgham]

<rdar://problem/19842365>

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:607:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


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

[Brent Fulgham]

(In reply to comment #3)
> Attachment 251500 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:607:  Place brace
> on its own line for function definitions.  [whitespace/braces] [4]
> Total errors found: 1 in 12 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

This is a Block, which is styled properly.

[Tim Horton]

Comment on attachment 251500 [details]
Patch

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

> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:127
> +    WebCore::FloatRect viewRectForSelection(PDFSelection *) const override { return WebCore::FloatRect(); }

I have to say, these things are getting pretty not abstract. Maybe we should give up on the abstraction for some things and talk directly to PDFPlugin? Or make these things more generic (sufficiently generic that they could be IPCable in the case we ever want to put PDFPlugin in its own process?).

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1937
> +static NSRect rectInWindowSpaceForRectInLayoutSpace(PDFLayerController* pdfLayerController, NSRect rectInLayout)

"window", really?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:622
> +    dataForSelection.contentImageScaleFactor = 1.0;

Does this work on 2x displays?

[Brent Fulgham]

Comment on attachment 251500 [details]
Patch

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

>> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:127
>> +    WebCore::FloatRect viewRectForSelection(PDFSelection *) const override { return WebCore::FloatRect(); }
> 
> I have to say, these things are getting pretty not abstract. Maybe we should give up on the abstraction for some things and talk directly to PDFPlugin? Or make these things more generic (sufficiently generic that they could be IPCable in the case we ever want to put PDFPlugin in its own process?).

I'm fine grabbing the PDFPlugin directly. Why don't I move the PDFSelection-based API out of PluginView, since other plugins are not going to deal in terms of PDFSelections.

I could just pass the hit test point again, but that seems like extra work for little gain (although that is the way we would want to do this if we were IPC'ing to a separate process.

>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1937
>> +static NSRect rectInWindowSpaceForRectInLayoutSpace(PDFLayerController* pdfLayerController, NSRect rectInLayout)
> 
> "window", really?

rectInViewSpaceForRectInLayoutSpace?

>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:622
>> +    dataForSelection.contentImageScaleFactor = 1.0;
> 
> Does this work on 2x displays?

Well, images don't work at all so this is just a placeholder so it's not uninitialized. I guess I could supply 'contentScaleFactor' or something here, but since I do not supply images in general I don't know that it makes much of a difference?

[Tim Horton]

Comment on attachment 251500 [details]
Patch

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

>>> Source/WebKit2/WebProcess/Plugins/PluginProxy.h:127
>>> +    WebCore::FloatRect viewRectForSelection(PDFSelection *) const override { return WebCore::FloatRect(); }
>> 
>> I have to say, these things are getting pretty not abstract. Maybe we should give up on the abstraction for some things and talk directly to PDFPlugin? Or make these things more generic (sufficiently generic that they could be IPCable in the case we ever want to put PDFPlugin in its own process?).
> 
> I'm fine grabbing the PDFPlugin directly. Why don't I move the PDFSelection-based API out of PluginView, since other plugins are not going to deal in terms of PDFSelections.
> 
> I could just pass the hit test point again, but that seems like extra work for little gain (although that is the way we would want to do this if we were IPC'ing to a separate process.

> I'm fine grabbing the PDFPlugin directly.

I ask this mostly in a bigger sense; a lot of things were added to PluginProxy/PluginView just for PDFPlugin, and in retrospect it seems silly (especially watching things with PDF in their name get added).

Try talking to PDFPlugin directly and see how ugly it is. Or maybe talk to Anders.

>>> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1937
>>> +static NSRect rectInWindowSpaceForRectInLayoutSpace(PDFLayerController* pdfLayerController, NSRect rectInLayout)
>> 
>> "window", really?
> 
> rectInViewSpaceForRectInLayoutSpace?

Sure.

>>> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:622
>>> +    dataForSelection.contentImageScaleFactor = 1.0;
>> 
>> Does this work on 2x displays?
> 
> Well, images don't work at all so this is just a placeholder so it's not uninitialized. I guess I could supply 'contentScaleFactor' or something here, but since I do not supply images in general I don't know that it makes much of a difference?

Ah, I mixed this up with the TextIndicator content image scale factor.

[Brent Fulgham]

Created attachment 251509 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:607:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 10 files


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

[Brent Fulgham]

Created attachment 251516 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:608:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 11 files


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

[Brent Fulgham]

Created attachment 251534 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:608:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 14 files


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

[Tim Horton]

Comment on attachment 251534 [details]
Patch

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

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:1942
> +    CGFloat scaleFactor = [pdfLayerController contentScaleFactor];
> +    CGPoint scrollOffset = [pdfLayerController scrollPosition];

dot notation everywhere

[Brent Fulgham]

Committed r183277: <http://trac.webkit.org/changeset/183277>