1619798 - Move `onLoadRequest()` into `DocumentLoadListener::Open()`

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

This also requires us to move to DocumentChannel.

<mattwoodrow> I'd do it at the start of DocumentLoadListener::Open, and if it rejects, then do SendAsyncOpenFailed(NS_BINDING_ABORTED);

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Dylan, it shouldn't take too long if you want to give this a try. Flip browser.tabs.documentchannel on and put the delegate stuff into DocumentLoadListener.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Whoops, see comment #1

Comment 3

[Dylan Roeh (:droeh) (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) (he/him) from comment #0)

This also requires us to move to DocumentChannel.

<mattwoodrow> I'd do it at the start of DocumentLoadListener::Open, and if it rejects, then do SendAsyncOpenFailed(NS_BINDING_ABORTED);

Matt, I'm looking into this and it doesn't seem like SendAsyncOpenFailed() exists... what did you mean here?

Comment 4

[Matt Woodrow (:mattwoodrow)]

Sorry, set *aRv = NS_BINDING_ABORTED, and return false. The caller will handle sending a message back to the content process to abort the load.

Comment 5

[Dylan Roeh (:droeh) (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)

Sorry, set *aRv = NS_BINDING_ABORTED, and return false. The caller will handle sending a message back to the content process to abort the load.

Alright, now I'm more confused :)

The goal here was to update LoadURIDelegate::loadURI to return a promise that resolves to a boolean rather than spinning the event loop to synchronously return a boolean. I don't see how this gets us any closer to that goal if we need to set an outparam and conditionally return from DocumentLoadListener::Open based on the output of LoadURIDelegate::loadURI -- don't we still just end up needing to spin the event loop?

Comment 6

[Matt Woodrow (:mattwoodrow)]

The main difference is that we'll now be running the delegate check in the parent process, rather than the child, so I was hoping we could just do it synchronously.

Comment 7

[Matt Woodrow (:mattwoodrow)]

I think it'd also be fairly easy to split DocumentLoadListener::Open into two parts and do something like:

DocumentLoadListener::Open() {
  mLoadDelegate->LoadURI()->Then(ContinueOpen);
}

Comment 8

[Dylan Roeh (:droeh) (he/him)]

Attachment: Bug 1619798 - Move GeckoView onLoadRequest calls to DocumentLoadListener.cpp r=snorp!,mattwoodrow!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/762ac95ae11e
Move GeckoView onLoadRequest calls to DocumentLoadListener.cpp r=snorp,mattwoodrow

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/762ac95ae11e