Retry to add MockPresentationClient to use in layout tests.

Retry to add MockPresentationClient to use in layout tests.

Now there's a MockPresentationService that's owned by WebTestProxy and
accepts private method calls from the tests.

MockPresentationClient was moved to WebFrameTestProxy instance since
we want one per each frame. Reset logic is added too to make tests more
independent.

BUG=412331
Committed: https://crrev.com/57b3af2412a81a4c0abf54e0146c0f7bf5530249
Cr-Commit-Position: refs/heads/master@{#316199}

[whywhat, 2015-02-09 15:03:47]

avayvod@chromium.org changed reviewers:
+ mkwst@chromium.org, peter@chromium.org

[whywhat, 2015-02-09 15:03:48]

Please, take another look.
I removed the ASSERT causing the failures in the original change. Although,
maybe I should rather reset the MockPresentationClient in
WebTestProxyBase::Reset() ?

[Mike West, 2015-02-09 16:46:46]

1. Can you update the CL description to be a little less confusing? :)

2. Can you add a test that triggers the DCHECK with the original patch, but
doesn't with this patch? Or are you just guessing at what the problem might be?

[Peter Beverloo, 2015-02-09 17:40:33]

On 2015/02/09 16:46:46, Mike West wrote:
> 1. Can you update the CL description to be a little less confusing? :)

+100. Please don't do these "Revert Revert"-trains.

https://chromium.googlesource.com/chromium/src/+/514e2a654a74ca47ee5f4e6e1e10...

> 2. Can you add a test that triggers the DCHECK with the original patch, but
> doesn't with this patch? Or are you just guessing at what the problem might
be?

It's my understanding that the Blink try-bots weren't run, and that the DCHECK
was triggered by *any* test which ends up setting a presentation controller
following another test having the same constraint.

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/mock_presentation_client.cc (right):

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/mock_presentation_client.cc:34: controller_ =
controller;
I don't like removing the DCHECK() here because you do have the DCHECK in the
PresentationController. The lifetime constraints of objects should be identical.

You can add a line to TestRunner::Reset() (in the delegate_ section), but
ideally fix Blink to properly nullify the controller after the testing page is
done.

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/mock_presentation_client.h (right):

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/mock_presentation_client.h:17: explicit
MockPresentationClient();
s/explicit//

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/mock_presentation_client.h:23: // From
blink::WebPresentationClient.
nit: We usually don't document the implementation of an API, and instead have
them in a section similar to:

// blink::WebPresentationClient implementation.
virtual void ...
virtual void ...

This would be consistent with PresentationDispatcher.

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/mock_presentation_client.h:25: virtual void
setController(blink::WebPresentationController*);
Please name the argument.

[Peter Beverloo, 2015-02-11 13:44:16]

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/web_frame_test_proxy.h (right):

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/web_frame_test_proxy.h:45: return
base_proxy_->GetPresentationClientMock();
So, in regards to hierarchy, WebFrameTestProxy is analogous to RenderFrame
whereas WebTestProxy is analogous to RenderView.

You store your mock as a member on WebTestProxy (View-level), whereas the real
PresentationDispatcher is stored on RenderFrameImpl. This means that the crash
you're seeing occurs on pages with an iframe because the
MockPresentationDispatcher is shared between several frames, whereas in
production you've got a different object for each frame.

Perhaps you should consider something like the following:

- MockPresentationClient, owned by WebFrameTestProxy
- MockPresentationService, owned by WebTestProxy

MPC "connects" (in this case: registers) to the MPS when it's being watched,
just like you do with the service. Test runner calls are then made to the MPS,
which calls through to the MPCs registered to it. That's much closer to the real
flow of your feature.

[whywhat, 2015-02-12 16:27:58]

