702448 - Create some view source UI mochitests

Status: RESOLVED FIXED

(Toolkit :: View Source, defect)

Description

[Geoff Lankow (:darktrojan)]

I'm working on a few regression tests for the view source UI, since there are none, and it got broken a lot with the parser change.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: tests

This patch has tests for the window title (bug 699356, currently broken so the test is a known-fail), line wrap, syntax highlighting, and tab size prefs.

(I seem to be picking on you for reviews today Gavin, sorry!)

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 574687 [details] [diff] [review]
tests

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

Overall looks fine. I've pushed to try server: https://tbpl.mozilla.org/?tree=Try&rev=99d57a5d87e7

::: toolkit/components/viewsource/test/browser/browser_viewsourceprefs.js
@@ +13,5 @@
> +    mWindow = aWindow;
> +    wrapMenuItem = aWindow.document.getElementById('menu_wrapLongLines');
> +    syntaxMenuItem = aWindow.document.getElementById('menu_highlightSyntax');
> +
> +    isnot(wrapMenuItem.getAttribute("checked"), "true", "Wrap menu item not checked by default");

Is it possible to use |is(wrapMenuItem.hasAttribute("checked"), false, "Wrap menu item not checked by default");| here or do we set the |.checked| attribute to |"false"|, and if so, can we explicitly check for |"false"| instead of just checking for anything but |"true"|? According to the |simulateClick| function, we are removing the attribute, so we should just be able to use |hasAttribute|.

@@ +25,5 @@
> +
> +// Check that the Wrap Long Lines menu item works.
> +function test1() {
> +  simulateClick(wrapMenuItem);
> +  executeSoon(function() {

Is it possible to use an event listener for the |"click"| event here so we don't need the executeSoon?

@@ +35,5 @@
> +}
> +
> +function test2() {
> +  simulateClick(wrapMenuItem);
> +  executeSoon(function() {

Same question here about the possible use of a |"click"| event listener.

@@ +46,5 @@
> +
> +// Check that the Syntax Highlighting menu item works.
> +function test3() {
> +  mWindow.gBrowser.addEventListener("pageshow", function() {
> +    mWindow.gBrowser.removeEventListener("pageshow", arguments.callee, false);

Can you provide a name to the callee function so we don't have to depend on arguments.callee? According to this page, arguments.callee is deprecated in ES5 strict mode: https://developer.mozilla.org/en/JavaScript/Reference/Functions_and_function_scope/arguments/callee

@@ +47,5 @@
> +// Check that the Syntax Highlighting menu item works.
> +function test3() {
> +  mWindow.gBrowser.addEventListener("pageshow", function() {
> +    mWindow.gBrowser.removeEventListener("pageshow", arguments.callee, false);
> +    isnot(syntaxMenuItem.getAttribute("checked"), "true", "Syntax menu item unchecked");

Can we check for |hasAttribute| here as well?

@@ +59,5 @@
> +}
> +
> +function test4() {
> +  mWindow.gBrowser.addEventListener("pageshow", function() {
> +    mWindow.gBrowser.removeEventListener("pageshow", arguments.callee, false);

Same question about arguments.callee here.

@@ +80,5 @@
> +    ["view_source.wrap_long_lines", true],
> +    ["view_source.syntax_highlight", false]
> +  ]}, function() {
> +    openViewSourceWindow(source, function(aWindow) {
> +      isnot(wrapMenuItem.getAttribute("checked"), "true", "Wrap menu item unchecked");

Same question about |hasAttribute| here.

::: toolkit/components/viewsource/test/browser/head.js
@@ +6,5 @@
> +  let viewSourceWindow = openDialog("chrome://global/content/viewSource.xul", null, null, aURI);
> +  let gBrowser;
> +  waitForFocus(function() {
> +    gBrowser = viewSourceWindow.gBrowser;
> +    executeSoon(function() {

Is it possible to use the |"load"| event listener all the time and remove the need for executeSoon here?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 574687 [details] [diff] [review]
tests

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

::: toolkit/components/viewsource/test/browser/head.js
@@ +4,5 @@
> +
> +function openViewSourceWindow(aURI, aCallback) {
> +  let viewSourceWindow = openDialog("chrome://global/content/viewSource.xul", null, null, aURI);
> +  let gBrowser;
> +  waitForFocus(function() {

Could you add a "load" event listener to viewSourceWindow and when the event fires just check the target to know if the gBrowser.contentWindow has loaded instead of doing the waitForFocus, etc?

Comment 4

[Geoff Lankow (:darktrojan)]

Attachment: tests v2

I've managed to clean up most of the stuff I put in as an attempt to just make the damn thing work. The checked attribute is sometimes false for a variety of reasons (one of which I can only explain as "a Mac thing"). I've checked for it and removed it, then used hasAttribute.

Comment 5

[Geoff Lankow (:darktrojan)]

Oh, and try is all good. https://tbpl.mozilla.org/?tree=Try&rev=63163396023d

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 575773 [details] [diff] [review]
tests v2

Looks good! Thanks for adding more tests :)

Comment 7

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/def93a49ec50

Comment 8

[Geoff Lankow (:darktrojan)]

Comment on attachment 575773 [details] [diff] [review]
tests v2

I think we'll want this on Aurora, so that the regressions from bug 482921 can land tests there. I've put it through Try on top of Aurora and there were no problems.

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/def93a49ec50

Comment 10

[Henri Sivonen (:hsivonen)]

I have a fix for bug 699356 and can see visually that the window title updates correctly. However, when openViewSourceWindow() calls the callback, aWindow.document.documentElement.getAttribute("title") returns "Nightly". Did the the window title test pass with the old View Source?

Comment 11

[Geoff Lankow (:darktrojan)]

No, the attribute doesn't get updated. Only the first of those tests will pass by fixing bug 699356. I may file and fix the other bug if I can work out how. I should've mentioned it, sorry.

Comment 12

[LegNeato]

Comment on attachment 575773 [details] [diff] [review]
tests v2

[triage comment]
Approved for tests.

Comment 13

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69482d1fd036

Comment 14

[Ed Morley [:emorley]]

toolkit/components/viewsource/test/browser/Makefile.in was created in the tree by this bug, but was not listed in toolkit/toolkit-makefiles.sh, so I've included it here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Ed Morley [:edmorley] from comment #14)
> toolkit/components/viewsource/test/browser/Makefile.in was created in the
> tree by this bug, but was not listed in toolkit/toolkit-makefiles.sh, so
> I've included it here:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/17e8e969fa09

Seems like we would want this change also pushed to Aurora, right?

Comment 16

[Ed Morley [:emorley]]

That shouldn't be necessary, since it only helps to speed up makefile creation & won't (at least in this case) cause anything to break :-)

Comment 17

[u279076]

Marking qa- as this is not something QA needs to verify. Please set to qa+ if this is not the case.