Lots of perf improvements, quite big, were done on this app recently, and it's quite difficult to uplift them one by one because we didn't do that progressively as we landed in master. Also, this app is (probably) not concerned by the certification. Lucas Adamski, by mail, said it was a good idea. Some perf improvements are still coming, so we have here imho 2 questions: 1. do we uplift this in v1-train and v1.0.1 ? 2. if yes, when do we do that ?

Yeah I think we need to do this for email, even though gives me shivers. Lets do this immediately to get as much soak time is possible. Yes, there isn't really anything certification related here so the desire to improve email performance in v1.0.1 is purely for benefit of the end-user.

Will do that as soon as Bug 850904 and (I think) Bug 822882 are fixed, hopefully tomorrow morning CET.

Shiver me timbers, lets uplift this thang!

Marking bug 822882 as dependent but we don't need to wait on bug 850904 (that bug is already present on v1.0.1, so we're not making things any worse by bringing it up from master again).

I'm considering the bug 822882 regression fixed, so we're ready for an exciting wave of uplifts. v1.0.1 is not currently planned to have some of the changes to other apps relating to web activities for attachments, so we will need to ensure that: - in message-cards.js, the regexp that whitelists mime types should be reduced to only include image types because audio will not be supported by the audio app. So the clause for "/^audio\//.test(attachment.mimetype)" should go away. - in compose-cards.js, the onAttachmentAdd method should not be added as a click listener to .cmp-attachment-container nodes. Likewise, the '+' to add an attachment should not be visible, so the cmp-attachment-add node should either be removed or marked collapsed. And as noted elsewhere, we may need to fork copies of any building blocks we use into the e-mail app itself so we look right but don't regress other apps. I had already done this for v1.0.1 for the progress activity building block. (Which we just uplifted entirely for v1.1) It hasn't landed yet, but soon we'll have some changes to handling the addition of contacts which would require uplifted changes to the contacts app, so if that gets landed and we do a bulk uplift after that, we'd need to reduce the set of menu items that are enabled since the contacts app won't be able to service some of the new activity variations we send it.

v1.0.1: cf0d5b7527cc6a321f166d0c6f390787e325a894 should we do the same to v1-train ?

Yes, same on v1-train, but there is no need to disable any features for v1-train, although we do need to uplift the things affecting other apps that are leo+, like the gallery/camera/wallpaper fixes in bug 838009.

ok so we need to wait for that this bug is fixed, right ?

That bug already has a patch landed; unfortunately there needs to be a follow-up patch landed to implement video attachment support and it currently is planned to happen on the same bug because it's leo+.

I'd like to wait that it is completely finished :)

Who's going to take this?

Oops, Julien is taking care of it, we just forgot to assign.

This added strings on 1.0.1, which I'm going to publish to localizers now, also for v1-train, even though the patch didn't land there yet.

I didn't have time to fully look at this bug on friday, I had compiled and installed the specific 1.0.1 build based on Comment 6. It took longer than I had anticipated. Going to be doing testing today around the fixes.

Julien, did this land on v1-train as well as v1.0.1? (we need to set status-b2g18 if it landed on v1-train)

Not on v1-train yet; likely tomorrow.

Thanks for the update!

Regression 1 from the patch: For v1.0.1 having the attachments line, I believe this is a bug. it serves no ui purpose though when you create an email and you can't click on it. End users will think it's a bug because it's visible with no functionality. You have to go through gallery in order to add a picture.

regression 2: alignment issues with attaching an image from gallery with the attachments line. (this has been a known issues with the other lines [to, cc, bcc, subject before.])

To note : Most of what I tested was with attachments via gallery and deleting them, basic functionality such as using an existing contact, composing an contact, composing an email, reading an email, downloading and viewing an attachment. selecting a http link, using imap, as well as some error types when trying to configure the account. using English as well as French I did not try arabic as that's not a shipping language. I am unsure how to select and send multiple images from the device, I did receive multiple attachments. I also checked any other bug that I ran into with v1 train and if it also occurred in v1 train I reported them separately. I am blocked in testing hotmail/activesync: bug 852348 note there are bugs that already exist such as bug 848266 These are the only two things I encountered specific to this code change thus far.

