[webkit-reviews] review requested: [Bug 29425] [Qt] SIGSEGV after WebCore::Frame::loader() : [Attachment 42088] patch v3 (Alternative)

[bugzilla-daemon at webkit.org]

jedrzej.nowacki at nokia.com has asked  for review:
Bug 29425: [Qt] SIGSEGV after WebCore::Frame::loader()
https://bugs.webkit.org/show_bug.cgi?id=29425

Attachment 42088: patch v3 (Alternative)
https://bugs.webkit.org/attachment.cgi?id=42088&action=review

------- Additional Comments from jedrzej.nowacki at nokia.com
Alternative patch including all suggestions. Personally I prefer the previous
one (patch v3), but from technical point of view it is the same.

(In reply to comment #12)
> (From update of attachment 41036 [details])
> This looks good based on my understanding of hos this works.
> 
>  180	   RefPtr<Frame> frame(m_originatingProgressFrame);  // Delete
> protection.
> 
> We prefer RefPtr<Frame> frame = m_originatingProgressFrame;
> Also, the comment is unnecessary.
Could you point me to the rule? I have checked coding style guide, the mailing
list discussion about unwritten rules of webkit style, source code of RefPtr
and PassRefPtr. In the FrameLoader.cpp this is a standard pattern to call the
constructor (RefPtr<Frame> protect(m_frame);) 


>  181	   FrameLoader* loader = frame->loader();  //Shortcut.
> 
> I wouldn't bother with this short cut.
It is easier to read then xxx.yyyy->zzz->aaa();
 
>  183	   loader->client()->willChangeEstimatedProgress();
> 
> Why did we add this client callback?
I guess... To checkout an old version of estimation, before it will be
overwritten? It would be nice to have comments :-)


Thanks for reviewing.