[webkit-reviews] review denied: [Bug 66137] add shouldValidate() to WebHistory : [Attachment 103774] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Gavin Peters
<gavinp at chromium.org>'s request for review:
Bug 66137: add shouldValidate() to WebHistory
https://bugs.webkit.org/show_bug.cgi?id=66137

Attachment 103774: Patch
https://bugs.webkit.org/attachment.cgi?id=103774&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=103774&action=review


> Source/WebCore/ChangeLog:4
> +	   add shouldValidate() to WebHistory
> +	   https://bugs.webkit.org/show_bug.cgi?id=66137

WebCore doesn't have "WebHistory", nor is this a change to the WebKit global
"WebHistory"

It actually adds shouldValidate to HistoryItem.

> Source/WebCore/loader/FrameLoader.cpp:-3070
>	       case FrameLoadTypeBack:
>	       case FrameLoadTypeForward:
>	       case FrameLoadTypeIndexedBackForward:
> -		   // If the first load within a frame is a navigation within a
back/forward list that was attached 
> -		   // without any of the items being loaded then we should use
the default caching policy (<rdar://problem/8131355>).
> -		   if (m_stateMachine.committedFirstRealDocumentLoad() &&
!itemURL.protocolIs("https"))
> -		       request.setCachePolicy(ReturnCacheDataElseLoad);
> -		   break;

I think your changes need to go here in the switch statement...

> Source/WebCore/loader/FrameLoader.cpp:3084
> +
> +#if !PLATFORM(CHROMIUM) 
> +    // On platforms that do not set shouldValidate() based on session
history, we use the state machine
> +    // as a proxy for if this history item has been loaded in this session.
> +    if (!m_stateMachine.committedFirstRealDocumentLoad())
> +	 item->setShouldValidate(false);
> +#endif    
> +
> +    if (!item->shouldValidate())
> +	   request.setCachePolicy(ReturnCacheDataElseLoad);
> +

Because by putting them out here, you lose the meaning of the special behavior
being *only* for back/forward load types.  And yes - By using API, an app can
load a history item not as a back/forward load type and  that has special
meaning.

Also, you're losing the https exemption here.

Also, shouldValidate is *ALWAYS* false, as nothing ever sets it to true.  I
know Chromium will probably be setting it to true in some cases, but you're
basically changing the behavior for all other ports.

> Source/WebKit/chromium/ChangeLog:4
> +	   add shouldValidate() to WebHistory
> +	   https://bugs.webkit.org/show_bug.cgi?id=66137

Your port, your call - but it's actually WebHistoryItem right?

> Source/WebKit/chromium/ChangeLog:27
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   No new tests because DumpRenderTree doesn't have a good mechanism
for checking history.
> +
> +	   * history/HistoryItem.cpp:
> +	   (WebCore::HistoryItem::HistoryItem):
> +	   (WebCore::HistoryItem::reset):
> +	   * history/HistoryItem.h:
> +	   (WebCore::HistoryItem::setShouldValidate):
> +	   (WebCore::HistoryItem::shouldValidate):
> +	   * loader/FrameLoader.cpp:
> +	   (WebCore::FrameLoader::addExtraFieldsToRequest):
> +	   (WebCore::FrameLoader::loadDifferentDocumentItem):

This seems to be a copy of the WebCore changes and not the actual Chromium
WebKit changes.