1131219 - Add focusmanager.testmode: true to default prefs for Firefox desktop for Marionette client

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, defect)

Description

[Barbara Miller (:galgeek)]

The Marionette client should set focusmanager.testmode: true for Firefox desktop.

Comment 1

[Barbara Miller (:galgeek)]

Attachment: 0001-Bug-1131219-Add-focusmanager.testmode-true-to-defaul.patch

This patch simply adds focusmanager.testmode: True to required_prefs.

I see on line 152 of the same file, geckoinstance.py, the class definition "class B2GDesktopInstance(GeckoInstance)" that, among other things, adds focusmanager.testmode: True. I wonder if it's better to do something similar to add this pref for Firefox desktop.

Comment 2

[David Burns :automatedtester]

Comment on attachment 8562286 [details] [diff] [review]
0001-Bug-1131219-Add-focusmanager.testmode-true-to-defaul.patch

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

looks great, can you push to try and see how it works?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Barbara doesn't have push access so far. So I took this action and pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a89ba73cafe8

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

David, can you please have a look at the try results? For me all looks fine.

Comment 5

[David Burns :automatedtester]

Comment on attachment 8562286 [details] [diff] [review]
0001-Bug-1131219-Add-focusmanager.testmode-true-to-defaul.patch

Changing f+ to r+ now we have try results. Thanks for doing this!

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Barbara, please update the patch so that the commit message lists David as reviewer. Maybe you can also update teh patch so that the prefs are alphabetically sorted. Once done and the updated patch is uploaded, set the 'checkin-needed' keyword here.

Comment 7

[Barbara Miller (:galgeek)]

Attachment: 0001-Bug-1131219-Sort-updated-prefs-alphbetically.-r-dbur.patch

Thank you!

I've updated the patch per Henrik's comment.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83b65d75fcf7

Comment 9

[Barbara Miller (:galgeek)]

Attachment: 0001-Bug-1131219-Add-focusmanager.testmode-true-to-defaul.patch

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/83b65d75fcf7

David, I assume that was a kinda broken landing! We should back this out, given that this was not the correct patch.

Barbara, you have to be very careful when requesting checkin-needed that the correct patch is attached. Please double check before setting this flag.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Sorry, I'm gone for the weekend. Please ping KWierso on IRC if you need immediate help.

Comment 12

[David Burns :automatedtester]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #8)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/83b65d75fcf7
> 
> David, I assume that was a kinda broken landing! We should back this out,
> given that this was not the correct patch.
> 
> Barbara, you have to be very careful when requesting checkin-needed that the
> correct patch is attached. Please double check before setting this flag.

No we should not back it out! Unfortunately the drive by review comment in comment 6, which should have actually been a separate patch or even bug to sort that, has clearly caused the confusion here. 

Also, please do not n-i individual sheriffs, it is not one person's responsibility to back things out, there is a team.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to David Burns :automatedtester from comment #12)
> No we should not back it out! Unfortunately the drive by review comment in
> comment 6, which should have actually been a separate patch or even bug to
> sort that, has clearly caused the confusion here. 

Sorry for that, but the diff I was commenting on had nearly no context, so it was not clear that way more prefs are above. My intention was only to let Barbara put the line at the end.

> Also, please do not n-i individual sheriffs, it is not one person's
> responsibility to back things out, there is a team.

Usually the person who lands patches is responsible for backing them out in case of problems. Given that Barbara has no permissions and Ryan landed the fix, I added n-i for him.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/83b65d75fcf7

Comment 15

[Andrew Halberstadt [:ahal]]

Looks like there is still work to do here.

Comment 16

[Barbara Miller (:galgeek)]

Attachment: Add focusmanager.testmode: True to current master.

It looks like focusmanager.testmode has not yet made it into mozilla-central.
This new patch should make it so.

I apologize for all the confusion! It was my mistake. 

Thanks for your help!

Comment 17

[David Burns :automatedtester]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Moving bugs for Marionette client due to component changes.