[webkit-reviews] review denied: [Bug 35773] [chromium] add logging of cross-frame property accesses for site isolation : [Attachment 52067] patch with renames and Frame parameter

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied John Gregg
<johnnyg at google.com>'s request for review:
Bug 35773: [chromium] add logging of cross-frame property accesses for site
isolation
https://bugs.webkit.org/show_bug.cgi?id=35773

Attachment 52067: patch with renames and Frame parameter
https://bugs.webkit.org/attachment.cgi?id=52067&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebCore/bindings/v8/V8Utilities.cpp
...
> +void logPropertyAccess(v8::Local<v8::String> name, const v8::AccessorInfo&
info)
...
> +		   eventId = (unsigned long long)
V8Event::toNative(v8::Handle<v8::Object>::Cast(jsEvent));

^^^ shouldn't that be a reinterpret_cast?


> Index: WebKit/chromium/public/WebFrameClient.h
> ===================================================================
> --- WebKit/chromium/public/WebFrameClient.h	(revision 56799)
> +++ WebKit/chromium/public/WebFrameClient.h	(working copy)
> @@ -284,6 +284,9 @@ public:
>      // scripts.
>      virtual void didCreateIsolatedScriptContext(WebFrame*) { }
>  
> +    // Notifies that a cross-frame access was made.
> +    virtual void didMakeCrossFrameAccess(WebFrame* active, WebFrame* target,
bool crossOrigin, const WebString& property, unsigned long long eventId) { }

Any reason not to name this the same as the FrameLoaderClient method?
Unless there is a good reason, I think it would be better to make them
match.