Bug 164535 – Introduce PageSchemeRegistry

• Status: NEW
• Component: New Bugs

[Alex Christensen]

Introduce PageSchemeRegistry

[Alex Christensen]

Created attachment 294202 [details]
Patch

[WebKit Commit Bot]

Attachment 294202 [details] did not pass style-queue:


ERROR: Source/WebCore/page/PageSchemeRegistry.h:37:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 222 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 294203 [details]
Patch

[Daniel Bates]

Comment on attachment 294203 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No change in behavior. Just introduced a stub.

This description is insufficient. Neither the description for this bug nor any of the ChangeLog files describe what this change is about and its motivation. Although this change is introducing a stub it is a step towards a larger architecture change and we should describe in these ChangeLog files what is PageSchemeRegistry, who creates/owns it, and how other fundamental classes (e.g. Document) relate to it or make use of it. We should also describe the motivation for this change, including why SchemeRegistry no longer works well in the post-WebKit2/Network process world.

> Source/WebCore/dom/Document.h:1305
> +    Document(Frame*, const URL&, const PageSchemeRegistry&, unsigned = DefaultDocumentClass, unsigned constructionFlags = 0);

Can you please elaborate on this? In particular, can you please elaborate on the lifetime of the Document's PageSchemeRegistry with respect to the lifetime of the Frame object associated with the Document.

> Source/WebCore/loader/DocumentLoader.cpp:1137
>      m_archive = archive;

We are underutilizing the purpose of passing by rvalue reference to remove unnecessary ref count churn. This will cause ref count churn. We should WTFMove() archive into m_archive.

> Source/WebCore/loader/FrameLoader.cpp:894
> +void FrameLoader::loadArchive(RefPtr<Archive>&& archive)

We should WTFMove() archive into Document::setArchive() below. Otherwise, we are are underutilizing the properties of passing by rvalue reference.

> Source/WebCore/page/SecurityOrigin.cpp:273
> +bool SecurityOrigin::canRequest(const URL& url, const PageSchemeRegistry& schemeRegistry) const

How did you come to the decision to pass a scheme registry to the SecurityOrigin when invoking a member function as opposed to using the scheme registry that was used to instantiate the SecurityOrigin? Are there cases where we want to pass the scheme registry for a document B when querying the security origin of document A? (Maybe this is answered if I keep reading through this patch?)

> Source/WebCore/page/SecurityOrigin.cpp:328
> +bool SecurityOrigin::canDisplay(const URL& url, const PageSchemeRegistry& schemeRegistry) const

Ditto.