420659 - initial Mochitest support for running Camino

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I have things working now such that "python runtests.py --autorun" will launch Camino using the testing profile, open the test url, and start running tests. 

There are still some harness/launching issues that will need fixing, and the changes so far may not be the cleanest or stylistically correct (or completely correct, for that matter; I don't know python and my Makefile fu is young), but Mochitest now runs for Camino "out-of-the-box".

Summary of the changes:

* build/pgo/Makefile.in and testing/mochitest/Makefile.in both contain changes to set the correct path for Camino and pass an "IS_CAMINO" to automation.py(.in).  The copy in testing/mochitest/ seems to be the one used for Mochitests, but I took the comment in the first paragraph of bug 420238 comment 4 to mean that the copy in build/pgo/ will replace it soon (or should at least remain synced).

* build/pgo/automation.py.in 
  * defines IS_CAMINO. It's unclear to me which style (IS_MAC, IS_CYGWIN) to 
    copy, but I chose the latter since it seemed like it's for the "odd case 
    most people shouldn't need to notice"
  * add Camino pref analogues for the general set of default profile prefs and 
    for the proxy prefs, only if IS_CAMINO is defined, so no other app gets 
    "excess" prefs set
  * Removes Camino from the "-bin"-appending, though I'm not positive this fix 
    is correct for the others (is 
    --app=/path/to/APP_NAME.app/Contents/MacOS/appname supposed to go through 
    this code and get fixed-up for "-bin", or is --app an absolute override?)
  * Passes relevant args to Camino (but see below about "-no-remote")

* testing/mochitest/runtests.py.in defines the required environment variables for Camino, i.e. makes Camino actually use the mochitesttestingprofile instead of the default Camino profile.

There are two issues left with the automation itself in terms of getting "proper" operation:

* AFAIK, Camino doesn't actually support -no-remote, but if I don't include it, Camino launches with two windows: 1 the proper test url, and 1 the test-url-as-error that happens if you assume that Camino and toolkit both use the same command-line handling.  I assume this means I'm missing something yet in replacing the toolkit-style command-line handling with the "-url" style that Camino supports.

* Camino, not being a XUL app, doesn't support -foreground.  runtests.pl has a mention of using AppleScript to bring the app to the foreground, but I can't actually find where it calls any AppleScript.  This will need to be implemented at some point.

Jeff, are you a good reviewer for this?

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Mochitest support for Camino, v1

Let's see if Bugzilla will actually upload my patch this time :-(

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 306977 [details] [diff] [review]
Mochitest support for Camino, v1

>Index: build/pgo/Makefile.in

>+ifeq ($(MOZ_BUILD_APP),camino)
>+browser_path = \"../$(DIST)/Camino.app/Contents/MacOS/Camino\"
>+endif
>+

Would prefer this be merged in, possibly at the top, of the existing browser_path-setting code.  Also, is MOZ_BUILD_APP really the canonical variable here?  You use MOZ_MACBROWSER elsewhere; please use one variable consistently.


>Index: build/pgo/automation.py.in

>+#ifdef MOZ_MACBROWSER
>+#expand IS_CAMINO = __IS_CAMINO__ == 1
>+#else
>+IS_CAMINO = False
>+#endif

Better to always define IS_CAMINO and just have a single #expand line.


>+  if IS_CAMINO:
>+    part = """
>+user_pref("camino.check_default_browser", false);
>+user_pref("camino.warn_when_closing", false);
>+user_pref("browser.sessionstore.resume_from_crash", false);
>+user_pref("camino.remember_window_state", false);
>+"""
>+    prefs.append(part)

For the resume_from_crash one, is that needed?  You've got a clean profile that shouldn't have any crash data in it to resume from, I'd think.  I'd also kinda prefer if you just set these unconditionally, probably with a comment that they're Camino-specific but harmless otherwise.


>+  if IS_CAMINO:
>+    part = """
>+user_pref("camino.use_system_proxy_settings", false);
>+"""
>+    prefs.append(part)

Again, prefer unconditionally with a comment regarding Camino-onlyness.


>   # now run with the profile we created
>   cmd = app
>-  if IS_MAC and not cmd.endswith("-bin"):
>+  if IS_MAC and not IS_CAMINO and not cmd.endswith("-bin"):
>     cmd += "-bin"
>   cmd = os.path.abspath(cmd)

Sigh; don't really like this, but probably can't do better for now.


>-  args.extend(("-no-remote", "-profile", profileDirectory, testURL))
>+  if IS_CAMINO:
>+    args.extend(("-no-remote", "-url", testURL))
>+  else:
>+    args.extend(("-no-remote", "-profile", profileDirectory, testURL))

Sigh; how much effort would it be to just hack in the same URL support to Camino?  I can't imagine it'd be that horrible, and I'd prefer it to yet more app-forking here.


>Index: testing/mochitest/Makefile.in

>+ifeq ($(MOZ_BUILD_APP),camino)
>+browser_path = \"../$(DIST)/Camino.app/Contents/MacOS/Camino\"
>+endif

Again don't know whether this should be MOZ_MACBROWSER or not, and consolidate with other browser_path-setting gunk.


>+ifeq ($(MOZ_BUILD_APP),camino)
>+TEST_DRIVER_PPARGS += -DIS_CAMINO=1
>+endif

MOZ_MACBROWSER?


>Index: testing/mochitest/runtests.py.in

>   def start(self):
>     "Run the Mochitest server, returning the process ID of the server."
>     
>     env = dict(os.environ)
>     if automation.UNIXISH:
>       env["LD_LIBRARY_PATH"] = automation.DIST_BIN
>       env["MOZILLA_FIVE_HOME"] = automation.DIST_BIN
>       env["XPCOM_DEBUG_BREAK"] = "warn"
>+    if automation.IS_CAMINO:
>+      env["CAMINO_PROFILE_DIR"] = PROFILE_DIRECTORY  

I doubt you need this.


Enough for now, possibly more on the next iteration...

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> (From update of attachment 306977 [details] [diff] [review])
> >Index: build/pgo/Makefile.in
> 
> >+ifeq ($(MOZ_BUILD_APP),camino)
> >+browser_path = \"../$(DIST)/Camino.app/Contents/MacOS/Camino\"
> >+endif
> >+
> 
> Would prefer this be merged in, possibly at the top, of the existing
> browser_path-setting code.

Sure; I had it there before (which is the only other place I had it working) and then noticed the cygwin stuff had a separate block and wrongly assumed I should be copying it ;)

