Bug 87002 – SecurityOrigin returns 0 as port when it is the default port for the protocol

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Thiago Marcos P. Santos]

It should return the correct value instead, like 80 for http, 443 for https, etc.

[Thiago Marcos P. Santos]

Created attachment 143015 [details]
Patch

[Build Bot]

Comment on attachment 143015 [details]
Patch

Attachment 143015 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12738424

[Thiago Marcos P. Santos]

Created attachment 143032 [details]
Patch

[Build Bot]

Comment on attachment 143032 [details]
Patch

Attachment 143032 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12732663

[Adam Barth]

Comment on attachment 143032 [details]
Patch

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

Please add an API test that shows the change in behavior.  These tests are easy to write for Chromium, but you should feel free to write one for a different port if you prefer.

Would you be willing to list in this bug which APIs are changing in which port?  There's some risk of a compatibility problem.  For example, if some client of the API is trying to construct storage identifiers manually, they might get a different answer after this patch...

> Source/WebCore/WebCore.order:6589
> +__ZNK7WebCore14SecurityOrigin4portEv

You probably need to add this line to WebCore.exp.in to fix the apple-mac build.

> Source/WebCore/page/SecurityOrigin.h:68
> -    unsigned short port() const { return m_port; }
> +    unsigned short port() const;

I wonder if we should change the name of this function so it doesn't look like an accessor for m_port.  Perhaps we should call it portNumber()?

[Adam Barth]

It's still slightly unclear to me whether this patch is solving a real problem.  We need to balance the risk of changing an API against the benefit of doing so.  I don't mind changing this in the Chromium API, but other folks might be more conservative about how they'd like their APIs to evolve.

[Thiago Marcos P. Santos]

Created attachment 143047 [details]
Patch

[Thiago Marcos P. Santos]

It is too risky to change this method. Should be fixed on the API layer if needed.