Move url_request_mock_http_job to net/test/url_request/

Move url_request_mock_http_job to net/test/url_request/

This CL is a part of the effort to move mock URLRequestJob files
from content/test/net to net/test/url_request/. The reason for this
refactoring is that Cronet will need to use these classes for testing,
so it will be nice if they can be in net/test, as the dependency can
work out nicely.

BUG=

Committed: https://crrev.com/0332c191aa02efc6c24d0c87a1d41b7c12d3b8a8
Cr-Commit-Position: refs/heads/master@{#294255}

[xunjieli, 2014-09-04 13:52:23]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org

[xunjieli, 2014-09-04 17:25:00]

Hi Matt, 

This is the CL that we talked about. Could you take a look? The patch compiles
and passes browser_tests locally. I am running try jobs to check whether I miss
any. 

Let me know whether the general direction is okay.
Thanks!

Helen

[mmenke, 2014-09-04 18:05:12]

This looks really good.

https://codereview.chromium.org/541743002/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):

https://codereview.chromium.org/541743002/diff/40001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_browsertest.cc:43:
content::BrowserThread::GetBlockingPool()));
I think you need to wrap these
make_scoped_refptr(content::BrowserThread::GetBlockingPool()) to fix the compile
errors.  May need to use "make_scoped_refptr<SequencedWorkerPool>", but I don't
believe that's needed.

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.cc:26: class MockJobInterceptor :
public net::URLRequestInterceptor {
nit:  Remove all the "net::", now that we're in net/

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.cc:105: std::string url =
"view-source";
This is a layering violation.  net shouldn't know about the view-source scheme.

This function is only called from two files, so I suggest just getting rid of
this function and inlining instead.  I took a look at where we "officially" att
the view-source scheme to URLs, and it's inlined there as well
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
adn
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...)

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
File net/test/url_request/url_request_mock_http_job.h (right):

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:12: #include
"net/url_request/url_request_file_job.h"
This was a bug from before, but should base/memory/scoped_ptr.h and url/gurl.h

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:15: class FilePath;
Need to forward declare SequencedWorkerPool

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:24: class URLRequestMockHTTPJob
: public net::URLRequestFileJob {
"net::" no longer needed in this file.

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:42: base::SequencedWorkerPool*
worker_pool);
These should probably actually be:

const scoped_refptr<base::SequencedWorkerPool>& worker_pool (And move the refptr
all the way down into the interceptor)

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:79: void
GetResponseInfoConst(net::HttpResponseInfo* info) const;
Another bug from before:  There should be a
"DISALLOW_COPY_AND_ASSIGN(URLRequestMockHTTPJob);" here (And include
"base/macros.h" for that)

[xunjieli, 2014-09-04 18:07:11]

Patchset #4 (id:60001) has been deleted

[xunjieli, 2014-09-04 18:44:25]

Thanks Matt! I have addressed your comments. PTAL. Thanks!

https://codereview.chromium.org/541743002/diff/40001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):

https://codereview.chromium.org/541743002/diff/40001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_browsertest.cc:43:
content::BrowserThread::GetBlockingPool()));
On 2014/09/04 18:05:11, mmenke wrote:
> I think you need to wrap these
> make_scoped_refptr(content::BrowserThread::GetBlockingPool()) to fix the
compile
> errors.  May need to use "make_scoped_refptr<SequencedWorkerPool>", but I
don't
> believe that's needed.

Done. Thanks!

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.cc:105: std::string url =
"view-source";
On 2014/09/04 18:05:12, mmenke wrote:
> This is a layering violation.  net shouldn't know about the view-source
scheme.
> 
> This function is only called from two files, so I suggest just getting rid of
> this function and inlining instead.  I took a look at where we "officially"
att
> the view-source scheme to URLs, and it's inlined there as well
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> adn
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...)

