Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1554313002:
Adding changes to Remote_Desktop_BrowserTest ConnecToRemoteHost to verify if the remote host is onl… (Closed)
DescriptionAdding changes to Remote_Desktop_BrowserTest ConnecToRemoteHost to verify if the remote host is online before clicking on the control and entering pin.
BUG=
Committed: https://crrev.com/56a3da99c798b10726e231881efd851c11aa3b9b
Cr-Commit-Position: refs/heads/master@{#368410}
Patch Set 1 #
Total comments: 2
Patch Set 2 #
Total comments: 10
Patch Set 3 : Fixing the CL Comments #
Total comments: 1
Patch Set 4 : Formatting the files #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== Adding changes to Remote_Desktop_BrowserTest ConnecToRemoteHost to verify if the remote host is online before clicking on the control and entering pin. BUG= ========== to ========== Adding changes to Remote_Desktop_BrowserTest ConnecToRemoteHost to verify if the remote host is online before clicking on the control and entering pin. BUG= ==========
alog@google.com changed reviewers: + anandc@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
anandc@google.com changed reviewers: + anandc@google.com, joedow@chromium.org
lgtm, after comments addressed. +joedow@, for final approval. https://codereview.chromium.org/1554313002/diff/40001/chrome/test/remoting/re... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/1554313002/diff/40001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:588: ConditionalTimeoutWaiter hostReadyWaiter( hostOnlineWaiter? https://codereview.chromium.org/1554313002/diff/40001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:819: bool RemoteDesktopBrowserTest::IsHostOnline(std::string element_id) { It would be clearer to pass in the hostID to this function, rather than a host's elementID.
On 2016/01/06 01:29:51, anandc1 wrote: > lgtm, after comments addressed. > +joedow@, for final approval. > > https://codereview.chromium.org/1554313002/diff/40001/chrome/test/remoting/re... > File chrome/test/remoting/remote_desktop_browsertest.cc (right): > > https://codereview.chromium.org/1554313002/diff/40001/chrome/test/remoting/re... > chrome/test/remoting/remote_desktop_browsertest.cc:588: ConditionalTimeoutWaiter > hostReadyWaiter( > hostOnlineWaiter? > > https://codereview.chromium.org/1554313002/diff/40001/chrome/test/remoting/re... > chrome/test/remoting/remote_desktop_browsertest.cc:819: bool > RemoteDesktopBrowserTest::IsHostOnline(std::string element_id) { > It would be clearer to pass in the hostID to this function, rather than a host's > elementID. Also, please add to CL description the tracking bug #: https://code.google.com/p/chromium/issues/detail?id=570777.
https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:589: base::TimeDelta::FromSeconds(30), base::TimeDelta::FromMilliseconds(5000), Why base::TimeDelta::FromMilliseconds instead of using FromSeconds? If you want a larger timeout, I think it would be simpler to use FromSeconds(5) here. https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:590: base::Bind(&RemoteDesktopBrowserTest::IsHostOnline, this, host_id)); You should declare the ownership of the this pointer you are passing in, I'm assuming you want base::Unretained(this). https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:819: bool RemoteDesktopBrowserTest::IsHostOnline(std::string host_id) { host_id should be passed as a const ref. https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:825: } Add a newline here to make this more readable. Also, should this be moved into its own helper function? https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:827: // Verify the host is online. I think comment should be moved up one line so the related code lines are bunched together.
https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:589: base::TimeDelta::FromSeconds(30), base::TimeDelta::FromMilliseconds(5000), On 2016/01/06 16:33:52, joedow wrote: > Why base::TimeDelta::FromMilliseconds instead of using FromSeconds? If you want > a larger timeout, I think it would be simpler to use FromSeconds(5) here. Done. https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:590: base::Bind(&RemoteDesktopBrowserTest::IsHostOnline, this, host_id)); On 2016/01/06 16:33:52, joedow wrote: > You should declare the ownership of the this pointer you are passing in, I'm > assuming you want base::Unretained(this). Done. https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:819: bool RemoteDesktopBrowserTest::IsHostOnline(std::string host_id) { On 2016/01/06 16:33:52, joedow wrote: > host_id should be passed as a const ref. Done. https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:825: } On 2016/01/06 16:33:52, joedow wrote: > Add a newline here to make this more readable. > > Also, should this be moved into its own helper function? Done. https://codereview.chromium.org/1554313002/diff/60001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:827: // Verify the host is online. On 2016/01/06 16:33:52, joedow wrote: > I think comment should be moved up one line so the related code lines are > bunched together. Done.
lgtm https://codereview.chromium.org/1554313002/diff/80001/chrome/test/remoting/re... File chrome/test/remoting/remote_desktop_browsertest.cc (right): https://codereview.chromium.org/1554313002/diff/80001/chrome/test/remoting/re... chrome/test/remoting/remote_desktop_browsertest.cc:590: base::Bind(&RemoteDesktopBrowserTest::IsHostOnline, base::Unretained(this), host_id)); I think this line overflows now, you should fix that up before submitting.
The CQ bit was checked by alog@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from anandc@google.com, joedow@chromium.org Link to the patchset: https://codereview.chromium.org/1554313002/#ps100001 (title: "Formatting the files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554313002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554313002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alog@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554313002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554313002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by alog@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1554313002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1554313002/100001
Message was sent while issue was closed.
Description was changed from ========== Adding changes to Remote_Desktop_BrowserTest ConnecToRemoteHost to verify if the remote host is online before clicking on the control and entering pin. BUG= ========== to ========== Adding changes to Remote_Desktop_BrowserTest ConnecToRemoteHost to verify if the remote host is online before clicking on the control and entering pin. BUG= ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adding changes to Remote_Desktop_BrowserTest ConnecToRemoteHost to verify if the remote host is online before clicking on the control and entering pin. BUG= ========== to ========== Adding changes to Remote_Desktop_BrowserTest ConnecToRemoteHost to verify if the remote host is online before clicking on the control and entering pin. BUG= Committed: https://crrev.com/56a3da99c798b10726e231881efd851c11aa3b9b Cr-Commit-Position: refs/heads/master@{#368410} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/56a3da99c798b10726e231881efd851c11aa3b9b Cr-Commit-Position: refs/heads/master@{#368410} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
