Logging API in chromedriver2.

Logging API in chromedriver2.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195198

[klm, 2013-04-15 21:00:30]

Ken, you'll likely want to reshuffle things, let me know what your preferences
are. I wanted to have an end to end thing running, that way it's easier to see
how everything fits together and what the options are.

I was also thinking of splitting the DevToolsEventLogger into a more formal base
and adding DevToolsConsoleLogger that produces more human-friendly console
messages, as discussed in another thread.

Java program that I used to test with an actual browser:

import java.io.File;
import java.io.FileWriter;
import java.net.URL;
import java.util.List;
import java.util.Set;
import java.util.logging.Level;

import org.json.JSONArray;
import org.json.JSONObject;
import org.openqa.selenium.WebDriver;
import org.openqa.selenium.logging.LogEntry;
import org.openqa.selenium.logging.LoggingPreferences;
import org.openqa.selenium.logging.Logs;
import org.openqa.selenium.remote.CapabilityType;
import org.openqa.selenium.remote.DesiredCapabilities;
import org.openqa.selenium.remote.RemoteWebDriver;

public class Test {

  public void myTest() throws Exception {
    DesiredCapabilities caps = DesiredCapabilities.chrome();
    LoggingPreferences logPrefs = new LoggingPreferences();
    logPrefs.enable("performance", Level.INFO);
    caps.setCapability(CapabilityType.LOGGING_PREFS, logPrefs);

    WebDriver driver = new RemoteWebDriver(new URL("http://localhost:9515/"),
caps);

    List<LogEntry> perfEntries;
    try {
      driver.get("http://www.google.com");
      Logs logs = driver.manage().logs();
      Set<String> types = logs.getAvailableLogTypes();
      System.out.println("Log types: " + types);
      perfEntries = logs.get("performance").getAll();
    } finally {
      driver.quit();
    }

    if (null != perfEntries) {
      JSONArray devToolsLog = new JSONArray();
      System.out.println(perfEntries.size() + " performance log entries found");
      for (LogEntry entry : perfEntries) {
        JSONObject message = new JSONObject(entry.getMessage());
        JSONObject devToolsMessage = message.getJSONObject("message");
        System.out.println(devToolsMessage.getString("method") + " webview " +
message.getString("webview"));
        devToolsLog.put(devToolsMessage);
      }
      FileWriter writer = new FileWriter("devtools.log");
      try {
        devToolsLog.write(writer);
      } finally {
        writer.close();
      }
    } else {
      System.err.println("Performance log not found");
    }
  }

  public static void main(String[] argv) throws Exception {
    new Test().myTest();
  }
}

[klm, 2013-04-16 00:29:24]

Ken, I would actually appreciate even partial comments especially if they
are about refactoring the code. It would be awesome to have this checked in
by EOW - GTAC.
Thanks.
Michael
Sent from my mobile device

[kkania, 2013-04-16 22:51:30]

Just a few high-level comments:
-revert changes to webdriver_server.cc (which I suppose were accidental)
-add your devtools_event_logger files to this CL, right?
-refactor a bit:
I want to keep knowledge of the DevTools stuff to stay inside the Chrome layer.
You can just move the DevToolsEventLogger into chrome/. You can still create the
DevToolsEventLogger in the same place. Also, instead of adding
Chrome::AddEventLogger (which logs an error if there's already webviews), pass
std::vector<DevToolsEventLogger*> to the ChromeImpl constructor.

Sorry for the delay. Once you make these changes go ahead and ping me and I'll
give you a more in depth review.


------------------------------------------------------------
Also, FYI, if you later want to do a patch for console, I'd propose, instead of
inheriting from DevToolsEventLogger, do:
class DevToolsPerfLogger : DTEL {
  DevToolsPerfLogger(Log* perf_log) : log_(perf_log) {}
  Log* log_;
}
class DevToolsConsoleLogger : DTEL {
  DevToolsConsoleLogger(Log* console_log) : log_(console_log) {}
  Log* log_;
}
class Log {
  Log(LogLevel min_level)
  LogLevel GetLevel();
  void Log(LogLevel level, const std::string& message);
  void Log(const base::Time& time, LogLevel level, const std::string& message); 
// if you need it...
  void GetAndClearLogs(...);
}
Then, you'd change ChromeImpl to take:
ChromeImpl(Log* perf_log, Log* console_log, ...) and ChromeImpl would create and
own the DevToolsLoggers.
Session would have a ScopedVector<Log>. This is a bit more complicated, but
makes it a bit cleaner for your console stuff and for when we add "driver" and
"browser" log types. You can already do some of this refactoring now if you
want, but it's not required.

[klm, 2013-04-17 05:18:01]

On 2013/04/16 22:51:30, kkania wrote:
> Just a few high-level comments:
> -revert changes to webdriver_server.cc (which I suppose were accidental)
> -add your devtools_event_logger files to this CL, right?

Woops yes, thanks, learning my chromium CL management...:)

