From [1]:

// WebFrameTestProxy is used during LayoutTests and always instantiated, at time
// of writing with Base=RenderFrameImpl. It does not directly inherit from it
// for layering purposes.

This has caused me a few days of debugging and workaround pain at [2].
In that CL, I need to add an extra parameter to RenderFrameImpl::DidFailProvisionalLoad(). That extra parameter is actually only needed in RenderFrameImpl and dcheng@ would prefer that I don't modify the base class method of WebFrameClient::DidFailProvisionalLoad(), so the natural thing thing is to just create an RenderFrameImpl::DidFailProvisionalLoadWithErrorPage() function, and call that one directly from inside the class when needed.

This causes the `http/tests/loading/bad-server-subframe.html` layout test to fail, because that test expects WebFrameTestProxy::DidFailProvisionalLoad() to be called when RenderFrameImpl (which is actually a WebFrameTestProxy with a base class of RenderFrameImpl in the test) calls DidFailProvisionalLoad() on itself. Everyone agrees that we should remove the DidFailProvisionalLoad() call from the the test expectations.

The obvious minimal change to make that test pick up the new code path (without modifying WebFrameClient) would be to also create WebFrameTestClient::DidFailProvisionalLoadWithErrorPage() and WebFrameTestProxy::DidFailProvisionalLoadWithErrorPage(), where the latter proxies to the former as well as proxying back to DidFailProvisionalLoadWithErrorPage() [3].

However, this doesn't work using the existing proxy mechanism unless WebFrameTestProxy and RenderFrameImpl inherit the DidFailProvisionalLoadWithErrorPage() method from the same class.

My latest comment on the CL:

> dcheng@ wanted me to avoid modifying WebFrameClient::DidFailProvisionalLoad()
> 
> After some discussion, we concluded that it *might* be possible to add a new DidFailProvisionalLoadWithErrorPage() function only to:
> 
> RenderFrameImpl
> WebFrameTestProxy
> WebFrameTestClient
> There is a throwaway CL wit this approach at https://chromium-review.googlesource.com/c/chromium/src/+/740375
> 
> It does not appear to work. My guess is that because RenderFrameImpl and WebFrameTestClient don't inherit DidFailProvisionalLoadWithErrorPage() from the same base class, they don't end up as the same function in the vtable. That prevents the RenderFrameImpl-which-is-actually-a-WebFrameTestProxy from going through the proxy method when it calls DidFailProvisionalLoadWithErrorPage() on itself.
> 
> I can't think of a nice way around this. :-/

Via chat:

lgarron@: Yeah.
lgarron@: I literally don't have any new ideas, apart from maybe making the test proxy an actual subclass of RFI.
lgarron@: But it looks very intentional that this was not done.
dcheng@: My understanding is we originally had this indirection due to the experiment they were doing with... uhh
dcheng@: What's that mojo browser thing they were building
dcheng@: Anyway I don't think we actually need the indirection anymore
dcheng@: So I think combining it would actually be OK
dcheng@: I would check with jam

jam@, what are your thoughts on this? Do you see any other approaches?

Since this has already dragged on for a while and is not a core architecture issue, I'd like to spend as little time as possible on exploring and implementing a solution. :-/

[1] 
https://cs.chromium.org/chromium/src/content/shell/test_runner/web_frame_test_proxy.h?l=51&rcl=d5cdb7c29401d3e32709716724b6ad7b34801af4
[2] https://chromium-review.googlesource.com/c/chromium/src/+/625337
[3] This Unless we do something clever, this means that any layout test that only expected a call to DidFailProvisionalLoad() would need to be updated to expect an immediately following call to DidFailProvisionalLoadWithErrorPage(), but that sounds okay to me.
 

From [1]:

// WebFrameTestProxy is used during LayoutTests and always instantiated, at time
// of writing with Base=RenderFrameImpl. It does not directly inherit from it
// for layering purposes.

This has caused me a few days of debugging and workaround pain at [2].
In that CL, I need to add an extra parameter to RenderFrameImpl::DidFailProvisionalLoad(). That extra parameter is actually only needed in RenderFrameImpl and dcheng@ would prefer that I don't modify the base class method of WebFrameClient::DidFailProvisionalLoad(), so the natural thing thing is to just create an RenderFrameImpl::DidFailProvisionalLoadWithErrorPage() function, and call that one directly from inside the class when needed.

This causes the `http/tests/loading/bad-server-subframe.html` layout test to fail, because that test expects WebFrameTestProxy::DidFailProvisionalLoad() to be called when RenderFrameImpl (which is actually a WebFrameTestProxy with a base class of RenderFrameImpl in the test) calls DidFailProvisionalLoad() on itself. Everyone agrees that we should avoid removing the test expectation that the provisional load failed.

The obvious minimal change to make that test pick up the new code path (without modifying WebFrameClient) would be to also create WebFrameTestClient::DidFailProvisionalLoadWithErrorPage() and WebFrameTestProxy::DidFailProvisionalLoadWithErrorPage(), where the latter proxies to the former as well as proxying back to DidFailProvisionalLoadWithErrorPage() [3].

However, this doesn't work using the existing proxy mechanism unless WebFrameTestProxy and RenderFrameImpl inherit the DidFailProvisionalLoadWithErrorPage() method from the same class.

My latest comment on the CL:

