|
Description
Yong Li
2009-07-22 08:49:03 PDT
Created attachment 33265 [details]
files for WINCE port in platform/wince
I have used cpplint to check coding style errors and fixed them.
Comment on attachment 33265 [details] files for WINCE port in platform/wince > +#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 Created attachment 33281 [details]
part 1 (not including ScrollView Widget and ScrollBar files)
Created attachment 33282 [details]
part 2 (ScrollView, Widget, ScrollBar files)
Comment on attachment 33282 [details]
part 2 (ScrollView, Widget, ScrollBar files)
No changelog. Also is this style checked? I didn't notice any issues so far but it's a big patch.
(In reply to comment #5) > (From update of attachment 33282 [details]) > No changelog. Also is this style checked? I didn't notice any issues so far > but it's a big patch. will clean up, and split it into more patches Created attachment 33823 [details]
1) Clipboard
Created attachment 33824 [details]
2) ScrollView
Created attachment 33825 [details]
3) Widget, Scrollbar
Created attachment 33826 [details]
4) SharedBuffer
Created attachment 33827 [details]
5) ContextMenu
Created attachment 33828 [details]
6) SharedTimer and SystemTime
Created attachment 33829 [details]
7) KeygenWince
Created attachment 33830 [details]
8) Other
Comment on attachment 33823 [details]
1) Clipboard
Too large.
(In reply to comment #15) > (From update of attachment 33823 [details]) > Too large. The ChangeLog is for all. we probably should split the ChangeLog, too. But the patch contains only 3 files for clipboard. Comment on attachment 33823 [details]
1) Clipboard
nonsense
(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 33823 [details] [details]) > > Too large. > > The ChangeLog is for all. we probably should split the ChangeLog, too. But the > patch contains only 3 files for clipboard. Yes, please split the changelog and leave the patches as they are, including the appropriate changelogs. (In reply to comment #17) > (From update of attachment 33823 [details]) > nonsense Wow. I think that's totally inappropriate behavior from a reviewer. None of these have ChangeLogs. Why are they marked for review? Created attachment 34372 [details]
1) Clipboard
Created attachment 34373 [details]
1) Clipboard
oops, written by Joe Mason
Created attachment 34374 [details]
1) Clipboard
Created attachment 34375 [details]
2) ScrollView
Created attachment 34376 [details]
3) Scrollbar & Widget
Created attachment 34378 [details]
4) SharedBuffer & OptimizedBuffer
Allow SharedBuffer to base on a segmented buffer or streaming data source.
Created attachment 34379 [details]
5) ContextMenu
Written by Joe Mason
Created attachment 34380 [details]
6) SharedTimer & SystemTime
Created attachment 34381 [details]
7) Keygen (Security)
Written by Lyon Chen
Created attachment 34382 [details]
8) Pasteboard & SearchPopupMenu
Written by Joe Mason
Created attachment 34383 [details]
8) Pasteboard & SearchPopupMenu
Oops, last one is wrong
Created attachment 34384 [details]
9) MIMETypeRegistry
Created attachment 34385 [details]
10) Cursor
Created attachment 34386 [details]
11) FileSystem and FileChooser
Comment on attachment 34381 [details] 7) Keygen (Security) For some meaningful review, >> +#include "config.h" > +#include "SSLKeyGenerator.h" > + > +#include "Base64.h" > +#include "CString.h" > + > +#include <windows.h> > +#include <wincrypt.h> Is this out of order by design? > + > +#define KEYGEN_DEBUG_KEY 0x00000001 > + > +#ifndef NDEBUG > +#define KEYGEN_DEBUG (0) > +#else > +#define KEYGEN_DEBUG 0 > +#endif This is odd. Why 0 or 0? I think we need to do better on the debug scheme. > +String WebCore::signedPublicKeyAndChallengeString(unsigned index, const String& challenge, const KURL& url) > +{ > + String keystr; > + > + HCRYPTPROV hProv = 0; > + HCRYPTKEY hKey = 0; > + PCERT_PUBLIC_KEY_INFO pPubInfo = 0; > + DWORD dwPubInfoLength = 0; > + CERT_KEYGEN_REQUEST_INFO requestInfo; > + CRYPT_ALGORITHM_IDENTIFIER signAlgo; > + LPBYTE pbEncoded = 0; > + DWORD dwEncodedLength = 0; > + LPWSTR pszString = 0; > + Vector<char> binary; > + Vector<char> base64; A good number of these can be moved to the relevant area so we don't lose track of what they're used for and if they're no-longer used. > + BOOL success = CryptAcquireContext(&hProv, _T("keygen_container"), MS_ENHANCED_PROV, PROV_RSA_FULL, CRYPT_NEWKEYSET); > + if (!success) > + goto error; Can we do better than lots of gotos? > + pPubInfo = (PCERT_PUBLIC_KEY_INFO) new char[dwPubInfoLength]; > + if (!pPubInfo) > + goto error; No need to check new return. > + if (challenge.length()) { > + pszString = new WCHAR[challenge.length() + 1]; > + if (pszString) { Same. > + pbEncoded = (LPBYTE) new char[dwEncodedLength]; > + if (!pbEncoded) > + goto error; Same > +error: > + if (pszString) > + delete[] pszString; > + if (pbEncoded) > + delete[] pbEncoded; > + if (pPubInfo) > + delete[] pPubInfo; Those if() are redundant Comment on attachment 34384 [details]
9) MIMETypeRegistry
& style is inconsistent - type& name in some places. Otherwise it's fine so r+ on condition those are fixed on checkin.
Created attachment 34387 [details]
12) Other (simple stubs to make it build)
(In reply to comment #35) > (From update of attachment 34381 [details]) > For some meaningful review, > > >> +#include "config.h" > > +#include "SSLKeyGenerator.h" > > + > > +#include "Base64.h" > > +#include "CString.h" > > + > > +#include <windows.h> > > +#include <wincrypt.h> > > Is this out of order by design? The order of headers are sometimes important on Windows. Not sure with this one, but I think it's good to put <windows.h> before <wincrypt.h> Comment on attachment 34384 [details]
9) MIMETypeRegistry
No need for the explicit String cast here:
113 return String("xml");
Comment on attachment 34387 [details]
12) Other (simple stubs to make it build)
The create* functions returning 0 might cause crashes. You might want to add notImplemented() calls to those as well.
Why is this needed?
34 #define _SYS_GUID_OPERATORS_
Please remove the _SYS_GUID_OPERATORS_ when landing.
(In reply to comment #39) > (From update of attachment 34384 [details]) > No need for the explicit String cast here: > 113 return String("xml"); thanks. but isn't it better than implicit cast? (In reply to comment #40) > (From update of attachment 34387 [details]) > The create* functions returning 0 might cause crashes. You might want to add > notImplemented() calls to those as well. > > Why is this needed? > 34 #define _SYS_GUID_OPERATORS_ > > Please remove the _SYS_GUID_OPERATORS_ when landing. thanks Comment on attachment 34385 [details]
10) Cursor
An interesting approach. :)
Indent:
41 : m_impl(other.m_impl)
46 : m_impl(CursorNone)
61 : m_impl(c)
Normally we don't use "get" in function names. I might have named it cursorForType(type) instead.
I've never seen a .cpp file use single-line function declarations like that before. But I also don't think it's harmful.
OK. Please fix the indent nit on landing.
(In reply to comment #41) > (In reply to comment #39) > > (From update of attachment 34384 [details] [details]) > > No need for the explicit String cast here: > > 113 return String("xml"); > > thanks. but isn't it better than implicit cast? The implicit construction is fewer characters to read. :) And there are already other return statements in that same function which use the implicit String(char*) constructor. Created attachment 34590 [details]
7) Keygen (Security)
refactored by Yong
(In reply to comment #45) > Created an attachment (id=34590) [details] > 7) Keygen (Security) > > refactored by Yong Are you sure? It looks the same to me. (In reply to comment #46) > (In reply to comment #45) > > Created an attachment (id=34590) [details] [details] > > 7) Keygen (Security) > > > > refactored by Yong > > Are you sure? It looks the same to me. hm... a mistake Created attachment 34602 [details]
7) KeyGen (modified)
Comment on attachment 34602 [details]
7) KeyGen (modified)
I think standard for your for(;;) is really to do:
do {
} while (0);
Best to change to that and then your comment isn't necessary. Also you have a double semicolon in your first if (). Those minor fixes and the rest is fine with me.
(In reply to comment #49) > I think standard for your for(;;) is really to do: > > do { > } while (0); for(;;) is an infinite loop. The above is not an infinite loop, it's a one time loop, so it can't be used. For infinite loops, the most common pattern in WebKit is while (true). I like for (;;) so much personally :), but seems not allowed in webkit?
Probably I'd better change it back to goto.
Another way to avoid goto is
if (...) {
if (...) {
...
// free resources
}
// free resources
}
I also like this way, but I'm afraid that someone will say there're too many indentations.
I also planed to create wrapper classes for HCRYPTKEY and HCRYPTPROV that release resource automatically, but probably it's a waste for this single usage. Sadly, OwnPtr<> doesn't work in this case because HCRYPTKEY and HCRYPTPROV are just "void*".
I considered a lot about this.
I'm going to use goto, which makes everyone understand
(In reply to comment #50) > (In reply to comment #49) > > I think standard for your for(;;) is really to do: > > > > do { > > } while (0); > > for(;;) is an infinite loop. The above is not an infinite loop, it's a one time > loop, so it can't be used. For infinite loops, the most common pattern in > WebKit is while (true). You got confused in much the same way I was trying to point out. :-) It's -never- supposed to loop. That's what the comment says at the top. That's why it should change. Created attachment 34678 [details]
7) KeygenWince.cpp fixed a bug
changed to do while(0).
also fixed a bug that pPubInfo leaked in last patch, because it's defined twice.
goto is not good because local object construction after goto doesn't compile.
Comment on attachment 34386 [details]
11) FileSystem and FileChooser
Seems like a lot of work to be done in these files still, but if we ever want to be building in svn we have to get it checked in, and what is done seems to work well enough. The FIXMEs and notImplemented can be done another day.
Comment on attachment 34379 [details] 5) ContextMenu > + m_platformDescription = new PlatformMenuItemDescriptionType; > + if (!m_platformDescription) > + return; Don't think we should be checking for null here. > +void ContextMenuItem::setSubMenu(ContextMenu* subMenu) > +{ > + if (!m_platformDescription) > + return; In fact can we remove all of these checks? Comment on attachment 34374 [details] 1) Clipboard > +static bool getWebLocData(IDataObject* dataObject, String& url, String* title) > +{ > + return false; > +} Should probably add some UNUSED_PARAM() here > + CString cf_html = "Version:0.9"; > + CString startHTML = "\nStartHTML:"; > + CString endHTML = "\nEndHTML:"; > + CString startFragment = "\nStartFragment:"; > + CString endFragment = "\nEndFragment:"; > + CString sourceURL = "\nSourceURL:"; > + > + append(result, cf_html); > + > + bool shouldFillSourceURL = !srcURL.isEmpty() && (srcURL != "about:blank"); > + > + CString startMarkup = "\n<HTML>\n<BODY>\n<!--StartFragment-->\n"; > + CString endMarkup = "\n<!--EndFragment-->\n</BODY>\n</HTML>"; all these CString should be "const" right? static too maybe? > +void ClipboardWince::clearData(const String& type) > +{ > + notImplemented(); > +} This file can use lots of UNUSED_PARAM() The rest seems fine Comment on attachment 34380 [details] 6) SharedTimer & SystemTime Clear flag as was landed in r47132. Comment on attachment 34383 [details] 8) Pasteboard & SearchPopupMenu Clear flags as landed with r47133. Comment on attachment 34380 [details]
6) SharedTimer & SystemTime
Rejecting patch 34380 from commit-queue. This patch will require manual commit.
WebKitTools/Scripts/run-webkit-tests --no-launch-safari --quiet failed with exit code 1
Comment on attachment 34384 [details] 9) MIMETypeRegistry Clear flags as landed with r47134. Created attachment 34680 [details]
5) ContextMenu (modified)
modified according George's suggestion.
we have to check m_platformDescription in other functions because it can be cleared by releasePlatformDescription
Comment on attachment 34385 [details] 10) Cursor Clear flags as landed with r47135. Comment on attachment 34386 [details] 11) FileSystem and FileChooser Clear flags as landed with r47136. Comment on attachment 34387 [details] 12) Other (simple stubs to make it build) Clearing flags as landed with r47137. Comment on attachment 34678 [details] 7) KeygenWince.cpp fixed a bug Clear flags as landed with r47139. Created attachment 34681 [details]
1) Clipboard (modified)
modified according George's suggestion
This bug is basically impossible to follow with this many patches on it. :( Comment on attachment 34378 [details]
4) SharedBuffer & OptimizedBuffer
This does not follow WebKit style:
22 #ifndef _OPTIMIZED_BUFFER_H_
Early returns?
if (OptimizedBuffer::isCreatorInstalled()) {
246 if (OptimizedBuffer* optimizedBuffer = OptimizedBuffer::createInstance(m_size, m_optimizedBuffer, true)) {
Comment on attachment 34376 [details]
3) Scrollbar & Widget
Does this not exist anywhere else in teh code base?
// stableRound rounds -0.5 to 0, where lround rounds -0.5 to -1.
41 static inline int stableRound(double d)
42 {
The ChnageLog is pretty bare.
What does the "c" mean here? (they're not const) Are you sure these follow WebKit style?
static int cHorizontalWidth[] = { 15, 11 };
61 static int cHorizontalHeight[] = { 15, 11 };
I would look up the size and have a single setFrameRec call:
if (orientation == VerticalScrollbar)
79 setFrameRect(IntRect(0, 0, cVerticalWidth[controlSize()], cVerticalHeight[controlSize()]));
80 else
81 setFrameRect(IntRect(0, 0, cHorizontalWidth[controlSize()], cHorizontalHeight[controlSize()]));
Also that would allow you to use IntRect(IntPoint(), scrollbarSize) constructor as well.
Indent:
default:
126 {
127 IntRect beforeThumbRect, thumbRect, afterThumbRect;
128 splitTrack(trackRect(), beforeThumbRect, thumbRect, afterThumbRect);
129 if (part == BackTrackPart)
Early return is preferred:
if (enabled != isEnabled()) {
164 Scrollbar::setEnabled(enabled);
this really should have its own bug, and this bug should be retired.
This bug is out of control. Please file separate bugs and re-post the patches that are still relevant. Uber bugs like this don't work very well in our processes. Comment on attachment 34375 [details]
2) ScrollView
Clearing review flags on these patches so they can be moved to separate bugs.
(In reply to comment #71) > (From update of attachment 34375 [details]) > Clearing review flags on these patches so they can be moved to separate bugs. some of them have been landed (In reply to comment #72) > some of them have been landed Even better. :) When patch land, it's helpful to clear the review flags so that other reviewers don't invest more time examining the patches. |