Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503533003:
[chromedriver] Retry alert handling if HandleJavaScriptDialog fails. (Closed)
|
Created:
4 years, 1 month ago by gmanikpure Modified:
4 years, 1 month ago Reviewers:
samuong CC:
chromium-reviews, samuong+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[chromedriver] Retry alert handling if HandleJavaScriptDialog fails.
BUG=chromedriver:1500
Committed: https://crrev.com/2bc717ca99f16e07f704058c6f3faea8cfa647e9
Cr-Commit-Position: refs/heads/master@{#432904}
Patch Set 1 #
Total comments: 8
Patch Set 2 : remove the error message check. #
Total comments: 2
Patch Set 3 : add comment #Messages
Total messages: 18 (5 generated)
gmanikpure@chromium.org changed reviewers: + samuong@chromium.org
Hi Sam, Could you please take a look whenever time permits? It's a workaround for https://bugs.chromium.org/p/chromedriver/issues/detail?id=1500 Thanks, Gauri
https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... File chrome/test/chromedriver/chrome/javascript_dialog_manager.cc (right): https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:42: // JavaScriptDialogManager::HandleJavaScriptDialog function fails. This comment doesn't add much, I don't think it's necessary https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:44: std::string::npos)) { This isn't a very specific error message (it gets sent back for any error that Chrome's JavaScriptDialogManager::HandleJavaScriptDialog sends back) so there's not much to be gained from checking this. https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:46: } nit: don't need braces for a single-line if body
https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... File chrome/test/chromedriver/chrome/javascript_dialog_manager.cc (right): https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:42: // JavaScriptDialogManager::HandleJavaScriptDialog function fails. On 2016/11/16 19:06:05, samuong wrote: > This comment doesn't add much, I don't think it's necessary ok.I will remove it. https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:44: std::string::npos)) { On 2016/11/16 19:06:04, samuong wrote: > This isn't a very specific error message (it gets sent back for any error that > Chrome's JavaScriptDialogManager::HandleJavaScriptDialog sends back) so there's > not much to be gained from checking this. Oh ok, the response received is :- "[38.955][DEBUG]: DEVTOOLS RESPONSE Page.handleJavaScriptDialog (id=20) {"code":-32603,"message":"Could not handle JavaScript dialog"}" Shall i check the code '-32603' instead ? https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:46: } On 2016/11/16 19:06:04, samuong wrote: > nit: don't need braces for a single-line if body oops, sorry. I had some logging statements inside the if body for analysis purpose and forgot to remove those braces.
https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... File chrome/test/chromedriver/chrome/javascript_dialog_manager.cc (right): https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:44: std::string::npos)) { On 2016/11/16 20:22:40, gmanikpure wrote: > On 2016/11/16 19:06:04, samuong wrote: > > This isn't a very specific error message (it gets sent back for any error that > > Chrome's JavaScriptDialogManager::HandleJavaScriptDialog sends back) so > there's > > not much to be gained from checking this. > > Oh ok, the response received is :- "[38.955][DEBUG]: DEVTOOLS RESPONSE > Page.handleJavaScriptDialog (id=20) {"code":-32603,"message":"Could not handle > JavaScript dialog"}" > > Shall i check the code '-32603' instead ? You can just check "status.IsError()", this should be enough to determine whether an error occurred or not.
https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... File chrome/test/chromedriver/chrome/javascript_dialog_manager.cc (right): https://codereview.chromium.org/2503533003/diff/1/chrome/test/chromedriver/ch... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:44: std::string::npos)) { On 2016/11/16 21:04:04, samuong wrote: > On 2016/11/16 20:22:40, gmanikpure wrote: > > On 2016/11/16 19:06:04, samuong wrote: > > > This isn't a very specific error message (it gets sent back for any error > that > > > Chrome's JavaScriptDialogManager::HandleJavaScriptDialog sends back) so > > there's > > > not much to be gained from checking this. > > > > Oh ok, the response received is :- "[38.955][DEBUG]: DEVTOOLS RESPONSE > > Page.handleJavaScriptDialog (id=20) {"code":-32603,"message":"Could not handle > > JavaScript dialog"}" > > > > Shall i check the code '-32603' instead ? > > You can just check "status.IsError()", this should be enough to determine > whether an error occurred or not. Done.Please take a look.
https://codereview.chromium.org/2503533003/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/javascript_dialog_manager.cc (right): https://codereview.chromium.org/2503533003/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:40: if (status.IsError()) { sorry, one more thing: can you add a comment here saying: Retry once to work around https://bugs.chromium.org/p/chromedriver/issues/detail?id=1500
https://codereview.chromium.org/2503533003/diff/20001/chrome/test/chromedrive... File chrome/test/chromedriver/chrome/javascript_dialog_manager.cc (right): https://codereview.chromium.org/2503533003/diff/20001/chrome/test/chromedrive... chrome/test/chromedriver/chrome/javascript_dialog_manager.cc:40: if (status.IsError()) { On 2016/11/16 22:53:34, samuong wrote: > sorry, one more thing: can you add a comment here saying: > > Retry once to work around > https://bugs.chromium.org/p/chromedriver/issues/detail?id=1500 Done.
lgtm
The CQ bit was checked by gmanikpure@chromium.org
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
Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by gmanikpure@chromium.org
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [chromedriver] Retry alert handling if HandleJavaScriptDialog fails. BUG=chromedriver:1500 ========== to ========== [chromedriver] Retry alert handling if HandleJavaScriptDialog fails. BUG=chromedriver:1500 Committed: https://crrev.com/2bc717ca99f16e07f704058c6f3faea8cfa647e9 Cr-Commit-Position: refs/heads/master@{#432904} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/2bc717ca99f16e07f704058c6f3faea8cfa647e9 Cr-Commit-Position: refs/heads/master@{#432904} |
