Last Comment Bug 787984 - Add tests for imContentSink.jsm
: Add tests for imContentSink.jsm
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: Florian Quèze [:florian] [:flo]
: instant-messaging
Depends on:
  Show dependency treegraph
Reported: 2012-09-03 11:25 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-09-04 11:52 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

WIP (10.17 KB, patch)
2012-09-03 11:26 PDT, Florian Quèze [:florian] [:flo]
no flags Details | Diff | Splinter Review
Patch v2 (18.01 KB, patch)
2012-09-03 16:01 PDT, Florian Quèze [:florian] [:flo]
clokep: review+
mconley: feedback+
Details | Diff | Splinter Review

Description User image Florian Quèze [:florian] [:flo] 2012-09-03 11:25:17 PDT
imContentSink.jsm is the file where we filter out unwanted content from received messages before displaying them, so we should really not let regressions slip into it.
Comment 1 User image Florian Quèze [:florian] [:flo] 2012-09-03 11:26:52 PDT
Created attachment 657887 [details] [diff] [review]
Comment 2 User image Florian Quèze [:florian] [:flo] 2012-09-03 16:01:03 PDT
Created attachment 657938 [details] [diff] [review]
Patch v2

Mike, I think you wrote way more unit tests than Patrick and me, so I'm wondering if you would have some interesting feedback to offer on this.

Note for the code review: The change to newRuleset is just some cleanup. The change to cleanupNode fixes
Comment 3 User image Patrick Cloke [:clokep] 2012-09-03 16:55:09 PDT
Comment on attachment 657938 [details] [diff] [review]
Patch v2

+  // Allow id only with numbers.
+  addGlobalAllowedAttribute("id", function(aId) /^[0-9]+$/.test(aId));
Not /^\d+$/?

Besides that it looks OK (and all the tests pass ;)). As far as I can tell this should be a pretty thorough test of the content sink.
Comment 4 User image Mike Conley (:mconley) - Getting through review / needinfo backlog 2012-09-04 07:48:51 PDT
Comment on attachment 657938 [details] [diff] [review]
Patch v2

Review of attachment 657938 [details] [diff] [review]:

Looks OK to me.
Comment 5 User image Florian Quèze [:florian] [:flo] 2012-09-04 08:00:54 PDT

Note You need to log in before you can comment on or make changes to this bug.