> Also, is MOZ_BUILD_APP really the canonical
> variable here?  You use MOZ_MACBROWSER elsewhere; please use one variable
> consistently.

http://mxr.mozilla.org/seamonkey/source/configure.in#4451 seems to indicate that MOZ_BUILD_APP is the way to define things going forward, so I'll standardize on that here.

> >+  if IS_CAMINO:
> >+    part = """
> >+user_pref("camino.check_default_browser", false);
> >+user_pref("camino.warn_when_closing", false);
> >+user_pref("browser.sessionstore.resume_from_crash", false);
> >+user_pref("camino.remember_window_state", false);
> >+"""
> >+    prefs.append(part)
> 
> For the resume_from_crash one, is that needed?  You've got a clean profile that
> shouldn't have any crash data in it to resume from, I'd think.

I didn't check carefully, but at one point it seemed like I was getting a profile that had been used before.  We do set that on the tinderbox, so based on those two things I included it here.  I'll check more.

> prefer if you just set these unconditionally, probably with a comment that
> they're Camino-specific but harmless otherwise.

Sure.
 
> >-  args.extend(("-no-remote", "-profile", profileDirectory, testURL))
> >+  if IS_CAMINO:
> >+    args.extend(("-no-remote", "-url", testURL))
> >+  else:
> >+    args.extend(("-no-remote", "-profile", profileDirectory, testURL))
> 
> Sigh; how much effort would it be to just hack in the same URL support to
> Camino?  I can't imagine it'd be that horrible, and I'd prefer it to yet more
> app-forking here.

Stuart, can you comment on this? It looks like right now everything (except -url foo) passed after Contents/MacOS/Appname is treated as a file:// URL, which AFAIK is standard Mac OS X behavior for invoking via the full path to the executable.

> Enough for now, possibly more on the next iteration...

Thanks for the quick review!

Comment 4

[Stuart Morgan]

