[webkit-reviews] review granted: [Bug 186820] Reduce size of WebCore::URL : [Attachment 343100] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 186820: Reduce size of WebCore::URL
https://bugs.webkit.org/show_bug.cgi?id=186820

Attachment 343100: Patch

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




--- Comment #4 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 343100
  --> https://bugs.webkit.org/attachment.cgi?id=343100
Patch

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

> Source/WebCore/platform/URL.h:225
>      bool m_isValid : 1;
>      bool m_protocolIsInHTTPFamily : 1;
>      bool m_cannotBeABaseURL : 1;

I think MSVC does not pack small-sized members if they have different types.
(maybe). I think this is why EWS is red.
If it is correct, you can avoid this behavior by changing them to `unsigned`.
At that time, we should take care of the types of these fields. For example,
URL::encode and URL::decode should take extra care.

> Source/WebCore/platform/URL.h:245
>  void URL::encode(Encoder& encoder) const

URL::encode does not encode m_cannotBeABaseURL. Is it correct?

> Source/WebCore/platform/URL.h:286
>      if (!decoder.decode(protocolIsInHTTPFamily))

URL::decode does not handle m_cannotBeABaseURL, is it correct?

> Source/WebCore/platform/URLParser.cpp:2701
> -	       m_url.m_portEnd = currentPosition(iterator);
> +	       unsigned portLength = currentPosition(iterator) -
m_url.m_hostEnd;
> +	       RELEASE_ASSERT(portLength <= URL::maxPortLength);
> +	       m_url.m_portLength = portLength;

I think `m_url.m_portLength = 0` since `iterator.atEnd()` immediately after
setting `m_url.m_hostEnd = currentPosition(iterator);`, correct?

> Source/WebCore/platform/URLParser.cpp:2724
> -		   m_url.m_portEnd = currentPosition(iterator);
> +		   unsigned portLength = currentPosition(iterator) -
m_url.m_hostEnd;
> +		   RELEASE_ASSERT(portLength <= URL::maxPortLength);
> +		   m_url.m_portLength = portLength;

Ditto.

> Source/WebCore/platform/URLParser.cpp:2789
> -	       m_url.m_portEnd = currentPosition(iterator);
> +	       unsigned portLength = currentPosition(iterator) -
m_url.m_hostEnd;
> +	       RELEASE_ASSERT(portLength <= URL::maxPortLength);
> +	       m_url.m_portLength = portLength;

Ditto.

> Source/WebCore/platform/URLParser.cpp:2803
> -    m_url.m_portEnd = currentPosition(iterator);
> +    unsigned portLength = currentPosition(iterator) - m_url.m_hostEnd;
> +    RELEASE_ASSERT(portLength <= URL::maxPortLength);
> +    m_url.m_portLength = portLength;

Ditto.