[webkit-dev] Platform specific fields on WebCore::HistoryItem

[Brady Eidson]

On Dec 21, 2010, at 4:26 PM, Darin Fisher wrote:

> On Tue, Dec 21, 2010 at 3:59 PM, Darin Fisher <darin at chromium.org> wrote:
> On Tue, Dec 21, 2010 at 2:56 PM, Brady Eidson <beidson at apple.com> wrote:
> 
> On Dec 21, 2010, at 1:57 PM, Darin Fisher wrote:
> 
>> 
>> 
>> On Tue, Dec 21, 2010 at 11:49 AM, Darin Fisher <darin at chromium.org> wrote:
>> 
>> 
>> On Tue, Dec 21, 2010 at 11:46 AM, Brady Eidson <beidson at apple.com> wrote:
>> 
>> On Dec 21, 2010, at 11:39 AM, Darin Fisher wrote:
>> 
>>> I'm working on fixing some session history bugs related to a HistoryItem's URL property changing.
>>> See for example the call to HistoryItem::setURL in HistoryController::updateForReload [1].
>>> 
>>> I'm curious about the platform specific fields on WebCore::HistoryItem.  *** Do any of those need to
>>> be updated when the URL of the HistoryItem changes? ***
>>> 
>>> Here are the fields I'm referring to:
>>> 
>>> class HistoryItem ... {
>>> private:
>>>     ...
>>> #if PLATFORM(MAC)
>>>     RetainPtr<id> m_viewState;
>> 
>> This is used for the Page Cache only.  The URL had sure better not change while the page is cached!
>> 
>> OK, I will assert that it is 0.
>> 
>> 
>> Awesome, I found a layout test where the URL of the HistoryItem changes, and there is an associated cached page for the HistoryItem!  I presume the right fix is to clear the cached page.
> 
> No no no!!!
> 
> Why is anyone trying to do anything to the URL of an item that represents a cached page???
> 
> Hmm, the testcase is simply assigning location within onload (the equivalent of location.replace).  The document is getting replaced with a new document, but the same HistoryItem is being overwritten with the URL of the new document.
> 
> I'm not super familiar with the cached page implementation since Chromium does not enable it, but if that system assumes there is a 1:1 relationship between HistoryItems and cached pages / documents, then that could be problematic.
> 
> I'm toying with calling HistoryController::invalidateCurrentItemCachedPage() in this case, but maybe that would be suboptimal?
> 
> OK, false alarm.  The issue is as follows:
> 
> The cached page corresponding to the new document is assigned to the HistoryItem before we change its URL.  In other words, I don't need to do anything special.
> 

Sounds fine.

~Brady

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20101221/6cfbcb6c/attachment.html>