[webkit-reviews] review denied: [Bug 27543] Add platform/wince/ files for WINCE port : [Attachment 33265] files for WINCE port in platform/wince

[bugzilla-daemon at webkit.org]

Adam Treat <treat at kde.org> has denied Yong Li <yong.li at torchmobile.com>'s
request for review:
Bug 27543: Add platform/wince/ files for WINCE port
https://bugs.webkit.org/show_bug.cgi?id=27543

Attachment 33265: files for WINCE port in platform/wince
https://bugs.webkit.org/attachment.cgi?id=33265&action=review

------- Additional Comments from Adam Treat <treat at kde.org>
> +#include "CString.h"
> +#include "DocumentFragment.h"
> +#include "KURL.h"
> +#include "PlatformString.h"
> +#include "TextEncoding.h"
> +#include "markup.h"

Alphabetical sorting problem. Is this a bug in cpplint with capitalization?

> +static String extractURL(const String &inURL, String* title)

Decoration of 'const String &inURL' is wrong.  Another cpplint error?

> +    String url = inURL;
> +    int splitLoc = url.find('\n');
> +    if (splitLoc > 0) {

Minor nit: splitLoc should be spelled out fully: splitLocation.

> +	   if (title)
> +	       *title = url.substring(splitLoc+1);

Space between splitLoc and 1. Rule #2 of spacing.

> +//Firefox text/html
> +static FORMATETC* texthtmlFormat()
> +{
> +    static UINT cf = RegisterClipboardFormat(L"text/html");
> +    static FORMATETC texthtmlFormat = {cf, 0, DVASPECT_CONTENT, -1,
TYMED_HGLOBAL};
> +    return &texthtmlFormat;
> +}

Should be 'textHTMLFormat()' I believe.  What is the Firefox comment for?

> +    HGLOBAL cbData = GlobalAlloc(GPTR, size * sizeof(UChar));

Should be clipboardData or so.

> +    // calculate offsets
> +    const unsigned UINT_MAXdigits = 10; // number of digits in UINT_MAX in
base 10
> +    unsigned startHTMLOffset = cf_html.length() + startHTML.length() +
endHTML.length() + startFragment.length() + endFragment.length() +
(shouldFillSourceURL ? sourceURL.length() + srcURL.length() : 0) +
(4*UINT_MAXdigits);

Space around '4*UINT_MAXdigits'.

> +String getURL(IDataObject* dataObject, bool& success, String* title)
> +{
> +    return String();
> +}

Compiler doesn't complain of unused arguments?

> +#endif // ClipboardWin_h

Should be // ClipboardWince_h I think.

> +void ContextMenuItem::setSubMenu(ContextMenu* subMenu)
> +{
> +    if (m_platformDescription) {
> +	   if (m_platformDescription->subMenu !=
subMenu->platformDescription())
> +	       delete m_platformDescription->subMenu;
> +	   m_platformDescription->subMenu =
subMenu->releasePlatformDescription();
> +    }
> +}

Early return please.

> +void ContextMenuItem::setChecked(bool checked)
> +{
> +    if (m_platformDescription) {
> +	   if (checked)
> +	       m_platformDescription->state |=
PlatformMenuItemDescriptionType::CheckedState;
> +	   else
> +	       m_platformDescription->state &=
~PlatformMenuItemDescriptionType::CheckedState;
> +    }
> +}

Early return please.

> +void ContextMenuItem::setEnabled(bool enabled)
> +{
> +    if (m_platformDescription) {
> +	   if (enabled)
> +	       m_platformDescription->state |=
PlatformMenuItemDescriptionType::EnabledState;
> +	   else
> +	       m_platformDescription->state &=
~PlatformMenuItemDescriptionType::EnabledState;
> +    }
> +}

Early return please.

> +PlatformMenuDescriptionType::PlatformMenuDescriptionType()
> +: hMenu(::CreatePopupMenu())
> +, itemCount(0)
> +{
> +}

Needs indentation of member variable initialization.

> +    if (desc->type == SeparatorType) {
> +	   flags |= MF_SEPARATOR;
> +    } else {

One line if clause...

> +#if 0
> +    if (!m_platformDescription)
> +	   return;
> +
> +    // FIXME: the Windows version did this, is it necessary?
> +    MENUINFO menuInfo = {0};
> +    menuInfo.cbSize = sizeof(MENUINFO);
> +    menuInfo.fMask = MIM_STYLE;
> +    ::GetMenuInfo(m_platformDescription, &menuInfo);
> +    menuInfo.fMask = MIM_STYLE;
> +    menuInfo.dwStyle |= MNS_NOTIFYBYPOS;
> +    ::SetMenuInfo(m_platformDescription, &menuInfo);
> +#endif

--commented out code

Going to stop there.  For the next iteration can you break it up in the
following way?

Patch 1:


 * platform/wince/ClipboardUtilitiesWince.cpp: Added.
 * platform/wince/ClipboardWince.cpp: Added.
 * platform/wince/ClipboardWince.h: Added.
 * platform/wince/ContextMenuItemWince.cpp: Added.
 * platform/wince/ContextMenuWince.cpp: Added.
 * platform/wince/CursorWince.cpp: Added.
 * platform/wince/DragDataWince.cpp: Added.
 * platform/wince/DragImageWince.cpp: Added.
 * platform/wince/EditorWince.cpp: Added.
 * platform/wince/FileChooserWince.cpp: Added.
 * platform/wince/FileSystemWince.cpp: Added.
 * platform/wince/KURLWince.cpp: Added.
 * platform/wince/KeygenWince.cpp: Added.
 * platform/wince/MIMETypeRegistryWince.cpp: Added.
 * platform/wince/OptimizedBuffer.cpp: Added.
 * platform/wince/OptimizedBuffer.h: Added.
 * platform/wince/PasteboardWince.cpp: Added. 
 * platform/wince/SearchPopupMenuWince.cpp: Added.
 * platform/wince/SharedBuffer.cpp: Added.
 * platform/wince/SharedTimerWince.cpp: Added.
 * platform/wince/SystemTimeWince.cpp: Added. 

Patch 2:

The rest of the ScrollView/Widget related files?

Thanks,
Adam
I Indent as above. t