Defer loading of audio/video tags while prerendering.

Defer loading of audio/video tags while prerendering.

BUG=98690
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113742

[Shishir, 2011-09-30 19:45:57]

Initial implementation for comments.

[cbentzel, 2011-10-01 02:08:36]

I definitely suggest getting someone who knows the media stack to take a look
and make sure this approach is fine.

I like that it only needs to remember the URL and a boolean, rather than
buffering the entire audio/video in memory while it's prerendered.

[scherkus (not reviewing), 2011-10-03 18:19:45]

nits

overall I'm not entirely sure if it'll work -- it's hard to tease out the
interactions between HTMLMediaElement and WebMediaPlayerImpl

for example, HTMLMediaElement will call play() and then think the player is
playing and will start updating the player time, etc...

how extensively has this been tested?

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:295:
DCHECK(!prerender::PrerenderHelper::IsPrerendering(render_view));
call ShouldOverrideXXX() method instead?

also, what do you think about combining the two methods similar to
OverrideCreatePlugin() -- if it returns false then player == NULL

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:23: :
RenderViewObserver(render_view),
indent by 2 more spaces

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:33:
PrerenderWebMediaPlayer::~PrerenderWebMediaPlayer() {
nit: collapse empty methods to {}

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:52: void
PrerenderWebMediaPlayer::play() {
is it possible to receive play if we never finish loading?

it's hard to tell from WebKit's HTMLMediaElement.cpp code whether it'll wait to
call play() until we've called back from the load() with an update to
network/ready state

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:60: void
PrerenderWebMediaPlayer::pause() {
ditto

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:79: // that enables
prerendering.
I haven't looked at prerendering before, but why have a SetIsPrerendering if it
can only ever be set to false?

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:83: load(*url_to_load_);
is it possible to have this IPC called before we get a load()?

in that case url_to_load_ would be NULL

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.h:9: #include
"content/renderer/render_view_observer.h"
alphabetize includes

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.h:19: class
PrerenderWebMediaPlayer : public RenderViewObserver,
nit: use initializer-list style

class A
    : public B,
      public C {
  // ...
};

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.h:50: 
nit: remove extra blank line

http://codereview.chromium.org/8095007/diff/1/content/renderer/content_render...
File content/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8095007/diff/1/content/renderer/content_render...
content/renderer/content_renderer_client.h:25: class WebMediaPlayerClient;
alphabetize forward declares

http://codereview.chromium.org/8095007/diff/1/content/renderer/content_render...
content/renderer/content_renderer_client.h:32: class MessageLoopFactory;
alphabetize forward declares

http://codereview.chromium.org/8095007/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8095007/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:1925: DCHECK(result.get());
should'nt this DCHECK go inside the ShouldOverrideXXX method?

[Shishir, 2011-10-04 23:02:39]

Hi Andrew,

Thanks for the quick review. I have addressed your comments below.

I have tested this code with some examples and am still testing it for some of
the more complicated cases (like when there are seeks etc). It seems to work
fine since the whole system seems to be state driven. Since play does not happen
till the url actually loads, the media controller seems to be ok. Is there any
specific case you have in mind?

Thanks
Shishir

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:295:
DCHECK(!prerender::PrerenderHelper::IsPrerendering(render_view));
Changed to ShouldOverrideCreateWebMediaPlayer().

I had combined them initially, but felt that it was confusing because its not
clear what happens to the ownership of the input pointers if the player is NULL.
What do you think?

On 2011/10/03 18:19:45, scherkus wrote:
> call ShouldOverrideXXX() method instead?
> 
> also, what do you think about combining the two methods similar to
> OverrideCreatePlugin() -- if it returns false then player == NULL

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:23: :
RenderViewObserver(render_view),
On 2011/10/03 18:19:45, scherkus wrote:
> indent by 2 more spaces

Done.

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:33:
PrerenderWebMediaPlayer::~PrerenderWebMediaPlayer() {
On 2011/10/03 18:19:45, scherkus wrote:
> nit: collapse empty methods to {}

Done.

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:52: void
PrerenderWebMediaPlayer::play() {
Looks like you are correct. I dug into the webkit code and it seems to be using
the states to determine when to play. removed.

On 2011/10/03 18:19:45, scherkus wrote:
> is it possible to receive play if we never finish loading?
> 
> it's hard to tell from WebKit's HTMLMediaElement.cpp code whether it'll wait
to
> call play() until we've called back from the load() with an update to
> network/ready state

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:60: void
PrerenderWebMediaPlayer::pause() {
On 2011/10/03 18:19:45, scherkus wrote:
> ditto

removed.

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:79: // that enables
prerendering.
On 2011/10/03 18:19:45, scherkus wrote:
> I haven't looked at prerendering before, but why have a SetIsPrerendering if
it
> can only ever be set to false?

It can be set to true. The argument here is that it should have been set to true
much before this class can be instantiated. So this class should never receive a
notification of "true".

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.cc:83: load(*url_to_load_);
On 2011/10/03 18:19:45, scherkus wrote:
> is it possible to have this IPC called before we get a load()?
> 
> in that case url_to_load_ would be NULL

Thanks for the catch. Fixed.

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.h:9: #include
"content/renderer/render_view_observer.h"
On 2011/10/03 18:19:45, scherkus wrote:
> alphabetize includes

Done.

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.h:19: class
PrerenderWebMediaPlayer : public RenderViewObserver,
On 2011/10/03 18:19:45, scherkus wrote:
> nit: use initializer-list style
> 
> class A
>     : public B,
>       public C {
>   // ...
> };

Done.

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/prerender/prere...
chrome/renderer/prerender/prerender_webmediaplayer.h:50: 
On 2011/10/03 18:19:45, scherkus wrote:
> nit: remove extra blank line

Done.

http://codereview.chromium.org/8095007/diff/1/content/renderer/content_render...
File content/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8095007/diff/1/content/renderer/content_render...
content/renderer/content_renderer_client.h:25: class WebMediaPlayerClient;
On 2011/10/03 18:19:45, scherkus wrote:
> alphabetize forward declares

Done.

http://codereview.chromium.org/8095007/diff/1/content/renderer/content_render...
content/renderer/content_renderer_client.h:32: class MessageLoopFactory;
On 2011/10/03 18:19:45, scherkus wrote:
> alphabetize forward declares

Done.

http://codereview.chromium.org/8095007/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/8095007/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:1925: DCHECK(result.get());
On 2011/10/03 18:19:45, scherkus wrote:
> should'nt this DCHECK go inside the ShouldOverrideXXX method?

Done.

[scherkus (not reviewing), 2011-10-05 20:21:58]

overall it's looking good

one thing I'm not sure on how to handle are progress/stalled events

based on the spec (and WebKit does this), when we call load() we start the
progress event timer.

after 3 seconds we'll fire a stalled event -- but perhaps that's not such a bad
thing since that's what we're doing and would be equivalent to if we navigated
to a page and then had a really bad connection to a media server :)

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/chrome_content_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8095007/diff/1/chrome/renderer/chrome_content_...
chrome/renderer/chrome_content_renderer_client.cc:295:
DCHECK(!prerender::PrerenderHelper::IsPrerendering(render_view));
On 2011/10/04 23:02:39, Shishir wrote:
> Changed to ShouldOverrideCreateWebMediaPlayer().
> 
> I had combined them initially, but felt that it was confusing because its not
> clear what happens to the ownership of the input pointers if the player is
NULL.
> What do you think?
> 
> On 2011/10/03 18:19:45, scherkus wrote:
> > call ShouldOverrideXXX() method instead?
> > 
> > also, what do you think about combining the two methods similar to
> > OverrideCreatePlugin() -- if it returns false then player == NULL
> 

I'm pretty indifferent -- if you feel the current setup is more clear then go
with that :)

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
chrome/renderer/prerender/prerender_webmediaplayer.cc:32:
PrerenderWebMediaPlayer::~PrerenderWebMediaPlayer() { }
nit: no spaces inside {}

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
chrome/renderer/prerender/prerender_webmediaplayer.cc:58: void
PrerenderWebMediaPlayer::OnSetIsPrerendering(bool is_prerendering) {
one final question -- does this get cdalled on the main render thread?

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_audio.html (right):

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_audio.html:3: This test checks that
prerender is cancelled when an audio tag is
does this need to be updated?

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_audio.html:7: <title>Prerender
cancellation for HTML5 audio.</title>
updated?

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_audio.html:19: <audio
controls="controls" autoplay="autoplay">
nit: correct syntax is to just list the attributes (they don't need a value)
<audio controls autoplay>

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_video.html (right):

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video.html:3: This test checks that
prerender is cancelled when an video tag is
update docs?

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video.html:7: <title>Prerender
cancellation for HTML5 video.</title>
ditto

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video.html:19: <video
src="nonexistant.mp4" width="320" height="240" controls="controls">
nit:

you don't need to list width/height (in the case of no src it'll default to
300x150 and in the case of a valid src it'll resize itself)

nit: controls="controls" -> controls

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_video_script.html (right):

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video_script.html:3: This test checks
that prerender is cancelled when an video starts playing
ditto on updating docs

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video_script.html:26:
element.appendChild(videoTag);
you should consider adding a test that makes sure certain loading-related events
aren't fired (i.e., playing, canplay, canplaythrough)

[ddorwin, 2011-10-10 22:19:45]

drive-by nit

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
File chrome/renderer/prerender/prerender_helper.h (left):

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
chrome/renderer/prerender/prerender_helper.h:44: WebKit::WebMediaPlayerClient*
client) OVERRIDE;
Remove the forward declaration of WebMediaPlayerClient on line 20.

[Shishir, 2011-10-20 21:20:39]

Hi Andrew,

Sorry I got a bit delayed on this. I have addressed your comments and also added
much more test coverage. Please take another look.

Thanks
Shishir

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
File chrome/renderer/prerender/prerender_helper.h (left):

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
chrome/renderer/prerender/prerender_helper.h:44: WebKit::WebMediaPlayerClient*
client) OVERRIDE;
On 2011/10/10 22:19:46, ddorwin wrote:
> Remove the forward declaration of WebMediaPlayerClient on line 20.

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
chrome/renderer/prerender/prerender_webmediaplayer.cc:32:
PrerenderWebMediaPlayer::~PrerenderWebMediaPlayer() { }
On 2011/10/05 20:21:58, scherkus wrote:
> nit: no spaces inside {}

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/renderer/prerender/pr...
chrome/renderer/prerender/prerender_webmediaplayer.cc:32:
PrerenderWebMediaPlayer::~PrerenderWebMediaPlayer() { }
On 2011/10/05 20:21:58, scherkus wrote:
> nit: no spaces inside {}

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_audio.html (right):

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_audio.html:3: This test checks that
prerender is cancelled when an audio tag is
On 2011/10/05 20:21:58, scherkus wrote:
> does this need to be updated?

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_audio.html:7: <title>Prerender
cancellation for HTML5 audio.</title>
On 2011/10/05 20:21:58, scherkus wrote:
> updated?

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_audio.html:19: <audio
controls="controls" autoplay="autoplay">
On 2011/10/05 20:21:58, scherkus wrote:
> nit: correct syntax is to just list the attributes (they don't need a value)
> <audio controls autoplay>

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_video.html (right):

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video.html:3: This test checks that
prerender is cancelled when an video tag is
On 2011/10/05 20:21:58, scherkus wrote:
> update docs?

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video.html:7: <title>Prerender
cancellation for HTML5 video.</title>
On 2011/10/05 20:21:58, scherkus wrote:
> ditto

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video.html:19: <video
src="nonexistant.mp4" width="320" height="240" controls="controls">
On 2011/10/05 20:21:58, scherkus wrote:
> nit:
> 
> you don't need to list width/height (in the case of no src it'll default to
> 300x150 and in the case of a valid src it'll resize itself)
> 
> nit: controls="controls" -> controls

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
File chrome/test/data/prerender/prerender_html5_video_script.html (right):

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video_script.html:3: This test checks
that prerender is cancelled when an video starts playing
On 2011/10/05 20:21:58, scherkus wrote:
> ditto on updating docs

Done.

http://codereview.chromium.org/8095007/diff/7001/chrome/test/data/prerender/p...
chrome/test/data/prerender/prerender_html5_video_script.html:26:
element.appendChild(videoTag);
On 2011/10/05 20:21:58, scherkus wrote:
> you should consider adding a test that makes sure certain loading-related
events
> aren't fired (i.e., playing, canplay, canplaythrough)

Done.

[scherkus (not reviewing), 2011-10-24 18:30:43]

nits on tests

also you should take a look at what avi@ did w/ WebMediaPlayerDelegate in case
it's applicable:
http://src.chromium.org/viewvc/chrome?view=rev&revision=106328

http://codereview.chromium.org/8095007/diff/21001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio_jsplay.html (right):

http://codereview.chromium.org/8095007/diff/21001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio_jsplay.html:28: if (e.type ==
'canplay') {
nit: I'd use a switch/case statement here instead

http://codereview.chromium.org/8095007/diff/21001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio_jsplay.html:43:
audioEl.addEventListener('playing', audioEventHandler, false);
is it possible to move some of this code inside a script file to avoid
duplication?

[Shishir, 2011-10-31 21:16:31]

Hi Andrew,

I looked at the CL about the WebMediaPlayer delegate, but I am not sure if we
want to implement that here. Would you rather that we use the delegate to watch
for load events and pause the playey, keeping state in the PrerenderHelper?

Thanks
Shishir

http://codereview.chromium.org/8095007/diff/21001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio_jsplay.html (right):

http://codereview.chromium.org/8095007/diff/21001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio_jsplay.html:28: if (e.type ==
'canplay') {
On 2011/10/24 18:30:44, scherkus wrote:
> nit: I'd use a switch/case statement here instead

Done.

http://codereview.chromium.org/8095007/diff/21001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio_jsplay.html:43:
audioEl.addEventListener('playing', audioEventHandler, false);
On 2011/10/24 18:30:44, scherkus wrote:
> is it possible to move some of this code inside a script file to avoid
> duplication?

Done.

[scherkus (not reviewing), 2011-11-08 06:21:05]

few nits/sanity checks but overall looking good!

http://codereview.chromium.org/8095007/diff/39001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:1310: // Checks that video
tags inserted by javascript are deferred and  played
nit: "and  played " -> "and played"

http://codereview.chromium.org/8095007/diff/39001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.h:21: class
PrerenderWebMediaPlayer
class-level comment please

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio.html (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio.html:11: <source src="bear.wav"
type="audio/wav" />
nit: for this and the rest of the files just inline the src="bear.wav" into the
<audio> tag instead of using <source>

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio_autoplay.html (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio_autoplay.html:13: <source
src="bear.wav" type="audio/wav" />
nit: use ">" instead of "/>"

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio_jsplay.html (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio_jsplay.html:13: <source
src="bear.wav" type="audio/wav" />
nit: use ">" instead of "/>"

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:1: 
copyright ?

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:1: 
nit: get rid of blank line

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:16: {
nit: { goes on previous line

http://codereview.chromium.org/8095007/diff/39001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8095007/diff/39001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:941: if range[1]:
what's this change for?

[Shishir, 2011-11-09 22:27:46]

Hi Andrew,

I am unable to make this CL work on the trybots. It doesnt seem to find the two
new media files that have been added. Chris mentioned that trybots dont deal
well with binary files and it might be worth just checking in the media files
first. What do you think?

Thanks
Shishir

http://codereview.chromium.org/8095007/diff/39001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:1310: // Checks that video
tags inserted by javascript are deferred and  played
On 2011/11/08 06:21:06, scherkus wrote:
> nit: "and  played " -> "and played"

Done.

http://codereview.chromium.org/8095007/diff/39001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.h:21: class
PrerenderWebMediaPlayer
On 2011/11/08 06:21:06, scherkus wrote:
> class-level comment please

Done.

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio.html (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio.html:11: <source src="bear.wav"
type="audio/wav" />
On 2011/11/08 06:21:06, scherkus wrote:
> nit: for this and the rest of the files just inline the src="bear.wav" into
the
> <audio> tag instead of using <source>

Done.

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio_autoplay.html (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio_autoplay.html:13: <source
src="bear.wav" type="audio/wav" />
On 2011/11/08 06:21:06, scherkus wrote:
> nit: use ">" instead of "/>"

Integrated src.

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio_jsplay.html (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio_jsplay.html:13: <source
src="bear.wav" type="audio/wav" />
On 2011/11/08 06:21:06, scherkus wrote:
> nit: use ">" instead of "/>"

integrated src.

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:1: 
On 2011/11/08 06:21:06, scherkus wrote:
> nit: get rid of blank line

Done.

http://codereview.chromium.org/8095007/diff/39001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:16: {
On 2011/11/08 06:21:06, scherkus wrote:
> nit: { goes on previous line

Done.

http://codereview.chromium.org/8095007/diff/39001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8095007/diff/39001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:941: if range[1]:
On 2011/11/08 06:21:06, scherkus wrote:
> what's this change for?

This adds support for right open ended range. The media requests seem to not
have the end range.

[Shishir, 2011-11-10 01:42:10]

The trybots seem to be doing ok. The win breakages look unrelated.

[scherkus (not reviewing), 2011-11-10 07:47:58]

LGTM w/ one nit

http://codereview.chromium.org/8095007/diff/46001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8095007/diff/46001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:944: end = len(data)
if this implements open ended ranges then it looks like you should update the
comment! :)

[Shishir, 2011-11-14 19:27:23]

+akalin for net/tools/testserver/... changes.

http://codereview.chromium.org/8095007/diff/46001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8095007/diff/46001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:944: end = len(data)
On 2011/11/10 07:47:58, scherkus wrote:
> if this implements open ended ranges then it looks like you should update the
> comment! :)

Done.

[dominich, 2011-11-14 19:40:35]

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:68: if
(url_to_load_.get()) {
nit: remove these braces

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.h:45: virtual void
OnSetIsPrerendering(bool is_prerendering);
why is this virtual if it's not an OVERRIDE?

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.h:48:
scoped_ptr<WebKit::WebURL> url_to_load_;
Consider storing a WebKit::WebURL instead of as a pointer. Save the heap
alloc/dealloc.

http://codereview.chromium.org/8095007/diff/52001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio.html (right):

http://codereview.chromium.org/8095007/diff/52001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio.html:10: <audio id="mediaEl"
src="bear.wav" controls>
has this .wav been committed already?

http://codereview.chromium.org/8095007/diff/52001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_video.html (right):

http://codereview.chromium.org/8095007/diff/52001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_video.html:10: <video id="mediaEl"
src="bear.ogv" controls>
has this .ogv been committed already?

[Shishir, 2011-11-14 19:51:53]

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:68: if
(url_to_load_.get()) {
On 2011/11/14 19:40:35, dominich wrote:
> nit: remove these braces

Done.

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.h (right):

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.h:45: virtual void
OnSetIsPrerendering(bool is_prerendering);
On 2011/11/14 19:40:35, dominich wrote:
> why is this virtual if it's not an OVERRIDE?

Done.

http://codereview.chromium.org/8095007/diff/52001/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.h:48:
scoped_ptr<WebKit::WebURL> url_to_load_;
On 2011/11/14 19:40:35, dominich wrote:
> Consider storing a WebKit::WebURL instead of as a pointer. Save the heap
> alloc/dealloc.

Its cleaner to create this on load and destroy on cancel. The pointer implicitly
serves as a clean presence check (WebUrl has an empty check but no clear).

http://codereview.chromium.org/8095007/diff/52001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_audio.html (right):

http://codereview.chromium.org/8095007/diff/52001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_audio.html:10: <audio id="mediaEl"
src="bear.wav" controls>
On 2011/11/14 19:40:35, dominich wrote:
> has this .wav been committed already?

Yes. CL: 8505038

http://codereview.chromium.org/8095007/diff/52001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_video.html (right):

http://codereview.chromium.org/8095007/diff/52001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_video.html:10: <video id="mediaEl"
src="bear.ogv" controls>
On 2011/11/14 19:40:35, dominich wrote:
> has this .ogv been committed already?

Yes. CL: 8505038

[dominich, 2011-11-14 19:55:02]

LGTM modulo nit

http://codereview.chromium.org/8095007/diff/54005/content/public/renderer/con...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8095007/diff/54005/content/public/renderer/con...
content/public/renderer/content_renderer_client.h:112: virtual void
OverrideCreateWebMediaPlayer(
It might be worth creating a struct to pass these parameters and clean up the
interface a bit.

struct WebMediaPlayerCreationInfo {
  ...
};

virtual void OverrideCreateWebMediaPlayer(
    const WebMediaPlayerCreationInfo& info) = 0;

[tburkard, 2011-11-14 20:23:36]

LGTM

[cbentzel, 2011-11-15 18:55:32]

Please update the TEST= field in the CL description.

This looks pretty good. Most of my questions are due to lack-of-knowledge of the
WebMediaPlayer class and audio/video elements.

http://codereview.chromium.org/8095007/diff/54005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:1278: string16 expected_title
= ASCIIToUTF16("PASS");
Move the rest of this method into a helper function since it's  duped a few
times.

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:39: url_to_load_.reset(new
WebKit::WebURL(url));
Will this ever get called multiple times if the media element has multiple child
<source elements>? 

I'm reading through
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#media-...
and it looks like the resource algorithm will go through the sources serially,
but I just want to confirm.

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:42:
WebMediaPlayerImpl::load(url);
Will the "stalled" event get triggered here, or will that not happen since the
load isn't even initiated?

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:16: console.log(e.type);
Is this console.log statement needed anymore?

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:39:
mediaEl.addEventListener('playing', mediaEventHandler, false);
Should this be using the network events (loadstart, progress, etc.) in addition
to the more playback-oriented events?

[cbentzel, 2011-11-15 18:57:16]

Also - what happens if the media has a preload attribute but not autoplay. Will
the loading also be deferred until after the prerendered page is swapped in?

[scherkus (not reviewing), 2011-11-16 02:02:23]

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:39: url_to_load_.reset(new
WebKit::WebURL(url));
On 2011/11/15 18:55:33, cbentzel wrote:
> Will this ever get called multiple times if the media element has multiple
child
> <source elements>? 
> 
> I'm reading through
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#media-...
> and it looks like the resource algorithm will go through the sources serially,
> but I just want to confirm.

AFAIK load() is only called once per lifetime -- in other words a new media
player object is created if the source changes

it would be good to codify this with a CHECK()

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:42:
WebMediaPlayerImpl::load(url);
On 2011/11/15 18:55:33, cbentzel wrote:
> Will the "stalled" event get triggered here, or will that not happen since the
> load isn't even initiated? 

I believe that's the case -- would be good to use some tests here :)

cbentzel: do you think that's an issue?

in combination with page visibility API it might be OK

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:39:
mediaEl.addEventListener('playing', mediaEventHandler, false);
On 2011/11/15 18:55:33, cbentzel wrote:
> Should this be using the network events (loadstart, progress, etc.) in
addition
> to the more playback-oriented events?

I agree -- loadstart, progress, and stalled are good ones to watch

[akalin, 2011-11-17 03:37:10]

willchan's a better reviewer for the testserver stuff than me.

[Shishir, 2011-11-17 21:18:48]

http://codereview.chromium.org/8095007/diff/54005/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:1278: string16 expected_title
= ASCIIToUTF16("PASS");
On 2011/11/15 18:55:33, cbentzel wrote:
> Move the rest of this method into a helper function since it's  duped a few
> times.

Done.

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
File chrome/renderer/prerender/prerender_webmediaplayer.cc (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:39: url_to_load_.reset(new
WebKit::WebURL(url));
On 2011/11/16 02:02:23, scherkus wrote:
> On 2011/11/15 18:55:33, cbentzel wrote:
> > Will this ever get called multiple times if the media element has multiple
> child
> > <source elements>? 
> > 
> > I'm reading through
> >
>
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#media-...
> > and it looks like the resource algorithm will go through the sources
serially,
> > but I just want to confirm.
> 
> AFAIK load() is only called once per lifetime -- in other words a new media
> player object is created if the source changes
> 
> it would be good to codify this with a CHECK()

Verified that a second load form js causes a new media player to be created. So
adding a dcheck.
I am unable to test with a double load because the destructor of the first media
player causes the test to hang. Probably has to do with
WebMediaPlayerImpl::Destroy() call. Verfied that it doesnt hang on a test page
using the browser.

http://codereview.chromium.org/8095007/diff/54005/chrome/renderer/prerender/p...
chrome/renderer/prerender/prerender_webmediaplayer.cc:42:
WebMediaPlayerImpl::load(url);
On 2011/11/16 02:02:23, scherkus wrote:
> On 2011/11/15 18:55:33, cbentzel wrote:
> > Will the "stalled" event get triggered here, or will that not happen since
the
> > load isn't even initiated? 
> 
> I believe that's the case -- would be good to use some tests here :)
> 
> cbentzel: do you think that's an issue?
> 
> in combination with page visibility API it might be OK


This is no necessarily true. The stalled event is fired if there is no progress
for 3 seconds. Some tests last long enough that they are fired and for some they
are not. In either case, I think if the script waits for the canplay event to
fire before interacting with the media or uses page visibility we should be
fine. The stalled event may / may not fire.

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:16: console.log(e.type);
On 2011/11/15 18:55:33, cbentzel wrote:
> Is this console.log statement needed anymore? 

Helpful for debugging failing tests, so might be worth keeping this.

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:39:
mediaEl.addEventListener('playing', mediaEventHandler, false);
On 2011/11/16 02:02:23, scherkus wrote:
> On 2011/11/15 18:55:33, cbentzel wrote:
> > Should this be using the network events (loadstart, progress, etc.) in
> addition
> > to the more playback-oriented events?
> 
> I agree -- loadstart, progress, and stalled are good ones to watch

The network events are not very reliable and its not clear to me which of them
will fire. It  seems to depend more on timing. I see stalled / waiting fire on a
couple of tests but not on all. Also the network state seems to to be
"loading_data (2)" when any of canplay, canplaythrough etc.. events fire. 

Any suggestions on cases we should test.

http://codereview.chromium.org/8095007/diff/54005/content/public/renderer/con...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8095007/diff/54005/content/public/renderer/con...
content/public/renderer/content_renderer_client.h:112: virtual void
OverrideCreateWebMediaPlayer(
On 2011/11/14 19:55:03, dominich wrote:
> It might be worth creating a struct to pass these parameters and clean up the
> interface a bit.
> 
> struct WebMediaPlayerCreationInfo {
>   ...
> };
> 
> virtual void OverrideCreateWebMediaPlayer(
>     const WebMediaPlayerCreationInfo& info) = 0;

As discussed in person, not making this change.

[willchan no longer on Chromium, 2011-11-17 21:38:44]

lgtm

[scherkus (not reviewing), 2011-11-17 23:27:49]

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:16: console.log(e.type);
On 2011/11/17 21:18:49, Shishir wrote:
> On 2011/11/15 18:55:33, cbentzel wrote:
> > Is this console.log statement needed anymore? 
> 
> Helpful for debugging failing tests, so might be worth keeping this.

I'm a big proponent of quiet tests.

If a test started failing I'd assume a developer would do the necessary things
to debug it but leaving in log statements don't help us.

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:39:
mediaEl.addEventListener('playing', mediaEventHandler, false);
On 2011/11/17 21:18:49, Shishir wrote:
> On 2011/11/16 02:02:23, scherkus wrote:
> > On 2011/11/15 18:55:33, cbentzel wrote:
> > > Should this be using the network events (loadstart, progress, etc.) in
> > addition
> > > to the more playback-oriented events?
> > 
> > I agree -- loadstart, progress, and stalled are good ones to watch
> 
> The network events are not very reliable and its not clear to me which of them
> will fire. It  seems to depend more on timing. I see stalled / waiting fire on
a
> couple of tests but not on all. Also the network state seems to to be
> "loading_data (2)" when any of canplay, canplaythrough etc.. events fire. 
> 
> Any suggestions on cases we should test. 

This is interesting to discuss and would like to hear cbentzel's thoughts on the
matter.

I think it might be worth biting the bullet and having at least one test take
3-5s to run in order to listen to loadstart/progress/stalled. The other tests
don't require it and can run fast, but it's important to understand which
networking/load events are getting fired during prerendering.

What about a test for the following:
loadStartSeen && stalledSeen

[cbentzel, 2011-11-18 16:07:40]

On 2011/11/17 23:27:49, scherkus wrote:
>
http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
> File chrome/test/data/prerender/prerender_html5_common.js (right):
> 
>
http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
> chrome/test/data/prerender/prerender_html5_common.js:16: console.log(e.type);
> On 2011/11/17 21:18:49, Shishir wrote:
> > On 2011/11/15 18:55:33, cbentzel wrote:
> > > Is this console.log statement needed anymore? 
> > 
> > Helpful for debugging failing tests, so might be worth keeping this.
> 
> I'm a big proponent of quiet tests.

> 
> If a test started failing I'd assume a developer would do the necessary things
> to debug it but leaving in log statements don't help us.

+1

> 
>
http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
> chrome/test/data/prerender/prerender_html5_common.js:39:
> mediaEl.addEventListener('playing', mediaEventHandler, false);
> On 2011/11/17 21:18:49, Shishir wrote:
> > On 2011/11/16 02:02:23, scherkus wrote:
> > > On 2011/11/15 18:55:33, cbentzel wrote:
> > > > Should this be using the network events (loadstart, progress, etc.) in
> > > addition
> > > > to the more playback-oriented events?
> > > 
> > > I agree -- loadstart, progress, and stalled are good ones to watch
> > 
> > The network events are not very reliable and its not clear to me which of
them
> > will fire. It  seems to depend more on timing. I see stalled / waiting fire
on
> a
> > couple of tests but not on all. Also the network state seems to to be
> > "loading_data (2)" when any of canplay, canplaythrough etc.. events fire. 
> > 
> > Any suggestions on cases we should test. 
> 
> This is interesting to discuss and would like to hear cbentzel's thoughts on
the
> matter.
> 
> I think it might be worth biting the bullet and having at least one test take
> 3-5s to run in order to listen to loadstart/progress/stalled. The other tests
> don't require it and can run fast, but it's important to understand which
> networking/load events are getting fired during prerendering.
> 
> What about a test for the following:
> loadStartSeen && stalledSeen

Question: what if the deferral of loads were done in
WebMediaPlayerClientImpl::load instead? Would that prevent the loadStart and
stalled to be fired at all? Would that be preferable?

[scherkus (not reviewing), 2011-11-18 23:37:52]

On 2011/11/18 16:07:40, cbentzel wrote:
> On 2011/11/17 23:27:49, scherkus wrote:
> >
>
http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
> > File chrome/test/data/prerender/prerender_html5_common.js (right):
> > 
> >
>
http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
> > chrome/test/data/prerender/prerender_html5_common.js:16:
console.log(e.type);
> > On 2011/11/17 21:18:49, Shishir wrote:
> > > On 2011/11/15 18:55:33, cbentzel wrote:
> > > > Is this console.log statement needed anymore? 
> > > 
> > > Helpful for debugging failing tests, so might be worth keeping this.
> > 
> > I'm a big proponent of quiet tests.
> 
> > 
> > If a test started failing I'd assume a developer would do the necessary
things
> > to debug it but leaving in log statements don't help us.
> 
> +1
> 
> > 
> >
>
http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
> > chrome/test/data/prerender/prerender_html5_common.js:39:
> > mediaEl.addEventListener('playing', mediaEventHandler, false);
> > On 2011/11/17 21:18:49, Shishir wrote:
> > > On 2011/11/16 02:02:23, scherkus wrote:
> > > > On 2011/11/15 18:55:33, cbentzel wrote:
> > > > > Should this be using the network events (loadstart, progress, etc.) in
> > > > addition
> > > > > to the more playback-oriented events?
> > > > 
> > > > I agree -- loadstart, progress, and stalled are good ones to watch
> > > 
> > > The network events are not very reliable and its not clear to me which of
> them
> > > will fire. It  seems to depend more on timing. I see stalled / waiting
fire
> on
> > a
> > > couple of tests but not on all. Also the network state seems to to be
> > > "loading_data (2)" when any of canplay, canplaythrough etc.. events fire. 
> > > 
> > > Any suggestions on cases we should test. 
> > 
> > This is interesting to discuss and would like to hear cbentzel's thoughts on
> the
> > matter.
> > 
> > I think it might be worth biting the bullet and having at least one test
take
> > 3-5s to run in order to listen to loadstart/progress/stalled. The other
tests
> > don't require it and can run fast, but it's important to understand which
> > networking/load events are getting fired during prerendering.
> > 
> > What about a test for the following:
> > loadStartSeen && stalledSeen
> 
> Question: what if the deferral of loads were done in
> WebMediaPlayerClientImpl::load instead? Would that prevent the loadStart and
> stalled to be fired at all? Would that be preferable?

Nope -- WebKit's HTMLMediaElement fires the loadstart then asks us to load.

[Shishir, 2011-11-22 00:45:16]

Added relevant tests. The new test uses a sleep I am not too thrilled about, but
it seems to work.

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:16: console.log(e.type);
On 2011/11/17 23:27:50, scherkus wrote:
> On 2011/11/17 21:18:49, Shishir wrote:
> > On 2011/11/15 18:55:33, cbentzel wrote:
> > > Is this console.log statement needed anymore? 
> > 
> > Helpful for debugging failing tests, so might be worth keeping this.
> 
> I'm a big proponent of quiet tests.
> 
> If a test started failing I'd assume a developer would do the necessary things
> to debug it but leaving in log statements don't help us.

Done.

http://codereview.chromium.org/8095007/diff/54005/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:39:
mediaEl.addEventListener('playing', mediaEventHandler, false);
On 2011/11/17 23:27:50, scherkus wrote:
> On 2011/11/17 21:18:49, Shishir wrote:
> > On 2011/11/16 02:02:23, scherkus wrote:
> > > On 2011/11/15 18:55:33, cbentzel wrote:
> > > > Should this be using the network events (loadstart, progress, etc.) in
> > > addition
> > > > to the more playback-oriented events?
> > > 
> > > I agree -- loadstart, progress, and stalled are good ones to watch
> > 
> > The network events are not very reliable and its not clear to me which of
them
> > will fire. It  seems to depend more on timing. I see stalled / waiting fire
on
> a
> > couple of tests but not on all. Also the network state seems to to be
> > "loading_data (2)" when any of canplay, canplaythrough etc.. events fire. 
> > 
> > Any suggestions on cases we should test. 
> 
> This is interesting to discuss and would like to hear cbentzel's thoughts on
the
> matter.
> 
> I think it might be worth biting the bullet and having at least one test take
> 3-5s to run in order to listen to loadstart/progress/stalled. The other tests
> don't require it and can run fast, but it's important to understand which
> networking/load events are getting fired during prerendering.
> 
> What about a test for the following:
> loadStartSeen && stalledSeen

Added a test for loadStart and stalled.

[scherkus (not reviewing), 2011-11-23 01:43:31]

http://codereview.chromium.org/8095007/diff/75001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/75001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:83: sleep(5000);  //
Stalled happens after 3000 ms.
there's no way to signal when the test has completed?

this seems really unfortunate!

[Shishir, 2011-11-23 01:48:36]

http://codereview.chromium.org/8095007/diff/75001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/75001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:83: sleep(5000);  //
Stalled happens after 3000 ms.
On 2011/11/23 01:43:31, scherkus wrote:
> there's no way to signal when the test has completed?
> 
> this seems really unfortunate!

akaik the stalled event is fired when there is no progress for 3 seconds.  The
event seems to be fired on the renderer side and the browser doenst know about
it.

[scherkus (not reviewing), 2011-12-01 05:38:13]

LGTM -- I won't hold you up on tests or anything and will defer to the prerender
folks to see if they have any better ideas

http://codereview.chromium.org/8095007/diff/75001/chrome/test/data/prerender/...
File chrome/test/data/prerender/prerender_html5_common.js (right):

http://codereview.chromium.org/8095007/diff/75001/chrome/test/data/prerender/...
chrome/test/data/prerender/prerender_html5_common.js:83: sleep(5000);  //
Stalled happens after 3000 ms.
On 2011/11/23 01:48:37, Shishir wrote:
> On 2011/11/23 01:43:31, scherkus wrote:
> > there's no way to signal when the test has completed?
> > 
> > this seems really unfortunate!
> 
> akaik the stalled event is fired when there is no progress for 3 seconds.  The
> event seems to be fired on the renderer side and the browser doenst know about
> it. 

Yeah I don't really know how prerender tests work -- when/how is this function
invoked? There's no way to signal to the browser somehow that the test completed
at all?

For example what's the TitleWatcher stuff?  Could that be used at all?

[Shishir, 2011-12-01 22:50:09]

Fixed the sleep issue. Dont know how I didnt see the fix before.

[commit-bot: I haz the power, 2011-12-02 23:31:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/8095007/86001

[commit-bot: I haz the power, 2011-12-02 23:32:03]

Can't process patch for file
chrome/test/data/prerender/prerender_html5_audio.html.
File's status is None, patchset upload is incomplete.

[Shishir, 2011-12-02 23:49:05]

Adding jam for:

content/public/renderer/content_renderer_client.h
content/renderer/mock_content_renderer_client.cc
content/renderer/render_view_impl.cc
content/renderer/mock_content_renderer_client.h

[jam, 2011-12-03 00:39:08]

http://codereview.chromium.org/8095007/diff/97001/content/public/renderer/con...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8095007/diff/97001/content/public/renderer/con...
content/public/renderer/content_renderer_client.h:108: virtual bool
ShouldOverrideCreateWebMediaPlayer(RenderView* view) = 0;
we try to keep the public interface as small as possible. here, it seems that 1
method will do (i.e. OverrideCreateWebMdediaPlayer). it can have a bool to
indicate  to indicate whether it overrode or not, see OverrideCreatePlugin above

[Shishir, 2011-12-05 22:23:06]

http://codereview.chromium.org/8095007/diff/97001/content/public/renderer/con...
File content/public/renderer/content_renderer_client.h (right):

http://codereview.chromium.org/8095007/diff/97001/content/public/renderer/con...
content/public/renderer/content_renderer_client.h:108: virtual bool
ShouldOverrideCreateWebMediaPlayer(RenderView* view) = 0;
On 2011/12/03 00:39:08, John Abd-El-Malek wrote:
> we try to keep the public interface as small as possible. here, it seems that
1
> method will do (i.e. OverrideCreateWebMdediaPlayer). it can have a bool to
> indicate  to indicate whether it overrode or not, see OverrideCreatePlugin
above

Done.

[jam, 2011-12-05 22:40:16]

lgtm with nit

http://codereview.chromium.org/8095007/diff/99001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8095007/diff/99001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:286: *player = NULL;
nit: since this is an override function and you're returning false, this
shouldn't be needed

[Shishir, 2011-12-05 22:46:20]

http://codereview.chromium.org/8095007/diff/99001/chrome/renderer/chrome_cont...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8095007/diff/99001/chrome/renderer/chrome_cont...
chrome/renderer/chrome_content_renderer_client.cc:286: *player = NULL;
On 2011/12/05 22:40:17, John Abd-El-Malek wrote:
> nit: since this is an override function and you're returning false, this
> shouldn't be needed

Done.

[commit-bot: I haz the power, 2011-12-07 22:24:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/8095007/107001

[commit-bot: I haz the power, 2011-12-07 23:38:19]

Try job failure for 8095007-107001 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-08 22:37:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/8095007/112001

[commit-bot: I haz the power, 2011-12-08 23:42:25]

Try job failure for 8095007-112001 (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-09 00:04:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/8095007/114002

[commit-bot: I haz the power, 2011-12-09 02:45:47]

Change committed as 113742