967516 - Enable DEVICE_DEBUG by default for non production builds

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia, defect)

Description

[Kevin Grandon :kgrandon]

DEVICE_DEBUG is very helpful for developers, and now that we have the app manager and devtools overlay, we should enable it by default for non production builds. Perhaps making it less aggressive would be a good thing to do as well.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Step 2 - Land gaia patch

Comment 2

[Kevin Grandon :kgrandon]

Comment on attachment 8370063 [details] [review]
Step 2 - Land gaia patch

Alex - what are your thoughts here? Would you have time to review this one?

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8370063 [details] [review]
Step 2 - Land gaia patch

I have some doubts about toggling off so much stuff by default :/
You can see some failures in tests because of the lockscreen being turned off.
Turning devtools on by default sounds unharmful as it doesn't disable a feature.
Setting a longer timeout sounds also ok.
But disabling the lockscreen... I don't know.
I'd prefer not disabling it in this iteration, we can do that in a followup if that's really justified. It would also be worth sending a note to dev-gaia while landing this.

Comment 4

[Kevin Grandon :kgrandon]

Sounds good, I agree about the lockscreen - and devs can easily disable those ones from the settings panel anyway. I will update and prepare an email for dev-gaia.

Comment 5

[Kevin Grandon :kgrandon]

Going to fix indentation first in bug 967579.

Comment 6

[Kevin Grandon :kgrandon]

Comment on attachment 8370063 [details] [review]
Step 2 - Land gaia patch

Ok, this should be ready for another review round. I've updated it to address your comments, and no longer disable the lockscreen (devs can do that via custom-prefs or by using the flag. Please let me know if you see anything else. Thanks!

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8370063 [details] [review]
Step 2 - Land gaia patch

Please ensure Travis is green before landing, as that's not the case
and I can't get why...

Comment 8

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/eea8e3770b1be1bf35503a23bbe30ca0f9981204

Comment 9

[Ed Morley [:emorley]]

Reverted for multiple failures on TBPL:
https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=6d1b8c7f7859
https://tbpl.mozilla.org/php/getParsedLog.php?id=36330359&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36329820&tree=B2g-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36329990&tree=B2g-Inbound

https://github.com/mozilla-b2g/gaia/commit/25dea1143d7e7c6d9638c7e6250b866083c18fde

Comment 11

[Kevin Grandon :kgrandon]

I'm a bit confused here. I tried the same patch on gaia-try, and everything seemed to pass: https://tbpl.mozilla.org/?tree=Try&rev=80d819279264

I'm wondering if gaia try is not working too well with this test somehow? In any case as there is likely a test change *and* gaia change needed for this - I would propose that we do this in three steps so the tree always stays green.

1 - Disable test.
2 - Land gaia code.
3 - Update and enable test.

Comment 12

[Kevin Grandon :kgrandon]

Attachment: Step 1 - Temporarily disable test

On try: https://tbpl.mozilla.org/?tree=Try&rev=edd50f7b7b55

Comment 13

[Kevin Grandon :kgrandon]

Attachment: Step 1 - Temporarily disable test

Oops - better patch.

On try: https://tbpl.mozilla.org/?tree=Try&rev=b48a15df3cb8

Comment 14

[Kevin Grandon :kgrandon]

Attachment: Step 3 - Update test expected value

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8395796 [details] [diff] [review]
Step 1 - Temporarily disable test

Hey Alex - would you have a minute to review these test updates? Thanks!

Comment 16

[Kevin Grandon :kgrandon]

Attachment: Gaia patch

New gaia PR, carrying review flag.

Comment 17

[Alexandre Poirot [:ochameau]]

Comment on attachment 8395798 [details] [diff] [review]
Step 3 - Update test expected value

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

::: toolkit/devtools/apps/tests/test_webapps_actor.html
@@ +182,4 @@
>    },
>    function() {
>      info("== TEST == Get all apps");
> +    getAll(true);

By doing that, we won't ever test when the pref do forbid certified apps access!
What about adding:
  [["devtools.debugger.forbid-certified-apps", true]]
In the first setup function, in the call to SpecialPowers.pushPrefEnv?

Then you shouldn't need two patches, just this one.

Comment 18

[Kevin Grandon :kgrandon]

Attachment: Gecko patch - update default preferences

Yup - that sounds like a much better approach! I'm still new to gecko tests, but I'll figure them out some day :)

On try: https://tbpl.mozilla.org/?tree=Try&rev=85d8e29552f5

Comment 19

[Alexandre Poirot [:ochameau]]

Comment on attachment 8396472 [details] [diff] [review]
Gecko patch - update default preferences

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

r+ assuming it actually fix your issue!

Comment 20

[Kevin Grandon :kgrandon]

Adding checkin-needed for the gecko patch here: https://bug967516.bugzilla.mozilla.org/attachment.cgi?id=8396472

Feel free to land the gaia patch as well, the gecko patch just needs to land first. Thanks!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/36ed1dfdda41
Master: https://github.com/mozilla-b2g/gaia/commit/5ad38b98016c6b0eece65e7a41844043c2947c66

Comment 22

[Kevin Grandon :kgrandon]

Ugh, backing this out as it may be making a B2G ICS Emulator Debug crash worse. See also bug 980537.

https://tbpl.mozilla.org/?tree=B2g-Inbound&showall=1&rev=1423221181e2