Done. Thanks! That was a bad thing to do. I was planning to ask you about it,
but somehow it got lost when I tried to make the change compile. Thanks for
being thorough!

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
File net/test/url_request/url_request_mock_http_job.h (right):

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:24: class URLRequestMockHTTPJob
: public net::URLRequestFileJob {
On 2014/09/04 18:05:12, mmenke wrote:
> "net::" no longer needed in this file.

Done.

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:42: base::SequencedWorkerPool*
worker_pool);
On 2014/09/04 18:05:12, mmenke wrote:
> These should probably actually be:
> 
> const scoped_refptr<base::SequencedWorkerPool>& worker_pool (And move the
refptr
> all the way down into the interceptor)

Done.

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:79: void
GetResponseInfoConst(net::HttpResponseInfo* info) const;
On 2014/09/04 18:05:12, mmenke wrote:
> Another bug from before:  There should be a
> "DISALLOW_COPY_AND_ASSIGN(URLRequestMockHTTPJob);" here (And include
> "base/macros.h" for that)

Done. Thanks!

[mmenke, 2014-09-04 19:02:38]

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:8: #include "base/macros.h"
This should be in the header file (After adding DISALLOW_COPY_AND_ASSIGN to the
URLRequestMockHTTPJob class)

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:66: const
scoped_refptr<base::SequencedWorkerPool>& worker_pool_;
IMPORTANT:  Remove the &.  Otherwise, we'll have a reference to the
scoped_refptr owned back the callback object, which is destroyed once the
callback has been run and then deleted.

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.h (right):

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:15: class FilePath;
Think you missed some of my comment in this file (Forward declaration, missing
includes, DISALLOW_COPY_AND_ASSIGN)

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:42: const
scoped_refptr<base::SequencedWorkerPool>& worker_pool);
Should include base/memory/ref_counted.h here (And make sure files you added
make_scoped_refptr to include it as well)

[xunjieli, 2014-09-04 19:24:16]

Thanks Matt!

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
File net/test/url_request/url_request_mock_http_job.h (right):

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:12: #include
"net/url_request/url_request_file_job.h"
On 2014/09/04 18:05:12, mmenke wrote:
> This was a bug from before, but should base/memory/scoped_ptr.h and url/gurl.h

Done. Sorry I didn't see this.

https://codereview.chromium.org/541743002/diff/40001/net/test/url_request/url...
net/test/url_request/url_request_mock_http_job.h:15: class FilePath;
On 2014/09/04 18:05:12, mmenke wrote:
> Need to forward declare SequencedWorkerPool

Done.

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:8: #include "base/macros.h"
Done. Should have read your comment more carefully.

On 2014/09/04 19:02:38, mmenke wrote:
> This should be in the header file (After adding DISALLOW_COPY_AND_ASSIGN to
the
> URLRequestMockHTTPJob class)

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:66: const
scoped_refptr<base::SequencedWorkerPool>& worker_pool_;
On 2014/09/04 19:02:38, mmenke wrote:
> IMPORTANT:  Remove the &.  Otherwise, we'll have a reference to the
> scoped_refptr owned back the callback object, which is destroyed once the
> callback has been run and then deleted.

Done.

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.h (right):

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:15: class FilePath;
On 2014/09/04 19:02:38, mmenke wrote:
> Think you missed some of my comment in this file (Forward declaration, missing
> includes, DISALLOW_COPY_AND_ASSIGN)

Acknowledged.

https://codereview.chromium.org/541743002/diff/100001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:42: const
scoped_refptr<base::SequencedWorkerPool>& worker_pool);
On 2014/09/04 19:02:38, mmenke wrote:
> Should include base/memory/ref_counted.h here (And make sure files you added
> make_scoped_refptr to include it as well)

Done.

[mmenke, 2014-09-04 19:43:44]

I'll carefully go over this tomorrow, will probably be the last pass.

