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

Fri Jul 13 11:49:55 UTC 2012

> On July 12, 2012, 11:04 p.m., Ralf Engels wrote:
> > tests/core-impl/playlists/types/file/m3u/TestM3UPlaylist.cpp, line 77
> > <http://git.reviewboard.kde.org/r/105524/diff/3/?file=72272#file72272line77>
> >
> >     If we need to have special code here to ensure a sensible cleanup, might we not run into troubles in Amarok itself?
> >     Could you add the old comment back?
> >     The "Wait for other jobs, ..."

Changing TestM3UPlaylist is apparently not needed at all in new patch version, altough I don't know whether this fill pass on Jenkins, too. Lets try without and perhaps resurrect this piece of patch.

> On July 12, 2012, 11:04 p.m., Ralf Engels wrote:
> > tests/core/collections/support/TestTrackForUrlWorker.cpp, line 42
> > <http://git.reviewboard.kde.org/r/105524/diff/3/?file=72274#file72274line42>
> >
> >     And these two lines had me confused.
> >     Why do we explicitely need to register the types? Isn't the Amarok library doing it on it's own?

> Why do we explicitely need to register the types? Isn't the Amarok library doing it on it's own?

Yes, but the unit test doesn't reach that code. It is correct, because in Amarok this is shared between many code paths.

> On July 12, 2012, 11:04 p.m., Ralf Engels wrote:
> > tests/core/collections/support/TestTrackForUrlWorker.cpp, line 38
> > <http://git.reviewboard.kde.org/r/105524/diff/3/?file=72274#file72274line38>
> >
> >     What a sorry excuse for a singleton we have here.
> >     We should look into that after the release and check if we can't do it better.

Resolved by having CollectionManager::trackForUrl() not calling EngineController at all.

> On July 12, 2012, 11:04 p.m., Ralf Engels wrote:
> > tests/playlistmanager/sql/TestSqlUserPlaylistProvider.cpp, line 48
> > <http://git.reviewboard.kde.org/r/105524/diff/3/?file=72279#file72279line48>
> >
> >     Is having the code in four places in the test cases really better than an additional check in CollectionManager::trackForUrl?
> >     
> >     And again, it's just because of the one function call in trackForUrl that implements a function that is not documented and could be surprising for the user in  some corner cases.

Resolved by having CollectionManager::trackForUrl() not calling EngineController at all.

- Matěj

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

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/19568422/attachment-0001.html>