725647 - [SeaMonkey] mochitest-a11y: test_embeds.xul needs to support non-Firefox applications too

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Serge Gautherie (:sgautherie)]

Newly added test, which needs similar fix as bug 717753 had in the meantime :-|

PS: Noticed in bug 717963 comment 1.

Comment 1

[Mark Capella [:capella]]

Attachment: (Av1-moz12) test_embeds.xul: Use "browser.chromeURL" [Checkin: Comment 13]

Okay, 1 file changed for this bug: test_embeds.xul.

   (Note self... review changeset 1a94821b465 associated with bug / file history, not simply bug attachments.)

Comment 2

[Serge Gautherie (:sgautherie)]

Comment on attachment 596278 [details] [diff] [review]
(Av1-moz12) test_embeds.xul: Use "browser.chromeURL" [Checkin: Comment 13]

This is a copy of what I did in bug 717753.
But you should use openBrowserWindow() from bug 719754 rewrite now.

Comment 3

[Mark Capella [:capella]]

Attachment: (Bv1) test_embeds.xul: Use new openBrowserWindow()

Now using common.js functions... openBrowserWindow(), etc.

Comment 4

[Serge Gautherie (:sgautherie)]

Comment on attachment 596417 [details] [diff] [review]
(Bv1) test_embeds.xul: Use new openBrowserWindow()

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

This looks very nice, well done! Just a few nits.

::: accessible/tests/mochitest/relations/test_embeds.xul
@@ +27,5 @@
>    <script type="application/javascript">
>    <![CDATA[
>      ////////////////////////////////////////////////////////////////////////////
>      // Helpers
>  

Nit: These 3 lines can go away too.

@@ +101,5 @@
>        gQueue.invoke();
>      }
>  
>      SimpleTest.waitForExplicitFinish();
> +    openBrowserWindow(doTests);

You missed to re-add |, "about:"| here.

@@ +116,5 @@
> +    <a target="_blank"
> +       href="https://bugzilla.mozilla.org/show_bug.cgi?id=725647"
> +       title="[SeaMonkey] mochitest-a11y: test_embeds.xul needs to support non-Firefox applications too">
> +      Mozilla Bug 725647
> +    </a>

Nit: I don't think adding this block is very valuable...

Comment 5

[Mark Capella [:capella]]

Attachment: (Bv1a) test_embeds.xul: Use new openBrowserWindow() [Checked in: Comment 9]

Still swinging ... Nits fixed, agrees they look better.

   Apologies for missing the "about:" in the openBrowserWindow function ... I noticed it, but it looked optional... (I thought maybe loadURI or loadOneTab addressed it internally since RUNTESTS.py seemed ok, and I didn't look deeper into it ...)

Comment 6

[Serge Gautherie (:sgautherie)]

Comment on attachment 596560 [details] [diff] [review]
(Bv1a) test_embeds.xul: Use new openBrowserWindow()
[Checked in: Comment 9]

(In reply to Mark Capella [:capella] from comment #5)
>    Apologies for missing the "about:" in the openBrowserWindow function ...
> I noticed it, but it looked optional...

Yeah, I don't know, so it's safer to keep it.
Reviewer should tell you if it can be removed.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 596560 [details] [diff] [review]
(Bv1a) test_embeds.xul: Use new openBrowserWindow()
[Checked in: Comment 9]

r=me

Comment 8

[Serge Gautherie (:sgautherie)]

Comment on attachment 596278 [details] [diff] [review]
(Av1-moz12) test_embeds.xul: Use "browser.chromeURL" [Checkin: Comment 13]

Oh, actually I would like this patch to fix mozilla12 (too).

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 596560 [details] [diff] [review]
(Bv1a) test_embeds.xul: Use new openBrowserWindow()
[Checked in: Comment 9]

https://hg.mozilla.org/mozilla-central/rev/817614044d46

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 596278 [details] [diff] [review]
(Av1-moz12) test_embeds.xul: Use "browser.chromeURL" [Checkin: Comment 13]

[Approval Request Comment]
Regression caused by (bug #): Bug 707654.
User impact if declined: None, but perma-orange on SeaMonkey.
Testing completed (on m-c, etc.): Comment 9.
Risk to taking this patch (and alternatives if risky): None, test-only.
String changes made by this patch: None.

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 596278 [details] [diff] [review]
(Av1-moz12) test_embeds.xul: Use "browser.chromeURL" [Checkin: Comment 13]

[Triage Comment]
Test only fix - approved for Aurora 12.

Comment 12

[Serge Gautherie (:sgautherie)]

Mark, from now on, please include author and description to your patches:
see http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

I did it for you for patch Bv1a.
Jens, please, do it for patch Av1-moz12 for this one time.

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 596278 [details] [diff] [review]
(Av1-moz12) test_embeds.xul: Use "browser.chromeURL" [Checkin: Comment 13]

http://hg.mozilla.org/releases/mozilla-aurora/rev/a08366de6833

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to Serge Gautherie (:sgautherie) from comment #9)
> https://hg.mozilla.org/mozilla-central/rev/817614044d46

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1329726992.1329731718.29744.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/02/20 00:36:32

V.Fixed


(In reply to Jens Hatlak (:InvisibleSmiley) from comment #13)
> http://hg.mozilla.org/releases/mozilla-aurora/rev/a08366de6833

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1330206173.1330211402.24221.gz
WINNT 5.2 comm-aurora debug test mochitest-other on 2012/02/25 13:42:53

firefox12: verified.