Closed Bug 864250 Opened 7 years ago Closed 6 years ago

Create a browser chrome test that checks the zoom level property of the object

Categories

(Firefox :: PDF Viewer, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36

People

(Reporter: cornel_ionce, Assigned: mihaelav)

Details

(Whiteboard: [pdfjs-c-testing][sprint])

Attachments

(1 file, 2 obsolete files)

STR:

Precondition: Preferences/Options >Applications - PDFs use "Preview in <Firefox>"

1. Open a pdf document
2. Zoom in/out using the +/- buttons
3. Zoom in/out using zoom picker
4. Zoom in/out using CTRL+/CTRL-
5. Zoom in/out using CTRL + mouse scroll

Expected Results:
Document zooms in/out properly.

Notes:
This test is available in MozTrap as test case #368.
Assignee: nobody → cornel.ionce
Status: NEW → ASSIGNED
Summary: Create a test that checks the zoom level property of the object → Create a browser chrome test that checks the zoom level property of the object
Whiteboard: [pdfjs-c-testing]
Assignee: cornel.ionce → mihaela.velimiroviciu
Attached patch v1 (obsolete) — Splinter Review
Test zooming the pdf using the +/- buttons, CTRL++, CTRL+- and the zoom picker.
Attachment #8509532 - Flags: review?(dtownsend+bugmail)
Comment on attachment 8509532 [details] [diff] [review]
v1

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

I may not be the best person to review that as I don't know the details of the pdfjs document, I'm hoping there are some events that can be used instead of timeouts to catch the zoom changes.

::: browser/extensions/pdfjs/test/browser_pdfjs_zoom.js
@@ +19,5 @@
> +      selector: "button#zoomOut",
> +      event: "click"
> +    },
> +    expectedZoom: -1, // -1 - zoom out
> +    message: "Zoomed in using the '-' (zoom out) button"

* Zoomed out

@@ +112,5 @@
> +      deferred.resolve();
> +    }
> +  }, 500);
> +
> +  return deferred.promise;

Is there no event we can wait for here?

@@ +115,5 @@
> +
> +  return deferred.promise;
> +}
> +
> +function zoomPDF(document, window, test, endCallback) {

No need to pass the test in here, just do let test = TESTS.shift() at the start.

@@ +177,5 @@
> +      deferred.resolve();
> +    }
> +  }, 500);
> +
> +  return deferred.promise;

Is there no event we can wait for here?
Attachment #8509532 - Flags: review?(dtownsend+bugmail)
Whiteboard: [pdfjs-c-testing] → [pdfjs-c-testing][sprint]
Flags: needinfo?(ydelendik)
Comment on attachment 8509532 [details] [diff] [review]
v1

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

Looks good with the changes below

::: browser/extensions/pdfjs/test/browser.ini
@@ +6,5 @@
>  skip-if = debug # bug 1058695
>  [browser_pdfjs_savedialog.js]
>  [browser_pdfjs_views.js]
>  skip-if = debug # bug 1058695
> +[browser_pdfjs_zoom.js]

I think you shall add `skip-if = debug # bug 1058695` for this entry as well

::: browser/extensions/pdfjs/test/browser_pdfjs_zoom.js
@@ +165,5 @@
> +    ok(false, "Test '" + test.message + "' failed with timeout.");
> +  });
> +}
> +
> +function waitForZoom(document) {

You can use 'pagerendered' event here. See example at http://mxr.mozilla.org/mozilla-central/source/browser/extensions/pdfjs/content/web/viewer.js#6375 (make sure e.detail.pageNumber equal to page 1).
Attachment #8509532 - Flags: review+
Attached patch v1.1 (obsolete) — Splinter Review
Updated based on review.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d2847249da5
Attachment #8509532 - Attachment is obsolete: true
Attachment #8517298 - Flags: review?(ydelendik)
Comment on attachment 8517298 [details] [diff] [review]
v1.1

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

Looks good with the event handler change

::: browser/extensions/pdfjs/test/browser_pdfjs_zoom.js
@@ +119,5 @@
> +function zoomPDF(document, window, test, endCallback) {
> +  var renderedPage;
> +
> +  document.addEventListener("pagerendered", function onPageRendered(e) {
> +    document.removeEventListener("pagerendered", onPageRendered, true);

Several pages might be rendered at the same time, e.g. when browser window is tall enough (and test pdf file has multiple pages). Check if rendered page is 1 before detaching the event handler.
Attachment #8517298 - Flags: review?(ydelendik) → review+
Attached patch v1.2Splinter Review
Updated based un Yury's review. 
Carrying over the r+

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8612d94d1d6e
Attachment #8517298 - Attachment is obsolete: true
Flags: needinfo?(ydelendik)
Attachment #8518988 - Flags: review+
(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #6)
> try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8612d94d1d6e

Try is green.
Marking for checkin
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/68514a13df83
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [pdfjs-c-testing][sprint] → [pdfjs-c-testing][sprint][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/68514a13df83
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [pdfjs-c-testing][sprint][fixed-in-fx-team] → [pdfjs-c-testing][sprint]
Target Milestone: --- → Firefox 36
You need to log in before you can comment on or make changes to this bug.