Last Comment Bug 725647 - [SeaMonkey] mochitest-a11y: test_embeds.xul needs to support non-Firefox applications too
: [SeaMonkey] mochitest-a11y: test_embeds.xul needs to support non-Firefox appl...
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla13
Assigned To: Mark Capella [:capella]
:
Mentors:
http://mxr.mozilla.org/mozilla-centra...
Depends on: 719754
Blocks: a11ytestdev SmTestFail 717969 707654
  Show dependency treegraph
 
Reported: 2012-02-09 06:58 PST by Serge Gautherie (:sgautherie)
Modified: 2012-02-26 00:07 PST (History)
2 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
(Av1-moz12) test_embeds.xul: Use "browser.chromeURL" [Checkin: Comment 13] (1.42 KB, patch)
2012-02-10 23:54 PST, Mark Capella [:capella]
surkov.alexander: review+
bugzillamozillaorg_serge_20140323: feedback+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(Bv1) test_embeds.xul: Use new openBrowserWindow() (4.12 KB, patch)
2012-02-11 20:29 PST, Mark Capella [:capella]
bugzillamozillaorg_serge_20140323: feedback+
Details | Diff | Splinter Review
(Bv1a) test_embeds.xul: Use new openBrowserWindow() [Checked in: Comment 9] (3.72 KB, patch)
2012-02-12 20:59 PST, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2012-02-09 06:58:53 PST
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] 2012-02-10 23:54:55 PST
Created attachment 596278 [details] [diff] [review]
(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) 2012-02-11 09:08:34 PST
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] 2012-02-11 20:29:04 PST
Created attachment 596417 [details] [diff] [review]
(Bv1) test_embeds.xul: Use new openBrowserWindow()


   Now using common.js functions... openBrowserWindow(), etc.
Comment 4 Serge Gautherie (:sgautherie) 2012-02-12 20:33:25 PST
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] 2012-02-12 20:59:15 PST
Created attachment 596560 [details] [diff] [review]
(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) 2012-02-12 23:52:30 PST
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 2012-02-12 23:59:01 PST
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) 2012-02-13 11:50:05 PST
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) 2012-02-13 12:09:18 PST
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) 2012-02-13 16:46:49 PST
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] 2012-02-14 11:38:13 PST
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) 2012-02-24 08:14:27 PST
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) 2012-02-25 02:59:07 PST
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) 2012-02-26 00:07:23 PST
(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.

Note You need to log in before you can comment on or make changes to this bug.