[ChromeDriver] Expose CPU Profiling DevTools API into chromeWebDriver

[ChromeDriver] Expose CPU Profiling DevTools API into chromeWebDriver

BUG=chromedriver:796
https://code.google.com/p/chromedriver/issues/detail?id=796

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

[klm, 2014-05-10 02:21:56]

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/cpu_profile.cc (right):

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.cc:14: client_->AddListener(this);
Why do we need to be a listener at all, if our OnEvent is a no-op?

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.cc:24: if (statusDebug.IsError())
Please check the status right after sending the command, otherwise a failure for
Profiler.enable may go unhandled if both fail.

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.cc:26: else if
(statusProfiler.IsError())
SendCommand("Debugger.disable") here, to try to clean up on failure as much as
we can?

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.cc:37: if (statusDebug.IsError())
Same comments as in InitProfileInternal.

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.cc:45: Status
CpuProfile::StartProfile(scoped_ptr<base::Value>* result) {
result is unused. Seems to be an artifact of how this is structured in
window_commands.cc, but bottom line is this method conceptually has no return
value, so IMHO it should take no arg, as well as its WebView counterpart, and
window_commands.cc would simply not pass its "value" var -- it remains unchanged
anyway.

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.cc:49: if (statusInit.IsError())
Call StopProfileInternal() here, to clean up?

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.cc:71: const std::string& method,
indentation

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.cc:73: return Status(kOk);
indentation

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/cpu_profile.h (right):

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.h:24: class CpuProfile: public
DevToolsEventListener {
I believe the C++ style wants a space before the ":". I.e. HeapSnapshotTaker may
be a violation too.

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.h:29: Status InitProfileInternal();
These should be private.

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/cpu_profile.h:40: Status CpuProfileInternal();
This method seems to have no implementation defined.

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
File chrome/test/chromedriver/chrome/web_view.h (right):

https://codereview.chromium.org/275193003/diff/1/chrome/test/chromedriver/chr...
chrome/test/chromedriver/chrome/web_view.h:158: virtual Status
StartProfile(scoped_ptr<base::Value>* snapshot) = 0;
Please add a descriptive comment similar to TakeHeapSnapshot.

[stgao, 2014-05-14 04:12:31]

Hi Diego,

Many thanks for the patch.
I added some comments.

Next time you upload a new patch set, you could click the "Publish+Mail Comments
('m')" on the left-top corner and we will receive notification email.

Thanks,
Shuotao Gao

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/cpu_profile.cc (right):

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:20: Status statusDebug =
client_->SendCommand("Debugger.enable", params);
For name convention, |statusDebug| should be |status_debug|.
Please also update other variable names in this patch.

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:20: Status statusDebug =
client_->SendCommand("Debugger.enable", params);
For Debugger.enable, it seems we still need it until Chrome 36 stable is
released.

http://crbug.com/260749 was fixed on blink in r171857
(http://src.chromium.org/viewvc/blink?view=revision&revision=171857), which is
rolled into chromium in r264619
(http://src.chromium.org/viewvc/chrome?view=revision&revision=264619).

Do you mind adding a comment for this with a TODO to remove Debugger.enable?

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:23: return statusDebug;
As it is only one line, we usually remove the "{}".

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:29:
client_->SendCommand("Debugger.disable", params);
Do you still need the debugger enabled even after Profiler.enable is executed?
Do you mind trying it out?

I prefer Debugger.enable to be disabled if possible as it generates a lot of
DevTools events for Javascript execution.

If you turn on verbose logging, you will see those events.

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:30: return statusProfiler;
"{}" is needed for this if clause.

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:51: base::DictionaryValue params;
Move this line down to just before it is used (line 57).

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:63: Status status =
client_->SendCommandAndGetResult(
This status is not checked. It may fail already.

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:68: return disableProfileStatus;
Remove "{}".

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

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.h:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
nit: 2013 -> 2014, same for cpu_profile.cc

It seems your email is not in
https://code.google.com/p/chromium/codesearch#chromium/src/AUTHORS

Please add your email to that file.

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.h:23: class CpuProfile : public
DevToolsEventListener {
It seems we don't the OnEvent function here.
So we don't have to use a DevToolsEventListener.
It would be better to integrate code of cpu_profile.h/cc into web_view_impl.cc

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.h:29: Status
EndProfile(scoped_ptr<base::Value>* snapshot);
maybe rename |snapshot| to |profile_data| or something?
Same for other occurrences.

[klm, 2014-05-14 05:23:10]

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/cpu_profile.cc (right):

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:23: return statusDebug;
On 2014/05/14 04:12:31, Shuotao wrote:
> As it is only one line, we usually remove the "{}".

The C++ style guide allows curly brackets for single statements:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...

See also the "goto fail" Apple SSL bug for a good reason to "always always"
require it:)

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:30: return statusProfiler;
On 2014/05/14 04:12:31, Shuotao wrote:
> "{}" is needed for this if clause.

Exactly. It's overall less error prone to "always always require {}", not even
think about it. Code changes all the time, and it's very easy to forget to add
{} where it was previously omitted.

[elfogris, 2014-05-14 22:27:23]

Hey guys, I did all the changes based on the latest comments. 
Per klm, I added braces to all if statements.
Per Shuotao I renamed the variables and removed some unneeded code.
I try to switch of the debugging after activating the profile and it does not
work so keeping it as it was.
So let me know if you find something else.

I have a question: I had to manually add cpu_profile.o to the
automation_client_lib.ninja, this should be done automatically when building the
hole chromium target right?

Thanks.

[klm, 2014-05-15 04:59:29]

On 2014/05/14 22:27:23, elfogris wrote:
> Hey guys, I did all the changes based on the latest comments. 
> Per klm, I added braces to all if statements.
> Per Shuotao I renamed the variables and removed some unneeded code.
> I try to switch of the debugging after activating the profile and it does not
> work so keeping it as it was.
> So let me know if you find something else.

I see no remaining issues, but I defer to Shuotao who knows the logic and
conventions of chromedriver a lot better than I do.

> 
> I have a question: I had to manually add cpu_profile.o to the
> automation_client_lib.ninja, this should be done automatically when building
the
> hole chromium target right?

You must update the gypi file where all these sources are enumerated, please add
it to this change, otherwise it would indeed fail to build.

> 
> Thanks.

[stgao, 2014-05-16 01:43:21]

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/cpu_profile.cc (right):

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:20: Status statusDebug =
client_->SendCommand("Debugger.enable", params);
On 2014/05/14 04:12:31, Shuotao wrote:
> For Debugger.enable, it seems we still need it until Chrome 36 stable is
> released.
> 
> http://crbug.com/260749 was fixed on blink in r171857
> (http://src.chromium.org/viewvc/blink?view=revision&revision=171857), which is
> rolled into chromium in r264619
> (http://src.chromium.org/viewvc/chrome?view=revision&revision=264619).
> 
> Do you mind adding a comment for this with a TODO to remove Debugger.enable?

Would you mind adding a comment about this with a brief description into the
code? Otherwise, we might forget to remove Debugger.enable after Chrome 36 is
released.

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:23: return statusDebug;
On 2014/05/14 05:23:11, klm wrote:
> On 2014/05/14 04:12:31, Shuotao wrote:
> > As it is only one line, we usually remove the "{}".
> 
> The C++ style guide allows curly brackets for single statements:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...
> 
> See also the "goto fail" Apple SSL bug for a good reason to "always always"
> require it:)

Yes, the C++ style does allow.
Personally, I also like curly brackets for each if/else/for/while as I used to
do in Java.

However, sorry, let us follow the convention here.

First, in chromedriver, we keep the convention of "without curly brackets" for a
single-line statement from the very beginning when we developing version 0.1.
Also in chromium, I spent a few minutes going through 10+ random *.cc files, and
I can't find an example of "*with* curly brackets" for a single-line statement.

https://codereview.chromium.org/275193003/diff/20001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:30: return statusProfiler;
On 2014/05/14 05:23:11, klm wrote:
> On 2014/05/14 04:12:31, Shuotao wrote:
> > "{}" is needed for this if clause.
> 
> Exactly. It's overall less error prone to "always always require {}", not even
> think about it. Code changes all the time, and it's very easy to forget to add
> {} where it was previously omitted.

See my comment above about this.

https://codereview.chromium.org/275193003/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/cpu_profile.cc (right):

https://codereview.chromium.org/275193003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.cc:61: Status
CpuProfile::EndProfile(scoped_ptr<base::Value>* result) {
How about using the same variable name (either |result| or |profile_data|)
instead |profile_data|, |result| and |snapshot| in three different files?

https://codereview.chromium.org/275193003/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/cpu_profile.h (right):

https://codereview.chromium.org/275193003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.h:12: #include
"chrome/test/chromedriver/chrome/devtools_event_listener.h"
This is unused.

https://codereview.chromium.org/275193003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/cpu_profile.h:35: DevToolsClient* client_;
This variable is unused.

Do you mind folding cpu_profile.h/cc into web_view_impl.cc?
We don't have much code here and it seems not necessary to have a separate class
for this feature.

The reason we need a separate class for HeapSnapshotTaker is because we have to
make it as a listener to receive DevTools events which contain result data.

After the folding, you don't have to worry about the compile error or adding
these two new files to the gypi file.

https://codereview.chromium.org/275193003/diff/40001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/web_view_impl.h (right):

https://codereview.chromium.org/275193003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/web_view_impl.h:93: virtual Status
EndProfile(scoped_ptr<base::Value>* snapshot) OVERRIDE;
|snapshot| -> |profile_data|?

https://codereview.chromium.org/275193003/diff/40001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/web_view_impl.h:112: scoped_ptr<CpuProfile>
cpu_profile_;
After folding cpu_profile.h/cc into web_view_impl.cc, this will become unused.

[elfogris, 2014-05-16 17:08:38]

Changes done.

Sorry guys for this back and forth, I know I made newbie mistakes, but I'm not
used to work in C++, neither familiar with your code guidelines. So bear with me
:) Hopefully we all done or almost done!

Thanks!

[stgao, 2014-05-16 20:36:14]

Hi Diego,

Many thanks for the new patch and your patience on the code style. I'm not
picky, but just want the style to be consistent.

I posted three small comments.
Everything else looks good to me.

Thanks,
Shuotao Gao

https://codereview.chromium.org/275193003/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/web_view_impl.cc (right):

https://codereview.chromium.org/275193003/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/web_view_impl.cc:435:
client_->SendCommand("Debugger.disable", params);
Maybe also check whether this one is successful.

https://codereview.chromium.org/275193003/diff/60001/chrome/test/chromedriver...
File chrome/test/chromedriver/chrome/web_view_impl.h (right):

https://codereview.chromium.org/275193003/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/web_view_impl.h:28: class CpuProfile;
Unused. Could be removed.

https://codereview.chromium.org/275193003/diff/60001/chrome/test/chromedriver...
chrome/test/chromedriver/chrome/web_view_impl.h:105: virtual Status
InitProfileInternal();
How about moving these two methods into the default namespace in
web_view_impl.cc just like the method GetContextIdForFrame there?

Otherwise, "virtual" before them needs deleting because they are private methods
and no sublcass could override them.

[elfogris, 2014-05-16 21:27:32]

Done

[stgao, 2014-05-16 21:56:25]

lgtm

cool, lgtm, going for CQ

[stgao, 2014-05-16 22:18:08]

The CQ bit was unchecked by stgao@chromium.org

[stgao, 2014-05-16 22:19:30]

The CQ bit was checked by stgao@chromium.org

[commit-bot: I haz the power, 2014-05-16 23:34:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/elfogris@gmail.com/275193003/80001

[commit-bot: I haz the power, 2014-05-17 05:40:40]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)

[commit-bot: I haz the power, 2014-05-17 05:45:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-17 05:45:11]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[elfogris, 2014-05-17 07:09:39]

On 2014/05/17 05:45:11, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_chromium_gn_compile_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
>   chromium_presubmit on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
>   linux_chromium_chromeos_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
>   linux_chromium_clang_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
>   linux_chromium_gn_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)


What went wrong? Can't make sense of all this logging.

[klm, 2014-05-17 15:01:58]

The CQ bit was checked by klm@google.com

[commit-bot: I haz the power, 2014-05-17 15:02:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/elfogris@gmail.com/275193003/80001

[commit-bot: I haz the power, 2014-05-17 19:07:11]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-17 19:10:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-17 19:10:12]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[klm, 2014-05-17 20:14:02]

The CQ bit was checked by klm@google.com

[commit-bot: I haz the power, 2014-05-17 20:14:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/elfogris@gmail.com/275193003/80001

[commit-bot: I haz the power, 2014-05-17 21:46:47]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-17 21:49:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-17 21:49:30]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[klm, 2014-05-18 03:25:12]

The CQ bit was checked by klm@google.com

[commit-bot: I haz the power, 2014-05-18 03:25:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/elfogris@gmail.com/275193003/80001

[commit-bot: I haz the power, 2014-05-18 05:10:23]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-18 05:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-18 05:11:59]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[klm, 2014-05-18 19:46:29]

The CQ bit was checked by klm@google.com

[commit-bot: I haz the power, 2014-05-18 19:46:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/elfogris@gmail.com/275193003/80001

[commit-bot: I haz the power, 2014-05-18 23:17:51]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-18 23:19:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-18 23:19:11]

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)

[klm, 2014-05-18 23:47:12]

Diego, please sync to head and upload another patch.

  Applied patch chrome/test/chromedriver/chrome/web_view_impl.cc cleanly.

Failed to apply patch for chrome/test/chromedriver/chrome/web_view_impl.h:
While running git apply --index -p1 --verbose;
  Checking patch chrome/test/chromedriver/chrome/web_view_impl.h...
  Hunk #1 succeeded at 89 (offset 1 line).
  error: while searching for:
                                     scoped_ptr<base::Value>* result);
    Status IsNotPendingNavigation(const std::string& frame_id,
                                  bool* is_not_pending);
    std::string id_;
    int build_no_;
    scoped_ptr<DomTracker> dom_tracker_;
  
  error: patch failed: chrome/test/chromedriver/chrome/web_view_impl.h:98
  error: chrome/test/chromedriver/chrome/web_view_impl.h: patch does not apply

On 2014/05/18 23:19:11, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_chromium_compile_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)

[elfogris, 2014-05-19 05:07:21]

Done.

[klm, 2014-05-19 12:27:24]

The CQ bit was checked by klm@google.com

[commit-bot: I haz the power, 2014-05-19 12:27:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/elfogris@gmail.com/275193003/100001

[commit-bot: I haz the power, 2014-05-19 14:24:35]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-19 15:02:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-19 15:02:19]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[klm, 2014-05-19 15:05:26]

Diego, please fix commands_unittest.cc: you added pure virtuals to the WebView,
and the test needs to override them all.

http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...

[klm, 2014-05-19 15:16:49]

On 2014/05/19 15:05:26, klm wrote:
> Diego, please fix commands_unittest.cc: you added pure virtuals to the
WebView,
> and the test needs to override them all.
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...

Also please actually run the tests:) -- see the .gypi file for the _tests and/or
_unittests target.

[stgao, 2014-05-19 17:39:21]

Sorry for my late reply.

Yes, chrome/test/chromedriver/chrome/stub_web_view.h and its .cc need
updating for the newly-added virtual methods.

Diego, please compile chromedriver_unittests and chromedriver_tests as you
did for chromedriver binary and make sure they are happy with the change :)

