Apache Cordova
  1. Apache Cordova
  2. CB-514

Do we need to monkey patch addEventListener()?

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: CordovaJS
    • Labels:
      None

      Description

      discussion here: http://markmail.org/message/mghdkbumdyjhmaye

      I noticed in cordova.js we override the following messages on window/document:

      document.addEventListener = function(evt, handler, capture) {...}
      window.addEventListener = function(evt, handler, capture) {...}
      document.removeEventListener = function(evt, handler, capture) {...}
      window.removeEventListener = function(evt, handler, capture) {...}
      

      aka as "monkey patching".

      Apparently, we aren't sure why we're doing this. My guess is that we've done this in the past when dispatching user-land events wasn't possible, and so trapping listeners was the only way to get the listeners so you could send them events.

      It would be nice to remove this monkey patching, if we can, by using:

      event = document.createEvent('Events')
      event.initEvent(blah)
      
      window.dispatchEvent(event)
      // or document.dispatchEvent(event)
      

      We'll need to figure out if this works on all our platforms. Even if it doesn't work on all of them, I think we should only use monkey patching on platforms that we have to.

        Activity

        Hide
        Filip Maj added a comment -

        I think our monkey-patching is sort of related to CB-491.

        If we didn't monkey-patch and just dispatched events like we're supposed to, jQuery 1.7.2 probably wouldn't barf... maybe.

        Show
        Filip Maj added a comment - I think our monkey-patching is sort of related to CB-491 . If we didn't monkey-patch and just dispatched events like we're supposed to, jQuery 1.7.2 probably wouldn't barf... maybe.
        Hide
        Filip Maj added a comment -

        I've looked into this a little bit more, and been working on a branch in my repo.

        I stumbled upon an issue, though. For example, in the battery API, we need to know when the first handler to a window or document event gets added, and when the last handler gets removed, so that we can tell the native code to start and stop listening for battery events. I can't see a way to do that unless we monkeypatch addEventListener et al.

        Any ideas, Pat?

        Show
        Filip Maj added a comment - I've looked into this a little bit more, and been working on a branch in my repo . I stumbled upon an issue, though. For example, in the battery API, we need to know when the first handler to a window or document event gets added, and when the last handler gets removed, so that we can tell the native code to start and stop listening for battery events. I can't see a way to do that unless we monkeypatch addEventListener et al. Any ideas, Pat?
        Hide
        Patrick Mueller added a comment -

        I guess the place checking the first event / last event is here?

        I don't really know anything about the "Battery API". Googling yielded this:

        This API doesn't expect you to use window as the EventTarget, but instead navigator.battery.

        And according to

        we are in fact installing navigator.battery.

        So why is window the EventTarget for these events? Looking through the older versions of the Battery spec, you can see they've moved the EventTarget from window to a factory to the navigator property. But none of the events look similar to the one's we've defined.

        Show
        Patrick Mueller added a comment - I guess the place checking the first event / last event is here? https://github.com/filmaj/incubator-cordova-js/blob/axemonkeypatch/lib/common/plugin/battery.js#L33 I don't really know anything about the "Battery API". Googling yielded this: http://www.w3.org/TR/battery-status/ This API doesn't expect you to use window as the EventTarget , but instead navigator.battery . And according to https://github.com/filmaj/incubator-cordova-js/blob/axemonkeypatch/lib/common/common.js we are in fact installing navigator.battery . So why is window the EventTarget for these events? Looking through the older versions of the Battery spec, you can see they've moved the EventTarget from window to a factory to the navigator property. But none of the events look similar to the one's we've defined.
        Hide
        Filip Maj added a comment -

        The original implementation of this API was based on this version of the spec:

        http://www.w3.org/TR/2011/WD-battery-status-20110602/

        ... which only existed on window.

        However this is only one example where we need to keep track of the number of handlers to events on window and document:

        ... think that's it.

        Show
        Filip Maj added a comment - The original implementation of this API was based on this version of the spec: http://www.w3.org/TR/2011/WD-battery-status-20110602/ ... which only existed on window . However this is only one example where we need to keep track of the number of handlers to events on window and document : keeping track of back button handlers on Android keeping track of all hardware button handlers on BlackBerry keeping track of resume and pause events on BlackBerry keeping track of back button handlers on WP7 ... think that's it.
        Hide
        Patrick Mueller added a comment -

        That's unfortunate. Thanks for the links.

        Not really clear to me if we need to do all this counting in all the cases, but it seems likely that even if we could fix them all now, we'd find another one next week.

        We should at least add a comment in the source where we're monkey patching, as to why.

        There is a bit of silver lining. When you wrap handlers like this, you can provide extra behavior, like logging, catching/reporting exceptions, etc.

        Show
        Patrick Mueller added a comment - That's unfortunate. Thanks for the links. Not really clear to me if we need to do all this counting in all the cases, but it seems likely that even if we could fix them all now, we'd find another one next week. We should at least add a comment in the source where we're monkey patching, as to why. There is a bit of silver lining. When you wrap handlers like this, you can provide extra behavior, like logging, catching/reporting exceptions, etc.
        Hide
        Filip Maj added a comment - - edited

        I would love to get rid of the monkeypatching but, as far as I can tell, if we start listening for the various device events willy-nilly we'd be using up more resources. In general the handler counting is there to trigger device listening of system events only when necessary.

        Setting to Won't Fix for now but if we can figure out a way around these issues then I am all for reopening.

        Show
        Filip Maj added a comment - - edited I would love to get rid of the monkeypatching but, as far as I can tell, if we start listening for the various device events willy-nilly we'd be using up more resources. In general the handler counting is there to trigger device listening of system events only when necessary. Setting to Won't Fix for now but if we can figure out a way around these issues then I am all for reopening.

          People

          • Assignee:
            Filip Maj
            Reporter:
            Patrick Mueller
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development