[webkit-reviews] review denied: [Bug 23477] Support for WCSS extensions : [Attachment 27397] wap input format support

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has denied Sreedhar Vaddi
<sreedhar.vaddi at nokia.com>'s request for review:
Bug 23477: Support for WCSS extensions
https://bugs.webkit.org/show_bug.cgi?id=23477

Attachment 27397: wap input format support
https://bugs.webkit.org/attachment.cgi?id=27397&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
The majority of this code seems misplaced.  There doesn't seem to be anything
Qt-specific about it, yet it's in Qt-specific files. In the WebKit directory no
less.  Code that deals with parsing of the values of CSS properties should live
in WebKit rather than WebCore, and should not be reimplemented for each
platform.

> +#if ENABLE(WCSS)
> +    bool createFormatMask();
> +    bool cancleEditing();
> +    bool validateFormatText();
> +    void updateEditingMode();
> +#endif

There's a typo in here.


> +// END OF FILE

This isn't a particularly useful comment.


> +typedef unsigned short int UChar;
> +

This header shouldn't be redeclaring UChar.

> +typedef enum {
> +    ELeLoSymPuc,
> +    ELeUpSymPuc,   
> +    ENumSymPuc,    
> +    ENumChar,      
> +    ELeLoNumSymPuc,
> +    ELeUpNumSymPuc,
> +    EAnyLow,
> +    EAnyUpper,
> +    EStatic,
> +    ENoFormat 
> +}InputFormatMaskType;

We don't typically prefix our enum values with E.
  
> Index: WebCore/page/FocusController.cpp
> ===================================================================
> --- WebCore/page/FocusController.cpp	(revision 9761)
> +++ WebCore/page/FocusController.cpp	(working copy)
> @@ -31,6 +31,9 @@
>  #include "Document.h"
>  #include "Editor.h"
>  #include "EditorClient.h"
> +#if ENABLE(WCSS)
> +#include "EditorClientQt.h"
> +#endif
>  #include "Element.h"
>  #include "Event.h"
>  #include "EventHandler.h"
> @@ -257,7 +260,11 @@
>	   
>      if (oldFocusedNode && oldFocusedNode->rootEditableElement() ==
oldFocusedNode && !relinquishesEditingFocus(oldFocusedNode))
>	   return false;
> -	   
> +#if ENABLE(WCSS)	   
> +   
if(!(static_cast<EditorClientQt*>(m_page->editorClient())->validateFormatText()
))
> +	   return false;
> +#endif

This is totally incorrect.  It's obviously broken on non-Qt platforms, but on a
deeper level it's not correct for WebCore to access types declared in WebKit. 
It's a layering violation.

> @@ -40,6 +44,9 @@
>  class CSSStyleSelector;
>  class StyleFlexibleBoxData;
>  class StyleMarqueeData;
> +#if ENABLE(WCSS)
> +class StyleWapInput;
> +#endif

Please don't put an #if'd block in the middle of a sorted list like this.  Add
it at the end.

>  class StyleMultiColData;
>  class StyleReflection;
>  class StyleTransformData;

> Index: WebCore/rendering/style/StyleWapInput.h
> ===================================================================
> --- WebCore/rendering/style/StyleWapInput.h	(revision 0)
> +++ WebCore/rendering/style/StyleWapInput.h	(revision 0)
> @@ -0,0 +1,32 @@
> +#ifndef  StyleWapInput_h 
> +#define  StyleWapInput_h 
> +
> +#include "AtomicString.h"
> +#include <wtf/RefCounted.h>
> +#include <wtf/PassRefPtr.h>
> +
> +namespace WebCore {
> +
> +class StyleWapInput : public RefCounted<StyleWapInput> {
> +public:
> +    static PassRefPtr<StyleWapInput> create() { return adoptRef(new
StyleWapInput); }
> +    PassRefPtr<StyleWapInput> copy() const { return adoptRef(new
StyleWapInput(*this)); }

Is this copy method ever used?	It seems inconsistent with the code below.

> +private:
> +    StyleWapInput();
> +    StyleWapInput(const StyleWapInput&);
> +};

If this is intended to prevent copying then it is not needed.  RefCounted
already inherits from WTF::Noncopyable.

> Index: WebCore/rendering/style/StyleWapInput.cpp
> ===================================================================
> --- WebCore/rendering/style/StyleWapInput.cpp (revision 0)
> +++ WebCore/rendering/style/StyleWapInput.cpp (revision 0)
> @@ -0,0 +1,19 @@
> +#include "config.h"
> +#include "StyleWapInput.h" 
> +
> +namespace WebCore {
> +
> +StyleWapInput::StyleWapInput() : 
> +		  required(false)
> +{
> +
> +}
> +
> +StyleWapInput::StyleWapInput(const StyleWapInput& o) :
> +		  RefCounted<StyleWapInput>(),
> +		  format(o.format),
> +		  required(o.required)

Please see our other files (eg, StyleRareNonInheritedData.cpp above) for how we
format our initialisation lists.