Thanks,
Shuotao Gao


On Mon, May 19, 2014 at 8:16 AM, <klm@google.com> wrote:

> On 2014/05/19 15:05:26, klm wrote:
>
>> Diego, please fix commands_unittest.cc: you added pure virtuals to the
>>
> WebView,
>
>> and the test needs to override them all.
>>
>
>
> http://build.chromium.org/p/tryserver.chromium/builders/
> linux_chromium_chromeos_rel/builds/28400/steps/compile%20%
> 28with%20patch%2C%20lkcr%2C%20clobber%2C%20nuke%29/logs/stdio
>
> Also please actually run the tests:) -- see the .gypi file for the _tests
> and/or
> _unittests target.
>
> https://codereview.chromium.org/275193003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[elfogris, 2014-05-20 06:40:41]

I added the definition for StartProfile and EndProfile, but Im getting this
error compiling:

Undefined symbols for architecture i386:
  "StubWebView::EndProfile(scoped_ptr<base::Value,
base::DefaultDeleter<base::Value> >*)", referenced from:
      vtable for StubWebView in chromedriver_unittests.stub_web_view.o
      vtable for (anonymous namespace)::ErrorCallFunctionWebView in
chromedriver_unittests.commands_unittest.o
      vtable for (anonymous namespace)::FindElementWebView in
