On Thu, 26 May 2022 17:00:46 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> Jay Bhaskar has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Adding space for map include > > modules/javafx.web/src/main/native/Source/WebCore/bindings/java/EventListenerManager.cpp > line 106: > >> 104: else >> 105: isReferringToOtherListener = true; >> 106: } > > I think I now understand what this is trying to do, but it doesn't looks like > it's implemented correctly nor is it optimal. > > It seems that the `isReferringToOtherListener` flag is intended to be true > iff there is any `JavaEventListener` with a ref count > 1 associated with the > Window being unregistered. Ignoring any possible bugs in how it is > implemented, there are two problems with this approach. First, you want to > remove entries associated with a particular listener if _that_ listener isn't > used by another window. So a global "is any listener referring to another > window" isn't what you want. Second, since this is multimap, it would seem > better to remove individual `(key,value)` pairs associated with the specific > window being removed rather than calling erase only for those listeners that > aren't being shared at all. If this is feasible, then you wouldn't have to > even care if that listener is used by a node in another window. I'm not > familiar enough with C++ multimap calls to know how easy this would be, but > presumably, it shouldn't be too hard. > > Not directly related to this, it might be cleaner to move this logic to > `unregisterDOMWindow` instead of having a separate `resetDOMWindow` method, > since this is really part of the same conceptual operation. I have used the suggested method , and tested it works expected. Thanks ------------- PR: https://git.openjdk.java.net/jfx/pull/799