PTAL.

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/web_frame_test_proxy.h (right):

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/web_frame_test_proxy.h:45: return
base_proxy_->GetPresentationClientMock();
On 2015/02/11 13:44:16, Peter Beverloo wrote:
> So, in regards to hierarchy, WebFrameTestProxy is analogous to RenderFrame
> whereas WebTestProxy is analogous to RenderView.
> 
> You store your mock as a member on WebTestProxy (View-level), whereas the real
> PresentationDispatcher is stored on RenderFrameImpl. This means that the crash
> you're seeing occurs on pages with an iframe because the
> MockPresentationDispatcher is shared between several frames, whereas in
> production you've got a different object for each frame.
> 
> Perhaps you should consider something like the following:
> 
> - MockPresentationClient, owned by WebFrameTestProxy
> - MockPresentationService, owned by WebTestProxy
> 
> MPC "connects" (in this case: registers) to the MPS when it's being watched,
> just like you do with the service. Test runner calls are then made to the MPS,
> which calls through to the MPCs registered to it. That's much closer to the
real
> flow of your feature.

Thanks for digging this up for me, Peter. I followed your suggestion in the
uploaded patch.

[whywhat, 2015-02-12 16:30:48]

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/mock_presentation_client.cc (right):

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/mock_presentation_client.cc:34: controller_ =
controller;
On 2015/02/09 17:40:32, Peter Beverloo wrote:
> I don't like removing the DCHECK() here because you do have the DCHECK in the
> PresentationController. The lifetime constraints of objects should be
identical.
> 
> You can add a line to TestRunner::Reset() (in the delegate_ section), but
> ideally fix Blink to properly nullify the controller after the testing page is
> done.

Acknowledged.

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
File content/shell/renderer/test_runner/mock_presentation_client.h (right):

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/mock_presentation_client.h:17: explicit
MockPresentationClient();
On 2015/02/09 17:40:32, Peter Beverloo wrote:
> s/explicit//

Done.

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/mock_presentation_client.h:23: // From
blink::WebPresentationClient.
On 2015/02/09 17:40:32, Peter Beverloo wrote:
> nit: We usually don't document the implementation of an API, and instead have
> them in a section similar to:
> 
> // blink::WebPresentationClient implementation.
> virtual void ...
> virtual void ...
> 
> This would be consistent with PresentationDispatcher.

Done.

https://codereview.chromium.org/907893002/diff/1/content/shell/renderer/test_...
content/shell/renderer/test_runner/mock_presentation_client.h:25: virtual void
setController(blink::WebPresentationController*);
On 2015/02/09 17:40:32, Peter Beverloo wrote:
> Please name the argument.

Done.

[Peter Beverloo, 2015-02-12 17:39:20]

mostly lg, thank you!! A few questions

https://codereview.chromium.org/907893002/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_service.cc (right):

https://codereview.chromium.org/907893002/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_service.cc:16:
DCHECK(!screen_availability_ && !presentation_clients_.might_have_observers())
Was this DCHECK hitting because of screen_availability_ or because of having
clients?

If it's the former, could we instead only keep a Reset() method in this class
for setting |screen_availability_| to |false|, and still maintain the DCHECK for
not having clients?

I don't see why MockPresentationClient would be around for longer than
MockPresentationService, generally same-origin frames shouldn't outlive the page
they belong to.

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/l...
File content/shell/renderer/layout_test/webkit_test_runner.cc (right):

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/l...
content/shell/renderer/layout_test/webkit_test_runner.cc:253:
MockPresentationService* mock_service = proxy()->GetPresentationServiceMock();
Why not just call:

proxy()->GetPresentationServiceMock()->SetScreenAvailability(availabile);

