Split most of RenderViewTest and associated classes into content.

Split most of RenderViewTest and associated classes into content.

BUG=99224
TEST=existing browser_tests, unit_tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105511

[jam, 2011-10-13 17:35:32]

lgtm

http://codereview.chromium.org/8230034/diff/3041/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8230034/diff/3041/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:143: 'renderer/chrome_mock_render_thread.cc',
nit: ordering

http://codereview.chromium.org/8230034/diff/3041/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1882: 'test/base/chrome_render_view_test.cc',
nit: ordering

http://codereview.chromium.org/8230034/diff/3041/chrome/test/base/render_view...
File chrome/test/base/render_view_test.cc (left):

http://codereview.chromium.org/8230034/diff/3041/chrome/test/base/render_view...
chrome/test/base/render_view_test.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
weird how rietveld shows this as M instead of D

http://codereview.chromium.org/8230034/diff/3041/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/8230034/diff/3041/content/content_tests.gypi#n...
content/content_tests.gypi:61: 'test/render_view_test.cc',
nit: ordering

http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
File content/test/mock_render_thread.h (right):

http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
content/test/mock_render_thread.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
nit: it would be better if this file was svn moved so that it's easy to see what
got factored out of it, but no worries, i trust you did the write thing

http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
content/test/mock_render_thread.h:20: class MockRenderThread : public
content::RenderThread {
nit: add small comment?

http://codereview.chromium.org/8230034/diff/3041/content/test/render_view_test.h
File content/test/render_view_test.h (right):

http://codereview.chromium.org/8230034/diff/3041/content/test/render_view_tes...
content/test/render_view_test.h:41: class Message;
nit: not needed

[Jói, 2011-10-13 18:47:26]

Addressed review comments and merged to lkgr (there was a bit of manual merging
needed for r104975).

Will ask CQ to commit once local tests pass.

Cheers,
Jói

http://codereview.chromium.org/8230034/diff/3041/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8230034/diff/3041/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:143: 'renderer/chrome_mock_render_thread.cc',
On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
> nit: ordering

Done.

http://codereview.chromium.org/8230034/diff/3041/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:1882: 'test/base/chrome_render_view_test.cc',
On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
> nit: ordering

Done.

http://codereview.chromium.org/8230034/diff/3041/content/content_tests.gypi
File content/content_tests.gypi (right):

http://codereview.chromium.org/8230034/diff/3041/content/content_tests.gypi#n...
content/content_tests.gypi:61: 'test/render_view_test.cc',
On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
> nit: ordering

Done.

http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
File content/test/mock_render_thread.h (right):

http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
content/test/mock_render_thread.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
> nit: it would be better if this file was svn moved so that it's easy to see
what
> got factored out of it, but no worries, i trust you did the write thing

Yeah, sorry about that, it's hard to fix now but I think perhaps when this gets
committed to SVN it will get treated as a move (not sure).

http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
content/test/mock_render_thread.h:20: class MockRenderThread : public
content::RenderThread {
On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
> nit: add small comment?

Done.

http://codereview.chromium.org/8230034/diff/3041/content/test/render_view_test.h
File content/test/render_view_test.h (right):

http://codereview.chromium.org/8230034/diff/3041/content/test/render_view_tes...
content/test/render_view_test.h:41: class Message;
On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
> nit: not needed

Done.

[jam, 2011-10-13 19:03:44]

http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
File content/test/mock_render_thread.h (right):

http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
content/test/mock_render_thread.h:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2011/10/13 18:47:27, Jói wrote:
> On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
> > nit: it would be better if this file was svn moved so that it's easy to see
> what
> > got factored out of it, but no worries, i trust you did the write thing
> 
> Yeah, sorry about that, it's hard to fix now but I think perhaps when this
gets
> committed to SVN it will get treated as a move (not sure).

this won't get automatically fixed on commit, but it's actually easy to fix :)
the benefits would be we'd keep the history of that file, which is very useful
sometimes.

move content/test/mock_render_thread.h blah.h
svn revert chrome/renderer/mock_render_thread.h
content/test/mock_render_thread.h
svn mv chrome/renderer/mock_render_thread.h content/test/mock_render_thread.h
move blah.h content/test/mock_render_thread.h

[Jói, 2011-10-13 19:09:40]

I'm using git and I have lots of local changes in between, so it's a
bit more complicated, but I'll figure it out before I commit.

Cheers,
Jói


On Thu, Oct 13, 2011 at 12:03 PM,  <jam@chromium.org> wrote:
>
>
http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
> File content/test/mock_render_thread.h (right):
>
>
http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
> content/test/mock_render_thread.h:1: // Copyright (c) 2011 The Chromium
> Authors. All rights reserved.
> On 2011/10/13 18:47:27, Jói wrote:
>>
>> On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
>> > nit: it would be better if this file was svn moved so that it's easy
>
> to see
>>
>> what
>> > got factored out of it, but no worries, i trust you did the write
>
> thing
>
>> Yeah, sorry about that, it's hard to fix now but I think perhaps when
>
> this gets
>>
>> committed to SVN it will get treated as a move (not sure).
>
> this won't get automatically fixed on commit, but it's actually easy to
> fix :) the benefits would be we'd keep the history of that file, which
> is very useful sometimes.
>
> move content/test/mock_render_thread.h blah.h
> svn revert chrome/renderer/mock_render_thread.h
> content/test/mock_render_thread.h
> svn mv chrome/renderer/mock_render_thread.h
> content/test/mock_render_thread.h
> move blah.h content/test/mock_render_thread.h
>
> http://codereview.chromium.org/8230034/
>

