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.
Created attachment 26797 [details] prototype patch
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.
(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.
OK, will do. Thanks!
Created attachment 26805 [details] v1 patch
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
+1 on no members in additions.
Good suggestion about leveraging the linker and about pruning the ChangeLog.
http://trac.webkit.org/changeset/39989