[mmenke, 2014-09-05 15:54:10]

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:151:
base::ThreadRestrictions::SetIOAllowed(true);
This has me rather concerned - this basically allows anything that wants to do
file IO on the (Misleadingly named) IO thread, so we lose test coverage of the
fact we should, in general, never do file IO on that thread, in production code.

So how do we fix it?  We could just disallow IO again, but that seems very
regression prone, and is basically getting around whatever warning you're
getting about ScopedAllowIO by just inlining it, which isn't really the
intention of the warning.

So we already have a task runner that we can do file IO on that we're passing to
the base class.  Let's just reuse it.  You can add a Start() function, and have
it call PostTaskAndReplyWithResult on the task runner.  The function should just
take in a FilePath, and return the header string or the HttpResponseHeaders
(Since the latter is refcounted, a string may be a little simpler, though less
efficient).

The function that gets the result back should then set a class variable that
contains the headers, and call URLRequestFileJob::Start, and then this function
should just return those headers, and we're done.

Sorry to go so far afield of the original intention of this CL.  The old code
was just grandfathered in.

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.h (right):

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:31: URLRequestMockHTTPJob(
Think it's worth a comment that all file IO is done using the |worker_pool|.

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:62: static GURL
GetMockViewSourceUrl(const base::FilePath& path);
This is no longer implemented.

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:86:
DISALLOW_COPY_AND_ASSIGN(URLRequestMockHTTPJob);
nit:  Blank line before DISALLOW_COPY_AND_ASSIGN.

[xunjieli, 2014-09-05 19:26:15]

Thanks Matt! PTAL

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:151:
base::ThreadRestrictions::SetIOAllowed(true);
Done. If it's a problem, we should definitely fix it. Thanks for the detailed
how-tos!

On 2014/09/05 15:54:09, mmenke wrote:
> This has me rather concerned - this basically allows anything that wants to do
> file IO on the (Misleadingly named) IO thread, so we lose test coverage of the
> fact we should, in general, never do file IO on that thread, in production
code.
> 
> So how do we fix it?  We could just disallow IO again, but that seems very
> regression prone, and is basically getting around whatever warning you're
> getting about ScopedAllowIO by just inlining it, which isn't really the
> intention of the warning.
> 
> So we already have a task runner that we can do file IO on that we're passing
to
> the base class.  Let's just reuse it.  You can add a Start() function, and
have
> it call PostTaskAndReplyWithResult on the task runner.  The function should
just
> take in a FilePath, and return the header string or the HttpResponseHeaders
> (Since the latter is refcounted, a string may be a little simpler, though less
> efficient).
> 
> The function that gets the result back should then set a class variable that
> contains the headers, and call URLRequestFileJob::Start, and then this
function
> should just return those headers, and we're done.
> 
> Sorry to go so far afield of the original intention of this CL.  The old code
> was just grandfathered in.

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.h (right):

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:31: URLRequestMockHTTPJob(
On 2014/09/05 15:54:10, mmenke wrote:
> Think it's worth a comment that all file IO is done using the |worker_pool|.

Done.

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:62: static GURL
GetMockViewSourceUrl(const base::FilePath& path);
On 2014/09/05 15:54:10, mmenke wrote:
> This is no longer implemented.

Done. Good catch. Thanks!

https://codereview.chromium.org/541743002/diff/120001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.h:86:
DISALLOW_COPY_AND_ASSIGN(URLRequestMockHTTPJob);
On 2014/09/05 15:54:10, mmenke wrote:
> nit:  Blank line before DISALLOW_COPY_AND_ASSIGN.

Done.

[mmenke, 2014-09-05 20:06:15]

Just two things, and I'm good.  Not sure why the bots are unhappy.

https://codereview.chromium.org/541743002/diff/140001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/140001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:133:
base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)),
Rather than creating two TaskRunners, I think we should just create one in
MaybeInterceptRequest, and pass it in to the URLRequestMockHTTPJob constructor,
so the two classes can share a single TaskRunner.

