Closed Bug 879386 Opened 8 years ago Closed 8 years ago

[mozprofile][mozrunner] Add support for application type Firefox Metro

Categories

(Testing :: Mozbase, defect)

All
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=3][mozmill-2.0])

Attachments

(1 file, 2 obsolete files)

To be able to natively support Firefox Metro builds we have to update mozprofile and mozrunner with the appropriate classes. I will have a patch soon, only struggling with the default prefs for those profiles at the moment.
Attached patch Patch v1 (obsolete) — Splinter Review
So this patch adds support for Firefox Metro mode on Windows 8. The included metrotestharness.exe application has been taken from the latest Firefox tests zip file from the FTP server.

Not sure if all the code pieces are at the right place but at least for the exception I tried to raise it before we call the actual Runner.__init__() method, but that fails with another exception, so it's not that helpful. Please let me know what you think.
Attachment #758194 - Flags: review?(jhammel)
Comment on attachment 758194 [details] [diff] [review]
Patch v1

Review of attachment 758194 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozprofile/mozprofile/profile.py
@@ +3,5 @@
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +__all__ = ['Profile',
> +           'FirefoxProfile', 'FirefoxMetroProfile',
> +           'ThunderbirdProfile']

I'd prefer if each was on one line per attribute

@@ +327,5 @@
> +                   # Don't report telemetry information
> +                   'toolkit.telemetry.enabled' : False,
> +                   'toolkit.telemetry.enabledPreRelease' : False,
> +                   }
> +

Is there a reason that A. this inherits from Profile vs FirefoxProfile? and B. that we copy and paste prefs vs copy + extend/pop?  I am concerned that currently there is a central place to add a firefox preference; now there will be two if it is a shared preference.  IMHO it is much better to inherit from FirefoxProfile and add/remove prefs from a central set.  At some point in time (soon?) prefs will be removed from these files, but we can cross that bridge then.

::: mozrunner/mozrunner/runner.py
@@ +5,5 @@
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +__all__ = ['CLI', 'cli', 'Runner', 'runners', 'package_metadata',
> +           'FirefoxRunner', 'FirefoxMetroRunner',
> +           'ThunderbirdRunner']

Again, I'd rather have these one/line

@@ +268,5 @@
> +    def __init__(self, profile, binary=None, cmdargs=None, **kwargs):
> +
> +        # take the binary from BROWSER_PATH environment variable
> +        if (not binary) and 'BROWSER_PATH' in os.environ:
> +            binary = os.environ['BROWSER_PATH']

I'd do:

binary = binary or os.environ.get('BROWSER_PATH')

@@ +271,5 @@
> +        if (not binary) and 'BROWSER_PATH' in os.environ:
> +            binary = os.environ['BROWSER_PATH']
> +
> +        cmdargs = cmdargs or []
> +        cmdargs.extend(['-firefoxpath', binary])

What if binary is None? (as will be the case when binary is not passed in and os.environ doesn't contain BROWSER_PATH

@@ +276,5 @@
> +
> +        Runner.__init__(self, profile, immersiveHelperPath, cmdargs, **kwargs)
> +
> +        if not mozinfo.isWin:
> +            raise Exception('Firefox Metro mode is only supported on Windows 8 and onwards.')

I would put this check earlier; is there any reason it is this late?
Comment on attachment 758194 [details] [diff] [review]
Patch v1

Please address the review concerns, particularly the inheritence and prefs issue regarding the new profile class.  More documentation, etc, would be appreciated as well.  Will the new helper .exe be included in github?  If so, documenting what it does (and why) would be nice too.
Comment on attachment 758194 [details] [diff] [review]
Patch v1

Review of attachment 758194 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozprofile/mozprofile/profile.py
@@ +3,5 @@
>  # You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +__all__ = ['Profile',
> +           'FirefoxProfile', 'FirefoxMetroProfile',
> +           'ThunderbirdProfile']

Sure, I can do.

@@ +327,5 @@
> +                   # Don't report telemetry information
> +                   'toolkit.telemetry.enabled' : False,
> +                   'toolkit.telemetry.enabledPreRelease' : False,
> +                   }
> +

Firefox Desktop and Firefox Metro are completely different applications, even they have the same version of Gecko underneath. I really subclassed from the base Profile class to not introduce any broken cross-application behaviors. There might be some preferences which are the same, but as of now I would really suggest we keep those separated.

::: mozrunner/mozrunner/runner.py
@@ +268,5 @@
> +    def __init__(self, profile, binary=None, cmdargs=None, **kwargs):
> +
> +        # take the binary from BROWSER_PATH environment variable
> +        if (not binary) and 'BROWSER_PATH' in os.environ:
> +            binary = os.environ['BROWSER_PATH']

That's the code I have taken from FirefoxRunner. If we want to change that we should do it on both locations. Shall I do that?

@@ +271,5 @@
> +        if (not binary) and 'BROWSER_PATH' in os.environ:
> +            binary = os.environ['BROWSER_PATH']
> +
> +        cmdargs = cmdargs or []
> +        cmdargs.extend(['-firefoxpath', binary])

I could copy the code we have in Runner.__init__ and which checks for the binary to be set and exists. Would that work? I'm not that happy with that given that on other places we might not get all the binary information e.g. application.ini because we specify the harness helper now. Let me play around with that and see if we can leave the behavior but when we actually call the command that I can inject the appropriate options.

