Collapse HTTP and non-HTTP codepaths into single methods.

Collapse HTTP and non-HTTP codepaths into single methods.

Duplicating ~100 lines of code for the ~5 lines of code that are different isn't worth it.

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=146242

[scherkus (not reviewing), 2012-07-06 16:28:02]

Depends on http://codereview.chromium.org/10692106/ -- after some investigation
I realized that the HTTP and non-HTTP codepaths only differ by a few lines. I
don't think it's worth having them completely separated especially given the
duplicated hairy locking code we have at the moment.

In the long run I want WMPI to be the only place where we make decisions based
on the URL scheme but baby steps...

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.cc (left):

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:276:
base::Bind(&BufferedDataSource::NonHttpInitialStartCallback, this),
I believe this was a bug!

if for whatever reason we retried a failed read on non-HTTP resources we would
never execute the read again

[Ami GONE FROM CHROMIUM, 2012-07-09 03:30:57]

minor comments.
I love this CL.

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.cc (left):

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:276:
base::Bind(&BufferedDataSource::NonHttpInitialStartCallback, this),
On 2012/07/06 16:28:02, scherkus wrote:
> I believe this was a bug!
> 
> if for whatever reason we retried a failed read on non-HTTP resources we would
> never execute the read again

Worth a test?

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:97: // For all other protocols, assume they
support range request. We fetch
s/support/don't support/?

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:361: // HTTP responses only require a
successful response even if content length
I think you meant the "only" to bind to "HTTP responses" instead of "require".
Reword?

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:361: // HTTP responses only require a
successful response even if content length
I think this if/else would be a lot clearer if you laid out the criteria for
success without interleaving them with member-setting statements.

bool is_http = url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme);
bool success = (status == BufferedResourceLoader::kOk) && 
    (is_http || loader_->instance_size() != kPositionNotSpecified);

if (success) {
  if (is_http) {
    ...
  } else {
    ...
  }
} else {
  loader_->Stop();
}

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:375: buffered_bytes_ =
loader_->instance_size();
This is consistent with the "before" code, but is it right?  Does this CB only
fire after the full FTP file (or whatevs) is d/l'ed, or do we lie?

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:471: // We assume non-HTTP resources are
fully loaded: ignore any udpates.
udpates

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:471: // We assume non-HTTP resources are
fully loaded: ignore any udpates.
Can we DCHECK the assumption?

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:511: // We assume non-HTTP resources are
fully loaded: ignore any udpates.
ditto to DCHECKing if possible.

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.h (right):

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.h:120: // when accessing ranges that are
outside initial buffered region).
Old comment made promises about thread of execution.  
Should new code make same/different/no promise?

[scherkus (not reviewing), 2012-07-10 18:37:27]

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.cc (left):

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:276:
base::Bind(&BufferedDataSource::NonHttpInitialStartCallback, this),
On 2012/07/09 03:30:57, Ami Fischman wrote:
> On 2012/07/06 16:28:02, scherkus wrote:
> > I believe this was a bug!
> > 
> > if for whatever reason we retried a failed read on non-HTTP resources we
would
> > never execute the read again
> 
> Worth a test?

I'm going to prep a separate CL that I'll land before this CL that beefs up our
test coverage -- we have no tests for non-HTTP codepath at all :s

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:97: // For all other protocols, assume they
support range request. We fetch
On 2012/07/09 03:30:57, Ami Fischman wrote:
> s/support/don't support/?

My memory is foggy but I believe it _is_ support but IIRC we don't get a C-L
header or something when we issue ranges against non-HTTP protocols (i.e.,
bytes=0-), hence issuing a regular GET

I suppose the "real fix" is digging into protocol handler to see when we do get
/ don't get C-L/C-R headers

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:361: // HTTP responses only require a
successful response even if content length
On 2012/07/09 03:30:57, Ami Fischman wrote:
> I think this if/else would be a lot clearer if you laid out the criteria for
> success without interleaving them with member-setting statements.
> 
> bool is_http = url_.SchemeIs(kHttpScheme) || url_.SchemeIs(kHttpsScheme);
> bool success = (status == BufferedResourceLoader::kOk) && 
>     (is_http || loader_->instance_size() != kPositionNotSpecified);
> 
> if (success) {
>   if (is_http) {
>     ...
>   } else {
>     ...
>   }
> } else {
>   loader_->Stop();
> }

Done.

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:361: // HTTP responses only require a
successful response even if content length
On 2012/07/09 03:30:57, Ami Fischman wrote:
> I think you meant the "only" to bind to "HTTP responses" instead of "require".
> Reword?

Done.

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:375: buffered_bytes_ =
loader_->instance_size();
On 2012/07/09 03:30:57, Ami Fischman wrote:
> This is consistent with the "before" code, but is it right?  Does this CB only
> fire after the full FTP file (or whatevs) is d/l'ed, or do we lie?

