Add WebWidgetTestProxy.

Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when
running LayoutTests and wraps RenderWidget in order to execute
test-specific code.

This is a reland of https://codereview.chromium.org/2174663005/.

BUG=583347
Committed: https://crrev.com/1568d113829186f7b1e7a96adbcac2b3a5818056
Cr-Commit-Position: refs/heads/master@{#415308}

[lfg, 2016-08-10 19:24:59]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 19:26:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-08-10 21:38:19]

Patchset #2 (id:20001) has been deleted

[lfg, 2016-08-10 21:39:10]

The CQ bit was unchecked by lfg@chromium.org

[lfg, 2016-08-11 15:40:39]

lfg@chromium.org changed reviewers:
+ lukasza@chromium.org

[lfg, 2016-08-11 15:40:40]

+lukasza, please take a look. PS1 is what I had landed before, and PS2 are the
changes needed to fix the issue with site-per-process.

[Łukasz Anforowicz, 2016-08-11 17:19:28]

Thanks - LGTM with minor nits / suggestions.

https://codereview.chromium.org/2238573002/diff/40001/content/renderer/render...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2238573002/diff/40001/content/renderer/render...
content/renderer/render_widget.h:139: using RenderWidgetCreatedCallback = void
(*)(RenderWidget*);
nit: Would it be more accurate to call this RenderWidgetInitedCallback? (or
"Initialized" if "Inited" is not a word :-P).

https://codereview.chromium.org/2238573002/diff/40001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2238573002/diff/40001/content/test/layouttest...
content/test/layouttest_support.cc:120: if (!(g_widget_test_proxy_callback ==
0)) {
nit: I found the code for this condition a bit weird.  OTOH, now I realize that
LazyInstance only exposes ==, so one has to write |!(x != 0)| and cannot write
|x != 0| or |x|.  Still, let me propose 2 alternatives:

Alternative #1: !(g_widget_test_proxy_callback == nullptr)

Alternative #2: !g_widget_test_proxy_callback.Get().is_null()  // This "creates"
the LazyInstance, but checks if the callback it wraps was "set".

[lfg, 2016-08-11 19:49:39]

Please, take another look.

https://codereview.chromium.org/2238573002/diff/40001/content/renderer/render...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/2238573002/diff/40001/content/renderer/render...
content/renderer/render_widget.h:139: using RenderWidgetCreatedCallback = void
(*)(RenderWidget*);
On 2016/08/11 17:19:28, Łukasz Anforowicz wrote:
> nit: Would it be more accurate to call this RenderWidgetInitedCallback? (or
> "Initialized" if "Inited" is not a word :-P).

Done.

https://codereview.chromium.org/2238573002/diff/40001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2238573002/diff/40001/content/test/layouttest...
content/test/layouttest_support.cc:120: if (!(g_widget_test_proxy_callback ==
0)) {
On 2016/08/11 17:19:28, Łukasz Anforowicz wrote:
> nit: I found the code for this condition a bit weird.  OTOH, now I realize
that
> LazyInstance only exposes ==, so one has to write |!(x != 0)| and cannot write
> |x != 0| or |x|.  Still, let me propose 2 alternatives:
> 
> Alternative #1: !(g_widget_test_proxy_callback == nullptr)
> 
> Alternative #2: !g_widget_test_proxy_callback.Get().is_null()  // This
"creates"
> the LazyInstance, but checks if the callback it wraps was "set".

I agree that this isn't very nice. Looking at this code, I can't see any reason
to be using base::Callback/base::LazyInstance here. I switched to regular
function pointers, which simplifies the code and makes it more readable.

[Łukasz Anforowicz, 2016-08-11 19:58:45]

Still LGTM.  I agree that the code using function pointers does look better and
simpler - thanks for making this change.

[lfg, 2016-08-11 20:11:15]

lfg@chromium.org changed reviewers:
+ jochen@chromium.org

[lfg, 2016-08-11 20:11:16]

+jochen, please take a look.

[lfg, 2016-08-11 20:16:12]

lfg@chromium.org changed reviewers:
+ jam@chromium.org

[lfg, 2016-08-11 20:16:13]

+jam, please take a look (jochen@ is OOO).

[jam, 2016-08-11 21:24:52]

On 2016/08/11 20:16:13, lfg wrote:
> +jam, please take a look (jochen@ is OOO).

What do you want me to look at?

you made changes to components/test_runner but I don't see an owner from there.
please pick one as specific owners should be used (i.e. I'm not really familiar
with the components code)

[lfg, 2016-08-11 22:02:36]

On 2016/08/11 21:24:52, jam wrote:
> On 2016/08/11 20:16:13, lfg wrote:
> > +jam, please take a look (jochen@ is OOO).
> 
> What do you want me to look at?
> 
> you made changes to components/test_runner but I don't see an owner from
there.
> please pick one as specific owners should be used (i.e. I'm not really
familiar
> with the components code)

This was already reviewed in https://codereview.chromium.org/2174663005/ , but I
had to revert the change due to an issue with the linux_site_isolation bots.
There
were no changes in the test_runner after that, only in content (PS1 was the
original
landed CL).

I'm just looking for an owners review of the changes in content and a
rubberstamp
for the other files.

[jam, 2016-08-15 20:07:44]

https://codereview.chromium.org/2238573002/diff/40001/content/test/layouttest...
File content/test/layouttest_support.cc (right):

https://codereview.chromium.org/2238573002/diff/40001/content/test/layouttest...
content/test/layouttest_support.cc:120: if (!(g_widget_test_proxy_callback ==
0)) {
On 2016/08/11 19:49:39, lfg wrote:
> On 2016/08/11 17:19:28, Łukasz Anforowicz wrote:
> > nit: I found the code for this condition a bit weird.  OTOH, now I realize
> that
> > LazyInstance only exposes ==, so one has to write |!(x != 0)| and cannot
write
> > |x != 0| or |x|.  Still, let me propose 2 alternatives:
> > 
> > Alternative #1: !(g_widget_test_proxy_callback == nullptr)
> > 
> > Alternative #2: !g_widget_test_proxy_callback.Get().is_null()  // This
> "creates"
> > the LazyInstance, but checks if the callback it wraps was "set".
> 
> I agree that this isn't very nice. Looking at this code, I can't see any
reason
> to be using base::Callback/base::LazyInstance here. I switched to regular
> function pointers, which simplifies the code and makes it more readable.

if (!g_widget_test_proxy_callback.Get().is_null()))?

seems fine.

base::Callback is the standard way of holding on to method calls, so switching
to C function pointers seems unnecessary

[lfg, 2016-08-15 21:57:12]

On 2016/08/15 20:07:44, jam wrote:
>
https://codereview.chromium.org/2238573002/diff/40001/content/test/layouttest...
> File content/test/layouttest_support.cc (right):
> 
>
https://codereview.chromium.org/2238573002/diff/40001/content/test/layouttest...
> content/test/layouttest_support.cc:120: if (!(g_widget_test_proxy_callback ==
> 0)) {
> On 2016/08/11 19:49:39, lfg wrote:
> > On 2016/08/11 17:19:28, Łukasz Anforowicz wrote:
> > > nit: I found the code for this condition a bit weird.  OTOH, now I realize
> > that
> > > LazyInstance only exposes ==, so one has to write |!(x != 0)| and cannot
> write
> > > |x != 0| or |x|.  Still, let me propose 2 alternatives:
> > > 
> > > Alternative #1: !(g_widget_test_proxy_callback == nullptr)
> > > 
> > > Alternative #2: !g_widget_test_proxy_callback.Get().is_null()  // This
> > "creates"
> > > the LazyInstance, but checks if the callback it wraps was "set".
> > 
> > I agree that this isn't very nice. Looking at this code, I can't see any
> reason
> > to be using base::Callback/base::LazyInstance here. I switched to regular
> > function pointers, which simplifies the code and makes it more readable.
> 
> if (!g_widget_test_proxy_callback.Get().is_null()))?
> 
> seems fine.
> 
> base::Callback is the standard way of holding on to method calls, so switching
> to C function pointers seems unnecessary

I'm ok with it either way. Should I also change the existing uses in
RenderViewImpl/RenderFrameImpl:
https://cs.chromium.org/chromium/src/content/renderer/render_view_impl.cc?rcl...
?

[lfg, 2016-08-16 21:25:33]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 21:25:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 21:29:59]

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

[commit-bot: I haz the power, 2016-08-16 21:30:00]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lfg, 2016-08-16 22:31:57]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 22:32:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 22:36:49]

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

