[webkit-reviews] review denied: [Bug 52449] Crash when logging into gmail.com with frame flattening turned on. : [Attachment 81666] Patch.

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied Yael <yael.aharon at nokia.com>'s
request for review:
Bug 52449: Crash when logging into gmail.com with frame flattening turned on.
https://bugs.webkit.org/show_bug.cgi?id=52449

Attachment 81666: Patch.
https://bugs.webkit.org/attachment.cgi?id=81666&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=81666&action=review

The patch is basically ok but  I still say r- as I'd like to see a round of
cleanups.

> Source/WebCore/page/FrameView.cpp:728
> +    if (m_frame->settings() &&
m_frame->settings()->frameFlatteningEnabled()) {

Please make a variable for  "parent() && m_frame->settings() &&
m_frame->settings()->frameFlatteningEnabled()" and call it
"inSubframeLayoutWithFrameFlattening" or something. You can use it here and
later...

> Source/WebCore/page/FrameView.cpp:730
> +	   FrameView* parentView = static_cast<FrameView*>(parent());
> +	   if (parentView && !parentView->m_nestedLayoutCount) {

You shouldn't just cast without testing the type (even if it currently might
work in practice). See other places in the file for examples on how to get the
parent FrameView without casting by using tree().

> Source/WebCore/page/FrameView.cpp:770
> +    if (!m_nestedLayoutCount && !m_inSynchronousPostLayout &&
m_hasPendingPostLayoutTasks && (!(parent() && m_frame->settings() &&
m_frame->settings()->frameFlatteningEnabled()))) {

...here...

> Source/WebCore/page/FrameView.cpp:947
> +	   bool deferPostLayoutTasks = parent() && m_frame->settings() &&
m_frame->settings()->frameFlatteningEnabled();
> +	   if (!m_inSynchronousPostLayout && !deferPostLayoutTasks) {

and here...

>> Source/WebCore/page/FrameView.cpp:954
>> +	    if (!m_hasPendingPostLayoutTasks && ((needsLayout() ||
m_inSynchronousPostLayout) ||  deferPostLayoutTasks)) {
> 
> Extra space here.

and here. Also remove the extra space and remove the inner parenthesis from the
or condition, they do nothing.