Closed Bug 1619798 Opened 7 months ago Closed 6 months ago

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

Categories

(GeckoView :: General, enhancement, P1)

Unspecified
All
enhancement

Tracking

(firefox77 fixed)

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: snorp, Assigned: droeh)

References

(Blocks 1 open bug, Regressed 6 open bugs)

Details

(Whiteboard: [geckoview:m76][geckoview:m77])

Attachments

(1 file)

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);

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.

Whoops, see comment #1

Flags: needinfo?(droeh)

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (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?

Flags: needinfo?(droeh) → needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)

(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?

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)

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);
}
Assignee: nobody → droeh
Whiteboard: [geckoview:m76] → [geckoview:m76][geckoview:m77]
Blocks: 1611820
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/762ac95ae11e
Move GeckoView onLoadRequest calls to DocumentLoadListener.cpp r=snorp,mattwoodrow
Regressions: 1629145
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
Regressions: 1629631
Regressions: 1630062
Regressions: 1630064
Regressions: 1630066
Regressions: 1630069
Regressions: 1630073
You need to log in before you can comment on or make changes to this bug.