Partially related to https://bugzilla.mozilla.org/show_bug.cgi?id=1101994, we are going to have a hard time enabling logshake by default since its pretty much impossible to only trigger it when the user wants to (android introduced then had to disable the same feature) Alternative is to have a hardware button trigger similiar to screenshots but use power up, this could be enabled by default, Alexandre how does that sound?

(In reply to Dale Harvey (:daleharvey) from comment #1) > Partially related to https://bugzilla.mozilla.org/show_bug.cgi?id=1101994, > we are going to have a hard time enabling logshake by default since its > pretty much impossible to only trigger it when the user wants to (android > introduced then had to disable the same feature) Maybe someone should just fix this ? I don't have time right now. > > Alternative is to have a hardware button trigger similiar to screenshots but > use power up, this could be enabled by default, Alexandre how does that > sound? That's just another way to trigger, that should be fine.

(In reply to Dale Harvey (:daleharvey) from comment #1) > Partially related to https://bugzilla.mozilla.org/show_bug.cgi?id=1101994, > we are going to have a hard time enabling logshake by default since its > pretty much impossible to only trigger it when the user wants to (android > introduced then had to disable the same feature) > > Alternative is to have a hardware button trigger similiar to screenshots but > use power up, this could be enabled by default, Alexandre how does that > sound? That being said, considering the number of times I accidently took screenshots of my pockets, moving from one way to trigger to another way to trigger may not be enough. The current heuristic to trigger logshake is way too trivial, and that explains why we have bug 1101994. But before complaining and adding another way to enable the feature, we should fix this one before. I've provided an app on the bug to be able to record shaking movements and experiment new heuristics.

> The current heuristic to trigger logshake is way too trivial, and that explains > why we have bug 1101994. But before complaining and adding another way to enable > the feature, we should fix this one before. I dont think its possible to fix. I shake my phone for a lot of reasons that arent to file bugs. I had to disable this functionality on android and I believe they ended up removing it by default for the same reason. > That being said, considering the number of times I accidently took screenshots of my pockets, > moving from one way to trigger to another way to trigger may not be enough. Our screenshot triggering was broken, but I fixed that and since then have had no complaints about accidental screenshots

(In reply to Dale Harvey (:daleharvey) from comment #4) > > The current heuristic to trigger logshake is way too trivial, and that explains > > why we have bug 1101994. But before complaining and adding another way to enable > > the feature, we should fix this one before. > > I dont think its possible to fix. I shake my phone for a lot of reasons that > arent to file bugs. I had to disable this functionality on android and I > believe they ended up removing it by default for the same reason. Make the way to trigger configurable ? Key combos or shaking, for example ? > > > That being said, considering the number of times I accidently took screenshots of my pockets, > > moving from one way to trigger to another way to trigger may not be enough. > > Our screenshot triggering was broken, but I fixed that and since then have > had no complaints about accidental screenshots

This should probably be done around https://dxr.mozilla.org/mozilla-central/source/b2g/components/LogShake.jsm#120

Sweet cheers

Thats the gaia side attached, Alexandre you got a pointer to implementing the gecko side of this, can I have LogShake.jsm listen to the mozContentEvent directly?

Is there any reason for doing this detection on Gaia's side ? Specifically, there was discussion on this point back in bug 1019816 comment 41, and the whole detection logic was moved into Gecko upon Fabrice's suggestions.

(In reply to Dale Harvey (:daleharvey) from comment #9) > Thats the gaia side attached, Alexandre you got a pointer to implementing > the gecko side of this, can I have LogShake.jsm listen to the > mozContentEvent directly? I was suggesting to implement your Gaia PR in Gecko, indeed :). Regarding the mozContentEvent, again, fabrice suggested to avoid using this and create a specific event. That being said, you should be able to listen those yes.

> Is there any reason for doing this detection on Gaia's side ? The rest of the hardware key state handling is in that code, we have an fsm so we can deal with conflicting shortcuts, random pressed buttons and overriding the behaviour so it seems by far the best place to do it. If Fabrice agrees it looks like I can take your code + the comments and implement the gecko side. Fabrice, in https://bugzilla.mozilla.org/attachment.cgi?id=8585813 I implemented a volume up + sleep shortcut to capture the system logs (previous implementation was to shake, preffed off), Alexandre mentioned you were concerned about having the event capturing in gaia, however I it seems like the best place to me, it matches the screenshot implementation and is the module thats responsible for handling the hardware keys, having the individual modules (logshake) deal with its own event capturing of hardware keys seems like it would lead to bugs, you ok with this approach?

I'm worried about doing the data capture in gaia and send it to gecko. Just passing events around is fine. Just don't send vanilla "mozChromeEvent" from gecko to gaia (we ended up with so many listeners checking for `type` that we were regressing app launch time). I think we always send mozContentEvent from gaia to gecko, which is not great either so I would not mind starting a better trend here!

Comment on attachment 8585813 [details] [gaia] daleharvey:1136777 > mozilla-b2g:master Code looks good w/o thinking if the feature and the key combination make sense (should I consider that as well?).

Comment on attachment 8587049 [details] [diff] [review] Bug 1136777 - Enable LogShake by default and listen for new content events Review of attachment 8587049 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, just a nit, but it's good :) ::: b2g/chrome/content/settings.js @@ +208,1 @@ > SettingsListener.observe('devtools.logshake', false, (value) => { So the pref will control whether we can shake to get logs :) ::: b2g/components/LogShake.jsm @@ +55,5 @@ > */ > const EXCITEMENT_THRESHOLD = 500; > const DEVICE_MOTION_EVENT = "devicemotion"; > const SCREEN_CHANGE_EVENT = "screenchange"; > +const CAPTURE_LOGS_CONTENT_EVENT = "requestSystemLogs"; nit: maybe we should be consistent in the naming, and we should have 'request-system-logs'. @@ +190,5 @@ > > var excitement = acc.x * acc.x + acc.y * acc.y + acc.z * acc.z; > > if (excitement > EXCITEMENT_THRESHOLD) { > + this.startCapture(); Ok, just moving this out.

Comment on attachment 8585813 [details] [gaia] daleharvey:1136777 > mozilla-b2g:master Thanks, that looks good but I'm not a peer for this.

Cool, there are no harmful side effects to adding this shortcut without Gecko support, so taking tims review Green @ https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=fbb7003702d3f8f816cf60367e1ace4d6edbcdbd Landed in https://github.com/mozilla-b2g/gaia/commit/c2b0b11d5ea94c03e08d51d823f8fbd2fa86a7fc

Comment on attachment 8587049 [details] [diff] [review] Bug 1136777 - Enable LogShake by default and listen for new content events Review of attachment 8587049 [details] [diff] [review]: ----------------------------------------------------------------- In https://mxr.mozilla.org/mozilla-central/source/b2g/components/LogShake.jsm#192 we disable and enable the deviceMotionListener based on the screen state. It looks like this will interfere with the settings based enabling. I'm happy to be proven wrong but can you double check? ::: b2g/chrome/content/settings.js @@ +208,1 @@ > SettingsListener.observe('devtools.logshake', false, (value) => { nit: |value => { ... | ::: b2g/components/LogShake.jsm @@ +195,5 @@ > + } > + }, > + > + startCapture: function() { > + if (!this.captureRequested) { do an early return instead: if (this.captureRequested) { return; }

Yeh cheers, this shouldnt have caused any problems since the setting should likely only be enabled while the screen was enabled, but if a background process managed to trigger the change somehow it could have been enabled without the screen. I think its clearer now, we only start the device motion listened if there is not one currently attached + the screen is enabled + the setting is enabled

Pushed to try just in case - https://treeherder.mozilla.org/#/jobs?repo=try&revision=71beb47a3177

Thanks Fabrice Merged to b2g-inbound: https://hg.mozilla.org/integration/b2g-inbound/rev/81c856a268e7

Reopened as these should be closed when the hit central, not inbound

I couldn't find any references to updated documentation or updated documents, so I've documented this at https://developer.mozilla.org/en-US/Firefox_OS/Debugging/On-device_console_logging#Hardware_buttons_%28VolumeUp.2Bsleep%29_to_save_system_log by the existing mention of "shake to log".