Create some view source UI mochitests

RESOLVED FIXED in Firefox 10

Status

()

Toolkit
View Source
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: darktrojan, Assigned: darktrojan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla11
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(firefox10 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 699356, 469434, 700260
(Assignee)

Updated

6 years ago
Blocks: 700042
(Assignee)

Updated

6 years ago
Blocks: 455888
(Assignee)

Comment 1

6 years ago
Created attachment 574687 [details] [diff] [review]
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!)
Attachment #574687 - Flags: review?(gavin.sharp)
Attachment #574687 - Flags: review?(gavin.sharp) → review?(jwein)
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?
(Assignee)

Updated

6 years ago
Blocks: 703965
(Assignee)

Comment 4

6 years ago
Created attachment 575773 [details] [diff] [review]
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.
Attachment #574687 - Attachment is obsolete: true
Attachment #574687 - Flags: review?(jwein)
Attachment #575773 - Flags: review?(jwein)
(Assignee)

Comment 5

6 years ago
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 :)
Attachment #575773 - Flags: review?(jwein) → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/def93a49ec50
Flags: in-testsuite-
Flags: in-litmus-
Target Milestone: --- → mozilla11
(Assignee)

Comment 8

6 years ago
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.
Attachment #575773 - Flags: approval-mozilla-aurora?

Comment 9

6 years ago
https://hg.mozilla.org/mozilla-central/rev/def93a49ec50
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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?
(Assignee)

Comment 11

6 years ago
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

6 years ago
Comment on attachment 575773 [details] [diff] [review]
tests v2

[triage comment]
Approved for tests.
Attachment #575773 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/69482d1fd036
status-firefox10: --- → fixed
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.
Whiteboard: [qa-]
Depends on: 728628
(Assignee)

Updated

5 years ago
Blocks: 728655
You need to log in before you can comment on or make changes to this bug.