Clarification on "these are the two issues I ran across...": That's in regards to the 2 screenshots.

(In reply to Naoki Hirata :nhirata from comment #18) > > For v1.0.1 having the attachments line, I believe this is a bug. it serves > no ui purpose though when you create an email and you can't click on it. > End users will think it's a bug because it's visible with no functionality. > You have to go through gallery in order to add a picture. I asked Andrew about that and he told me to keep it because we need this line when we attach an image from the gallery. I guess showing it or not depending on the case would take some specific code. If you thing we need it, the best would be to file a new bug and ask for tef?.

(In reply to Naoki Hirata :nhirata from comment #19) > Created attachment 726406 [details] > screenshot of attachment > > regression 2: alignment issues with attaching an image from gallery with the > attachments line. (this has been a known issues with the other lines [to, > cc, bcc, subject before.]) Andrew, is it a BB issue ? is it possible to fix this with a small CSS change in email only ? Again, I guess the best is to file another bug for that.

(In reply to Julien Wajsberg [:julienw] from comment #22) > I guess showing it or not depending on the case would take some specific > code. If you thing we need it, the best would be to file a new bug and ask > for tef?. We don't need to support attachments initiated from the email app for 1.0.1 -- we were hoping to bring it up and get it for free since that work is done in master but it's not a requirement. But we do need to hide the "Attachments" field from compose view if it doesn't work.

(In reply to Dylan Oliver [:doliver] from comment #24) > (In reply to Julien Wajsberg [:julienw] from comment #22) > > I guess showing it or not depending on the case would take some specific > > code. If you thing we need it, the best would be to file a new bug and ask > > for tef?. > > We don't need to support attachments initiated from the email app for 1.0.1 > -- we were hoping to bring it up and get it for free since that work is done > in master but it's not a requirement. But we do need to hide the > "Attachments" field from compose view if it doesn't work. What I'm saying is that this field is used when we have an attachment initiated from the Gallery. That's why it's not as simple as just hiding it in HTML, because we need it sometimes. And that's why I'm saying it's better to handle this in another bug, because we'll need a patch.

(In reply to Julien Wajsberg [:julienw] from comment #23) > > regression 2: alignment issues with attaching an image from gallery with the > > attachments line. (this has been a known issues with the other lines [to, > > cc, bcc, subject before.]) > > Andrew, is it a BB issue ? is it possible to fix this with a small CSS > change in email only ? I haven't investigated; I've been leaving such things up to the visual design team. > Again, I guess the best is to file another bug for that. This sounds reasonable, yes.

v1-train: 53d1cc8ad2920d1b5b9a950c100864c2f247345b I've seen that I left some useless files in v1.0.1 (because they were deleted in master so they're not used anymore). I correctly removed them in v1-train. Is it too late to remove them in v1.0.1 too or should I go ahead ? This is very easy to do that using git diff : git diff --name-only --diff-filter A master apps/email | xargs git rm (the filter "A" means to show only "added" files, ie the files that are new in this branch when compared to master)

v1.0.1: 5bfcadc1e62c5b4097acb821c5ce567683b05917 follow-up commit to remove the unwanted files All seemed to work for me but please test it :)

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Jeni> nothing special here.

Julien Wajsberg if you can Verify this bug out, it would be much appreciated.

I don't exactly get what I should do here. I'm not in QA so I don't want to badly do this important task. Please verify that the app is working in those branches and change the status if it seems ok.

This seems more of a process bug than something to directly verify beyond the fact that the code was landed and email works in general. Any issues that arose once email was uplifted and in basic working condition should have been filed individually and addressed individually.

According to comment 33,delete keywords verifyme.