Closed
Bug 967516
Opened 11 years ago
Closed 10 years ago
Enable DEVICE_DEBUG by default for non production builds
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
2.1 S6 (10oct)
People
(Reporter: kgrandon, Assigned: kgrandon)
References
Details
(Whiteboard: [systemsfe])
Attachments
(3 files, 6 obsolete files)
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.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Comment on attachment 8370063 [details]
Step 2 - Land gaia patch
Alex - what are your thoughts here? Would you have time to review this one?
Attachment #8370063 -
Flags: review?(poirot.alex)
Comment 3•11 years ago
|
||
Comment on attachment 8370063 [details]
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.
Attachment #8370063 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
Going to fix indentation first in bug 967579.
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 8370063 [details]
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!
Attachment #8370063 -
Flags: review?(poirot.alex)
Comment 7•11 years ago
|
||
Comment on attachment 8370063 [details]
Step 2 - Land gaia patch
Please ensure Travis is green before landing, as that's not the case
and I can't get why...
Attachment #8370063 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 8•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 9•11 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 11•11 years ago
|
||
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.
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
Oops - better patch.
On try: https://tbpl.mozilla.org/?tree=Try&rev=b48a15df3cb8
Attachment #8395789 -
Attachment is obsolete: true
Assignee | ||
Comment 14•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #8370063 -
Attachment description: Github pull request → Step 2 - Land gaia patch
Assignee | ||
Updated•11 years ago
|
Attachment #8395798 -
Attachment description: Step 2 - Update test expected value → Step 3 - Update test expected value
Assignee | ||
Comment 15•11 years ago
|
||
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!
Attachment #8395796 -
Flags: review?(poirot.alex)
Assignee | ||
Updated•11 years ago
|
Attachment #8395798 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 16•11 years ago
|
||
New gaia PR, carrying review flag.
Attachment #8370063 -
Attachment is obsolete: true
Attachment #8395824 -
Flags: review+
Comment 17•11 years ago
|
||
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.
Attachment #8395798 -
Flags: review?(poirot.alex) → review-
Updated•11 years ago
|
Attachment #8395796 -
Flags: review?(poirot.alex)
Assignee | ||
Comment 18•11 years ago
|
||
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
Attachment #8395796 -
Attachment is obsolete: true
Attachment #8395798 -
Attachment is obsolete: true
Attachment #8396472 -
Flags: review?(poirot.alex)
Comment 19•11 years ago
|
||
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!
Attachment #8396472 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #8395824 -
Attachment description: Step 2 - Land gaia patch → Gaia patch
Assignee | ||
Comment 20•11 years ago
|
||
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!
Keywords: checkin-needed
Comment 21•11 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/36ed1dfdda41
Master: https://github.com/mozilla-b2g/gaia/commit/5ad38b98016c6b0eece65e7a41844043c2947c66
Keywords: checkin-needed
Assignee | ||
Comment 22•11 years ago
|
||
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
Status: REOPENED → ASSIGNED
Assignee | ||
Comment 23•11 years ago
|
||
The same patch as before, going to test this against gaia-try.
Assignee | ||
Comment 24•11 years ago
|
||
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•11 years ago
|
||
Assignee | ||
Comment 26•11 years ago
|
||
Assignee | ||
Comment 27•11 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Comment 28•11 years ago
|
||
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
Assignee | ||
Comment 30•11 years ago
|
||
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(kgrandon)
Resolution: FIXED → ---
Whiteboard: [systemsfe]
Assignee | ||
Comment 31•10 years ago
|
||
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.
Assignee | ||
Comment 32•10 years ago
|
||
Comment on attachment 8483839 [details]
Re-land Part 1 - Decouple screen.timeout from device debug
Carrying review.
Attachment #8483839 -
Flags: review+
Assignee | ||
Comment 33•10 years ago
|
||
Landed the first part in master which includes removing screen.timeout from the main patch: https://github.com/mozilla-b2g/gaia/commit/1c76fda5fa05cf352d36620fcc0784c6044ebf37
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Attachment #8395824 -
Attachment is obsolete: true
Updated•10 years ago
|
Target Milestone: --- → 2.1 S4 (12sep)
Assignee | ||
Comment 34•10 years ago
|
||
Here is a rebased part 2 of the original patch. I'm carrying the R+ as nothing has changed about the Makefile usage.
Attachment #8397459 -
Attachment is obsolete: true
Attachment #8496340 -
Flags: review+
Comment hidden (obsolete) |
Assignee | ||
Updated•10 years ago
|
Target Milestone: 2.1 S5 (26sep) → 2.1 S6 (10oct)
Assignee | ||
Comment 36•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 37•10 years ago
|
||
(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
Assignee | ||
Comment 38•10 years ago
|
||
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.
Resolution: FIXED → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•