[webkit-reviews] review denied: [Bug 76035] Add state attribute to history's dom interface. : [Attachment 124856] history.state

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Pablo Flouret
<pablof at motorola.com>'s request for review:
Bug 76035: Add state attribute to history's dom interface.
https://bugs.webkit.org/show_bug.cgi?id=76035

Attachment 124856: history.state
https://bugs.webkit.org/attachment.cgi?id=124856&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=124856&action=review


r- due to history->stateChanged()

> Source/WebCore/bindings/js/JSHistoryCustom.cpp:172
> +    if (!cachedValue.isEmpty() && !history->stateChanged())

I guess this might be dangerous. What happens if another call path updates
history.state? For example,

(1) JSHistory::state() caches 1111 in |m_state|.
(2) Another call path updates history.state to 2222.
(3) Another call path calls History::state(), which returns 2222.
(4) JSHistory::state() is called again. It calls history->stateChanged() and it
returns false. Consequently, JSHistory::state() will return the cached 1111.

> Source/WebCore/page/History.idl:40
> +	   readonly attribute [CachedAttribute, Custom] SerializedScriptValue
state;

[CachedAttribute] is not necessary, since the getter and setter are written as
a custom getter and setter.