[Webkit-unassigned] [Bug 22443] XMLHttpRequest's PreflightResultCache should be thread-safe.

[bugzilla-daemon at webkit.org]

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


ap at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25473|review?(ap at webkit.org)      |review-
               Flag|                            |




------- Comment #10 from ap at webkit.org  2008-11-25 02:01 PDT -------
(From update of attachment 25473)
-    PassRefPtr<StringImpl> copy();
+    PassRefPtr<StringImpl> copy(unsigned pos = 0, unsigned len = UINT_MAX);

I'm not convinced this is a good change. Constructing a String from a substring
makes a copy anyway, so it's jst confusing that we have another way to achieve
the same result.

On the other hand, this could help if we ever decided to make Strings share
buffers like UStrings do, but it would be a tough discipline to always use
copy() just for this, even where it's not currently needed.

+    bool ParseResponse(const ResourceResponse& response);

Method names should start with lower case letter.

Also, we omit parameter names from declarations where they do not provide any
additional information. This is an issue in many declarations below, too.

+    bool isCrossSiteMethodAllowedByItem(const String& method);

Making these PreflightResultCacheItem methods was a great idea, it really
helps! But this reads as if the method parameter were an item. I suggest
calling this "allowsCrossSiteMethod".

+    bool areCrossSiteHeadersAllowedByItem(const HTTPHeaderMap&
requestHeaders);
+    bool isRequestAllowedByItem(bool includeCredentials, const String& method,
const HTTPHeaderMap& requestHeaders);

Same concern - what about "allowsCrossSiteHeaders" and "allowsRequest"?

+bool PreflightResultCacheItem::parseAccessControlAllowList(const String&
string, HashSet<String, HashType>* set)
+{
+    ASSERT(set);

Now that m_methods is not an OwnPtr, I think you could change the method to
take a reference.

These are extremely minor nitpicks, and the patch is very close to r+, but we
need one more quick round, at least for ParseResponse.


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