Bug 100155 - Full-page PDFPlugin should support inline form editing
Summary: Full-page PDFPlugin should support inline form editing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: PDF (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-23 13:01 PDT by Tim Horton
Modified: 2012-10-31 00:56 PDT (History)
6 users (show)

See Also:


Attachments
patch (63.84 KB, patch)
2012-10-23 17:02 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
rebase patch (64.42 KB, patch)
2012-10-23 17:17 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
addressing most of Darin's comments (64.37 KB, patch)
2012-10-24 13:54 PDT, Tim Horton
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2012-10-23 13:01:36 PDT
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.
Comment 1 Radar WebKit Bug Importer 2012-10-23 13:02:10 PDT
<rdar://problem/12558511>
Comment 2 Tim Horton 2012-10-23 17:02:35 PDT
Created attachment 170270 [details]
patch
Comment 3 Tim Horton 2012-10-23 17:17:05 PDT
Created attachment 170273 [details]
rebase patch
Comment 4 WebKit Review Bot 2012-10-23 17:20:54 PDT
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.
Comment 5 Darin Adler 2012-10-23 17:21:51 PDT
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?
Comment 6 Tim Horton 2012-10-24 13:35:36 PDT
(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.
Comment 7 Tim Horton 2012-10-24 13:54:55 PDT
Created attachment 170465 [details]
addressing most of Darin's comments
Comment 8 WebKit Review Bot 2012-10-24 13:57:51 PDT
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 9 mitz 2012-10-30 23:15:13 PDT
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.
Comment 10 Tim Horton 2012-10-30 23:44:40 PDT
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!).
Comment 11 Tim Horton 2012-10-31 00:29:32 PDT
(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.
Comment 12 Tim Horton 2012-10-31 00:56:03 PDT
http://trac.webkit.org/changeset/132992

Thanks, Dan!