[webkit-reviews] review denied: [Bug 111074] REGRESSION(r141450): failed ASSERT in FrameView::scheduleRelayout() : [Attachment 191168] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alberto Garcia <agarcia at igalia.com>'s
request for review:
Bug 111074: REGRESSION(r141450): failed ASSERT in FrameView::scheduleRelayout()
https://bugs.webkit.org/show_bug.cgi?id=111074

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

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


review- because this is too mysterious to do without a comment, and is probably
not exactly the right fix

> Source/WebCore/page/FrameView.cpp:2061
> +    if (!frame()->view())
> +	   return;

The strange thing about this is that we could add a check like this to all
FrameView functions. We need a rule to tell us which ones need this. It’s
particularly important because you are not contributing a test for this, so no
one would be able to tell functions that need this kind of check from functions
that don’t.

Further, it’s non-obvious why taking a round trip to check for null is the
right thing here. This needs a comment, not just the code, because it’s
non-obvious. Further, maybe it should be a check for frame()->view() == this
instead of frame()->view() being non-null.

What’s special about contentsResized that means this is needed? Maybe the check
belongs at the caller instead of inside this function?