> -refactor a bit:
> I want to keep knowledge of the DevTools stuff to stay inside the Chrome
layer.
> You can just move the DevToolsEventLogger into chrome/. You can still create
the

It's already under chrome/.

> DevToolsEventLogger in the same place. Also, instead of adding
> Chrome::AddEventLogger (which logs an error if there's already webviews), pass
> std::vector<DevToolsEventLogger*> to the ChromeImpl constructor.

Weeelll... Then I'll need to drag it through LaunchChrome, ChromeDesktopImpl,
ChromeAndroidImpl, and anything else that inherits ChromeImpl in the future,
while none of these places really care about loggers. The way it is now, it has
a more limited footprint.

If Chrome+ChromeImpl could manage DevToolsEventListeners -- make sure they get
added to any new DevToolsClients + call OnConnected (what
DevToolsEventLogger::AddDevToolsClient() does now), then I wouldn't even need to
have Chrome know about the logger concept at all, loggers would stay confined to
the Session where they belong. The code in Chrome+Impl would be very similar to
what I wrote for logger management -- an add method, a vector, and a couple of
lines on client creation. I know you don't expose the concept of DevTools from
Chrome right now, but it might be worth considering -- the logger example
illustrates how that may enable looser coupling.

Another potential option is to follow the discipline where WebView implements
and therefore knows about most WD functionality, so add logger management API to
WebView+WebViewImpl instead of Chrome+ChromeImpl. To hook it up to the session,
introduce the concept of a WebViewListener that listens for WebView creation &
death, registered via Chrome::AddWebViewListener() -- again with the Impl code
similar to what I have now. The session would implement a WebViewListener that
adds all its loggers to each new WebView. WebViewImpl would continue to
encapsulate the DevToolsClient and would call DTEL::AddDevToolsClient when we
add a DTEL to the WebView. I don't know if it's such a big deal to compile-time
restrict adding loggers to the WebView (or Chrome) at construction time. I think
loggers are specific enough that there would be no temptation to abuse the Add
method, and a runtime check is sufficient. Also *even if* a logger is added
later, it seems the worst that would happen is it would miss some events,
nothing would crash or get into an inconsistent state. Not adding loggers in the
constructor saves us from having to drag them through many places that have no
need to know about them.

> 
> Sorry for the delay. Once you make these changes go ahead and ping me and I'll
> give you a more in depth review.
> 
> 
> ------------------------------------------------------------
> Also, FYI, if you later want to do a patch for console, I'd propose, instead
of
> inheriting from DevToolsEventLogger, do:
> class DevToolsPerfLogger : DTEL {
>   DevToolsPerfLogger(Log* perf_log) : log_(perf_log) {}
>   Log* log_;
> }
> class DevToolsConsoleLogger : DTEL {
>   DevToolsConsoleLogger(Log* console_log) : log_(console_log) {}
>   Log* log_;
> }

Yes I was thinking about a common base class, perhaps with GetLogEntry left pure
virtual for the children to override. IsOurMethod can be pure virtual with the
current implementation as a utility function separate from the class, or could
be left in the base. Console would use it + additionally filter for just the
"Console.messageAdded" events and their log level.

> class Log {
>   Log(LogLevel min_level)
>   LogLevel GetLevel();

In the wire protocol the level is always a string, only Java translates it to
java.util.logging.Level constants. BTW do we need GetLevel() on the Log
publicly? My understanding of the WD API is the LoggingPrefs specify min level
at which to collect the message, and anything lower is safe to discard right
away. The GetAvailableLogTypes and GetLog methods don't pass the level, we are
supposed to return everything that we had collected, unconditionally. The Java
client has pure client-side level filtering, but the server doesn't know about
it. OTOH we could introduce a LogLevel class that encapsulates string level
comparison and validation -- that would be useful for capabilities parsing and
for console log min level filtering.

An implicit assumption in the WD API is that the server is forwarding log
messages that have a level intrinsically -- like JS console or browser logs.
Performance logs have no such intrinsic concept. Therefore I fudged it a bit --
I repurposed the LoggingPrefs level as *the* level for all performance logs
messages. I could potentially just ignore the pref level and hardcode the level
for all performance entries. I don't think it's a big deal, but mentioning just
so you're aware of this subtle difference.

>   void Log(LogLevel level, const std::string& message);
>   void Log(const base::Time& time, LogLevel level, const std::string&
message); 
> // if you need it...
>   void GetAndClearLogs(...);
> }
> Then, you'd change ChromeImpl to take:
> ChromeImpl(Log* perf_log, Log* console_log, ...) and ChromeImpl would create
and
> own the DevToolsLoggers.
> Session would have a ScopedVector<Log>. This is a bit more complicated, but
> makes it a bit cleaner for your console stuff and for when we add "driver" and
> "browser" log types. You can already do some of this refactoring now if you
> want, but it's not required.

Yes I was also thinking about this, but I'd say let's hold off until its time
comes -- the specifics could look differently then. Even if I do it now, I kind
of doubt that it would remain untouched when you add driver and browser log
types:) Also if/when I add dynamic enable/disable to the WD Logging API
including the wire protocol (if the elders bless it), the Session would need
better access than just the Log data sink object -- it would need to ultimately
reach the DevToolsClients, as I had implemented initially (and prematurely,
because the API isn't there yet). So I'm not yet convinced that this is the best
way to separate it.

[kkania, 2013-04-17 17:03:06]

On 2013/04/17 05:18:01, klm wrote:
> On 2013/04/16 22:51:30, kkania wrote:
> > Just a few high-level comments:
> > -revert changes to webdriver_server.cc (which I suppose were accidental)
> > -add your devtools_event_logger files to this CL, right?
> 
> Woops yes, thanks, learning my chromium CL management...:)
> 
> > -refactor a bit:
> > I want to keep knowledge of the DevTools stuff to stay inside the Chrome
> layer.
> > You can just move the DevToolsEventLogger into chrome/. You can still create
> the
> 
> It's already under chrome/.
> 
> > DevToolsEventLogger in the same place. Also, instead of adding
> > Chrome::AddEventLogger (which logs an error if there's already webviews),
pass
> > std::vector<DevToolsEventLogger*> to the ChromeImpl constructor.
> 
> Weeelll... Then I'll need to drag it through LaunchChrome, ChromeDesktopImpl,
> ChromeAndroidImpl, and anything else that inherits ChromeImpl in the future,
> while none of these places really care about loggers. The way it is now, it
has
> a more limited footprint.

Correct. I know the way you have now is simpler and easier. However, the Chrome
layer is responsible for controlling Chrome remotely via DevTools. The WebDriver
layer on top is supposed to use this layer for accomplishing WebDriver stuff.
Following this model, I think it makes sense to include managing the browser
log, the devtools messages log, and your performance logs in the Chrome layer.
This layer split sometimes requires more coding, but I think the layer model
overall makes things simpler to understand.

> 
> If Chrome+ChromeImpl could manage DevToolsEventListeners -- make sure they get
> added to any new DevToolsClients + call OnConnected (what
> DevToolsEventLogger::AddDevToolsClient() does now), then I wouldn't even need
to
> have Chrome know about the logger concept at all, loggers would stay confined
to
> the Session where they belong. The code in Chrome+Impl would be very similar
to
> what I wrote for logger management -- an add method, a vector, and a couple of
> lines on client creation. I know you don't expose the concept of DevTools from
> Chrome right now, but it might be worth considering -- the logger example
> illustrates how that may enable looser coupling.

True. However, I think the logger concept should be a Chrome concept, not a
WebDriver concept.

> 
> Another potential option is to follow the discipline where WebView implements
> and therefore knows about most WD functionality, so add logger management API
to
> WebView+WebViewImpl instead of Chrome+ChromeImpl. To hook it up to the
session,
> introduce the concept of a WebViewListener that listens for WebView creation &
> death, registered via Chrome::AddWebViewListener() -- again with the Impl code
> similar to what I have now. The session would implement a WebViewListener that
> adds all its loggers to each new WebView. WebViewImpl would continue to
> encapsulate the DevToolsClient and would call DTEL::AddDevToolsClient when we
> add a DTEL to the WebView. I don't know if it's such a big deal to
compile-time
> restrict adding loggers to the WebView (or Chrome) at construction time. I
think
> loggers are specific enough that there would be no temptation to abuse the Add
> method, and a runtime check is sufficient. Also *even if* a logger is added
> later, it seems the worst that would happen is it would miss some events,
> nothing would crash or get into an inconsistent state. Not adding loggers in
the
> constructor saves us from having to drag them through many places that have no
> need to know about them.

WebView is not supposed to know about WebDriver. It just provides an interface
to control a RenderView through DevTools. I have considered adding a
WebViewListener before, but I don't think this is necessary now.

About the runtime log error, it'd be nice for most if not all errors to be
reported back to the user and not just put in logs. There's a couple other
places where we LOG errors, and we have a bug open to fix those. Right now *even
if* a logger is added later, it just miss all events from already existing
clients, right? I think if we were to keep the same approach, the Add function
should add the listener to all current clients too.

> 
> > 
> > Sorry for the delay. Once you make these changes go ahead and ping me and
I'll
> > give you a more in depth review.
> > 
> > 
> > ------------------------------------------------------------
> > Also, FYI, if you later want to do a patch for console, I'd propose, instead
> of
> > inheriting from DevToolsEventLogger, do:
> > class DevToolsPerfLogger : DTEL {
> >   DevToolsPerfLogger(Log* perf_log) : log_(perf_log) {}
> >   Log* log_;
> > }
> > class DevToolsConsoleLogger : DTEL {
> >   DevToolsConsoleLogger(Log* console_log) : log_(console_log) {}
> >   Log* log_;
> > }
> 
> Yes I was thinking about a common base class, perhaps with GetLogEntry left
pure
> virtual for the children to override. IsOurMethod can be pure virtual with the
> current implementation as a utility function separate from the class, or could
> be left in the base. Console would use it + additionally filter for just the
> "Console.messageAdded" events and their log level.

Whoops, I was a bit unclear here. DTEL refers to DevToolsEventListener, not
DevToolsEventLogger. I would prefer composition over inheritance. The only thing
they really need to share is the ability to store the log entries somewhere.
It'd be nice to generalize this to the Log class which can be used elsewhere.

> 
> > class Log {
> >   Log(LogLevel min_level)
> >   LogLevel GetLevel();
> 
> In the wire protocol the level is always a string, only Java translates it to
> java.util.logging.Level constants. BTW do we need GetLevel() on the Log
> publicly? My understanding of the WD API is the LoggingPrefs specify min level
> at which to collect the message, and anything lower is safe to discard right
> away. The GetAvailableLogTypes and GetLog methods don't pass the level, we are
> supposed to return everything that we had collected, unconditionally. The Java
> client has pure client-side level filtering, but the server doesn't know about
> it. OTOH we could introduce a LogLevel class that encapsulates string level
> comparison and validation -- that would be useful for capabilities parsing and
> for console log min level filtering.

I see, thanks for the info. As you say, I think log entries below the minimum
specified level should be discarded immediately. I was thinking the Log class
would just drop such entries. I imagined the GetLevel method might be a simple
way for a logger to know if it needs to do something special do enable
higher-detail logging. About LogLevel, I think your idea is fine.

> 
> An implicit assumption in the WD API is that the server is forwarding log
> messages that have a level intrinsically -- like JS console or browser logs.
> Performance logs have no such intrinsic concept. Therefore I fudged it a bit
--
> I repurposed the LoggingPrefs level as *the* level for all performance logs
> messages. I could potentially just ignore the pref level and hardcode the
level
> for all performance entries. I don't think it's a big deal, but mentioning
just
> so you're aware of this subtle difference.
> 
> >   void Log(LogLevel level, const std::string& message);
> >   void Log(const base::Time& time, LogLevel level, const std::string&
> message); 
> > // if you need it...
> >   void GetAndClearLogs(...);
> > }
> > Then, you'd change ChromeImpl to take:
> > ChromeImpl(Log* perf_log, Log* console_log, ...) and ChromeImpl would create
> and
> > own the DevToolsLoggers.
> > Session would have a ScopedVector<Log>. This is a bit more complicated, but
> > makes it a bit cleaner for your console stuff and for when we add "driver"
and
> > "browser" log types. You can already do some of this refactoring now if you
> > want, but it's not required.
> 
> Yes I was also thinking about this, but I'd say let's hold off until its time
> comes -- the specifics could look differently then. Even if I do it now, I
kind
> of doubt that it would remain untouched when you add driver and browser log
> types:) Also if/when I add dynamic enable/disable to the WD Logging API
> including the wire protocol (if the elders bless it), the Session would need
> better access than just the Log data sink object -- it would need to
ultimately
> reach the DevToolsClients, as I had implemented initially (and prematurely,
> because the API isn't there yet). So I'm not yet convinced that this is the
best
> way to separate it.

I'm ok punting this for this CL. About your concern on how this would work if we
add the ability to configure log levels dynamically, we could add a LogListener
{ void OnLogLevelChanged(...); } or something like
Chrome::UpdateLoggingPrefs(...);



------
A lot of words, but my main concern is that we make the Chrome layer responsible
for interacting with DevTools and keep the WebDriver layer about WebDriver
stuff. There may be times where we need to break this model, but I don't think
this is one of them.

[klm, 2013-04-18 01:08:31]

PTAL

[kkania, 2013-04-18 17:34:39]

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/chrome_impl.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/chrome_impl.cc:63: Status
add_status((*logger)->AddDevToolsClient(client.get()));
get rid of AddDevToolsClient, and just do client->AddListener(*logger); i think
this should allow you to get rid of the error below too

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/chrome_impl.h (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/chrome_impl.h:18: class DevToolsEventLogger;
whoops, out of order

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/devtools_event_logger.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:46: return "";  // Page
domain is always enabled for core features.
Just return Page.enable. I'd prefer to call Page.enable again. I don't want to
depend on internals of other code.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:77: bool
DevToolsEventLogger::IsOurMethod(const std::string& method) {
rename to ShouldLogEvent

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:96: // I (klm) have
benchmarked potential implementations, with 100000 iterations:
change this to a one line summary comment about perf

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:119:
log_entry_dict->SetDouble("timestamp", (int64_t)timestamp_epoch_ms);
static_cast<int64>, include base/basictypes.h, remove stdint.h

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:133: // TODO(klm):
C++11: std::move(log_entry_json).
remove

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/devtools_event_logger.h (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.h:18: // Tracks all
WebViews of a given session, via their respective DevToolsClients.
Chrome doesn't know anything about sessions. Update comment

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.h:38: Status
AddDevToolsClient(DevToolsClient* client);
remove

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/devtools_event_logger_unittest.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger_unittest.cc:285:
TEST(DevToolsEventLogger, BenchmarkEventFunctions) {
remove all this benchmarking code

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:82: /* KLM REMOVE BEFORE SUBMISSION
whoops

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/l...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/l...
chrome/test/chromedriver/logging.cc:12: Status ParseLoggingPrefs(const
base::Value& option,
move this to the capabilities file

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/s...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/s...
chrome/test/chromedriver/session_commands.cc:481: if (params.size() != 0) {
remove this; we don't bother checking this anywhere else

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/s...
chrome/test/chromedriver/session_commands.cc:498: if (params.size() != 1) {
remove this

[klm, 2013-04-18 19:11:31]

Thanks! PTAL.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/chrome_impl.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/chrome_impl.cc:63: Status
add_status((*logger)->AddDevToolsClient(client.get()));
On 2013/04/18 17:34:39, kkania wrote:
> get rid of AddDevToolsClient, and just do client->AddListener(*logger); i
think
> this should allow you to get rid of the error below too

Done and noted that OnConnect gets called later -- that's what the source of the
Status was.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/chrome_impl.h (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/chrome_impl.h:18: class DevToolsEventLogger;
On 2013/04/18 17:34:39, kkania wrote:
> whoops, out of order

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/devtools_event_logger.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:46: return "";  // Page
domain is always enabled for core features.
On 2013/04/18 17:34:39, kkania wrote:
> Just return Page.enable. I'd prefer to call Page.enable again. I don't want to
> depend on internals of other code.

Done. Hope it's idempotent:)

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:77: bool
DevToolsEventLogger::IsOurMethod(const std::string& method) {
On 2013/04/18 17:34:39, kkania wrote:
> rename to ShouldLogEvent

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:96: // I (klm) have
benchmarked potential implementations, with 100000 iterations:
On 2013/04/18 17:34:39, kkania wrote:
> change this to a one line summary comment about perf

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:119:
log_entry_dict->SetDouble("timestamp", (int64_t)timestamp_epoch_ms);
On 2013/04/18 17:34:39, kkania wrote:
> static_cast<int64>, include base/basictypes.h, remove stdint.h

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.cc:133: // TODO(klm):
C++11: std::move(log_entry_json).
On 2013/04/18 17:34:39, kkania wrote:
> remove

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/devtools_event_logger.h (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.h:18: // Tracks all
WebViews of a given session, via their respective DevToolsClients.
On 2013/04/18 17:34:39, kkania wrote:
> Chrome doesn't know anything about sessions. Update comment

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger.h:38: Status
AddDevToolsClient(DevToolsClient* client);
On 2013/04/18 17:34:39, kkania wrote:
> remove

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/chrome/devtools_event_logger_unittest.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/chrome/devtools_event_logger_unittest.cc:285:
TEST(DevToolsEventLogger, BenchmarkEventFunctions) {
On 2013/04/18 17:34:39, kkania wrote:
> remove all this benchmarking code

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/c...
chrome/test/chromedriver/commands.cc:82: /* KLM REMOVE BEFORE SUBMISSION
On 2013/04/18 17:34:39, kkania wrote:
> whoops

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/l...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/l...
chrome/test/chromedriver/logging.cc:12: Status ParseLoggingPrefs(const
base::Value& option,
On 2013/04/18 17:34:39, kkania wrote:
> move this to the capabilities file

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/s...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/s...
chrome/test/chromedriver/session_commands.cc:481: if (params.size() != 0) {
On 2013/04/18 17:34:39, kkania wrote:
> remove this; we don't bother checking this anywhere else

Done.

https://codereview.chromium.org/14263024/diff/3003/chrome/test/chromedriver/s...
chrome/test/chromedriver/session_commands.cc:498: if (params.size() != 1) {
On 2013/04/18 17:34:39, kkania wrote:
> remove this

Done.

[kkania, 2013-04-18 20:27:54]

lgtm

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities.cc:15: #include
"chrome/test/chromedriver/logging.h"
you can drop this now

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/commands.cc:4: 
#include <list>

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/logging.cc:11: 
remove newline

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/logging.cc:23: if ("performance" == type) {
i notice the java client has a "profiling" type; did you want to use that
instead?

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
File chrome/test/chromedriver/logging.h (right):

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/logging.h:9: #include <vector>
drop both

[klm, 2013-04-18 22:53:34]

Note how I changed the parsing call in capabilities.cc -- wasn't working with
Android.

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
File chrome/test/chromedriver/capabilities.cc (right):

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities.cc:15: #include
"chrome/test/chromedriver/logging.h"
On 2013/04/18 20:27:55, kkania wrote:
> you can drop this now

Done.

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/capabilities.cc:236: parser_map["loggingPrefs"] =
base::Bind(&ParseLoggingPrefs);
Notice how I changed the way I parse logging prefs -- we don't get this far when
IsAndroid()!

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
File chrome/test/chromedriver/commands.cc (right):

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/commands.cc:4: 
On 2013/04/18 20:27:55, kkania wrote:
> #include <list>

Done.

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/logging.cc:11: 
On 2013/04/18 20:27:55, kkania wrote:
> remove newline

Done.

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/logging.cc:23: if ("performance" == type) {
On 2013/04/18 20:27:55, kkania wrote:
> i notice the java client has a "profiling" type; did you want to use that
> instead?

Done. Meant to look that up and never did, thanks:)

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
File chrome/test/chromedriver/logging.h (right):

https://codereview.chromium.org/14263024/diff/46004/chrome/test/chromedriver/...
chrome/test/chromedriver/logging.h:9: #include <vector>
On 2013/04/18 20:27:55, kkania wrote:
> drop both

Done.

[commit-bot: I haz the power, 2013-04-19 03:19:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/klm@google.com/14263024/68001

[commit-bot: I haz the power, 2013-04-19 03:51:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/klm@google.com/14263024/71006

[commit-bot: I haz the power, 2013-04-19 03:53:02]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-04-19 03:56:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/klm@google.com/14263024/71006

[commit-bot: I haz the power, 2013-04-19 15:06:52]

Change committed as 195198