[Webkit-unassigned] [Bug 73533] Upstream the Blackberry porting of multipart delegate

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73533





--- Comment #14 from Chris.Guan <chris.guan at torchmobile.com.cn>  2011-12-11 18:06:01 PST ---
(In reply to comment #13)
> (From update of attachment 118543 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118543&action=review
> 
> Hm, I'm stopping my initial review with a question: Do we have the time to refactor this right now to avoid std::string. I don't want to delay the initial upstreaming, hence I'm asking. If we agree to clean it up later, no problem with me, but maybe with others :-)
I sent you an email to explain it. I think we can keep std::string in initial upstream and refactor later.:)
> 
> > Source/WebCore/ChangeLog:9
> > +        Test cases:
> > +        http://avhs.axis.com
> 
> This needs a more detailed ChangeLog, at least one or two descriptive lines. Also listing this testcase doesn't help a lot.
yes, it needs a username and password, I am not sure that I can mark them in it. So I may remove this test cases in initial upstream. agree? 

> 
> > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:4
> > +// Copyright (C) 2011 Research In Motion Limited. All rights reserved.
> > +// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
> > +// Use of this source code is governed by a BSD-style license that can be
> > +// found in the LICENSE file.
> 
> We normally include licenses inline - why is it different here? Can you change it?
my multipart code referred to Google chromium's code, so I kept the license. please instruct me how to change it? 
> 
> > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:15
> > +static bool compareChar(char x, char y)
> 
> For example: s/compareChar/doCharactersMatchCaseInsensitive/. Anything descriptive should be used, compareChar is mystic, what does it do if you read this name on a call-site? :-)
yes, agree, thanks.
> 
> > Source/WebCore/platform/network/blackberry/MultipartResponseDelegate.cpp:24
> > +static void stringToLowerASCII(std::string& s)
> > +{
> > +    for (std::string::iterator i = s.begin(); i != s.end(); ++i)
> > +        *i = toASCIILower(*i);
> > +}
> 
> This looks very slow.
> IMHO the whole code should move away from std::string. I'd highly recommend to convert this to WTF::String natively.
Yes, refactor later.

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