[webkit-reviews] review denied: [Bug 41592] ProgressTracker looks not smart : [Attachment 60705] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Ryuan Choi <ryuan.choi at gmail.com>'s
request for review:
Bug 41592: ProgressTracker looks not smart
https://bugs.webkit.org/show_bug.cgi?id=41592

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
Thanks for the patch, but a number of things are unclear to me:

WebCore/loader/ProgressTracker.cpp:184
 +	if (frame->loader()->client())
Do we need to null-check the client here?  Is the client disappearing somehow?

WebCore/loader/ProgressTracker.cpp: 
 +	// FIXME: Can this ever happen?
Did we resolve this FIXME?  It might be worth explaining when item might be
NULL.

In general, we would like to see a test with each patch.  Is there a way to
test this code change?	I'm not 100% sure this is the right direction.	It's
difficult to tell from this bug report and from the ChangeLog exactly what
problem we're solving here.  It's also unclear to me how this change effects
all the different clients of resource load notifier.