Bug 100155 – Full-page PDFPlugin should support inline form editing

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

Full-page PDFPlugin should support inline form editing. It's possible we want to support inline-PDFPlugin forms too, but for now, the full-page case is easiest.

[Radar WebKit Bug Importer]

<rdar://problem/12558511>

[Tim Horton]

Created attachment 170270 [details]
patch

[Tim Horton]

Created attachment 170273 [details]
rebase patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h:44:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 17 files


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

[Darin Adler]

Comment on attachment 170270 [details]
patch

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

Patch is not applying for EWS.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:188
> +    // FIXME: Should we support forms for inline PDFs? Since we touch the document, this might be difficult.

I don’t think the term “inline PDF” is a thing. Do you mean PDFs used as images?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:202
> +        annotationContainer->setInlineStyleProperty(CSSPropertyOverflow, "hidden");
> +        annotationContainer->setInlineStyleProperty(CSSPropertyPosition, "absolute");
> +        annotationContainer->setInlineStyleProperty(CSSPropertyPointerEvents, "none");
> +        annotationContainer->setInlineStyleProperty(CSSPropertyTop, 0.0, CSSPrimitiveValue::CSS_PX);
> +        annotationContainer->setInlineStyleProperty(CSSPropertyLeft, 0.0, CSSPrimitiveValue::CSS_PX);
> +        annotationContainer->setInlineStyleProperty(CSSPropertyRight, 0.0, CSSPrimitiveValue::CSS_PX);
> +        annotationContainer->setInlineStyleProperty(CSSPropertyBottom, 0.0, CSSPrimitiveValue::CSS_PX);

Is this more efficient than setting a hardcoded style attribute string? Doing that would avoid the need to cast to StyledElement* and cut down a lot on the lines of code.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:585
> +    // FIXME: Should we support forms for inline PDFs? Since we touch the document, this might be difficult.

Would be nice to not repeat this comment. One way to do that would be to add a helper function that just returns this value named something like “supportsForms()”.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:596
> +    }
> +    else

WebKit coding style puts both of these on the same line.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:52
> +    PassRefPtr<WebCore::Element> element() { return m_element; }

Correct return type for this kind of getter is is WebCore::Element*, not PassRefPtr. Also should be const.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:53
> +    RetainPtr<PDFAnnotation> annotation() { return m_annotation; }

PDFAnnotation * is the correct return type. Also should be const.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:54
> +    PDFPlugin* plugin() { return m_plugin; }

Should be const.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:57
> +    virtual void updateGeometry();
> +    virtual void commit() = 0;

Can these be private or protected?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:70
> +    virtual PassRefPtr<WebCore::Element> createAnnotationElement() = 0;

Can this be private instead of protected?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:71
> +    WebCore::Element* parent() { return m_parent; }

Should be const.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:73
> +    PDFLayerController *pdfLayerController() { return m_pdfLayerController; }

Should be const.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:83
> +         bool operator==(const EventListener& listener) OVERRIDE { return this == &listener; }

Need virtual keyword here.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:84
> +         virtual void handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) OVERRIDE;

I suggest making this private.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:93
> +         {
> +
> +         }

Extra blank line.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:47
> +#import "PDFLayerControllerDetails.h"

This should be in with the rest of the includes, sorted.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:59
> +    else if ([annotation isKindOfClass:pdfAnnotationChoiceWidgetClass()])

No else after if in WebKit coding style.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.h:44
> +    virtual ~PDFPluginChoiceAnnotation() { }

This is not needed.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.h:47
> +    virtual void updateGeometry() OVERRIDE;
> +    virtual void commit() OVERRIDE;

Can these be private?

[Tim Horton]

