Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2267643002:
Adjust callers and networking delegates in remoting/ to modified APIs (Closed)
|
Created:
4 years, 4 months ago by maksims (do not use this acc) Modified:
4 years, 3 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org, mmenke Base URL:
https://chromium.googlesource.com/chromium/src.git@URLRequestRead Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdjust callers and networking delegates in remoting/ to modified APIs
Use modified Read and delegate methods from the following
CL- https://codereview.chromium.org/2262653003/
BUG=423484
Committed: https://crrev.com/c023fa24de14f77906b984a7bf1d3e4238539f09
Cr-Commit-Position: refs/heads/master@{#420262}
Patch Set 1 #Patch Set 2 : rebased and improved code #
Total comments: 8
Patch Set 3 : comments #
Total comments: 3
Patch Set 4 : git cl format #Messages
Total messages: 45 (32 generated)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Adjust callers and networking delegates in remoting/ to modified APIs BUG= ========== to ========== Adjust callers and networking delegates in remoting/ to modified APIs BUG= ==========
maksim.sisov@intel.com changed reviewers: + kelvinp@chromium.org, sergeyu@chromium.org
Description was changed from ========== Adjust callers and networking delegates in remoting/ to modified APIs BUG= ========== to ========== Adjust callers and networking delegates in remoting/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 ==========
would someone of you take a look into this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
CC mmenke@
maksim.sisov@intel.com changed reviewers: + garykac@chromium.org
gentle reminder to review this + garykac@
https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... File remoting/host/token_validator_base.cc (right): https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... remoting/host/token_validator_base.cc:89: int net_error) { nit: maybe call this parameter net_result - it's not always an error. https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... remoting/host/token_validator_base.cc:90: DCHECK_NE(net::ERR_IO_PENDING, net_error); nit: please swap these two parameters to make code more readable: DCHECK_NE(net_error, net::ERR_IO_PENDING); https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... remoting/host/token_validator_base.cc:93: int bytes_read = request_->Read(buffer_.get(), kBufferSize); Should we even try reading the response when net_error != OK? https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... remoting/host/token_validator_base.cc:98: int bytes_read) { rename this parameter to result or net_result and then you won't need net_error below.
On 2016/09/06 12:19:20, maksims wrote: > gentle reminder to review this + garykac@ You sent this CL over the weekend and Monday was a holiday in US.
On 2016/09/06 17:46:07, Sergey Ulanov wrote: > On 2016/09/06 12:19:20, maksims wrote: > > gentle reminder to review this + garykac@ > > You sent this CL over the weekend and Monday was a holiday in US. Sorry. I might wanted to have too quick answer from you. I've been busy as well and couldn't go through the comments you left. Sorry again.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... File remoting/host/token_validator_base.cc (right): https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... remoting/host/token_validator_base.cc:90: DCHECK_NE(net::ERR_IO_PENDING, net_error); On 2016/09/06 17:43:23, Sergey Ulanov wrote: > nit: please swap these two parameters to make code more readable: > DCHECK_NE(net_error, net::ERR_IO_PENDING); Well, I've always thought it should have been other way round as long as many people complained that it should have been DCHECK_NE(net::ERR, net_error); But as you wish! https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... remoting/host/token_validator_base.cc:93: int bytes_read = request_->Read(buffer_.get(), kBufferSize); On 2016/09/06 17:43:23, Sergey Ulanov wrote: > Should we even try reading the response when net_error != OK? Done. https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... remoting/host/token_validator_base.cc:98: int bytes_read) { On 2016/09/06 17:43:23, Sergey Ulanov wrote: > rename this parameter to result or net_result and then you won't need net_error > below. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Patchset #4 (id:80001) has been deleted
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... File remoting/host/token_validator_base.cc (right): https://codereview.chromium.org/2267643002/diff/20001/remoting/host/token_val... remoting/host/token_validator_base.cc:90: DCHECK_NE(net::ERR_IO_PENDING, net_error); On 2016/09/21 05:17:07, maksims wrote: > On 2016/09/06 17:43:23, Sergey Ulanov wrote: > > nit: please swap these two parameters to make code more readable: > > DCHECK_NE(net_error, net::ERR_IO_PENDING); > > Well, I've always thought it should have been other way round as long as many > people complained that it should have been DCHECK_NE(net::ERR, net_error); It matters for EXPECT_EQ/EXPECT_NE from gtest, where the first argument is the expected value. This is not applicable here. > > But as you wish! https://codereview.chromium.org/2267643002/diff/100001/remoting/host/token_va... File remoting/host/token_validator_base.cc (right): https://codereview.chromium.org/2267643002/diff/100001/remoting/host/token_va... remoting/host/token_validator_base.cc:199: LOG(ERROR) << "Error validating token, err=" << net_result; incorrect indentation https://codereview.chromium.org/2267643002/diff/100001/remoting/host/token_va... remoting/host/token_validator_base.cc:205: LOG(ERROR) << "Error " << response << " validating token: '" << data_ << "'"; incorrect indentation. Revert this change and the changes below?
https://codereview.chromium.org/2267643002/diff/100001/remoting/host/token_va... File remoting/host/token_validator_base.cc (right): https://codereview.chromium.org/2267643002/diff/100001/remoting/host/token_va... remoting/host/token_validator_base.cc:205: LOG(ERROR) << "Error " << response << " validating token: '" << data_ << "'"; On 2016/09/21 07:13:51, Sergey Ulanov wrote: > incorrect indentation. Revert this change and the changes below? Oops, is "git cl format" OK to you?
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by maksim.sisov@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adjust callers and networking delegates in remoting/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 ========== to ========== Adjust callers and networking delegates in remoting/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Adjust callers and networking delegates in remoting/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 ========== to ========== Adjust callers and networking delegates in remoting/ to modified APIs Use modified Read and delegate methods from the following CL- https://codereview.chromium.org/2262653003/ BUG=423484 Committed: https://crrev.com/c023fa24de14f77906b984a7bf1d3e4238539f09 Cr-Commit-Position: refs/heads/master@{#420262} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c023fa24de14f77906b984a7bf1d3e4238539f09 Cr-Commit-Position: refs/heads/master@{#420262} |