[commit-bot: I haz the power, 2016-08-16 22:36:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lfg, 2016-08-16 22:53:54]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 22:54:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 01:09:52]

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

[commit-bot: I haz the power, 2016-08-17 01:09:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lfg, 2016-08-17 17:04:10]

On 2016/08/15 21:57:12, lfg wrote:
> On 2016/08/15 20:07:44, jam wrote:
> > if (!g_widget_test_proxy_callback.Get().is_null()))?
> > 
> > seems fine.
> > 
> > base::Callback is the standard way of holding on to method calls, so
switching
> > to C function pointers seems unnecessary

I switched back to base::Callback, please take another look.

[jochen (gone - plz use gerrit), 2016-08-23 12:54:54]

deferring to jam@ happy to rubberstamp once he approves

[lfg, 2016-08-26 15:13:29]

jam: ping

[jam, 2016-08-29 21:20:58]

lgtm

[jochen (gone - plz use gerrit), 2016-08-30 12:13:45]

lgtm

[lfg, 2016-08-30 13:18:38]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-08-30 13:18:38]

The patchset sent to the CQ was uploaded after l-g-t-m from lukasza@chromium.org
Link to the patchset: https://codereview.chromium.org/2238573002/#ps120001
(title: "add missing files")

[commit-bot: I haz the power, 2016-08-30 13:18:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-30 16:06:59]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-08-30 16:10:10]

Description was changed from

==========
Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when
running LayoutTests and wraps RenderWidget in order to execute
test-specific code.

This is a reland of https://codereview.chromium.org/2174663005/.

BUG=583347
==========

to

==========
Add WebWidgetTestProxy.

This patch adds a WebWidgetTestProxy interface that is instantiated when
running LayoutTests and wraps RenderWidget in order to execute
test-specific code.

This is a reland of https://codereview.chromium.org/2174663005/.

BUG=583347

Committed: https://crrev.com/1568d113829186f7b1e7a96adbcac2b3a5818056
Cr-Commit-Position: refs/heads/master@{#415308}
==========

[commit-bot: I haz the power, 2016-08-30 16:10:12]

Patchset 6 (id:??) landed as
https://crrev.com/1568d113829186f7b1e7a96adbcac2b3a5818056
Cr-Commit-Position: refs/heads/master@{#415308}