Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Description • 7 years ago

Firefox Puppeteer has a wrapper for `self.marionette.session_capabilities['platformName']` which can be accessed via `self.puppeteer.platform`. I would propose to get rid of this extra layer which doesn't serve underlying purpose of puppeteer for ui classes. 

The property can be found here:

https://dxr.mozilla.org/mozilla-central/rev/3b96f277325747fe668ca8cd896d2f581238e4ee/testing/marionette/puppeteer/firefox/firefox_puppeteer/puppeteer.py#53

Also the files in /testing/firefox-ui have to be updated for this change.

Please see the following document in how to get started:
https://wiki.mozilla.org/User:Mjzffr/New_Contributors

	dsmith16.mozilla
	Comment 1 • 7 years ago

I'm a student from Western Oregon University and my group and in Open Source Software Development would like to claim this bug if possible.

	dsmith16.mozilla
	Comment 2 • 7 years ago

(In reply to dsmith16.mozilla from comment #1)
> I'm a student from Western Oregon University and my group and in Open Source
> Software Development would like to claim this bug if possible.

Correction: I'm a student from Western Oregon University and my group and I in Open Source Software Development would like to claim this bug if possible.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 3 • 7 years ago

Feel free to work on it. Once a patch has been submitted via mozreview I will assign you to this bug. Otherwise let me know if you have questions.

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8869938 [details]
Bug 1364349 - Removed Puppeteer.platform and references as requested.

https://reviewboard.mozilla.org/r/141506/#review146084

::: commit-message-9851f:1
(Diff revision 1)
> +Bug1364349  - 'Removed 'Puppeteer.platform' wrapper in favor of 'marionette.session_capabilities['platformName']'' r=hskupin@gmail.com

Please fix this commit message.  There should be a space after “Bug”, not two spaces but one before the dash.  Then the summary should not be wrapped in single quotes, and it should ideally not be longer than ~50 characters.  Finally, the reviewer should be listed as "r=whimboo" or "r?whimboo".  mozreview will rewrite the commit message from "r?" to "r=" when the changeset is sent to Autoland.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8869938 [details]
Bug 1364349 - Removed Puppeteer.platform and references as requested.

https://reviewboard.mozilla.org/r/141506/#review146108

Please note that when you want to have a review you should set the reviewer correctly in the commit message, or within mozreview directly.

The patch looks fine, but would need small updates.

::: testing/firefox-ui/tests/puppeteer/test_page_info_window.py:72
(Diff revision 1)
>                             'shortcut',
>                             opener,
>                             )
>  
>          for trigger in open_strategies:
> -            if trigger == 'shortcut' and self.puppeteer.platform == 'windows_nt':
> +            if trigger == 'shortcut' and self.marionette.session_capabilities['platformName'] == 'windows_nt':

This line is too long and will cause a linting failure. Please wrap it appropriately.

::: testing/firefox-ui/tests/puppeteer/test_page_info_window.py:95
(Diff revision 1)
>                              'shortcut',
>                              closer,
>                              )
>          for trigger in close_strategies:
>              # menu only works on OS X
> -            if trigger == 'menu' and self.puppeteer.platform != 'darwin':
> +            if trigger == 'menu' and self.marionette.session_capabilities['platformName'] != 'darwin':

Same here. Please wrap the line.

	dsmith16.mozilla
	Comment 7 • 7 years ago

OK, I'll have a corrected patch up sometime in the next couple days. I'll also make sure the commit message matches the guidelines you provided. Thanks much!

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8869938 [details]
Bug 1364349 - Removed Puppeteer.platform and references as requested.

https://reviewboard.mozilla.org/r/141506/#review147774

Sorry, but the indentation is still wrong. I would suggest that you install a linter (like flake8) locally and let it check the modified files. Also please have a look at the commit message again. It should contain the `what` has been changed in the summary, and `why` in the details. If you need help with the latter please let me know on IRC and we can figure it out.

	dsmith16.mozilla
	Comment 11 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 8869938 [details]
> Bug 1364349 - Removed Puppeteer.platform and references as requested.
> 
> https://reviewboard.mozilla.org/r/141506/#review147774
> 
> Sorry, but the indentation is still wrong. I would suggest that you install
> a linter (like flake8) locally and let it check the modified files. Also
> please have a look at the commit message again. It should contain the `what`
> has been changed in the summary, and `why` in the details. If you need help
> with the latter please let me know on IRC and we can figure it out.

Updated. I ran flake8 over the files before submission. Hopefully, the commit message is acceptable. I tried to keep it at ~50 characters as recommended by :ato (currently 54 characters with spaces). Let me know if I missed anything or how I can improve the commit message if necessary. 

Thank you so much for all your patience and help with this process!

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8869938 [details]
Bug 1364349 - Removed Puppeteer.platform and references as requested.

https://reviewboard.mozilla.org/r/141506/#review150274

::: commit-message-9851f:1
(Diff revisions 2 - 3)
> -Bug 1364349 - Removed wrapper and references in found firefox-ui r?whimboo
> +Bug 1364349 - Removed Puppeteer.platform and references as requested. r?whimboo

The description of the commit should contain what has been changed. "as requested" doesn't give any value for a person looking just at the commit message.

Please try to make it clearer.

::: testing/firefox-ui/tests/puppeteer/test_page_info_window.py:74
(Diff revisions 2 - 3)
>                             )
>  
>          for trigger in open_strategies:
> -            if (trigger == 'shortcut' 
> -                    and self.marionette.session_capabilities['platformName'] 
> +            if (trigger == 'shortcut'
> +                    and self.marionette.session_capabilities['platformName']
>                      == 'windows_nt'):

I still don't like the wrapping here, and I doubt the linter will be also happy about.

So better define a variable before the `for` loop, and use it here.

::: testing/firefox-ui/tests/puppeteer/test_page_info_window.py:99
(Diff revisions 2 - 3)
>                              )
>          for trigger in close_strategies:
>              # menu only works on OS X
> -            if (trigger == 'menu' 
> -                    and self.marionette.session_capabilities['platformName'] 
> +            if (trigger == 'menu'
> +                    and self.marionette.session_capabilities['platformName']
>                      != 'darwin'):

Same as above.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 13 • 7 years ago

dsmith, do you have an update for us? It would be great to get this finally landed. Thanks.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 14 • 7 years ago

No reponse from assignee. I'm going to put this bug back into the pool for someone else to pick up the work.

	Henrik Skupin [:whimboo][⌚️UTC+1] Reporter
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8911713 [details]
Bug 1364349 - Removed platform property of Firefox Puppeteer.

https://reviewboard.mozilla.org/r/183114/#review188324

Code-wise it looks fine. Just update the commit message and we can get it landed. Meanwhile I will trigger a try build so we can ensure nothing will be broken with the landing.

::: commit-message-7e962:1
(Diff revision 1)
> +Bug 1364349 - Removed the wrapper puppeteer.platform in Firefox UI Tests. r=whimboo

I would say "Bug 1364349 - Removed platform property of Firefox Puppeteer".

::: commit-message-7e962:3
(Diff revision 1)
> +Bug 1364349 - Removed the wrapper puppeteer.platform in Firefox UI Tests. r=whimboo
> +
> +The wrapper puppeteer.platform in Firefox UI tests was removed and replaced with marionette.session_capabilities['platformName'].

For the detailed description we usually explain why it was necessary. So as best just say that this wrapper is not needed anymore, or like in this case we won't need a detailed description at all.

	Pulsebot
	Comment 18 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/49c4cf5d041d
Removed platform property of Firefox Puppeteer. r=whimboo

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/49c4cf5d041d