[Jói, 2011-10-13 21:15:31]

OK, so I tried very hard and jumped through several hoops to try to get git to
register these split files as two separate moves, and I haven't been successful
- they still show up as new files in the Rietveld diff.

OTOH, it would appear according to this thread from back in May that there is
move detection that happens in [git svn] when you commit:
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

OK if we see what happens on commit, and I could then revert and reland via an
SVN repo if it doesn't detect the move?

Cheers,
Jói


On 2011/10/13 19:09:40, Jói wrote:
> I'm using git and I have lots of local changes in between, so it's a
> bit more complicated, but I'll figure it out before I commit.
> 
> Cheers,
> Jói
> 
> 
> On Thu, Oct 13, 2011 at 12:03 PM,  <mailto:jam@chromium.org> wrote:
> >
> >
>
http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
> > File content/test/mock_render_thread.h (right):
> >
> >
>
http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thr...
> > content/test/mock_render_thread.h:1: // Copyright (c) 2011 The Chromium
> > Authors. All rights reserved.
> > On 2011/10/13 18:47:27, Jói wrote:
> >>
> >> On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
> >> > nit: it would be better if this file was svn moved so that it's easy
> >
> > to see
> >>
> >> what
> >> > got factored out of it, but no worries, i trust you did the write
> >
> > thing
> >
> >> Yeah, sorry about that, it's hard to fix now but I think perhaps when
> >
> > this gets
> >>
> >> committed to SVN it will get treated as a move (not sure).
> >
> > this won't get automatically fixed on commit, but it's actually easy to
> > fix :) the benefits would be we'd keep the history of that file, which
> > is very useful sometimes.
> >
> > move content/test/mock_render_thread.h blah.h
> > svn revert chrome/renderer/mock_render_thread.h
> > content/test/mock_render_thread.h
> > svn mv chrome/renderer/mock_render_thread.h
> > content/test/mock_render_thread.h
> > move blah.h content/test/mock_render_thread.h
> >
> > http://codereview.chromium.org/8230034/
> >

[jam, 2011-10-13 21:39:07]

oh I wasn't aware that you were using git. if you're doing it like the
recommended way, then it'll probably show up in svn. sorry, i assumed you
were using svn.

On Thu, Oct 13, 2011 at 2:15 PM, <joi@chromium.org> wrote:

