1090867 - [Messages][Refactoring] Don't consider cancelled activity as unhandled error

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Oleg Zasypkin [:azasypkin]]

Currently when activity request is cancelled by user (via Cancel button in activity target selection menu) we log it as "Unhandled error spawning activity" without any useful details. This pollutes log with confusing non-valuable entries.

Cancelled activity case can be recognized like this "e.target.error.name === 'ActivityCanceled'" where "target" is the corresponding "MozActivity" instance. We can either put meaningful message to the log or just be silent in this case.

Unfortunately there is no any strict agreement on exception name when activity is cancelled by target activity itself during a normal app flow (eg. if user taps on back button in Gallery\Video\Music without selecting anything), though all currently available built-in apps use "pick cancelled" exception name in this case.

Comment 1

[Julien Wajsberg [:julienw]]

I'm quite sure there is an existing bug for this :) But can't find it right now.

Should we make it a mentored bug?

Comment 2

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #1)
> I'm quite sure there is an existing bug for this :) But can't find it right
> now.

Ah, haven't found it from the first attempt :( Will try one more time and dupe one of them in this case.

> Should we make it a mentored bug?

I thought that it may involve some changes in how "Compose.requestAttachment" processes "file too large" exception to always have the same error object structure in "onerror" callback, but probably it can be mentored as well :)

Comment 4

[Maxime Lo Re [:solfen]]

Hello, I've put some condition in the "onerror" callback so that it doesn't log anything if the user cancel the attachment. I didn't change how "Compose.requestAttachment" processes the "file too large" exception because in the unit tests it use the same patern (calling onerror with a simple string) and I thought it could be use like that in other part of the code.

Comment 5

[Maxime Lo Re [:solfen]]

Attachment: pull request github

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8515636 [details] [review]
pull request github

Hey Maxime,

Thanks for you contribution! I've left just two comments on Github mostly about unifying.

Please set r? again once you're ready and ping me if you have any questions!

Comment 7

[Maxime Lo Re [:solfen]]

Hello,
Sorry for taking this long (flashing my device and updating my linux took me a looong time), I've added some changes to the pull request and I had a problem that explained in a comment on the pull request.
Bye

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8515636 [details] [review]
pull request github

Hey Maxime,

(In reply to Maxime Lo Re [:solfen] from comment #7)
> Hello,
> Sorry for taking this long (flashing my device and updating my linux took me
> a looong time),

Don't worry and always take your time if you need it :)

> I've added some changes to the pull request and I had a
> problem that explained in a comment on the pull request.
> Bye

I've replied on the GitHub + some other nits, but overall it looks good!

Please set r? again, whenever you're ready.

Thanks!

Comment 9

[Maxime Lo Re [:solfen]]

Okay I've done the requested changes, let me know what you think :)
Bye

Comment 10

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8515636 [details] [review]
pull request github

Looks good to me! Just two tiny syntax nits on Github. 

Please, squash your commits and once nits are fixed and Treeherder is green add "checkin-needed" into bug "Keywords: " field.

Thanks a lot for contribution!

Comment 11

[Maxime Lo Re [:solfen]]

It's done. 
Thanks a lot for all the help !

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Linter failures on the Gaia Try run.

Comment 13

[Maxime Lo Re [:solfen]]

Apparently on B2G desktop *64 DOMError is not defined, this is weird since there is no problem when testing on a phone. Any idea why ?

Comment 14

[Oleg Zasypkin [:azasypkin]]

(In reply to Maxime Lo Re [:solfen] from comment #13)
> Apparently on B2G desktop *64 DOMError is not defined, this is weird since
> there is no problem when testing on a phone. Any idea why ?

Hey Maxime,

I believe you just need to add new line with "DOMError" to JSHint config here in [1] to make linter aware about this type.

[1] https://github.com/mozilla-b2g/gaia/blob/1dd8ad8f96988afebc9691e1b818fa37aa32c790/apps/sms/js/compose.js#L10

Comment 15

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL (rebased)

Hey Maxime,

I took the liberty of adding "DOMError" string to the JSHint annotation that I mentioned in comment 14 + rebased your PR on the latest master to get the latest Threeherder results.

Obviously you're preserved as patch\commit author, I've just done some bureaucratic stuff for you :)

Thanks for the great work!

Comment 16

[Oleg Zasypkin [:azasypkin]]

Treeherder is green now!

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

Comment 17

[Maxime Lo Re [:solfen]]

Okay thanks a lot ^^