[webkit-reviews] review granted: [Bug 28688] REGRESSION(r24994): Cannot create a frame with a javascript URL : [Attachment 38507] remove unrelated changes

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 28688: REGRESSION(r24994): Cannot create a frame with a javascript URL
https://bugs.webkit.org/show_bug.cgi?id=28688

Attachment 38507: remove unrelated changes
https://bugs.webkit.org/attachment.cgi?id=38507&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -	       if (!checkNodeSecurity(exec,
static_cast<HTMLFrameElementBase*>(ownerElement)->contentDocument()))
> +	       HTMLFrameElementBase* frameElement =
static_cast<HTMLFrameElementBase*>(ownerElement);
> +	       if (frameElement->contentDocument() && !checkNodeSecurity(exec,
frameElement->contentDocument()))
>		   return;

I think it would be better to put the contentDocument into a local variable
instead of the frame.

>	   HTMLFrameElementBase* frame =
static_cast<HTMLFrameElementBase*>(element);
> -	   if (!checkNodeSecurity(exec, frame->contentDocument()))
> +	   if (frame->contentDocument() && !checkNodeSecurity(exec,
frame->contentDocument()))
>	       return false;

Ditto.

> -	   if (!checkNodeSecurity(exec, imp->contentDocument()))
> +	   if (imp->contentDocument() && !checkNodeSecurity(exec,
imp->contentDocument()))
>	       return false;

I think this might read slightly better if the contentDocument was in a local
variable. I had trouble seeing the relationship between the two.

> -	   if (!checkNodeSecurity(exec, imp->contentDocument()))
> +	   if (imp->contentDocument() && !checkNodeSecurity(exec,
imp->contentDocument()))
>	       return;

Ditto.

It would be better if there was some shared code rather than this null check
having to be everywhere. Maybe a checkDocumentSecurity function that takes a
HTMLFrameElementBase* or Document*?

r=me