[Webkit-unassigned] [Bug 117497] Improve algorithm of removing user and pass, by seperating new function

[bugzilla-daemon at webkit.org]

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


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #204298|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #5 from Darin Adler <darin at apple.com>  2013-06-12 10:14:04 PST ---
(From update of attachment 204298)
View in context: https://bugs.webkit.org/attachment.cgi?id=204298&action=review

I’m not sure about this patch. Improving performance of a hot code path seems well worth it. If this code path is hot. The changes to setUser and setPass seem to be unmotivated; they remove the optimization for the empty string, without increasing code clarity significantly. I’d just leave those functions alone.

Since we are proposing this patch as a performance optimization, I would love to see data about whether a speedup is measurable. I’m not sure it’s worth making changes here that add a new function and bit more code unless there is a real benefit.

I’m going to say review- because the benefit seems so marginal. But I could be convinced to change my mind. It is more elegant to remove both of these at once, but I’d prefer not to expand the number of functions in the KURL class except to do something truly worthwhile. There are only two call sites for this!

> Source/WebCore/platform/KURL.cpp:776
> +    parse(m_string.left(m_userStart) + u + m_string.substring(end));

This removes the small performance optimization we used to have, where we don’t re-parse if we are not making a change. Please don’t remove that in a patch that is supposed to be about optimization. Perhaps you rationale is that you think the only call sites that needed this optimization were the same ones that are calling the new function. But still, I don’t think this change is a significant improvement.

> Source/WebCore/platform/KURL.cpp:800
> +    parse(m_string.left(m_userEnd) + p + m_string.substring(end));

Same issue here.

> Source/WebCore/platform/KURL.cpp:802
> +}
> +void KURL::removeUserPass()

For a new function, I suggest we use naming without abbreviation: removeUserAndPassword.

Also, need a blank line here.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:137
>      if (m_url.user().isEmpty() && m_url.pass().isEmpty())
>          return;

Since the idea is to improve performance, I suggest we write this in a way that avoids having to build a string for the user and another string for password just to check if they are present. The way to do that would be to add a boolean return value from removeUserAndPassword and return true if anything was removed.

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