[webkit-reviews] review granted: [Bug 14845] Frame's noResize attribute can not be set by JavaScript : [Attachment 87056] Patch and test cases

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 14845: Frame's noResize attribute can not be set by JavaScript
https://bugs.webkit.org/show_bug.cgi?id=14845

Attachment 87056: Patch and test cases
https://bugs.webkit.org/attachment.cgi?id=87056&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=87056&action=review

OK as-is. Suggestions for further improvement.

> Source/WebCore/html/HTMLFrameElement.cpp:90
> +	   m_noResize = !attr->isNull(); // This will evaluate to false when
frame.noResize := false, which is what we want.

The reason to cache the existence of an attribute in a boolean local is
performance; I do not think this code is performance critical and hence I don’t
think m_noResize is a good idea.

I suggest we make the noResize() function non-inline and change it to call
hasAttribute. This eliminates the need for an m_noResize data member.

I don’t understand the syntax "when frame.noResize := false" so I don’t know
what your comment is saying.

> Source/WebCore/html/HTMLFrameElement.cpp:95
> +	   if (HTMLFrameSetElement* frameSetElement =
containingFrameSetElement(this)) {
> +	       RenderObject* frameSetRenderer = frameSetElement->renderer();
> +	       if (frameSetRenderer && frameSetRenderer->isFrameSet())
> +		   toRenderFrameSet(frameSetRenderer)->computeEdgeInfo();
> +	   }

The division of labor here seems wrong. The RenderFrame is the class that knows
that edge info is based on noResize and hasFrameBorder, but now the HTML frame
element is responsible for calling directly to the RenderFrameSet. I don’t
think that RenderFrame should be half responsible for this. Instead it should
be entirely responsible.

I think that here in HTMLFrameElement we should be calling updateFromRenderer
on RenderFrame and RenderFrame in turn should be turning around and calling the
RenderFrameSet to say that its edge info changed and then the RenderFrameSet,
based on the fact that a frame called to say its edge info changed, will decide
to recompute the edge info.