[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


mitz at webkit.org <mitz at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #170465|review?                     |review+
               Flag|                            |




--- Comment #9 from mitz at webkit.org <mitz at webkit.org>  2012-10-30 23:15:13 PST ---
(From update of attachment 170465)
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.

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