It'd fit on a single line and doesn't hurt readability. Dito re: line 258-259.

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_client.h (right):

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_client.h:36: // Lives
within the same frame as the presentation client.
nit: "Associated with the same frame as this client"?

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_service.cc (right):

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_service.cc:24:
FOR_EACH_OBSERVER(
They're not really observers, but I guess this works well :-).

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_service.h (right):

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_service.h:18: //
MockPresentationClient is owned by WebFrameTextProxy (so one exists per each
You're repeating lifetime constraints of both MockPresentationService and
MockPresentationClient in both class-level comments. Perhaps it's better to
focus on the own class, and just mention that "MockPresentationService will
inform all registered MockPresentationClients of status updates"?

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_service.h:24: virtual
~MockPresentationService();
I don't think this has to be virtual.

[whywhat, 2015-02-12 18:40:14]

https://codereview.chromium.org/907893002/diff/20001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_service.cc (right):

https://codereview.chromium.org/907893002/diff/20001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_service.cc:16:
DCHECK(!screen_availability_ && !presentation_clients_.might_have_observers())
On 2015/02/12 17:39:20, Peter Beverloo wrote:
> Was this DCHECK hitting because of screen_availability_ or because of having
> clients?

Having clients. Some added logging showed that at least one client is destroyed
after the service (I guess the client that belongs to the main frame).

> 
> If it's the former, could we instead only keep a Reset() method in this class
> for setting |screen_availability_| to |false|, and still maintain the DCHECK
for
> not having clients?
> 
> I don't see why MockPresentationClient would be around for longer than
> MockPresentationService, generally same-origin frames shouldn't outlive the
page
> they belong to.

I'll just keep Reset.

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/l...
File content/shell/renderer/layout_test/webkit_test_runner.cc (right):

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/l...
content/shell/renderer/layout_test/webkit_test_runner.cc:253:
MockPresentationService* mock_service = proxy()->GetPresentationServiceMock();
On 2015/02/12 17:39:20, Peter Beverloo wrote:
> Why not just call:
> 
> proxy()->GetPresentationServiceMock()->SetScreenAvailability(availabile);
> 
> It'd fit on a single line and doesn't hurt readability. Dito re: line 258-259.

Done.
For consistency maybe? :)

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_client.h (right):

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_client.h:36: // Lives
within the same frame as the presentation client.
On 2015/02/12 17:39:20, Peter Beverloo wrote:
> nit: "Associated with the same frame as this client"?

Done.

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_service.cc (right):

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_service.cc:24:
FOR_EACH_OBSERVER(
On 2015/02/12 17:39:20, Peter Beverloo wrote:
> They're not really observers, but I guess this works well :-).

I guess we may want to rename ObserverList to Registry or something :)

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/mock_presentation_service.h (right):

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_service.h:18: //
MockPresentationClient is owned by WebFrameTextProxy (so one exists per each
On 2015/02/12 17:39:20, Peter Beverloo wrote:
> You're repeating lifetime constraints of both MockPresentationService and
> MockPresentationClient in both class-level comments. Perhaps it's better to
> focus on the own class, and just mention that "MockPresentationService will
> inform all registered MockPresentationClients of status updates"?

Done.
MockPresentationClient doesn't say anything about MockPresentationService.

https://codereview.chromium.org/907893002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/mock_presentation_service.h:24: virtual
~MockPresentationService();
On 2015/02/12 17:39:20, Peter Beverloo wrote:
> I don't think this has to be virtual.

Done.

[Peter Beverloo, 2015-02-12 18:55:18]

Ok, thanks for checking.

lgtm

[Mike West, 2015-02-13 04:58:31]

content/shell LGTM.

[whywhat, 2015-02-13 09:10:52]

The CQ bit was checked by avayvod@chromium.org

[commit-bot: I haz the power, 2015-02-13 09:11:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/907893002/60001

[commit-bot: I haz the power, 2015-02-13 10:08:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-13 10:09:30]

Patchset 4 (id:??) landed as
https://crrev.com/57b3af2412a81a4c0abf54e0146c0f7bf5530249
Cr-Commit-Position: refs/heads/master@{#316199}

[whywhat, 2015-02-26 14:18:05]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/963493002/ by avayvod@chromium.org.

The reason for reverting is: While the LayoutTests only test the logic in the
mocks and the plumbing, maintaining the mocks while working on the
platform/public interface is a pain and requires landing patches simultaneously
in Blink and Chromium repositories. There's no layout test using this anyway
yet.
I'll add layout tests later when the Blink-Chromium interface is stable and
maybe Blink is merged into Chromium..