Add metrofx browser chrome talos test support to pageloader

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jimm, Assigned: jimm)

Tracking

Trunk
x86_64
Windows 8
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

See bug 773817, pageloader currently assumes it can create desktop windows. We need to be able to load pageloader in a background tab to do chrome talos testing.
Assignee: nobody → jmathies
Summary: Add browser chrome talos test support to pageloader → Add metrofx browser chrome talos test support to pageloader
Posted patch talos patch v.1 (obsolete) — Splinter Review
Comment on attachment 780375 [details] [diff] [review]
browser patch v.1

I think test specific stuff like this is ok provided it doesn't ad overhead. mbrubeck, curious what you think?

This comes at us from one of two places - 

normal browser startup - 

http://mxr.mozilla.org/mozilla-central/source/browser/metro/components/BrowserCLH.js#239

and talos pageloader command line processor - 

http://mxr.mozilla.org/build/source/talos/talos/pageloader/components/tp-cmdline.js#103

which I've modified slightly in the patch posted here.
Attachment #780375 - Flags: review?(mbrubeck)
Comment on attachment 780375 [details] [diff] [review]
browser patch v.1

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

r=mbrubeck, but please remove getBrowserObject unless it's actually required.

::: browser/metro/base/content/browser.js
@@ +32,5 @@
> + * Returns the Browser object.
> + */
> +function getBrowserObject() {
> +  return Browser;
> +}

Why is this function useful?  Can't we just use chromewin.Browser instead of chromewin.getBrowserObject()?
Attachment #780375 - Flags: review?(mbrubeck) → review+
(In reply to Matt Brubeck (:mbrubeck) from comment #4)
> Why is this function useful?  Can't we just use chromewin.Browser instead of
> chromewin.getBrowserObject()?

I think I tried that but maybe I was on the wrong window object. Will test a bit, if you say it'll work, I'm sure it will. :)
Comment on attachment 780373 [details] [diff] [review]
talos patch v.1

I can't really test these changes aside from local runs. Can I hand this off to releng from here jmaher? Be happy to help test or whatever help is needed.
Attachment #780373 - Flags: feedback?(jmaher)
Whiteboard: [leave-open]
Updated per mbrubeck's requested changes.
Attachment #780373 - Attachment is obsolete: true
Attachment #780373 - Flags: feedback?(jmaher)
Attachment #780911 - Flags: feedback?(jmaher)
Comment on attachment 780911 [details] [diff] [review]
talos patch v.2

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

this looks fine, let me test a bit more in depth on mobile and other configs before landing it.
Attachment #780911 - Flags: feedback?(jmaher) → feedback+
this patch for talos doesn't seem to cause any problems with other tests on all platforms.  

I did the f+, are there other changes needed, would you like to get it checked in?  I plan to update talos on Monday or Tuesday on the trees.
(In reply to Joel Maher (:jmaher) from comment #11)
> this patch for talos doesn't seem to cause any problems with other tests on
> all platforms.  
> 
> I did the f+, are there other changes needed, would you like to get it
> checked in?  I plan to update talos on Monday or Tuesday on the trees.

Nope, this should do it.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
You need to log in before you can comment on or make changes to this bug.