The original attachment menu markup was set as same level as view panels, which is not appropriate especially in new architecture. Maybe we should remove this static markup/specific attachment_menu.js and utilize option menu instead.

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master Hi Julien, it's a small polish that remove the existing attachment menu markup, since it's stay at the same level as the panel and we don't need to duplicate with markup/logic for thread/compose view if we could reuse the option menu component widget instead. I think this small clean up is also nice to have in current master so I create the patch ahead.

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master Hi Oleg, I think this patch is related to your path in bug 1156631 and maybe you can give some suggestion about this change.

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master Looks good, I like this cleanup! But there is one typo that breaks context menu for "other" attachment (e.g. contact attachment) and some old references to deleted files left. Thanks!

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master less code = julien is \o/ :-) (Even if I usually prefer preexisting markup code than inserted code) I left some comments that you can handle if you feel like it. I didn't do a full review because Oleg already did it. You can follow up with Oleg only :)

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master Hi Oleg, some nits/typo/clean up addressed with requestAttachment rewrite, thanks for the detailed review! BTW I also fixed the possible error in python test. Do you think we should ask for review by a-team? Or we could just landed it once GIP pass.

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master (In reply to Steve Chung [:steveck] from comment #6) > Comment on attachment 8595833 [details] > [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master > > BTW I also fixed the possible error in > python test. Do you think we should ask for review by a-team? Or we could > just landed it once GIP pass. Let's ask them if these tests are still maintained, why we didn't see any related failures on Treeherder (I believe some Python tests should have failed in the previous version of PR) and whether we should take care about such tests at all since I know they're going to be replaced with Marionette JS. Patch looks good, but some tests will always succeed + few more nits/ideas (commented at GitHub) and I think we're good to go :) Thanks for this cleanup!

Hey Johan, In previous version of the patch for this bug we removed some markup that was used in python test (namely test_sms_with_picture_attached.py), but Treeherder was actually green and we noticed that we should update python test only by accident :) So the questions are: * Does Treeherder run this test? * Is this test (and some other python Messages tests) still maintained and we should take care of it? If so, could you please take a look at the patch and check if python test changes seem OK for you? Thanks!

* Nope, this test actually sends an SMS, so it doesn't run against b2gdesktop[1] * Yes, the existing tests are still maintained. The QA team just doesn't add more tests in Marionette Python. I added some comments in the PR. [1] https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/functional/messages/manifest.ini#L25

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master Hi Oleg, thanks for discovering the bug for unit test, I'll create a follow up for the DOMREQUEST promise later. Hi Johnan, might need your feedback for python test changes here, thanks!

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master My bad, I made a mistake in my previous review. If we put self.root_element in a Wait, we'll ask marionette to search for this element every millisecond. I put a way to prevent this problem. Once that is done, I can f+!

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #11) > Comment on attachment 8595833 [details] > [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master > > My bad, I made a mistake in my previous review. If we put self.root_element > in a Wait, we'll ask marionette to search for this element every > millisecond. I put a way to prevent this problem. Once that is done, I can > f+! Updated per your suggestion, thanks!

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master LGTM!

Comment on attachment 8595833 [details] [gaia] steveck-chung:new-message-attachment-menu > mozilla-b2g:master Looks good to me now! Just one optional nit and the following question: I see there is one change in app behavior: previously when you choose "Replace attachment" and cancel activity you'll still see attachment menu, with the patch you won't see attachment menu anymore. I believe that your patch just unintentionally fixed this bug, but could you please confirm with Jenny? Thanks!

Thanks for the review! I just asked Jenny about the behavior changes. For the view action it's reasonable to remove the menu, but for replace action she would prefer to keep the menu if we cancel the pick acton so that user could make another pick acton easily. Since we don't have an option for the menu to close the menu manually, I'll file a bug for it.

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

Created Bug 1160049 for option menu behavior and Bug 1160054 for mozActivity migration