https://codereview.chromium.org/541743002/diff/140001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:165:
base::FilePath(file_path_.value() + kMockHeaderFileSuffix);
It's unsafe to dereference members of URLRequestMockHTTPJob on this thread - the
mock job may have been cancelled by the time this executes.  Instead, this
function should not be a member of URLRequestMockHTTPJob, it should be in the
anonymous namespace at the top of this file, and take in the FilePath as an
argument.  They way, it can still run after the destruction of the
URLRequestMockHTTPJob (The callback that's run on the source thread will then
notice it's been deleted when it's invoked, and safely do nothing).

[xunjieli, 2014-09-05 20:59:45]

Thanks Matt! PTAL

https://codereview.chromium.org/541743002/diff/140001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/140001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:133:
base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)),
On 2014/09/05 20:06:15, mmenke wrote:
> Rather than creating two TaskRunners, I think we should just create one in
> MaybeInterceptRequest, and pass it in to the URLRequestMockHTTPJob
constructor,
> so the two classes can share a single TaskRunner.

Done.The callers of the URLRequestMockHTTPJob will now pass in a task runner
instead of a worker pool right?

https://codereview.chromium.org/541743002/diff/140001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:165:
base::FilePath(file_path_.value() + kMockHeaderFileSuffix);
On 2014/09/05 20:06:15, mmenke wrote:
> It's unsafe to dereference members of URLRequestMockHTTPJob on this thread -
the
> mock job may have been cancelled by the time this executes.  Instead, this
> function should not be a member of URLRequestMockHTTPJob, it should be in the
> anonymous namespace at the top of this file, and take in the FilePath as an
> argument.  They way, it can still run after the destruction of the
> URLRequestMockHTTPJob (The callback that's run on the source thread will then
> notice it's been deleted when it's invoked, and safely do nothing).

Done.I will this in mind. Thanks for explaining!

[mmenke, 2014-09-05 21:10:29]

Ahh, looks like you'll need to rebase because of changes to
content/browser/download/download_browsertest.cc.  Do you know how to do that? 
I'm ready to sign off, after a green try run (And fixing the indents).

https://codereview.chromium.org/541743002/diff/140001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/140001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:133:
base::SequencedWorkerPool::SKIP_ON_SHUTDOWN)),
On 2014/09/05 20:59:44, xunjieli1 wrote:
> On 2014/09/05 20:06:15, mmenke wrote:
> > Rather than creating two TaskRunners, I think we should just create one in
> > MaybeInterceptRequest, and pass it in to the URLRequestMockHTTPJob
> constructor,
> > so the two classes can share a single TaskRunner.
> 
> Done.The callers of the URLRequestMockHTTPJob will now pass in a task runner
> instead of a worker pool right?

Yes, they will - I hadn't considered that.  Still think it's the way to go,
though.

https://codereview.chromium.org/541743002/diff/180001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/180001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:50: }
nit:  You seem to have accidentally indented the rest of this file by 2 more
than it should be.

[xunjieli, 2014-09-05 21:16:39]

Thanks Matt for catching the indentation part! I rebased and am running try jobs
now.

https://codereview.chromium.org/541743002/diff/180001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/180001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:50: }
Thanks for catching that! I couldn't figure out why the review tool shows so
much change. thanks!

[mmenke, 2014-09-08 16:20:17]

On 2014/09/05 21:16:39, xunjieli1 wrote:
> Thanks Matt for catching the indentation part! I rebased and am running try
jobs
> now.
> 
>
https://codereview.chromium.org/541743002/diff/180001/net/test/url_request/ur...
> File net/test/url_request/url_request_mock_http_job.cc (right):
> 
>
https://codereview.chromium.org/541743002/diff/180001/net/test/url_request/ur...
> net/test/url_request/url_request_mock_http_job.cc:50: }
> Thanks for catching that! I couldn't figure out why the review tool shows so
> much change. thanks!

