[webkit-reviews] review granted: [Bug 23384] PLATFORM(CHROMIUM) needs to delegate BackForwardList.cpp methods to the embedder : [Attachment 26805] v1 patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Darin Fisher (Google)
<darin at chromium.org>'s request for review:
Bug 23384: PLATFORM(CHROMIUM) needs to delegate BackForwardList.cpp methods to
the embedder
https://bugs.webkit.org/show_bug.cgi?id=23384

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   * history/BackForwardListChromium.cpp: Added.
> +	   (WebCore::BackForwardList::BackForwardList):
> +	   (WebCore::BackForwardList::~BackForwardList):
> +	   (WebCore::BackForwardList::addItem):
> +	   (WebCore::BackForwardList::goBack):
> +	   (WebCore::BackForwardList::goForward):
> +	   (WebCore::BackForwardList::goToItem):
> +	   (WebCore::BackForwardList::backItem):
> +	   (WebCore::BackForwardList::currentItem):
> +	   (WebCore::BackForwardList::forwardItem):
> +	   (WebCore::BackForwardList::backListWithLimit):
> +	   (WebCore::BackForwardList::forwardListWithLimit):
> +	   (WebCore::BackForwardList::capacity):
> +	   (WebCore::BackForwardList::setCapacity):
> +	   (WebCore::BackForwardList::enabled):
> +	   (WebCore::BackForwardList::setEnabled):
> +	   (WebCore::BackForwardList::backListCount):
> +	   (WebCore::BackForwardList::forwardListCount):
> +	   (WebCore::BackForwardList::itemAtIndex):
> +	   (WebCore::BackForwardList::entries):
> +	   (WebCore::BackForwardList::close):
> +	   (WebCore::BackForwardList::closed):
> +	   (WebCore::BackForwardList::removeItem):
> +	   (WebCore::BackForwardList::containsItem):

As I mentioned in other recent reviews, I don't think it's especially useful to
keep prepare-ChangeLog's output list of functions when we're dealing with new
files. That's just a personal view though. Maybe we should discuss it.

> +#if PLATFORM(CHROMIUM)
> +// In the Chromium port, the back/forward list is managed externally.
> +// See BackForwardListChromium.cpp
> +class BackForwardListClient {
> +public:
> +    virtual ~BackForwardListClient() {}
> +    virtual void addItem(PassRefPtr<HistoryItem>) = 0;
> +    virtual void goToItem(HistoryItem*) = 0;
> +    virtual HistoryItem* currentItem() = 0;
> +    virtual HistoryItem* itemAtIndex(int) = 0;
> +    virtual int backListCount() = 0;
> +    virtual int forwardListCount() = 0;
> +    virtual void close() = 0;
> +};
> +#endif

It might make more sense to make this part of the frame load client in the
future, but it seems good for now.

> +void BackForwardList::goBack()
> +{
> +    ASSERT_NOT_REACHED();
> +}
> +
> +void BackForwardList::goForward()
> +{
> +    ASSERT_NOT_REACHED();
> +}
> +
> +HistoryItem* BackForwardList::backItem()
> +{
> +    ASSERT_NOT_REACHED();
> +    return 0;
> +}
> +
> +HistoryItem* BackForwardList::forwardItem()
> +{
> +    ASSERT_NOT_REACHED();
> +    return 0;
> +}
> +
> +void BackForwardList::backListWithLimit(int limit, HistoryItemVector& list)
> +{
> +    list.clear();
> +    ASSERT_NOT_REACHED();
> +}
> +
> +void BackForwardList::forwardListWithLimit(int limit, HistoryItemVector&
list)
> +{
> +    list.clear();
> +    ASSERT_NOT_REACHED();
> +}
> +
> +void BackForwardList::removeItem(HistoryItem* item)
> +{
> +    ASSERT_NOT_REACHED();
> +}
> +
> +bool BackForwardList::containsItem(HistoryItem* entry)
> +{
> +    ASSERT_NOT_REACHED();
> +    return false;
> +}

Why not simply omit these definitions and let the linker catch the mistake if
someone calls one of them?

r=me