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.

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 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 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?

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.

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

Comment on attachment 575773 [details] [diff] [review] tests v2 Looks good! Thanks for adding more tests :)

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.

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?

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 on attachment 575773 [details] [diff] [review] tests v2 [triage comment] Approved for tests.

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

(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?

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 :-)

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