> dcheng@ wanted me to avoid modifying WebFrameClient::DidFailProvisionalLoad()
> 
> After some discussion, we concluded that it *might* be possible to add a new DidFailProvisionalLoadWithErrorPage() function only to:
> 
> RenderFrameImpl
> WebFrameTestProxy
> WebFrameTestClient
> There is a throwaway CL wit this approach at https://chromium-review.googlesource.com/c/chromium/src/+/740375
> 
> It does not appear to work. My guess is that because RenderFrameImpl and WebFrameTestClient don't inherit DidFailProvisionalLoadWithErrorPage() from the same base class, they don't end up as the same function in the vtable. That prevents the RenderFrameImpl-which-is-actually-a-WebFrameTestProxy from going through the proxy method when it calls DidFailProvisionalLoadWithErrorPage() on itself.
> 
> I can't think of a nice way around this. :-/

Via chat:

lgarron@: Yeah.
lgarron@: I literally don't have any new ideas, apart from maybe making the test proxy an actual subclass of RFI.
lgarron@: But it looks very intentional that this was not done.
dcheng@: My understanding is we originally had this indirection due to the experiment they were doing with... uhh
dcheng@: What's that mojo browser thing they were building
dcheng@: Anyway I don't think we actually need the indirection anymore
dcheng@: So I think combining it would actually be OK
dcheng@: I would check with jam

jam@, what are your thoughts on this? Do you see any other approaches?

Since this has already dragged on for a while and is not a core architecture issue, I'd like to spend as little time as possible on exploring and implementing a solution. :-/

[1] 
https://cs.chromium.org/chromium/src/content/shell/test_runner/web_frame_test_proxy.h?l=51&rcl=d5cdb7c29401d3e32709716724b6ad7b34801af4
[2] https://chromium-review.googlesource.com/c/chromium/src/+/625337
[3] This Unless we do something clever, this means that any layout test that only expected a call to DidFailProvisionalLoad() would need to be updated to expect an immediately following call to DidFailProvisionalLoadWithErrorPage(), but that sounds okay to me.
 

From [1]:

// WebFrameTestProxy is used during LayoutTests and always instantiated, at time
// of writing with Base=RenderFrameImpl. It does not directly inherit from it
// for layering purposes.

This has caused me a few days of debugging and workaround pain at [2].
In that CL, I need to add an extra parameter to RenderFrameImpl::DidFailProvisionalLoad(). That extra parameter is actually only needed in RenderFrameImpl and dcheng@ would prefer that I don't modify the base class method of WebFrameClient::DidFailProvisionalLoad(), so the natural thing thing is to just create an RenderFrameImpl::DidFailProvisionalLoadWithErrorPage() function, and call that one directly from inside the class when needed.

This causes the `http/tests/loading/bad-server-subframe.html` layout test to fail, because that test expects WebFrameTestProxy::DidFailProvisionalLoad() to be called when RenderFrameImpl (which is actually a WebFrameTestProxy with a base class of RenderFrameImpl in the test) calls DidFailProvisionalLoad() on itself. Everyone agrees that we should avoid removing the test expectation that the provisional load failed.

The obvious minimal change to make that test pick up the new code path (without modifying WebFrameClient) would be to also create WebFrameTestClient::DidFailProvisionalLoadWithErrorPage() and WebFrameTestProxy::DidFailProvisionalLoadWithErrorPage(), where the latter proxies to the former as well as proxying back to DidFailProvisionalLoadWithErrorPage() [3].

However, this doesn't work using the existing proxy mechanism unless WebFrameTestProxy and RenderFrameImpl inherit the DidFailProvisionalLoadWithErrorPage() method from the same class.

My latest comment on the CL:

> dcheng@ wanted me to avoid modifying WebFrameClient::DidFailProvisionalLoad()
> 
> After some discussion, we concluded that it *might* be possible to add a new DidFailProvisionalLoadWithErrorPage() function only to:
> 
> RenderFrameImpl
> WebFrameTestProxy
> WebFrameTestClient
> There is a throwaway CL wit this approach at https://chromium-review.googlesource.com/c/chromium/src/+/740375
> 
> It does not appear to work. My guess is that because RenderFrameImpl and WebFrameTestClient don't inherit DidFailProvisionalLoadWithErrorPage() from the same base class, they don't end up as the same function in the vtable. That prevents the RenderFrameImpl-which-is-actually-a-WebFrameTestProxy from going through the proxy method when it calls DidFailProvisionalLoadWithErrorPage() on itself.
> 
> I can't think of a nice way around this. :-/

Via chat:

lgarron@: Yeah.
lgarron@: I literally don't have any new ideas, apart from maybe making the test proxy an actual subclass of RFI.
lgarron@: But it looks very intentional that this was not done.
dcheng@: My understanding is we originally had this indirection due to the experiment they were doing with... uhh
dcheng@: What's that mojo browser thing they were building
dcheng@: Anyway I don't think we actually need the indirection anymore
dcheng@: So I think combining it would actually be OK
dcheng@: I would check with jam

jam@, what are your thoughts on this? Do you see any other approaches?

Since this has already dragged on for a while and is not a core architecture issue, I'd like to spend as little time as possible on exploring and implementing a solution. :-/

[1] 
https://cs.chromium.org/chromium/src/content/shell/test_runner/web_frame_test_proxy.h?l=51&rcl=d5cdb7c29401d3e32709716724b6ad7b34801af4
[2] https://chromium-review.googlesource.com/c/chromium/src/+/625337
[3] This Unless we do something clever, this means that any layout test that only expected a call to DidFailProvisionalLoad() would need to be updated to expect an immediately following call to DidFailProvisionalLoadWithErrorPage(), but that sounds okay to me.