Closed Bug 862442 Opened 8 years ago Closed 8 years ago

Use a content script to listen for input and change events

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Whiteboard: [e10s])

Attachments

(1 file)

No description provided.
Straight forward patch that makes the content script listen for change and input events. Changes that updated the code:

* Removed the DOMAutoComplete event. The code for this existed since forever (hg@1) and isn't actually needed. We receive input and change events when text fields are autocompleted.

* Removed the keypress listener for content editable iframes. I checked and despite the comment saying we don't, we actually do receive input events. I checked this manually and included this in the test.

As with the previous bug there were no tests failing when I removed the code listening for form data changes so I wrote one.
Attachment #738090 - Flags: review?(dteller)
Comment on attachment 738090 [details] [diff] [review]
use a content script to listen for input and change events

Review of attachment 738090 [details] [diff] [review]:
-----------------------------------------------------------------

I like it.

::: browser/components/sessionstore/content/content.js
@@ +15,2 @@
>    init: function EL_init() {
> +    this.DOM_EVENTS.forEach(e => addEventListener(e, this, true));

Out of curiosity: why not the following?
  for (let e of DOM_EVENTS) {
    addEventListener(e, this, true);
  }

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +62,5 @@
>    "DNSPrefetch", "Auth", "WindowControl"
>  ];
>  
> +const MESSAGES = [
> +  "SessionStore:input", "SessionStore:pageshow"

Could you document both |MESSAGES| and each individual message?
In particular, this will become necessary if messages end up carrying a payload (e.g. tab number).

@@ +2248,5 @@
>          }
>        }
>  
>        // designMode is undefined e.g. for XUL documents (as about:config)
> +      if ((aContent.document.designMode || "") == "on" && aContent.document.body)

nice deuglification
Attachment #738090 - Flags: review?(dteller) → review+
(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Out of curiosity: why not the following?
>   for (let e of DOM_EVENTS) {
>     addEventListener(e, this, true);
>   }

No particular reason. I like fat arrow functions and functional style :)

> > +const MESSAGES = [
> > +  "SessionStore:input", "SessionStore:pageshow"
> 
> Could you document both |MESSAGES| and each individual message?

Sure!

> In particular, this will become necessary if messages end up carrying a
> payload (e.g. tab number).

Tab number? Sorry, not sure what exactly you're referring to?
(In reply to Tim Taubert [:ttaubert] from comment #3)
> > In particular, this will become necessary if messages end up carrying a
> > payload (e.g. tab number).
> 
> Tab number? Sorry, not sure what exactly you're referring to?

I'm referring to the day we start tracking invalidation of individual tabs, frames, etc. This day, we'll need to start sending more information in messages.
(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> (In reply to Tim Taubert [:ttaubert] from comment #3)
> > > In particular, this will become necessary if messages end up carrying a
> > > payload (e.g. tab number).
> > 
> > Tab number? Sorry, not sure what exactly you're referring to?
> 
> I'm referring to the day we start tracking invalidation of individual tabs,
> frames, etc. This day, we'll need to start sending more information in
> messages.

More information yes but we can figure out which browser the message came from. So by receiving 'SessionStore:input' and figuring out which browser it comes from this patch already tracks invalidation of individual tabs. The service is just not yet intelligent enough to not invalidate the whole window :)
https://hg.mozilla.org/mozilla-central/rev/c44ac3de77e6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.