1597500 - Fix uses of nsTypeAheadFind::FindItNow in toolkit/components/typeaheadfind/nsTypeAheadFind.cpp

Status: NEW

(Core :: Find Backend, defect, P3)

Description

[Kannan Vijayan [:djvj]]

In file toolkit/components/typeaheadfind/nsTypeAheadFind.cpp

The comment here indicates that this functionality has already been fixed to work in post-fission:

• https://searchfox.org/mozilla-central/rev/6566d92dd46417a2f57e75c515135ebe84c9cef5/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp#375

The remaining work is just to change this over from using DocShellEnumerator to a BrowsingContextEnumerator, skipping out-of-process

Comment 1

[Chris Peterson [:cpeterson]]

Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).

Comment 2

[Chris Peterson [:cpeterson]]

Neil, can you please audit this use of the nsIDocShellTreeItem interface in nsTypeAheadFind?

With Fission enabled, Documents and nsDocShells for related frames, such as subframes and parent documents, may not be available within the current process and the corresponding nsIDocShellTreeItem methods will return null.

If this code is broken with Fission, fixing it blocks enabling Fission is Nightly and your team should prioritize (or delegate) the fix accordingly.

If this code works as-is with Fission, we don't need to remove this usage of nsIDocShellTreeItem until when we remove nsIDocShellTreeItem entirely (bug 1607591) after we ship Fission MVP.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Comment 3

[Neil Deakin]

This code is only called for non-fission mode. In fission mode, a different codepath using JSWindowActors is used.

One special case though is that this codepath is also used if modal highlighter is enabled (only changeable using about:config), since the modal highlighter doesn't work in fission mode. That would this break if the code is removed, but the modal highlighter doesn't work very well anyway.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to Neil Deakin from comment #3)

This code is only called for non-fission mode. In fission mode, a different codepath using JSWindowActors is used.

Thanks! In that case, IIUC, this bug doesn't need to block shipping Fission MVP... unless we need to enable the modal highlighter for Fission?

One special case though is that this codepath is also used if modal highlighter is enabled (only changeable using about:config), since the modal highlighter doesn't work in fission mode. That would this break if the code is removed, but the modal highlighter doesn't work very well anyway.

Is the modal highlighter enabled by default in non-Fission mode? Is there a bug # for making it work with Fission? I couldn't find one.

Comment 5

[Neil Deakin]

Is the modal highlighter enabled by default in non-Fission mode? Is there a bug # for making it work with Fission? I couldn't find one.

The modal highlighter is not enabled by default. I don't see a bug about it. You might ask mikedeboer about it what the plans are for the modal highlighter.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Neil Deakin from comment #5)

Is the modal highlighter enabled by default in non-Fission mode? Is there a bug # for making it work with Fission? I couldn't find one.

The modal highlighter is not enabled by default. I don't see a bug about it. You might ask mikedeboer about it what the plans are for the modal highlighter.

@ Mike, what are the plans for the modal highlighter? If it's not currently enabled by default, does making the modal highlighter compatible with Fission need to block shipping Fission?

Tentatively tracking for Fission riding the trains to Beta (M7)

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Chris Peterson [:cpeterson] from comment #6)

@ Mike, what are the plans for the modal highlighter? If it's not currently enabled by default, does making the modal highlighter compatible with Fission need to block shipping Fission?

The plans are to pick it back up whenever the resources are available and push it forward to release. One of the first tasks would now be to make it Fission compatible and the process should be quite similar to how Devtools refactored their highlighters (modal highlighting uses the same mechanism; anonymous content).

There's absolutely no need to have this block the Fission project.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:mikedeboer, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 9

[Chris Peterson [:cpeterson]]

(In reply to Mike de Boer [:mikedeboer] from comment #7)

There's absolutely no need to have this block the Fission project.

Thanks. In that case, I will move this bug to our Fission Future milestone (for post-MVP work).

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Clear a needinfo that is pending on an inactive user.

Inactive users most likely will not respond; if the missing information is essential and cannot be collected another way, the bug maybe should be closed as INCOMPLETE.

For more information, please visit auto_nag documentation.