Looks like we're getting a lot of test failures on some of the modified tests. 
I suggest you start investigating with the PrerenderTracketTest (prerendering is
complicated, but those are unit tests, so may be simpler to investigate than
full fledged browser tests).  Looks like the jobs aren't completing.  Your code
looks correct to me, but I'd starting with investigating the callback to read
the headers.

[littleone691_gmail.com, 2014-09-08 16:22:03]

thanks





On Mon, Sep 8, 2014 at 12:20 PM, <mmenke@chromium.org> wrote:

> On 2014/09/05 21:16:39, xunjieli1 wrote:
>
>> Thanks Matt for catching the indentation part! I rebased and am running
>> try
>>
> jobs
>
>> now.
>>
>
>
> https://codereview.chromium.org/541743002/diff/180001/net/
> test/url_request/url_request_mock_http_job.cc
>
>> File net/test/url_request/url_request_mock_http_job.cc (right):
>>
>
>
> https://codereview.chromium.org/541743002/diff/180001/net/
> test/url_request/url_request_mock_http_job.cc#newcode50
>
>> net/test/url_request/url_request_mock_http_job.cc:50: }
>> Thanks for catching that! I couldn't figure out why the review tool shows
>> so
>> much change. thanks!
>>
>
> Looks like we're getting a lot of test failures on some of the modified
> tests.
> I suggest you start investigating with the PrerenderTracketTest
> (prerendering is
> complicated, but those are unit tests, so may be simpler to investigate
> than
> full fledged browser tests).  Looks like the jobs aren't completing.  Your
> code
> looks correct to me, but I'd starting with investigating the callback to
> read
> the headers.
>
> https://codereview.chromium.org/541743002/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Extensions reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to extensions-reviews+unsubscribe@chromium.org.
> To view this discussion on the web visit https://groups.google.com/a/
> chromium.org/d/msgid/extensions-reviews/90e6ba613c2ccc407705029031cb%
> 40google.com.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-09-08 18:10:13]

Thanks Matt! I will start investigating. Asanka suggested me to look
into DownloadOctetStream
which is a straightforward test case. I will let you know what I find.

On Mon, Sep 8, 2014 at 12:21 PM, Jahlen Lay <littleone691@gmail.com> wrote:

> thanks
>
>
>
>
>
> On Mon, Sep 8, 2014 at 12:20 PM, <mmenke@chromium.org> wrote:
>
>> On 2014/09/05 21:16:39, xunjieli1 wrote:
>>
>>> Thanks Matt for catching the indentation part! I rebased and am running
>>> try
>>>
>> jobs
>>
>>> now.
>>>
>>
>>
>> https://codereview.chromium.org/541743002/diff/180001/net/
>> test/url_request/url_request_mock_http_job.cc
>>
>>> File net/test/url_request/url_request_mock_http_job.cc (right):
>>>
>>
>>
>> https://codereview.chromium.org/541743002/diff/180001/net/
>> test/url_request/url_request_mock_http_job.cc#newcode50
>>
>>> net/test/url_request/url_request_mock_http_job.cc:50: }
>>> Thanks for catching that! I couldn't figure out why the review tool
>>> shows so
>>> much change. thanks!
>>>
>>
>> Looks like we're getting a lot of test failures on some of the modified
>> tests.
>> I suggest you start investigating with the PrerenderTracketTest
>> (prerendering is
>> complicated, but those are unit tests, so may be simpler to investigate
>> than
>> full fledged browser tests).  Looks like the jobs aren't completing.
>> Your code
>> looks correct to me, but I'd starting with investigating the callback to
>> read
>> the headers.
>>
>> https://codereview.chromium.org/541743002/
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Extensions reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to extensions-reviews+unsubscribe@chromium.org.
>> To view this discussion on the web visit https://groups.google.com/a/
>> chromium.org/d/msgid/extensions-reviews/90e6ba613c2ccc407705029031cb%
>> 40google.com.
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[asanka, 2014-09-08 18:51:43]

