Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(265)

Issue 19866006: [Net] Propagate priority changes from HttpNetworkTransaction to its request (Closed)

Created:
7 years, 5 months ago by akalin
Modified:
7 years, 4 months ago
Reviewers:
mmenke
CC:
chromium-reviews, cbentzel+watch_chromium.org
Visibility:
Public.

Description

[Net] Propagate priority changes from HttpNetworkTransaction to its request Also propagate it further down to HttpStreamFactoryImpl::Job. BUG=166689 R=mmenke@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217582

Patch Set 1 #

Total comments: 2

Patch Set 2 : Address comments #

Patch Set 3 : Minor fixes #

Patch Set 4 : Fix Win, use WeakPtrs #

Total comments: 2

Patch Set 5 : Address comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+288 lines, -6 lines) Patch
M net/http/http_network_transaction.cc View 1 2 3 4 1 chunk +3 lines, -2 lines 0 comments Download
M net/http/http_network_transaction_unittest.cc View 1 2 3 2 chunks +158 lines, -0 lines 0 comments Download
M net/http/http_stream_factory.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M net/http/http_stream_factory_impl.h View 1 2 3 2 chunks +5 lines, -2 lines 0 comments Download
M net/http/http_stream_factory_impl_job.h View 1 chunk +3 lines, -0 lines 0 comments Download
M net/http/http_stream_factory_impl_job.cc View 1 chunk +7 lines, -2 lines 0 comments Download
M net/http/http_stream_factory_impl_request.h View 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_stream_factory_impl_request.cc View 1 chunk +9 lines, -0 lines 0 comments Download
A net/http/http_stream_factory_impl_request_unittest.cc View 1 2 3 4 1 chunk +98 lines, -0 lines 1 comment Download
M net/net.gyp View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
akalin
+mmenke for review
7 years, 5 months ago (2013-07-23 00:56:23 UTC) #1
mmenke
https://codereview.chromium.org/19866006/diff/1/net/http/http_network_transaction_unittest.cc File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/19866006/diff/1/net/http/http_network_transaction_unittest.cc#newcode11755 net/http/http_network_transaction_unittest.cc:11755: EXPECT_EQ(MEDIUM, request->priority()); Again, these requests that just write directly ...
7 years, 5 months ago (2013-07-23 18:33:33 UTC) #2
akalin
PTAL! https://codereview.chromium.org/19866006/diff/1/net/http/http_network_transaction_unittest.cc File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/19866006/diff/1/net/http/http_network_transaction_unittest.cc#newcode11755 net/http/http_network_transaction_unittest.cc:11755: EXPECT_EQ(MEDIUM, request->priority()); On 2013/07/23 18:33:33, mmenke wrote: > ...
7 years, 4 months ago (2013-08-09 18:53:35 UTC) #3
akalin
On 2013/08/09 18:53:35, akalin wrote: > PTAL! > > https://codereview.chromium.org/19866006/diff/1/net/http/http_network_transaction_unittest.cc > File net/http/http_network_transaction_unittest.cc (right): > ...
7 years, 4 months ago (2013-08-12 21:27:32 UTC) #4
mmenke
On 2013/08/12 21:27:32, akalin wrote: > On 2013/08/09 18:53:35, akalin wrote: > > PTAL! > ...
7 years, 4 months ago (2013-08-12 21:34:56 UTC) #5
mmenke
LGTM, modulo comments https://codereview.chromium.org/19866006/diff/34001/net/http/http_stream_factory_impl_request_unittest.cc File net/http/http_stream_factory_impl_request_unittest.cc (right): https://codereview.chromium.org/19866006/diff/34001/net/http/http_stream_factory_impl_request_unittest.cc#newcode85 net/http/http_stream_factory_impl_request_unittest.cc:85: request.AttachJob(job); EXPECT_EQ(DEFAULT_PRIORITY, job->priority());? Also, really should ...
7 years, 4 months ago (2013-08-14 14:46:34 UTC) #6
mmenke
LGTM, modulo comments https://codereview.chromium.org/19866006/diff/34001/net/http/http_stream_factory_impl_request_unittest.cc File net/http/http_stream_factory_impl_request_unittest.cc (right): https://codereview.chromium.org/19866006/diff/34001/net/http/http_stream_factory_impl_request_unittest.cc#newcode85 net/http/http_stream_factory_impl_request_unittest.cc:85: request.AttachJob(job); EXPECT_EQ(DEFAULT_PRIORITY, job->priority());? Also, really should ...
7 years, 4 months ago (2013-08-14 14:46:34 UTC) #7
akalin
Committing soon https://codereview.chromium.org/19866006/diff/34001/net/http/http_stream_factory_impl_request_unittest.cc File net/http/http_stream_factory_impl_request_unittest.cc (right): https://codereview.chromium.org/19866006/diff/34001/net/http/http_stream_factory_impl_request_unittest.cc#newcode85 net/http/http_stream_factory_impl_request_unittest.cc:85: request.AttachJob(job); On 2013/08/14 14:46:34, mmenke wrote: > ...
7 years, 4 months ago (2013-08-14 17:48:47 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/akalin@chromium.org/19866006/50001
7 years, 4 months ago (2013-08-14 17:49:27 UTC) #9
akalin
Committed patchset #5 manually as r217582 (presubmit successful).
7 years, 4 months ago (2013-08-14 18:01:52 UTC) #10
mmenke
7 years, 4 months ago (2013-08-14 18:05:16 UTC) #11
Message was sent while issue was closed.
https://chromiumcodereview.appspot.com/19866006/diff/50001/net/http/http_stre...
File net/http/http_stream_factory_impl_request_unittest.cc (right):

https://chromiumcodereview.appspot.com/19866006/diff/50001/net/http/http_stre...
net/http/http_stream_factory_impl_request_unittest.cc:86:
EXPECT_EQ(DEFAULT_PRIORITY, job->priority());
DEFAULT_PRIORITY should have been replaced, not LOW.  We should not rely on
DEFUALT_PRIORITY not being for the test to be correct

Powered by Google App Engine
This is Rietveld 408576698