Update webview-helpers to handle crosswalk webviews #238
Update webview-helpers to handle crosswalk webviews #238imurchie merged 2 commits intoappium:masterfrom
Conversation
jlipps
left a comment
jlipps
left a comment
There was a problem hiding this comment.
@blutter thanks for taking this on! it's a huge win. I made a small comment. Otherwise if I'm reading this right, it shouldn't hurt to merge this in even before chromedriver is updated, right? It just won't find anything yet? Or do we need to hold off on this until the Chromedriver update?
| let webviewPid; | ||
| let crosswalkWebviewSocket; | ||
|
|
||
| if ((webviewPid = line.match(new RegExp(WEBVIEW_REGEXP)))) { |
There was a problem hiding this comment.
this block looks duplicated; should we pull it out of the conditional so it's not?
There was a problem hiding this comment.
Sure - I originally pulled it up out of the conditional but didn't think it was as readable.
I'll have another go at refactoring.
There was a problem hiding this comment.
ok - updated with an alternative to avoid DRY
|
Thanks. It shouldn't hurt to merge this before chromedriver is updated as it is already broken for crosswalk webviews. The only risk is that the chromedriver patch gets some further changes down the track that break my changes. But given that it is already broken I would prefer that this change goes in now as it means one less thing for my team to maintain/patch. |
imurchie
left a comment
imurchie
left a comment
There was a problem hiding this comment.
Awesome!
My only comment would be that the regular expressions ought to be actual regular expressions. They are all constant (not dependent on any input from any runtime systems) so they can be real constants at the top, and not need to be made into RegExp objects right before using.
Also, can you run gulp eslint? There is an error that is causing CI to fail (https://travis-ci.org/appium/appium-android-driver/jobs/233108399#L3317).
| // same as chrome-backed webviews) | ||
| // TODO: some of this function belongs in appium-adb | ||
| async function webviewsFromProcs (adb, deviceSocket) { | ||
| const WEBVIEW_REGEXP = `@?webview_devtools_remote_(\\d+)`; |
There was a problem hiding this comment.
Why not just make this a RegExp object and skip the step in line 38. It could also go above wth the rest of the constants.
There was a problem hiding this comment.
Done.
Though IMHO I consider this less readable as the scope of the constant is only the function.
| // not some other running app | ||
| // TODO: this should be called procFromPid and exist in appium-adb | ||
| async function procFromWebview (adb, webview) { | ||
| if (webview.match(new RegExp(WEBVIEW_BASE + "(\\d+)")) === null) { |
There was a problem hiding this comment.
Use string interpolation instead of concatenating strings
`${WEBVIEW_BASE}(\\d+)`This is also constant, so could be a constant like the other regular expressions.
| if (deviceSocket) | ||
| { | ||
| if (crosswalkWebviewSocket[0].slice(1) === deviceSocket) { | ||
| webviews.push(WEBVIEW_BASE + crosswalkWebviewSocket[1]); |
There was a problem hiding this comment.
String interpolation here and line 49.
jlipps
left a comment
jlipps
left a comment
There was a problem hiding this comment.
agree with isaac's subsequent feedback, then we should be good to go
blutter
force-pushed
the
fix-crosswalk-webviews
branch
from
e8fc425 to
6881325
Compare
|
I've also fixed the eslint issue. |
* Update webview-helpers to handle crosswalk webviews * Refactor with code review feedback
3 participants
There has been a long running issue with finding crosswalk webviews on Android as logged in appium/appium#4597
There is a two part fix:
I have updated the tests to cover this scenario and implemented the changes to the webview-helper code.