TestRunner library classes should be marked as NonCopyable.

TestRunner library classes should be marked as NonCopyable.

Mark the TestRunner library classes as NonCopyable. This is the first patch in this direction, where we are deriving the classes from WebKit::WebNonCopyable (defined as a base class in public/platform/WebNonCopyable.h). Next patch would sync public/platform/WebNonCopyable.h with wtf/Noncopyable.h and define WebNonCopyable as a macro and correct all classes defined in public/* which are deriving from WebNonCopyable class to rather use the macro.

BUG=229178
TEST=None; No behavior change; No breakge with webkit_unit_tests, content_unittests, layout_tests.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157884

[r.kasibhatla, 2013-09-12 06:30:03]

PTAL. Thanks

[jochen (gone - plz use gerrit), 2013-09-12 09:45:37]

lgtm

[tonyg, 2013-09-12 16:51:18]

I'm not a good reviewer for this change. Any chance you meant to add tony@
instead of tonyg@?

[tony, 2013-09-12 17:26:15]

LGTM too.

[commit-bot: I haz the power, 2013-09-12 19:25:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/23440025/2001

[commit-bot: I haz the power, 2013-09-12 21:25:07]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[r.kasibhatla, 2013-09-13 13:45:26]

On 2013/09/12 21:25:07, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_blink_rel for step(s) webkit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

I will upload the patch correcting the failing layout tests by tomorrow. I was
held up today with another patch.

[r.kasibhatla, 2013-09-16 11:16:20]

On 2013/09/13 13:45:26, r.kasibhatla wrote:
> On 2013/09/12 21:25:07, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_blink_rel for step(s) webkit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
> 
> I will upload the patch correcting the failing layout tests by tomorrow. I was
> held up today with another patch.
@jochen,@tony:
Updated the patch for fixing the failing layout test cases. PTAL.

[jochen (gone - plz use gerrit), 2013-09-16 11:25:39]

On 2013/09/16 11:16:20, r.kasibhatla wrote:
> On 2013/09/13 13:45:26, r.kasibhatla wrote:
> > On 2013/09/12 21:25:07, I haz the power (commit-bot) wrote:
> > > Retried try job too often on linux_blink_rel for step(s) webkit_tests
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
> > 
> > I will upload the patch correcting the failing layout tests by tomorrow. I
was
> > held up today with another patch.
> @jochen,@tony:
> Updated the patch for fixing the failing layout test cases. PTAL.

seems like the only difference is that CppVariant's dtor is non-virtual?

Can you explain how this made the layout tests fail?

[r.kasibhatla, 2013-09-16 11:40:00]

On 2013/09/16 11:25:39, jochen wrote:
> On 2013/09/16 11:16:20, r.kasibhatla wrote:
> > On 2013/09/13 13:45:26, r.kasibhatla wrote:
> > > On 2013/09/12 21:25:07, I haz the power (commit-bot) wrote:
> > > > Retried try job too often on linux_blink_rel for step(s) webkit_tests
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
> > > 
> > > I will upload the patch correcting the failing layout tests by tomorrow. I
> was
> > > held up today with another patch.
> > @jochen,@tony:
> > Updated the patch for fixing the failing layout test cases. PTAL.
> 
> seems like the only difference is that CppVariant's dtor is non-virtual?
> 
> Can you explain how this made the layout tests fail?
Frankly, I haven't been to figure it myself. My suspicion was more around
deriving from WebNonCopyable itself - which might be a problem if internally the
CppVariant object was copied using operator= or copy constructor. But the
problem is just the dtor being virtual. Ideally, virtual dtor shouldn't cause
any problems for CppVariant object at all (APMK), but I might be wrong and some
c++ expert might help me out.

[jochen (gone - plz use gerrit), 2013-09-16 13:00:56]

ok

Since WebNonCopyable isn't virtual, we shouldn't add virtual to the dtors

Can you remove the virtual keywords you added to the other classes as well?

[r.kasibhatla, 2013-09-16 15:19:23]

On 2013/09/16 13:00:56, jochen wrote:
> ok
> 
> Since WebNonCopyable isn't virtual, we shouldn't add virtual to the dtors
> 
> Can you remove the virtual keywords you added to the other classes as well?
Done. 
Sorry for adding virtual for dtors in the earlier patch. Didn't check properly
that WebNonCopyable was not virtual. :)

[jochen (gone - plz use gerrit), 2013-09-16 15:21:57]

lgtm

[commit-bot: I haz the power, 2013-09-16 15:33:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/23440025/31001

[commit-bot: I haz the power, 2013-09-16 16:54:51]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_layout.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[r.kasibhatla, 2013-09-16 17:54:37]

On 2013/09/16 16:54:51, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on win_layout.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
> Your code is likely broken or HEAD is junk. Please ensure your
> code is not broken then alert the build sheriffs.
> Look at the try server FAQ for more details.

@jochen, @tony: I don't have windows setup, so couldn't check the patch on
windows. Is win_layout bots build different from win_blink_rel bot builds,
because the patch is only failing for win_layout. 
The build error says WebNonCopyable is not dll export class, hence we cannot
make it base for WebTaskList. Can you suggest me what needs to be done to fix
the win_layout build? Should I simply revoke the inheriting changes for
WebTaskList class?

[r.kasibhatla, 2013-09-16 19:25:16]

On 2013/09/16 17:54:37, r.kasibhatla wrote:
> On 2013/09/16 16:54:51, I haz the power (commit-bot) wrote:
> > Sorry for I got bad news for ya.
> > Compile failed with a clobber build on win_layout.
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
> > Your code is likely broken or HEAD is junk. Please ensure your
> > code is not broken then alert the build sheriffs.
> > Look at the try server FAQ for more details.
> 
> @jochen, @tony: I don't have windows setup, so couldn't check the patch on
> windows. Is win_layout bots build different from win_blink_rel bot builds,
> because the patch is only failing for win_layout. 
> The build error says WebNonCopyable is not dll export class, hence we cannot
> make it base for WebTaskList. Can you suggest me what needs to be done to fix
> the win_layout build? Should I simply revoke the inheriting changes for
> WebTaskList class?
@tony, @jochen: Uploaded a new patch to fix the build failure of win_layout bots
also. PTAL.

In this patch, I have reverted couple of classes to their original state, since
they were marked as  WEBTESTRUNNER_EXPORT classes and I couldn't derive them
from WebNonCopyable which is not marked as WEBTESTRUNNER_EXPORT. I thought of
marking WebNonCopyable itself as WEBTESTRUNNER_EXPORT class, but felt it is
wrong since WebNonCopyable is not an export type class. Hence, choose to do the
latter and for now skip the inheritance. In fact, for WebTestProxyBase, I have
added FIXME also, since it marks noncopyable independently by itself by making
its operator= & copy constructor private.

My idea is to get all these classes to same platform in the follow up patch when
we sync WebNonCopyable with wtf/Noncopyable.h and make WebNonCopyable as a macro
rather than a class. The missing classes (from this patch) can be taken into
non-copyable ambit in that patch. Hope my approach & thoughts are ok. Please let
me know if otherwise.

[r.kasibhatla, 2013-09-16 19:27:30]

On 2013/09/16 19:25:16, r.kasibhatla wrote:
> On 2013/09/16 17:54:37, r.kasibhatla wrote:
> > On 2013/09/16 16:54:51, I haz the power (commit-bot) wrote:
> > > Sorry for I got bad news for ya.
> > > Compile failed with a clobber build on win_layout.
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...
> > > Your code is likely broken or HEAD is junk. Please ensure your
> > > code is not broken then alert the build sheriffs.
> > > Look at the try server FAQ for more details.
> > 
> > @jochen, @tony: I don't have windows setup, so couldn't check the patch on
> > windows. Is win_layout bots build different from win_blink_rel bot builds,
> > because the patch is only failing for win_layout. 
> > The build error says WebNonCopyable is not dll export class, hence we cannot
> > make it base for WebTaskList. Can you suggest me what needs to be done to
fix
> > the win_layout build? Should I simply revoke the inheriting changes for
> > WebTaskList class?
> @tony, @jochen: Uploaded a new patch to fix the build failure of win_layout
bots
> also. PTAL.
> 
> In this patch, I have reverted couple of classes to their original state,
since
> they were marked as  WEBTESTRUNNER_EXPORT classes and I couldn't derive them
> from WebNonCopyable which is not marked as WEBTESTRUNNER_EXPORT. I thought of
> marking WebNonCopyable itself as WEBTESTRUNNER_EXPORT class, but felt it is
> wrong since WebNonCopyable is not an export type class. Hence, choose to do
the
> latter and for now skip the inheritance. In fact, for WebTestProxyBase, I have
> added FIXME also, since it marks noncopyable independently by itself by making
> its operator= & copy constructor private.
> 
> My idea is to get all these classes to same platform in the follow up patch
when
> we sync WebNonCopyable with wtf/Noncopyable.h and make WebNonCopyable as a
macro
> rather than a class. The missing classes (from this patch) can be taken into
> non-copyable ambit in that patch. Hope my approach & thoughts are ok. Please
let
> me know if otherwise.
I am not sure of the reason, but the diff between patchset# 3 & patchset# 4 is
shown wrong here. I guess it might be due to fact that I uploaded these 2
patchsets from diff machines. But if you check the patchset# 4 in total, then
the changes are shown correctly (as they should be).

[tony, 2013-09-16 19:39:29]

LGTM

https://codereview.chromium.org/23440025/diff/52001/public/testing/WebTestPro...
File public/testing/WebTestProxy.h (right):

https://codereview.chromium.org/23440025/diff/52001/public/testing/WebTestPro...
public/testing/WebTestProxy.h:272: // as Noncopyable.
Nit: You might want to link to a bug URL here.

[r.kasibhatla, 2013-09-17 01:37:50]

On 2013/09/16 19:39:29, tony wrote:
> LGTM
> 
>
https://codereview.chromium.org/23440025/diff/52001/public/testing/WebTestPro...
> File public/testing/WebTestProxy.h (right):
> 
>
https://codereview.chromium.org/23440025/diff/52001/public/testing/WebTestPro...
> public/testing/WebTestProxy.h:272: // as Noncopyable.
> Nit: You might want to link to a bug URL here.

Done. Added the bug url in FIXME.

[commit-bot: I haz the power, 2013-09-17 05:33:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/23440025/7001

[commit-bot: I haz the power, 2013-09-17 07:03:05]

Change committed as 157884