886046 - Add a MockColorPicker module in SpecialPowers

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 1

[cmtalbert]

Comment on attachment 766337 [details] [diff] [review]
Patch

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

This looks pretty good. I think my only nit is that the exception you throw in open should start with TEST-UNEXPECTED-FAIL | <issue> so that it shows up in the test logs (it will be highlighted as a failure).

Ideally, I'd love to see a test for this (to ensure the mock object properly restores the real colorpicker on cleanup, but I don't see any way to easily verify that, so I'll leave that one up to you.  The other mock objects also don't have these types of tests. (Tests for specialpowers should be in testing/mochitest/tests if you think of a good way to add one)

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch

I did a few changes. Some being related to nsIColorPicker interface changes and some more related to the test infra: if there is no component, the test will no longer fail and it will just use the mock instead. It is very useful to be able to land stuff without all backends being implemented.

Comment 3

[cmtalbert]

Comment on attachment 772089 [details] [diff] [review]
Patch

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

This looks good. I like it better. The only issue I see (and I'm not sure it is an issue) is that in the previous patch you had a returnOK and returnCancel constants on the MockColorPicker. Not sure if the spec changed and you left them off intentionally or if it was an omission.

Otherwise, it looks good. I like this better.

Comment 4

[Mounir Lamouri (:mounir)]

(In reply to Clint Talbert ( :ctalbert ) from comment #3)
> Comment on attachment 772089 [details] [diff] [review]
> Patch
> 
> Review of attachment 772089 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good. I like it better. The only issue I see (and I'm not sure it
> is an issue) is that in the previous patch you had a returnOK and
> returnCancel constants on the MockColorPicker. Not sure if the spec changed
> and you left them off intentionally or if it was an omission.

We change the interface so those things are no longer there. Thanks for the quick review :)

Comment 5

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0b2e977847

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cc0b2e977847