chromedriver_unittests.commands_unittest.o
ld: symbol(s) not found for architecture i386

I have no idea why the signature is changed and where the DefaultDeleter is
coming from... Probably this will be trivial for you guys, but I have not
clue what is wrong here... Hints?


2014-05-19 10:39 GMT-07:00 Shuotao Gao <stgao@chromium.org>:

> Sorry for my late reply.
>
> Yes, chrome/test/chromedriver/chrome/stub_web_view.h and its .cc need
> updating for the newly-added virtual methods.
>
> Diego, please compile chromedriver_unittests and chromedriver_tests as you
> did for chromedriver binary and make sure they are happy with the change :)
>
> Thanks,
> Shuotao Gao
>
>
> On Mon, May 19, 2014 at 8:16 AM, <klm@google.com> wrote:
>
>> On 2014/05/19 15:05:26, klm wrote:
>>
>>> Diego, please fix commands_unittest.cc: you added pure virtuals to the
>>>
>> WebView,
>>
>>> and the test needs to override them all.
>>>
>>
>>
>> http://build.chromium.org/p/tryserver.chromium/builders/
>> linux_chromium_chromeos_rel/builds/28400/steps/compile%20%
>> 28with%20patch%2C%20lkcr%2C%20clobber%2C%20nuke%29/logs/stdio
>>
>> Also please actually run the tests:) -- see the .gypi file for the _tests
>> and/or
>> _unittests target.
>>
>> https://codereview.chromium.org/275193003/
>>
>
>