@@ +276,5 @@
> +
> +        Runner.__init__(self, profile, immersiveHelperPath, cmdargs, **kwargs)
> +
> +        if not mozinfo.isWin:
> +            raise Exception('Firefox Metro mode is only supported on Windows 8 and onwards.')

See my comment inside the patch request.
(In reply to Henrik Skupin (:whimboo) from comment #4)
> Comment on attachment 758194 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 758194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozprofile/mozprofile/profile.py
> @@ +3,5 @@
> >  # You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> > +__all__ = ['Profile',
> > +           'FirefoxProfile', 'FirefoxMetroProfile',
> > +           'ThunderbirdProfile']
> 
> Sure, I can do.

Cool; thanks.

> @@ +327,5 @@
> > +                   # Don't report telemetry information
> > +                   'toolkit.telemetry.enabled' : False,
> > +                   'toolkit.telemetry.enabledPreRelease' : False,
> > +                   }
> > +
> 
> Firefox Desktop and Firefox Metro are completely different applications,
> even they have the same version of Gecko underneath. I really subclassed
> from the base Profile class to not introduce any broken cross-application
> behaviors. There might be some preferences which are the same, but as of now
> I would really suggest we keep those separated.

Okay; that makes sense.  With a bit of trepidation, I'll agree for the time being.

> ::: mozrunner/mozrunner/runner.py
> @@ +268,5 @@
> > +    def __init__(self, profile, binary=None, cmdargs=None, **kwargs):
> > +
> > +        # take the binary from BROWSER_PATH environment variable
> > +        if (not binary) and 'BROWSER_PATH' in os.environ:
> > +            binary = os.environ['BROWSER_PATH']
> 
> That's the code I have taken from FirefoxRunner. If we want to change that
> we should do it on both locations. Shall I do that?

Nah, its just a code niceity.  Ideally this would be a function, I suppose, if needed for more than one place, but no need to bother here. I didn't realize it was copied code.

> @@ +271,5 @@
> > +        if (not binary) and 'BROWSER_PATH' in os.environ:
> > +            binary = os.environ['BROWSER_PATH']
> > +
> > +        cmdargs = cmdargs or []
> > +        cmdargs.extend(['-firefoxpath', binary])
> 
> I could copy the code we have in Runner.__init__ and which checks for the
> binary to be set and exists. Would that work? I'm not that happy with that
> given that on other places we might not get all the binary information e.g.
> application.ini because we specify the harness helper now. Let me play
> around with that and see if we can leave the behavior but when we actually
> call the command that I can inject the appropriate options.

It is probably fine as is.  I will leave to your judgement.
Comment on attachment 758194 [details] [diff] [review]
Patch v1

ABICT this is probably good enough for round 1.
Attachment #758194 - Flags: review?(jhammel) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
I did all the requested changes and even modified in how we construct the command. That way it's indeed much cleaner and we can base on all the binary checks we already have in Runner. We only inject the harness application when the command gets requested.

Given that we do not have any documentation for mozrunner and mozprofile yet, I hope it's ok when I don't update it.
Attachment #758194 - Attachment is obsolete: true
Attachment #758283 - Flags: review?(jhammel)
Comment on attachment 758283 [details] [diff] [review]
Patch v2

Review of attachment 758283 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

::: mozprofile/mozprofile/profile.py
@@ +293,5 @@
>                     'toolkit.telemetry.enabledPreRelease' : False,
>                     }
>  
> +class FirefoxMetroProfile(Profile):
> +    """Specialized Profile subclass for Firefox"""

Please change the docstring (e.g. "...for Firefox metro")

::: mozrunner/mozrunner/runner.py
@@ +33,5 @@
>  
> +here = os.path.dirname(os.path.abspath(__file__))
> +
> +# helper application to launch Firefox in Metro mode
> +immersiveHelperPath = os.path.join(here, 'tools','metrotestharness.exe')

I wonder if this should be a class level attribute?
Attachment #758283 - Flags: review?(jhammel) → review+
Attached patch Patch v3Splinter Review
Updated patch with latest comments and ready for landing.
Attachment #758283 - Attachment is obsolete: true
Attachment #758300 - Flags: review+
Landed on master:
https://github.com/mozilla/mozbase/commit/66bc3ab8c2266e6503835dbeaaeb48e4b9aa0302
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 877131
Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=3]
Blocks: 880839
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Created attachment 758194 [details] [diff] [review]
> Patch v1
> 
> So this patch adds support for Firefox Metro mode on Windows 8. The included
> metrotestharness.exe application has been taken from the latest Firefox
> tests zip file from the FTP server.

It might be worth thinking about a script that checks/updates the binary from the latest FTP server.  This wouldn't have to (and IMHO shouldn't) live in the module code.  Or am I wrong in thinking that this binary will change?  I think there is some thinking here
(In reply to Jeff Hammel [:jhammel] from comment #11)
> It might be worth thinking about a script that checks/updates the binary
> from the latest FTP server.  This wouldn't have to (and IMHO shouldn't) live
> in the module code.  Or am I wrong in thinking that this binary will change?
> I think there is some thinking here

I don't think it's worth to keep it up-to-date with each new version of the harness launcher. Whenever we detect that something is not working, we can wait for a fix and update our version.
Sure; I guess I view a script as usable documentation.  If not a (simple) script, the update procedure might be worth documenting.
Whiteboard: [ateamtrack: p=mozmill q=2013q2 m=3] → [ateamtrack: p=mozmill q=2013q2 m=3][mozmill-2.0]
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.