[webkit-reviews] review granted: [Bug 183969] Use SecurityOriginData more consistently in Service Worker code : [Attachment 336454] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 183969: Use SecurityOriginData more consistently in Service Worker code
https://bugs.webkit.org/show_bug.cgi?id=183969

Attachment 336454: Patch

https://bugs.webkit.org/attachment.cgi?id=336454&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 336454
  --> https://bugs.webkit.org/attachment.cgi?id=336454
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=336454&action=review

> Source/WebCore/ChangeLog:17
> +	   * page/SecurityOriginData.cpp:
> +	   (WebCore::SecurityOriginData::toString const):
> +	   Update the SecurityOriginData::toString() implementation to match
the one in
> +	   SecurityOrigin. In particular, in the common case where there is no
port, the
> +	   string is now https://www.webkit.org, not https://www.webkit.org:0.

This comment is slightly misleading. The function does *not* match
SecurityOrigin::toRawString in the case where m_protocol is "file".

> Source/WebCore/page/SecurityOriginData.cpp:64
> -    return makeString(protocol, "://", host, ":",
String::number(port.value_or(0)));
> +    StringBuilder result;
> +    result.reserveCapacity(protocol.length() + host.length() + 10);
> +    result.append(protocol);
> +    result.appendLiteral("://");
> +    result.append(host);
> +
> +    if (port) {
> +	   result.append(':');
> +	   result.appendNumber(port.value());
> +    }
> +
> +    return result.toString();

Here’s another way to write it that is simpler and more efficient:

    if (!port)
	return makeString(protocol, "://", host);
    return makeString(protocol, "://", host, ':', *port);

We should change SecurityOrigin::toRawString to do it that way, I think. Should
we eventually make SecurityOrigin use a SecurityOriginData data member instead
of m_protocol, m_host, and m_port? If so, then SecurityOrigin::toRawString
could actually call this function.

> Source/WebCore/page/SecurityOriginData.h:29
> +#include "URL.h"
>  #include <wtf/text/WTFString.h>

Can delete the include of WTFString.h since we are adding the include of URL.h.

> Source/WebCore/workers/service/server/SWOriginStore.h:30
> +#include "SecurityOriginData.h"

Can remove the include of WTFString.h since we are adding the include of
SecurityOriginData.h.

> Source/WebCore/workers/service/server/SWOriginStore.h:32
>  #include <wtf/text/StringHash.h>

We can remove this include too since we aren’t hashing strings here any more.