1618219 - Use BrowsingContext for activeness check in nsFrameSelection.cpp

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect, P2)

Description

[Henri Sivonen (:hsivonen)]

https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/layout/generic/nsFrameSelection.cpp#2910
https://searchfox.org/mozilla-central/rev/96f1457323cc598a36f5701f8e67aedaf97acfcf/layout/generic/nsFrameSelection.cpp#1460

These use GetActiveWindow just to null-check it. Maybe they should instead check something BrowsingContext-related. It's unclear what exactly these want to check in the out-of-process iframe case.

Do these just want to check that Firefox is the frontmost app or do these want to check that the selection is in the active Firefox window?

Comment 1

[Mats Palmgren (inactive)]

It's a bug in searchfox that it displays me as the author of those lines. AFAICT, they were added in bug 1261299:
https://searchfox.org/mozilla-central/diff/6d8584507fc5baa35562fe3a8542420740032b5f/layout/generic/nsSelection.cpp#1983

Comment 2

[Chris Peterson [:cpeterson]]

M5 because Henri has patches for related bugs

Comment 3

[Henri Sivonen (:hsivonen)]

The patch author hasn't been active for a while. Needinfoing the reviewer.

Comment 4

[Markus Stange [:mstange]]

https://hg.mozilla.org/mozilla-central/rev/0ea81a4d9c2f2ece2c339e60c0f4ccbaabb4b3d4 was reviewed by masayuki and smaug. There's some explanation of it in bug 1261299 comment 116 but I currently don't understand it very well.

Comment 5

[Henri Sivonen (:hsivonen)]

masayuki, is it clearer to you what the active window null-check is supposed to be checking?

Comment 6

[Neil Deakin]

That code updates the system selection buffer; if you select text on macos, you can then use commands on the services menu to perform actions on the selected text, such as 'look up in dictionary'. It should only update when for the selection changes in the topmost active window, and not update it for other windows.

The code though looks to just check whether there is an active window, but as long as the child process focus manager knows whether it is in the active window or not, it should suffice to look at that.

Comment 7

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay to reply.

As enn said, the former checks whether Firefox is active application on the environment to avoid overwriting the selection buffer with selection changes by JS or something.

I'm not familiar with the latter, but looks like that it does same thing only on macOS. So, it also checks whether Firefox is active for avoiding to overwrite selection buffer which another application may have written.

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1618219 - Use BrowsingContext for activeness check in nsFrameSelection.cpp.

Comment 10

[Henri Sivonen (:hsivonen)]

Thanks. Let's try this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bca37552e48e37ade9d4d3afff5a0bae6756f5b

Comment 11

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c345e258b6c2b6da113326c8cd9d6672cb55e11

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8318b018c859
Use BrowsingContext for activeness check in nsFrameSelection.cpp. r=NeilDeakin

Comment 13

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8318b018c859