Bug 130865

Created attachment 228011 [details] the patch

(In reply to comment #0) > That way, it will be possible later to track on a per-frame level the order in which identifiers were handed out. How will it be possible? What will this accomplish? I'm not sure DocumentLoader is the right place for this.

Per discussion with Brady, we concluded that it's better architecture for the main frame's document loader to vend unique identifiers. That way there is only one vendor per frame tree, and thus for 129391 we'll only need to save one identifier order vector. I'm going to upload another patch with this alternate arrangement, then rebase a patch for 129391 and test that before setting r?.

Created attachment 228115 [details] Patch

Comment on attachment 228115 [details] Patch Looks fine to me. I'll let Brady look it over.

I haven't posted a new patch for 129391 yet, that will be up later tonight or tomorrow.

(In reply to comment #6) > I haven't posted a new patch for 129391 yet, that will be up later tonight or tomorrow. Other patch is up, so this thing is ready for review.

Can anyone review this?

This is inelegant. Going to the main frame’s document loader each time is super-awkward. Adding something to all document loaders that is only used for main frame document loaders is a bad pattern that we should not do more of. Is there anything we can do to make the idiom clearer and not involve so much reliance on traversing pointers that might be null?

(In reply to comment #9) > This is inelegant. > > Going to the main frame’s document loader each time is super-awkward. Adding something to all document loaders that is only used for main frame document loaders is a bad pattern that we should not do more of. > > Is there anything we can do to make the idiom clearer and not involve so much reliance on traversing pointers that might be null? I don't see a way to make this more elegant without adding a new abstraction. Last month I heard that some folks had the desire to add an abstraction to represent the "current page", and that such a class would get new instances on main frame navigations. Every document and frame and loader would have a reference to the "current page", like MainFrame, but with more integration with navigation somehow. I don't know enough about the big picture to write a patch for that abstraction. This hypothetical class is where vending and tracking of per-main frame resource identifiers should go, but I don't think anyone has had time to work on that refactoring. I would prefer to add FIXMEs and open a new bug for adding that abstraction and develop that idea separately, rather than gating this smaller refactor (and the remainder of web replay patches that depend on network replay).

(In reply to comment #10) > (In reply to comment #9) > > This is inelegant. > > > > Going to the main frame’s document loader each time is super-awkward. Adding something to all document loaders that is only used for main frame document loaders is a bad pattern that we should not do more of. > > > > Is there anything we can do to make the idiom clearer and not involve so much reliance on traversing pointers that might be null? > > I don't see a way to make this more elegant without adding a new abstraction. > > Last month I heard that some folks had the desire to add an abstraction to represent the "current page", and that such a class would get new instances on main frame navigations It would actually get new instances on any navigation, including subframe navigations. So you'd always have a non-null MainFrame, and you could always ask it for "current page". > Every document and frame and loader would have a reference to the "current page", like MainFrame, but with more integration with navigation somehow. Not really - Every document and frame and loader has a path to their MainFrame, who is the sole keeper of "the current page" > This hypothetical class is where vending and tracking of per-main frame resource identifiers should go, but I don't think anyone has had time to work on that refactoring. I would prefer to add FIXMEs and open a new bug for adding that abstraction and develop that idea separately, rather than gating this smaller refactor (and the remainder of web replay patches that depend on network replay). Don't vending of per-MainFrame resource IDs belong on the MainFrame itself?

(In reply to comment #11) > (In reply to comment #10) > It would actually get new instances on any navigation, including subframe navigations. > ... > So you'd always have a non-null MainFrame, and you could always ask it for "current page". I suppose it could be per-any-navigation. Though that level of granularity isn't needed for replay but is compatible. > Not really - Every document and frame and loader has a path to their MainFrame, who is the sole keeper of "the current page" Oh, true, we always go through MainFrame in the patch, as written. > Don't vending of per-MainFrame resource IDs belong on the MainFrame itself? Ah, I should have been more specific. For replay purposes, we need to be able to record what resource IDs were handed out on a per-main-frame-*navigation* basis. The main benefit of using the main frame's document loader is that its lifetime is obvious and corresponds exactly to the "main frame navigation" boundary used to segment executions into independent replayable pieces when capturing. Vending and recording IDs from MainFrame itself is more complicated. 1. MainFrame will stick around across multiple navigations, so we'd have to reach into it to clear out the recorded vended IDs when the main frame navigates. 2. It also will interleave IDs vended to requests from the main frame's provisional and active document loaders, but the two loaders are not necessarily both being captured or replayed. If we start capturing the next main frame navigation and a resource request is created when the old main frame is unloaded, then the vended ID for the request will be mistakenly captured. 3. Disambiguation is hard without the natural segmenting provided by the lifetime of the main frame document loader. To disambiguate whether the ID should be captured or not, we need to look at a flag on the associated document's input cursor. AFAIK, we don't always have a document around when requesting a resource ID (e.g., on initial document resource request), so not sure how we could always disambiguate correctly.

Comment on attachment 228115 [details] Patch Clearing r? flag as this patch is bit-rotted.

Closing web replay-related bugs until we resume working on the feature again.