[webkit-reviews] review denied: [Bug 117497] Improve algorithm of removing user and pass, by seperating new function : [Attachment 204298] Improve algorithm of removing user and pass, by seperating removeUserPass()

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Meeyoung Kim <meeyoung.kim at lge.com>'s
request for review:
Bug 117497: Improve algorithm of removing user and pass, by seperating new
function
https://bugs.webkit.org/show_bug.cgi?id=117497

Attachment 204298: Improve algorithm of removing user and pass, by seperating
removeUserPass()
https://bugs.webkit.org/attachment.cgi?id=204298&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.