We currently lie for all non-HTTP protocols

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:471: // We assume non-HTTP resources are
fully loaded: ignore any udpates.
On 2012/07/09 03:30:57, Ami Fischman wrote:
> udpates

Done.

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:471: // We assume non-HTTP resources are
fully loaded: ignore any udpates.
On 2012/07/09 03:30:57, Ami Fischman wrote:
> Can we DCHECK the assumption?

I've switched to using assume_fully_buffered_ since buffered_bytes_ was only
used for non-HTTP case -- let me know what you think

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.cc:511: // We assume non-HTTP resources are
fully loaded: ignore any udpates.
On 2012/07/09 03:30:57, Ami Fischman wrote:
> ditto to DCHECKing if possible.

Done.

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
File webkit/media/buffered_data_source.h (right):

https://chromiumcodereview.appspot.com/10735016/diff/1/webkit/media/buffered_...
webkit/media/buffered_data_source.h:120: // when accessing ranges that are
outside initial buffered region).
On 2012/07/09 03:30:57, Ami Fischman wrote:
> Old comment made promises about thread of execution.  
> Should new code make same/different/no promise?

BRL lives + executes on render thread as do the callbacks -- I'll update BRL's
class comment to be more clear

[scherkus (not reviewing), 2012-07-11 00:25:44]

You'll want to deal with https://chromiumcodereview.appspot.com/10698139/ first

https://chromiumcodereview.appspot.com/10735016/diff/1005/webkit/media/buffer...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10735016/diff/1005/webkit/media/buffer...
webkit/media/buffered_data_source.cc:111: assume_fully_buffered_ = true;
NOTE!!!

my newly minted tests revealed that since StartCallback() is executed after
LoadingStateChangedCallback(), we would report that we are downloading in the
non-HTTP case

I've isolated the HTTP-vs-non-HTTP checks to the single one here and replaced
subsequent ones with assume_fully_buffered_

[Ami GONE FROM CHROMIUM, 2012-07-11 00:56:41]

https://chromiumcodereview.appspot.com/10735016/diff/6002/webkit/media/buffer...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10735016/diff/6002/webkit/media/buffer...
webkit/media/buffered_data_source.cc:386: assume_fully_buffered_ = true;
you want this for ftp??

https://chromiumcodereview.appspot.com/10735016/diff/6002/webkit/media/buffer...
File webkit/media/buffered_data_source.h (right):

https://chromiumcodereview.appspot.com/10735016/diff/6002/webkit/media/buffer...
webkit/media/buffered_data_source.h:151: // need to report the what |loader_|
has buffered.
drop "the"

https://chromiumcodereview.appspot.com/10735016/diff/1005/webkit/media/buffer...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10735016/diff/1005/webkit/media/buffer...
webkit/media/buffered_data_source.cc:111: assume_fully_buffered_ = true;
I mildly dislike that assume_fully_buffered_ === !is_http.
I don't know how to improve this.

[scherkus (not reviewing), 2012-07-11 18:39:01]

READY TO REVIEW!

rebased on trunk + comments + etc

https://chromiumcodereview.appspot.com/10735016/diff/6002/webkit/media/buffer...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10735016/diff/6002/webkit/media/buffer...
webkit/media/buffered_data_source.cc:386: assume_fully_buffered_ = true;
On 2012/07/11 00:56:41, Ami Fischman wrote:
> you want this for ftp??

Not ideally but this patch doesn't change that fact.

TODO-ify? We have http://crbug.com/113736

https://chromiumcodereview.appspot.com/10735016/diff/6002/webkit/media/buffer...
File webkit/media/buffered_data_source.h (right):

https://chromiumcodereview.appspot.com/10735016/diff/6002/webkit/media/buffer...
webkit/media/buffered_data_source.h:151: // need to report the what |loader_|
has buffered.
On 2012/07/11 00:56:41, Ami Fischman wrote:
> drop "the"

Done.

https://chromiumcodereview.appspot.com/10735016/diff/1005/webkit/media/buffer...
File webkit/media/buffered_data_source.cc (right):

https://chromiumcodereview.appspot.com/10735016/diff/1005/webkit/media/buffer...
webkit/media/buffered_data_source.cc:111: assume_fully_buffered_ = true;
On 2012/07/11 00:56:41, Ami Fischman wrote:
> I mildly dislike that assume_fully_buffered_ === !is_http.
> I don't know how to improve this.

That's true today but perhaps one day ftp:// (and other network-y protocols)
will not get included w/ assume_fully_buffered_ but it requires more
investigation

[Ami GONE FROM CHROMIUM, 2012-07-11 18:50:21]

LGTM

[commit-bot: I haz the power, 2012-07-11 22:21:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/10735016/1006

[commit-bot: I haz the power, 2012-07-11 23:39:20]

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