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
(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.
(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.
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.
(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 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.
(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
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?
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 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.
(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.
2009-07-22 09:25 PDT, Yong Li
2009-07-22 12:43 PDT, Yong Li
2009-07-22 12:44 PDT, Yong Li
2009-07-30 13:29 PDT, Yong Li
2009-07-30 13:30 PDT, Yong Li
2009-07-30 13:30 PDT, Yong Li
2009-07-30 13:31 PDT, Yong Li
2009-07-30 13:31 PDT, Yong Li
2009-07-30 13:32 PDT, Yong Li
2009-07-30 13:33 PDT, Yong Li
2009-07-30 13:33 PDT, Yong Li
2009-08-08 10:47 PDT, Yong Li
2009-08-08 10:50 PDT, Yong Li
2009-08-08 10:53 PDT, Yong Li
2009-08-08 10:58 PDT, Yong Li
2009-08-08 11:02 PDT, Yong Li
2009-08-08 11:07 PDT, Yong Li
2009-08-08 11:12 PDT, Yong Li
2009-08-08 11:16 PDT, Yong Li
2009-08-08 11:20 PDT, Yong Li
2009-08-08 11:25 PDT, Yong Li
2009-08-08 11:30 PDT, Yong Li
2009-08-08 11:32 PDT, Yong Li
2009-08-08 11:36 PDT, Yong Li
2009-08-08 11:38 PDT, Yong Li
2009-08-08 11:41 PDT, Yong Li
2009-08-11 14:10 PDT, Yong Li
2009-08-11 15:17 PDT, Yong Li
2009-08-12 11:13 PDT, Yong Li
2009-08-12 12:35 PDT, Yong Li
2009-08-12 12:52 PDT, Yong Li