[Webkit-unassigned] [Bug 30004] [GStreamer] Should handle BUFFERING messages

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=30004


Gustavo Noronha (kov) <gns at gnome.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #47507|review?                     |review-
               Flag|                            |




--- Comment #24 from Gustavo Noronha (kov) <gns at gnome.org>  2010-02-01 09:04:24 PST ---
(From update of attachment 47507)
 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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.