1141266 - execute_script calls should not use shortcuts for Cc, Ci, Cr, and Cu

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Given that the mentioned shortcuts are not defined in all type of windows, we should not use them at all in our libraries. To be consistent lets change them all:

Cc. => Components.classes.
Ci. => Components.interfaces.
Cu. => Components.utils

For the fix please create a pull request on Github, and add a text attachment which contains the plain link to the PR to this bug. Ask for review from me or :chmanchester.

Comment 1

[naru.aditya]

Hello, I would like to work on this. Could you tell me on how to proceed? I've the basic build of mozilla-central setup. I just switched to mac from Ubuntu so I would like to know if I'd need new tools too!

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Naru! Thank you for your interest. Regarding your questions, this bug is not about code in mozilla-central, but it uses its own repository. See the URL as attached to the URL field above.

In regards of tools I would suggest that you use virtualenv to get all necessary packages installed, which will be done by `python setup.py develop` once you forked, and cloned the repo locally.

Comment 3

[naru.aditya]

Okay; forked, cloned and setup is done!

Comment 4

[naru.aditya]

Found the shortcuts used. Should Cr also be changed? And the shortcuts are to be changed in every file that gives a call to execute_script call right?
For eg:

    def user_agent(self):
        with self.marionette.using_context('chrome'):
            return self.marionette.execute_script("""
              return Cc["@mozilla.org/network/protocol;1?name=http"]
                     .getService(Ci.nsIHttpProtocolHandler)
                     .userAgent;
            """)

should become,

    def user_agent(self):
        with self.marionette.using_context('chrome'):
            return self.marionette.execute_script("""
              return Cc["@mozilla.org/network/protocol;1?name=http"]
                     .getService(Components.interfaces.nsIHttpProtocolHandler)
                     .userAgent;
            """)

or am I mistaken?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to naru.aditya from comment #4)
> Found the shortcuts used. Should Cr also be changed?

Oh yes! Thanks for seeing this. It's also a shortcut which can cause breakage maybe even more often than the other ones.

> And the shortcuts are to be changed in every file that gives a call to execute_script call right?

Yes, those are Javascript specific. Only execute_script and execute_ascync_script make use of them.

> should become,
> 
>     def user_agent(self):
>         with self.marionette.using_context('chrome'):
>             return self.marionette.execute_script("""
>               return Cc["@mozilla.org/network/protocol;1?name=http"]
>                      .getService(Components.interfaces.nsIHttpProtocolHandler)
>                      .userAgent;
>             """)

Yes, but don't forget the Cc here too. For the correct indentation please see all the other instances which have already been fixed by other patches.

Comment 6

[naru.aditya]

How do I check if the changes I made have not broken the code but done the right thing?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You can run all the tests locally via 'firefox-ui-tests --binary %path/to/firefox%'. If all is passing you can create a pull request on github. Then Travis will also check your changes on Linux and OS X.

Comment 8

[naru.aditya]

(In reply to Henrik Skupin (:whimboo) [away 03/25 - 03/31] from comment #7)
> You can run all the tests locally via 'firefox-ui-tests --binary
> %path/to/firefox%'. If all is passing you can create a pull request on
> github. Then Travis will also check your changes on Linux and OS X.


When I run the test on the unchanged repo, I get 

0:00.00 LOG: MainThread ERROR Failure during execution of the update test.

with, AttributeError: 'NoneType' object has no attribute 'update'

Are there any dependencies that I am missing or arguments perhaps?

Comment 9

[naru.aditya]

firefox-ui-tests throw up errors even on an unchanged repo cloned from git. Every test fails. Is something to be changed?

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Hi, Henrik is out of office for a few weeks, but maybe I can help. There were a few recent changes that require a clean install. If you run |python setup.py develop| in the firefox-ui-tests directory and provide a recent nightly to the --binary, does the error persist? If so, can you post a full stacktrace here? Thanks!

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Naru, were Chris' comments helpful for you? Have you had a chance to test them out?

Comment 12

[Aditya Motwani (:AdityaM)]

Hi I would like to work on this Bug. Can this be assigned to me?
I also have a question as this is my first bug fix.

1)How many files are there which use the above variables? I also noticed that the bug is fixed in the 'user_agent' method.

Awaiting your reply. :)

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Aditya. I have already Utvar who is currently marked as assignee on bug 1207689 to most likely work on this bug given that he already started with the conversion of one of those methods.

Would you mind taking a different bug? Something like bug 1209087?

Comment 14

[utvar]

Whimboo, to clarify, do you still want the following changed in all firefox-ui-tests libraries? 

Cc. => Components.classes.
Ci. => Components.interfaces.
Cu. => Components.utils

Also, in the above comments, I saw mention of "Cr" -- but nothing came up on a search for this within firefox-ui-tests. Do I need to fix these, and if so, where are they located?

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

That is right. All of those shortcuts should be expanded to the full length. If 'Cr' is not present you won't have to do any action for it.

Comment 16

[utvar]

Attachment: github pull request

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Looks great to me.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

PR got merged as:
https://github.com/mozilla/firefox-ui-tests/commit/6bc75867b61523431ab311cf52ca87115edd1e8c

I don't see a need yet to get this patch backported to older branches. If we have to, it can be done later on.

Thanks you utvar for the patch! Please let me know if you want to work on something else.