1597477 - Audit nsIDocShellTreeItem usage in ResetFocusState in layout/base/nsDocumentViewer.cpp

Status: RESOLVED FIXED

(Core :: Layout, task, P2)

Description

[Kannan Vijayan [:djvj]]

In file layout/base/nsDocumentViewer.cpp

Uses DocShellEnumerator to call ClearFocus on each inner document window.

Called from ExitPrintPreview only.

Change to using some BrowsingContextEnumerator (new data structure).

Use IPC to handle out-of-process children.

Rare situation, not too perf sensitive, to IPC is ok.

Comment 1

[Chris Peterson [:cpeterson]]

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

Comment 2

[Nika Layzell [:nika] (ni? for response)]

:smaug, the comment here suggests that this code may not be necessary at all anymore. Do you think it would be possible to remove it entirely? https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/layout/base/nsDocumentViewer.cpp#3762-3764

Comment 3

[Chris Peterson [:cpeterson]]

Moving to Layout.

Please audit this use of the nsIDocShellTreeItem interface. 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 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 4

[Chris Peterson [:cpeterson]]

Auditing whether this use of nsIDocShellTreeItem breaks when Fission is enabled blocks Fission Nightly.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I believe it could be removed, but jwatt could confirm too. He wrote the comment.

Comment 6

[Jonathan Watt [:jwatt]]

If I'd convinced myself it was true I would have just removed it. ;) It seemed likely to me, hence the comment, but it needs to be investigated to make sure we don't need it to, say, break reference loops. I left the needinfo to see if I could make time to look into this but so far I haven't been able to, so I guess I should at least comment here.

Comment 7

[Sean Voisen (:svoisen)]

Putting the print whiteboard tag on this since it seems to be solely print related.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Looks like kashav has started replacing the usage in ResetFocusState in bug 1655521, so regardless whether the function call of ResetFocusState is necessary or not, we can close this bug once after bug 1655521. (I mean we can defer the decision of the necessity in a later bug).

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Now the function was dropped in bug 1655521.