asanka@chromium.org changed reviewers:
+ asanka@chromium.org

[asanka, 2014-09-08 18:51:44]

https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:171:
weak_ptr_factory_.GetWeakPtr()));
Nit: Run this change through 'git cl format'. The indentation doesn't look
right.

https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:176:
ReplaceSubstringsAfterOffset(&raw_headers_, 0, "\r\n", "\n");
You are modifying raw_headers_. You want raw_headers.

[xunjieli, 2014-09-08 19:00:22]

Still investigating why some of the tests timeout. Uploading a patch to fix the
two issues that Asanka has brought up.

https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:171:
weak_ptr_factory_.GetWeakPtr()));
On 2014/09/08 18:51:44, asanka wrote:
> Nit: Run this change through 'git cl format'. The indentation doesn't look
> right.

Done. Thanks for the tip! "git cl format" is very cool!

https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:176:
ReplaceSubstringsAfterOffset(&raw_headers_, 0, "\r\n", "\n");
On 2014/09/08 18:51:44, asanka wrote:
> You are modifying raw_headers_. You want raw_headers.

Done. Good catch! Thanks!

[mmenke, 2014-09-08 19:14:31]

https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
File net/test/url_request/url_request_mock_http_job.cc (right):

https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
net/test/url_request/url_request_mock_http_job.cc:176:
ReplaceSubstringsAfterOffset(&raw_headers_, 0, "\r\n", "\n");
On 2014/09/08 19:00:22, xunjieli1 wrote:
> On 2014/09/08 18:51:44, asanka wrote:
> > You are modifying raw_headers_. You want raw_headers.
> 
> Done. Good catch! Thanks!

Is that what was causing the test failures?

[xunjieli, 2014-09-08 19:21:52]

On 2014/09/08 19:14:31, mmenke wrote:
>
https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
> File net/test/url_request/url_request_mock_http_job.cc (right):
> 
>
https://codereview.chromium.org/541743002/diff/200001/net/test/url_request/ur...
> net/test/url_request/url_request_mock_http_job.cc:176:
> ReplaceSubstringsAfterOffset(&raw_headers_, 0, "\r\n", "\n");
> On 2014/09/08 19:00:22, xunjieli1 wrote:
> > On 2014/09/08 18:51:44, asanka wrote:
> > > You are modifying raw_headers_. You want raw_headers.
> > 
> > Done. Good catch! Thanks!
> 
> Is that what was causing the test failures?

That seems to be it. I am resubmitting try jobs just to make sure. Thanks Asanka
and Matt for finding the bug! Should have be more careful last Friday.

[xunjieli, 2014-09-09 18:49:46]

Patchset #13 (id:260001) has been deleted

[xunjieli, 2014-09-09 19:02:23]

Patchset #13 (id:280001) has been deleted

[xunjieli, 2014-09-09 21:02:15]

Matt, try jobs are all green now! (except linux_redux, which has been failing
for other ppl too). PTAL.

[mmenke, 2014-09-09 21:15:23]

LGTM.  So you'll need owners signoffs to land this.  I suggest getting a
browser/policy owner to sign off on the policy_browsertest change, and for the
others just get one content/ owner and one chrome/ owner, as there are just too
many affected directories to use lower level owners.

https://codereview.chromium.org/541743002/diff/290001/content/browser/databas...
File content/browser/database_browsertest.cc (right):

https://codereview.chromium.org/541743002/diff/290001/content/browser/databas...
content/browser/database_browsertest.cc:17: #include
"net/test/url_request/url_request_mock_http_job.h"
This isn't used in this file, so let's get rid of it.

[xunjieli, 2014-09-09 21:28:15]

xunjieli@chromium.org changed reviewers:
+ avi@chromium.org, pastarmovj@chromium.org, thestig@chromium.org

