[Webkit-unassigned] [Bug 100155] Full-page PDFPlugin should support inline form editing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=100155





--- Comment #5 from Darin Adler <darin at apple.com>  2012-10-23 17:21:51 PST ---
(From update of attachment 170270)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.