[webkit-reviews] review granted: [Bug 100155] Full-page PDFPlugin should support inline form editing : [Attachment 170465] addressing most of Darin's comments

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has granted Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 100155: Full-page PDFPlugin should support inline form editing
https://bugs.webkit.org/show_bug.cgi?id=100155

Attachment 170465: addressing most of Darin's comments
https://bugs.webkit.org/attachment.cgi?id=170465&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
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.