1910087 - Merge `navigations` and `preRegisteredNavigationIds` maps into one in NavigationManager

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P3)

Description

[Alexandra Borovova [:Sasha]]

In the scope of bug 1846601 in NavigationManager we've introduced a new map with navigable id as a key and navigation info as a value, it's similar to the existing map (preRegisteredNavigationIds) to pre-register navigations which contains the same data but is cleaned up when events are sent. So we could merge these two maps into one, and replace finished property with the property state with 3 different values registered, started and finished. The name of the map should stay navigations.

Comment 1

[Liam DeBeasi]

Hi there! I am interested in contributing a patch for this. I have a couple questions:

1. Am I correct in my understanding that there should be no behavior changes as a result of this?
2. When getOrCreateNavigationId is called, we check #preRegisteredNavigationIds and delete an entry from it if it is found. The navigation ID from that entry is then typically used to add an entry to #navigations. What should happen in this case? Should I replace #preRegisteredNavigationIds with #navigations even though the entry will potentially be added back to #navigations?

Comment 2

[Alexandra Borovova [:Sasha]]

Hi Liam!

1. Am I correct in my understanding that there should be no behavior changes as a result of this?

Yes, you're correct, this is supposed to be just a refactoring and behavior should not change.

2. When getOrCreateNavigationId is called, we check #preRegisteredNavigationIds and delete an entry from it if it is found. The navigation ID from that entry is then typically used to add an entry to #navigations. What should happen in this case? Should I replace #preRegisteredNavigationIds with #navigations even though the entry will potentially be added back to #navigations?

You should replace the #preRegisteredNavigationIds with #navigations (meaning add an entry to the #navigations map with a new field state with the value registered) but you shouldn't remove the entry here anymore, and then in this place instead of creating a new entry you should just update the existing entry by changing the state field to the value started. Does it make sense? Please let me know if it doesn't :)

If you have more questions, I'm happy to answer! Also, if you want to get a bit quicker feedback, we have a matrix channel, you can join and ask there.

Comment 3

[Liam DeBeasi]

Thank you! I started work on this change, and I have two follow up questions:

1. You mentioned I should update the existing entry here to change the state field to started. My understanding of notifyFragmentNavigated is that this runs after the fragment has already navigated. Should I be updating this to finished instead?
2. While running tests I noticed that these checks are now passing where they were failing prior to my change. Is that ok?

Comment 4

[Alexandra Borovova [:Sasha]]

1. You mentioned I should update the existing entry here to change the state field to started. My understanding of notifyFragmentNavigated is that this runs after the fragment has already navigated. Should I be updating this to finished instead?

Ah, yes, you're right! In this case, you should update it to finished.

2. While running tests I noticed that these checks are now passing where they were failing prior to my change. Is that ok?

Hm... No, I think it should keep on failing, does this test still pass?

Comment 5

[Liam DeBeasi]

Attachment: Bug 1910087 - Remove preRegisteredNavigationIds in favor of navigations. r?#webdriver-reviewers

Comment 6

[Liam DeBeasi]

Good catch! My local project was outdated. Looks like the tests are working correctly on my end now after grabbing the latest changes.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a062f959094
Remove preRegisteredNavigationIds in favor of navigations. r=webdriver-reviewers,Sasha

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/1a062f959094