Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1827003004:
[Chromedriver] Chromedriver should handle unexpected alert automatically. (Closed)
|
Created:
4 years, 9 months ago by gmanikpure Modified:
4 years ago Reviewers:
samuong CC:
chromium-reviews, samuong+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Chromedriver] Chromedriver should handle unexpected alert automatically.
Chrome currently leaves the alert around which causes subsequent commands to fail.
https://buganizer.corp.google.com/u/0/issues/6507465#comment12
BUG=chromedriver:877
Committed: https://crrev.com/2c6f4026ae8b74c33dacfb2a7512db813b214833
Cr-Commit-Position: refs/heads/master@{#434696}
Patch Set 1 #
Total comments: 4
Patch Set 2 : fixed nits #Patch Set 3 : close the dialog depending on 'unexpectedAlertBehaviour' value #Patch Set 4 : fix nits & remove obsolete py test #Patch Set 5 : New modifications #
Total comments: 15
Patch Set 6 : make ignore value as default & fix nits. #
Total comments: 16
Patch Set 7 : fix nits, optimize, change arg of CreateCapabilities() #Patch Set 8 : fix some more nits #Patch Set 9 : remove getter, use variable directly. #
Messages
Total messages: 35 (12 generated)
gmanikpure@chromium.org changed reviewers: + samuong@chromium.org
Hi Sam, this CL contains fix for comment #12 of bugnaizer issue https://buganizer.corp.google.com/u/0/issues/6507465#comment12 It dismisses the unexpected alert & returns alert text information in the exception message. But UnhandledAlertException.getAlertText() still returns null, trying to figure out how it works and may be we can take that in the another CL (?). Could you please review whenever time permits? Thanks
lgtm with nits nit: add a space between "unexpected" and "alert" in the title. https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:225: status = web_view->GetJavaScriptDialogManager() please add a comment above this line saying: // Close the dialog before returning an error, so that subsequent commands do not fail. https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:226: ->HandleDialog(true, session->prompt_text.get()); nit: break line after the opening ( for HandleDialog rather than before the ->
Description was changed from ========== [Chromedriver] Chromedriver should handle unexpectedalert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. BUG= ========== to ========== [Chromedriver] Chromedriver should handle unexpectedalert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG= ==========
Description was changed from ========== [Chromedriver] Chromedriver should handle unexpectedalert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG= ========== to ========== [Chromedriver] Chromedriver should handle unexpected alert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG= ==========
Thanks Sam, Fixed the nits, will run presubmit tonight before committing the CL. https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:225: status = web_view->GetJavaScriptDialogManager() On 2016/03/30 17:22:56, samuong wrote: > please add a comment above this line saying: > > // Close the dialog before returning an error, so that subsequent commands do > not fail. Done. https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... chrome/test/chromedriver/window_commands.cc:226: ->HandleDialog(true, session->prompt_text.get()); On 2016/03/30 17:22:56, samuong wrote: > nit: break line after the opening ( for HandleDialog rather than before the -> Done.
On 2016/04/01 16:54:07, gmanikpure wrote: > Thanks Sam, > Fixed the nits, will run presubmit tonight before committing the CL. > > https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... > File chrome/test/chromedriver/window_commands.cc (right): > > https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... > chrome/test/chromedriver/window_commands.cc:225: status = > web_view->GetJavaScriptDialogManager() > On 2016/03/30 17:22:56, samuong wrote: > > please add a comment above this line saying: > > > > // Close the dialog before returning an error, so that subsequent commands do > > not fail. > > Done. > > https://codereview.chromium.org/1827003004/diff/1/chrome/test/chromedriver/wi... > chrome/test/chromedriver/window_commands.cc:226: ->HandleDialog(true, > session->prompt_text.get()); > On 2016/03/30 17:22:56, samuong wrote: > > nit: break line after the opening ( for HandleDialog rather than before the -> > > Done. not lgtm, let's hide this behind unexpectedAlertBehaviour as marc suggested
Hi Sam, Please take a look.Thanks
Sam, I uploaded new patch with more modifications as I felt patch4 is not implemented like other capabilities. Could you please have a look at both patch 4 & the latest patch5? Also, I executed run_py_tests.py , run_java_tests.py , chromedriver_tests and chromedriver_unittests to make sure no regression is introduced. Presubmit also looks good - https://test.corp.google.com/ui#cl=137204043 (please ignore android test failures, they failed because google3 has older version of apk) Although Java test suite already has tests 'UnexpectedAlertBehaviorTest.*' for this feature, I have included another test in run_py_tests.py because these java tests fails on android (i guess because 2 drivers are created)
https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/capabilities.cc (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/capabilities.cc:616: unexpected_alert_behaviour(""), should this be the empty string, or "ignore"? https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1666: nit: put two blank lines between classes https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1674: driver.ExecuteScript('alert(\'HI\');') nit: use " and ' instead of escaping the nested ' https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1675: self.WaitForCondition(lambda: driver.IsAlertOpen()) no need for a lambda, just do self.WaitForCondition(driver.IsAlertOpen) https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/window_commands.cc:223: std::string alert_behaviour = session->chrome->UnexpectedAlertBehaviour(); nit: do this just before the if statement on line 231 https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/window_commands.cc:236: ->HandleDialog(false, session->prompt_text.get()); does "ignore" mean "don't throw an error", or should we still be throwing an unexpected alert error?
Description was changed from ========== [Chromedriver] Chromedriver should handle unexpected alert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG= ========== to ========== [Chromedriver] Chromedriver should handle unexpected alert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG=chromedriver:877 ==========
https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/capabilities.cc (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/capabilities.cc:616: unexpected_alert_behaviour(""), On 2016/11/02 22:48:48, samuong wrote: > should this be the empty string, or "ignore"? Done.Making ignore value as default. https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1666: On 2016/11/02 22:48:48, samuong wrote: > nit: put two blank lines between classes Done.Also added blank lines for other classes. https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1674: driver.ExecuteScript('alert(\'HI\');') On 2016/11/02 22:48:48, samuong wrote: > nit: use " and ' instead of escaping the nested ' Done. https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1675: self.WaitForCondition(lambda: driver.IsAlertOpen()) On 2016/11/02 22:48:48, samuong wrote: > no need for a lambda, just do self.WaitForCondition(driver.IsAlertOpen) Done, but remvoing lambda throws "TypeError: 'bool' object is not callable". Shall i change it back? https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/window_commands.cc:223: std::string alert_behaviour = session->chrome->UnexpectedAlertBehaviour(); On 2016/11/02 22:48:48, samuong wrote: > nit: do this just before the if statement on line 231 Done. https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/window_commands.cc:236: ->HandleDialog(false, session->prompt_text.get()); On 2016/11/02 22:48:48, samuong wrote: > does "ignore" mean "don't throw an error", or should we still be throwing an > unexpected alert error? As discussed, we will be throwing unexpected alert error for ignore value. https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.h (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.h:90: static const char kAccept[]; As discussed, adding values of unexpectedalertbehaviour as constants in session.h/session.cc. https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/window_commands.cc:230: std::string alert_behaviour = session->GetUnexpectedAlertBehaviour(); As discussed, directly putting it in session instead of session->chrome.
Ping
https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.cc:66: if (unexpected_alert_behaviour.empty()) is it possible for this to ever be set to the empty string? if not, i think this getter should just return |unexpected_alert_behaviour| also, "simple" (inline) getters should be named using camel case, so you can name this function "Session::unexpected_alert_behavior()" https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.h (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.h:90: static const char kAccept[]; On 2016/11/04 00:23:24, gmanikpure wrote: > As discussed, adding values of unexpectedalertbehaviour as constants in > session.h/session.cc. these don't need to be inside the Session class, you can put them at the top level instead https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.h:93: std::string GetUnexpectedAlertBehaviour() const; this is a method, so group it with the other methods, rather than with the variables https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session_commands.cc:107: std::unique_ptr<base::DictionaryValue> CreateCapabilities(Chrome* chrome) { since capabilities belong to the session, i think it would be reasonable to change the argument of this function to: const Session* session then you wouldn't need to call GetThreadLocalSession, and it might simplify the implementation of some capabilities that we add in the future https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/window_commands.cc:220: given the number of times we call GetJavaScriptDialogManager, it would be nice if there was a JavaScriptDialogManager* dialog_manager = web_view->GetJavaScriptDialogManager(); above the if statement, and then you can just refer to dialog_manager
https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1675: self.WaitForCondition(lambda: driver.IsAlertOpen()) On 2016/11/04 00:23:24, gmanikpure wrote: > On 2016/11/02 22:48:48, samuong wrote: > > no need for a lambda, just do self.WaitForCondition(driver.IsAlertOpen) > > Done, but remvoing lambda throws "TypeError: 'bool' object is not callable". > Shall i change it back? I added lambda back. https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.cc:66: if (unexpected_alert_behaviour.empty()) On 2016/11/22 19:52:16, samuong wrote: > is it possible for this to ever be set to the empty string? if not, i think this > getter should just return |unexpected_alert_behaviour| > > also, "simple" (inline) getters should be named using camel case, so you can > name this function "Session::unexpected_alert_behavior()" Empty string can be set in DesiredCapabilities "dc.setCapability("unexpectedAlertBehaviour", "");" But I am already checking that it should'nt be empty in ParseUnexpectedAlertBehaviour(). Sorry, removed it. https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.h (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.h:90: static const char kAccept[]; On 2016/11/22 19:52:16, samuong wrote: > On 2016/11/04 00:23:24, gmanikpure wrote: > > As discussed, adding values of unexpectedalertbehaviour as constants in > > session.h/session.cc. > > these don't need to be inside the Session class, you can put them at the top > level instead Done. https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.h:93: std::string GetUnexpectedAlertBehaviour() const; On 2016/11/22 19:52:16, samuong wrote: > this is a method, so group it with the other methods, rather than with the > variables Done. https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session_commands.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session_commands.cc:107: std::unique_ptr<base::DictionaryValue> CreateCapabilities(Chrome* chrome) { On 2016/11/22 19:52:16, samuong wrote: > since capabilities belong to the session, i think it would be reasonable to > change the argument of this function to: > > const Session* session > > then you wouldn't need to call GetThreadLocalSession, and it might simplify the > implementation of some capabilities that we add in the future Done.Thank you! https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/window_commands.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/window_commands.cc:220: On 2016/11/22 19:52:16, samuong wrote: > given the number of times we call GetJavaScriptDialogManager, it would be nice > if there was a > > JavaScriptDialogManager* dialog_manager = > web_view->GetJavaScriptDialogManager(); > > above the if statement, and then you can just refer to dialog_manager Done.Thank you for this suggestion, it helped optimizing the code.
lgtm, with two nits https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1675: self.WaitForCondition(lambda: driver.IsAlertOpen()) On 2016/11/23 18:34:25, gmanikpure wrote: > On 2016/11/04 00:23:24, gmanikpure wrote: > > On 2016/11/02 22:48:48, samuong wrote: > > > no need for a lambda, just do self.WaitForCondition(driver.IsAlertOpen) > > > > Done, but remvoing lambda throws "TypeError: 'bool' object is not callable". > > Shall i change it back? > > I added lambda back. I think you forgot to remove the () after IsAlertOpen. You can change it to: self.WaitForCondition(driver.IsAlertOpen) but you might've had: self.WaitForCondition(driver.IsAlertOpen()) ? https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.cc:66: if (unexpected_alert_behaviour.empty()) On 2016/11/23 18:34:26, gmanikpure wrote: > On 2016/11/22 19:52:16, samuong wrote: > > is it possible for this to ever be set to the empty string? if not, i think > this > > getter should just return |unexpected_alert_behaviour| > > > > also, "simple" (inline) getters should be named using camel case, so you can > > name this function "Session::unexpected_alert_behavior()" > > Empty string can be set in DesiredCapabilities > "dc.setCapability("unexpectedAlertBehaviour", "");" > But I am already checking that it should'nt be empty in > ParseUnexpectedAlertBehaviour(). Sorry, removed it. There's no need for the "get_" prefix for this function name, you can just call it "unexpected_alert_behavior()" as per https://google.github.io/styleguide/cppguide.html#Function_Names
https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... File chrome/test/chromedriver/test/run_py_tests.py (right): https://codereview.chromium.org/1827003004/diff/80001/chrome/test/chromedrive... chrome/test/chromedriver/test/run_py_tests.py:1675: self.WaitForCondition(lambda: driver.IsAlertOpen()) On 2016/11/23 21:28:01, samuong wrote: > On 2016/11/23 18:34:25, gmanikpure wrote: > > On 2016/11/04 00:23:24, gmanikpure wrote: > > > On 2016/11/02 22:48:48, samuong wrote: > > > > no need for a lambda, just do self.WaitForCondition(driver.IsAlertOpen) > > > > > > Done, but remvoing lambda throws "TypeError: 'bool' object is not callable". > > > Shall i change it back? > > > > I added lambda back. > > I think you forgot to remove the () after IsAlertOpen. You can change it to: > > self.WaitForCondition(driver.IsAlertOpen) > > but you might've had: > > self.WaitForCondition(driver.IsAlertOpen()) > > ? Oops sorry, yes you are right :) Made the change. https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.cc:66: if (unexpected_alert_behaviour.empty()) On 2016/11/23 21:28:01, samuong wrote: > On 2016/11/23 18:34:26, gmanikpure wrote: > > On 2016/11/22 19:52:16, samuong wrote: > > > is it possible for this to ever be set to the empty string? if not, i think > > this > > > getter should just return |unexpected_alert_behaviour| > > > > > > also, "simple" (inline) getters should be named using camel case, so you can > > > name this function "Session::unexpected_alert_behavior()" > > > > Empty string can be set in DesiredCapabilities > > "dc.setCapability("unexpectedAlertBehaviour", "");" > > But I am already checking that it should'nt be empty in > > ParseUnexpectedAlertBehaviour(). Sorry, removed it. > > There's no need for the "get_" prefix for this function name, you can just call > it "unexpected_alert_behavior()" as per > https://google.github.io/styleguide/cppguide.html#Function_Names Actually, I was getting multiple 'duplicate member' errors on using same function name as the variable name "unexpected_alert_behaviour" so added "get_". But as you suggested, I will use "unexpected_alert_behavior()" without 'u'.
https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.cc:66: if (unexpected_alert_behaviour.empty()) On 2016/11/23 22:04:10, gmanikpure wrote: > On 2016/11/23 21:28:01, samuong wrote: > > On 2016/11/23 18:34:26, gmanikpure wrote: > > > On 2016/11/22 19:52:16, samuong wrote: > > > > is it possible for this to ever be set to the empty string? if not, i > think > > > this > > > > getter should just return |unexpected_alert_behaviour| > > > > > > > > also, "simple" (inline) getters should be named using camel case, so you > can > > > > name this function "Session::unexpected_alert_behavior()" > > > > > > Empty string can be set in DesiredCapabilities > > > "dc.setCapability("unexpectedAlertBehaviour", "");" > > > But I am already checking that it should'nt be empty in > > > ParseUnexpectedAlertBehaviour(). Sorry, removed it. > > > > There's no need for the "get_" prefix for this function name, you can just > call > > it "unexpected_alert_behavior()" as per > > https://google.github.io/styleguide/cppguide.html#Function_Names > > Actually, I was getting multiple 'duplicate member' errors on using same > function name as the variable name "unexpected_alert_behaviour" so added "get_". > But as you suggested, I will use "unexpected_alert_behavior()" without 'u'. Oh, I just realized that Session is a struct and not a class. You don't need a getter at all then, just reference the variable directly.
https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... File chrome/test/chromedriver/session.cc (right): https://codereview.chromium.org/1827003004/diff/100001/chrome/test/chromedriv... chrome/test/chromedriver/session.cc:66: if (unexpected_alert_behaviour.empty()) On 2016/11/23 22:11:00, samuong wrote: > On 2016/11/23 22:04:10, gmanikpure wrote: > > On 2016/11/23 21:28:01, samuong wrote: > > > On 2016/11/23 18:34:26, gmanikpure wrote: > > > > On 2016/11/22 19:52:16, samuong wrote: > > > > > is it possible for this to ever be set to the empty string? if not, i > > think > > > > this > > > > > getter should just return |unexpected_alert_behaviour| > > > > > > > > > > also, "simple" (inline) getters should be named using camel case, so you > > can > > > > > name this function "Session::unexpected_alert_behavior()" > > > > > > > > Empty string can be set in DesiredCapabilities > > > > "dc.setCapability("unexpectedAlertBehaviour", "");" > > > > But I am already checking that it should'nt be empty in > > > > ParseUnexpectedAlertBehaviour(). Sorry, removed it. > > > > > > There's no need for the "get_" prefix for this function name, you can just > > call > > > it "unexpected_alert_behavior()" as per > > > https://google.github.io/styleguide/cppguide.html#Function_Names > > > > Actually, I was getting multiple 'duplicate member' errors on using same > > function name as the variable name "unexpected_alert_behaviour" so added > "get_". > > But as you suggested, I will use "unexpected_alert_behavior()" without 'u'. > > Oh, I just realized that Session is a struct and not a class. You don't need a > getter at all then, just reference the variable directly. Hmm, yeah. I missed noticing that, thanks for pointing it out! Please take a look.
still lgtm, thanks
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: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1480356447962620,
"parent_rev": "dfa3182281731bc3a4afcb51abcedb421bd19e3a", "commit_rev":
"44a872667aeab7ce062c7d25e7627f92e048a9af"}
Message was sent while issue was closed.
Description was changed from ========== [Chromedriver] Chromedriver should handle unexpected alert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG=chromedriver:877 ========== to ========== [Chromedriver] Chromedriver should handle unexpected alert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG=chromedriver:877 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Chromedriver] Chromedriver should handle unexpected alert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG=chromedriver:877 ========== to ========== [Chromedriver] Chromedriver should handle unexpected alert automatically. Chrome currently leaves the alert around which causes subsequent commands to fail. https://buganizer.corp.google.com/u/0/issues/6507465#comment12 BUG=chromedriver:877 Committed: https://crrev.com/2c6f4026ae8b74c33dacfb2a7512db813b214833 Cr-Commit-Position: refs/heads/master@{#434696} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/2c6f4026ae8b74c33dacfb2a7512db813b214833 Cr-Commit-Position: refs/heads/master@{#434696} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