(In reply to comment #3)
> > Sigh; how much effort would it be to just hack in the same URL support to
> > Camino?  I can't imagine it'd be that horrible, and I'd prefer it to yet more
> > app-forking here.
> 
> Stuart, can you comment on this? It looks like right now everything (except
> -url foo) passed after Contents/MacOS/Appname is treated as a file:// URL,
> which AFAIK is standard Mac OS X behavior for invoking via the full path to the
> executable.

Right; AppKit will automatically turn any |-foo bar| pair into an user default with key |foo| and value |bar|, then everything else is treated as a file to open. Since it's assumed by the system to be a relative path, it's converted to a (crazy) absolute path, so by the time we see it it's not obvious that we have a URL. We'd have to either do something skanky and potentially fragile like look for protocol strings in every URL we are given to open through app callbacks, or completely bypass the standard AppKit command-line handling, which I'd prefer not to do. Either one would be substantially more ugly than the |if| here (from my perspective, at least).

On the other hand, -profile support would be pretty easy for us to add, if we want to avoid forking that bit.

Comment 5

[Jeff Walden [:Waldo]]

Sigh; if -profile is easy, I'll take that and call it enough for now.  We probably need to consolidate this app-specific stuff behind a few classes or something, to prevent it from spreading everywhere uncontrollably over time.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Mochitest support for Camino, v4

This should address all the review comments from the last iteration.

I'm not sure how I wasn't getting clean testing profiles every time in the past, but I did when working on this revision, so I was able to remove three prefs that only matter on second-run.

I'm still puzzled as to how -no-remote (which we have no support for) suppresses the opening of a window showing the mochitesttestingprofile folder, and there's still the issue of making Camino come to the front once launched, but these changes should be sufficient to get Mochitest up and running in Camino.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 319339 [details] [diff] [review]
Mochitest support for Camino, v4

>Index: build/pgo/Makefile.in

>+ifeq ($(MOZ_BUILD_APP),camino)
>+browser_path = \"../$(DIST)/Camino.app/Contents/MacOS/Camino\"
>+else

I didn't notice this last time, but why is the .. needed at the start here?  Why isn't it just $(DIST) like everything else?


>Index: build/pgo/automation.py.in

>+user_pref("camino.warn_when_closing", false); // Camino-only, harmless to others
>+user_pref("camino.use_system_proxy_settings", false); // Camino-only, harmless to others

Put an empty line before each of these to separate them from the more general prefs.


>-  args.extend(("-no-remote", "-profile", profileDirectory, testURL))
>+  if IS_CAMINO:
>+    args.extend(("-no-remote", "-profile", profileDirectory, "-url", testURL))
>+  else:
>+    args.extend(("-no-remote", "-profile", profileDirectory, testURL))

Hm, let's common the first three args and then only split on the last one/two:

  args.extend(("-no-remote", "-profile", profileDirectory))
  if IS_CAMINO:
    args.extend(("-url", testURL))
  else:
    args.append(testURL)


Do the above, explain the $(DIST) thing, and r=me.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Mochitest support for Camino, v4a (comments addressed)

> >Index: build/pgo/Makefile.in
> 
> >+ifeq ($(MOZ_BUILD_APP),camino)
> >+browser_path = \"../$(DIST)/Camino.app/Contents/MacOS/Camino\"
> >+else
> 
> I didn't notice this last time, but why is the .. needed at the start here? 
> Why isn't it just $(DIST) like everything else?

Because I copied from the other Makefile and forgot to change it :(  I've now fixed that and all the other comments.

Does this need additional review and/or approval (I'm assuming the latter at least)?

Comment 9

[Jeff Walden [:Waldo]]

Test-only change, not really part of the build except on pgo machines (and that's not even "the build" but rather the process), and even there pretty clearly shouldn't affect anything but Camino, I'd say be bold and commit.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Checked in.  Thanks, Jeff!

Checking in build/pgo/Makefile.in;
/cvsroot/mozilla/build/pgo/Makefile.in,v  <--  Makefile.in
new revision: 1.2; previous revision: 1.1
done
Checking in build/pgo/automation.py.in;
/cvsroot/mozilla/build/pgo/automation.py.in,v  <--  automation.py.in
new revision: 1.11; previous revision: 1.10
done
Checking in testing/mochitest/Makefile.in;
/cvsroot/mozilla/testing/mochitest/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done

Comment 11

[Jeff Walden [:Waldo]]

Moving a bunch of Core :: Testing bugs to Testing :: Mochitest to clear out the former, which is obsolete now that we have more specialized categories for such bugs; filter on the string "MochitestMmMm" to delete all these notifications.