Bug 23384 – PLATFORM(CHROMIUM) needs to delegate BackForwardList.cpp methods to the embedder

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Darin Fisher (:fishd, Google)]

PLATFORM(CHROMIUM) needs to delegate BackForwardList.cpp methods to the embedder

Locally, we have just #ifdef'd this file and introduced a BackForwardListClient interface.  I will attach that patch for consideration.  I'm definitely open to alternative solutions that avoid hacking up BackForwardList.{h,cpp} so much.

[Darin Fisher (:fishd, Google)]

Created attachment 26797 [details]
prototype patch

[Darin Fisher (:fishd, Google)]

One idea I was considering was to create a BackForwardListChromium.cpp file.  That way at least the normal .cpp file could be left unmodified.  The changes to BackForwardList.h are minor, so perhaps this solution would be nicer.

[Darin Adler]

(In reply to comment #2)
> One idea I was considering was to create a BackForwardListChromium.cpp file. 
> That way at least the normal .cpp file could be left unmodified.  The changes
> to BackForwardList.h are minor, so perhaps this solution would be nicer.

I think that's the way to go.

[Darin Fisher (:fishd, Google)]

OK, will do.  Thanks!

[Darin Fisher (:fishd, Google)]

Created attachment 26805 [details]
v1 patch

[Darin Adler]

Comment on attachment 26805 [details]
v1 patch

> +        * 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

[Dimitri Glazkov (Google)]

+1 on no members in additions. 

[Darin Fisher (:fishd, Google)]

Good suggestion about leveraging the linker and about pruning the ChangeLog.

[Darin Fisher (:fishd, Google)]

http://trac.webkit.org/changeset/39989