[ChromeDriver] Added CommandListener interface

[ChromeDriver] Added CommandListener interface

Added CommandListener interface and updated Session implementation to use it. When WebDriver commands are issued, CommandListener::OnCommand is called. This is a first step toward tracing support in ChromeDriver. PerformanceLogger will need OnCommand in order to request trace logs at appropriate points (e.g. when "GetLog" command is issued).

BUG=

[johnmoore, 2014-06-13 20:19:51]

The proposed design for ChromeDriver tracing support (comments welcome) is
outlined in this doc:
https://docs.google.com/a/google.com/document/d/1n8cISVs87uHHVW_kbX2tzHrwzhgZ...

This CL is the first step toward implementation of the proposed design.

[stgao, 2014-06-15 23:54:41]

Besides "GetLog", are there any other commands you'd like to whitelisted?

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/command_listener.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/command_listener.cc:10: Status
CommandListener::OnCommand(const std::string& command_name) {
I think this default implementation file could be removed, if we don't create
any instance of CommandListener.

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/command_listener.h (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/command_listener.h:19: virtual Status OnCommand(const
std::string& command_name);
How about renaming "OnCommand" to "BeforeCommand"?

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/command_listener.h:19: virtual Status OnCommand(const
std::string& command_name);
add " = 0" before ";"?

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/commands_unittest.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/commands_unittest.cc:629: MockCommandListener* listener
= new MockCommandListener();
Memory leak here.
You could use a scoped_ptr, just like linked_ptr above.

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session.h (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session.h:40: struct Session : public CommandListener {
Composite pattern might not be good here.
We'd better keep Session as close to a plain object as possible.
How about removing the inheritance of CommandListener here?
You could move logic in Session::OnCommand to
ExecuteSessionCommandOnSessionThread or chrome/test/chromedriver/util.cc/h

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_unittest.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_unittest.cc:35: class MockCommandListener :
public CommandListener {
Duplicate code as the one in commands_unittest.cc

You could create a stub_command_listener.h/cc.
In the constructor, you could pass in a cmd_name that will be notified, instead
of a hard-code one.

[stgao, 2014-06-15 23:57:38]

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_unittest.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_unittest.cc:130: MockCommandListener* listener1
= new MockCommandListener();
Memory leak: please use a scoped_ptr.

[klm, 2014-06-16 16:11:29]

Thanks Shuotao for the first round!

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/command_listener.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/command_listener.cc:10: Status
CommandListener::OnCommand(const std::string& command_name) {
On 2014/06/15 23:54:40, Shuotao wrote:
> I think this default implementation file could be removed, if we don't create
> any instance of CommandListener.

+1 -- pure virtual, just like other interfaces, e.g. there is no web_view.cc.
The virtual destructor body should be inline in the header.

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/commands.cc:155: Status status =
session->OnCommand(std::string(command_name));
I am concerned that this is only done on session commands -- from under
ExecuteSessionCommand, but not from Execute{Window,Element,Alert}Command, unless
those go through ExecuteSessionCommand, which I don't see that they would,
although then I'm not sure how the window/element/alert commands end up running
on the session thread... See http_handler.cc for the top level command dispatch
logic.

The listener should get invoked on all commands, not just session, and then it's
up to the listener implementation to decide what to do on what commands.

[johnmoore, 2014-06-16 17:28:34]

Thanks Shuotao and Michael for the initial review. I'll address most of your
concerns with a new patch set soon. I've followed up with two of your comments
for now.

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/commands.cc:155: Status status =
session->OnCommand(std::string(command_name));
On 2014/06/16 16:11:29, klm wrote:
> I am concerned that this is only done on session commands -- from under
> ExecuteSessionCommand, but not from Execute{Window,Element,Alert}Command,
unless
> those go through ExecuteSessionCommand, which I don't see that they would,
> although then I'm not sure how the window/element/alert commands end up
running
> on the session thread... See http_handler.cc for the top level command
dispatch
> logic.
> 
> The listener should get invoked on all commands, not just session, and then
it's
> up to the listener implementation to decide what to do on what commands.


I believe that the window and element commands do go through
ExecuteSessionCommand due to overloading of HttpHandler::WrapToCommand. I
checked to make sure OnCommand would be triggered for "Navigate" (window
command) and "FindElement" (element command). I have now confirmed the same for
"GetAlertMessage" (alert command); it is triggered as well.

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_unittest.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_unittest.cc:35: class MockCommandListener :
public CommandListener {
On 2014/06/15 23:54:41, Shuotao wrote:
> Duplicate code as the one in commands_unittest.cc
> 
> You could create a stub_command_listener.h/cc.
> In the constructor, you could pass in a cmd_name that will be notified,
instead
> of a hard-code one.

Would it make sense for the stub class to perform the ASSERTs too? It seems that
would make it more of a mock class than a stub class. If I had to define a
MockCommandListener that inherits from StubCommandListener in both unittest .cc
files, there would be just as much duplicated code, plus the separate stub .h
and .cc files. I'm not sure what the best practice would be here.

[johnmoore, 2014-06-16 20:49:18]

This patch set addresses all concerns except that of the duplicate
MockCommandListener code.

Please see my comments on the design doc for an explanation of why the
CommandListener interface is needed (i.e. why we will dump logs on certain
WebDriver commands).

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
File chrome/test/chromedriver/commands_unittest.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/com...
chrome/test/chromedriver/commands_unittest.cc:629: MockCommandListener* listener
= new MockCommandListener();
On 2014/06/15 23:54:40, Shuotao wrote:
> Memory leak here.
> You could use a scoped_ptr, just like linked_ptr above.

(See reply to other memory leak concern)

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_unittest.cc (right):

https://codereview.chromium.org/331943003/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_unittest.cc:130: MockCommandListener* listener1
= new MockCommandListener();
On 2014/06/15 23:57:38, Shuotao wrote:
> Memory leak: please use a scoped_ptr.

session.AddListener should take care of transferring ownership to session, which
then destroys the listeners if I am not mistaken

[johnmoore, 2014-06-20 18:47:53]

Hi all,

In working on a follow-up CL to this one, I've realized that the current
structure will cause a dependency check failure in the presubmit process on
future related CLs. Please see my comment below for more info. We should resolve
this issue in this CL since it affects the next one.

https://codereview.chromium.org/331943003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/command_listener.h (right):

https://codereview.chromium.org/331943003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/command_listener.h:5: #ifndef
CHROME_TEST_CHROMEDRIVER_COMMAND_LISTENER_H_
I've realized that anything in chromedriver/chrome/ that includes
chromedriver/command_listener.h will fail the pre-submit dependency checks since
chromedriver/chrome/ is supposed to be independent of everything in
chromedriver/.

Question for Shuotao (all comments welcome): Should I move command_listener.h
into chromedriver/chrome/, or should I make an exception in the dependency
checks?

[stgao, 2014-06-20 22:36:49]

Hi John,

I replied your question first.
Will do a detailed review afterwards.

Thanks,
Shuotao Gao

https://codereview.chromium.org/331943003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/command_listener.h (right):

https://codereview.chromium.org/331943003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/command_listener.h:5: #ifndef
CHROME_TEST_CHROMEDRIVER_COMMAND_LISTENER_H_
On 2014/06/20 18:47:53, johnmoore wrote:
> I've realized that anything in chromedriver/chrome/ that includes
> chromedriver/command_listener.h will fail the pre-submit dependency checks
since
> chromedriver/chrome/ is supposed to be independent of everything in
> chromedriver/.
> 
> Question for Shuotao (all comments welcome): Should I move command_listener.h
> into chromedriver/chrome/, or should I make an exception in the dependency
> checks?

We'd better still separate chromedriver/chrome/ entirely from chromedriver/.

chromedriver/chrome/ is an abstraction of Chrome and it should know nothing
about webdriver.

But why do you want CommandListener to be aware of by chromedriver/chrome?
Is it because of PerformanceLogger?
If so, let's move PerformanceLogger out to /chromedriver.

[johnmoore, 2014-06-23 15:11:09]

As per Shuotao's recommendation, I have moved PerformanceLogger to
chromedriver/. Now it can properly include CommandListener, which is located in
chromedriver/ as well.