Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10413015:
Upstream implementation for embedded video for WebMediaPlayerAndroid (Closed)
|
Created:
8 years, 7 months ago by qinmin Modified:
8 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jochen+watch-content_chromium.org, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpstream implementation for embedded video for WebMediaPlayerAndroid
This change implements the embedded video for chrome on android.
However, the implementation of StreamTextureFactoryAndroid class is not included in this change.
So we only pass a null pointer to the constructor of WebMediaPlayerAndroid in render_view_impl.cc.
The class will be upstreamed later when we upstream java surface related code.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=139978
Patch Set 1 #
Total comments: 4
Patch Set 2 : refactored the code to remove SetVideoSurface() call #
Total comments: 2
Patch Set 3 : remove GetWebMediaPlayerManager() from render_view.h #
Total comments: 38
Patch Set 4 : addressing feedbacks #
Total comments: 32
Patch Set 5 : addressing feedbacks #
Total comments: 10
Patch Set 6 : addressing feedbacks #
Total comments: 6
Patch Set 7 : use first party url for cookies #
Total comments: 4
Patch Set 8 : fix the usage of first party url #Patch Set 9 : add the missing DidLoadingProgress() function due to recent webkit change #Patch Set 10 : rebase after https://chromiumcodereview.appspot.com/10469003/ got committed #Messages
Total messages: 29 (0 generated)
https://chromiumcodereview.appspot.com/10413015/diff/1/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://chromiumcodereview.appspot.com/10413015/diff/1/content/renderer/rende... content/renderer/render_view_impl.h:259: void SetVideoSurface(jobject j_surface, int player_id); where does the "jobject" type come from? jni.h? feels pretty darn weird for render_view_impl.h to have to #include jni.h if so, and I'm not sure how it is doing that now https://chromiumcodereview.appspot.com/10413015/diff/1/webkit/media/android/w... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/1/webkit/media/android/w... webkit/media/android/webmediaplayer_android.cc:38: // TODO(qinmin): Figure out where we should define this more appropriately let's try to figure this out, the answer is definitely not this file. gman@ might know
https://chromiumcodereview.appspot.com/10413015/diff/1/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://chromiumcodereview.appspot.com/10413015/diff/1/content/renderer/rende... content/renderer/render_view_impl.h:259: void SetVideoSurface(jobject j_surface, int player_id); I forgot to upload render_view.h in this patch, that file defines this function, and includes "jni.h". I have changed this function to return WebMediaPlayerManagerAndroid instead, So that the caller (render_thread_impl.cc) can directly get the WebMediaPlayerManagerAndroid and pass the video surface to it, instead of relaying it through RenderViewImpl. On 2012/05/18 23:10:24, jamesr wrote: > where does the "jobject" type come from? jni.h? feels pretty darn weird for > render_view_impl.h to have to #include jni.h if so, and I'm not sure how it is > doing that now https://chromiumcodereview.appspot.com/10413015/diff/1/webkit/media/android/w... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/1/webkit/media/android/w... webkit/media/android/webmediaplayer_android.cc:38: // TODO(qinmin): Figure out where we should define this more appropriately This variable is defined in both src/gpu/command_buffer/service/gl_utils.h and src/third_party/WebKit/Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h, however, both of them should be included in webkit/media. Gregg, any idea where we should put this? On 2012/05/18 23:10:24, jamesr wrote: > let's try to figure this out, the answer is definitely not this file. gman@ > might know
http://codereview.chromium.org/10413015/diff/5002/content/public/renderer/ren... File content/public/renderer/render_view.h (right): http://codereview.chromium.org/10413015/diff/5002/content/public/renderer/ren... content/public/renderer/render_view.h:143: GetWebMediaPlayerManager() = 0; does this need to be in the public API? i.e. who's going to be calling this function, code in src/content or src/chrome?
http://codereview.chromium.org/10413015/diff/5002/content/public/renderer/ren... File content/public/renderer/render_view.h (right): http://codereview.chromium.org/10413015/diff/5002/content/public/renderer/ren... content/public/renderer/render_view.h:143: GetWebMediaPlayerManager() = 0; Not necessarily. We call this in content/renderer/render_thread_impl.cc. The caller VideoSurfaceVisitor is inherited from content::RenderViewVisitor, and calling this function in the inherited visit() function. Because visit() takes an RenderView* param, so we expose this for convenience. Alternatively, I can use render_view->GetWebView() and then call RenderViewImpl::FromWebView() to get the pointer to RenderViewImpl. I will change the code to do this. On 2012/05/21 15:39:13, John Abd-El-Malek wrote: > does this need to be in the public API? i.e. who's going to be calling this > function, code in src/content or src/chrome?
Done, removed GetWebMediaPlayerManager() from render_view.h On 2012/05/21 16:42:38, qinmin wrote: > http://codereview.chromium.org/10413015/diff/5002/content/public/renderer/ren... > File content/public/renderer/render_view.h (right): > > http://codereview.chromium.org/10413015/diff/5002/content/public/renderer/ren... > content/public/renderer/render_view.h:143: GetWebMediaPlayerManager() = 0; > Not necessarily. We call this in content/renderer/render_thread_impl.cc. > The caller VideoSurfaceVisitor is inherited from content::RenderViewVisitor, and > calling this function in the inherited visit() function. Because visit() takes > an RenderView* param, so we expose this for convenience. > > Alternatively, I can use render_view->GetWebView() and then call > RenderViewImpl::FromWebView() to get the pointer to RenderViewImpl. I will > change the code to do this. > > On 2012/05/21 15:39:13, John Abd-El-Malek wrote: > > does this need to be in the public API? i.e. who's going to be calling this > > function, code in src/content or src/chrome?
content lgtm On 2012/05/21 16:42:38, qinmin wrote: > http://codereview.chromium.org/10413015/diff/5002/content/public/renderer/ren... > File content/public/renderer/render_view.h (right): > > http://codereview.chromium.org/10413015/diff/5002/content/public/renderer/ren... > content/public/renderer/render_view.h:143: GetWebMediaPlayerManager() = 0; > Not necessarily. We call this in content/renderer/render_thread_impl.cc. > The caller VideoSurfaceVisitor is inherited from content::RenderViewVisitor, and > calling this function in the inherited visit() function. Because visit() takes > an RenderView* param, so we expose this for convenience. > We only add stuff in the public interfaces if they are needed by embedders of content, i.e. code in chrome/. Code inside content can cast to RenderViewImpl. This is just like the WebKit API. more about the content api: http://www.chromium.org/developers/content-module/content-api
https://chromiumcodereview.appspot.com/10413015/diff/9001/content/renderer/re... File content/renderer/render_view_impl.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/content/renderer/re... content/renderer/render_view_impl.cc:4910: media_player_manager_->ReleaseMediaResources(); do we not want to do anything when we're restored? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/stream_texture_factory_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/stream_texture_factory_android.h:42: virtual void EstablishPeer(int stream_id, int player_id) = 0; seeing as this code knows about WMPA, couldn't it use WMPA* instead of player_id? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:460: // Disconnecting the Surface(Texture) from the producer side allows nit on consistent terminology: what's Surface(Texture)? do you mean surface, texture, or surface texture? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:466: media_player_->SetVideoSurface(0); what does 0 represent here? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:468: needs_establish_peer_ = true; I'm not sure I follow what's happening here in PlayInternal() we establish a peer and set needs_establish_peer_ to false what code is forcing us to reestablish a peer? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:530: DCHECK(!stream_id_ && !texture_id_); nit: break into 2 DCHECKs https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:532: if (texture_id_) when would this happen? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:540: base::Bind(&WebMediaPlayerAndroid::DestroyStreamTexture, do you know which thread this will eventually get called on? which threads are calling CreateStreamTexture/DestroyStreamTexture? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:545: DCHECK(texture_id_ && stream_id_); nit: break into 2 DCHECKs https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:551: WebVideoFrame* WebMediaPlayerAndroid::getCurrentFrame() { which thread is this getting called on? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:553: stream_texture_proxy_->Initialize( this code assumes that CreateStreamTexture() has been called first? could we move the proxy_ initialization code there instead of relying on getCurrentFrame()? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:565: // This gets called both on impl and main thread. what's the impl thread? is this function racy? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.h:206: // Player Id assigned by the media player manager. nit: Id/id -> ID here and below https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_manager_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.cc:13: routing_id_(routing_id) { this isn't used anywhere https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.cc:16: WebMediaPlayerManagerAndroid::~WebMediaPlayerManagerAndroid() { nit: empty methods typically get collapsed w/o spaces inside {} https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_manager_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.h:19: class WebMediaPlayerManagerAndroid { ISTM this class is mostly here for two purposes: 1) Notifying content when WMPA is created/deleted 2) Forward messages from content to WMPA Since WMPA implements WeakPtr<T>, could you replace this class with: std::vector<WeakPtr<WebMediaPlayerAndroid> > media_players_; Inside createMediaPlayer(): WMPA* media_player = new WMPA(...); media_players_.push_back(media_player); Then to dispatch messages you can do: for (...) { if (media_players_[i]) { media_players_[i]->Foo(); } } https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.h:25: virtual int RegisterMediaPlayer(WebMediaPlayerAndroid* player); do we need to return an int ID or can we use the player pointer itself? for example, who who calls SetVideoSurface()? would they have a pointer?
https://chromiumcodereview.appspot.com/10413015/diff/9001/content/renderer/re... File content/renderer/render_view_impl.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/content/renderer/re... content/renderer/render_view_impl.cc:4910: media_player_manager_->ReleaseMediaResources(); I will address this when upstreaming the next patch, basically all the information including currentTime, duration, size will be recorded On 2012/05/23 22:45:46, scherkus wrote: > do we not want to do anything when we're restored? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/stream_texture_factory_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/stream_texture_factory_android.h:42: virtual void EstablishPeer(int stream_id, int player_id) = 0; This request will go all the way to the browser process to create a java surface texture. And when it finishes, it will pass the surface back to the java renderer sandbox process. The java sandbox process will find the correct render view and player using the player_id/routing_id. So during all this process, the WMPA* will just be treated as an integer due to java/c++ conversion happened. it is not as straightforward as a player_id and we have to deal with 32/64 bit address space later on. On 2012/05/23 22:45:46, scherkus wrote: > seeing as this code knows about WMPA, couldn't it use WMPA* instead of > player_id? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:460: // Disconnecting the Surface(Texture) from the producer side allows Surface is wrapper of surfaceTexture. I remember sometime for honeycomb, mediaplayer->setSurfaceTexture() is called. But now everything switched to surface. Just changed it to Surface to be consistent. On 2012/05/23 22:45:46, scherkus wrote: > nit on consistent terminology: what's Surface(Texture)? do you mean surface, > texture, or surface texture? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:466: media_player_->SetVideoSurface(0); NULL On 2012/05/23 22:45:46, scherkus wrote: > what does 0 represent here? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:468: needs_establish_peer_ = true; When calling SetVideoSurface(0), we are losing the surface peer. So setting it to true will make us call establishPeer() next time when PlayInternal() is called. On 2012/05/23 22:45:46, scherkus wrote: > I'm not sure I follow what's happening here > > in PlayInternal() we establish a peer and set needs_establish_peer_ to false > > what code is forcing us to reestablish a peer? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:530: DCHECK(!stream_id_ && !texture_id_); On 2012/05/23 22:45:46, scherkus wrote: > nit: break into 2 DCHECKs Done. https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:532: if (texture_id_) For DRT tests, we currently don't allocate any texture thing. So no image diff for all the media tests. On 2012/05/23 22:45:46, scherkus wrote: > when would this happen? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:540: base::Bind(&WebMediaPlayerAndroid::DestroyStreamTexture, Both will be called on the main render thread. On 2012/05/23 22:45:46, scherkus wrote: > do you know which thread this will eventually get called on? > > which threads are calling CreateStreamTexture/DestroyStreamTexture? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:545: DCHECK(texture_id_ && stream_id_); On 2012/05/23 22:45:46, scherkus wrote: > nit: break into 2 DCHECKs Done. https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:551: WebVideoFrame* WebMediaPlayerAndroid::getCurrentFrame() { This is called on the compositor thread. On 2012/05/23 22:45:46, scherkus wrote: > which thread is this getting called on? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:553: stream_texture_proxy_->Initialize( The stream_texture_proxy has to be created on the main thread, as it is listening to the frame producer on that thread. And the initialize function has to be called on the compositor thread, as this is where the frame available messages will be sent to. The class has locks inside it to guarantee thread safety. On 2012/05/23 22:45:46, scherkus wrote: > this code assumes that CreateStreamTexture() has been called first? > > could we move the proxy_ initialization code there instead of relying on > getCurrentFrame()? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:565: // This gets called both on impl and main thread. Impl thread is the compositor thread. This should not be racy based on how compositor thread is implemented. And when calling this on the main thread, we are passing a null params to the function to clean it up. On 2012/05/23 22:45:46, scherkus wrote: > what's the impl thread? > > is this function racy? https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.h:206: // Player Id assigned by the media player manager. On 2012/05/23 22:45:46, scherkus wrote: > nit: Id/id -> ID here and below Done. https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_manager_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.cc:13: routing_id_(routing_id) { removed On 2012/05/23 22:45:46, scherkus wrote: > this isn't used anywhere https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.cc:16: WebMediaPlayerManagerAndroid::~WebMediaPlayerManagerAndroid() { On 2012/05/23 22:45:46, scherkus wrote: > nit: empty methods typically get collapsed w/o spaces inside {} Done. https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_manager_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.h:19: class WebMediaPlayerManagerAndroid { I think we need this class for as it has more functions that haven't been upstreamed. We need it for resource management purpose so that the number of active players won't exceed a certain threshold. When a mediaplayer wants to start decoding, it will send a request to this class. The class will free up all the currently paused mediaplayers based on that request. And keeping track of all the active players in the same renderer process. On 2012/05/23 22:45:46, scherkus wrote: > ISTM this class is mostly here for two purposes: > 1) Notifying content when WMPA is created/deleted > 2) Forward messages from content to WMPA > > Since WMPA implements WeakPtr<T>, could you replace this class with: > > std::vector<WeakPtr<WebMediaPlayerAndroid> > media_players_; > > Inside createMediaPlayer(): > WMPA* media_player = new WMPA(...); > media_players_.push_back(media_player); > > > Then to dispatch messages you can do: > > for (...) { > if (media_players_[i]) { > media_players_[i]->Foo(); > } > } https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.h:25: virtual int RegisterMediaPlayer(WebMediaPlayerAndroid* player); So the whole process of getting a video surface works like this: We pass the player_id to the browser process. browser proess will create an video surface in the java code, and passing it back to the java sandboxed process. The sandboxed process will then find the correct mediaplayer using this player_id and routing id. So if we pass an address to the java process, we are actually treating it as an integer due to conversion. The address pretty much becomes an ID in that sense and we have to deal with 64bit/32bit address space later on. On 2012/05/23 22:45:46, scherkus wrote: > do we need to return an int ID or can we use the player pointer itself? > > for example, who who calls SetVideoSurface()? would they have a pointer?
next round of comments I'm still pretty confused at how all of this is supposed to fit together -- maybe we should sync up offline https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:532: if (texture_id_) On 2012/05/24 20:30:28, qinmin wrote: > For DRT tests, we currently don't allocate any texture thing. So no image diff > for all the media tests. That seems unfortunate! Do we have DRT image diff tests at all or is it only media? > On 2012/05/23 22:45:46, scherkus wrote: > > when would this happen? > https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_manager_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.h:19: class WebMediaPlayerManagerAndroid { On 2012/05/24 20:30:28, qinmin wrote: > I think we need this class for as it has more functions that haven't been > upstreamed. We need it for resource management purpose so that the number of > active players won't exceed a certain threshold. When a mediaplayer wants to > start decoding, it will send a request to this class. The class will free up all > the currently paused mediaplayers based on that request. And keeping track of > all the active players in the same renderer process. Hrmm... I think we're going to have to sort out some layering stuff. In general src/webkit/ code isn't multiprocess (src/content/ code is!) so the sign that we want a routing_id here is a hint that we might have to restructure some things > On 2012/05/23 22:45:46, scherkus wrote: > > ISTM this class is mostly here for two purposes: > > 1) Notifying content when WMPA is created/deleted > > 2) Forward messages from content to WMPA > > > > Since WMPA implements WeakPtr<T>, could you replace this class with: > > > > std::vector<WeakPtr<WebMediaPlayerAndroid> > media_players_; > > > > Inside createMediaPlayer(): > > WMPA* media_player = new WMPA(...); > > media_players_.push_back(media_player); > > > > > > Then to dispatch messages you can do: > > > > for (...) { > > if (media_players_[i]) { > > media_players_[i]->Foo(); > > } > > } > https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:81: if (stream_texture_factory_.get()) so right now this is NULL but what about after you upstream and no longer pass in NULL? will these checks go away? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:88: media_player_->SetVideoSurface(0); should this also be NULL? OOC do these methods need to be called prior to destruction? what would happen if we didn't? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:464: // At the same time we happily avoid recreating the SurfaceTexture to s/SurfaceTexture/Surface https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:466: media_player_->SetVideoSurface(NULL); we delete the media player on the next line -- what would happen if we didn't have this line of code? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:467: media_player_.reset(NULL); nit: s/reset(NULL)/reset()/ https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:468: needs_establish_peer_ = true; requiring a peer seems roughly correlated with having a new media player -- does it make sense to be able to query the media player object (via some methods) that it requires peering? for example, is it true that peering is requiring if the media player's video surface is NULL? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:532: stream_id_ = stream_texture_factory_->CreateStreamTexture(&texture_id_); isn't this NULL right now? is CreateStreamTexture() called anywhere else than PlayInternal()? if not you may want to consider inlining into PlayInternal() because right now (for example) you have DCHECKs checking for !stream_id_ ... but you have an if statement that handles that https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:554: if (!stream_texture_proxy_initialized_ && stream_id_) { would stream_texture_proxy_initialized_ be better served as an attribute of StreamTextureProxy itself? it seems strange to track the state of an object externally https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:567: // This gets called both on impl and main thread. s/impl/compositor https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:567: // This gets called both on impl and main thread. who is calling it on the main thread and for what purpose? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... File webkit/media/android/webmediaplayer_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.h:130: void SetVideoSurface(jobject j_surface); so this method is called by the manager... but I don't see anyone calling the manager's SetVideoSurface() method? who calls what here? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... File webkit/media/android/webmediaplayer_manager_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.cc:27: media_players_.find(player_id); fix ident https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... File webkit/media/android/webmediaplayer_manager_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:22: virtual ~WebMediaPlayerManagerAndroid(); why are all these methods virtual? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:26: virtual void UnRegisterMediaPlayer(int player_id); nit: "Un" isn't a word! let's have Unregister https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:35: virtual void SetVideoSurface(jobject j_surface, int player_id); who calls this method? I'm not sure how many more methods we're going to have like this, but does it make more sense to have a method that returns a WMPA* given a player_id so that we don't need to add forwarding methods? Is it illegal to call this method with an invalid player_id? Note that if you want to keep this method I'd suggest swapping the parameter order (i.e., objects that are getting operated on [player_id] typically go first followed by parameters to that object) https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:48: // ID used to create the next media player for passing the Surface. "for passing the Surface" doesn't read very well -- perhaps just remove this comment -- the variable name is pretty obvious
https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_android.cc:532: if (texture_id_) I think we are currently turning all image diff off due to the settings in our image decoders. On 2012/05/24 21:36:32, scherkus wrote: > On 2012/05/24 20:30:28, qinmin wrote: > > For DRT tests, we currently don't allocate any texture thing. So no image diff > > for all the media tests. > > That seems unfortunate! Do we have DRT image diff tests at all or is it only > media? > > > On 2012/05/23 22:45:46, scherkus wrote: > > > when would this happen? > > > https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... File webkit/media/android/webmediaplayer_manager_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/9001/webkit/media/androi... webkit/media/android/webmediaplayer_manager_android.h:19: class WebMediaPlayerManagerAndroid { we still have only 1 render process and 1 browser process in this case. But the passing the surface from the browser process to the renderer process is through Java Binder, not the chromium IPCs. This is probably the only object that we are not able to pass through IPC. On 2012/05/24 21:36:32, scherkus wrote: > On 2012/05/24 20:30:28, qinmin wrote: > > I think we need this class for as it has more functions that haven't been > > upstreamed. We need it for resource management purpose so that the number of > > active players won't exceed a certain threshold. When a mediaplayer wants to > > start decoding, it will send a request to this class. The class will free up > all > > the currently paused mediaplayers based on that request. And keeping track of > > all the active players in the same renderer process. > > Hrmm... I think we're going to have to sort out some layering stuff. In general > src/webkit/ code isn't multiprocess (src/content/ code is!) so the sign that we > want a routing_id here is a hint that we might have to restructure some things > > > On 2012/05/23 22:45:46, scherkus wrote: > > > ISTM this class is mostly here for two purposes: > > > 1) Notifying content when WMPA is created/deleted > > > 2) Forward messages from content to WMPA > > > > > > Since WMPA implements WeakPtr<T>, could you replace this class with: > > > > > > std::vector<WeakPtr<WebMediaPlayerAndroid> > media_players_; > > > > > > Inside createMediaPlayer(): > > > WMPA* media_player = new WMPA(...); > > > media_players_.push_back(media_player); > > > > > > > > > Then to dispatch messages you can do: > > > > > > for (...) { > > > if (media_players_[i]) { > > > media_players_[i]->Foo(); > > > } > > > } > > > https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:81: if (stream_texture_factory_.get()) Yes, eventually when StreamTextureProxy implementation got upstreamed, I will remove this check. On 2012/05/24 21:36:32, scherkus wrote: > so right now this is NULL but what about after you upstream and no longer pass > in NULL? will these checks go away? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:88: media_player_->SetVideoSurface(0); Changed to NULL. If we don't call it, the surface will not be released immediately until JVM decides to do a GC on the java MediaPlayer. And that is causing a lot of OOM problems. On 2012/05/24 21:36:32, scherkus wrote: > should this also be NULL? > > OOC do these methods need to be called prior to destruction? what would happen > if we didn't? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:464: // At the same time we happily avoid recreating the SurfaceTexture to On 2012/05/24 21:36:32, scherkus wrote: > s/SurfaceTexture/Surface Done. https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:466: media_player_->SetVideoSurface(NULL); Just as the reason above, deleting media_player_ will not immediately GC the java mediaplayer. And surface is taking a lot of memories. On 2012/05/24 21:36:32, scherkus wrote: > we delete the media player on the next line -- what would happen if we didn't > have this line of code? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:467: media_player_.reset(NULL); On 2012/05/24 21:36:32, scherkus wrote: > nit: s/reset(NULL)/reset()/ Done. https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:468: needs_establish_peer_ = true; need_establish_peer_ does not always correlate to a new media player. The situation could happen when user enters fullscreen mode. In that case, we will release the embedded surface, and create an android surfaceView, and pass that surfaceView to the mediaplayer. When user quit the fullscreen mode, we will request the embedded video surface again. That will make us to set needs_establish_peer_ to true again. The mediaplayer will not get recreated during the embed->fullscreen->embed process. Also there is a delay between EstablishPeer() and SetVideoSurface() due to IPC. So in PlayInternal(), if we call EstablishPeer(), and then do a pause, and play again, the mediaplayer might not hold the surface when the 2nd PlayInternal got called. On 2012/05/24 21:36:32, scherkus wrote: > requiring a peer seems roughly correlated with having a new media player -- does > it make sense to be able to query the media player object (via some methods) > that it requires peering? > > for example, is it true that peering is requiring if the media player's video > surface is NULL? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:532: stream_id_ = stream_texture_factory_->CreateStreamTexture(&texture_id_); This function will not get called if stream_texture_factory_ is NULL. On 2012/05/24 21:36:32, scherkus wrote: > isn't this NULL right now? > > is CreateStreamTexture() called anywhere else than PlayInternal()? > > if not you may want to consider inlining into PlayInternal() because right now > (for example) you have DCHECKs checking for !stream_id_ ... but you have an if > statement that handles that https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:554: if (!stream_texture_proxy_initialized_ && stream_id_) { On 2012/05/24 21:36:32, scherkus wrote: > would stream_texture_proxy_initialized_ be better served as an attribute of > StreamTextureProxy itself? > > it seems strange to track the state of an object externally Done. https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:567: // This gets called both on impl and main thread. On 2012/05/24 21:36:32, scherkus wrote: > s/impl/compositor Done. https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:567: // This gets called both on impl and main thread. It is called in WebMediaPlayerClientImpl::~WebMediaPlayerClientImpl() to clean up things. This change has been upstreamed to webkit. On 2012/05/24 21:36:32, scherkus wrote: > who is calling it on the main thread and for what purpose? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... File webkit/media/android/webmediaplayer_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_android.h:130: void SetVideoSurface(jobject j_surface); That code hasn't been upstreamed yet. Eventually it will be our java SandBoxedProcessService calling functions within renderer_main_platform_delegate.cc. On 2012/05/24 21:36:32, scherkus wrote: > so this method is called by the manager... but I don't see anyone calling the > manager's SetVideoSurface() method? > > who calls what here? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... File webkit/media/android/webmediaplayer_manager_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.cc:27: media_players_.find(player_id); On 2012/05/24 21:36:32, scherkus wrote: > fix ident Done. https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... File webkit/media/android/webmediaplayer_manager_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:22: virtual ~WebMediaPlayerManagerAndroid(); removed all the virtual keyword On 2012/05/24 21:36:32, scherkus wrote: > why are all these methods virtual? https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:26: virtual void UnRegisterMediaPlayer(int player_id); On 2012/05/24 21:36:32, scherkus wrote: > nit: "Un" isn't a word! let's have Unregister Done. https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:35: virtual void SetVideoSurface(jobject j_surface, int player_id); When creating a video surface, the renderer process sends an IPC to the browser process. After creating the video surface in the java side, the browser process will pass the surface to the renderer process through binder (http://developer.android.com/reference/android/os/Binder.html). Surface is the only object that we cannot pass through chromium IPC. When the renderer process receives the surface through binder, it will call setVideoSurface in our own RendererMainPlatformDelegate. I changed this function to GetWebMediaPlayer() so that we only return a WMPA* to the RendererMainPlatformDelegate. And then it can call setVideoSurface Directly If the player_id does not exist (player got deleted before surface is created), then nothing should happen. On 2012/05/24 21:36:32, scherkus wrote: > who calls this method? > > I'm not sure how many more methods we're going to have like this, but does it > make more sense to have a method that returns a WMPA* given a player_id so that > we don't need to add forwarding methods? > > Is it illegal to call this method with an invalid player_id? > > Note that if you want to keep this method I'd suggest swapping the parameter > order (i.e., objects that are getting operated on [player_id] typically go first > followed by parameters to that object) https://chromiumcodereview.appspot.com/10413015/diff/16001/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:48: // ID used to create the next media player for passing the Surface. On 2012/05/24 21:36:32, scherkus wrote: > "for passing the Surface" doesn't read very well -- perhaps just remove this > comment -- the variable name is pretty obvious Done.
one last round! https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... File webkit/media/android/stream_texture_factory_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/stream_texture_factory_android.h:18: virtual ~StreamTextureProxy() { } nit: no spaces in braces {} https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:480: media_player_.reset(new MediaPlayerBridge()); if a media_player_ existed... would we want to call SetVideoSurface(NULL) here? Should ~MediaPlayerBridge dtor automatically call SetVideoSurface(NULL) or are there cases where we don't want it to release it's video surface? It scares me knowing that if we forget to call SVS(NULL) before deleting media_player_ that we'll cause a performance regression! https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... File webkit/media/android/webmediaplayer_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/webmediaplayer_android.h:130: void SetVideoSurface(jobject j_surface); if it's possible could you try to not include methods/variables that aren't used yet? I understand it's probably really tricky but it makes it awfully confusing for me to figure out what's going on! https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... File webkit/media/android/webmediaplayer_manager_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:31: // Get the pointer to WebMediaPlayerAndroid given the player_id. nit: |player_id| also what's the return value if the player isn't found? crash or NULL? https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:32: WebMediaPlayerAndroid* GetWebMediaPlayer(int player_id); nit: you have Register/UnregisterMediaPlayer ... but this one is Get_Web_MediaPlayer Want to drop the "Web"?
https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... File webkit/media/android/stream_texture_factory_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/stream_texture_factory_android.h:18: virtual ~StreamTextureProxy() { } On 2012/05/25 22:01:46, scherkus wrote: > nit: no spaces in braces {} Done. https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:480: media_player_.reset(new MediaPlayerBridge()); Moved SetVideoSurface(NULL) to MediaPlayerBridge dtor. On 2012/05/25 22:01:46, scherkus wrote: > if a media_player_ existed... would we want to call SetVideoSurface(NULL) here? > > Should ~MediaPlayerBridge dtor automatically call SetVideoSurface(NULL) or are > there cases where we don't want it to release it's video surface? > > It scares me knowing that if we forget to call SVS(NULL) before deleting > media_player_ that we'll cause a performance regression! https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... File webkit/media/android/webmediaplayer_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/webmediaplayer_android.h:130: void SetVideoSurface(jobject j_surface); This function is removed from this class, will upstream it later when we upstream the RendererMainPlatformDelegate. On 2012/05/25 22:01:46, scherkus wrote: > if it's possible could you try to not include methods/variables that aren't used > yet? > > I understand it's probably really tricky but it makes it awfully confusing for > me to figure out what's going on! https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... File webkit/media/android/webmediaplayer_manager_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:31: // Get the pointer to WebMediaPlayerAndroid given the player_id. On 2012/05/25 22:01:46, scherkus wrote: > nit: |player_id| > > also what's the return value if the player isn't found? crash or NULL? Done. https://chromiumcodereview.appspot.com/10413015/diff/12002/webkit/media/andro... webkit/media/android/webmediaplayer_manager_android.h:32: WebMediaPlayerAndroid* GetWebMediaPlayer(int player_id); On 2012/05/25 22:01:46, scherkus wrote: > nit: you have Register/UnregisterMediaPlayer ... but this one is > Get_Web_MediaPlayer > > Want to drop the "Web"? Done.
LGTM w/ one nit https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:39: #define GL_TEXTURE_EXTERNAL_OES 0x8D65 static const uint32 and kCamelCase?
https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... File webkit/media/android/stream_texture_factory_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... webkit/media/android/stream_texture_factory_android.h:6: #define WEBKIT_MEDIA_ANDROID_STREAM_TEXTURE_FACTORY_ANDROID_H_ #pragma once (here and in other headers) https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:473: cookies = UTF16ToUTF8(cookie_jar_->cookies(url, url)); this breaks third-party cookie blocking settings. The first party URL should be taken from frame->document().firstPartyForCookies() for whatever frame the contents is going to be displayed in
Swiched to use first party url. Jochen, would you please take a look again? thanks https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... File webkit/media/android/stream_texture_factory_android.h (right): https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... webkit/media/android/stream_texture_factory_android.h:6: #define WEBKIT_MEDIA_ANDROID_STREAM_TEXTURE_FACTORY_ANDROID_H_ On 2012/05/26 10:24:57, jochen wrote: > #pragma once (here and in other headers) Done. https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:39: #define GL_TEXTURE_EXTERNAL_OES 0x8D65 On 2012/05/26 00:56:09, scherkus wrote: > static const uint32 and kCamelCase? Done. https://chromiumcodereview.appspot.com/10413015/diff/20011/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:473: cookies = UTF16ToUTF8(cookie_jar_->cookies(url, url)); On 2012/05/26 10:24:57, jochen wrote: > this breaks third-party cookie blocking settings. The first party URL should be > taken from frame->document().firstPartyForCookies() for whatever frame the > contents is going to be displayed in Done.
https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:476: WebURL url(frame_->document().firstPartyForCookies()); I'd name this "first_party_url", and then https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:477: cookies = UTF16ToUTF8(cookie_jar_->cookies(url, url)); it should be cookies(url_, first_party_url)
hi, jochen, I've fixed the usage of first_party_url, please check. https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/andro... File webkit/media/android/webmediaplayer_android.cc (right): https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:476: WebURL url(frame_->document().firstPartyForCookies()); On 2012/05/30 08:41:37, jochen wrote: > I'd name this "first_party_url", and then Done. https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/andro... webkit/media/android/webmediaplayer_android.cc:477: cookies = UTF16ToUTF8(cookie_jar_->cookies(url, url)); On 2012/05/30 08:41:37, jochen wrote: > it should be cookies(url_, first_party_url) Done.
cookie jar changes lgtm, thanks! On Wed, May 30, 2012 at 7:06 PM, <qinmin@chromium.org> wrote: > hi, jochen, I've fixed the usage of first_party_url, please check. > > > > https://chromiumcodereview.**appspot.com/10413015/diff/** > 18003/webkit/media/android/**webmediaplayer_android.cc<https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/android/webmediaplayer_android.cc> > File webkit/media/android/**webmediaplayer_android.cc (right): > > https://chromiumcodereview.**appspot.com/10413015/diff/** > 18003/webkit/media/android/**webmediaplayer_android.cc#**newcode476<https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/android/webmediaplayer_android.cc#newcode476> > webkit/media/android/**webmediaplayer_android.cc:476: WebURL > url(frame_->document().**firstPartyForCookies()); > On 2012/05/30 08:41:37, jochen wrote: > >> I'd name this "first_party_url", and then >> > > Done. > > > https://chromiumcodereview.**appspot.com/10413015/diff/** > 18003/webkit/media/android/**webmediaplayer_android.cc#**newcode477<https://chromiumcodereview.appspot.com/10413015/diff/18003/webkit/media/android/webmediaplayer_android.cc#newcode477> > webkit/media/android/**webmediaplayer_android.cc:477: cookies = > UTF16ToUTF8(cookie_jar_->**cookies(url, url)); > On 2012/05/30 08:41:37, jochen wrote: > >> it should be cookies(url_, first_party_url) >> > > Done. > > https://chromiumcodereview.**appspot.com/10413015/<https://chromiumcodereview... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/10413015/12017
Try job failure for 10413015-12017 (retry) (previous was lost) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/10413015/12017
Try job failure for 10413015-12017 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/10413015/12017
Try job failure for 10413015-12017 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/10413015/29006
Try job failure for 10413015-29006 (retry) (previous was lost) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/qinmin@chromium.org/10413015/25010
Change committed as 139978 |