-- 
Diego Ferreiro Val
@diervo

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-20 07:58:06]

Can you upload the patch with the actual unittest mods?

Michael
Sent from my mobile device

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[elfogris, 2014-05-20 16:51:27]

On 2014/05/20 07:58:06, chromium-reviews_chromium.org wrote:
> Can you upload the patch with the actual unittest mods?
> 
> Michael
> Sent from my mobile device
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Done

[klm, 2014-05-20 16:52:52]

https://codereview.chromium.org/275193003/diff/120001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/stub_web_view.cc (right):

https://codereview.chromium.org/275193003/diff/120001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/stub_web_view.cc:139: Status
EndProfile(scoped_ptr<base::Value>* profile_data) {
Missing StubWebView::, that's why you're getting a link error -- it's not
defined as a member function at all.

[elfogris, 2014-05-20 17:13:00]

On 2014/05/20 16:52:52, klm wrote:
>
https://codereview.chromium.org/275193003/diff/120001/chrome/test/chromedrive...
> File chrome/test/chromedriver/chrome/stub_web_view.cc (right):
> 
>
https://codereview.chromium.org/275193003/diff/120001/chrome/test/chromedrive...
> chrome/test/chromedriver/chrome/stub_web_view.cc:139: Status
> EndProfile(scoped_ptr<base::Value>* profile_data) {
> Missing StubWebView::, that's why you're getting a link error -- it's not
> defined as a member function at all.

The poor C++ newb strikes again :)
All good now, and uploaded, hopefully now will pass.

Thanks for the patience

[klm, 2014-05-20 17:16:45]

The CQ bit was checked by klm@google.com

[commit-bot: I haz the power, 2014-05-20 17:17:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/elfogris@gmail.com/275193003/130001

[commit-bot: I haz the power, 2014-05-20 20:24:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-20 21:26:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 21:26:32]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[klm, 2014-05-20 22:57:58]

The CQ bit was checked by klm@google.com

[commit-bot: I haz the power, 2014-05-20 22:59:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/elfogris@gmail.com/275193003/130001

[commit-bot: I haz the power, 2014-05-21 01:51:12]

Change committed as 271803