[webkit-reviews] review denied: [Bug 131637] Clean up unnecessary methods in the BackForwardClient interface : [Attachment 229919] the patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Brian Burg <burg at cs.washington.edu>'s
request for review:
Bug 131637: Clean up unnecessary methods in the BackForwardClient interface
https://bugs.webkit.org/show_bug.cgi?id=131637

Attachment 229919: the patch
https://bugs.webkit.org/attachment.cgi?id=229919&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=229919&action=review


Thanks for tackling this!

> Source/WebCore/history/BackForwardClient.h:31
> -#include <wtf/Forward.h>
> -#include <wtf/RefCounted.h>
> +#include <wtf/PassRefPtr.h>

Why include PassRefPtr.h instead of Forward.h?

> Source/WebCore/history/BackForwardController.h:42
> +    BackForwardController(Page&, BackForwardClient*);

This should take a std::unique_ptr, not a raw pointer, since this object needs
to take ownership.

> Source/WebCore/history/BackForwardController.h:71
> +    BackForwardClient* m_client;

This needs to be a std::unique_ptr, otherwise the client will never be
destroyed.

> Source/WebCore/history/BackForwardList.h:44
> +    explicit BackForwardList(Page*);

Can this take a reference instead of a pointer?

> Source/WebCore/history/BackForwardList.h:84
>      Page* m_page;

Can this be a reference instead of a pointer?

> Source/WebCore/page/Page.cpp:1605
> +    , backForwardClient(nullptr)

We should not make this change.

> Source/WebCore/page/Page.h:140
> +	   BackForwardClient* backForwardClient;

We should make this a std::unique_ptr.

> Source/WebKit/mac/History/WebBackForwardList.mm:116
> +    return [self initWithBackForwardList:new BackForwardList(0)];

This is no good. It leaks the BackForwardList. Someone needs to own it. We’ll
probably have to leave it RefCounted if we can’t figure out a single-ownership
model.

> Source/WebKit/mac/History/WebBackForwardListInternal.h:39
> +- (id)initWithBackForwardList:(WebCore::BackForwardList*)backForwardList;

Not sure this makes sense; what’s the lifetime relationship between the
WebBackForwardList and the WebCore::BackForwardList? I think we might have to
go back to making it RefCounted.

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h:39
> +    explicit WebBackForwardListProxy(WebPage*);

Can this take a reference instead of a pointer?

> Source/WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h:64
>      WebPage* m_page;

Can this be a reference instead of a pointer?

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:322
> +    pageClients.backForwardClient = new WebBackForwardListProxy(this);

Where is the code to delete this? For the drag client, for example, the
function that deletes it is WebDragClient::dragControllerDestroyed. But it
looks like this just leaks. I suggest using std::make_unique here.