(In reply to comment #5)
> I don’t think the term “inline PDF” is a thing. Do you mean PDFs used as images?

I meant "subframe PDFs"; I will fix the terminology.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:202
> > +        annotationContainer->setInlineStyleProperty(CSSPropertyOverflow, "hidden");
> > +        annotationContainer->setInlineStyleProperty(CSSPropertyPosition, "absolute");
> > +        annotationContainer->setInlineStyleProperty(CSSPropertyPointerEvents, "none");
> > +        annotationContainer->setInlineStyleProperty(CSSPropertyTop, 0.0, CSSPrimitiveValue::CSS_PX);
> > +        annotationContainer->setInlineStyleProperty(CSSPropertyLeft, 0.0, CSSPrimitiveValue::CSS_PX);
> > +        annotationContainer->setInlineStyleProperty(CSSPropertyRight, 0.0, CSSPrimitiveValue::CSS_PX);
> > +        annotationContainer->setInlineStyleProperty(CSSPropertyBottom, 0.0, CSSPrimitiveValue::CSS_PX);
> 
> Is this more efficient than setting a hardcoded style attribute string? Doing that would avoid the need to cast to StyledElement* and cut down a lot on the lines of code.

I've put what I can in a <style> attached to the animation container. Not sure if I should put that in a separate file, right now it's just constructed from a static string in PDFPlugin.mm.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:57
> > +    virtual void updateGeometry();
> > +    virtual void commit() = 0;
> 
> Can these be private or protected?

They can't be private, as PDFPlugin needs to call them! Is there a reason to make them protected and make PDFPlugin a friend of PDFPluginAnnotation rather than leaving them public? I've not heard any clear policy here.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:70
> > +    virtual PassRefPtr<WebCore::Element> createAnnotationElement() = 0;
> 
> Can this be private instead of protected?

Yep.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.h:83
> > +         bool operator==(const EventListener& listener) OVERRIDE { return this == &listener; }
> 
> Need virtual keyword here.

Weird! I thought override complained when the method wasn't virtual. Weird weird weird.

New patch shortly.

[Tim Horton]

Created attachment 170465 [details]
addressing most of Darin's comments

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/Plugins/PDF/PDFLayerControllerDetails.h:44:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 17 files


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

Comment on attachment 170465 [details]
addressing most of Darin's comments

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

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.h:53
>  class PluginView;
> +class PDFPluginAnnotation;

D comes before l.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:42
> +#import <WebCore/CSSPrimitiveValue.h>
> +#import <WebCore/CSSPropertyNames.h>

Uppercase S comes before lowercase H.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:58
> +#import "PDFLayerControllerDetails.h"

This should go after #import "PDFKitImports.h" in the main #import block.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:75
> +#annotationContainer > .annotation { \

Is it necessary to make this selector this complicated? Would .annotation not work by itself?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:79
> +#annotationContainer > textarea.annotation { \

How about textarea.annotation here?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:217
> +        m_annotationContainer->appendChild(m_annotationStyle.get(), ec);
> +        document->body()->appendChild(m_annotationContainer.get(), ec);

Why not use the ASSERT_NO_EXCEPTION default?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:78
> +    m_parent->appendChild(m_element, ec);

Can’t use the default ASSERT_NO_EXCEPTION?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:87
> +    m_element->removeEventListener("change", m_eventListener.get(), false);
> +    m_element->removeEventListener("blur", m_eventListener.get(), false);

Can you import EventNames.h and use eventNames().{change,blur}Event?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:90
> +    m_parent->removeChild(element(), ec);

…?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:96
> +    IntSize documentSize([m_pdfLayerController contentSizeRespectingZoom]);
> +    IntPoint scrollPosition([m_pdfLayerController scrollPosition]);

I’d use = and property notation here.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:108
> +    if (event->type() == eventNames().blurEvent || event->type() == eventNames().changeEvent)

Apparently you can use eventNames!

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:33
> +#import "PDFKitImports.h"
> +#import <PDFKit/PDFKit.h>
> +#import "PDFLayerControllerDetails.h"

Unsorted.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:35
> +#import <WebCore/ColorMac.h>
> +#import <WebCore/CSSPrimitiveValue.h>

S < o

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:59
> +    styledElement->setInlineStyleProperty(CSSPropertyFontSize, [[choiceAnnotation() font] pointSize] * [pdfLayerController() tileScaleFactor], CSSPrimitiveValue::CSS_PX);

Property notation for pointSize?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:76
> +    styledElement->setInlineStyleProperty(CSSPropertyColor, colorFromNSColor([choiceAnnotation fontColor]).serialized());

.fontColor?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:77
> +    styledElement->setInlineStyleProperty(CSSPropertyFontFamily, [[choiceAnnotation font] familyName]);

.font.familyName?

What about things like font weight and style?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:83
> +    for (NSUInteger i = 0, count = choices.count; i < count; ++i) {

Please use some modern way to enumerate an NSArray (fast enumeration or using block).

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:41
> +#import "PDFKitImports.h"
> +#import <PDFKit/PDFKit.h>
> +#import "PDFLayerControllerDetails.h"
> +#import <WebCore/ColorMac.h>
> +#import <WebCore/CSSPrimitiveValue.h>
> +#import <WebCore/CSSPropertyNames.h>
> +#import <WebCore/HTMLElement.h>
> +#import <WebCore/HTMLInputElement.h>
> +#import <WebCore/HTMLNames.h>
> +#import <WebCore/HTMLTextAreaElement.h>
> +#import <WebCore/Page.h>

:-\

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:64
> +    default:

Not case NSNaturalTextAlignment?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:65
> +        return "inherit";

Not -webkit-start?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:90
> +    styledElement->setInlineStyleProperty(CSSPropertyColor, colorFromNSColor([textAnnotation fontColor]).serialized());
> +    styledElement->setInlineStyleProperty(CSSPropertyFontFamily, [[textAnnotation font] familyName]);
> +    styledElement->setInlineStyleProperty(CSSPropertyTextAlign, cssAlignmentValueForNSTextAlignment([textAnnotation alignment]));

Less brackets, more dots.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:91
> +

What about setting the dir attribute or the direction CSS property?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:95
> +    if (isMultiline)
> +        static_cast<HTMLTextAreaElement*>(styledElement)->setValue([textAnnotation stringValue]);
> +    else
> +        static_cast<HTMLInputElement*>(styledElement)->setValue([textAnnotation stringValue]);

More dots.

[Tim Horton]

Thanks for the review, Dan!

(In reply to comment #9)
> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginAnnotation.mm:96
> > +    IntSize documentSize([m_pdfLayerController contentSizeRespectingZoom]);
> > +    IntPoint scrollPosition([m_pdfLayerController scrollPosition]);
> 
> I’d use = and property notation here.

It seems I then need to use the IntSize/Point constructor explicitly? That seems uglier.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:64
> > +    default:
> 
> Not case NSNaturalTextAlignment?

Hmm, sure.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:65
> > +        return "inherit";
> 
> Not -webkit-start?

What?

> What about setting the dir attribute or the direction CSS property?

I don't know where I'd get the direction from. I wonder if PDF supports that (it must!).

[Tim Horton]

(In reply to comment #9)
> What about things like font weight and style?

I am less sure of exactly how to translate from NSFont properties to CSS here. I'll add a comment and come back to that.

[Tim Horton]

http://trac.webkit.org/changeset/132992

Thanks, Dan!