Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2375613002:
Fix Chromedriver Issue 749
|
Created:
4 years, 2 months ago by Rahul Kavalapara Modified:
3 years, 5 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBug Fix for chromedriver issue 749
For android devices, we are currently looking for the default devtools socket which is "webview_devtools_remote_pid". Based on chrome browser implementation, we are currently looking for the packages devtools_remote socket instead of the default socket.
BUG=chromedriver:749
Patch Set 1 #Patch Set 2 : Fix for chromedriver issue 749 #
Total comments: 26
Patch Set 3 : Fix for chromedriver issue 749 #Patch Set 4 : Bug Fix for chromedriver issue 749 #
Total comments: 3
Patch Set 5 : Bug Fix for chromedriver issue 749 #
Total comments: 1
Messages
Total messages: 40 (13 generated)
rkavalap@gmail.com changed reviewers: + gmanikpure@chromium.org, paulirish@chromium.org, samuong@chromium.org, stgao@chromium.org
Hello I'd like you to review my code
The CQ bit was checked by rkavalap@gmail.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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by rkavalap@gmail.com
The CQ bit was checked by rkavalap@gmail.com
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Thanks Rahul. I'm currently out of the office so I won't get a chance to look at your code until later this week. Will get back to you then, thanks for your patience.
Description was changed from ========== Fix presubmit messages Fix for chromedriver issue 749 We should be looking for packages devtools_remote socket instead of the the default "webview_devtools_remote_pid" BUG=749 ========== to ========== Fix presubmit messages Fix for chromedriver issue 749 We should be looking for packages devtools_remote socket instead of the the default "webview_devtools_remote_pid" BUG=749 ==========
stgao@chromium.org changed reviewers: - stgao@chromium.org
https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb.h (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb.h:38: std::string* device_socket) = 0; indent lines 37-38 so that they line up with the first parameter on line 36 https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb.h:42: don't add a blank line here https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:243: Status status = ExecuteHostShellCommand(device_serial, "cat /proc/net/unix", &response); this line is too long (>80 chars) https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:254: if (fields[3] != "00010000" || fields[5] != "01") it's not really clear to me what these magic numbers/flags mean - can you link to somewhere that documents the format of this response? https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:259: if (path_field.find(process_name) == std::string::npos) what if there are two processes, where one of the process names is a substring of the other? https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:268: "Failed to find the remote devtools socket for the following process: " + process_name); this line is too long (>80 chars) https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.h (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.h:55: don't add a blank line here https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/device_manager.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/device_manager.cc:125: device_socket); reindent so that all arguments are aligned https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/device_manager.cc:132: // *device_socket = base::StringPrintf("webview_devtools_remote_%d", pid); don't uncomment, just delete this line https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/device_manager_unittest.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/device_manager_unittest.cc:70: std::string* device_socket) override { reindent so that function parameters are aligned
Samuong. Thank you for your review. I will send another patch for indentations. Regarding your other comments: You can see the implementation in MapSocketsToProcesses method. https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:254: if (fields[3] != "00010000" || fields[5] != "01") On 2016/10/03 18:50:40, samuong wrote: > it's not really clear to me what these magic numbers/flags mean - can you link > to somewhere that documents the format of this response? Please check MapSocketsToProcesses function. https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:259: if (path_field.find(process_name) == std::string::npos) On 2016/10/03 18:50:40, samuong wrote: > what if there are two processes, where one of the process names is a substring > of the other? This is the adaptation from devtools code. That would be an interesting scenario.
https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:254: if (fields[3] != "00010000" || fields[5] != "01") On 2016/10/03 22:15:52, Rahul Kavalapara wrote: > On 2016/10/03 18:50:40, samuong wrote: > > it's not really clear to me what these magic numbers/flags mean - can you link > > to somewhere that documents the format of this response? > > Please check MapSocketsToProcesses function. > https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... I did some more digging, Similar question was posted here: http://unix.stackexchange.com/questions/183140/what-is-the-meaning-of-the-con... If you want me to document the answer as well please let me know. Answer: The code that generates this file is in the unix_seq_show() function in net/unix/af_unix.c in the kernel source. Looking at include/net/af_unix.h is also helpful, to see the data structures in use. The socket path is always the last column in the output, and the Android kernel source matches the stock kernel in this respect. So unless I'm mistaken, that number that looks like a column isn't actually a separate column. You can name UNIX domain sockets practically anything you want, as long as the total path length is less than 108 bytes. So you can't make any assumptions as to what these paths will look like. It's possible that the userspace code that's choosing those names is using a tab character followed by a number, or even padding the name out to a certain length with spaces. To test my theory, you might try looking at the socket files in /dev/socket/qmux_radio/.
https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb.h (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb.h:38: std::string* device_socket) = 0; On 2016/10/03 18:50:40, samuong wrote: > indent lines 37-38 so that they line up with the first parameter on line 36 Done. https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb.h:42: On 2016/10/03 18:50:40, samuong wrote: > don't add a blank line here Done. https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:243: Status status = ExecuteHostShellCommand(device_serial, "cat /proc/net/unix", &response); On 2016/10/03 18:50:40, samuong wrote: > this line is too long (>80 chars) Done. https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:268: "Failed to find the remote devtools socket for the following process: " + process_name); On 2016/10/03 18:50:40, samuong wrote: > this line is too long (>80 chars) Done. https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.h (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.h:55: On 2016/10/03 18:50:40, samuong wrote: > don't add a blank line here Done. https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/device_manager.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/device_manager.cc:125: device_socket); On 2016/10/03 18:50:40, samuong wrote: > reindent so that all arguments are aligned Done. https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/device_manager.cc:132: // *device_socket = base::StringPrintf("webview_devtools_remote_%d", pid); On 2016/10/03 18:50:40, samuong wrote: > don't uncomment, just delete this line Done.
https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:254: if (fields[3] != "00010000" || fields[5] != "01") On 2016/10/04 01:18:04, Rahul Kavalapara wrote: > On 2016/10/03 22:15:52, Rahul Kavalapara wrote: > > On 2016/10/03 18:50:40, samuong wrote: > > > it's not really clear to me what these magic numbers/flags mean - can you > link > > > to somewhere that documents the format of this response? > > > > Please check MapSocketsToProcesses function. > > > https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... > > I did some more digging, Similar question was posted here: > http://unix.stackexchange.com/questions/183140/what-is-the-meaning-of-the-con... > If you want me to document the answer as well please let me know. > > Answer: > > The code that generates this file is in the unix_seq_show() function in > net/unix/af_unix.c in the kernel source. Looking at include/net/af_unix.h is > also helpful, to see the data structures in use. > > The socket path is always the last column in the output, and the Android kernel > source matches the stock kernel in this respect. So unless I'm mistaken, that > number that looks like a column isn't actually a separate column. > > You can name UNIX domain sockets practically anything you want, as long as the > total path length is less than 108 bytes. So you can't make any assumptions as > to what these paths will look like. It's possible that the userspace code that's > choosing those names is using a tab character followed by a number, or even > padding the name out to a certain length with spaces. To test my theory, you > might try looking at the socket files in /dev/socket/qmux_radio/. > Thanks for digging that up. I was mostly curious about what "flags"="00010000" and "st"="01" mean, and why we only care about entries with these values. It looks like "flags" is used to determine whether the socket is accepting connections, and "st(ate)" is used to determine the unconnected/disconnecting state. This is based on what I'm reading in the kernel sources: https://github.com/torvalds/linux/blob/master/net/unix/af_unix.c#L2795 Can you please document this in the comments? Also, if the socket name can have whitespace in it, that sounds like something that needs handling. It's possible that the DevTools code only cares about devtools sockets, which they can control the naming of. But is the idea here that we're going to accept arbitrarily named sockets? https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:259: if (path_field.find(process_name) == std::string::npos) On 2016/10/03 22:15:52, Rahul Kavalapara wrote: > On 2016/10/03 18:50:40, samuong wrote: > > what if there are two processes, where one of the process names is a substring > > of the other? > > This is the adaptation from devtools code. That would be an interesting > scenario. I think we should know the full socket name that we're looking for (correct me if I'm wrong) so it should be possible to do a full string comparison rather than looking for a substring?
https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:254: if (fields[3] != "00010000" || fields[5] != "01") On 2016/10/05 17:29:49, samuong wrote: > On 2016/10/04 01:18:04, Rahul Kavalapara wrote: > > On 2016/10/03 22:15:52, Rahul Kavalapara wrote: > > > On 2016/10/03 18:50:40, samuong wrote: > > > > it's not really clear to me what these magic numbers/flags mean - can you > > link > > > > to somewhere that documents the format of this response? > > > > > > Please check MapSocketsToProcesses function. > > > > > > https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... > > > > I did some more digging, Similar question was posted here: > > > http://unix.stackexchange.com/questions/183140/what-is-the-meaning-of-the-con... > > If you want me to document the answer as well please let me know. > > > > Answer: > > > > The code that generates this file is in the unix_seq_show() function in > > net/unix/af_unix.c in the kernel source. Looking at include/net/af_unix.h is > > also helpful, to see the data structures in use. > > > > The socket path is always the last column in the output, and the Android > kernel > > source matches the stock kernel in this respect. So unless I'm mistaken, that > > number that looks like a column isn't actually a separate column. > > > > You can name UNIX domain sockets practically anything you want, as long as the > > total path length is less than 108 bytes. So you can't make any assumptions as > > to what these paths will look like. It's possible that the userspace code > that's > > choosing those names is using a tab character followed by a number, or even > > padding the name out to a certain length with spaces. To test my theory, you > > might try looking at the socket files in /dev/socket/qmux_radio/. > > > > Thanks for digging that up. I was mostly curious about what "flags"="00010000" > and "st"="01" mean, and why we only care about entries with these values. It > looks like "flags" is used to determine whether the socket is accepting > connections, and "st(ate)" is used to determine the unconnected/disconnecting > state. This is based on what I'm reading in the kernel sources: > https://github.com/torvalds/linux/blob/master/net/unix/af_unix.c#L2795 > > Can you please document this in the comments? > > Also, if the socket name can have whitespace in it, that sounds like something > that needs handling. It's possible that the DevTools code only cares about > devtools sockets, which they can control the naming of. But is the idea here > that we're going to accept arbitrarily named sockets? This code is essentially exercised targeting android devices. I believe there is a naming convention for the apps like "com.<companyname>.appname". So in this scenario I am expecting devtools_socket name to be something like "com.abc.myapp_devtools_remote". I am highly skeptical on the whitespace issue if that would happen. We are just getting the string prior to devtools_remote as that will passed as a chromedriver capabilities value by the end user. I have documented my findings in comments as well. https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:259: if (path_field.find(process_name) == std::string::npos) On 2016/10/05 17:29:49, samuong wrote: > On 2016/10/03 22:15:52, Rahul Kavalapara wrote: > > On 2016/10/03 18:50:40, samuong wrote: > > > what if there are two processes, where one of the process names is a > substring > > > of the other? > > > > This is the adaptation from devtools code. That would be an interesting > > scenario. > > I think we should know the full socket name that we're looking for (correct me > if I'm wrong) so it should be possible to do a full string comparison rather > than looking for a substring? I am sending back the entire name when trying to connect to the socket. I would not know the pid of this socket ahead of time if there was one, but just the app name. So i am comparing only the appname detected from the socket with the what was passed from the capabilities by the end user.
Samuong, I would like to commit this change if you don't have any more comments. Thanks, Rahul
Description was changed from ========== Fix presubmit messages Fix for chromedriver issue 749 We should be looking for packages devtools_remote socket instead of the the default "webview_devtools_remote_pid" BUG=749 ========== to ========== Fix presubmit messages Fix for chromedriver issue 749 We should be looking for packages devtools_remote socket instead of the the default "webview_devtools_remote_pid" BUG=chromedriver:749 ==========
Can you make sure that the "description" field in "edit issue" is written according to https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:254: if (fields[3] != "00010000" || fields[5] != "01") On 2016/10/05 18:41:53, Rahul Kavalapara wrote: > On 2016/10/05 17:29:49, samuong wrote: > > On 2016/10/04 01:18:04, Rahul Kavalapara wrote: > > > On 2016/10/03 22:15:52, Rahul Kavalapara wrote: > > > > On 2016/10/03 18:50:40, samuong wrote: > > > > > it's not really clear to me what these magic numbers/flags mean - can > you > > > link > > > > > to somewhere that documents the format of this response? > > > > > > > > Please check MapSocketsToProcesses function. > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... > > > > > > I did some more digging, Similar question was posted here: > > > > > > http://unix.stackexchange.com/questions/183140/what-is-the-meaning-of-the-con... > > > If you want me to document the answer as well please let me know. > > > > > > Answer: > > > > > > The code that generates this file is in the unix_seq_show() function in > > > net/unix/af_unix.c in the kernel source. Looking at include/net/af_unix.h is > > > also helpful, to see the data structures in use. > > > > > > The socket path is always the last column in the output, and the Android > > kernel > > > source matches the stock kernel in this respect. So unless I'm mistaken, > that > > > number that looks like a column isn't actually a separate column. > > > > > > You can name UNIX domain sockets practically anything you want, as long as > the > > > total path length is less than 108 bytes. So you can't make any assumptions > as > > > to what these paths will look like. It's possible that the userspace code > > that's > > > choosing those names is using a tab character followed by a number, or even > > > padding the name out to a certain length with spaces. To test my theory, you > > > might try looking at the socket files in /dev/socket/qmux_radio/. > > > > > > > Thanks for digging that up. I was mostly curious about what "flags"="00010000" > > and "st"="01" mean, and why we only care about entries with these values. It > > looks like "flags" is used to determine whether the socket is accepting > > connections, and "st(ate)" is used to determine the unconnected/disconnecting > > state. This is based on what I'm reading in the kernel sources: > > https://github.com/torvalds/linux/blob/master/net/unix/af_unix.c#L2795 > > > > Can you please document this in the comments? > > > > Also, if the socket name can have whitespace in it, that sounds like something > > that needs handling. It's possible that the DevTools code only cares about > > devtools sockets, which they can control the naming of. But is the idea here > > that we're going to accept arbitrarily named sockets? > > This code is essentially exercised targeting android devices. I believe there is > a naming convention for the apps like "com.<companyname>.appname". So in this > scenario I am expecting devtools_socket name to be something like > "com.abc.myapp_devtools_remote". I am highly skeptical on the whitespace issue > if that would happen. We are just getting the string prior to devtools_remote as > that will passed as a chromedriver capabilities value by the end user. > > I have documented my findings in comments as well. > > OK, that makes sense. However I'd still like to make sure there is a comment explaining specifically what 'fields[3] != "00010000"' and 'fields[5] != "01"' mean. Just a one-liner is fine.
Description was changed from ========== Fix presubmit messages Fix for chromedriver issue 749 We should be looking for packages devtools_remote socket instead of the the default "webview_devtools_remote_pid" BUG=chromedriver:749 ========== to ========== Bug Fix for chromedriver issue 749 For android devices, we are currently looking for the default devtools socket which is "webview_devtools_remote_pid". Based on chrome browser implementation, we are currently looking for the packages devtools_remote socket instead of the default socket. BUG=chromedriver:749 ==========
https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:254: if (fields[3] != "00010000" || fields[5] != "01") On 2016/10/12 05:18:35, samuong wrote: > On 2016/10/05 18:41:53, Rahul Kavalapara wrote: > > On 2016/10/05 17:29:49, samuong wrote: > > > On 2016/10/04 01:18:04, Rahul Kavalapara wrote: > > > > On 2016/10/03 22:15:52, Rahul Kavalapara wrote: > > > > > On 2016/10/03 18:50:40, samuong wrote: > > > > > > it's not really clear to me what these magic numbers/flags mean - can > > you > > > > link > > > > > > to somewhere that documents the format of this response? > > > > > > > > > > Please check MapSocketsToProcesses function. > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... > > > > > > > > I did some more digging, Similar question was posted here: > > > > > > > > > > http://unix.stackexchange.com/questions/183140/what-is-the-meaning-of-the-con... > > > > If you want me to document the answer as well please let me know. > > > > > > > > Answer: > > > > > > > > The code that generates this file is in the unix_seq_show() function in > > > > net/unix/af_unix.c in the kernel source. Looking at include/net/af_unix.h > is > > > > also helpful, to see the data structures in use. > > > > > > > > The socket path is always the last column in the output, and the Android > > > kernel > > > > source matches the stock kernel in this respect. So unless I'm mistaken, > > that > > > > number that looks like a column isn't actually a separate column. > > > > > > > > You can name UNIX domain sockets practically anything you want, as long as > > the > > > > total path length is less than 108 bytes. So you can't make any > assumptions > > as > > > > to what these paths will look like. It's possible that the userspace code > > > that's > > > > choosing those names is using a tab character followed by a number, or > even > > > > padding the name out to a certain length with spaces. To test my theory, > you > > > > might try looking at the socket files in /dev/socket/qmux_radio/. > > > > > > > > > > Thanks for digging that up. I was mostly curious about what > "flags"="00010000" > > > and "st"="01" mean, and why we only care about entries with these values. It > > > looks like "flags" is used to determine whether the socket is accepting > > > connections, and "st(ate)" is used to determine the > unconnected/disconnecting > > > state. This is based on what I'm reading in the kernel sources: > > > https://github.com/torvalds/linux/blob/master/net/unix/af_unix.c#L2795 > > > > > > Can you please document this in the comments? > > > > > > Also, if the socket name can have whitespace in it, that sounds like > something > > > that needs handling. It's possible that the DevTools code only cares about > > > devtools sockets, which they can control the naming of. But is the idea here > > > that we're going to accept arbitrarily named sockets? > > > > This code is essentially exercised targeting android devices. I believe there > is > > a naming convention for the apps like "com.<companyname>.appname". So in this > > scenario I am expecting devtools_socket name to be something like > > "com.abc.myapp_devtools_remote". I am highly skeptical on the whitespace issue > > if that would happen. We are just getting the string prior to devtools_remote > as > > that will passed as a chromedriver capabilities value by the end user. > > > > I have documented my findings in comments as well. > > > > > > OK, that makes sense. However I'd still like to make sure there is a comment > explaining specifically what 'fields[3] != "00010000"' and 'fields[5] != "01"' > mean. Just a one-liner is fine. Fixed.
Ping !
Please let me know if anything else is needed here.
https://codereview.chromium.org/2375613002/diff/60001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/60001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:248: nit: no need for a blank line here https://codereview.chromium.org/2375613002/diff/60001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:263: nit: no need for a blank line here https://codereview.chromium.org/2375613002/diff/60001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:265: // fields[5] (State) != TCP_ESTABLISHED (inlclude/net/tcp_states.h) Sorry, I still feel this is too cryptic to developers who might be reading this in the future. Can you write these as full sentences, and just mention that we're looking for sockets that are accepting connections and currently unconnected?
Samuong, Can this be merged ? -- Rahul
On 2016/11/14 18:21:52, Rahul Kavalapara wrote: > Samuong, > > Can this be merged ? > > -- > Rahul Hi Rahul, Sam is out of office. He will get back to you once he is back, thanks for your patience!
On 2016/11/14 18:23:53, gmanikpure wrote: > On 2016/11/14 18:21:52, Rahul Kavalapara wrote: > > Samuong, > > > > Can this be merged ? > > > > -- > > Rahul > > Hi Rahul, > > Sam is out of office. He will get back to you once he is back, thanks for your > patience! Hi there. Is there an ETA on getting this merged? A lot of users are waiting for this as they are blocked until this gets merged (anyone using appium/crosswalk). Sincerely, Danqa
On 2016/10/20 04:40:40, samuong wrote: > https://codereview.chromium.org/2375613002/diff/60001/chrome/test/chromedrive... > File chrome/test/chromedriver/chrome/adb_impl.cc (right): > > https://codereview.chromium.org/2375613002/diff/60001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome/adb_impl.cc:248: > nit: no need for a blank line here > > https://codereview.chromium.org/2375613002/diff/60001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome/adb_impl.cc:263: > nit: no need for a blank line here > > https://codereview.chromium.org/2375613002/diff/60001/chrome/test/chromedrive... > chrome/test/chromedriver/chrome/adb_impl.cc:265: // fields[5] (State) != > TCP_ESTABLISHED (inlclude/net/tcp_states.h) > Sorry, I still feel this is too cryptic to developers who might be reading this > in the future. Can you write these as full sentences, and just mention that > we're looking for sockets that are accepting connections and currently > unconnected? Hi samoung, Is there a ETA for this to be merged to the chromedriver?
Just to echo Danqa's comments above, many users are waiting for this as it's causing severe issues for automation with the combination of appium and crosswalk apps. Could this be merged into chromedriver soon, please?
johnchen@chromium.org changed reviewers: + johnchen@chromium.org - samuong@chromium.org
My apology for taking so long to respond to this review. Sam no longer works on the ChromeDriver, and we're now going over issues that were assigned to him. It appears that this change will break WebView scenario. Please see the comments below. Also, the crosswalk project no longer appears to be active (https://crosswalk-project.org/blog/crosswalk-final-release.html), so is there still interest in making this change? https://codereview.chromium.org/2375613002/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/adb_impl.cc (right): https://codereview.chromium.org/2375613002/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/adb_impl.cc:229: Status AdbImpl::GetDevtoolsRemoteSocket(const std::string& device_serial, This doesn't work correctly for WebView apps. The sockets for WebView apps are named webview_devtools_remote_<pid>, regardless of the package name or process name, so this function won't be able to find their sockets.
John, It's been a while I have sent this PR. Bunch of the code is based on the chrome browser's behavior. We should have chromedriver behave the same way as chrome. https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... When you say it breaks the Webview scenario, can you elaborate ? -- Rahul On Wed, Jul 12, 2017 at 2:54 PM, <johnchen@chromium.org> wrote: > My apology for taking so long to respond to this review. Sam no longer > works on > the ChromeDriver, and we're now going over issues that were assigned to > him. > > It appears that this change will break WebView scenario. Please see the > comments > below. Also, the crosswalk project no longer appears to be active > (https://crosswalk-project.org/blog/crosswalk-final-release.html), so is > there > still interest in making this change? > > > https://codereview.chromium.org/2375613002/diff/80001/ > chrome/test/chromedriver/chrome/adb_impl.cc > File chrome/test/chromedriver/chrome/adb_impl.cc (right): > > https://codereview.chromium.org/2375613002/diff/80001/ > chrome/test/chromedriver/chrome/adb_impl.cc#newcode229 > chrome/test/chromedriver/chrome/adb_impl.cc:229: Status > AdbImpl::GetDevtoolsRemoteSocket(const std::string& device_serial, > This doesn't work correctly for WebView apps. The sockets for WebView > apps are named webview_devtools_remote_<pid>, regardless of the package > name or process name, so this function won't be able to find their > sockets. > > https://codereview.chromium.org/2375613002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/07/13 05:20:28, Rahul Kavalapara wrote: > It's been a while I have sent this PR. Bunch of the code is based on the > chrome browser's behavior. We should have chromedriver behave the same way > as chrome. > https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/devtoo... > When you say it breaks the Webview scenario, can you elaborate ? Rahul, On Android devices, in addition to the full Chrome browser, Chrome can also be used as a component that can be embedded in other apps. This component is called WebView[1]. ChromeDriver is designed to support any applications that use WebView, in addition to the Chrome browser. The existing code near the beginning of Device::ForwardDevtoolsPort was designed for WebView apps. As a test app for WebView, you can try Android WebView shell[2]. Thanks, John [1] https://developer.android.com/reference/android/webkit/WebView.html [2] https://www.chromium.org/developers/testing/android-tests/android-webview-tests
I will take a look at this later today, Is there any specific command for the android webview tests to execute which consume chromedriver ? On Thu, Jul 13, 2017 at 8:03 AM, <johnchen@chromium.org> wrote: > On 2017/07/13 05:20:28, Rahul Kavalapara wrote: > > > It's been a while I have sent this PR. Bunch of the code is based on the > > chrome browser's behavior. We should have chromedriver behave the same > way > > as chrome. > > > https://chromium.googlesource.com/chromium/src/+/master/ > chrome/browser/devtools/device/android_device_info_query.cc > > When you say it breaks the Webview scenario, can you elaborate ? > > Rahul, > > On Android devices, in addition to the full Chrome browser, Chrome can > also be > used as a component that can be embedded in other apps. This component is > called > WebView[1]. ChromeDriver is designed to support any applications that use > WebView, in addition to the Chrome browser. The existing code near the > beginning > of Device::ForwardDevtoolsPort was designed for WebView apps. As a test > app for > WebView, you can try Android WebView shell[2]. > > Thanks, > John > > [1] https://developer.android.com/reference/android/webkit/WebView.html > [2] > https://www.chromium.org/developers/testing/android- > tests/android-webview-tests > > https://codereview.chromium.org/2375613002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Here is one way to make sure that ChromeDriver works correctly with WebView: 1. Create a Chromium build configuration with target_os="android" (unless you have one already). 2. Under that configuration, use ninja to build target chromedriver_webview_shell_apk. 3. Install resulting ChromeDriverWebViewShell.apk onto the device. 4. Run the following command: chrome/test/chromedriver/test/run_py_tests.py --chromedriver=/path/to/chromedriver --android-package=chromedriver_webview_shell |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
