[webkit-reviews] review denied: [Bug 30004] [GStreamer] Should handle BUFFERING messages : [Attachment 47507] initial support for on-disk buffering of videos

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 30004: [GStreamer] Should handle BUFFERING messages
https://bugs.webkit.org/show_bug.cgi?id=30004

Attachment 47507: initial support for on-disk buffering of videos
https://bugs.webkit.org/attachment.cgi?id=47507&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
 642	 if (gst_element_query(m_playBin, query)) {
 643	     gint64 start, stop;
 644 

I think this should become an early return. It will make the code more
readable. This would also enable us to make the following code much more
readable:

 664	     if (m_fillStatus == 100.0) {
 665		 // Buffering is done, remove the fill source from the main
loop.
 666		 m_fillTimeoutId = 0;
 667 
 668		 // Media is now fully loaded. It will play even if network
 669		 // connection is cut.
 670		 updateStates();
 671 
 672		 gst_query_unref(query);
 673		 return FALSE;
 674	     }
 675	     updateStates();
 676	 }
 677 
 678	 gst_query_unref(query);
 679	 return TRUE;

Maybe you will be able to call updateStates outside that if (the m_fillStatus
== 100.0 one), since you are going to call it anyway, right? And you can also
call gst_query_unref(query) before that if. And then you'll have a much more
pleasant code flow, with a simple if that sets the variable, you will be able
to unconditionaly return FALSE in the end, because the early return already
handled the TRUE case.

 682 void MediaPlayerPrivate::updateDownloadLocation(gchar* location)
 683 {
 684	 if (m_downloadFilename)
 685	     g_free(m_downloadFilename);
 686	 LOG_VERBOSE(Media, "Download filename: %s", location);
 687	 m_downloadFilename = location;
 688 }

I wonder if m_downloadFilename could be a GRefPtr. That would make the code a
bit cleaner. Code looks quite good, except for the function I was talking about
above. I'll say r- to see one more iteration of it before this lands. Thanks!