[webkit-reviews] review denied: [Bug 104472] [Windows]ASSERTION failed in Windows: css3/css3-modsel-33.html : [Attachment 178402] patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied huangxueqing
<huangxueqing at baidu.com>'s request for review:
Bug 104472: [Windows]ASSERTION failed in Windows: css3/css3-modsel-33.html
https://bugs.webkit.org/show_bug.cgi?id=104472

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=178402&action=review


Thanks for taking the time to post this fix.  I think it looks good, but we
would like you to move the COMPtr declaration inside the two "if" cases. This
will make it clearer that the COMPtr is not needed elsewhere.
For this reason, r-, but please update the patch and I'll approve it.

>>> Source/WebKit/win/WebCoreSupport/WebFrameLoaderClient.cpp:429
>>> +	 frameLoadDelegatePrivate = 0;
>> 
>> This seems like a good change, but would it be clearer if we used separate
COMPtrs for the two possible cases? Maybe we should have each one declared
within the scope of the DidFirstlayout and DidFirstVisuallyNonEmpty Layout?
> 
> I agree, Brent's plan (two separate locals inside the if blocks) is clearer.

Okay -- please move the scope of "COMPtr<IWebFrameLoadDelegatePrivate>
frameLoadDelegatePrivate" to be enclosed inside the two cases.