[Merge] lp:~justinmcp/media-hub/1603729 into lp:media-hub

[Jim Hodapp]

Review: Needs Fixing code

A couple of comments inline.

Diff comments:

> 
> === modified file 'src/core/media/gstreamer/engine.cpp'
> --- src/core/media/gstreamer/engine.cpp	2016-07-11 01:21:38 +0000
> +++ src/core/media/gstreamer/engine.cpp	2016-08-17 07:37:18 +0000
> @@ -427,16 +427,10 @@
>  }
>  
>  bool gstreamer::Engine::open_resource_for_uri(const media::Track::UriType& uri,
> -                                              bool do_pipeline_reset)
> -{
> -    d->playbin.set_uri(uri, core::ubuntu::media::Player::HeadersType{}, do_pipeline_reset);
> -    return true;
> -}
> -
> -bool gstreamer::Engine::open_resource_for_uri(const media::Track::UriType& uri,
> -                                              const core::ubuntu::media::Player::HeadersType& headers)
> -{
> -    d->playbin.set_uri(uri, headers);
> +                                              const core::ubuntu::media::Player::HeadersType& headers,
> +                                              bool do_pipelline_reset)

typo, two l's, only need one.

> +{
> +    d->playbin.set_uri(uri, headers, do_pipelline_reset);
>      return true;
>  }
>  
> 
> === modified file 'src/core/media/player_implementation.cpp'
> --- src/core/media/player_implementation.cpp	2016-07-11 01:21:38 +0000
> +++ src/core/media/player_implementation.cpp	2016-08-17 07:37:18 +0000
> @@ -853,28 +873,13 @@
>  template<typename Parent>
>  bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri)
>  {
> -    d->track_list->reset();
> -
> -    // If empty uri, give the same meaning as QMediaPlayer::setMedia("")
> -    if (uri.empty())
> -    {
> -        MH_DEBUG("Resetting current media");
> -        return true;
> -    }
> -
> -    static const bool do_pipeline_reset = false;
> -    const bool ret = d->engine->open_resource_for_uri(uri, do_pipeline_reset);
> -    // Don't set new track as the current track to play since we're calling open_resource_for_uri above
> -    static const bool make_current = false;
> -    d->track_list->add_track_with_uri_at(uri, media::TrackList::after_empty_track(), make_current);
> -
> -    return ret;
> +    return d->open_uri(uri);
>  }
>  
>  template<typename Parent>
>  bool media::PlayerImplementation<Parent>::open_uri(const Track::UriType& uri, const Player::HeadersType& headers)
>  {
> -    return d->engine->open_resource_for_uri(uri, headers);
> +    return d->open_uri(uri, headers);

What's your concern on this? It looks ok to me, I would just want to make sure this is heavily tested as there's enough rework of opening a uri for all playback cases that it needs extensive testing not only in the browser cases but also the entire media-hub testplan.

>  }
>  
>  template<typename Parent>


-- 
https://code.launchpad.net/~justinmcp/media-hub/1603729/+merge/303097
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.