Review Request: EngineController: fixes to canDecode() and supportedMimeTypes(): make them non-static, thread-safe even on first call. (squached patches, recent on top)

[Matěj Laitl]

> On July 13, 2012, 7:55 a.m., Bart Cerneels wrote:
> > src/browsers/filebrowser/FileView.cpp, line 431
> > <http://git.reviewboard.kde.org/r/105524/diff/3/?file=72267#file72267line431>
> >
> >     We should remove this canDecode() as well for the same reason I mention in the comment.

Resolved in new patch version.


- Matěj


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105524/#review15772
-----------------------------------------------------------


On July 12, 2012, 2:15 p.m., Matěj Laitl wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105524/
> -----------------------------------------------------------
> 
> (Updated July 12, 2012, 2:15 p.m.)
> 
> 
> Review request for Amarok, Ralf Engels and Sven Krohlas.
> 
> 
> Description
> -------
> 
> EngineController: use QFileInfo instead of apparently non-reentrant KFileItem
> 
> ...remote files are not needed here, so QFileInfo does a good job. This
> is the cause of last crashes in testm3uplaylist. (I have about 25%
> probability of it crashing until this commit)
> 
> EngineController: make supportedMimeTypes() thread-safe even on first call
> 
> If supportedMimeTypes() is called for the first time in a thread,
> Phonon::BackendCapabilities::availableMimeTypes() creates some QObject
> subclasses in non-main thread and that causes problems later on, and is
> the cause of testm3uplaylist failures. Add signal/slot/QSemaphore
> trickery that causes Phonon::BackendCapabilities::availableMimeTypes()
> is called from the main thread without performance penalty for 2nd and
> next calls.
> 
> Test is added for EngineController::supportedMimeTypes() so that this
> fragile code (hopefully) never breaks again.
> 
> This fixes (perhaps along with the next commit) 2 failing tests (
> testm3uplaylist and apparently testplaylistmodels), so the fail count
> is down to 4 here.
> 
> EngineController: make canDecode() and supportedMimeTypes() non-static
> 
> Static methods have no sense in a singleton class. Additionally, it was
> very hacky (and impossible in corner-cases) to keep these thread-safe,
> see testm3uplaylist failures. Making them non-static will allow us to
> do some tricks so that the calls are more robust.
> 
> Never-used and confusing destory() method is removed, too.
> 
> Care is taken not to break existing tests.
> v2: fix even more tests that have been added or fixed while this patch
>     slept.
> 
> Revert "Prevent hang in testmetamultitrack."
> 
> This reverts commit 115cb80f9bd94b23640ca9245c97d6c8e25d2c97.
> 
> Proper fix will follow, see next commits.
> 
> 
> Diffs
> -----
> 
>   src/EngineController.h ad2e0c4a5e7c80c79bf448bf74cd6b52cd1f0ed3 
>   src/EngineController.cpp 83f0a6ed0a92ae992e1809800cee65d9349dc680 
>   src/browsers/filebrowser/FileBrowser.cpp 567ff799df7ef8bfcd93de73ee120bfc5be634b7 
>   src/browsers/filebrowser/FileView.cpp 8eae4b6731ebfcb7310d3d719687989be125de92 
>   src/core-impl/collections/support/CollectionManager.cpp 9085b5a27d9a7ce94d2325d94bec5fce8d126abe 
>   tests/CMakeLists.txt 901c716a600bca63639939f4e62dbd89d9db707f 
>   tests/TestEngineController.h PRE-CREATION 
>   tests/TestEngineController.cpp PRE-CREATION 
>   tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp 4bbf29be01aef9043a64075a196ac04a544134cb 
>   tests/core/collections/support/TestTrackForUrlWorker.h PRE-CREATION 
>   tests/core/collections/support/TestTrackForUrlWorker.cpp PRE-CREATION 
>   tests/core/meta/TestMetaTrack.cpp 0adb7633538533202c50487b1eaff8e4ead150e9 
>   tests/dynamic/TestDynamicModel.h ce9934cf464172b75b1858f884a64c00cdcf4e24 
>   tests/dynamic/TestDynamicModel.cpp 7387fbcbf28ad838fbf56076553da56d630451af 
>   tests/playlistmanager/file/TestPlaylistFileProvider.cpp 931eefba03e55f4ae2beec089369087958311013 
>   tests/playlistmanager/sql/TestSqlUserPlaylistProvider.cpp bb09515d617e8149e7e66e804ff46aa5ddb76045 
> 
> Diff: http://git.reviewboard.kde.org/r/105524/diff/
> 
> 
> Testing
> -------
> 
> Works, fixes testm3uplaylist
> 
> 
> Thanks,
> 
> Matěj Laitl
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20120713/c7bd75be/attachment.html>