Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2571143003:
arc: enable use_new_wrapper_types for net.mojom (Closed)
|
Created:
4 years ago by abhishekbh Modified:
4 years ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, cbentzel+watch_chromium.org, yusukes+watch_chromium.org, abhishekbh_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, Kevin Cernekee, Sameer Nanda Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: enable use_new_wrapper_types for net.mojom
BUG=665983
TEST=try
Committed: https://crrev.com/cc2b33c4b4af32e831b227f81ccd0be0a04c056d
Cr-Commit-Position: refs/heads/master@{#438711}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed has_value usage #
Total comments: 3
Messages
Total messages: 28 (18 generated)
Description was changed from ========== arc: enable use_new_wrapper_types for net.mojom BUG=665983 TEST=try ========== to ========== arc: enable use_new_wrapper_types for net.mojom BUG=665983 TEST=try ==========
abhishekbh@google.com changed reviewers: + hidehiko@chromium.org, lhchavez@chromium.org, yusukes@chromium.org
abhishekbh@google.com changed required reviewers: + yusukes@chromium.org
The CQ bit was checked by abhishekbh@google.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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for working on this! https://codereview.chromium.org/2571143003/diff/1/components/arc/BUILD.gn File components/arc/BUILD.gn (left): https://codereview.chromium.org/2571143003/diff/1/components/arc/BUILD.gn#old... components/arc/BUILD.gn:130: "//ui/gfx/geometry/mojo", You probably have to move this line to arc_bindings. https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:455: if (cfg->hexssid.has_value() || !cfg->details) { Did you mean: if (!cfg->hexssid.has_value() || !...) { ? https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:478: if (!details->passphrase.has_value()) { same. Remove '!'? if (details->passphrase.has_value()) { ?
On 2016/12/14 05:19:08, Yusuke Sato (ooo Dec 16 to 31) wrote: > Thanks for working on this! > > https://codereview.chromium.org/2571143003/diff/1/components/arc/BUILD.gn > File components/arc/BUILD.gn (left): > > https://codereview.chromium.org/2571143003/diff/1/components/arc/BUILD.gn#old... > components/arc/BUILD.gn:130: "//ui/gfx/geometry/mojo", > You probably have to move this line to arc_bindings. > > https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... > File components/arc/net/arc_net_host_impl.cc (right): > > https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.cc:455: if (cfg->hexssid.has_value() || > !cfg->details) { > Did you mean: if (!cfg->hexssid.has_value() || !...) { ? > > https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... > components/arc/net/arc_net_host_impl.cc:478: if > (!details->passphrase.has_value()) { > same. Remove '!'? > > if (details->passphrase.has_value()) { ? PTAL
The CQ bit was checked by abhishekbh@google.com
The CQ bit was unchecked by abhishekbh@google.com
The CQ bit was checked by abhishekbh@google.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...
abhishekbh@google.com changed reviewers: + cernekee@chromium.org
lgtm
https://codereview.chromium.org/2571143003/diff/20001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/2571143003/diff/20001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:104: std::vector<std::string> strings; nit: maybe strings.reserve(list->GetSize())? Same in all other cases.
https://codereview.chromium.org/2571143003/diff/1/components/arc/BUILD.gn File components/arc/BUILD.gn (left): https://codereview.chromium.org/2571143003/diff/1/components/arc/BUILD.gn#old... components/arc/BUILD.gn:130: "//ui/gfx/geometry/mojo", On 2016/12/14 05:19:08, Yusuke Sato (ooo Dec 16 to 31) wrote: > You probably have to move this line to arc_bindings. Done. https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:455: if (cfg->hexssid.has_value() || !cfg->details) { On 2016/12/14 05:19:08, Yusuke Sato (ooo Dec 16 to 31) wrote: > Did you mean: if (!cfg->hexssid.has_value() || !...) { ? Oops I didn't realize the logical meaning gets inverted. Good catch !! https://codereview.chromium.org/2571143003/diff/1/components/arc/net/arc_net_... components/arc/net/arc_net_host_impl.cc:478: if (!details->passphrase.has_value()) { On 2016/12/14 05:19:08, Yusuke Sato (ooo Dec 16 to 31) wrote: > same. Remove '!'? > > if (details->passphrase.has_value()) { ? Ditto. https://codereview.chromium.org/2571143003/diff/20001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/2571143003/diff/20001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:104: std::vector<std::string> strings; On 2016/12/14 22:55:35, Luis Héctor Chávez wrote: > nit: maybe strings.reserve(list->GetSize())? Same in all other cases. I'd rather keep this CL simple. Also, the first push should allocate a big enough array for most cases. Maybe we can address this in a separate CL. I do agree with going for performant code wherever possible.
lgtm https://codereview.chromium.org/2571143003/diff/20001/components/arc/net/arc_... File components/arc/net/arc_net_host_impl.cc (right): https://codereview.chromium.org/2571143003/diff/20001/components/arc/net/arc_... components/arc/net/arc_net_host_impl.cc:104: std::vector<std::string> strings; On 2016/12/14 23:07:47, abhishekbh wrote: > On 2016/12/14 22:55:35, Luis Héctor Chávez wrote: > > nit: maybe strings.reserve(list->GetSize())? Same in all other cases. > > I'd rather keep this CL simple. Also, the first push should allocate a big > enough array for most cases. Maybe we can address this in a separate CL. I do > agree with going for performant code wherever possible. separate CL is also fine by me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm (assuming that the network functions are still working.)
The CQ bit was checked by abhishekbh@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481763732257290,
"parent_rev": "c6c1373dadfe3a12d678dc7cf8ee25710478d616", "commit_rev":
"8e1a9b8aa137c03e82a286dfd3da458a855e0431"}
Message was sent while issue was closed.
Description was changed from ========== arc: enable use_new_wrapper_types for net.mojom BUG=665983 TEST=try ========== to ========== arc: enable use_new_wrapper_types for net.mojom BUG=665983 TEST=try Review-Url: https://codereview.chromium.org/2571143003 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== arc: enable use_new_wrapper_types for net.mojom BUG=665983 TEST=try Review-Url: https://codereview.chromium.org/2571143003 ========== to ========== arc: enable use_new_wrapper_types for net.mojom BUG=665983 TEST=try Committed: https://crrev.com/cc2b33c4b4af32e831b227f81ccd0be0a04c056d Cr-Commit-Position: refs/heads/master@{#438711} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cc2b33c4b4af32e831b227f81ccd0be0a04c056d Cr-Commit-Position: refs/heads/master@{#438711} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
