[webkit-reviews] review granted: [Bug 32052] Implement HTML5 state object history API : [Attachment 44163] Same patch, removing a printf() I missed and fixing *some* of the style-guideline bot's qualms.

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 32052: Implement HTML5 state object history API
https://bugs.webkit.org/show_bug.cgi?id=32052

Attachment 44163: Same patch, removing a printf() I missed and fixing *some* of
the style-guideline bot's qualms.
https://bugs.webkit.org/attachment.cgi?id=44163&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
> Index: WebCore/bindings/js/JSHistoryCustom.cpp
> ===================================================================
> --- WebCore/bindings/js/JSHistoryCustom.cpp	(revision 51545)
> +++ WebCore/bindings/js/JSHistoryCustom.cpp	(working copy)
> @@ -163,4 +163,52 @@ void JSHistory::getOwnPropertyNames(Exec
>      Base::getOwnPropertyNames(exec, propertyNames);
>  }
>  
> +JSValue JSHistory::pushState(ExecState* exec, const ArgList& args)
> +{
> +    PassRefPtr<SerializedScriptValue> historyState =
SerializedScriptValue::create(exec, args.at(0));

This should be a RefPtr.


> +JSValue JSHistory::replaceState(ExecState* exec, const ArgList& args)
> +{
> +    PassRefPtr<SerializedScriptValue> historyState =
SerializedScriptValue::create(exec, args.at(0));

Here too.

> +
> +JSValue JSPopStateEvent::initPopStateEvent(ExecState* exec, const ArgList&
args)
> +{
> +    const UString& typeArg = args.at(0).toString(exec);
> +    bool canBubbleArg = args.at(1).toBoolean(exec);
> +    bool cancelableArg = args.at(2).toBoolean(exec);
> +    PassRefPtr<SerializedScriptValue> stateObjectArg =
SerializedScriptValue::create(exec, args.at(3));

Here too.


> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
> + * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> + *
> + */
> +#include "config.h"
> +#include "PopStateEvent.h"

Missing newline.

> @@ -54,12 +55,16 @@ BackForwardList::~BackForwardList()
>  
>  void BackForwardList::addItem(PassRefPtr<HistoryItem> prpItem)
>  {
> +    insertItemAfterCurrent(prpItem, true);
> +}
> +void BackForwardList::insertItemAfterCurrent(PassRefPtr<HistoryItem>
prpItem, bool removeForwardList)
> +{

Missing newline.


> +
> +void HistoryItem::setDocument(Document* document)
> +{
> +    if (m_document == document)
> +	   return;
> +    
> +    if (m_document)
> +	   m_document->unregisterHistoryItem(this);40

40 eh?

> +    if (document)
> +	   document->registerHistoryItem(this);
> +	   
> +    m_document = document;
> +    
> +}

Extra newline.

>      
> +    void setStateObject(PassRefPtr<SerializedScriptValue> object);
> +    SerializedScriptValue* stateObject() { return m_stateObject.get(); }

This should be const.

> +    void setDocument(Document* document);
> +    Document* document() { return m_document; }

This should be const.

> @@ -209,6 +216,7 @@ private:
>      String m_parent;
>      String m_title;
>      String m_displayTitle;
> +    String m_stateData;

I don't think this is use.

> +    if (hashChange)
> +	   if (FrameView* view = m_frame->view())
> +	       view->scrollToFragment(m_URL);

The top if needs braces.


> +    
> +    // Check if we'll be using the page cache.  We only use the page cache
> +    // if one exists and it is less than _backForwardCacheExpirationInterval

> +    // seconds old.	If the cache is expired it gets flushed here.
> +    if (RefPtr<CachedPage> cachedPage = pageCache()->get(item)) {
> +	       
> +	   // FIXME: 1800 should not be hardcoded, it should come from

Extra newline.


> +	       case FrameLoadTypeIndexedBackForward:
> +		   if (itemURL.protocol() != "https")
> +		       request.setCachePolicy(ReturnCacheDataElseLoad);

This should use protocolIs.


>	   [DoNotCheckDomainSecurity] void go(in long distance);
> +	   
> +	   [DoNotCheckDomainSecurity, Custom] void pushState(in any data, in
DOMString title, in optional DOMString url)
> +	       raises(DOMException);
> +	   [DoNotCheckDomainSecurity, Custom] void replaceState(in any data, in
DOMString title, in optional DOMString url)
> +	       raises(DOMException);

These two methods should not have DoNotCheckDomainSecurity.


You are missing the definitions of the attribute event listers for onpopstate
on HTMLBodyElement and HTMLFrameSetElement.  The latter is also missing the
parsedMappedAttribute implementation.