Bug 144631 – Merge CachedFrameBase into CachedFrame

• Status: ASSIGNED
• Component: Page Loading

[Chris Dumez]

Merge CachedFrameBase into CachedFrame. There is no reason to split this into 2 classes since CachedFrame is the only subclass of CachedFrameBase.

[Chris Dumez]

Created attachment 252388 [details]
Patch

[Daniel Bates]

Comment on attachment 252388 [details]
Patch

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

> Source/WebCore/history/CachedFrame.cpp:135
> +    , m_url(frame.document()->url())

[1] This line assumes that frame.document() is a non-null pointer.

> Source/WebCore/history/CachedFrame.cpp:137
> +    , m_isComposited(frame.view()->hasCompositedContent())

[2] This line assumes that frame.view() is a non-null pointer.

> Source/WebCore/history/CachedFrame.h:45
>      Document* document() const { return m_document.get(); }

I know you didn't write this code. The use of const in this function declaration is disingenuous since the underlying Document object can be modified through the pointer. There is an unwritten convention [3] for non-container type data structures of making functions that return non-const raw pointers be non-const. Can we make this function be non-const? Or can we make this function return a const Document*? Or even better, can we make this function return a reference by [1]?

[3] "On returning mutable pointers from const methods", <https://lists.webkit.org/pipermail/webkit-dev/2012-October/022576.html> (start of email thread).

> Source/WebCore/history/CachedFrame.h:46
>      FrameView* view() const { return m_view.get(); }

Similarly, can we make this return a reference by [2]? Or make this function be non-const? Or make this function return a const FrameView*?

> Source/WebCore/history/CachedFrame.h:63
> +    RefPtr<Document> m_document;

We should make this a Ref instead of a RefPtr because of [1].

> Source/WebCore/history/CachedFrame.h:65
> +    RefPtr<FrameView> m_view;

We should make this a Ref instead of a RefPtr because of [2].

[Daniel Bates]

I'm unclear if we want this change given that CachedFrameBase was explicitly extracted from CachedFrame in <https://trac.webkit.org/changeset/47989>: "The pattern of creating a 'CachedFrameBase' as the interface CachedFrame provides to FrameLoader was suggested by Darin Adler."