830379 - Create mozmill test for restarting the browser while having an opened PDF file

Status: RESOLVED WONTFIX

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andreea Matei [:AndreeaMatei]]

This is the automated test for testcase 371:
https://moztrap.mozilla.org/manage/case/371/

Here are the steps:
1. "Show my windows and tabs from last time" preference is selected for startup. 
 Preferences/Options >Applications - PDFs use "Preview in <Firefox>". 
 Open some PDFs in some tabs
2. Exit the browser
3. Start the browser

Expected behavior:
1. Document loads and displays properly using the Firefox PDF preview
2. Browser closes smoothly
3. PDFs are opened using the Firefox PDF Viewer

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This feature is new in Firefox 20 (enabled by default). So we don't want to expose it to all the branches.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that Mihaela is working on it it's no longer a sprint entry. I will find another one for this week.

Comment 3

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v1

Comment 4

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 703915 [details] [diff] [review]
patch v1

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

Thanks for the quick patch Mihaela. I have a few comments and I would also like Henrik or Dave's opinion about checking something else here besides the viewerContainer. Maybe the location bar url for the second test, to be sure the page is there, not just the pdf viewer.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/manifest.ini
@@ +1,3 @@
> +[test1.js]
> +[test2.js]
> +

No need for this extra line

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +10,5 @@
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../../data/');
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + 'pdf/test.pdf';
> +
> +var setupModule = function (module) {

Lets use function name() instead of making them private, for all functions in the test.

@@ +28,5 @@
> +  controller.open(LOCAL_TEST_FILE);
> +  controller.waitForPageLoad();
> +
> +  // Verify PDF is opened with internal PDF viewer
> +  var pdfViewer = new elementslib.ID(controller.tabs.activeTab, 

Please remove the whitespace at the end of this line and move the id a space to the left, so it's aligned with "controller".

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
@@ +14,5 @@
> + * Test that PDF is opened with inline PDF Viewer after a restart
> + */
> +var testPDFViewerAfterRestart = function () {
> +  pdfViewer = new elementslib.ID(controller.tabs.activeTab,
> +                                  "viewerContainer");

Same here as in test1.js

@@ +19,5 @@
> +  assert.ok(pdfViewer.exists(),
> +            'PDF is opened with a different viewer after restart');
> +}
> +
> +var teardownModule = function (){

Please move this up, after setupModule.

@@ +20,5 @@
> +            'PDF is opened with a different viewer after restart');
> +}
> +
> +var teardownModule = function (){
> +  prefs.preferences.setPref("browser.startup.page", 1);

I'd rather have prefs.preferences.clearUserPref() used here and declare the pref as a const after imported modules. Same for the other test file.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 703915 [details] [diff] [review]
patch v1

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

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +10,5 @@
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../../data/');
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + 'pdf/test.pdf';
> +
> +var setupModule = function (module) {

It doesn't mean the function is private. It's just a different method in defining it, which is even faster. So we do not have a clear coding style for it at the moment.

@@ +28,5 @@
> +  controller.open(LOCAL_TEST_FILE);
> +  controller.waitForPageLoad();
> +
> +  // Verify PDF is opened with internal PDF viewer
> +  var pdfViewer = new elementslib.ID(controller.tabs.activeTab, 

Please create a new library module for that feature. It needs a class with the getElement/getElements method added. Andreea, or anyone else can assist you in it.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
@@ +5,5 @@
> +// Include required modules
> +var { assert } = require("../../../../lib/assertions");
> +var prefs = require("../../../../lib/prefs");
> +
> +var setupModule = function(module) {

nit: blank before the rounded opening bracket.

@@ +16,5 @@
> +var testPDFViewerAfterRestart = function () {
> +  pdfViewer = new elementslib.ID(controller.tabs.activeTab,
> +                                  "viewerContainer");
> +  assert.ok(pdfViewer.exists(),
> +            'PDF is opened with a different viewer after restart');

I wonder what else we can check. Checking for the viewer doesn't mean the PDF has been loaded. We could compare the number of pages and other before and after the restart.

nit: please keep double quotes across the files.

@@ +19,5 @@
> +  assert.ok(pdfViewer.exists(),
> +            'PDF is opened with a different viewer after restart');
> +}
> +
> +var teardownModule = function (){

I wouldn't mind that. In the future the teardown method should be located at the end so it's a better flow.

Comment 6

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: pach v2

Created pdf.js library and updated test

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 710716 [details] [diff] [review]
pach v2

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

Great work Mihaela, it's definitely improved, but still needs a few changes.

::: lib/pdf.js
@@ +11,5 @@
> +
> +// Include required modules
> +var { assert } = require("assertions");
> +var utils = require("utils");
> +var domUtils = require("dom-utils");

This need to be reorder. Also, I don't see where utils is used.

@@ +19,5 @@
> + *
> + * @param {MozMillController} controller
> + *        MozMill controller of the browser window to operate on.
> + */
> +function pdfViewer(controller) {

Please add prefix 'a' to all parameters used across the file.

@@ +56,5 @@
> +    var elements = this.getElements(aSpec);
> +
> +    return (elements.length > 0) ? elements[0] : undefined;
> +  },
> +  

trailing whitespaces

@@ +100,5 @@
> +        nodeCollector.queryNodes("#pageContainer1");
> +        break;
> +      default:
> +        throw new Error(arguments.callee.name
> +                        + ": Unknown element type - " + spec.type);

Please move the '+' on the first line.

@@ +112,5 @@
> +   *
> +   * @returns {Number} Index of current viewed page
> +   */
> +  get currentPage() {
> +    curPgNo = this.getElement({type: "pageNumber"}).value;

I think we can return this directly. But this is not returning the value, I believe it needs getNode().value.

@@ +122,5 @@
> +   *
> +   * @returns {Number} Number of pages of the document
> +   */
> +  get numberOfPages() {
> +    maxPgNo = this.getElement({type: "pageNumber"}).getNode().getAttribute("max");

Same here, getNode().max
Mind adding a test in lib/tests to check all these elements?

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +5,5 @@
> +// Include required modules
> +var { assert } = require("../../../../lib/assertions");
> +var prefs = require("../../../../lib/prefs");
> +var tabs = require("../../../../lib/tabs");
> +var pdfs = require("../../../../lib/pdf");

Please move this before prefs, so it's alphabetically sorted.

@@ +10,5 @@
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../../data/");
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> +const STARTUP_PREF_NAME = "browser.startup.page";
> +const STARTUP_PREF_VALUE = 3; // value for restoring previous session

I see this being used only one on setting the pref, we could do that directly.
Also we need a blank line to separate LOCAL_TEST consts from PREF one.

@@ +25,5 @@
> +/**
> + * Test that PDF is opened with inline PDF Viewer
> + */
> +var testPDFViewer = function () {
> +  // Open the test file

No need for this comment

@@ +29,5 @@
> +  // Open the test file
> +  controller.open(LOCAL_TEST_FILE);
> +  controller.waitForPageLoad();
> +
> +  doc = new pdfs.pdfViewer(controller);

We should better have this in setupModule().

@@ +30,5 @@
> +  controller.open(LOCAL_TEST_FILE);
> +  controller.waitForPageLoad();
> +
> +  doc = new pdfs.pdfViewer(controller);
> +  doc.waitForDocumentLoad();    

You have trailing whitespaces here.

@@ +33,5 @@
> +  doc = new pdfs.pdfViewer(controller);
> +  doc.waitForDocumentLoad();    
> +
> +  // Verify PDF is opened with internal PDF viewer
> +  assert.ok(doc.isInlineViewerUsed(), 

Whitespace here too.

@@ +34,5 @@
> +  doc.waitForDocumentLoad();    
> +
> +  // Verify PDF is opened with internal PDF viewer
> +  assert.ok(doc.isInlineViewerUsed(), 
> +            "PDF is opened with a different viewer");

The message should reflect the expect behaviour, so if fails we get "PDF is opened with internal PDF viewer - got false".

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
@@ +4,5 @@
> +
> +// Include required modules
> +var { assert } = require("../../../../lib/assertions");
> +var prefs = require("../../../../lib/prefs");
> +var pdfs = require("../../../../lib/pdf");

Please reorder this and add a blank line to separate modules from following const.

@@ +15,5 @@
> +/**
> + * Test that PDF is opened with inline PDF Viewer after a restart
> + */
> +var testPDFViewerAfterRestart = function () {
> +  doc = new pdfs.pdfViewer(controller);

This should also be moved in setupModule().

@@ +16,5 @@
> + * Test that PDF is opened with inline PDF Viewer after a restart
> + */
> +var testPDFViewerAfterRestart = function () {
> +  doc = new pdfs.pdfViewer(controller);
> +  assert.ok(doc.isInlineViewerUsed(), 

Trailing whitespace

@@ +20,5 @@
> +  assert.ok(doc.isInlineViewerUsed(), 
> +            "PDF is opened with a different viewer after restart");
> +
> +  initialPagesNumber = persisted.numberOfPages;
> +  currentPagesNumber = doc.numberOfPages;

I don't see the need for these extra variables, we should check directly.

@@ +22,5 @@
> +
> +  initialPagesNumber = persisted.numberOfPages;
> +  currentPagesNumber = doc.numberOfPages;
> +
> +  assert.ok(Number(initialPagesNumber) == Number(currentPagesNumber),

assert.equal() fits better, it will also show us the values in case of failure.
Instead of Number, please use parseInt.

Comment 8

[Andreea Matei [:AndreeaMatei]]

Adding as blocker because of the new library.

Comment 9

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v3

Addressed changes from previous feedback.

Comment 10

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 712437 [details] [diff] [review]
patch v3

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

We're getting closer :)

I would also like Dave's thougths regarding the library at least, I'll add a flag for him.

::: lib/pdf.js
@@ +131,5 @@
> +  isInlineViewerUsed : function pdfViewer_isInlineViewerUsed() {
> +    viewerContainer = this.getElement({type : "viewer"});
> +
> +    if (viewerContainer.exists())
> +      return true;

If pdf mode view is not enabled, viewerContainer will be undefined. We should either check that viewerContainer !== undefined or get and return the pdfjs.disabled preference value.

@@ +140,5 @@
> +  /**
> +   * Wait for the document content to load
> +   */
> +  waitForDocumentLoad : function pdfViewer_waitForDocumentLoad() {
> +    container = new elementslib.ID(this._controller.tabs.activeTab, "pageContainer1");

Why not get this with getElement()? I see it's added to the list.

::: lib/tests/testPdf.js
@@ +9,5 @@
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../data/");
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> +
> +//const TIMEOUT = 5000;

No need for this comment

@@ +12,5 @@
> +
> +//const TIMEOUT = 5000;
> +
> +const NODES = [
> +  {type: "viewer", parent: undefined},

Don't think we need parent in these cases.

@@ +36,5 @@
> +  // Test all available elements
> +  NODES.forEach(function (element) {
> +    var node = doc.getElement({
> +      type: element.type,
> +      subtype: element.subtype,

Don't need these either giving that we provide type only.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 712437 [details] [diff] [review]
patch v3

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

This is looking really good! Looking forward to seeing this landed. :)

::: lib/pdf.js
@@ +110,5 @@
> +   *
> +   * @returns {Number} Index of current viewed page
> +   */
> +  get currentPage() {
> +    return this.getElement({type : "pageNumber"}).getNode().value;

We should probably parse this to an integer here, rather than returning a string.

@@ +119,5 @@
> +   *
> +   * @returns {Number} Number of pages of the document
> +   */
> +  get numberOfPages() {
> +    return this.getElement({type : "pageNumber"}).getNode().max;

As above, we should parse this to an integer.

::: lib/tests/testPdf.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules
> +var { assert, expect } = require("../assertions");
> +var pdfs = require("../pdf");

Can we rename this variable to 'pdf' to match the lib name.

@@ +21,5 @@
> +];
> +
> +var setupModule = function (module) {
> +  controller = mozmill.getBrowserController();
> +  doc = new pdfs.pdfViewer(controller);

I think 'doc' is ambiguous here, could we rename this to pdfViewer?

@@ +33,5 @@
> +  controller.waitForPageLoad();
> +  doc.waitForDocumentLoad();
> +
> +  // Test all available elements
> +  NODES.forEach(function (element) {

Rename argument to aNode

@@ +51,5 @@
> +  assert.ok(parseInt(doc.numberOfPages) >= 1, "Number of pages should be at least 1");
> +  // Get current page number
> +  assert.equal(doc.currentPage, "1", "Current page should be 1");
> +}
> +

nit: Please remove the trailing blank line.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules
> +var { assert } = require("../../../../lib/assertions");
> +var pdfs = require("../../../../lib/pdf");

Rename to 'pdf'

@@ +14,5 @@
> +const STARTUP_PREF_NAME = "browser.startup.page";
> +
> +var setupModule = function (module) {
> +  controller = mozmill.getBrowserController();
> +  doc = new pdfs.pdfViewer(controller);

Rename this to 'pdfViewer'

@@ +19,5 @@
> +  tabBrowser = new tabs.tabBrowser(controller);
> +
> +  tabBrowser.closeAllTabs();
> +
> +  prefs.preferences.setPref(STARTUP_PREF_NAME, 3);

Could you add a comment, to explain why we set this to 3?

@@ +31,5 @@
> +  controller.waitForPageLoad();
> +
> +  doc.waitForDocumentLoad();
> +
> +  //Verify PDF is opened with internal PDF viewer

I think this comment is not needed, the following assertion is pretty self explanatory.

@@ +37,5 @@
> +            "PDF is opened with the internal PDF viewer");
> +
> +  persisted.numberOfPages = doc.numberOfPages;
> +}
> +

nit: Remove any trailing blank lines.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules
> +var { assert } = require("../../../../lib/assertions");
> +var pdfs = require("../../../../lib/pdf");

As with the other tests, rename to 'pdf'

@@ +10,5 @@
> +const STARTUP_PREF_NAME = "browser.startup.page";
> +
> +var setupModule = function (module) {
> +  controller = mozmill.getBrowserController();
> +  doc = new pdfs.pdfViewer(controller);

As with the other tests, rename to 'pdfViewer'

@@ +27,5 @@
> +
> +var teardownModule = function() {
> +  prefs.preferences.clearUserPref(STARTUP_PREF_NAME);
> +}
> +

nit: Remove any trailing blank lines.

Comment 12

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v4

Comment 13

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 715414 [details] [diff] [review]
patch v4

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

Just a few small things to edit and we're done. Please request review from Henrik and Dave after you do those changes.

::: lib/tests/testPdf.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules
> +var { assert, expect } = require("../assertions");

Do we use expect?

@@ +34,5 @@
> +  // Test all available elements
> +  NODES.forEach(function (aNode) {
> +    var node = pdfViewer.getElement({type: aNode.type});
> +
> +    assert.ok(node.exists(), "Element has been found");

No need for the blank line, these are connected.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +19,5 @@
> +  tabBrowser = new tabs.tabBrowser(controller);
> +
> +  tabBrowser.closeAllTabs();
> +
> +  // browser.startup.page = 3 for restoring previous session

Lets change this to "Set browser to restore previous session", sounds better.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 715414 [details] [diff] [review]
patch v4

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

Let me do a drive-by review while I was checking the last comments. It's a good start but some more things have to be done.

::: lib/pdf.js
@@ +18,5 @@
> + *
> + * @param {MozMillController} aController
> + *        MozMill controller of the browser window to operate on.
> + */
> +function pdfViewer(aController) {

Classes should start with a capital letter. Here we also have an abbreviation, so please call it PDFViewer.

@@ +85,5 @@
> +    switch (type) {
> +      case "viewer":
> +        nodeCollector.queryNodes("#viewerContainer");
> +        break;
> +      case "pageNumber":

`viewer` is the parent of all the other elements. So please prefix them like `viewer_pageNumber`.

@@ +98,5 @@
> +      case "content":
> +        nodeCollector.queryNodes("#pageContainer1");
> +        break;
> +      default:
> +        throw new Error(arguments.callee.name + ": Unknown element type - " + spec.type);

I'm sure we wanted to use an assert call here too.

@@ +127,5 @@
> +   * Verify if inline PDF viewer is used to handle the document
> +   *
> +   * @returns {Boolean} True if the inline PDF Viewer is used
> +   */
> +  isInlineViewerUsed : function pdfViewer_isInlineViewerUsed() {

Why we can't make it a getter too? Also I would call it `enabled` given that we are already inside the PDFViewer class.

@@ +133,5 @@
> +
> +    if (viewerContainer !== undefined)
> +      return true;
> +    else
> +      return false;

Not sure about this if/else. `getElement()` will always return a non-null elementslib element. So you have to evaluate getNode() instead.

@@ +139,5 @@
> +
> +  /**
> +   * Wait for the document content to load
> +   */
> +  waitForDocumentLoad : function pdfViewer_waitForDocumentLoad() {

Can we just call this `waitForLoaded`?

::: lib/tests/testPdf.js
@@ +34,5 @@
> +  // Test all available elements
> +  NODES.forEach(function (aNode) {
> +    var node = pdfViewer.getElement({type: aNode.type});
> +
> +    assert.ok(node.exists(), "Element has been found");

nit: no need for the extra blank line.

@@ +37,5 @@
> +
> +    assert.ok(node.exists(), "Element has been found");
> +  });
> +
> +  // Check document opens with inline viewer

There is no need for those comments here. This is already part of the assertion message.

@@ +40,5 @@
> +
> +  // Check document opens with inline viewer
> +  assert.ok(pdfViewer.isInlineViewerUsed(), "Internal PDF viewer is used");
> +  // Get number of pages
> +  assert.ok(pdfViewer.numberOfPages >= 1, "Number of pages should be at least 1");

How many pages has the test pdf? I would say lets use an equal check here.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +12,5 @@
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> +
> +const STARTUP_PREF_NAME = "browser.startup.page";
> +
> +var setupModule = function (module) {

nit: aModule please.

@@ +33,5 @@
> +
> +  pdfViewer.waitForDocumentLoad();
> +
> +  assert.ok(pdfViewer.isInlineViewerUsed(),
> +            "PDF is opened with the internal PDF viewer");

I wonder if this check should be done inside of the currently existent `waitForDocumentLoad` method. Keep in mind that if it is not enabled we will spawn an external viewer! :( We should really avoid that because it cannot be closed. Any idea on how to prevent that?

@@ +35,5 @@
> +
> +  assert.ok(pdfViewer.isInlineViewerUsed(),
> +            "PDF is opened with the internal PDF viewer");
> +
> +  persisted.numberOfPages = pdfViewer.numberOfPages;

Something I would love to see if we could set page 2 and check after the restart that the same page is visible.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
@@ +16,5 @@
> +
> +/**
> + * Test that PDF is opened with inline PDF Viewer after a restart
> + */
> +var testPDFViewerAfterRestart = function (){

nit: missing blank before the '{' bracket.

@@ +18,5 @@
> + * Test that PDF is opened with inline PDF Viewer after a restart
> + */
> +var testPDFViewerAfterRestart = function (){
> +  assert.ok(pdfViewer.isInlineViewerUsed(),
> +            "PDF is opened with a different viewer after restart");

Not sure I understand this message. It seems wrong to me. Also is there no need to wait for the pdf being loaded first?

Comment 15

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v5

(In reply to Henrik Skupin (:whimboo) from comment #14)

> @@ +133,5 @@
> > +
> > +    if (viewerContainer !== undefined)
> > +      return true;
> > +    else
> > +      return false;
> 
> Not sure about this if/else. `getElement()` will always return a non-null
> elementslib element. So you have to evaluate getNode() instead.

viewerContainer is "undefined" if a plugin is used

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 718322 [details] [diff] [review]
patch v5

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

::: lib/pdf.js
@@ +125,5 @@
> +   */
> +  get enabled() {
> +    viewerContainer = this.getElement({type : "viewer"});
> +
> +    if (viewerContainer !== undefined)

So as said earlier this will not work. The element returned is always non-undefined. What you want here is:

`return !!viewerContainer.getNode();'

@@ +138,5 @@
> +  * @param {Number} aNumber
> +  *        The number of the page .
> +   */
> +  set currentPage(aNumber) {
> +    pn = new elementslib.ID(this._controller.tabs.activeTab, "pageNumber");

Please add this to getElements().

@@ +139,5 @@
> +  *        The number of the page .
> +   */
> +  set currentPage(aNumber) {
> +    pn = new elementslib.ID(this._controller.tabs.activeTab, "pageNumber");
> +    pn.getNode().value = '';

There is no need to reset the value. When the element gets the focus a single keypress will replace the former content.

@@ +152,5 @@
> +   */
> +  waitForLoaded : function pdfViewer_waitForLoaded() {
> +    assert.ok(this.enabled, "Internal PDF viewer is used");
> +
> +    container = new elementslib.ID(this._controller.tabs.activeTab, "pageContainer1");

Why doesn't it work with `#viewerContainer`? If we have to get this via this element you will have to create it in getElements() too. Also you missed the var keyword.

@@ +153,5 @@
> +  waitForLoaded : function pdfViewer_waitForLoaded() {
> +    assert.ok(this.enabled, "Internal PDF viewer is used");
> +
> +    container = new elementslib.ID(this._controller.tabs.activeTab, "pageContainer1");
> +    this._controller.waitForElement(container); 

nit: trailing space.

::: lib/tests/testPdf.js
@@ +16,5 @@
> +  {type: "viewer_currentView"},
> +  {type: "viewer_print"},
> +];
> +
> +var setupModule = function (module) {

Please use aModule and attach any created variable in setupModule to it.

@@ +27,5 @@
> +
> +function testPdfClass() {
> +  controller.open(LOCAL_TEST_FILE);
> +  controller.waitForPageLoad();
> +  pdfViewer.waitForLoaded();

mind adding an empty line above?

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +12,5 @@
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> +
> +const STARTUP_PREF_NAME = "browser.startup.page";
> +
> +var setupModule = function (aModule) {

Please use aModule correctly.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
@@ +8,5 @@
> +var prefs = require("../../../../lib/prefs");
> +
> +const STARTUP_PREF_NAME = "browser.startup.page";
> +
> +var setupModule = function (module) {

Use aModule correctly.

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v6

(In reply to Henrik Skupin (:whimboo) from comment #16)
> Comment on attachment 718322 [details] [diff] [review]
> patch v5
> 
> Review of attachment 718322 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: lib/pdf.js
> @@ +125,5 @@
> > +   */
> > +  get enabled() {
> > +    viewerContainer = this.getElement({type : "viewer"});
> > +
> > +    if (viewerContainer !== undefined)
> 
> So as said earlier this will not work. The element returned is always
> non-undefined. What you want here is:
> 
> `return !!viewerContainer.getNode();'

This works for documents opened with the PDF.js viewer, but if document is not opened with PDF.js viewer, the viewerContainer is "undefined".

> @@ +138,5 @@
> > +  * @param {Number} aNumber
> > +  *        The number of the page .
> > +   */
> > +  set currentPage(aNumber) {
> > +    pn = new elementslib.ID(this._controller.tabs.activeTab, "pageNumber");
>
> Please add this to getElements().

"pageNumber" is added in getElements() but here I needed to use elementslib in order to be able to change it's value. Otherwise I get an exception: "Permission denied for <resource://pdf.js> to create wrapper for object of class UnnamedClass" when typing the page number

> @@ +139,5 @@
> > +  *        The number of the page .
> > +   */
> > +  set currentPage(aNumber) {
> > +    pn = new elementslib.ID(this._controller.tabs.activeTab, "pageNumber");
> > +    pn.getNode().value = '';
> 
> There is no need to reset the value. When the element gets the focus a
> single keypress will replace the former content.

Done.

> @@ +152,5 @@
> > +   */
> > +  waitForLoaded : function pdfViewer_waitForLoaded() {
> > +    assert.ok(this.enabled, "Internal PDF viewer is used");
> > +
> > +    container = new elementslib.ID(this._controller.tabs.activeTab, "pageContainer1");
> 
> Why doesn't it work with `#viewerContainer`? If we have to get this via this
> element you will have to create it in getElements() too. Also you missed the
> var keyword.

There are 2 phases when a PDF document is loaded: first, only the viewer container (#viewerContainer) and then the document pages are loaded, each of them in it's own page container(#pageContainer1, #pageContainer2, ...). If getElement is used right after page loading (with pageContainer1), the document content (pages) won't be loaded and "container" will be undefined.

> @@ +153,5 @@
> > +  waitForLoaded : function pdfViewer_waitForLoaded() {
> > +    assert.ok(this.enabled, "Internal PDF viewer is used");
> > +
> > +    container = new elementslib.ID(this._controller.tabs.activeTab, "pageContainer1");
> > +    this._controller.waitForElement(container); 
> 
> nit: trailing space.
> 
> ::: lib/tests/testPdf.js
> @@ +16,5 @@
> > +  {type: "viewer_currentView"},
> > +  {type: "viewer_print"},
> > +];
> > +
> > +var setupModule = function (module) {
> 
> Please use aModule and attach any created variable in setupModule to it.
> 
> @@ +27,5 @@
> > +
> > +function testPdfClass() {
> > +  controller.open(LOCAL_TEST_FILE);
> > +  controller.waitForPageLoad();
> > +  pdfViewer.waitForLoaded();
> 
> mind adding an empty line above?
> 
> ::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
> @@ +12,5 @@
> > +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> > +
> > +const STARTUP_PREF_NAME = "browser.startup.page";
> > +
> > +var setupModule = function (aModule) {
> 
> Please use aModule correctly.
> 
> ::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
> @@ +8,5 @@
> > +var prefs = require("../../../../lib/prefs");
> > +
> > +const STARTUP_PREF_NAME = "browser.startup.page";
> > +
> > +var setupModule = function (module) {
> 
> Use aModule correctly.

All done

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #17)
> > > +  get enabled() {
> > > +    viewerContainer = this.getElement({type : "viewer"});
> > > +
> > > +    if (viewerContainer !== undefined)
> > 
> > So as said earlier this will not work. The element returned is always
> > non-undefined. What you want here is:
> > 
> > `return !!viewerContainer.getNode();'
> 
> This works for documents opened with the PDF.js viewer, but if document is
> not opened with PDF.js viewer, the viewerContainer is "undefined".

Oh wait. You are right. It's an element you have retrieved via getElement(). I missed that, so it looks nearly fine. One thing you should change is to use `if (viewerContainer)` that will even catch the null case and make it not fail.

> > @@ +138,5 @@
> > > +  * @param {Number} aNumber
> > > +  *        The number of the page .
> > > +   */
> > > +  set currentPage(aNumber) {
> > > +    pn = new elementslib.ID(this._controller.tabs.activeTab, "pageNumber");
> >
> > Please add this to getElements().
> 
> "pageNumber" is added in getElements() but here I needed to use elementslib
> in order to be able to change it's value. Otherwise I get an exception:
> "Permission denied for <resource://pdf.js> to create wrapper for object of
> class UnnamedClass" when typing the page number

That sounds like an issue we should solve then. Can you provide a little snippets which exercises this path and fails? Thanks.

> > > +  waitForLoaded : function pdfViewer_waitForLoaded() {
> > > +    assert.ok(this.enabled, "Internal PDF viewer is used");
> > > +
> > > +    container = new elementslib.ID(this._controller.tabs.activeTab, "pageContainer1");
> > 
> > Why doesn't it work with `#viewerContainer`? If we have to get this via this
> > element you will have to create it in getElements() too. Also you missed the
> > var keyword.
> 
> There are 2 phases when a PDF document is loaded: first, only the viewer
> container (#viewerContainer) and then the document pages are loaded, each of
> them in it's own page container(#pageContainer1, #pageContainer2, ...). If
> getElement is used right after page loading (with pageContainer1), the
> document content (pages) won't be loaded and "container" will be undefined.

So shouldn't we have two different waitForXXX methods here? One for the global loading state of the PDF viewer and another one for specific pages? That way we can explicitly wait for a given page to be loaded. You can pass it as parameter.

I will remove the review request until the above has been solved. In the future please ask for needinfo instead of review if open questions exist and if I miss the bug comment.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mihaela, have you had time to update the patch yet?

Comment 20

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v7

(In reply to Henrik Skupin (:whimboo) from comment #18)
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #17)
> > > > +  get enabled() {
> > > > +    viewerContainer = this.getElement({type : "viewer"});
> > > > +
> > > > +    if (viewerContainer !== undefined)
> > > 
> > > So as said earlier this will not work. The element returned is always
> > > non-undefined. What you want here is:
> > > 
> > > `return !!viewerContainer.getNode();'
> > 
> > This works for documents opened with the PDF.js viewer, but if document is
> > not opened with PDF.js viewer, the viewerContainer is "undefined".
> 
> Oh wait. You are right. It's an element you have retrieved via getElement().
> I missed that, so it looks nearly fine. One thing you should change is to
> use `if (viewerContainer)` that will even catch the null case and make it
> not fail.

Done: used "if (viewerContainer)"
> 
> > > @@ +138,5 @@
> > > > +  * @param {Number} aNumber
> > > > +  *        The number of the page .
> > > > +   */
> > > > +  set currentPage(aNumber) {
> > > > +    pn = new elementslib.ID(this._controller.tabs.activeTab, "pageNumber");
> > >
> > > Please add this to getElements().
> > 
> > "pageNumber" is added in getElements() but here I needed to use elementslib
> > in order to be able to change it's value. Otherwise I get an exception:
> > "Permission denied for <resource://pdf.js> to create wrapper for object of
> > class UnnamedClass" when typing the page number
> 
> That sounds like an issue we should solve then. Can you provide a little
> snippets which exercises this path and fails? Thanks.
> 

Created test_pdf_snippet.js under lib/tests.

> > > > +  waitForLoaded : function pdfViewer_waitForLoaded() {
> > > > +    assert.ok(this.enabled, "Internal PDF viewer is used");
> > > > +
> > > > +    container = new elementslib.ID(this._controller.tabs.activeTab, "pageContainer1");
> > > 
> > > Why doesn't it work with `#viewerContainer`? If we have to get this via this
> > > element you will have to create it in getElements() too. Also you missed the
> > > var keyword.
> > 
> > There are 2 phases when a PDF document is loaded: first, only the viewer
> > container (#viewerContainer) and then the document pages are loaded, each of
> > them in it's own page container(#pageContainer1, #pageContainer2, ...). If
> > getElement is used right after page loading (with pageContainer1), the
> > document content (pages) won't be loaded and "container" will be undefined.
> 
> So shouldn't we have two different waitForXXX methods here? One for the
> global loading state of the PDF viewer and another one for specific pages?
> That way we can explicitly wait for a given page to be loaded. You can pass
> it as parameter.

So far, I don't need to wait for a specific page to be loaded. I only need to check that first page is loaded because that's the point when some of the PDF viewer properties (like the number of pages, current page, etc) are populated. I couldn't find any other event that can be used for checking this.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry for being late here but I missed the needinfo flag. Is this patch up for review now or what specifically is open for needinfo?

Comment 22

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 740245 [details] [diff] [review]
patch v7

(In reply to Henrik Skupin (:whimboo) from comment #21)
> Sorry for being late here but I missed the needinfo flag. Is this patch up
> for review now or what specifically is open for needinfo?

Yes, the patch is for review. And it also needs some investigation regarding the "Permission denied for <resource://pdf.js> to create wrapper for object of class UnnamedClass" exception (for which I added the test_pdf_snippet.js under lib/tests to trigger it)

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 740245 [details] [diff] [review]
patch v7

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

Andreea, would you mind taking this review? My plate is kinda full and I don't want to delay it even more. I would kinda like to give the final review later once it's proven that all works. Thanks!

Comment 24

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 740245 [details] [diff] [review]
patch v7

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

::: lib/pdf.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> +* License, v. 2.0. If a copy of the MPL was not distributed with this
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**

Please add "use strict";, we use this now, in all tests/libraries, right after the licence block.

@@ +54,5 @@
> +  getElement : function pdfViewer_getElement(aSpec) {
> +    var elements = this.getElements(aSpec);
> +
> +    return (elements.length > 0) ? elements[0] : undefined;
> +  },

We won't need this anymore.

@@ +87,5 @@
> +      case "viewer_pageNumber":
> +        nodeCollector.queryNodes("#pageNumber");
> +        break;
> +      case "viewer_currentView":
> +        nodeCollector.queryNodes("#viewBookmark");

I think we can use elementslib.ID here, since all of them are ids. Eventually in the future if needed we can add nodeCollector, but from what I inspected, it's well organized. I don't see the error anymore this way.

@@ +124,5 @@
> +
> +    if (viewerContainer)
> +      return true;
> +    else
> +      return false;

We could use here return viewerContainer ? true : false, to be more compact.

@@ +131,5 @@
> +  /**
> +   * Set the current page to a desired number
> +   *
> +  * @param {Number} aNumber
> +  *        The number of the page .

Whitespaces until we see the dot.

@@ +136,5 @@
> +   */
> +  set currentPage(aNumber) {
> +    var pn = new elementslib.ID(this._controller.tabs.activeTab, "pageNumber");
> +
> +    pn.getNode().click();

Lets use this._controller.click(pn) here as we did so far, until 2.0 comes up. No need for the blank line from above, the variable is linked to the actions over it.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include required modules

Please add "use strict";, we use this now, in all tests/libraries, right after the licence block.

@@ +10,5 @@
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../../data/");
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> +
> +const STARTUP_PREF_NAME = "browser.startup.page";

Please change this to PREF_STARTUP. PREF has to be a prefix.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
@@ +6,5 @@
> +var { assert } = require("../../../../lib/assertions");
> +var pdf = require("../../../../lib/pdf");
> +var prefs = require("../../../../lib/prefs");
> +
> +const STARTUP_PREF_NAME = "browser.startup.page";

Same here for the pref name.

@@ +20,5 @@
> +var testPDFViewerAfterRestart = function () {
> +  pdfViewer.waitForLoaded();
> +
> +  assert.equal(persisted.numberOfPages, pdfViewer.numberOfPages,
> +               "Number of pages is the same as before restart");

"The number of pages has been maintained after restart"

@@ +22,5 @@
> +
> +  assert.equal(persisted.numberOfPages, pdfViewer.numberOfPages,
> +               "Number of pages is the same as before restart");
> +  assert.equal(persisted.currentPage, pdfViewer.currentPage,
> +               "Document displays same page after after restart");

"The same page has been displayed after restart"

@@ +27,5 @@
> +}
> +
> +var teardownModule = function () {
> +  prefs.preferences.clearUserPref(STARTUP_PREF_NAME);
> +}

We also need to delete the persisted object.

Comment 25

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v8

Made the requested changes, but I have a problem with restoring the session on latest Nightly(27.0) build: the tab is not restored after restart. It works fine on latest Aurora (26.0) and when running the test manually on the same Nightly build.

Can someone please help with this issue?

Comment 26

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #25)
> Created attachment 822262 [details] [diff] [review]
> patch v8
> 
> Made the requested changes, but I have a problem with restoring the session
> on latest Nightly(27.0) build: the tab is not restored after restart. It
> works fine on latest Aurora (26.0) and when running the test manually on the
> same Nightly build.
> 
> Can someone please help with this issue?

I tracked down the Firefox regression window to the following pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=423b9c30c73d&tochange=4e7d1e2c93a6
Possible cause of issue: bug 923331

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Might be. Please continue with inbound builds so we can be fully sure. If you can't even reproduce manually then it would be perfect. But in any case please file a new bug against Firefox. Thanks.

Comment 28

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> Might be. Please continue with inbound builds so we can be fully sure. If
> you can't even reproduce manually then it would be perfect. But in any case
> please file a new bug against Firefox. Thanks.

I logged bug 932302 and provided the info there.

Comment 29

[Mihaela Velimiroviciu (:mihaelav)]

Bug 932302 seems to be fixed now and session is restored successfully on latest Nightly 28.0a1 (20131029030201) and Aurora 27.0a2 (20131029134505) builds.

Comment 30

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 822262 [details] [diff] [review]
patch v8

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

We're getting closer! :)

::: lib/pdf.js
@@ +53,5 @@
> +   *
> +   * @returns {ElemBase} The element which has been found
> +   */
> +  getElement : function pdfViewer_getElements(aSpec) {
> +  

Trailing whitespaces.

@@ +69,5 @@
> +      case "viewer_currentView":
> +        elem = new elementslib.ID(this._controller.window.document, "viewBookmark");
> +        break
> +      default:
> +        assert.fail(arguments.callee.name + ": Unknown element type - " + spec.type);

We don't use arguments.calee.name anymore, actually I think I should file a bug to remove these cases.

@@ +71,5 @@
> +        break
> +      default:
> +        assert.fail(arguments.callee.name + ": Unknown element type - " + spec.type);
> +    }
> +    //return nodeCollector.elements;

We can remove this now.

@@ +111,5 @@
> +  * @param {Number} aNumber
> +  *        The number of the page.
> +  */
> +  set currentPage(aNumber) {
> +    var pn = new elementslib.ID(this._controller.window.document, 'pageNumber');

Lets use pageNumber as the variable name.

@@ +116,5 @@
> +    this._controller.click(pn);
> +    this._controller.type(pn, String(aNumber));
> +    this._controller.keypress(pn, "VK_ENTER", {});
> +    pn.getNode().setAttribute('value', aNumber);
> +    return this.currentPage;

We should have return separated from the other lines.

::: tests/functional/restartTests/testPDF_restartWithOpenPDF/test2.js
@@ +29,5 @@
> +
> +var teardownModule = function (aModule) {
> +  prefs.preferences.clearUserPref(PREF_STARTUP);
> +  delete persisted.currentPage;
> +

You should also delete persisted.numberOfPages.

Comment 31

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: v9

Made the changes suggested in comment #30.

Comment 32

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 824677 [details] [diff] [review]
v9

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

Looks good to me now, tested it with 2.0 as well. One small nit though. Please request review from Henrik and Dave after you fix that.

::: lib/pdf.js
@@ +90,5 @@
> +   *
> +   * @returns {Number} Number of pages of the document
> +   */
> +  get numberOfPages() {
> +    return parseInt(this.getElement({type : "viewer_pageNumber"}).getNode().getAttribute('max'));

This line exceeds too much the 80char limit. You can move .getNode().getAttribute('max')); under .getElement

Comment 33

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v10

(In reply to Andreea Matei [:AndreeaMatei] from comment #32)
> ::: lib/pdf.js
> @@ +90,5 @@
> > +   *
> > +   * @returns {Number} Number of pages of the document
> > +   */
> > +  get numberOfPages() {
> > +    return parseInt(this.getElement({type : "viewer_pageNumber"}).getNode().getAttribute('max'));
> 
> This line exceeds too much the 80char limit. You can move
> .getNode().getAttribute('max')); under .getElement

Updated that and also in a few other places where the lines were longer. Thanks!

Comment 34

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 832760 [details] [diff] [review]
patch v10

I'm going to defer to Henrik for this review.

Comment 35

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v11

I only updated the folders structure, because it changed since the previous patch was created.

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8365993 [details] [diff] [review]
patch v11

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

Sorry for the long delay. But finally I got to it. Lets try to continue with those tests given that they seem to be very useful.

::: firefox/lib/pdf.js
@@ +5,5 @@
> +"use strict";
> +
> +/**
> + * @fileoverview
> + * The PDF API adds support for pdf related functions.

This is a ui module, and has to placed into the appropriate folder. Also is this only for Firefox? Or is/was it also accessible in Metro builds? If that is the case the top-level lib folder has to be used.

@@ +21,5 @@
> + * @param {MozMillController} aController
> + *        MozMill controller of the browser window to operate on.
> + */
> +function PDFViewer(aController) {
> +  this._controller = aController;

Lets assert that we have a valid controller here.

@@ +52,5 @@
> +   *                             [optional - default: document]
> +   *
> +   * @returns {ElemBase} The element which has been found
> +   */
> +  getElement : function pdfViewer_getElements(aSpec) {

Move this method down behind all getters and setters, and in alphabetical order. Also this is getElements and not getElement, and has to return an array. Create another getElement() method which returns a single element. See the other ui classes.

@@ +54,5 @@
> +   * @returns {ElemBase} The element which has been found
> +   */
> +  getElement : function pdfViewer_getElements(aSpec) {
> +
> +    var spec = aSpec || { };

nit: remove the blank line.

@@ +63,5 @@
> +      case "viewer":
> +        elem = new elementslib.ID(this._controller.window.document,
> +                                  "viewerContainer");
> +        break;
> +      case "viewer_pageNumber":

nit: please add a blank line after break so we can separate the cases a bit.

@@ +84,5 @@
> +   *
> +   * @returns {Number} Index of current viewed page
> +   */
> +  get currentPage() {
> +    return parseInt(this.getElement({type : "viewer_pageNumber"}).getNode().value);

Lets make that two line and get the element first. This is kinda hard to read. Also setters and getters should live next to each other.

@@ +94,5 @@
> +   * @returns {Number} Number of pages of the document
> +   */
> +  get numberOfPages() {
> +    return parseInt(this.getElement({type : "viewer_pageNumber"})
> +                        .getNode().getAttribute('max'));

Same here. Also what happens if the attribute doesn't exit? Do we get an exception?

@@ +105,5 @@
> +   */
> +  get enabled() {
> +    var viewerContainer = this.getElement({type : "viewer"});
> +
> +    return viewerContainer ? true : false;

This would always return true. You want to check with .getNode(). Maybe you return '!!container.getNode()'?

@@ +117,5 @@
> +  */
> +  set currentPage(aNumber) {
> +    var pageNumber = new elementslib.ID(this._controller.window.document,
> +                                        "pageNumber");
> +    this._controller.click(pageNumber);

Replace all those event calls with 'pageNumber.XYZ()'.

@@ +119,5 @@
> +    var pageNumber = new elementslib.ID(this._controller.window.document,
> +                                        "pageNumber");
> +    this._controller.click(pageNumber);
> +    this._controller.type(pageNumber, String(aNumber));
> +    this._controller.keypress(pageNumber, "VK_ENTER", {});

VK_RETURN please

@@ +120,5 @@
> +                                        "pageNumber");
> +    this._controller.click(pageNumber);
> +    this._controller.type(pageNumber, String(aNumber));
> +    this._controller.keypress(pageNumber, "VK_ENTER", {});
> +    pageNumber.getNode().setAttribute('value', aNumber);

Why do you manipulate the value directly? It should have been done already by the above events. You probably want to assert that the contained value is the one you expect.

@@ +132,5 @@
> +  waitForLoaded : function pdfViewer_waitForLoaded() {
> +    assert.ok(this.enabled, "Internal PDF viewer is used");
> +
> +    var container = new elementslib.ID(this._controller.tabs.activeTab,
> +                                       "pageContainer1");

Why are all the other elements in getElements() based on window.document while this is on tabs.activeTab? You should also move this into that method.

::: firefox/lib/tests/testPdf.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

strict please

@@ +10,5 @@
> +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/");
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> +
> +const NODES = [
> +  {type: "viewer"},

This is not the type but the id. But you also want to add the type here for each element.

@@ +15,5 @@
> +  {type: "viewer_pageNumber"},
> +  {type: "viewer_currentView"},
> +];
> +
> +var setupModule = function (aModule) {

No var declaration please.

@@ +24,5 @@
> +  aModule.tabBrowser.closeAllTabs();
> +}
> +
> +function testPdfClass(aModule) {
> +  controller.open(LOCAL_TEST_FILE);

What about testing enabled==false before? Would be good to have this tested too.

@@ +32,5 @@
> +
> +  // Test all available elements
> +  NODES.forEach(function (aNode) {
> +    var node = pdfViewer.getElement({type: aNode.type});
> +    assert.ok(node, "Element has been found");

Please check for localName and the expected type. This is not a valid check and would always be true.

@@ +36,5 @@
> +    assert.ok(node, "Element has been found");
> +  });
> +
> +  pdfViewer.currentPage = 2;
> +  assert.equal(pdfViewer.numberOfPages, 2, "Number of pages should be 2");

Put this earlier after loading.

@@ +37,5 @@
> +  });
> +
> +  pdfViewer.currentPage = 2;
> +  assert.equal(pdfViewer.numberOfPages, 2, "Number of pages should be 2");
> +  assert.equal(pdfViewer.currentPage, 2, "Current page should be 2");

Not necessary once we have the assert in the setter.

::: firefox/tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
@@ +14,5 @@
> +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> +
> +const PREF_STARTUP = "browser.startup.page";
> +
> +var setupModule = function (aModule) {

No var declaration

@@ +36,5 @@
> +  pdfViewer.waitForLoaded();
> +
> +  persisted.numberOfPages = pdfViewer.numberOfPages;
> +  pdfViewer.currentPage = 2;
> +  persisted.currentPage = pdfViewer.currentPage;

Create an object below persisted like .pdfViewer. That way it can be removed better at the end.

@@ +39,5 @@
> +  pdfViewer.currentPage = 2;
> +  persisted.currentPage = pdfViewer.currentPage;
> +}
> +
> +var teardownModule = function (aModule) {

No var declaration.

@@ +42,5 @@
> +
> +var teardownModule = function (aModule) {
> +  // restart the browser in Mozmill 2.0
> +  if ("restartApplication" in aModule.controller)
> +    aModule.controller.restartApplication();

We have mozmill 2.0 now. So the if condition can be removed. Also I wold like to see that we can have those two test files as a single file restart test. That makes a lot of things easier.

Comment 37

[Mihaela Velimiroviciu (:mihaelav)]

Attachment: patch v12

(In reply to Henrik Skupin (:whimboo) from comment #36)
> Comment on attachment 8365993 [details] [diff] [review]
> updated folder structure
> 

Updated all the requested changes, except the following:

> ::: firefox/lib/pdf.js

> @@ +105,5 @@
> > +   */
> > +  get enabled() {
> > +    var viewerContainer = this.getElement({type : "viewer"});
> > +
> > +    return viewerContainer ? true : false;
> 
> This would always return true. You want to check with .getNode(). Maybe you
> return '!!container.getNode()'?

container.getNode() throws an exception if viewerContainer is not found. It works fine if I return !!viewerContainer;
 
> @@ +117,5 @@
> > +  */
> > +  set currentPage(aNumber) {
> > +    var pageNumber = new elementslib.ID(this._controller.window.document,
> > +                                        "pageNumber");
> > +    this._controller.click(pageNumber);
> 
> Replace all those event calls with 'pageNumber.XYZ()'.

Replaced for click(); didn't find replacement for the others.

> ::: firefox/lib/tests/testPdf.js
> @@ +10,5 @@
> > +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/");
> > +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> > +
> > +const NODES = [
> > +  {type: "viewer"},
> 
> This is not the type but the id. But you also want to add the type here for
> each element.

This is the type as defined in the library. The ids are #viewerContainer, #pageNumber, #viewBookmark.

> @@ +32,5 @@
> > +
> > +  // Test all available elements
> > +  NODES.forEach(function (aNode) {
> > +    var node = pdfViewer.getElement({type: aNode.type});
> > +    assert.ok(node, "Element has been found");
> 
> Please check for localName and the expected type. This is not a valid check
> and would always be true.

This fails if the element is not found

> ::: firefox/tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
> @@ +36,5 @@
> > +  pdfViewer.waitForLoaded();
> > +
> > +  persisted.numberOfPages = pdfViewer.numberOfPages;
> > +  pdfViewer.currentPage = 2;
> > +  persisted.currentPage = pdfViewer.currentPage;
> 
> Create an object below persisted like .pdfViewer. That way it can be removed
> better at the end.

No need to do that since it's all in one file

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8418748 [details] [diff] [review]
patch v12

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

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #37)
> > ::: firefox/lib/pdf.js
> 
> > @@ +105,5 @@
> > > +   */
> > > +  get enabled() {
> > > +    var viewerContainer = this.getElement({type : "viewer"});
> > > +
> > > +    return viewerContainer ? true : false;
> > 
> > This would always return true. You want to check with .getNode(). Maybe you
> > return '!!container.getNode()'?
> 
> container.getNode() throws an exception if viewerContainer is not found. It
> works fine if I return !!viewerContainer;

As said earlier this cannot be done! But what you can do is simply: 'return viewerContainer.exists()'.

> > @@ +117,5 @@
> > > +  */
> > > +  set currentPage(aNumber) {
> > > +    var pageNumber = new elementslib.ID(this._controller.window.document,
> > > +                                        "pageNumber");
> > > +    this._controller.click(pageNumber);
> > 
> > Replace all those event calls with 'pageNumber.XYZ()'.
> 
> Replaced for click(); didn't find replacement for the others.

Replacement for which others? Please be more specific, and do not add a new patch until all the review comments have actually been addressed.

> > ::: firefox/lib/tests/testPdf.js
> > @@ +10,5 @@
> > > +const LOCAL_TEST_FOLDER = collector.addHttpResource("../../../data/");
> > > +const LOCAL_TEST_FILE = LOCAL_TEST_FOLDER + "pdf/test.pdf";
> > > +
> > > +const NODES = [
> > > +  {type: "viewer"},
> > 
> > This is not the type but the id. But you also want to add the type here for
> > each element.
> 
> This is the type as defined in the library. The ids are #viewerContainer,
> #pageNumber, #viewBookmark.

So check the other library tests how they do this. We want to have all the same.

> > @@ +32,5 @@
> > > +
> > > +  // Test all available elements
> > > +  NODES.forEach(function (aNode) {
> > > +    var node = pdfViewer.getElement({type: aNode.type});
> > > +    assert.ok(node, "Element has been found");
> > 
> > Please check for localName and the expected type. This is not a valid check
> > and would always be true.
> 
> This fails if the element is not found

Exactly that is what we want here. The element has to exist and needs to be of the expected type.

> > ::: firefox/tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
> > @@ +36,5 @@
> > > +  pdfViewer.waitForLoaded();
> > > +
> > > +  persisted.numberOfPages = pdfViewer.numberOfPages;
> > > +  pdfViewer.currentPage = 2;
> > > +  persisted.currentPage = pdfViewer.currentPage;
> > 
> > Create an object below persisted like .pdfViewer. That way it can be removed
> > better at the end.
> 
> No need to do that since it's all in one file

You misunderstood. Please do not add a couple of different properties to persisted but use a single object like persisted.pdfViewer = { ... }.

Comment 39

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Henrik Skupin (:whimboo) from comment #38)
> Comment on attachment 8418748 [details] [diff] [review]
> updated patch
> 
> Review of attachment 8418748 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #37)
> > > ::: firefox/lib/pdf.js
> > > @@ +117,5 @@
> > > > +  */
> > > > +  set currentPage(aNumber) {
> > > > +    var pageNumber = new elementslib.ID(this._controller.window.document,
> > > > +                                        "pageNumber");
> > > > +    this._controller.click(pageNumber);
> > > 
> > > Replace all those event calls with 'pageNumber.XYZ()'.
> > 
> > Replaced for click(); didn't find replacement for the others.
> 
> Replacement for which others? Please be more specific, and do not add a new
> patch until all the review comments have actually been addressed.

sendKeys() doesn't work ATM on the pageNumber element because it is of type "number" which is not yet supported by Mozmill (bug 1007596).

> > > ::: firefox/tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
> > > @@ +36,5 @@
> > > > +  pdfViewer.waitForLoaded();
> > > > +
> > > > +  persisted.numberOfPages = pdfViewer.numberOfPages;
> > > > +  pdfViewer.currentPage = 2;
> > > > +  persisted.currentPage = pdfViewer.currentPage;
> > > 
> > > Create an object below persisted like .pdfViewer. That way it can be removed
> > > better at the end.
> > 
> > No need to do that since it's all in one file
> 
> You misunderstood. Please do not add a couple of different properties to
> persisted but use a single object like persisted.pdfViewer = { ... }.

I was saying that since this will be a single file restart test we don't need to use the persisted object anymore as we can access all the variables.

Comment 40

[Mihaela Velimiroviciu (:mihaelav)]

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #39)
> > > > ::: firefox/tests/functional/restartTests/testPDF_restartWithOpenPDF/test1.js
> > > > @@ +36,5 @@
> > > > > +  pdfViewer.waitForLoaded();
> > > > > +
> > > > > +  persisted.numberOfPages = pdfViewer.numberOfPages;
> > > > > +  pdfViewer.currentPage = 2;
> > > > > +  persisted.currentPage = pdfViewer.currentPage;
> > > > 
> > > > Create an object below persisted like .pdfViewer. That way it can be removed
> > > > better at the end.
> > > 
> > > No need to do that since it's all in one file
> > 
> > You misunderstood. Please do not add a couple of different properties to
> > persisted but use a single object like persisted.pdfViewer = { ... }.
> 
> I was saying that since this will be a single file restart test we don't
> need to use the persisted object anymore as we can access all the variables.

Actually, we do need that, sorry.


Henrik, what about the other issue (with sendKeys). Do you want to wait until we have a new Mozmill release which supports that for input type number, or to do a work around (maybe just setting the value directly with pageNumber.getNode().value = aNumber; ?) for the moment?

Comment 41

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I will work on a patch for Mozmill so we can have this released soon. So I would wait.

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Mozmill is dead, WONTFIX the remaining bugs.