[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 #13 from Nikolas Zimmermann <zimmermann at kde.org>  2011-12-10 01:16:20 PST ---
(From update of attachment 118543)
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 :-)

> 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.

> 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?

> 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? :-)

> 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.

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