https://github.com/mozilla-b2g/gaia/commit/e909a131d5ae702b2befdbc165996f9930653171

Comment 23

[Kevin Grandon :kgrandon]

Attachment: Gaia patch to re-land

The same patch as before, going to test this against gaia-try.

Comment 24

[Kevin Grandon :kgrandon]

Here is the revert on b2g-inbound: https://tbpl.mozilla.org/?tree=B2g-Inbound&showall=1&rev=5953c846bb5a
And here is a push to gaia-try landing the same commit: https://tbpl.mozilla.org/?tree=Try&rev=5d123d920e5d

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/36ed1dfdda41

Comment 26

[Kevin Grandon :kgrandon]

On try: https://tbpl.mozilla.org/?tree=Try&rev=c1348c929ff8

Comment 27

[Kevin Grandon :kgrandon]

B2G ICS Emulator Debug seems to be green on that try push, so let's try to land this again.

Landed: https://github.com/mozilla-b2g/gaia/commit/f17c8b655e7a3b1b9db1b8eb50243734ae94993d

Comment 28

[Ed Morley [:emorley]]

Reverted again for the test failures in comment 9:
https://tbpl.mozilla.org/?tree=B2g-Inbound&jobname=b2g_emulator_vm b2g-inbound debug test mochitest-debug-15&fromchange=29e804e6693f&tochange=93747d9cdde6

eg: 
https://tbpl.mozilla.org/php/getParsedLog.php?id=37505779&tree=B2g-Inbound

Yey for try matching production...! (I've retriggered the job on Try to see if it was just a fluke pass) Though perhaps also worth checking what got merged to gaia was the same commit as was tested on try, and a patch hunk wasn't dropped or something.

https://github.com/mozilla-b2g/gaia/commit/0622ab1bcf2335f40488b90495b0b32dac985a90

Comment 29

[Gregor Wagner [:gwagner]]

Should we re-open or whats the state here?

Comment 30

[Kevin Grandon :kgrandon]

Thanks. Yes, this is something I really want but blockers and TBPL failures have discouraged me from completing this recently =(

Let's reopen for now and maybe I'll have some free time to re-visit this soon.

Comment 31

[Kevin Grandon :kgrandon]

Attachment: Re-land Part 1 - Decouple screen.timeout from device debug

This has been a battery killer lately, so I'm going to decouple the screen timeout logic from this patch and just land that first.

Comment 32

[Kevin Grandon :kgrandon]

Comment on attachment 8483839 [details] [review]
Re-land Part 1 - Decouple screen.timeout from device debug

Carrying review.

Comment 33

[Kevin Grandon :kgrandon]

Landed the first part in master which includes removing screen.timeout from the main patch: https://github.com/mozilla-b2g/gaia/commit/1c76fda5fa05cf352d36620fcc0784c6044ebf37

Comment 34

[Kevin Grandon :kgrandon]

Attachment: Part 2 - Enable devide_debug for non-production builds

Here is a rebased part 2 of the original patch. I'm carrying the R+ as nothing has changed about the Makefile usage.

Comment 35

[Kevin Grandon :kgrandon]

This is being run on try again: https://tbpl.mozilla.org/?tree=Try&rev=068e75e34f18

Comment 36

[Kevin Grandon :kgrandon]

Let's try this again. Try run looks good to me here: https://tbpl.mozilla.org/?tree=Try&rev=ae4f013b9e98

Unfortunately previous runs are now gone, but I believe that the failures were a case of bug 980537. I think we should be good here now.

Master: https://github.com/mozilla-b2g/gaia/commit/e1c71d9e6d125292709b0ddf99ede6ae57c753fe

Comment 37

[Kevin Grandon :kgrandon]

(In reply to Kevin Grandon :kgrandon from comment #36)
> Let's try this again. Try run looks good to me here:
> https://tbpl.mozilla.org/?tree=Try&rev=ae4f013b9e98

Oops, wrong try link. The errors here were fixed in the gaia-try run: https://tbpl.mozilla.org/?rev=8fc4a40c69ad92862935a5a0c5295c4c25470af0&tree=Gaia-Try

Comment 38

[Kevin Grandon :kgrandon]

This is causing some mochitest failures. https://tbpl.mozilla.org/php/getParsedLog.php?id=49340388&tree=B2g-Inbound

14:03:03     INFO -  652 INFO TEST-UNEXPECTED-FAIL | /tests/toolkit/devtools/apps/tests/test_webapps_actor.html | Test timed out. - expected PASS
14:03:05     INFO -  653 INFO TEST-OK | /tests/toolkit/devtools/apps/tests/test_webapps_actor.html | took 302396ms
14:03:09     INFO -  -*- NetworkService: NetworkService shutdown
14:03:09     INFO -  JavaScript error: resource://gre/modules/IndexedDBHelper.jsm, line 121: UnknownError: The operation failed for reasons unrelated to the database itself and not covered by any other error code.

Backed out: https://github.com/mozilla-b2g/gaia/commit/1b0c386befd3f22f3f4991e9eff45ffb213b6adf

With that backout, and the fact that there's a nice shortcut to do this from the WebIDE now, I'm not going to pursue this anymore as there's much more important things to do. If anyone wants to pick this up, I'd suggest opening a new bug.