Bug 23384 - PLATFORM(CHROMIUM) needs to delegate BackForwardList.cpp methods to the embedder
Summary: PLATFORM(CHROMIUM) needs to delegate BackForwardList.cpp methods to the embedder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Darin Fisher (:fishd, Google)
URL:
Keywords:
Depends on:
Blocks: 23480
  Show dependency treegraph
 
Reported: 2009-01-16 09:33 PST by Darin Fisher (:fishd, Google)
Modified: 2009-01-16 14:52 PST (History)
1 user (show)

See Also:


Attachments
prototype patch (7.33 KB, patch)
2009-01-16 09:36 PST, Darin Fisher (:fishd, Google)
no flags Details | Formatted Diff | Diff
v1 patch (8.12 KB, patch)
2009-01-16 12:36 PST, Darin Fisher (:fishd, Google)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Fisher (:fishd, Google) 2009-01-16 09:33:54 PST
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.
Comment 1 Darin Fisher (:fishd, Google) 2009-01-16 09:36:54 PST
Created attachment 26797 [details]
prototype patch
Comment 2 Darin Fisher (:fishd, Google) 2009-01-16 09:37:58 PST
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.
Comment 3 Darin Adler 2009-01-16 10:49:28 PST
(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.

Comment 4 Darin Fisher (:fishd, Google) 2009-01-16 11:36:47 PST
OK, will do.  Thanks!
Comment 5 Darin Fisher (:fishd, Google) 2009-01-16 12:36:46 PST
Created attachment 26805 [details]
v1 patch
Comment 6 Darin Adler 2009-01-16 14:09:54 PST
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
Comment 7 Dimitri Glazkov (Google) 2009-01-16 14:14:07 PST
+1 on no members in additions. 
Comment 8 Darin Fisher (:fishd, Google) 2009-01-16 14:36:43 PST
Good suggestion about leveraging the linker and about pruning the ChangeLog.
Comment 9 Darin Fisher (:fishd, Google) 2009-01-16 14:52:02 PST
http://trac.webkit.org/changeset/39989