1057310 - [email/activesync] Duplicate case label in GELAM/js/activesfunc/folder.js

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Max Schaefer]

There is a case for 'asb.FileReference' both on line 1308 (https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/ext/mailapi/activesync/configurator.js#L1308) and on line 1322 (https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/ext/mailapi/activesync/configurator.js#L1322); the latter is unreachable.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Thanks for the report!

configurator.js is an optimized build-file, the true source code is: https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/activesync/folder.js#L782

It looks like this is harmless-ish, specifically, we have:

 var attachDataText = attachData.children.length ? attachData.children[0].textContent : null;

 case asb.FileReference:
 case em.AttName:
   attachment.part = attachDataText;
   break;

 case asb.FileReference:
 case em.Att0Id:
   attachment.part = attachData.children[0].textContent;
   break;

These three distinct case clauses probably want to be consolidated.

Comment 2

[Kevin Grandon :kgrandon]

It seems the linked files no longer exist, and the new configurator does not have this problem. I believe this was fixed in bug 885110.

Closing this out for now, but please re-open or file a new bug if this issue still exists. Thanks for the report.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Thanks for looking into this, Kevin!  Unfortunately the deficiency still exists.  The gaia path is now apps/email/js/ext/activesync/folder.js.  The GELAM path is now js/activesfunc/folder.js.

Comment 4

[Kevin Grandon :kgrandon]

Cool - thanks for the updated path :)

Comment 5

[Kevin Grandon :kgrandon]

Maybe I'll spin a quick patch here.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia-email-libs-and-more] KevinGrandon:bug_1057310_consolidate_switch_statement > mozilla-b2g:master

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:bug_1057310_update_gelam_folder > mozilla-b2g:master

Comment 8

[Kevin Grandon :kgrandon]

Comment on attachment 8570642 [details] [review]
[gaia-email-libs-and-more] KevinGrandon:bug_1057310_consolidate_switch_statement > mozilla-b2g:master

Andrew - does this look ok? If so, am I good to update gaia as well? Thanks!

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8570642 [details] [review]
[gaia-email-libs-and-more] KevinGrandon:bug_1057310_consolidate_switch_statement > mozilla-b2g:master

Yep, this is the right fix per my understanding of my comments in comment 1.  Thanks very much to both you and Max (and semmle; I enjoyed the brownbag!)

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

And a GELAM r+ is always good for propagation through to gaia.  (We get into trouble if we don't propagate promptly.  I am of course willing to cede the hassle of that to the great and powerful autolander, of course! :)

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8570644 [details] [review]
[gaia] KevinGrandon:bug_1057310_update_gelam_folder > mozilla-b2g:master

and I guess autolander might need my explicit r+ here?  just in case...

Comment 12

[Kevin Grandon :kgrandon]

Thanks Andrew. I'm going to try adding checkin-needed to see what happens with two pull requests from different repos here. I've never done this before, so I'm not sure what will happen.

Comment 13

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia-email-libs-and-more/commit/0107b93f3c72d99b82e2d56fd25cffb34722b836

Comment 14

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/560f9866381a5ab96306aa7acc3ff257a9836169

Comment 15

[Kevin Grandon :kgrandon]

(In reply to Kevin Grandon :kgrandon from comment #12)
> Thanks Andrew. I'm going to try adding checkin-needed to see what happens
> with two pull requests from different repos here. I've never done this
> before, so I'm not sure what will happen.

Seemed to work ok. Probably would not be ideal if one pull request failed, but I'll deal with that later!

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

Yeah, it seems to work well enough that I think the parallel landing will be our new workflow.  (Back when we checked the optimized byproducts into gaia, this would not have been safe, but now it should be pretty fine.)  I recently checked in :lightsofapollo's patch that makes TC re-run things up to four times for GELAM to get a success, which should probably be sufficient to deal with mozTCPSocket's non-trivial probability of segfaulting the process.

Comment 17

[Kevin Grandon :kgrandon]

Sounds good. The case I am worried about is if the bug is marked as resolved but not all patches succeeded in landing.

Anyway, give it a shot and if you run into some weirdness/difficulty please file a bug for me.