> OK, so I tried very hard and jumped through several hoops to try to get git
> to
> register these split files as two separate moves, and I haven't been
> successful
> - they still show up as new files in the Rietveld diff.
>
> OTOH, it would appear according to this thread from back in May that there
> is
> move detection that happens in [git svn] when you commit:
> http://groups.google.com/a/**chromium.org/group/chromium-**
>
dev/browse_thread/thread/**a45a359d2a24e987<http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/a45a359d2a24e987>
>
> OK if we see what happens on commit, and I could then revert and reland via
> an
> SVN repo if it doesn't detect the move?
>
> Cheers,
> Jói
>
>
>
> On 2011/10/13 19:09:40, Jói wrote:
>
>> I'm using git and I have lots of local changes in between, so it's a
>> bit more complicated, but I'll figure it out before I commit.
>>
>
>  Cheers,
>> Jói
>>
>
>
>  On Thu, Oct 13, 2011 at 12:03 PM,  <mailto:jam@chromium.org> wrote:
>> >
>> >
>>
>
> http://codereview.chromium.**org/8230034/diff/3041/content/**
>
test/mock_render_thread.h<http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thread.h>
>
>> > File content/test/mock_render_**thread.h (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/8230034/diff/3041/content/**
>
test/mock_render_thread.h#**newcode1<http://codereview.chromium.org/8230034/diff/3041/content/test/mock_render_thread.h#newcode1>
>
>> > content/test/mock_render_**thread.h:1: // Copyright (c) 2011 The
>> Chromium
>> > Authors. All rights reserved.
>> > On 2011/10/13 18:47:27, Jói wrote:
>> >>
>> >> On 2011/10/13 17:35:32, John Abd-El-Malek wrote:
>> >> > nit: it would be better if this file was svn moved so that it's easy
>> >
>> > to see
>> >>
>> >> what
>> >> > got factored out of it, but no worries, i trust you did the write
>> >
>> > thing
>> >
>> >> Yeah, sorry about that, it's hard to fix now but I think perhaps when
>> >
>> > this gets
>> >>
>> >> committed to SVN it will get treated as a move (not sure).
>> >
>> > this won't get automatically fixed on commit, but it's actually easy to
>> > fix :) the benefits would be we'd keep the history of that file, which
>> > is very useful sometimes.
>> >
>> > move content/test/mock_render_**thread.h blah.h
>> > svn revert chrome/renderer/mock_render_**thread.h
>> > content/test/mock_render_**thread.h
>> > svn mv chrome/renderer/mock_render_**thread.h
>> > content/test/mock_render_**thread.h
>> > move blah.h content/test/mock_render_**thread.h
>> >
>> >
http://codereview.chromium.**org/8230034/<http://codereview.chromium.org/8230...
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8230034/<http://codereview.chromium.org/8230...
>

[Jói, 2011-10-13 21:40:12]

PTAL at translate_helper_browsertest.cc, this fixes the crash we were seeing
even in process-per-browsertest mode. I'm not sure we should expect multiple
tests to pass in single_process since it is intended for debugging a single test
(???)

[jam, 2011-10-13 21:43:31]

still lgtm

On 2011/10/13 21:40:12, Jói wrote:
> PTAL at translate_helper_browsertest.cc, this fixes the crash we were seeing
> even in process-per-browsertest mode. I'm not sure we should expect multiple
> tests to pass in single_process since it is intended for debugging a single
test
> (???)

it's unfortunate that some of these tests don't work when multiple ones run in
one process, but that's not your fault!

[commit-bot: I haz the power, 2011-10-13 21:45:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/8230034/16001

[commit-bot: I haz the power, 2011-10-13 21:46:11]

Presubmit check for 8230034-16001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    content/test/mock_keyboard.cc:20
    content/test/mock_keyboard.h:94
    content/test/mock_keyboard_driver_win.cc:152
    content/test/mock_keyboard_driver_win.h:24
    content/test/render_view_test.h:81

Presubmit checks took 3.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Jói, 2011-10-13 21:47:52]

will commit by hand once trybots are happy

On Thu, Oct 13, 2011 at 2:46 PM,  <commit-bot@chromium.org> wrote:
> Presubmit check for 8230034-16001 failed and returned exit status 1.
>
> Running presubmit commit checks ...
>
> ** Presubmit Warnings **
> New code should not use wstrings.  If you are calling an API that accepts a
> wstring, fix the API.
>    content/test/mock_keyboard.cc:20
>    content/test/mock_keyboard.h:94
>    content/test/mock_keyboard_driver_win.cc:152
>    content/test/mock_keyboard_driver_win.h:24
>    content/test/render_view_test.h:81
>
> Presubmit checks took 3.4s to calculate.
>
> Was the presubmit check useful? Please send feedback & hate mail to
> maruel@chromium.org!
>
>
> http://codereview.chromium.org/8230034/
>