Bug 114938 – PDFPlugin: Support unlocking encrypted PDFs

• Component: PDF
• Status: RESOLVED FIXED

[Tim Horton]

<rdar://problem/12872888>

[Tim Horton]

Created attachment 199002 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 199002 [details]
patch

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

> Source/WebKit2/ChangeLog:30
> +        Try to unlock the PDF with the given password. If this succeeds, remove
> +        the password field and relayout the scroll view.

Is there any feedback when the password is incorrect?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:96
> +"    -webkit-text-security: disc;"

Does this actually enable secure text input, or do we need an <input type=password> for that?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:352
> +        m_passwordField = 0;

nullptr

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:49
> +    virtual bool handleEvent(WebCore::Event*) OVERRIDE;

I'm a bit suspicious of the many public handleEvent functions in this patch. Most of the time, I expect functions with OVERRIDE to be private.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:58
> +    WTF::String value() const;

s/WTF:://

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:100
> +    if (textAnnotation) {

Can this be an early return instead?

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:125
> +    this->textAnnotation().stringValue = value();

I don't this that you need "this->" here any more.

[Tim Horton]

(In reply to comment #2)
> (From update of attachment 199002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199002&action=review
> 
> > Source/WebKit2/ChangeLog:30
> > +        Try to unlock the PDF with the given password. If this succeeds, remove
> > +        the password field and relayout the scroll view.
> 
> Is there any feedback when the password is incorrect?

Yes, but it's underneath us.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:96
> > +"    -webkit-text-security: disc;"
> 
> Does this actually enable secure text input, or do we need an <input type=password> for that?

I... don't know! I'll find out.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:352
> > +        m_passwordField = 0;
> 
> nullptr

Oooh la la.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:49
> > +    virtual bool handleEvent(WebCore::Event*) OVERRIDE;
> 
> I'm a bit suspicious of the many public handleEvent functions in this patch. Most of the time, I expect functions with OVERRIDE to be private.

Err, yeah, you're right.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.h:58
> > +    WTF::String value() const;
> 
> s/WTF:://
>
> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:100
> > +    if (textAnnotation) {
> 
> Can this be an early return instead?
> 
> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginTextAnnotation.mm:125
> > +    this->textAnnotation().stringValue = value();
> 
> I don't this that you need "this->" here any more.

Quite.

[Tim Horton]

(In reply to comment #3)
> (In reply to comment #2)
> > > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:96
> > > +"    -webkit-text-security: disc;"
> > 
> > Does this actually enable secure text input, or do we need an <input type=password> for that?
> 
> I... don't know! I'll find out.

Looks like not, so I'll override createAnnotationElement() as well and make an <input type=password>.

[Alexey Proskuryakov]

Maybe we should fix -webkit-text-security instead (and not necessarily right now).

[Tim Horton]

(In reply to comment #5)
> Maybe we should fix -webkit-text-security instead (and not necessarily right now).

Filed https://bugs.webkit.org/show_bug.cgi?id=114979 about that but I'm not planning on working on it.

[Tim Horton]

Created attachment 199106 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 199106 [details]
patch

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

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginPasswordField.mm:28
> +#if ENABLE(PDFKIT_PLUGIN)
> +
> +#import "config.h"

I think that ENABLE macros won't work before config.h.

> Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginPasswordField.mm:43
> +using namespace WebCore;
> +
> +namespace WebKit {
> +
> +using namespace HTMLNames;

Funny that using declarations are in different places.

[Alexey Proskuryakov]

s/declarations/directives/

[Tim Horton]

(In reply to comment #8)
> (From update of attachment 199106 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=199106&action=review
> 
> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginPasswordField.mm:28
> > +#if ENABLE(PDFKIT_PLUGIN)
> > +
> > +#import "config.h"
> 
> I think that ENABLE macros won't work before config.h.

And yet they do. Will move it.

> > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginPasswordField.mm:43
> > +using namespace WebCore;
> > +
> > +namespace WebKit {
> > +
> > +using namespace HTMLNames;
> 
> Funny that using declarations are in different places.

Oops!

Thanks!

[Tim Horton]

https://trac.webkit.org/changeset/148912