Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9372120:
Implementation of AutomationEventQueue and associated framework to support generic non-blocking aut… (Closed)
|
Created:
8 years, 10 months ago by craigdh Modified:
8 years, 9 months ago CC:
chromium-reviews, Nirnimesh, John Grabowski, kkania, anantha, robertshield, dyu1, dennis_jeffrey Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplementation of AutomationEventQueue and associated framework to support generic non-blocking automation events.
Change-Id: I39f50d1d5b321a863572eec6c61e11923092ed47
BUG=98289
TEST=functional/apptest.py
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125069
Patch Set 1 #
Total comments: 25
Patch Set 2 : Addressed Dennis' comments. #
Total comments: 67
Patch Set 3 : Addressed frankf's comments on the Python. #Patch Set 4 : Addressed Nirnimesh's comments. #
Total comments: 38
Patch Set 5 : Addressed Nirnimesh's second round of comments. #
Total comments: 40
Patch Set 6 : Addressed Frank and Nirnimesh's second set of comments. #Patch Set 7 : Lazy creation of the AutomationEventQueue instance. #Patch Set 8 : Made CompareEventId predicate class private to AutomationEventQueue. #
Total comments: 26
Patch Set 9 : Addressed Nirnimesh's most recent comments. #Messages
Total messages: 19 (0 generated)
Cool! Can't wait to start getting rid of our WaitUntil's :-) I have some drive-by comments from the python side of things (I wanted to look over it to see how to use the new functionality from a pyauto test). https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/data/apptes... File chrome/test/data/apptest_basic.html (right): https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/data/apptes... chrome/test/data/apptest_basic.html:1: <html> I recommend adding a comment at the top of this file describing its purpose. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... File chrome/test/functional/apptest.py (right): https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... chrome/test/functional/apptest.py:1: # Copyright (c) 2011 The Chromium Authors. All rights reserved. 2011 --> 2012 https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... chrome/test/functional/apptest.py:12: class PyAutoEvents(pyauto.PyUITest): nit: PyAutoEvents --> PyAutoEventsTest https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... chrome/test/functional/apptest.py:20: self.verifyNextEvent(id, 'init') Is there a place where we can see all the event strings we can wait for? Would it make sense to document that in pyauto.py? https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... chrome/test/functional/apptest.py:32: pass indent by 2 fewer spaces https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2801: def ObserveRaisedEvents(self, event_name="", tab_index=0, windex=0, prefer single quotes over double quotes https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2801: def ObserveRaisedEvents(self, event_name="", tab_index=0, windex=0, Would a name like "GetEventObserver" be better here? https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2813: 'frame_xpath' : frame_xpath, for the above 3 argument descriptions, we should have a brief sentence describing the argument, rather than just repeating the argument name. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2830: timeout=50000).get('observer_id') should we use "self.large_test_timeout_ms()" instead of hard-coding 50 seconds here? https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2830: timeout=50000).get('observer_id') will the call to get() here ever return None? (I think that happens in the case that the specified key doesn't exist in the dictionary). If so, we should mention this in the docstring above. If not, maybe we could just do ['observer_id'] instead of .get('observer_id') https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2832: def GetEvent(self, observer_id=-1, blocking=True): If this function gets an event, is that event removed from the event queue? https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2843: Event response. is this a string or a number? https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2881: def ClearEventObservers(self): Would it make sense to have pyauto automatically call this at the end of each executed test, or is that not necessary? (same with function ClearEvents above).
https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/data/apptes... File chrome/test/data/apptest_basic.html (right): https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/data/apptes... chrome/test/data/apptest_basic.html:1: <html> On 2012/02/23 19:26:36, dennis_jeffrey wrote: > I recommend adding a comment at the top of this file describing its purpose. Done. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... File chrome/test/functional/apptest.py (right): https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... chrome/test/functional/apptest.py:1: # Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2012/02/23 19:26:36, dennis_jeffrey wrote: > 2011 --> 2012 Done. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... chrome/test/functional/apptest.py:12: class PyAutoEvents(pyauto.PyUITest): On 2012/02/23 19:26:36, dennis_jeffrey wrote: > nit: PyAutoEvents --> PyAutoEventsTest Done. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... chrome/test/functional/apptest.py:20: self.verifyNextEvent(id, 'init') On 2012/02/23 19:26:36, dennis_jeffrey wrote: > Is there a place where we can see all the event strings we can wait for? Would > it make sense to document that in pyauto.py? The event name is just an argument to raiseEvent(). For example, here we are waiting for the app to call window.domAutomationController.raiseEvent('init'). (The actual Javascript implementation is actually slightly messier but is fixed in a CL I have ready to follow this one. It should all be clear if you take a look at raiseEvent() in apptest_basic.html) https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/functional/... chrome/test/functional/apptest.py:32: pass On 2012/02/23 19:26:36, dennis_jeffrey wrote: > indent by 2 fewer spaces Done. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2801: def ObserveRaisedEvents(self, event_name="", tab_index=0, windex=0, On 2012/02/23 19:26:36, dennis_jeffrey wrote: > Would a name like "GetEventObserver" be better here? Perhaps "AddRaisedEventObserver" as there are going to be a few different types of event observers in the future. The Raised Event Observer is just the simplest type (the DOM Mutation Event Observer is next and will build on the Raised Event Observer). https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2813: 'frame_xpath' : frame_xpath, On 2012/02/23 19:26:36, dennis_jeffrey wrote: > for the above 3 argument descriptions, we should have a brief sentence > describing the argument, rather than just repeating the argument name. Copy-Paste error. Done. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2830: timeout=50000).get('observer_id') On 2012/02/23 19:26:36, dennis_jeffrey wrote: > will the call to get() here ever return None? (I think that happens in the case > that the specified key doesn't exist in the dictionary). If so, we should > mention this in the docstring above. If not, maybe we could just do > ['observer_id'] instead of .get('observer_id') Good point. It should never return None. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2830: timeout=50000).get('observer_id') On 2012/02/23 19:26:36, dennis_jeffrey wrote: > should we use "self.large_test_timeout_ms()" instead of hard-coding 50 seconds > here? I wasn't aware that existed. Done. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2832: def GetEvent(self, observer_id=-1, blocking=True): On 2012/02/23 19:26:36, dennis_jeffrey wrote: > If this function gets an event, is that event removed from the event queue? Yes. Clarified this in the comments. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2843: Event response. On 2012/02/23 19:26:36, dennis_jeffrey wrote: > is this a string or a number? Neither, it is a dictionary. Added an example. https://chromiumcodereview.appspot.com/9372120/diff/1/chrome/test/pyautolib/p... chrome/test/pyautolib/pyauto.py:2881: def ClearEventObservers(self): On 2012/02/23 19:26:36, dennis_jeffrey wrote: > Would it make sense to have pyauto automatically call this at the end of each > executed test, or is that not necessary? (same with function ClearEvents above). So long as PyAuto always restarts the browser before each test the queue will be cleared then. Is there any case in which PyAuto does not restart the browser before a test?
More to come... https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2802: frame_xpath=''): alignment issue https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2809: 'event_name': The raised event name to watch for. By default all raised why is the arg in quotes? https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2811: windex: index of the window. follow the order of function signature. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2827: 'tab_index' : tab_index, same with ordering here. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2842: 'observer_id': The id of the observer to wait for, matches any event by again, why quotes? https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2864: return self._GetResultFromJSONRequest(cmd_dict, windex=None, timeout=50000) if feel like it, define 50000 as a constant like large_test_timeout_ms.
So excited to see this. Many lives will be saved by the simplicity this brings. http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/au... File chrome/browser/automation/automation_event_observer.cc (right): http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/au... chrome/browser/automation/automation_event_observer.cc:57: void RaisedEventObserver::OnJavascriptBlocked() { Add a test for one of the failure scenarios. Maybe when js is disabled (--disable-javascript) http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/au... File chrome/browser/automation/automation_event_observer.h (right): http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/au... chrome/browser/automation/automation_event_observer.h:38: class RaisedEventObserver Add 'Dom' http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/au... File chrome/browser/automation/automation_event_queue.cc (right): http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/au... chrome/browser/automation/automation_event_queue.cc:39: bool r = CheckReturnEvent(); use better varnames http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/au... File chrome/browser/automation/automation_event_queue.h (right): http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/au... chrome/browser/automation/automation_event_queue.h:22: class AutomationEventQueue { Should 'App' be added? or is this meant to be generic (for non-app events as well)? http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:6469: RaisedEventObserver* jsobserver = if the user fails to call RemoveEventObserver, does this leak? http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:6476: void TestingAutomationProvider::RemoveEventObserver( this method name does not have 'Raised'. http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:6485: automation_event_queue_.RemoveObserver(observer_id); what if observer_id is invalid? http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:6489: void TestingAutomationProvider::ClearEvents( Shouldn't this imply ClearEventObservers as well? http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.h (right): http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.h:957: void AddRaisedEventObserver( Add 'dom' or 'app' http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.h:976: void RemoveEventObserver( Move this next to AddRaisedEventObserver http://codereview.chromium.org/9372120/diff/4001/chrome/test/data/apptest_bas... File chrome/test/data/apptest_basic.html (right): http://codereview.chromium.org/9372120/diff/4001/chrome/test/data/apptest_bas... chrome/test/data/apptest_basic.html:1: <!DOCTYPE html> I recommend moving this file a dir 'apptest' so that you can add more samples http://codereview.chromium.org/9372120/diff/4001/chrome/test/data/apptest_bas... chrome/test/data/apptest_basic.html:3: use of the Automation Event Queue for testing. --> testing -> testing webapps http://codereview.chromium.org/9372120/diff/4001/chrome/test/data/apptest_bas... chrome/test/data/apptest_basic.html:3: use of the Automation Event Queue for testing. --> Describe a little what this app does. "Simulates an asynchronous flow for login.. http://codereview.chromium.org/9372120/diff/4001/chrome/test/data/apptest_bas... chrome/test/data/apptest_basic.html:15: function delay(f, ms) { Add doc http://codereview.chromium.org/9372120/diff/4001/chrome/test/data/apptest_bas... chrome/test/data/apptest_basic.html:15: function delay(f, ms) { rename: delayedCallback http://codereview.chromium.org/9372120/diff/4001/chrome/test/data/apptest_bas... chrome/test/data/apptest_basic.html:32: function loginReady() { createLoginLink http://codereview.chromium.org/9372120/diff/4001/chrome/test/data/apptest_bas... chrome/test/data/apptest_basic.html:41: return false; why? http://codereview.chromium.org/9372120/diff/4001/chrome/test/functional/appte... File chrome/test/functional/apptest.py (right): http://codereview.chromium.org/9372120/diff/4001/chrome/test/functional/appte... chrome/test/functional/apptest.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Include this test in PYAUTO_TESTS http://codereview.chromium.org/9372120/diff/4001/chrome/test/functional/appte... chrome/test/functional/apptest.py:9: import pyauto_errors unused? http://codereview.chromium.org/9372120/diff/4001/chrome/test/functional/appte... chrome/test/functional/apptest.py:18: id = self.AddRaisedEventObserver(); id is a python keyword. http://codereview.chromium.org/9372120/diff/4001/chrome/test/functional/appte... chrome/test/functional/apptest.py:26: def verifyNextEvent(self, id, event_name): Method name should begin with capital letter Prefix _ if it's meant to be private http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2: # Copyright (c) 2012 The Chromium Authors. All rights reserved. Explain the motivation in the CL description. http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2801: def AddRaisedEventObserver(self, event_name='', tab_index=0, windex=0, It's not clear to the end-user what 'Raised event' is, and when to use it. Rename (maybe add 'app') and explain in the docstring. http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2804: Add a TODO to add a corresponding method for extension views too (ala ExecuteJavascript) http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2833: def GetEvent(self, observer_id=-1, blocking=True): add 'app' somewhere in the name. http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2834: """Waits for an event to occur. Waits for one of the observed events to occur http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2864: return self._GetResultFromJSONRequest(cmd_dict, windex=None, timeout=50000) why 50s? Leave it as the default, but provide a way to pass it from param to GetEvent() since there might be app functions that'd require a timeout of > default automation timeout http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2882: """Removes all events currently in the event queue. event -> app event Repeat everywhere http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2901: return self._GetResultFromJSONRequest(cmd_dict, windex=None, timeout=50000) why override this timeout?
https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2802: frame_xpath=''): On 2012/02/24 22:05:07, frankf wrote: > alignment issue Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2809: 'event_name': The raised event name to watch for. By default all raised On 2012/02/24 22:05:07, frankf wrote: > why is the arg in quotes? Because it was copy-pasted from the cmd_dict and I missed them. Fixed. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2811: windex: index of the window. On 2012/02/24 22:05:07, frankf wrote: > follow the order of function signature. Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2827: 'tab_index' : tab_index, On 2012/02/24 22:05:07, frankf wrote: > same with ordering here. Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2842: 'observer_id': The id of the observer to wait for, matches any event by On 2012/02/24 22:05:07, frankf wrote: > again, why quotes? Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2864: return self._GetResultFromJSONRequest(cmd_dict, windex=None, timeout=50000) On 2012/02/24 22:05:07, frankf wrote: > if feel like it, define 50000 as a constant like large_test_timeout_ms. Oops, I meant to do self.large_test_timeout_ms(). Done.
https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... File chrome/browser/automation/automation_event_observer.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/automation_event_observer.cc:57: void RaisedEventObserver::OnJavascriptBlocked() { On 2012/02/24 23:18:09, Nirnimesh wrote: > Add a test for one of the failure scenarios. Maybe when js is disabled > (--disable-javascript) I'll add the suggested failure test to my upcoming CL fixing the RaisedEventObserver implementation. That will allow the test to be written using the event API as intended (without hacks). https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... File chrome/browser/automation/automation_event_observer.h (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/automation_event_observer.h:38: class RaisedEventObserver On 2012/02/24 23:18:09, Nirnimesh wrote: > Add 'Dom' Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... File chrome/browser/automation/automation_event_queue.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/automation_event_queue.cc:39: bool r = CheckReturnEvent(); On 2012/02/24 23:18:09, Nirnimesh wrote: > use better varnames Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... File chrome/browser/automation/automation_event_queue.h (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/automation_event_queue.h:22: class AutomationEventQueue { On 2012/02/24 23:18:09, Nirnimesh wrote: > Should 'App' be added? or is this meant to be generic (for non-app events as > well)? Although the initial intention is to use the event queue for app automation, it may find future use in non-app-related use cases. The event queue could be applied in any test where there is the possibility of multiple asynchronous events, there is concern about missing events, or there is a need to coordinate events between browser tabs. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... File chrome/browser/automation/testing_automation_provider.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/testing_automation_provider.cc:6469: RaisedEventObserver* jsobserver = On 2012/02/24 23:18:09, Nirnimesh wrote: > if the user fails to call RemoveEventObserver, does this leak? All un-removed observers are freed when the TestingAutomationProvider is destroyed (for example, when the browser is restarted for each PyAuto test). They will continue to observe and raise events until that point. I could add a "single use" option for the use case where you only want to observe for a single event. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/testing_automation_provider.cc:6476: void TestingAutomationProvider::RemoveEventObserver( On 2012/02/24 23:18:09, Nirnimesh wrote: > this method name does not have 'Raised'. It's not intended to, this method can remove any type of observer associated with the AutomationEventQueue (ex. DomMutationEvents which are upcoming). https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/testing_automation_provider.cc:6485: automation_event_queue_.RemoveObserver(observer_id); On 2012/02/24 23:18:09, Nirnimesh wrote: > what if observer_id is invalid? Then it does not remove anything. Do you think that should be an error instead? https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/testing_automation_provider.cc:6489: void TestingAutomationProvider::ClearEvents( On 2012/02/24 23:18:09, Nirnimesh wrote: > Shouldn't this imply ClearEventObservers as well? I was imagining cases where it may be desirable to clear all the events that have occurred but leave the observers in place, so I separated the two functions. It is a tradeoff though, and I don't feel strongly either way if you think it's not worth the additional API complexity. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... File chrome/browser/automation/testing_automation_provider.h (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/testing_automation_provider.h:957: void AddRaisedEventObserver( On 2012/02/24 23:18:09, Nirnimesh wrote: > Add 'dom' or 'app' Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/browser/autom... chrome/browser/automation/testing_automation_provider.h:976: void RemoveEventObserver( On 2012/02/24 23:18:09, Nirnimesh wrote: > Move this next to AddRaisedEventObserver Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/data/app... File chrome/test/data/apptest_basic.html (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/data/app... chrome/test/data/apptest_basic.html:15: function delay(f, ms) { On 2012/02/24 23:18:09, Nirnimesh wrote: > rename: delayedCallback Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/data/app... chrome/test/data/apptest_basic.html:15: function delay(f, ms) { On 2012/02/24 23:18:09, Nirnimesh wrote: > Add doc Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/data/app... chrome/test/data/apptest_basic.html:32: function loginReady() { On 2012/02/24 23:18:09, Nirnimesh wrote: > createLoginLink Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/data/app... chrome/test/data/apptest_basic.html:41: return false; On 2012/02/24 23:18:09, Nirnimesh wrote: > why? If I remember my JS correctly, doesn't the onclick handler need to return false to prevent the clicked link from loading its href=url (which would in this case reload the page)? https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/function... File chrome/test/functional/apptest.py (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/function... chrome/test/functional/apptest.py:9: import pyauto_errors On 2012/02/24 23:18:09, Nirnimesh wrote: > unused? Yep. Removed. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/function... chrome/test/functional/apptest.py:18: id = self.AddRaisedEventObserver(); On 2012/02/24 23:18:09, Nirnimesh wrote: > id is a python keyword. Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/function... chrome/test/functional/apptest.py:26: def verifyNextEvent(self, id, event_name): On 2012/02/24 23:18:09, Nirnimesh wrote: > Method name should begin with capital letter > Prefix _ if it's meant to be private Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/function... chrome/test/functional/apptest.py:26: def verifyNextEvent(self, id, event_name): On 2012/02/24 23:18:09, Nirnimesh wrote: > Method name should begin with capital letter > Prefix _ if it's meant to be private Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2801: def AddRaisedEventObserver(self, event_name='', tab_index=0, windex=0, On 2012/02/24 23:18:09, Nirnimesh wrote: > It's not clear to the end-user what 'Raised event' is, and when to use it. > > Rename (maybe add 'app') and explain in the docstring. Rename done. I have the usage description written up in a CL I will be submitting after this one, as in this CL the usage is hacky and I don't want to document functionality here that hasn't been introduced yet (as noted in apptest.py). https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2833: def GetEvent(self, observer_id=-1, blocking=True): On 2012/02/24 23:18:09, Nirnimesh wrote: > add 'app' somewhere in the name. I don't want to limit the naming to just apps, so I'm renaming this "GetQueuedEvent". https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2834: """Waits for an event to occur. On 2012/02/24 23:18:09, Nirnimesh wrote: > Waits for one of the observed events to occur Done. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2882: """Removes all events currently in the event queue. On 2012/02/24 23:18:09, Nirnimesh wrote: > event -> app event > Repeat everywhere There is no inherent requirement that they have anything to do with webapps, but I will clarify the naming. https://chromiumcodereview.appspot.com/9372120/diff/4001/chrome/test/pyautoli... chrome/test/pyautolib/pyauto.py:2901: return self._GetResultFromJSONRequest(cmd_dict, windex=None, timeout=50000) On 2012/02/24 23:18:09, Nirnimesh wrote: > why override this timeout? No reason. Removed.
http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:6485: automation_event_queue_.RemoveObserver(observer_id); On 2012/02/27 22:43:38, craigdh-chromium wrote: > On 2012/02/24 23:18:09, Nirnimesh wrote: > > what if observer_id is invalid? > > Then it does not remove anything. Do you think that should be an error instead? Yes, an error message would be nice. http://codereview.chromium.org/9372120/diff/4001/chrome/browser/automation/te... chrome/browser/automation/testing_automation_provider.cc:6489: void TestingAutomationProvider::ClearEvents( On 2012/02/27 22:43:38, craigdh-chromium wrote: > On 2012/02/24 23:18:09, Nirnimesh wrote: > > Shouldn't this imply ClearEventObservers as well? > > I was imagining cases where it may be desirable to clear all the events that > have occurred but leave the observers in place, so I separated the two > functions. It is a tradeoff though, and I don't feel strongly either way if you > think it's not worth the additional API complexity. Unless you call ClearEventObservers as well, you can't be sure what the next call to GetEvent() would get, since you don't know what events exactly got deleted (an event might be raised while you're deleting, and the observer will start processing it). http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto.py File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/9372120/diff/4001/chrome/test/pyautolib/pyauto... chrome/test/pyautolib/pyauto.py:2: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/02/24 23:18:09, Nirnimesh wrote: > Explain the motivation in the CL description. Please make the CL description somewhat meaningful and explanatory. http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/a... File chrome/browser/automation/automation_event_queue.cc (right): http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/a... chrome/browser/automation/automation_event_queue.cc:30: AutomationJSONReply(automation, reply_message) This class should not have to respond back to the client directly. It doesn't even need to know about the json interface. WDYT? http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/a... chrome/browser/automation/automation_event_queue.cc:104: std::map<int, AutomationEventObserver*>::iterator i; s/i/it/ http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/a... chrome/browser/automation/automation_event_queue.cc:106: delete (*i).second; i->second http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... File chrome/browser/automation/testing_automation_provider.cc (right): http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6461: Create an instance of AutomationJSONReply http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6465: .SendError("'event_name' missing or invalid"); then use it here http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6470: new DomRaisedEventObserver(automation_event_queue_, event_name); Since automation_event_queue_ owns the new observer, to make the ownership clearer, this should probably be like: automation_event_queue->AddEventObserver(new DomRaisedEventObserver(...)) Then it'd match line 6485 as well http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6473: AutomationJSONReply(this, reply_message).SendSuccess(return_value.get()); and here http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6479: int observer_id; create AutomationJSONReply once, the use it in line 6482 & 6486 http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6492: automation_event_queue_.Clear(); Does this really free up the observers? http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6496: void TestingAutomationProvider::ClearEventObservers( As a user, I don't understand how this differs from the previous method http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6503: void TestingAutomationProvider::GetQueuedEvent( rename to: GetNextEvent? http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6507: bool blocking; define AutomationJSONReply here and use it below http://codereview.chromium.org/9372120/diff/13001/chrome/browser/automation/t... chrome/browser/automation/testing_automation_provider.cc:6519: automation_event_queue_.GetEvent(this, reply_message, observer_id, blocking); does this respond back to the client? Why not fetch the data and return it from here? http://codereview.chromium.org/9372120/diff/13001/chrome/test/functional/PYAU... File chrome/test/functional/PYAUTO_TESTS (right): http://codereview.chromium.org/9372120/diff/13001/chrome/test/functional/PYAU... chrome/test/functional/PYAUTO_TESTS:453: 'apptest', why not in CONTINUOUS? FULL runs on branded builds only http://codereview.chromium.org/9372120/diff/13001/chrome/test/functional/appt... File chrome/test/functional/apptest.py (right): http://codereview.chromium.org/9372120/diff/13001/chrome/test/functional/appt... chrome/test/functional/apptest.py:32: You aren't "verifying" anything in this method assertEqual() here? http://codereview.chromium.org/9372120/diff/13001/chrome/test/pyautolib/pyaut... File chrome/test/pyautolib/pyauto.py (right): http://codereview.chromium.org/9372120/diff/13001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyauto.py:2805: TODO(craigdh): Add documentation for raising an event once it has been Move TODO's outside of the docstring, as comments. http://codereview.chromium.org/9372120/diff/13001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyauto.py:2834: def GetQueuedEvent(self, observer_id=-1, blocking=True, timeout=50000): why 50 secs? Just use the default automation timeout. http://codereview.chromium.org/9372120/diff/13001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyauto.py:2853: SAMPLE: Indent 2 spaces to the right http://codereview.chromium.org/9372120/diff/13001/chrome/test/pyautolib/pyaut... chrome/test/pyautolib/pyauto.py:2854: { 'observer_id': 1, same here
https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:30: AutomationJSONReply(automation, reply_message) On 2012/02/28 09:13:09, Nirnimesh wrote: > This class should not have to respond back to the client directly. It doesn't > even need to know about the json interface. WDYT? I agree. Changed to GetValue(). https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:104: std::map<int, AutomationEventObserver*>::iterator i; On 2012/02/28 09:13:09, Nirnimesh wrote: > s/i/it/ Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:106: delete (*i).second; On 2012/02/28 09:13:09, Nirnimesh wrote: > i->second Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6461: On 2012/02/28 09:13:09, Nirnimesh wrote: > Create an instance of AutomationJSONReply Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6465: .SendError("'event_name' missing or invalid"); On 2012/02/28 09:13:09, Nirnimesh wrote: > then use it here Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6470: new DomRaisedEventObserver(automation_event_queue_, event_name); On 2012/02/28 09:13:09, Nirnimesh wrote: > Since automation_event_queue_ owns the new observer, to make the ownership > clearer, this should probably be like: > > automation_event_queue->AddEventObserver(new DomRaisedEventObserver(...)) > > Then it'd match line 6485 as well Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6473: AutomationJSONReply(this, reply_message).SendSuccess(return_value.get()); On 2012/02/28 09:13:09, Nirnimesh wrote: > and here Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6479: int observer_id; On 2012/02/28 09:13:09, Nirnimesh wrote: > create AutomationJSONReply once, the use it in line 6482 & 6486 Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6492: automation_event_queue_.Clear(); On 2012/02/28 09:13:09, Nirnimesh wrote: > Does this really free up the observers? I've now merged ClearEventObservers() into Clear() to simplify the API. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6496: void TestingAutomationProvider::ClearEventObservers( On 2012/02/28 09:13:09, Nirnimesh wrote: > As a user, I don't understand how this differs from the previous method See previous. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6503: void TestingAutomationProvider::GetQueuedEvent( On 2012/02/28 09:13:09, Nirnimesh wrote: > rename to: GetNextEvent? Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6507: bool blocking; On 2012/02/28 09:13:09, Nirnimesh wrote: > define AutomationJSONReply here and use it below Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6519: automation_event_queue_.GetEvent(this, reply_message, observer_id, blocking); On 2012/02/28 09:13:09, Nirnimesh wrote: > does this respond back to the client? > Why not fetch the data and return it from here? Because if there is no matching event in the queue then this function will return without sending an automation reply and the AutomationEventQueue will take ownership of the AutomationJSONReply and respond once a matching event is added. It's similar to the observers in automation_provider_observers.h/cc. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/functio... File chrome/test/functional/PYAUTO_TESTS (right): https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/functio... chrome/test/functional/PYAUTO_TESTS:453: 'apptest', On 2012/02/28 09:13:09, Nirnimesh wrote: > why not in CONTINUOUS? > > FULL runs on branded builds only Moved. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/functio... File chrome/test/functional/apptest.py (right): https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/functio... chrome/test/functional/apptest.py:32: On 2012/02/28 09:13:09, Nirnimesh wrote: > You aren't "verifying" anything in this method > > assertEqual() here? Renamed "_ExpectEvent". The upcoming fixed version of this does an assert, but this hack just throws away everything it is not expecting because it has to ignore ChromeDriver events. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/pyautol... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2805: TODO(craigdh): Add documentation for raising an event once it has been On 2012/02/28 09:13:09, Nirnimesh wrote: > Move TODO's outside of the docstring, as comments. Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2834: def GetQueuedEvent(self, observer_id=-1, blocking=True, timeout=50000): On 2012/02/28 09:13:09, Nirnimesh wrote: > why 50 secs? Just use the default automation timeout. Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2853: SAMPLE: On 2012/02/28 09:13:09, Nirnimesh wrote: > Indent 2 spaces to the right Done. https://chromiumcodereview.appspot.com/9372120/diff/13001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2854: { 'observer_id': 1, On 2012/02/28 09:13:09, Nirnimesh wrote: > same here Done.
https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/automation_event_observer.h (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_observer.h:19: explicit AutomationEventObserver(AutomationEventQueue& event_queue); All reference parameters should be const. If the function is modifying the parameter, you should change this to a pointer. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_observer.h:41: DomRaisedEventObserver(AutomationEventQueue& event_queue, Same here. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_observer.h:42: std::string& event_name); alignment. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:23: : event_value_(event_value), observer_id_(observer_id) {} order of member value initialization should match function signature. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:25: void AutomationEventQueue::GetNextEvent(AutomationJSONReply* reply, You're making a copy of reply, why can't you make it const? https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:61: break; you could use find_if here. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:113: PopEvent(wait_observer_id_); alignment issue
https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/automation_event_observer.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_observer.cc:21: new AutomationEventQueue::AutomationEvent( who deletes this? https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_observer.cc:42: DictionaryValue* dict = new DictionaryValue; where is this deleted? (and line 50, 56) https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:61: break; You could directly call .erase(it) here, instead of the .remove() below https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:64: if (event) { event wil already be NULL if this if is not fired, so the return inside this if block is not necessary. if (event) event_queue.remove(event); return event; https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:85: if (observers_.count(observer_id)) { You don't really need the count. Use observers.find(observer_id) != observers_.end() https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:102: std::list<AutomationEvent*>::iterator i; s/i/it/ https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.h (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.h:57: int observer_id_count_; why is this necessary? It's the same as observers_.length() https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.h:59: scoped_ptr<AutomationJSONReply> wait_automation_reply_; What is this for? Add comments https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6500: scoped_ptr<AutomationJSONReply> reply( won't this get free'd when this method's scope ends? https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6513: automation_event_queue_.GetNextEvent(reply.release(), observer_id, blocking); When all cases of SendSuccess/SendError are not covered by the automation handler, and we expect the reply to be called by someone else, it's fine to create AutomationJSONReply(this, reply_message) in line 6505 and 6509 separately https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.h (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.h:1606: AutomationEventQueue automation_event_queue_; Does this have to be allocated on the stack? It'll end up using memory even if it's not used. How about using scoped_ptr like sync_waiter_ above. Or some sort of lazy initialization. (the automation handlers would need to create an instance if one is not available already, but at least there'll be one only when it's needed) https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/functio... File chrome/test/functional/apptest.py (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/functio... chrome/test/functional/apptest.py:30: while json.loads(self.GetNextEvent(event_id).get('name')) != event_name: if the test fails, this will be stuck forever. Run this loop only N times, for your choice of N https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/pyautol... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2802: frame_xpath=''): align under self https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2821: #TODO(craigdh): Add documentation for raising an event once it has been need space after # https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2848: self.large_test_timeout_ms(). this is too large. default to the regular timeout. self.action_max_timeout_ms() Also, it turns out if you use timeout=-1 above, self._GetResultFromJSONRequest() will use self.action_max_timeout_ms() anyway (pretty much what you said)
https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/automation_event_observer.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_observer.cc:21: new AutomationEventQueue::AutomationEvent( On 2012/02/29 03:22:08, Nirnimesh wrote: > who deletes this? The AutomationEventQueue (when it handles the event or the event is cleared). https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_observer.cc:42: DictionaryValue* dict = new DictionaryValue; On 2012/02/29 03:22:08, Nirnimesh wrote: > where is this deleted? (and line 50, 56) It is owned by the AutomationEvent (as a scoped_ptr) and is therefore destroyed along with the AutomationEvent. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:23: : event_value_(event_value), observer_id_(observer_id) {} On 2012/02/29 02:29:00, frankf wrote: > order of member value initialization should match function signature. Whoops. Done. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:25: void AutomationEventQueue::GetNextEvent(AutomationJSONReply* reply, On 2012/02/29 02:29:00, frankf wrote: > You're making a copy of reply, why can't you make it const? I'm not making a copy, a shared_ptr is taking ownership of this pointer. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:61: break; On 2012/02/29 03:22:08, Nirnimesh wrote: > You could directly call .erase(it) here, instead of the .remove() below Actually, erase() won't take a reverse_iterator and converting between them is messy (I think it would be something like event_queue_.erase(--(it.base())). https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:61: break; On 2012/02/29 02:29:00, frankf wrote: > you could use find_if here. I'd have to write a predicate class to check the id (insert long in-person discussion). Done. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:85: if (observers_.count(observer_id)) { On 2012/02/29 03:22:08, Nirnimesh wrote: > You don't really need the count. > Use observers.find(observer_id) != observers_.end() observers_ is a map so count just returns whether or not the key exists, but if you think it's clearer I'll switch to find. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:102: std::list<AutomationEvent*>::iterator i; On 2012/02/29 03:22:08, Nirnimesh wrote: > s/i/it/ Done. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:113: PopEvent(wait_observer_id_); On 2012/02/29 02:29:00, frankf wrote: > alignment issue Done. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.h (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.h:57: int observer_id_count_; On 2012/02/29 03:22:08, Nirnimesh wrote: > why is this necessary? It's the same as observers_.length() Not exactly, it's only the same until observers are removed. This is used to give each new observer a unique id. Using length() could give duplicate ids in some situations. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/automation_event_queue.h:59: scoped_ptr<AutomationJSONReply> wait_automation_reply_; On 2012/02/29 03:22:08, Nirnimesh wrote: > What is this for? Add comments If there is not a matching event already waiting in the queue when TestingAutomationProvider::GetNextEvent is called, then the automation reply is stored here until a matching event is raised and the automation call can return. Comments added. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6500: scoped_ptr<AutomationJSONReply> reply( On 2012/02/29 03:22:08, Nirnimesh wrote: > won't this get free'd when this method's scope ends? This function doesn't normally respond to the automation call directly, so ownership gets passed off to the AutomationEventQueue. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6513: automation_event_queue_.GetNextEvent(reply.release(), observer_id, blocking); On 2012/02/29 03:22:08, Nirnimesh wrote: > When all cases of SendSuccess/SendError are not covered by the automation > handler, and we expect the reply to be called by someone else, it's fine to > create AutomationJSONReply(this, reply_message) in line 6505 and 6509 separately It's actually cleaner to hold on to this AutomationJSONReply in the AutomationEventQueue than a pointer to the TestingAutomationProvider and the IPC::Message, so this is good. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.h (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.h:1606: AutomationEventQueue automation_event_queue_; On 2012/02/29 03:22:08, Nirnimesh wrote: > Does this have to be allocated on the stack? It'll end up using memory even if > it's not used. > > How about using scoped_ptr like sync_waiter_ above. Or some sort of lazy > initialization. > (the automation handlers would need to create an instance if one is not > available already, but at least there'll be one only when it's needed) A scoped_ptr seems reasonable. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/functio... File chrome/test/functional/apptest.py (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/functio... chrome/test/functional/apptest.py:30: while json.loads(self.GetNextEvent(event_id).get('name')) != event_name: On 2012/02/29 03:22:08, Nirnimesh wrote: > if the test fails, this will be stuck forever. Run this loop only N times, for > your choice of N Changed to just throw out anything that looks like a ChromeDriver response. An improved temporary hack. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/pyautol... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2802: frame_xpath=''): On 2012/02/29 03:22:08, Nirnimesh wrote: > align under self Done. That's the danger of renaming with a regular expression. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2821: #TODO(craigdh): Add documentation for raising an event once it has been On 2012/02/29 03:22:08, Nirnimesh wrote: > need space after # Done. https://chromiumcodereview.appspot.com/9372120/diff/21001/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2848: self.large_test_timeout_ms(). On 2012/02/29 03:22:08, Nirnimesh wrote: > this is too large. default to the regular timeout. self.action_max_timeout_ms() > > Also, it turns out if you use timeout=-1 above, self._GetResultFromJSONRequest() > will use self.action_max_timeout_ms() anyway > > (pretty much what you said) Done.
Made the CompareEventId class private to AutomationEventQueue.
lgtm
https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... File chrome/browser/automation/automation_event_observer.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/automation_event_observer.cc:7: remove blank line https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/automation_event_observer.cc:8: #include "base/logging.h" should go before line 5 https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/automation_event_queue.cc:9: why the blank line? https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.h (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/automation_event_queue.h:68: scoped_ptr<AutomationJSONReply> wait_automation_reply_; Add comment describing what these are for https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:2319: handler_map["AddDomRaisedEventObserver"] = See corresponding comment in pyauto.py https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6454: void TestingAutomationProvider::AddDomRaisedEventObserver( you pass a lot of other args from the python side, but you don't use them. tab_index, windex, frame_xpath https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6490: if (!automation_event_queue_.get()) { The if in line 6486 implies that this if will never succeed. Merge it with the if in line 6482, and add a mesg like: "did you call AddDomRaisedEventObserver?" https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6500: AutomationJSONReply reply(this, reply_message); remove https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6502: automation_event_queue_->Clear(); Also free up automation_event_queue_? https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6503: reply.SendSuccess(NULL); AutomationJSONReply reply(this, reply_message).SendSuccess(NULL); https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6522: reply->SendError("No observers have been created."); Add: "Did you call AddDomRaisedEventObserver?" https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6526: automation_event_queue_->GetNextEvent(reply.release(), observer_id, blocking); AutomationEventQueue::GetNextEvent takes AutomationJSONReply* as the 1st arg, not scoped_ptr<AutomationJSONReply>. When will reply be freed? https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/test/pyautol... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2831: windex=windex)['observer_id'] You've added AddDomRaisedEventObserver to handler_map in testing_automation_provider.cc in which case you should not pass windex. If you pass windex, add it to browser_handler_map. Not both. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2848: self.large_test_timeout_ms(). fix comment. defaults to automation timeout
https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... File chrome/browser/automation/automation_event_observer.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/automation_event_observer.cc:7: On 2012/03/01 10:34:12, Nirnimesh wrote: > remove blank line Done. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/automation_event_observer.cc:8: #include "base/logging.h" On 2012/03/01 10:34:12, Nirnimesh wrote: > should go before line 5 Done. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... File chrome/browser/automation/automation_event_queue.h (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/automation_event_queue.h:68: scoped_ptr<AutomationJSONReply> wait_automation_reply_; On 2012/03/01 10:34:12, Nirnimesh wrote: > Add comment describing what these are for Done. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... File chrome/browser/automation/testing_automation_provider.cc (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6454: void TestingAutomationProvider::AddDomRaisedEventObserver( On 2012/03/01 10:34:12, Nirnimesh wrote: > you pass a lot of other args from the python side, but you don't use them. > tab_index, windex, frame_xpath Those are left over from some point in the design when this function was injecting Javascript. Fixed. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6490: if (!automation_event_queue_.get()) { On 2012/03/01 10:34:12, Nirnimesh wrote: > The if in line 6486 implies that this if will never succeed. Merge it with the > if in line 6482, and add a mesg like: "did you call AddDomRaisedEventObserver?" Done. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6500: AutomationJSONReply reply(this, reply_message); On 2012/03/01 10:34:12, Nirnimesh wrote: > remove Done. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6502: automation_event_queue_->Clear(); On 2012/03/01 10:34:12, Nirnimesh wrote: > Also free up automation_event_queue_? Changed to just automation_event_queue_.reset() which will delete the object if it exists and the destructor will call Clear(). https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6503: reply.SendSuccess(NULL); On 2012/03/01 10:34:12, Nirnimesh wrote: > AutomationJSONReply reply(this, reply_message).SendSuccess(NULL); Done. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6522: reply->SendError("No observers have been created."); On 2012/03/01 10:34:12, Nirnimesh wrote: > Add: "Did you call AddDomRaisedEventObserver?" Done. Added a comment to the same effect but a little more generic so it doesn't have to be changed when more observer types are added. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/browser/auto... chrome/browser/automation/testing_automation_provider.cc:6526: automation_event_queue_->GetNextEvent(reply.release(), observer_id, blocking); On 2012/03/01 10:34:12, Nirnimesh wrote: > AutomationEventQueue::GetNextEvent takes AutomationJSONReply* as the 1st arg, > not scoped_ptr<AutomationJSONReply>. > > When will reply be freed? When a matching event is added to the queue and the reply is used to complete the automation call. Added a comment saying this. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/test/pyautol... File chrome/test/pyautolib/pyauto.py (right): https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2831: windex=windex)['observer_id'] On 2012/03/01 10:34:12, Nirnimesh wrote: > You've added AddDomRaisedEventObserver to handler_map in > testing_automation_provider.cc in which case you should not pass windex. > > If you pass windex, add it to browser_handler_map. > > Not both. Fixed. This was left over from a time when creating a DomRaisedEventObserver injected Javascript. https://chromiumcodereview.appspot.com/9372120/diff/33002/chrome/test/pyautol... chrome/test/pyautolib/pyauto.py:2848: self.large_test_timeout_ms(). On 2012/03/01 10:34:12, Nirnimesh wrote: > fix comment. defaults to automation timeout Done.
LGTM. Let's roll.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/craigdh@chromium.org/9372120/37003
Change committed as 125069 |