[xunjieli, 2014-09-09 21:28:16]

+pastarmovj for browser/policy
+avi for content/
+thestig for chrome/

Hi pastarmovj, avi and thesig, could you take a look at my CL? I need owner's
approval. Thank you!

https://codereview.chromium.org/541743002/diff/290001/content/browser/databas...
File content/browser/database_browsertest.cc (right):

https://codereview.chromium.org/541743002/diff/290001/content/browser/databas...
content/browser/database_browsertest.cc:17: #include
"net/test/url_request/url_request_mock_http_job.h"
On 2014/09/09 21:15:23, mmenke wrote:
> This isn't used in this file, so let's get rid of it.

Done. Good catch! thanks!

[Lei Zhang, 2014-09-09 21:37:59]

chrome/ sans c/b/{net,policy} gltm with some nits.

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_browsertest.cc (right):

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_browsertest.cc:33: path,
content::BrowserThread::GetBlockingPool());
nit: no need for content:: because of line 28 above.

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/captive_...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:59: using
net::URLRequestMockHTTPJob;
nit: sort

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:431:
content::BrowserThread::GetBlockingPool()
nit: Write this as
BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(

so it fits on 1 line. Ditto below and in some other files.

[Avi (use Gerrit), 2014-09-09 21:38:56]

content lgtm

[pastarmovj, 2014-09-09 21:42:35]

policy lgtm

[xunjieli, 2014-09-09 22:08:59]

Thanks for the reviews! thesig@, could you take another look?

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/browsing...
File chrome/browser/browsing_data/browsing_data_remover_browsertest.cc (right):

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/browsing...
chrome/browser/browsing_data/browsing_data_remover_browsertest.cc:33: path,
content::BrowserThread::GetBlockingPool());
On 2014/09/09 21:37:59, Lei Zhang wrote:
> nit: no need for content:: because of line 28 above.

Done.

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/captive_...
File chrome/browser/captive_portal/captive_portal_browsertest.cc (right):

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:59: using
net::URLRequestMockHTTPJob;
On 2014/09/09 21:37:59, Lei Zhang wrote:
> nit: sort

Done.

https://codereview.chromium.org/541743002/diff/310001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_browsertest.cc:431:
content::BrowserThread::GetBlockingPool()
On 2014/09/09 21:37:59, Lei Zhang wrote:
> nit: Write this as
> BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
> 
> so it fits on 1 line. Ditto below and in some other files.

Done.Thanks!

[Lei Zhang, 2014-09-09 22:11:33]

lgtm

[xunjieli, 2014-09-09 22:12:00]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:13:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xunjieli@chromium.org/541743002/330001

[commit-bot: I haz the power, 2014-09-09 22:38:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:38:21]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/53679)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xunjieli, 2014-09-09 23:53:42]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2014-09-10 00:04:17]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2014-09-10 00:19:07]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2014-09-10 00:30:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xunjieli@chromium.org/541743002/350001

[commit-bot: I haz the power, 2014-09-10 01:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 01:33:59]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[xunjieli, 2014-09-10 13:36:17]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2014-09-10 13:36:43]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2014-09-10 14:01:33]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2014-09-10 14:02:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xunjieli@chromium.org/541743002/370001

[commit-bot: I haz the power, 2014-09-10 22:42:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:42:31]

Failed to commit the patch.

[xunjieli, 2014-09-10 22:51:48]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2014-09-10 23:18:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541743002/370001

[commit-bot: I haz the power, 2014-09-10 23:25:05]

Committed patchset #17 (id:370001) as 82ad9867f1f222c6d08728ba3702775a8ed1310d

[commit-bot: I haz the power, 2014-09-10 23:27:22]

Patchset 17 (id:??) landed as
https://crrev.com/0332c191aa02efc6c24d0c87a1d41b7c12d3b8a8
Cr-Commit-Position: refs/heads/master@{#294255}