1005811 - Update Mozmill tests for in-content preferences pane

Reporter

Description

•

11 years ago

As we can see in bug 738797, the default behavior when opening preferences will be replaced with the new inContent page (about:preferences). Some of our tests will fail when waiting for the preferences window. All tests from : 'functional/testPreferences/' and 'remote/testPreferences/'. We need 'browser.preferences.inContent' set to false in this tests. I'll come with a fix soon & we can get this landed before to avoid the future failures.

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

Comment 1

•

11 years ago

I don't see the reasoning for such a summary of the bug. Why do we want to ultimately do this? What we want is support for in-content preferences. Updating the pref is a simple workaround for now, so not all of our tests are failing. Please see the big picture when filing bugs.

Summary: Set 'browser.preferences.inContent' to false when using the preferences window → Update Mozmill tests for in-content preferences pane

Daniel Gherasim

Reporter

Comment 2

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #1) > I don't see the reasoning for such a summary of the bug. Why do we want to > ultimately do this? What we want is support for in-content preferences. > Updating the pref is a simple workaround for now, so not all of our tests > are failing. Please see the big picture when filing bugs. Ok. After the workaround, we should refactor our code to test the new in-content page instead of the old window pref & completely remove testing it, right ?

Andrei Eftimie

Comment 3

•

11 years ago

(In reply to daniel.gherasim from comment #2) > (In reply to Henrik Skupin (:whimboo) from comment #1) > > I don't see the reasoning for such a summary of the bug. Why do we want to > > ultimately do this? What we want is support for in-content preferences. > > Updating the pref is a simple workaround for now, so not all of our tests > > are failing. Please see the big picture when filing bugs. > > Ok. After the workaround, we should refactor our code to test the new > in-content page instead of the old window pref & completely remove testing > it, right ? We might need new tests, and some of the ones we have now might not be viable anymore. I haven't checked if _all_ functionality is preserved (it should be). But the UI seems greatly simplified.

Daniel Gherasim

Reporter

Comment 4

•

11 years ago

Attached patch bug_1005811_wa.patch (obsolete) — Details — Splinter Review

This is the workaround patch.

Attachment #8417318 - Flags: review?(andrei.eftimie)

Attachment #8417318 - Flags: review?(andreea.matei)

Daniel Gherasim

Reporter

Comment 5

•

11 years ago

Once bug 738797 is fixed on m-c, most likely tomorrow or these days, our pref tests will fail, so marking this as P1.

Priority: -- → P1

Andrei Eftimie

Comment 6

•

11 years ago

Comment on attachment 8417318 [details] [diff] [review] bug_1005811_wa.patch Review of attachment 8417318 [details] [diff] [review]: ----------------------------------------------------------------- In bug 738797 I see some specific code for Win XP. Please check that, if there's a special case in XP we might need to handle it differently. r+ from me with the nit fixed. ::: firefox/tests/functional/restartTests/testPreferences_masterPassword/test1.js @@ +15,5 @@ > > const BASE_URL = collector.addHttpResource("../../../../../data/"); > const TEST_DATA = BASE_URL + "password_manager/login_form.html"; > > +const PREF_BROWSER_IN_CONTENT = 'browser.preferences.inContent'; nit: please use double quotes (for consistency). I'm not sure about the variable name... I suppose it will do.

Attachment #8417318 - Flags: review?(andrei.eftimie)

Attachment #8417318 - Flags: review?(andreea.matei)

Attachment #8417318 - Flags: review-

Daniel Gherasim

Reporter

Comment 7

•

11 years ago

•

11 years ago

(In reply to Andrei Eftimie from comment #10) > Comment on attachment 8417862 [details] [diff] [review] > bug_1005811_wa_2.patch > > Review of attachment 8417862 [details] [diff] [review]: > ----------------------------------------------------------------- > > I have a conflict in testPreferences/testPaneRetention.js > Worked on wrong branch, will fix that. > ::: firefox/tests/l10n/testAccessKeys/test1.js > @@ +20,5 @@ > > const WINDOW_MODAL= domUtils.DOMWalker.WINDOW_MODAL; > > const WINDOW_NEW = domUtils.DOMWalker.WINDOW_NEW; > > > > +const PREF_BROWSER_IN_CONTENT = "browser.preferences.inContent"; > > + > > So the 'instantApply' pref is not needed for the l10n tests? Oups, forgot to update those tests, will do that now.

Daniel Gherasim

Reporter

Comment 12

•

11 years ago

Attached patch bug_1005811_wa_3.patch (obsolete) — Details — Splinter Review

Assignee: nobody → daniel.gherasim

Attachment #8417862 - Attachment is obsolete: true

Status: NEW → ASSIGNED

Attachment #8418006 - Flags: review?(andrei.eftimie)

Attachment #8418006 - Flags: review?(andreea.matei)

Daniel Gherasim

Reporter

Comment 13

•

11 years ago

Attached patch bug_1005811_wa_3.1.patch (obsolete) — Details — Splinter Review

Also updated the templates/testPreferencesDialog.js.

Attachment #8418006 - Attachment is obsolete: true

Attachment #8418006 - Flags: review?(andrei.eftimie)

Attachment #8418006 - Flags: review?(andreea.matei)

Attachment #8418008 - Flags: review?(andrei.eftimie)

Attachment #8418008 - Flags: review?(andreea.matei)

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

Comment 14

•

11 years ago

Why has this not been reviewed yesterday? This is a P1 and has to land ASAP.

Flags: needinfo?(andreea.matei)

Andreea Matei [:AndreeaMatei]

Comment 15

•

11 years ago

Comment on attachment 8418008 [details] [diff] [review] bug_1005811_wa_3.1.patch Review of attachment 8418008 [details] [diff] [review]: ----------------------------------------------------------------- Just a few changes left. ::: firefox/tests/functional/restartTests/testPreferences_masterPassword/test1.js @@ +34,5 @@ > var teardownModule = function(aModule) { > controller.open("about:newtab"); > > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); Blank line after this block. ::: firefox/tests/functional/restartTests/testPreferences_masterPassword/test2.js @@ +11,5 @@ > var utils = require("../../../../lib/utils"); > > +const PREF_BROWSER_IN_CONTENT = "browser.preferences.inContent"; > +const PREF_BROWSER_INSTANT_APPLY = "browser.preferences.instantApply"; > + Please remove this extra line @@ +24,5 @@ > } > > var teardownModule = function(aModule) { > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); Same here, blank line after. ::: firefox/tests/functional/restartTests/testPreferences_masterPassword/test3.js @@ +21,5 @@ > } > > var teardownModule = function(aModule) { > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); And here. ::: firefox/tests/functional/testPreferences/testDisableCookies.js @@ +29,5 @@ > > var teardownModule = function(aModule) { > prefs.preferences.clearUserPref("network.cookie.cookieBehavior"); > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); blank line after this block. ::: firefox/tests/functional/testPreferences/testEnableCookies.js @@ +28,5 @@ > } > > var teardownModule = function(aModule) { > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); Please keep this prefs block separate from the rest. ::: firefox/tests/functional/testPreferences/testPasswordNotSaved.js @@ +28,5 @@ > } > > var teardownModule = function(aModule) { > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); Same here. ::: firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js @@ +31,5 @@ > } > > function teardownModule() { > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); Separate block. ::: firefox/tests/functional/testPreferences/testRemoveCookie.js @@ +28,5 @@ > } > > var teardownModule = function(aModule) { > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); Separate block. ::: firefox/tests/remote/testPreferences/testRemoveAllCookies.js @@ +30,5 @@ > } > > var teardownModule = function(aModule) { > + prefs.preferences.clearUserPref(PREF_BROWSER_IN_CONTENT); > + prefs.preferences.clearUserPref(PREF_BROWSER_INSTANT_APPLY); Separate block.

Attachment #8418008 - Flags: review?(andrei.eftimie)

Attachment #8418008 - Flags: review?(andreea.matei)

Attachment #8418008 - Flags: review-

Andreea Matei [:AndreeaMatei]

Comment 16

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #14) > Why has this not been reviewed yesterday? This is a P1 and has to land ASAP. Indeed, I will discuss with Andrei to better prioritize the reviews we're doing first. Would be great if the dashboard review list would have the priority as well to help with this.

Flags: needinfo?(andreea.matei)

Daniel Gherasim

Reporter

Comment 17

•

11 years ago

Attached patch bug_1005811_wa_3.2.patch (obsolete) — Details — Splinter Review

http://mozmill-crowd.blargon7.com/#/functional/report/ca725ca957252135fc72c3c08f21d6f0

Attachment #8418008 - Attachment is obsolete: true

Attachment #8419323 - Flags: review?(andrei.eftimie)

Attachment #8419323 - Flags: review?(andreea.matei)

Daniel Gherasim

Reporter

Comment 18

•

11 years ago

Windows: http://mozmill-crowd.blargon7.com/#/functional/report/ca725ca957252135fc72c3c08f221682

Andreea Matei [:AndreeaMatei]

Comment 19

•

11 years ago

Comment on attachment 8419323 [details] [diff] [review] bug_1005811_wa_3.2.patch Review of attachment 8419323 [details] [diff] [review]: ----------------------------------------------------------------- Great, landed: http://hg.mozilla.org/qa/mozmill-tests/rev/d60f684ee6d1 (default)

Attachment #8419323 - Flags: review?(andrei.eftimie)

Attachment #8419323 - Flags: review?(andreea.matei)

Attachment #8419323 - Flags: review+

Andreea Matei [:AndreeaMatei]

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #21) > What's the status here? We have to make some progress on that change given > that in-content prefs are default now. Hi Henrik, I can also start working on this while my other tasks are blocked. How would we handle the changes, all in the same patch on this bug ? I will look to all tests and what will need to be change to start a discussion where things may be unclear.

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

Comment 23

•

11 years ago

Well, mainly I think only the prefs.js module will have to be changed here. So all the tests should pass afterward. l10n tests might need special tweaking, so a different patch here would be good to have.

Daniel Gherasim

Reporter

Comment 24

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #23) > Well, mainly I think only the prefs.js module will have to be changed here. > So all the tests should pass afterward. l10n tests might need special > tweaking, so a different patch here would be good to have. Just one more question before starting working on this. How about having this new folder firefox/lib/ui/in-content-pages/ ? and add the new prefs.js library there. I remember we are also working on another in-content page (the security add exception one) and that can live there too. Does this sound good for you ? In the last prefs.js we can leave the 'preferences' object to access to nsIPrefBranch and add additional back-end code if it will be needed.

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

Comment 25

•

11 years ago

I wouldn't stick it under such a folder, but we should separate out the ui parts of prefs.js, that's correct. So lib/ui/preferences should be fine. Maybe you want to check how the page is called internally.

Daniel Gherasim

Reporter

Comment 26

•

11 years ago

Attached patch WIP.patch (obsolete) — Details — Splinter Review

I would like some feedback here before continue on other tests.

Attachment #8433361 - Flags: feedback?(hskupin)

Attachment #8433361 - Flags: feedback?(andrei.eftimie)

Attachment #8433361 - Flags: feedback?(andreea.matei)

Andrei Eftimie

Comment 27

•

11 years ago

Comment on attachment 8433361 [details] [diff] [review] WIP.patch Review of attachment 8433361 [details] [diff] [review]: ----------------------------------------------------------------- Good start. ::: firefox/lib/ui/preferences.js @@ +31,5 @@ > + get controller() { > + return this._controller; > + }, > + /** > + * Retrieve list of UI elements based on the given specification nit: space before comment, indentation @@ +59,5 @@ > + * @param {object} aSpec > + * Information of the UI elements which should be retrieved > + * @parma {string} aSpec.type > + * Identifier of the element > + * @param {string} [aSpec.subtype] subtype seems to be required @@ +61,5 @@ > + * @parma {string} aSpec.type > + * Identifier of the element > + * @param {string} [aSpec.subtype] > + * Attribute of the element to filter > + * @param {string} [aSpec.value] value is not used @@ +74,5 @@ > + var parent = spec.parent; > + > + var elems = null; > + var root = parent ? parent.getNode() : this._controller.window.document; > + var nodeCollector = new domUtils.nodeCollector(root); You don't have a use for NodeCollector in this method. (At the moment at least) @@ +92,5 @@ > + > + return elems; > + }, > + > + openPreferences : function PreferencesPage_openPreferences(aPane) { I think we can name this method simply `open`. API wise we're calling: `PreferencePage.open()`. Should be clear enough. @@ +95,5 @@ > + > + openPreferences : function PreferencesPage_openPreferences(aPane) { > + this._controller.open("about:preferences"); > + this._controller.waitForPageLoad(); > + this.switchPane(aPane); I think aPane should be optional, and if omitted we should not attempt to this switch. ::: firefox/tests/functional/testPreferences/testDefaultPhishingEnabled.js @@ +21,3 @@ > > + var attackElem = prefsPage.getElement({type: "contentItem", > + subtype : "blockAttackSites"}); nit: indent keys ::: firefox/tests/functional/testPreferences/testDisableCookies.js @@ +59,3 @@ > } > > +function prefSelectCustomHistory(aController) { we should also send the value as an argument to this function, move the function in the preference lib, and use the same method in other tests where we need it @@ +62,3 @@ > // Go to custom history settings and click on the show cookies button > + var historyMode = prefsPage.getElement({type: "contentItem", subtype: "historyMode"}); > + aController.select(historyMode, null, null, "custom"); historyMode.select()

Attachment #8433361 - Flags: feedback?(andrei.eftimie) → feedback+

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

Comment 28

•

11 years ago

Comment on attachment 8433361 [details] [diff] [review] WIP.patch Review of attachment 8433361 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a good start, but there are still some API changes necessary. See my inline comments. ::: firefox/lib/ui/preferences.js @@ +11,5 @@ > +var domUtils = require("../../../lib/dom-utils"); > +var utils = require("../utils"); > + > +/** > + * 'Preferences' in content page class I would say: 'The in-content preferences user interface'. @@ +14,5 @@ > +/** > + * 'Preferences' in content page class > + * > + * @constructor > + * @param {MozMillController} aController optional as visible below? @@ +17,5 @@ > + * @constructor > + * @param {MozMillController} aController > + * MozMillController of the Preferences Page > + */ > +function PreferencesPage(aController) { We don't need the Page suffix here. See the addon manager (which needs to moved out to its own ui module too - filing a bug for it?). @@ +61,5 @@ > + * @parma {string} aSpec.type > + * Identifier of the element > + * @param {string} [aSpec.subtype] > + * Attribute of the element to filter > + * @param {string} [aSpec.value] Shouldn't matter. Would be good to have the same API, and being easily extendable later. @@ +74,5 @@ > + var parent = spec.parent; > + > + var elems = null; > + var root = parent ? parent.getNode() : this._controller.window.document; > + var nodeCollector = new domUtils.nodeCollector(root); Not yet, but we might have to. There are lot of ui elements in preferences, which have to be added here. So lets see. @@ +92,5 @@ > + > + return elems; > + }, > + > + openPreferences : function PreferencesPage_openPreferences(aPane) { This method cannot be part of the class. At the time when you want to call this, the class has not been instantiated yet. So it should be a module global. @@ +95,5 @@ > + > + openPreferences : function PreferencesPage_openPreferences(aPane) { > + this._controller.open("about:preferences"); > + this._controller.waitForPageLoad(); > + this.switchPane(aPane); No, please do not include the category switching here at all. There is no UI, which let you do this as user. It's clearly a separate method. @@ +98,5 @@ > + this._controller.waitForPageLoad(); > + this.switchPane(aPane); > + }, > + > + switchPane : function PreferencesPage_switchPane(aPane) { This needs to become a getter and setter. Also better to rename it to `category`. @@ +100,5 @@ > + }, > + > + switchPane : function PreferencesPage_switchPane(aPane) { > + assert.equal(String(this._controller.tabs.activeTabWindow.document.location), > + "about:preferences", "Preferences page is highlighted"); I don't understand this assert line. Why do you need this? ::: firefox/tests/functional/testPreferences/testDefaultPhishingEnabled.js @@ +8,2 @@ > var prefs = require("../../../lib/prefs"); > +var prefsPageUI = require("../../../lib/ui/preferences"); When you import, name the variable like the imported module. @@ +10,4 @@ > > var setupModule = function(aModule) { > aModule.controller = mozmill.getBrowserController(); > + aModule.prefsPage = new prefsPageUI.PreferencesPage(aModule.controller); I don't see why this has to be part of setupModule. Lets instantiate it when it is necessary, and best let the instance be returned by the opening method. ::: firefox/tests/functional/testPreferences/testRestoreHomepageToDefault.js @@ +62,2 @@ > */ > +function prefSetCurrentHomePage() { Please leave the Callback suffix, it makes it easier to recognize. @@ +62,3 @@ > */ > +function prefSetCurrentHomePage() { > + tabBrowser.openTab(); Why do you open and close a new tab for the preferences? This gets done automatically when you open them. ::: firefox/tests/functional/testPreferences/testSetToCurrentPage.js @@ -12,5 @@ > > const BASE_URL = collector.addHttpResource("../../../../data/"); > const TEST_DATA = BASE_URL + "layout/mozilla.html"; > > -const BROWSER_HOMEPAGE = "browser.startup.homepage"; Why are you removing that line?

Attachment #8433361 - Flags: feedback?(hskupin) → feedback-

Daniel Gherasim

Reporter

Comment 29

•

11 years ago

Attached patch v1.patch (obsolete) — Details — Splinter Review

Thanks for your feedback, Updated patch based on them. * Had to modify a bit modal-dialog/findWindow to wait for a specific title window (it's needed in case 2 modals are triggered do open simultaneously). * I also added the browser.js library from where we can open the preferences. * Updated tabs.js/tabBrowser/openTab() so we can give a callback that opens a tab.

Attachment #8433361 - Attachment is obsolete: true

Attachment #8433361 - Flags: feedback?(andreea.matei)

Attachment #8438463 - Flags: review?(andrei.eftimie)

Attachment #8438463 - Flags: review?(andreea.matei)

Daniel Gherasim

Reporter

Comment 30

•

11 years ago

> > + assert.equal(String(this._controller.tabs.activeTabWindow.document.location), > > + "about:preferences", "Preferences page is highlighted"); > > I don't understand this assert line. Why do you need this? Just to make sure that the preferences tab is selected & highlighted before switching thorugh categories. > ::: firefox/tests/functional/testPreferences/testRestoreHomepageToDefault.js > > +function prefSetCurrentHomePage() { > > Please leave the Callback suffix, it makes it easier to recognize. I don't see why, it's not a callback function called elsewhere anymore. It's just used to structure code better.

Andrei Eftimie

Comment 31

•

11 years ago

Comment on attachment 8438463 [details] [diff] [review] v1.patch Review of attachment 8438463 [details] [diff] [review]: ----------------------------------------------------------------- This is a big patch. Nice work. We should also update the restart tests to only use 1 testfile and use `restartApplication("nextTestModule")` when we need a restart. ::: firefox/lib/modal-dialog.js @@ +25,5 @@ > this._opener = aOpener; > this._callback = aCallback; > this._timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > + if (aSpec) > + this._spec = aSpec; Use this._spec = aSpec || {} instead of the if clause. You won't need to check for its existence later in code. ::: firefox/lib/tabs.js @@ +837,5 @@ > } > > + try { > + if (typeof type === "function") { > + type(); I can't say I'm a fan of this implementation. We might enhance this method to have aEventType on _how_ to open a tab, and add a new argument (or use aSpec with multiple properties) for _what kind_ of tab to open. This might also be then used for AMO (or other pages that are opened in tabs). ::: firefox/lib/ui/browser.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/. */ > + > + "use strict"; nit: space @@ +58,5 @@ > + openPreferences : function BrowserWindow_openPreferences() { > + > + var tab = this._tabBrowser.getElement({type: "tabs_tab", > + subtype: "label", > + value: "Nightly Preferences"}); This needs to be localised. ::: firefox/lib/ui/preferences.js @@ +37,5 @@ > + * > + * @returns {ElemBase} The panel category element > + */ > + get category() { > + return this._categories.filter(this.isSelected, this)[0]; Instead of iterating over all _known_ panes, and checking each one for the "selected" attribute, I would query the DOM category list for the selected attribute. A selector based on this should work: > querySelector("#categories [selected=true]") @@ +71,5 @@ > + */ > + isSelected : function PreferencesPage_isSelected(aCategory) { > + var categoryButton = this.getElement({type: "category", > + subtype: aCategory}); > + return (String(categoryButton.getNode().getAttribute("selected")) === "true"); Isn't the attribute value already a String? @@ +117,5 @@ > + var root = parent ? parent.getNode() : this._controller.window.document; > + > + switch (spec.type) { > + case "category": > + elems = [findElement.ID(root, "category-" + spec.subtype)]; Ensure that subtype exists. @@ +161,5 @@ > + }, > + > + /** > + * Set the home page from the general category > + * @param {objecct} aSpec nit: cc @@ +172,5 @@ > + setHomePage : function PreferencesPage_setHomePage(aSpec) { > + var spec = aSpec || {}; > + > + if (spec.url) { > + var urlInput = prefsPage.getElemen({type: "general_homePageInput"}); who is `prefsPage` in this context? also typo: getElement @@ +191,5 @@ > + /** > + * Select custom history from the privacy category to extend preferences > + */ > + expandCustomHistory : function PreferencesPage_expandCustonHistory() { > + if (this.category !== "privacy") Please always use curly brackets. @@ +196,5 @@ > + this.category = "privacy"; > + > + var historyMode = this.getElement({type: "privacy_historyMode"}); > + historyMode.select(undefined, undefined, "custom"); > + assert.waitFor(function () { => ::: firefox/tests/functional/restartTests/testPreferences_masterPassword/test1.js @@ +115,4 @@ > * MozMillController of the window to operate on > */ > +function confirmHandler(aController) { > + var button = findElement.Lookup(aController.window.document, We should change this into nodeCollector. ::: firefox/tests/functional/testPreferences/testDisableCookies.js @@ +81,5 @@ > "Cookie is not saved"); > > var dtds = ["chrome://browser/locale/preferences/cookies.dtd"]; > var cmdKey = utils.getEntity(dtds, "windowClose.key"); > + aController.keypress(null, cmdKey, {accelKey: true}); Should call this method on an element. ::: firefox/tests/functional/testPreferences/testPaneRetention.js @@ -1,1 @@ > -/* This Source Code Form is subject to the terms of the Mozilla Public We should make sure that this isn't a feature anymore. I see it doesn't work, but it might be a bug.

Attachment #8438463 - Flags: review?(andrei.eftimie)

Attachment #8438463 - Flags: review?(andreea.matei)

Attachment #8438463 - Flags: review-

Daniel Gherasim

Reporter

Comment 32

•

11 years ago

Attached patch v2.patch (obsolete) — Details — Splinter Review

Updated based on last review. (In reply to Andrei Eftimie from comment #31) > Comment on attachment 8438463 [details] [diff] [review] > v1.patch > > Review of attachment 8438463 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: firefox/tests/functional/testPreferences/testPaneRetention.js > @@ -1,1 @@ > > -/* This Source Code Form is subject to the terms of the Mozilla Public > > We should make sure that this isn't a feature anymore. > I see it doesn't work, but it might be a bug. I looked for a bug but didn't find any, I guess this is the default behavior. Opening to a another pane implies only adding #pane at the end of about:preferences in url.

Attachment #8419323 - Attachment is obsolete: true

Attachment #8438463 - Attachment is obsolete: true

Attachment #8445208 - Flags: review?(andrei.eftimie)

Attachment #8445208 - Flags: review?(andreea.matei)

Andrei Eftimie

Comment 33

•

11 years ago

Comment on attachment 8445208 [details] [diff] [review] v2.patch Review of attachment 8445208 [details] [diff] [review]: ----------------------------------------------------------------- We're missing the masterPassword test. ::: firefox/lib/tabs.js @@ +815,5 @@ > * <dd>The keyboard shortcut is used</dd> > * <dt>tabStrip</dt> > * <dd>A double click on the tabstrip gets synthesized</dd> > * </dl> > + * @param {object} aType="new-tab" Please mark this as optional. @@ +848,5 @@ > + var menuItem = "#menu_newNavigatorTab"; > + var cmdKey = utils.getEntity(this.getDtds(), "tabCmd.commandkey");; > + break; > + case "preferences": > + var menuItem = "#menu_preferences"; We should also add the cmdKey for the preferences pane. On OSX it is `cmdKey + ","`, there should be a dtd for this as well. ::: firefox/lib/ui/browser.js @@ +73,5 @@ > + > + var tab = this._tabBrowser.getElement({type: "tabs_tab", > + subtype: "label", > + value: preferencesTabTitle}); > + if (tab.exists()) { Not sure if this check shouldn't be part of the openTab method... for these special tabs (about:%page%) if they are already open we should not check for the animation observers. I noticed that if we have this tab open, and we try to open it again waitForPageLoad() fails like this: > controller.waitForPageLoad(URI=about:preferences#, readyState=complete) Could it be that this is the cause for the failures we have in bug 1015126? @@ +74,5 @@ > + var tab = this._tabBrowser.getElement({type: "tabs_tab", > + subtype: "label", > + value: preferencesTabTitle}); > + if (tab.exists()) { > + this._controller.mainMenu.click("#menu_preferences"); We should also handle the shortcut. @@ +77,5 @@ > + if (tab.exists()) { > + this._controller.mainMenu.click("#menu_preferences"); > + } > + else { > + this._tabBrowser.openTab("menu", "preferences"); "menu" or "shortcut" should be added as an argument to this method and passed in further. ::: firefox/lib/ui/preferences.js @@ +18,5 @@ > +function Preferences(aController) { > + this._controller = aController || mozmill.getBrowserController(); > + this._categories = ["general", "content", "application", > + "privacy", "security", > + "sync", "advanced"]; For easier future maintainability please move each of them on a separate line. @@ +38,5 @@ > + * @returns {ElemBase} The panel category element > + */ > + get category() { > + return this.getElement({type: "categories"}).getNode() > + .querySelector(".category[selected=true]"); You should make this part of getElements and the query should go through findElement. @@ +66,5 @@ > + * > + * @param {string} aCategory > + * The category to switch to > + */ > + set category(aCategory) { Move the setter right after the getter for the same entity. @@ +202,5 @@ > + /** > + * Select custom history from the privacy category to extend preferences > + */ > + expandCustomHistory : function PreferencesPage_expandCustonHistory() { > + if (this.category !== "privacy") { I don't think we need this check. Nothing will break if we're already on the "privacy" category and we click again on the menu. ::: firefox/tests/functional/testPreferences/testDisableCookies.js @@ +15,5 @@ > > const BASE_URL = collector.addHttpResource("../../../../data/"); > const TEST_DATA = BASE_URL + "cookies/cookie_single.html"; > > +const NETWORK_COOKIE_BEHAVIOR = "network.cookie.cookieBehavior"; I don't see anything done with this pref. Do we still need it? ::: firefox/tests/functional/testPreferences/testEnableCookies.js @@ +15,5 @@ > > const BASE_URL = collector.addHttpResource("../../../../data/"); > const TEST_DATA = BASE_URL + "cookies/cookie_single.html"; > > +const NETWORK_COOKIE_BEHAVIOR = "network.cookie.cookieBehavior"; Same here. We don't appear to be using this. ::: firefox/tests/functional/testPreferences/testSwitchPanes.js @@ +26,3 @@ > > // Step through each of the panes > + prefsPage.categories.forEach(function (aPane) { You can also use the function arrow syntax here.

Attachment #8445208 - Flags: review?(andrei.eftimie)

Attachment #8445208 - Flags: review?(andreea.matei)

Attachment #8445208 - Flags: review-

•

11 years ago

The in-content preferences is currently worked out to make all the dialogs in-content. I believe we should wait for bug 752197 before we continue here.

Depends on: 752197

Daniel Gherasim

Reporter

Comment 38

•

11 years ago

Attached patch v4_WIP.patch (obsolete) — Details — Splinter Review

This is a patch with all tests working, handling both modal dialogs and the new in-content ones depending which ones we handle. Once the other modal-dialogs are changed we will only have to change the tests (small changes - replacing the utils.handleWindow with only calling the callback). I would split this work into small working patches (as tests are working fine with the old preferences dialog) and start by landing small parts (modifying only the tests for which in-content dialogs are already landed on nightly). If that's ok to you all, I will continue with doing this and ask for review right after.

Attachment #8447106 - Attachment is obsolete: true

Daniel Gherasim

Reporter

Comment 39

•

11 years ago

Hmm looking more into this I think we can start by fixing this step by step. So I would file 3 new bugs as dependencies for this and fix those first: 1. Enhance modal-dialog.js to wait for a GIVEN window (by type and/or title); -- would imply an extra parameter & add a condition in findWindow(). 2. Enhance tabs.js to add new waitForNewTab method -- will offer the possibility to open a tab from a test or another library; -- will also replace duplicated code in openTab, openInNewTab & reopen methods. 3. Add BrowserWindow class -- currently with the openPreferences() method -- inherited from Window implemented in 1006996 This bug will remain about adding the preferences.js library & modifying tests. I already implemented all this stuff in the patch above, including handling the new in-content preferences dialogs, but I would like to see it reviewed & landed in different parts (I just think would be easier & faster). Henrik, is this a good way to go ?

Flags: needinfo?(hskupin)

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

Comment 40

•

11 years ago

(In reply to daniel.gherasim from comment #39) > 1. Enhance modal-dialog.js to wait for a GIVEN window (by type and/or title); > -- would imply an extra parameter & add a condition in findWindow(). Why is this necessary? If I missed a comment please let me know. Otherwise I would like to know the reason for this change. > 2. Enhance tabs.js to add new waitForNewTab method > -- will offer the possibility to open a tab from a test or another library; > -- will also replace duplicated code in openTab, openInNewTab & reopen > methods. We should not introduce even more those strange methods. What you want is openTab() having a callback. Something we might wanna do is to combine openTab and openInNewTab later. > 3. Add BrowserWindow class > -- currently with the openPreferences() method > -- inherited from Window implemented in 1006996 Sounds fine > Henrik, is this a good way to go ? I think so, yes.

Flags: needinfo?(hskupin)

Daniel Gherasim

Reporter

•

11 years ago

Depends on: 1036825

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

Comment 44

•

11 years ago

(In reply to daniel.gherasim from comment #43) > Most likely in the future at least one of those wont be a modal window > anymore as with the new changes (not sure how this special case will be > handled by developers, given that we don't want to see the 2nd dialog if the > password is not correctly typed). > But until then, how else we can handle this situation ? In those situations two modal dialog handlers have to be used, which use the opener property internally to identify the correct parent window.

Daniel Gherasim

Reporter

Comment 45

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #44) > > But until then, how else we can handle this situation ? > > In those situations two modal dialog handlers have to be used, which use the > opener property internally to identify the correct parent window. Thanks! I'll try that.

Daniel Gherasim

Reporter

Updated

•

11 years ago

Depends on: 1047235

Daniel Gherasim

Reporter

Comment 46

•

11 years ago

Attached patch v5.patch (obsolete) — Details — Splinter Review

Summary about the patch: << All the dialogs are now in-content so I made all the changes needed >> 1. Moved & refactor to persist in 1 single file & handling the in-content dialogs: > functional/restartTests/testPreferences/(test1.js|test2.js|test3.js) into > functional/testPreferences/testMasterPassword.js 2. Refactored all preferences tests to work with the new in-content preferences page & dialogs from > functional/testPreferences/ & > remote/testPreferences/ 3. Added 2 new classes in 1 file: > firefox/lib/ui/preferences.js 3.1 class Preferences() *** controller -- the controller to handle the page *** category -- current category (pane) selected *** categories -- list of avaible categories *** dtds() *** subDialog() -- saves a intance of the current opened in-content dialog *** isSelected() -- test if a category is selected *** getElement() *** getElements() *** expandCustomHistory() -- goes to privacy & selects the custom history from the dropdown *** setHomePage(url) -- goes to general category & sets the home page url as requested *** openSubDialog(type) -- triggers opening a subdialog with the requested type & waits for the correct events 3.2 class SubDialog() *** controller -- controller to handle the dialog *** root -- the content part of the dialog included in a <browser>, <window> or <prefwindow> element (excepting the dialotTitle/closeDialog header part & the footer part containing the buttons -- accept||ok||cancel||help etc.) *** close(method) -- closes the current dialog *** getElement() *** getElements() 4. Also the method openPreferences was added with the BrowserWindow class, once bug 106996 & after that bug 1047235 gets fixed, we will only have to add the method, and not the whole class. Functional & Remote reports: http://mozmill-crowd.blargon7.com/#/functional/report/2f56cb3a3728c2c47cd1b44f77d4358e http://mozmill-crowd.blargon7.com/#/remote/report/2f56cb3a3728c2c47cd1b44f77d68ad1

Attachment #8452315 - Attachment is obsolete: true

Attachment #8480551 - Flags: review?(andrei.eftimie)

Attachment #8480551 - Flags: review?(andreea.matei)

Daniel Gherasim

Reporter

•

11 years ago

Comment on attachment 8480551 [details] [diff] [review] v5.patch Review of attachment 8480551 [details] [diff] [review]: ----------------------------------------------------------------- I really like the new changes, the tests look so much better and easier to understand. ::: firefox/lib/ui/browser.js @@ +9,5 @@ > +// Include required modules > +var { assert } = require("../../../lib/assertions"); > +var preferences = require("preferences"); > +var utils = require("../../../lib/utils"); > +var tabs = require("../tabs"); Please switch utils and tabs :) @@ +60,5 @@ > + * Information for opening the preferences > + * @params {object} [aSpec.type="menu"] > + * Type of event which triggers the action > + * > + * @returns {Preferences} Instance of an Preferences object Well, type here is still object then. @@ +80,5 @@ > + case "shortcut": > + if (mozmill.isMac) { > + var cmdKey = utils.getEntity(self.dtds, > + "preferencesCmdMac.commandkey"); > + self._controller.keypress(null, cmdKey, {}); We should use cmdKey.keypress() instead. @@ +111,5 @@ > + if (tab.exists()) { > + callbackOpen(); > + } > + else { > + this._tabBrowser.openTab({method: "callback", callback: () => { I would put the second argument "callback" under "method" for better readability. ::: firefox/lib/ui/preferences.js @@ +41,5 @@ > + > + /** > + * Retrieve the currently selected category > + * > + * @returns {ElemBase} The panel category element Please make sure to add all types with lowecase. We have a contributor that's refactoring all jsdoc and we don't want to add any more files like this. @@ +62,5 @@ > + > + /** > + * Retrieve the list of avaible categories > + * > + * @returns {array[]} The avaible categories elements nit: available, here and above. @@ +81,5 @@ > + > + /** > + * Get the current subdialog > + * > + * @returns {SubDialog} Dialog opened or that was last opened "Dialog opened or the last opened one" sounds clearer? @@ +149,5 @@ > + case "category_selected": > + var categoriesNode = this.getElement({type: "categories"}).getNode(); > + elems = [findElement.Selector(this._controller.tabs.activeTab, > + ".category[selected=true]")] > + break; Not sure where you're using categoriesNode in here. @@ +158,5 @@ > + case "content_chooseLanguage": > + elems = [findElement.ID(root, "chooseLanguage")]; > + break; > + case "frame": > + elems = [findElement.ID(root, "dialogFrame")] Semicolon missing. @@ +253,5 @@ > + assert.ok(aType, "Type of the subdialog has been specified"); > + > + var dialogFrame = this.getElement({type: "frame"}).getNode(); > + > + var dialogLoaded = false; Please remove the blank line @@ +261,5 @@ > + > + try { > + aCallback(); > + > + assert.waitFor(() => (dialogLoaded === true), You could return dialogLoaded here. @@ +279,5 @@ > +} > + > +/** > + * In-Content preferences SubDialog > + * Imlemented in bug 1021618 nit: Implemented @@ +333,5 @@ > + switch (method) { > + case "shortcut" : > + this._controller.keypress(null, "VK_ESCAPE", {}); > + break; > + case "callback": Please sort alphabetically. @@ +343,5 @@ > + default: > + assert.fail("Unknown method - " + spec.method); > + } > + > + assert.waitFor(() => (dialogClosed === true), Also here you can return dialogClosed. @@ +419,5 @@ > + case "frame": > + elems = [findElement.ID(root, "dialogFrame")] > + break; > + case "menuItems": > + elems = nodeCollector.queryNodes("menuitem") Semicolon missing for these lines. @@ +424,5 @@ > + break; > + case "overlay": > + elems = [findElement.ID(root, "dialogOverlay")]; > + break; > + case "dialogElement": This should be above in the list, before dlg. ::: firefox/tests/functional/testPreferences/testDisableCookies.js @@ +65,2 @@ > > + // Check that no cookies was added in the list nit: cookies were added @@ +75,5 @@ > + // Close the dialog > + cookiesDialog.close("callback", () => { > + var dtds = ["chrome://browser/locale/preferences/cookies.dtd"]; > + var cmdKey = utils.getEntity(dtds, "windowClose.key"); > + controller.keypress(null, cmdKey, {accelKey: true}); cmkdKey.keypress() ::: firefox/tests/functional/testPreferences/testEnableCookies.js @@ +75,5 @@ > + // Close the dialog > + cookiesDialog.close("callback", () => { > + var dtds = ["chrome://browser/locale/preferences/cookies.dtd"]; > + var cmdKey = utils.getEntity(dtds, "windowClose.key"); > + controller.keypress(null, cmdKey, {accelKey: true}); Same here. ::: firefox/tests/functional/testPreferences/testMasterPassword.js @@ +8,5 @@ > +var domUtils = require("../../../../lib/dom-utils"); > +var modalDialog = require("../../../../lib/modal-dialog"); > +var browser = require("../../../lib/ui/browser"); > +var toolbars = require("../../../lib/toolbars"); > +var utils = require("../../../../lib/utils"); These need sorting. @@ +129,5 @@ > + passwordsSubDialog.close("callback", () => { > + // Close the password manager > + var dtds = ["chrome://passwordmgr/locale/passwordManager.dtd"]; > + var cmdKey = utils.getEntity(dtds, "windowClose.key"); > + controller.keypress(null, cmdKey, {accelKey: true}); cmdKey.keypress() ::: firefox/tests/functional/testPreferences/testPasswordNotSaved.js @@ +59,5 @@ > + > + passwordsSubDialog.close("callback", () => { > + var dtds = ["chrome://passwordmgr/locale/passwordManager.dtd"]; > + var cmdKey = utils.getEntity(dtds, "windowClose.key"); > + controller.keypress(null, cmdKey, {accelKey: true}); Same here. ::: firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js @@ +122,5 @@ > + aDialog.close("callback", () => { > + // Close the password manager > + var dtds = ["chrome://passwordmgr/locale/passwordManager.dtd"]; > + var cmdKey = utils.getEntity(dtds, "windowClose.key"); > + controller.keypress(null, cmdKey, {accelKey: true}); Here too. ::: firefox/tests/functional/testPreferences/testRemoveCookie.js @@ +82,5 @@ > > + aCookiesDialog.close("callback", () => { > + var dtds = ["chrome://browser/locale/preferences/cookies.dtd"]; > + var cmdKey = utils.getEntity(dtds, "windowClose.key"); > + controller.keypress(null, cmdKey, {accelKey: true}); And here ::: firefox/tests/remote/testPreferences/testRemoveAllCookies.js @@ +74,5 @@ > > + aDialog.close("callback", () => { > + var dtds = ["chrome://browser/locale/preferences/cookies.dtd"]; > + var cmdKey = utils.getEntity(dtds, "windowClose.key"); > + controller.keypress(null, cmdKey, {accelKey: true}); Same here.

Attachment #8480551 - Flags: review?(andrei.eftimie)

Attachment #8480551 - Flags: review?(andreea.matei)

Attachment #8480551 - Flags: review-

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

•

11 years ago

Attached patch library_v1.patch (obsolete) — Details — Splinter Review

As we agreed on. We will first create the lib with it's test so we can get it landed and have it for the TPS tests.

Attachment #8480551 - Attachment is obsolete: true

Attachment #8494403 - Flags: review?(andrei.eftimie)

Attachment #8494403 - Flags: review?(andreea.matei)

Daniel Gherasim

Reporter

Comment 52

•

11 years ago

The in-content is by default only on nighlty so there is where we need this patch.

status-firefox32: affected → ---

status-firefox35: --- → affected

Andrei Eftimie

Comment 53

•

11 years ago

Comment on attachment 8494403 [details] [diff] [review] library_v1.patch Review of attachment 8494403 [details] [diff] [review]: ----------------------------------------------------------------- Big patch. ::: firefox/lib/tests/testPreferences.js @@ +9,5 @@ > +var prefs = require("../ui/prefs"); > + > +const PREF_ELEMENTS = [ > + { > + category: "layout", So what does "layout" test here? It's not one of the Categories... @@ +12,5 @@ > + { > + category: "layout", > + elements: [ > + {name: "categories", value: "richlistbox"}, > + //{name: "category", value: "panel"}, And why is this commented out? I see if fails with this left in... should be removed in this case? @@ +30,5 @@ > + elements:[ > + {name: "content_chooseLanguage", value: "button"} > + ] > + }, > + { I miss the "application" panel @@ +60,5 @@ > + {name: "sync_termsOfService", value: "label"}, > + {name: "sync_privacyPolicy", value: "label"}, > + {name: "sync_weavePrefsDeck", value: "deck"}, > + ] > + } And the "advanced" panel @@ +106,5 @@ > +function testPreferences() { > + // Test preferences page elements > + var prefsPage = browserWindow.openPreferences(); > + > + PREF_ELEMENTS.forEach(aCategory => { Please add a basic comment on what gets teste here. ::: firefox/lib/toolbars.js @@ +837,5 @@ > } > } > > /** > + * Menu Panel class So this is pretty limited, and only contains the things we need for the TPS sync tests. I'm fine with this. ::: firefox/lib/ui/browser.js @@ +109,5 @@ > + * Method to use when trigger the action ("menu" or "shortcut") > + * > + * @returns {Preferences} Instance of an Preferences object > + */ > +BrowserWindow.prototype.openPreferences = function BrowserWindow_openPref(aType="menu") { I wonder if this should be here, or the relationship should be reversed. Instead of opening the "about:preferences" with `var prefsPage = browserWindow.openPreferences();` if it would be better to do: > var prefsPage = new PreferencesPage(); > prefPage.open(); @@ +118,5 @@ > + break; > + case "shortcut": > + if (mozmill.isMac) { > + var cmdKey = utils.getEntity(this.dtds, > + "preferencesCmdMac.commandkey"); Something wrong here. The pref is included in baseMenuOverlay.dtd which is not loaded. @@ +119,5 @@ > + case "shortcut": > + if (mozmill.isMac) { > + var cmdKey = utils.getEntity(this.dtds, > + "preferencesCmdMac.commandkey"); > + this._controller.keypress(null, cmdKey, {}); And would need accelKey: true here. ::: firefox/lib/ui/prefs.js @@ +6,5 @@ > + > +Cu.import("resource://gre/modules/Services.jsm"); > + > +// Include required modules > +var { assert } = require("../../../lib/assertions"); We can remove this now. @@ +58,5 @@ > + * The category to switch to > + */ > + set category(aCategory) { > + assert.notEqual(this._pages.indexOf(aCategory), -1, > + "Wrong category has been specified"); "Specified category exists" @@ +64,5 @@ > + var categoryButton = this.getElement({type: "category", > + subtype: aCategory}); > + categoryButton.click(); > + var header = this.getElement({type: "header", subtype: aCategory}); > + assert.waitFor(() => utils.isDisplayed(this._controller, header), I see why you wait for the header, the "categories" are not grouped as "pages". All elements from all panels are dropped in the same container, and are shown based on `data-category`. Maybe we can improve this as wait for all elements of `data-category` === `pane%category%` to be displayed. If we do this we might be able to get rid of the below sleep call. *This is really ugly how it was implemented. I'm not sure why every "page" was not wrapped in a container and only the container to be hidden/shown... @@ +68,5 @@ > + assert.waitFor(() => utils.isDisplayed(this._controller, header), > + "Category has been loaded - " + aCategory); > + > + // Itermitent failures are encounterred if we access realy fast the tabbed elements > + // We don't have any attributes/events to wait for. File a bug mentioning this issue, and mark it here so we have a reference. EDIT. if the above works, we might be able to remove this. @@ +75,5 @@ > + > + /** > + * Retrieve the list of avaible categories > + * > + * @returns {array[]} The avaible categories elements type[] // in this case: string[] @@ +78,5 @@ > + * > + * @returns {array[]} The avaible categories elements > + */ > + get categories() { > + return this._categories; Should this be `this._pages` ? @@ +105,5 @@ > + * Get the title of the preferences tab > + * > + * @returns {string} Title of the preferences tab > + */ > + get title() { Please add a test for this method as well. I'm not this works as expected. On Nightly/OSX the title for me is "Preferences" (tab/window title) not "Nightly Preferences". Where to we use this `title`? @@ +120,5 @@ > + * Check if a category is selected > + * > + * @returns {boolean} True if the category is selected, false otherwise > + */ > + isSelected : function PreferencesPage_isSelected(aCategory) { Maybe we should have a single method that checks whether a category is loaded / selected. And that can be used when setting the category as well. That method should check 1) the menu item is properly selected - what is done here, and 2) all relevant content elements are visible (as mentioned in the category() setter, and the setter would just call this method). Debatable whether we should call it "isSelected" or "isLoaded". I'm more inclined to go with the latter. @@ +176,5 @@ > + break; > + case "category_selected": > + var categoriesNode = this.getElement({type: "categories"}).getNode(); > + elems = [findElement.Selector(this._controller.tabs.activeTab, > + ".category[selected=true]")] I see both "selected" and "current" used. Not sure about the difference. Maybe we should wait/check for both. @@ +178,5 @@ > + var categoriesNode = this.getElement({type: "categories"}).getNode(); > + elems = [findElement.Selector(this._controller.tabs.activeTab, > + ".category[selected=true]")] > + break; > + case "contentItem": Since all other items use an underscore instead of being camelCased, should use that here as well. This is kind of a broad item, and I don't see it used at all in this patch. Do we need it to accept any id? Should we then call it "content_id"? I'm not sure about its use, being that we have methods for all wanted items below... @@ +197,5 @@ > + break; > + case "general_useCurrent": > + elems = [findElement.ID(root, "useCurrent")]; > + break; > + case "header": We might not need this if we instead query elements based on `data-category`. @@ +268,5 @@ > + > + var historyMode = this.getElement({type: "privacy_historyMode"}); > + historyMode.select(undefined, undefined, "custom"); > + assert.waitFor(() => { > + return historyMode.getNode().value === "custom"; Please make sure this works if "custom" is already selected. (We've had issues prior with this call in this case) @@ +279,5 @@ > + * @param {object} aSpec > + * Information about setting the home page > + * @param {string} [aSpec.url] > + * Set home page to this url > + * @param {boolean} [aSpec.default] spec.url and spec.default are mutually exclusive, right? If they are both set, currently we'll use the `url` and return early, ignoring the `default` option. There's also a "Use Bookmark" button, which we currently ignore. To not get into weird situations, we should use the same field to denote the type. aspec = { type: url|bookmark|default, // required, maybe defaults to "default" subtype: "" // required for url and bookmark } @@ +326,5 @@ > + > + try { > + aCallback(); > + > + assert.waitFor(() => (dialogLoaded === true), Can be simplified to: > () => dialogLoaded @@ +349,5 @@ > + * Method to use when opening ("signIn" or "signUp") > + * > + * @returns {Preferences} Instance of an Preferences object > + */ > + openAboutAccountsPage : function PreferencesPage_openAboutAccountsPage(aMethod="signIn") { This method should be part of the AboutAccountsPage class. You instantiate AboutAccountsPage, then call the open method (which will instantiate the pref class, open the right pane, click on the requested links). There should also be a check that we are not already signed in, otherwise it will fail. @@ +580,5 @@ > + * Parent of the to find element > + * > + * @returns {ElemBase[]} Elements which have been found > + */ > + getElements : function PreferencesPage_getElements(aSpec) { We have a few elements which are _always_ available. The shell of the "dialog" is there. overlay, title, close_button. The contents of the dialog changes based on context. All that is loaded as a different document in `frame`. I wonder if we should make this separation more clearly. All preferences dialog instances will have the same base stuff. But the inside is different (check Privacy>Custom>Cookies vs General>Use Bookmark With this in mind I feel this class should only have the base stuff, and we extend it for each dialog for the content elements we need. @@ +616,5 @@ > + break; > + case "menuItems": > + elems = nodeCollector.queryNodes("menuitem") > + break; > + case "overlay": We can't interact with the overlay at all. All these appear to be modal dialogs. This would be useful if some are _not_ modal and we can close them by clicking on the overlay... If all are modal, we should remove this item as we can't use it for anything.

Attachment #8494403 - Flags: review?(andrei.eftimie)

Attachment #8494403 - Flags: review?(andreea.matei)

Attachment #8494403 - Flags: review-

Daniel Gherasim

Reporter

Comment 54

•

11 years ago

(In reply to Andrei Eftimie from comment #53) > ::: firefox/lib/tests/testPreferences.js > @@ +9,5 @@ > > +var prefs = require("../ui/prefs"); > > + > > +const PREF_ELEMENTS = [ > > + { > > + category: "layout", > > So what does "layout" test here? > It's not one of the Categories... Those are elements from the basic layout, present on all categories. > @@ +30,5 @@ > > + elements:[ > > + {name: "content_chooseLanguage", value: "button"} > > + ] > > + }, > > + { > > I miss the "application" panel We currently don't make use of that in our tests. I wouldn't add all elements from all panels now, but add them in the future when we find out that we need them. > ::: firefox/lib/ui/browser.js > @@ +109,5 @@ > > + * Method to use when trigger the action ("menu" or "shortcut") > > + * > > + * @returns {Preferences} Instance of an Preferences object > > + */ > > +BrowserWindow.prototype.openPreferences = function BrowserWindow_openPref(aType="menu") { > > I wonder if this should be here, or the relationship should be reversed. > > Instead of opening the "about:preferences" with `var prefsPage = > browserWindow.openPreferences();` if it would be better to do: > > var prefsPage = new PreferencesPage(); > > prefPage.open(); That's actually the reason we were blocked on this by the BrowserWindow class. That was Henrik's request in Comment 28 (both about the place of the method and that it should return an instance of the Preferences class). Also the structure was accepted by Henrik in Comment 40. > @@ +580,5 @@ > > + * Parent of the to find element > > + * > > + * @returns {ElemBase[]} Elements which have been found > > + */ > > + getElements : function PreferencesPage_getElements(aSpec) { > > We have a few elements which are _always_ available. The shell of the > "dialog" is there. overlay, title, close_button. > The contents of the dialog changes based on context. All that is loaded as a > different document in `frame`. > > I wonder if we should make this separation more clearly. > All preferences dialog instances will have the same base stuff. > But the inside is different (check Privacy>Custom>Cookies vs General>Use > Bookmark > > With this in mind I feel this class should only have the base stuff, and we > extend it for each dialog for the content elements we need. > All elements present are basic elements present on all subDialogs. We also have the posibility to get specific elements ("dialogElement" with subtype which is the id of the element wanted). I wouldn't make for the moment classes for each subDialog, we will end up with a long list of classes. I see it as the best solution and code structure, but I would do this in a follow-up bug, or along with the patch for the tests (where based on the elements we want, we can add the needed class with elements).

Daniel Gherasim

Reporter

Updated

•

11 years ago

Depends on: 1073483

Daniel Gherasim

Reporter

Comment 55

•

11 years ago

Attached patch library_v2.patch (obsolete) — Details — Splinter Review

Updated based on the last review and discussions we had on IRC.

Attachment #8494403 - Attachment is obsolete: true

Attachment #8497358 - Flags: review?(andrei.eftimie)

Attachment #8497358 - Flags: review?(andreea.matei)

Szabolcs Hubai (:xabolcs)

Updated

•

11 years ago

Blocks: 856018

Andrei Eftimie

Comment 56

•

11 years ago

Comment on attachment 8497358 [details] [diff] [review] library_v2.patch Review of attachment 8497358 [details] [diff] [review]: ----------------------------------------------------------------- Still have some things to fix. I'm still on the fence about certain things, and will have another closer look at them in the next patch version. ::: firefox/lib/tabs.js @@ +91,5 @@ > for (var i = 0; i < browsers.length; i++) { > var browser = browsers[i]; > + var test = (aIgnoreFragment) ? browser.currentURI.equalsExceptRef(uri) > + : browser.currentURI.equals(uri); > + if (test) { Can we change just the method we're calling? Something like: > var checkMethod = (aIgnoreFragment) ? "equalsExceptRef" : > : "equals"; > if (browser.currentURI[checkMethod](uri)) { ::: firefox/lib/toolbars.js @@ +900,5 @@ > + */ > + getElements : function MenuPanel_getElements(aSpec) { > + var spec = aSpec || { }; > + var type = spec.type; > + var parent = spec.parent; Please remove these local variables and use `spec.type` and `spec.parent` directly in this method. @@ +916,5 @@ > + return [findElement.ID(root, "PanelUI-popup")]; > + > + default: > + assert.fail("Unknown element type - " + spec.type); > + return null; Not sure this is a good idea. If we reach this part we will fail as per the assert above. But if the original call was made through getElement() we will get: > TypeError: null has no properties We should to return an empty array. @@ +923,5 @@ > + > + /** > + * Retrieve the email address displayed in the Menu panel as the account login email > + * > + * @returns {string] email address nit: } ::: firefox/lib/ui/browser.js @@ +107,5 @@ > + * > + * @returns {Preferences} Instance of an Preferences object > + */ > +BrowserWindow.prototype.openPreferences = function BrowserWindow_openPreferences(aMethod="menu", aCallback) { > + return new prefs.openPreferences(() => { This logic should be in the prefs.js and just pass all arguments forward. This is just a shortcut. @@ +145,5 @@ > + * > + * @returns {AboutAccounts Instance of an AboutAccountsPage object > + */ > +BrowserWindow.prototype.openAboutAccounts = function BrowserWindow_openAboutAccounts(aMethod="menu", aCallback) { > + return new prefs.openAboutAccounts(() => { Same here. ::: firefox/lib/ui/prefs.js @@ +78,5 @@ > + * @param {string} aCategory > + * The category to switch to > + */ > + set category(aCategory) { > + assert.ok(!!CATEGORIES[aCategory], "Specified category exists"); `undefined` is still falsey, no need to cast as boolean. @@ +117,5 @@ > + * Check if a category is selected/loaded > + * > + * @returns {boolean} True if the category is loaded, false otherwise > + */ > + isLoaded : function PreferencesPage_isSelected(aCategory) { This method is a bit hard to follow. Please make 2 separate checks and return early 1) check id, if not loaded yet, return false 2) check all categoryElements, if any one of them is not loaded, return false 3) return true at the end of the method @@ +166,5 @@ > + * @returns {ElemBase[]} Elements which have been found > + */ > + getElements : function PreferencesPage_getElements(aSpec) { > + var spec = aSpec || { }; > + var parent = spec.parent; no need for this, use `spec.parent` directly @@ +168,5 @@ > + getElements : function PreferencesPage_getElements(aSpec) { > + var spec = aSpec || { }; > + var parent = spec.parent; > + > + var elems = null; Should be an empty array. @@ +181,5 @@ > + assert.ok(spec.subtype, "Category has been specified"); > + elems = [findElement.ID(root, "category-" + spec.subtype)]; > + break; > + case "category_selected": > + var categoriesNode = this.getElement({type: "categories"}).getNode(); this is not used @@ +188,5 @@ > + break; > + case "category_elements": > + assert.ok(spec.subtype, "Category has been specified"); > + var selector = "[data-category='" + CATEGORIES[spec.subtype]["data-category"] + "']"; > + nodeCollector.queryNodes(selector); We don't need nodeCollector here. You can use directly: > elems = [findElement.Selector(this._controller.tabs.activeTab, > "[data-category='" + CATEGORIES[spec.subtype]["data-category"] + "']")]; @@ +243,5 @@ > + case "sync_email": > + elems = [findElement.ID(root, "fxaEmailAddress1")]; > + break; > + case "sync_preferences": > + nodeCollector.root = findElement.ID(root, "fxaSyncEngines").getNode(); Same as above, no need for NodeCollector: > elems = [findElement.Selector(root, "#fxaSyncEngines checkbox")]; @@ +266,5 @@ > + > + /** > + * Select custom history from the privacy category to extend preferences > + */ > + expandCustomHistory : function PreferencesPage_expandCustonHistory() { Please make this method more general. We could call it `selectHistory(aType="remember"|"never_remember"|"custom", aRestart=true|false)` We'll need the 2nd argument to dictate the outcome of the dialog that pops up when changing to/from never remember. This is a bit tricky. When true Firefox will restart (when prompted). When false the value of the history dropdown will remain at the previous value (when prompted)! @@ +272,5 @@ > + > + var historyMode = this.getElement({type: "privacy_historyMode"}); > + historyMode.select(undefined, undefined, "custom"); > + assert.waitFor(() => { > + return historyMode.getNode().value === "custom"; nit: we can remove the curly braces and the return statement @@ +292,5 @@ > + setHomePage : function PreferencesPage_setHomePage(aSpec) { > + var spec = aSpec || {}; > + this.category = "general"; > + > + var method = (spec.method) ? spec.method : "default"; > var method = spec.method || "default"; @@ +299,5 @@ > + case "current": > + var useCurrent = this.getElement({type: "general_useCurrent"}); > + useCurrent.click(); > + break; > + case "bookmark": Nice, you also implemented this! @@ +302,5 @@ > + break; > + case "bookmark": > + assert.equal(typeof spec.type, "number", > + "Bookmark row index to select has been defined"); > + assert.equal(typeof spec.type, "number", One of these checks is wrong, they both check for `type`. @@ +312,5 @@ > + }, "selectBookmarkDialog"); > + > + var tree = bookmarksDialog.getElement({type: "dialogElement", > + subtype: "bookmarks"}); > + widgets.clickTreeCell(this._controller, tree, spec.type, spec.subtype, {}); How is this supposed to work. If we support an index-based approach, we should only care for the rowIndex in this method, I'm not sure what the colIndex is doing for a vertical tree. So we should only expect spec.type, and always pass in the same (does 0 work?) colIndex. @@ +504,5 @@ > +} > + > +/** > + * In-Content preferences SubDialog > + * Imlemented in bug 1021618 typo: Implemented @@ +567,5 @@ > + default: > + assert.fail("Unknown method - " + aMethod); > + } > + > + assert.waitFor(() => (dialogClosed === true), `() => dialogClose` should suffice (it can only be true or false) @@ +611,5 @@ > + * @returns {ElemBase[]} Elements which have been found > + */ > + getElements : function PreferencesPage_getElements(aSpec) { > + var spec = aSpec || { }; > + var parent = spec.parent; Please use spec.parent directly @@ +621,5 @@ > + switch (spec.type) { > + case "closeButton": > + elems = [findElement.ID(root, "dialogClose")]; > + break; > + case "dlg": Can we rename this in something more human-readable? Like "button" (only if they all are buttons) @@ +637,5 @@ > + assert.ok(spec.subtype, "Dialog Element ID has been specified"); > + > + var browserFrame = this.getElement({type: "frame"}); > + nodeCollector.root = browserFrame.getNode().contentDocument.defaultView; > + nodeCollector.queryNodes("#" + spec.subtype); So we can only get elements by ID from inside the dialog window, right? If these are enough right now, then it's fine by me. We'll have to amend this when we'll need anything else. @@ +663,5 @@ > + * > + * @returns {PreferencesPage} Instance of the Preferences Page class > + */ > + > +function openPreferences(aCallback, aController) { This should be part of the `PreferencesPage` class. In tests we'll call it like: > var prefPage = new prefs.PreferencesPage(controller); > prefPage.open({method: "menu"}); Or with the shortcut from the window class: > var browserWindow = new browser.browserWindow(); > var prefPage = browserWindow.openPreferences(); @@ +689,5 @@ > + * Controller for the About Accounts page > + * > + * @returns {AboutAccountsPage} Instance of an AboutAccountsPage object > + */ > +function openAboutAccounts(aCallback, aController) { Same here. @@ +715,5 @@ > +exports.PreferencesPage = PreferencesPage; > +exports.AboutAccountsPage = AboutAccountsPage; > + > +// Export of methods > +exports.openPreferences = openPreferences; And we don't expose these.

Attachment #8497358 - Flags: review?(andrei.eftimie)

•

11 years ago

Attached patch library_v3.patch (obsolete) — Details — Splinter Review

Thanks Andrei and Henrik for all the feedback, ideas and discussions we had about this implementation. === So I updated the library patch with all we need ATM. Hope to get this landed soon.

Attachment #8497358 - Attachment is obsolete: true

Attachment #8499464 - Flags: review?(andrei.eftimie)

Attachment #8499464 - Flags: review?(andreea.matei)

Daniel Gherasim

Reporter

Updated

•

11 years ago

Blocks: 1077356

Daniel Gherasim

Reporter

Updated

•

11 years ago

No longer blocks: 856018

Daniel Gherasim

Reporter

Updated

•

11 years ago

Blocks: 856018

Daniel Gherasim

Reporter

Comment 65

•

11 years ago

Attached patch library_v3.1.patch (obsolete) — Details — Splinter Review

Let's create the selectHistory method in a follow-up bug (bug 1077356). We shouldn't be blocked on TPS tests by this (and I would really want to have the method complete, with all the options/restarts handled).

Attachment #8499464 - Attachment is obsolete: true

Attachment #8499464 - Flags: review?(andrei.eftimie)

Attachment #8499464 - Flags: review?(andreea.matei)

Attachment #8499483 - Flags: review?(andrei.eftimie)

Attachment #8499483 - Flags: review?(andreea.matei)

Andrei Eftimie

Comment 66

•

11 years ago

Comment on attachment 8499483 [details] [diff] [review] library_v3.1.patch Review of attachment 8499483 [details] [diff] [review]: ----------------------------------------------------------------- This looks good overall. No big/architecture changes from me. I'd like some better names as mentioned inline, and some nits to fix. ::: firefox/lib/tests/testPreferences.js @@ +8,5 @@ > +var browser = require("../ui/browser"); > +var prefs = require("../ui/prefs"); > +var tabs = require("../tabs"); > + > +const PREF_ELEMENTS = [ Since PREF_ is usually reserved for actual preferences, please use TEST_DATA @@ +72,5 @@ > +]; > + > +const ABOUT_ACCOUNTS_ELEMENTS = [ > + { > + category: "signIn", Please name these `method`, it gets confusing to refer them as `category` during the test. ::: firefox/lib/ui/prefs.js @@ +174,5 @@ > + var elems = []; > + var root = spec.parent ? spec.parent.getNode() : this._controller.tabs.activeTab; > + > + switch (spec.type) { > + case "categories": I'd like to refine the names a bit, so it is clear when we refer to menu items vs page items. Some suggestions on how to rename them. This one: "category_menu_wrapper" @@ +177,5 @@ > + switch (spec.type) { > + case "categories": > + elems = [findElement.ID(root, "categories")]; > + break; > + case "category": Maybe: "category_menu_item" @@ +181,5 @@ > + case "category": > + assert.ok(spec.subtype, "Category has been specified"); > + elems = [findElement.ID(root, "category-" + spec.subtype)]; > + break; > + case "category_selected": Maybe: "category_menu_selected" @@ +185,5 @@ > + case "category_selected": > + elems = [findElement.Selector(this._controller.tabs.activeTab, > + ".category[selected=true][current=true]")] > + break; > + case "category_elements": Maybe: "category_page_elements" @@ +195,5 @@ > + break; > + case "content_chooseLanguage": > + elems = [findElement.ID(root, "chooseLanguage")]; > + break; > + case "frame": This is the subDialog frame, I'd like to also have this prefixed: "subDialog_frame" Edit. actually, do we need this? We have it on the subDialog class. @@ +347,5 @@ > + * @params {function} [aSpec.callback] > + * Function that triggers the opening > + * @params {string} [aSpec.pane="general"] > + * Pane to focus after opening the preferences page > + * @params {MozMillController} [aSpec.controller] nit: sort alphabetically, also mention the default value @@ +378,5 @@ > + this._controller.keypress(null, cmdKey, {accelKey: true}); > + } > + else { > + // Shortcuts only avaible on Mac OSX for the moment > + assert.fail("Can't open preferences using shortcut on your platform"); Change this to something along the lines: "Opening the Preferences via shortcut is only available on OSX" @@ +395,5 @@ > + } > + > + var tabBrowser = new tabs.tabBrowser(this._controller); > + > + // Check if preferences is already opened nit: "...already open" @@ +398,5 @@ > + > + // Check if preferences is already opened > + var prefTabs = tabs.getTabsWithURL("about:preferences", true); > + > + // missing comment? Good idea to have a comment here for the 2 methods we employ if the Preference page is already open. But merge this with the above comment, and remove the empty newline before the if clause. @@ +408,5 @@ > + assert.waitFor(() => (tabBrowser.selectedIndex === prefTabs[0].index), > + "The tab with index '" + prefTabs[0].index + "' has been selected"); > + > + // General pane is auto-selected again when opening the preference tab > + if (paneChange) { Why aren't we waiting for the "general" category if we open the Preferences page via a callback? Also, do we really need this if we're changing the category (and waiting for it to load) anyway? This is fine by me if we're avoiding some race conditions. @@ +431,5 @@ > + * Type of the subdialog to open (it's ID) > + * > + * @returns {SubDialog} New instance of SubDialog > + */ > + openSubDialog : function PreferencesPage_openSubDialog(aCallback, aType) { We could call `aType` directly `aId` @@ +533,5 @@ > + case "manageButton": > + return [findElement.ID(root, "buttonOpenPrefs")]; > + > + case "submit": > + var iframe = this.getElement({type: "remoteFrame"}).getNode(); Rename iframe to root, we just replace the parent/root value for these elements. @@ +561,5 @@ > + * Close the About Accounts page (the tab actually) > + */ > + close : function AboutAccountsPage_close() { > + var tabBrowser = new tabs.tabBrowser(this._controller); > + tabBrowser.closeTab("middleClick", this._index); We should pass in null to get the default closeTab method. @@ +569,5 @@ > + * Open the about accounts in-content page > + * > + * @params {object} [aSpec={}] > + * Information about opening the page > + * @params {function} [aSpec.method="menu"] nit: sort alphabetically @@ +571,5 @@ > + * @params {object} [aSpec={}] > + * Information about opening the page > + * @params {function} [aSpec.method="menu"] > + * Method to use when opening the AboutAccountsPage > + * ("menu", "signIn", "signUp" or "callback") You can specify these directly above: [aSpec.method="menu"|"signIn"|"signUp"|"callback"] @@ +613,5 @@ > + > + // Check that we are not already signed in > + var username = prefsAPI.preferences.getPref("services.sync.username", ""); > + if (username.length > 0) { > + assert.fail("You are already signed in"); This might be to harsh, we might still want to check the logged-in window... this won't work with the opener from "signIn" and "signUp". Maybe we should throw there if the right conditions are not met. @@ +619,5 @@ > + > + var tabBrowser = new tabs.tabBrowser(this._controller); > + > + var aboutAccountsTabs = tabs.getTabsWithURL("about:accounts", true); > + if(aboutAccountsTabs.length > 0) { nit: missing space after if @@ +747,5 @@ > + : this._controller.tabs.activeTab; > + var nodeCollector = new domUtils.nodeCollector(root); > + > + switch (spec.type) { > + case "closeButton": Please prefix all outer vs inner dialog elements. Not sure what the best name would be. "inner" vs "outer" might work... if you have a better idea that would be great :) @@ +758,5 @@ > + nodeCollector.queryAnonymousNode("dlgtype", spec.subtype); > + > + elems = nodeCollector.elements; > + break; > + case "frame": This would be something like "inner_root"

Attachment #8499483 - Flags: review?(andrei.eftimie)

Attachment #8499483 - Flags: review?(andreea.matei)

Attachment #8499483 - Flags: review-

•

11 years ago

Attached patch library_v4.1.patch (obsolete) — Details — Splinter Review

Attachment #8501065 - Attachment is obsolete: true

Attachment #8501065 - Flags: review?(hskupin)

Attachment #8501066 - Flags: review?(hskupin)

Andreea Matei [:AndreeaMatei]

Updated

•

11 years ago

Whiteboard: [sprint]

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

Comment 72

•

11 years ago

Comment on attachment 8501066 [details] [diff] [review] library_v4.1.patch Review of attachment 8501066 [details] [diff] [review]: ----------------------------------------------------------------- This is a massive patch! It's great work Daniel, but I wish we can really cut it down into smaller pieces in the future. Especially I do not see why the about:accounts page has been included here. This is a totally different topic. There are way too many remaining things to work on here, so the patch should not have gotten r+ yet. Please see all my inline comments for closer details. ::: firefox/lib/toolbars.js @@ +843,5 @@ > + * @constructor > + * @param {MozMillController} [aController=mozmill.getBrowserController()] > + * MozMillController of the Browser > + */ > +function MenuPanel(aController) { What has this class to do with the in-content preferences? This is totally separate, and should be handled that way. Please move all this out to a separate bug. @@ +904,5 @@ > + var root = spec.parent ? spec.parent.getNode(): this._controller.window.document; > + var elems = []; > + > + switch (spec.type) { > + case "fxaStatus": For specific elements inside the panel you should bundle all with the same prefix. Here `panel_` might be a good choice. @@ +913,5 @@ > + break; > + case "panel": > + elems = [findElement.ID(root, "PanelUI-popup")]; > + break; > + case "preferences": This is indeed a way to open the preferences, but as said earlier it doesn't qualify to get this full UI added in that bug. Also add the prefix so it is named `panel_preferences`. @@ +929,5 @@ > + * > + * @returns {string} email address > + */ > + getEmailAddress : function MenuPanel_getEmailAddress() { > + return this.getElement({type: "fxaStatus"}).getNode().getAttribute("tooltiptext"); Why tooltiptext? What we want is .label. @@ +1041,5 @@ > +function openMenuPanel(aController) { > + var menuPanel = new MenuPanel(aController); > + menuPanel.open(); > + return menuPanel; > +} We wanted to have an opener for new windows because that is complex. I don't see why it is necessary here. ::: firefox/lib/ui/browser.js @@ +104,5 @@ > + * @params {string} [aSpec.method] > + * Method to use when opening > + * @params {function} [aSpec.callback] > + * Callback function that opens the page > + * @params {string} [aSpec.pane="general"] We should not force a default. If none is specified we want the pane, which is currently displayed. @@ +128,5 @@ > + * @returns {AboutAccountsPage} Instance of an AboutAccountsPage object > + */ > +BrowserWindow.prototype.openAboutAccounts = function BW_openAboutAccounts(aSpec={}) { > + aSpec.controller = this._controller; > + return new prefs.openAboutAccounts(aSpec); Again, this is not in-content preferences and should be moved out to a separate bug. ::: firefox/lib/ui/prefs.js @@ +19,5 @@ > +var widgets = require("../../../lib/ui/widgets"); > + > +const CATEGORIES = { > + general: { > + "data-category": "paneGeneral" Why do we need the extra level for "data-category"? It's always the same so it can be abstracted. This constant should be a simple listing like: general: "paneGeneral", content: "paneContent", @@ +24,5 @@ > + }, > + content: { > + "data-category": "paneContent" > + }, > + application: { I would name it 'applications', so it's in sync. @@ +46,5 @@ > + * 'Preferences' in content page class > + * > + * @constructor > + * @param {MozMillController} [aController=mozmill.getBrowserController()] > + * MozMillController of the Preferences Page This is not fully true. By default you take the chrome window and not the content one. In general you want the chrome window. @@ +52,5 @@ > +function PreferencesPage(aController) { > + this._controller = aController || mozmill.getBrowserController(); > +} > + > +PreferencesPage.prototype = { I thought that I mentioned to add a BaseInContentPage class, which can hold most of the properties and methods similar to BaseWindow. @@ +69,5 @@ > + * @returns {ElemBase} The panel category element > + */ > + get category() { > + return this.getElement({type: "category_menu_selected"}); > + }, Please try to be in sync with the add-ons manager here. Also something else helpful would be a getter/setter for categoryID similar to what we have in the add-on manager. @@ +80,5 @@ > + get dtds() { > + return ["chrome://browser/locale/preferences/preferences.dtd", > + "chrome://branding/locale/brand.dtd", > + "chrome://browser/locale/baseMenuOverlay.dtd"]; > + }, See above for a base class. Also for which entity is the baseMenuOverlay.dtd necessary? Otherwise please sort alphabetically. @@ +86,5 @@ > + /** > + * Select another category from the preferences page > + * > + * @param {string} aCategory > + * The category to switch to Getters and setters always have to be of the same type! @@ +92,5 @@ > + set category(aCategory) { > + assert.ok(CATEGORIES[aCategory], "Specified category exists"); > + > + var categoryButton = this.getElement({type: "category_menu_item", > + subtype: aCategory}); This should better be value instead of subtype. Subtype has to be used for e.g. attributes or properties. But here the name is already part of the id. @@ +102,5 @@ > + // Bug 1073483 > + // Discover a way to handle hidden elements in a page > + // (e.g. elements from the tabbed preferences page) > + // Remove the sleep once fixed > + this._controller.sleep(100); Please get this fixed. This might be a problem in isLoaded. Especially 100ms can cause a good chunk of race conditions. Lets really talk with a dev on which events we might have to wait for! @@ +116,5 @@ > + return false; > + } > + > + var categoryElements = this.getElements({type: "category_page_elements", > + subtype: aCategory}); See above regarding value. @@ +120,5 @@ > + subtype: aCategory}); > + > + return !categoryElements.some(aElement => { > + return aElement.getNode().getAttribute("hidden"); > + }); If we really need that you should make use of `hasAttribute()` because it gets removed if the element is visible. @@ +153,5 @@ > + * Identifier of the element > + * @param {string} aSpec.subtype > + * Attribute of the element to filter > + * @param {string} [aSpec.parent=document] > + * Parent of the to find element nit: of the what? @@ +165,5 @@ > + var root = spec.parent ? spec.parent.getNode() : this._controller.tabs.activeTab; > + > + switch (spec.type) { > + case "category_menu_wrapper": > + elems = [findElement.ID(root, "categories")]; I do not like our id for this element. It's not understandable. I had to search for what it actually is. So why aren't you simply name it 'categoryList' as what we have in the add-on manager? @@ +167,5 @@ > + switch (spec.type) { > + case "category_menu_wrapper": > + elems = [findElement.ID(root, "categories")]; > + break; > + case "category_menu_item": Why not using the nodeCollector here? So we can have the same API as for the add-on manager with using subtype and value? That will help to already filter out unwanted categories. @@ +169,5 @@ > + elems = [findElement.ID(root, "categories")]; > + break; > + case "category_menu_item": > + assert.ok(spec.subtype, "Category has been specified"); > + elems = [findElement.ID(root, "category-" + spec.subtype)]; As mentioned above, here you want value. @@ +173,5 @@ > + elems = [findElement.ID(root, "category-" + spec.subtype)]; > + break; > + case "category_menu_selected": > + elems = [findElement.Selector(this._controller.tabs.activeTab, > + ".category[selected=true][current=true]")] I do not see why we need a separate entry for this in getElements. It should be covered via 'categories' and subtype + value. Also why do we have to check both selected and current? Isn't one enough? @@ +175,5 @@ > + case "category_menu_selected": > + elems = [findElement.Selector(this._controller.tabs.activeTab, > + ".category[selected=true][current=true]")] > + break; > + case "category_page_elements": 'pane_elements'. The categories are on the left side. @@ +180,5 @@ > + assert.ok(spec.subtype, "Category has been specified"); > + elems = [findElement.Selector(this._controller.tabs.activeTab, > + "[data-category='" + > + CATEGORIES[spec.subtype]["data-category"] + > + "']")]; I would really suggest to use the nodeCollector here and do the filterbyDOMProperty(). @@ +219,5 @@ > + case "sync_disconnectButton": > + elems = [findElement.ID(root, "fxaUnlinkButton")]; > + break; > + case "sync_manageLink": > + elems = [findElement.Selector(root, '#fxaLoginStatus label.text-link')]; There are multiple labels below fxaLoginStatus. I would suggest you make use of the button ID 'verifiedManage' directly. @@ +231,5 @@ > + case "sync_email": > + elems = [findElement.ID(root, "fxaEmailAddress1")]; > + break; > + case "sync_preferences": > + elems = [findElement.Selector(root, "#fxaSyncEngines checkbox")]; I would use nodeCollector here with an optional filter via subtype and value for a specific checkbox. @@ +238,5 @@ > + elems = [findElement.ID(root, 'tosPP-small-ToS')]; > + break; > + case "sync_privacyPolicy": > + elems = [findElement.ID(root, 'tosPP-small-PP')]; > + break; Will we ever need those two entries? I doubt so. @@ +239,5 @@ > + break; > + case "sync_privacyPolicy": > + elems = [findElement.ID(root, 'tosPP-small-PP')]; > + break; > + case "sync_weavePrefsDeck": I would give it an easier name like 'sync_deck'. The 'weave' part is kinda optional for us. @@ +265,5 @@ > + selectHistory : function PP_expandCustonHistory(aSpec) { > + this.category = "privacy"; > + // TO DO > + // Bug 1077356 > + // Implement the selectHistory method in PreferencesPage Please use the format: // TODO: Bug 1077356 // %Desciption% @@ +273,5 @@ > + * Set the home page from the general category > + * > + * @param {object} [aSpec={}] > + * Information about setting the home page > + * @param {string} [aSpec.method="default"|"url"|"bookmark"|"current"] "default" should be "restore". But I'm not sure if we should make it the default value. Maybe better to make it a mandatory parameter. @@ +278,5 @@ > + * Method to use > + * @param {object} [aSpec.type] > + * Type of information we need > + * @param {object} [aSpec.subtype] > + * More information we need in opening Both descriptions don't say anything for someone reading this the first time. Please make sure to improve the information. @@ +301,5 @@ > + }, "selectBookmarkDialog"); > + > + var tree = bookmarksDialog.getElement({type: "inner_element", > + subtype: "bookmarks"}); > + widgets.clickTreeCell(this._controller, tree, aSpec.type, 0, {}); I don't think that all the above is handling opening/closing the sub folders, right? @@ +312,5 @@ > + var useDefault = this.getElement({type: "general_restoreHomePage"}); > + useDefault.click(); > + break; > + case "url": > + assert.ok(aSpec.type, "Url has been defined"); nit: 'URL' @@ +336,5 @@ > + * Method to use when opening the Preferences page > + * @params {string} [aSpec.pane="general"|"content"|...] > + * Pane to focus after opening the preferences page > + * @params {boolean} [aSpec.paneChange=true] > + * True if the pane will get changed automatically, false otherwise Not sure what this is. Please explain better in the description. @@ +338,5 @@ > + * Pane to focus after opening the preferences page > + * @params {boolean} [aSpec.paneChange=true] > + * True if the pane will get changed automatically, false otherwise > + * > + * @returns {AboutAccountsPage} Instance of an AboutAccountsPage object No, this is not what this method returns. @@ +341,5 @@ > + * > + * @returns {AboutAccountsPage} Instance of an AboutAccountsPage object > + */ > + open : function PreferencesPage_open(aSpec={}) { > + this._controller = aSpec.controller || this._controller; The controller we shouldn't make part of the spec. We don't do this anywhere else. @@ +372,5 @@ > + case "callback": > + assert.equal(typeof aSpec.callback, "function", > + "Callback has been defined"); > + > + aSpec.callback(); As we talked in the ask an expert meeting, the preferences class itself doesn't know how to open itself. All opening methods have to be passed in via a callback, and e.g. in the browser you have an openPreferences() method, which accepts the different methods and pass them into this method via a callback. @@ +424,5 @@ > + } > +} > + > +/** > + * 'About Accounts' in content page class Why is that class located in prefs.js? This is not part of the preferences at all. I'm not going to review all this code for this bug. @@ +596,5 @@ > + * Implemented in bug 1021618 > + * > + * @constructor > + * @param {MozMillController} aController > + * MozMillController of the Preferences Page Not true. You want the chrome controller. @@ +600,5 @@ > + * MozMillController of the Preferences Page > + */ > +function SubDialog(aController) { > + this._controller = aController || mozmill.getBrowserController(); > +} I assume that this type of dialog is not only used for preferences, but also for other types of in-content pages? We should move it to the base in-content module. Or even in windows.js if it is also used outside of those pages? @@ +634,5 @@ > + case "shortcut" : > + this._controller.keypress(null, "VK_ESCAPE", {}); > + break; > + case "callback": > + assert.ok(typeof aCallback, "function", assert.equal() @@ +694,5 @@ > + : this._controller.tabs.activeTab; > + var nodeCollector = new domUtils.nodeCollector(root); > + > + switch (spec.type) { > + case "inner_button": Why not simply use 'button'? Which of those can we retrieve here? @@ +698,5 @@ > + case "inner_button": > + assert.ok(spec.subtype, "Dlg subtype has been specified"); > + > + nodeCollector.root = this.getElement({type: "inner_root"}).getNode(); > + nodeCollector.queryAnonymousNode("dlgtype", spec.subtype); I think all that should be similar to the real dialog entries. Maybe really better to have a BaseDialog class based on BaseWindow. @@ +711,5 @@ > + nodeCollector.queryNodes("#" + spec.subtype); > + > + elems = nodeCollector.elements; > + break; > + case "inner_root": 'frame'? @@ +714,5 @@ > + break; > + case "inner_root": > + elems = [findElement.ID(root, "dialogFrame")]; > + break; > + case "outer_close": is this a button? @@ +717,5 @@ > + break; > + case "outer_close": > + elems = [findElement.ID(root, "dialogClose")]; > + break; > + case "outer_title": why the outer prefix?

Attachment #8501066 - Flags: review?(hskupin) → review-

Daniel Gherasim

Reporter

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #72) > ::: firefox/lib/toolbars.js > @@ +843,5 @@ > > + * @constructor > > + * @param {MozMillController} [aController=mozmill.getBrowserController()] > > + * MozMillController of the Browser > > + */ > > +function MenuPanel(aController) { > > What has this class to do with the in-content preferences? This is totally > separate, and should be handled that way. Please move all this out to a > separate bug. > The preferences-button. Filed bug 1079710. > @@ +52,5 @@ > > +function PreferencesPage(aController) { > > + this._controller = aController || mozmill.getBrowserController(); > > +} > > + > > +PreferencesPage.prototype = { > > I thought that I mentioned to add a BaseInContentPage class, which can hold > most of the properties and methods similar to BaseWindow. > You mention as : "would be good to have in the future". I won't stay blocked by it, and do it in in a follow-up, even though I don't see the point of it, as in-content pages are pretty different by their implementation. Filed bug 1079728. > @@ +80,5 @@ > > + get dtds() { > > + return ["chrome://browser/locale/preferences/preferences.dtd", > > + "chrome://branding/locale/brand.dtd", > > + "chrome://browser/locale/baseMenuOverlay.dtd"]; > > + }, > > See above for a base class. Also for which entity is the baseMenuOverlay.dtd > necessary? Otherwise please sort alphabetically. > For the "preferencesCmdMac.commandkey". > @@ +238,5 @@ > > + elems = [findElement.ID(root, 'tosPP-small-ToS')]; > > + break; > > + case "sync_privacyPolicy": > > + elems = [findElement.ID(root, 'tosPP-small-PP')]; > > + break; > > Will we ever need those two entries? I doubt so. > This is one of the checks we do in the TPS tests. > @@ +372,5 @@ > > + case "callback": > > + assert.equal(typeof aSpec.callback, "function", > > + "Callback has been defined"); > > + > > + aSpec.callback(); > > As we talked in the ask an expert meeting, the preferences class itself > doesn't know how to open itself. All opening methods have to be passed in > via a callback, and e.g. in the browser you have an openPreferences() > method, which accepts the different methods and pass them into this method > via a callback. > That's what we discussed and the implementation is done as you asked. I already did here all types of implemenations for this, so we are going back-and-forward with this for a good time. :( Anyway, both implementations are good and do the job, but, as well as Andrei, I prefer this one. > @@ +424,5 @@ > > + } > > +} > > + > > +/** > > + * 'About Accounts' in content page class > > Why is that class located in prefs.js? This is not part of the preferences > at all. I'm not going to review all this code for this bug. > Filed bug 1079717 > @@ +600,5 @@ > > + * MozMillController of the Preferences Page > > + */ > > +function SubDialog(aController) { > > + this._controller = aController || mozmill.getBrowserController(); > > +} > > I assume that this type of dialog is not only used for preferences, but also > for other types of in-content pages? We should move it to the base > in-content module. Or even in windows.js if it is also used outside of those > pages? > In the description of the class I added: /** * In-Content preferences SubDialog * Implemented in bug 1021618 * ... */ This subDialog is specific to preferences.

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

Comment 75

•

11 years ago

(In reply to Andrei Eftimie from comment #73) > > Especially I do not see why > > the about:accounts page has been included here. This is a totally different > > topic. > Because you pushed to delay TPS tests until we got libraries ready for the > new > in-content preference page ready. This ought to address all so we can copy it > over to the TPS repository. That's not an excuse for mixing implementations. A wise separation between new features with smaller patches would give us more throughput as massive patches like this one. Keep in mind that as larger a patch becomes as longer it will take to get it in. > > > +function MenuPanel(aController) { > > > > What has this class to do with the in-content preferences? This is totally > > separate, and should be handled that way. Please move all this out to a > > separate bug. > Same reason as mentioned above: TPS tests => last quarter goals. > Type: TPS-library Not a reason for inclusion into this patch, sorry. > > @@ +904,5 @@ > > > + var root = spec.parent ? spec.parent.getNode(): this._controller.window.document; > > > + var elems = []; > > > + > > > + switch (spec.type) { > > > + case "fxaStatus": > > > > For specific elements inside the panel you should bundle all with the same > > prefix. Here `panel_` might be a good choice. > Valid, though all elements are from inside the panel... Not all of them. We could consider to mark the e.g. panel button separately. It's not located inside the panel. > > @@ +1041,5 @@ > > > +function openMenuPanel(aController) { > > > + var menuPanel = new MenuPanel(aController); > > > + menuPanel.open(); > > > + return menuPanel; > > > +} > > > > We wanted to have an opener for new windows because that is complex. I don't > > see why it is necessary here. > > I don't understand. How is this more complex than opening the > about:preferences > page? You fought to have this "open helper method". > > Please compare with: > > function openPreferences(aSpec={}) { > > var prefsPage = new PreferencesPage(); > > prefsPage.open(aSpec); > > return prefsPage; > > } > > They seem pretty comparable complexity wise to me. The complexity is not contained in this method but in the open() method by checking tabs, possible events, or observers etc. But it might be indeed better for consistency to have this method for the menu panel. So taken. > The "about:preferences" page opens the #general pane whenver you open it > specifically. The only way the last item is preserved is when you have this Have we gotten a reply from a dev yet if that is wanted or a regression? I cannot remember that I have seen one. Maybe I missed it? > > I thought that I mentioned to add a BaseInContentPage class, which can hold > > most of the properties and methods similar to BaseWindow. > > So the about:accounts and the MenuPane are out-of-scope for this bug, but we > should have included a new (yey!) abstract class which doesn't really help > _now_ which you off-handed mentioned in one of the meetings once as a nice to > have in the future. It helps us similar as the new window ui classes. There would have been low overhead to create this class, which would handle the dtds, the controller etc. Keep in mind that we can go ahead with leaving all this but post-poning that much, I see a risk in getting again too much duplicated code. So lets test it when we will have the time for that rewrite. Maybe you can find a contributor to help us here. > > @@ +69,5 @@ > > > + * @returns {ElemBase} The panel category element > > > + */ > > > + get category() { > > > + return this.getElement({type: "category_menu_selected"}); > > > + }, > > > > Please try to be in sync with the add-ons manager here. Also something else > > helpful would be a getter/setter for categoryID similar to what we have in > > the add-on manager. > These are different beasts. They are built differently, and they have > different > functionalities. > > From your comment I assume you would rather name this `selectedCategory` > Type: name-change From the users perspective both are similar. They have categories listed on the left side, and separate panes with elements on the right. The tests should not know about the details of implementation. > > @@ +92,5 @@ > > > + set category(aCategory) { > > > + assert.ok(CATEGORIES[aCategory], "Specified category exists"); > > > + > > > + var categoryButton = this.getElement({type: "category_menu_item", > > > + subtype: aCategory}); > > > > This should better be value instead of subtype. Subtype has to be used for > > e.g. attributes or properties. But here the name is already part of the id. > Type: name-change This is not only a name change. Compared to other ui classes this is changing how to get a specific element from a list. > > @@ +102,5 @@ > > > + // Bug 1073483 > > > + // Discover a way to handle hidden elements in a page > > > + // (e.g. elements from the tabbed preferences page) > > > + // Remove the sleep once fixed > > > + this._controller.sleep(100); > > > > Please get this fixed. This might be a problem in isLoaded. Especially 100ms > > can cause a good chunk of race conditions. > > > > Lets really talk with a dev on which events we might have to wait for! > *sigh* > This is the main reason we don't have a Download test 1 year after the > feature > has been changed. We did decide to do workarounds but DOCUMENT them, file > appropriate bugs, and try fixing them ASAP. > I still firmly stand by: code not shipped === 0. Were you ever in contact with a dev about that problem? Have you gotten an answer? If not, have you continued to try to get an answer? This bug is already open for a while, so there was enough time to have a discussion about that. sleep() is eval, especially with such small numbers, and is highly the reason for race conditions. > > @@ +120,5 @@ > > > + subtype: aCategory}); > > > + > > > + return !categoryElements.some(aElement => { > > > + return aElement.getNode().getAttribute("hidden"); > > > + }); > > > > If we really need that you should make use of `hasAttribute()` because it > > gets removed if the element is visible. > No, the attribute is still present. Please check the code first. > Type: invalid That's exactly what I did with the DOM inspector. For me the attribute is getting removed. Just checked that again. > > @@ +167,5 @@ > > > + switch (spec.type) { > > > + case "category_menu_wrapper": > > > + elems = [findElement.ID(root, "categories")]; > > > + break; > > > + case "category_menu_item": > > > > Why not using the nodeCollector here? So we can have the same API as for the > > add-on manager with using subtype and value? That will help to already > > filter out unwanted categories. > The **only** valid reason to use node-collector is for anonymous elements. > We have ID's here. Please read my comment closely. We won't have a single element returned by that change. Consistency should be taken into account compared to individual implementations for each and every ui class. > > @@ +180,5 @@ > > > + assert.ok(spec.subtype, "Category has been specified"); > > > + elems = [findElement.Selector(this._controller.tabs.activeTab, > > > + "[data-category='" + > > > + CATEGORIES[spec.subtype]["data-category"] + > > > + "']")]; > > > > I would really suggest to use the nodeCollector here and do the > > filterbyDOMProperty(). > As above, I strongly disagree. That would mean: > - more code > - performance loss I totally don't see why this is more code. It would be a single line compared to 4 lines here. The performance loss would be minimal and would only apply to the call for the filter. > Why all these calls for nodeCollector? Do you hate CSS selectors that much? > All those filterings can be done directly via a CSS selector - which is > _much_ > faster than doing it by had in JS (or worse by NC). As you know Selector is returning only a single element. This drastically limits us in our flexibility, which was the reason why we have created nodeCollector a long time ago. You could have a single item via getElements with flexible filters, or dozen of separate entries in the switch block. > > // TODO: Bug 1077356 > > // %Desciption% > Style guidelines differ: > https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/ > Mozmill_Style_Guide#Commenting > Might as well remove the TODO item Then remove it. To be recognized by editors we might even have to make it a tag. Btw todo items are not covered yet in the style guide, so we cannot reference it. > > @@ +341,5 @@ > > > + * > > > + * @returns {AboutAccountsPage} Instance of an AboutAccountsPage object > > > + */ > > > + open : function PreferencesPage_open(aSpec={}) { > > > + this._controller = aSpec.controller || this._controller; > > > > The controller we shouldn't make part of the spec. We don't do this anywhere > > else. > Type: enhancement? No, this needs a fix. > > @@ +372,5 @@ > > > + case "callback": > > > + assert.equal(typeof aSpec.callback, "function", > > > + "Callback has been defined"); > > > + > > > + aSpec.callback(); > > > > As we talked in the ask an expert meeting, the preferences class itself > > doesn't know how to open itself. All opening methods have to be passed in > > via a callback, and e.g. in the browser you have an openPreferences() > > method, which accepts the different methods and pass them into this method > > via a callback. > No. In the meeting we discussed it _exactly_ like it is implemented here. > Type: invalid You are wrong here. As I said, you cannot put any shortcut, menu item etc. into the preferences class from the source window the preferences can be opened. All this is closely bound to the source window, and only that knows about it. Exactly that is what we have agreed on, but not that we use specific methods from a foreign class. I don't see that notes have been taken during the ask an expert session. > > @@ +634,5 @@ > > > + case "shortcut" : > > > + this._controller.keypress(null, "VK_ESCAPE", {}); > > > + break; > > > + case "callback": > > > + assert.ok(typeof aCallback, "function", > > > > assert.equal() > Type: valid-enhancement Why enhancement? This is clearly broken and has to be fixed. > > @@ +694,5 @@ > > > + : this._controller.tabs.activeTab; > > > + var nodeCollector = new domUtils.nodeCollector(root); > > > + > > > + switch (spec.type) { > > > + case "inner_button": > > > > Why not simply use 'button'? Which of those can we retrieve here? > I specifically asked in the previous review to have outer vs inner elements > separated by a prefix. All subDialogs have some common "chrome/outer" > elements. > > This should get all inner dlgtype elements based on the subtype. > Type: invalid This was based on your opinions but there was never an agreement about how to name them. So please refrain from calling things like those invalid. An early feedback or needinfo request would have been helpful so we could had a discussion way earlier about it. > Type: useless-enhancement // yey more abstract classes! I would suggest you calm down a bit, so we can return to have a constructive discussion. This is not helpful. > > why the outer prefix? > Because I asked to differentiate between chrome vs content elements on these > subdialogs. What is chrome on those dialogs? They are totally located in content and not chrome. I don't see a reasoning here. Maybe you want to explain.

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

Comment 76

•

11 years ago

(In reply to daniel.gherasim from comment #74) > > What has this class to do with the in-content preferences? This is totally > > separate, and should be handled that way. Please move all this out to a > > separate bug. > > The preferences-button. This is only another way to open the preferences. It can be done when its necessary. > > I thought that I mentioned to add a BaseInContentPage class, which can hold > > most of the properties and methods similar to BaseWindow. > > You mention as : "would be good to have in the future". > I won't stay blocked by it, and do it in in a follow-up, even though I don't > see the point of it, as in-content pages are pretty different by their > implementation. > > Filed bug 1079728. This is not the correct bug number. You want bug 1079725 here. > > > > See above for a base class. Also for which entity is the baseMenuOverlay.dtd > > necessary? Otherwise please sort alphabetically. > > For the "preferencesCmdMac.commandkey". This is only known by the browser window, but not by the in-content preferences. At maximum we need the DTDs as imported by the preferences itself: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.xul#18 > > @@ +372,5 @@ > > > + case "callback": > > > + assert.equal(typeof aSpec.callback, "function", > > > + "Callback has been defined"); > > > + > > > + aSpec.callback(); > > > > As we talked in the ask an expert meeting, the preferences class itself > > doesn't know how to open itself. All opening methods have to be passed in > > via a callback, and e.g. in the browser you have an openPreferences() > > method, which accepts the different methods and pass them into this method > > via a callback. > > That's what we discussed and the implementation is done as you asked. I > already did here all types of implemenations for this, so we are going > back-and-forward with this for a good time. :( This is not what we have discussed. See my comment above to Andrei. You are mixing behaviors between classes, and add additional logic the preferences class does not have to care about. > > I assume that this type of dialog is not only used for preferences, but also > > for other types of in-content pages? We should move it to the base > > in-content module. Or even in windows.js if it is also used outside of those > > pages? > > This subDialog is specific to preferences. Ok, fine then. So we can keep the class.

Daniel Gherasim

Reporter

Comment 77

•

11 years ago

Attached file structure.js — Details

(In reply to Henrik Skupin (:whimboo) from comment #76) > > > @@ +372,5 @@ > > > > + case "callback": > > > > + assert.equal(typeof aSpec.callback, "function", > > > > + "Callback has been defined"); > > > > + > > > > + aSpec.callback(); > > > > > > As we talked in the ask an expert meeting, the preferences class itself > > > doesn't know how to open itself. All opening methods have to be passed in > > > via a callback, and e.g. in the browser you have an openPreferences() > > > method, which accepts the different methods and pass them into this method > > > via a callback. > > > > That's what we discussed and the implementation is done as you asked. I > > already did here all types of implemenations for this, so we are going > > back-and-forward with this for a good time. :( > > This is not what we have discussed. See my comment above to Andrei. You are > mixing behaviors between classes, and add additional logic the preferences > class does not have to care about. > That's what I've understood from the meeting, the implementation I did here. I really didn't have any reason to fight against you're proposal and do a different implementation, especially given that I was going to ask you for review. :) Please confirm that this is the implementation you want (check the attachment) ============== Similar with the above we'll have the placesWindowOrganizer, pageInfoWindow, and so on, so BrowserWindow we'll become big with many classes we add that can be open from the browser window.

Flags: needinfo?(hskupin)

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

Updated

•

11 years ago

Attachment #8501744 - Attachment mime type: application/javascript → text/plain

Flags: needinfo?(hskupin)

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

Comment 78

•

11 years ago

Comment on attachment 8501744 [details] structure.js Yes, that is the implementation I was referring to in our last ask an expert meeting. This looks good and has a clear separation of ownership between the source and target window/page. >PreferencesPage.prototype.openAboutAccountsPage = function PP_openAboutAccountsPage(aMethod) { > var callback = () { > switch (aMethod) { > // This will only open about accounts through the signIn and signOut options > } > } >} We could do it this way or via two helper methods e.g. 'fxaSignIn()' and 'fxaSignout()'. Your version is more flexible, so we may want to use that. >BrowserWindow.prototype.openPreferences = function BW_openPreferences(aSpec) { > var callback = () => { > switch (aSpec.method) { > // ... > } > } > > return prefs.openPreferences({controller: this._controller, > callback: callback, > pane: aSpec.pane}); That looks way better, whereby we still have to come to an agreement how to pass the controller.

Daniel Gherasim

Reporter

Comment 79

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #78) > Comment on attachment 8501744 [details] > structure.js > > > > return prefs.openPreferences({controller: this._controller, > > callback: callback, > > pane: aSpec.pane}); > > That looks way better, whereby we still have to come to an agreement how to > pass the controller. How about as an optional parameter? (will default to mozmill.getBrowserController() in BaseInContentPage) The above will look like: > return prefs.openPreferences({callback: callback, pane: aSpec.pane}, this._controller);

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

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #82) > Given that no-one asked Jared yet regarding an event we can listen for, I > finally did that myself. So by inspecting the code Jared told me that there > is no event yet, but we should get a bug filed. Daniel, so please do so. > Thanks. Filed bug 1083094. Thanks.

No longer depends on: 1083094

Daniel Gherasim

Reporter

Updated

•

11 years ago

Depends on: 1083094

Daniel Gherasim

Reporter

Updated

•

11 years ago

status-firefox36: --- → affected

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 84

•

11 years ago

(In reply to Henrik Skupin (:whimboo) from comment #72) > > +function MenuPanel(aController) { > > What has this class to do with the in-content preferences? This is totally > separate, and should be handled that way. Please move all this out to a > separate bug. Fixed in bug 1079710 > > +BrowserWindow.prototype.openAboutAccounts = function BW_openAboutAccounts(aSpec={}) { > > + aSpec.controller = this._controller; > > + return new prefs.openAboutAccounts(aSpec); > Again, this is not in-content preferences and should be moved out to a > separate bug. > Bug 1079717.> ::: firefox/lib/ui/prefs.js > @@ +102,5 @@ > > + // Bug 1073483 > > + // Discover a way to handle hidden elements in a page > > + // (e.g. elements from the tabbed preferences page) > > + // Remove the sleep once fixed > > + this._controller.sleep(100); > > Please get this fixed. This might be a problem in isLoaded. Especially 100ms > can cause a good chunk of race conditions. > > Lets really talk with a dev on which events we might have to wait for! Bug 1083094 comment 20, waiting for Initialized event and "weave:service:ready" it's enough.

Cosmin Malutan, [:cosmin-malutan]

Assignee

Updated

•

11 years ago

Assignee: danisielm → cosmin.malutan

Andreea Matei [:AndreeaMatei]

Updated

•

11 years ago

Blocks: 1105780

Cosmin Malutan, [:cosmin-malutan]

Assignee

•

11 years ago

Attached patch patch v7.2 (obsolete) — Details — Splinter Review

(In reply to Andreea Matei [:AndreeaMatei] from comment #93) > Not sure why you removed the 2 libs, please add them back. I missed them when I fixed a reject, thanks for catching this.

Attachment #8538316 - Attachment is obsolete: true

Attachment #8538434 - Flags: review?(hskupin)

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

Comment 95

•

11 years ago

Comment on attachment 8538434 [details] [diff] [review] patch v7.2 Review of attachment 8538434 [details] [diff] [review]: ----------------------------------------------------------------- ::: firefox/lib/tests/testAboutPreferences.js @@ +167,5 @@ > + var aboutPreferences = browserWindow.openAboutPreferencesPage(); > + > + // Test preferences in-content elements exist > + TEST_DATA["prefsPage"].forEach(aCategory => { > + if (aCategory.category !== "layout") { If that is not a category you may want to re-organize the dict and move this a level up or into another sub category. @@ +190,5 @@ > + > + // Test subdialog & it's basic elements > + aboutPreferences.category = "content"; > + > + // Open the in-content langauges dialog nit: languages ::: firefox/lib/tests/testMenuPanel.js @@ +51,5 @@ > + var preferencesButton = menuPanel.getElement({type: "panel_preferences"}); > + var aboutPreferencesPage = browserWindow.openAboutPreferencesPage( > + {type: "callback", callback: () => { preferencesButton.click(); }} > + ); > + aboutPreferencesPage.close(); The above does not open the panel, which might be necessary to dynamically update its content and menu element status. ::: firefox/lib/ui/about-accounts-page.js @@ +128,5 @@ > +function openAboutAccountsPage(aSpec={}) { > + var accountsPage = new AboutAccountsPage(aSpec.browserWindow); > + accountsPage.open(aSpec); > + return accountsPage; > +} Same as what I mentioned for the preferences class. Not sure if this helper is useful. Because it was not added initially when this module has been created. ::: firefox/lib/ui/about-preferences-page.js @@ +8,5 @@ > + > +// For sync pane to be ready we have to wait for weave:service:ready observer. > +// This event is dispatched only once, and can be triggered by other action than > +// opening the preferences. I register it here and set the flag > +var wsrObserver = { The name should be more descriptive, e.g. weaveReadyObserver. Also please fix the last sentence of the comment, not sure who "I" is. @@ +36,5 @@ > + sync: "paneSync", > + advanced: "paneAdvanced" > +}; > + > +function AboutPreferencesPage(aBrowserWindow) { missing jsdoc comment. @@ +40,5 @@ > +function AboutPreferencesPage(aBrowserWindow) { > + baseInContentPage.BaseInContentPage.call(this, aBrowserWindow); > + > + this._dtds = ["chrome://browser/locale/preferences/preferences.dtd", > + "chrome://branding/locale/brand.dtd", nit: move the branding up for alphabetical order. @@ +90,5 @@ > + var categoryElements = this.getElements({type: "category_page_elements", > + subtype: aCategory}); > + return !categoryElements.some(aElement => { > + return aElement.getNode().getAttribute("hidden"); > + }); Why do we need this extra method? Can't we make sure that .category returns the currently selected one, which can be checked then via the id? @@ +119,5 @@ > + case "category_menu_wrapper": > + elems = [findElement.ID(root, "categories")]; > + break; > + case "category_menu_item": > + assert.ok(spec.subtype, "Category has been specified"); Those are not menus. So please remove the 'menu' part of the id. @@ +125,5 @@ > + break; > + case "category_menu_selected": > + elems = [findElement.Selector(root, ".category[selected=true][current=true]")]; > + break; > + case "category_page_elements": It's unclear for me where those elements live, what is page? @@ +163,5 @@ > + break; > + case "general_downloadsAlwaysAskRadio": > + elems = [findElement.ID(root, "alwaysAsk")]; > + break; > + case "general_downloadFolderFilefield": Suffixes like "filefield" i find kinda awkward. Maybe we should start to remove those suffixes at all. It would cause naming conflicts once the type is changed anyway. @@ +201,5 @@ > + elems = [findElement.ID(root, "addEngines")]; > + break; > + case "content_popupPolicyCheckbox": > + elems = [findElement.ID(root, "popupPolicy")]; > + break; Search comes after content in the alphabet. Please sort it that way. @@ +205,5 @@ > + break; > + case "content_popupPolicyExceptions": > + elems = [findElement.ID(root, "popupPolicyButton")]; > + break; > + case "content_fontMenu": Same here font vs. popup. This is not sorted. @@ +466,5 @@ > + * Information about setting the home page > + * @param {string} [aSpec.method="default"] > + * Method to use to set the category("default"|"url"|"bookmark"|"current") > + * @param {object} [aSpec.type] > + * Type of information we need Not sure what this is about, it needs a way better documentation. @@ +471,5 @@ > + * @param {object} [aSpec.subtype] > + * More information we need in opening > + */ > +AboutPreferencesPage.prototype.setHomePage = function APP_setHomePage(aSpec={}) { > + this.category = "general"; Do not hardcode but use a defined constant. @@ +473,5 @@ > + */ > +AboutPreferencesPage.prototype.setHomePage = function APP_setHomePage(aSpec={}) { > + this.category = "general"; > + > + var method = aSpec.method || "default"; This has to come first in this method. @@ +478,5 @@ > + > + switch (method) { > + case "bookmark": > + assert.equal(typeof aSpec.type, "number", > + "Bookmark row index to select has been defined"); What kind of index should that be? Not sure if it makes sense to combine it with spec.type, or better have a spec.index here. @@ +504,5 @@ > + var useDefault = this.getElement({type: "general_restoreHomePage"}); > + useDefault.click(); > + break; > + case "url": > + assert.ok(aSpec.type, "Url has been defined"); Same here. Maybe both would match spec.value, but its clearly not a type. @@ +567,5 @@ > + initialized = true; > + }); > + baseInContentPage.BaseInContentPage.prototype.open.call(this, callback); > + assert.waitFor(() => initialized && wsrObserver.ready, > + "Preferences panes have been initialized"); We leak this listener if we fail in that code. @@ +601,5 @@ > + baseInContentPage.BaseInContentPage.call(this, aBrowserWindow); > +} > + > +SubDialog.prototype = Object.create(baseInContentPage.BaseInContentPage.prototype); > +SubDialog.prototype.constructor = SubDialog; The dialog is not an in-content page, so it cannot subclassed from it. @@ +623,5 @@ > + try { > + switch (aMethod) { > + case "callback": > + assert.ok(typeof aCallback, "function", > + "Callback function has been specified"); nit: remove function. @@ +710,5 @@ > + * Id of the subdialog to open > + */ > +SubDialog.prototype.open = function SubDialog_openSubDialog(aSpec={}) { > + assert.equal(typeof aSpec.callback, "function", > + "Callback function has been specified"); remove function @@ +756,5 @@ > +function openAboutPreferencesPage(aSpec={}) { > + var prefsPage = new AboutPreferencesPage(aSpec.browserWindow); > + prefsPage.open(aSpec); > + return prefsPage; > +} The method in how to open the page, has to be defined in the source class similar to browser window. The prefs page doesn't know about it itself, and has to handle it via a callback. Further I would stop doing magic in terms of auto-selecting a pane after opening the in-content preferences. This should not be part of the open() method. So I question if this helper is useful at all. ::: firefox/lib/ui/browser.js @@ +175,5 @@ > + * > + * @params {object} [aSpec={}] > + * Information about opening the page > + * @params {string} [aSpec.method] > + * Method to use when opening The in-content page doesn't know anything about that. It has to be selected in this method and passed-in via a callback.

Attachment #8538434 - Flags: review?(hskupin) → review-

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 96

•

11 years ago

Attached patch patch v8.0 (obsolete) — Details — Splinter Review

(In reply to Henrik Skupin (:whimboo) from comment #95) > > + var preferencesButton = menuPanel.getElement({type: "panel_preferences"}); > > + var aboutPreferencesPage = browserWindow.openAboutPreferencesPage( > > + {type: "callback", callback: () => { preferencesButton.click(); }} > > + ); > > + aboutPreferencesPage.close(); > > The above does not open the panel, which might be necessary to dynamically > update its content and menu element status. The menu panel is opened at the beginning of the test. We open the about:preferences page by clicking on the menu element. This can be seen by human eye if you ran the test. > @@ +128,5 @@ > > +function openAboutAccountsPage(aSpec={}) { > > + var accountsPage = new AboutAccountsPage(aSpec.browserWindow); > > + accountsPage.open(aSpec); > > + return accountsPage; > > +} > > Same as what I mentioned for the preferences class. Not sure if this helper > is useful. Because it was not added initially when this module has been > created. No it doesn't, as you said it should live only under the browser module, even the name is duplicated, but I got confused by the structure.js attachment, where we have an openAboutAccountsPage method both in aboutAccounts class and in browser.js, I fixed this in this patch version. > > + var categoryElements = this.getElements({type: "category_page_elements", > > + subtype: aCategory}); > > + return !categoryElements.some(aElement => { > > + return aElement.getNode().getAttribute("hidden"); > > + }); > > Why do we need this extra method? Can't we make sure that .category returns > the currently selected one, which can be checked then via the id? It worked for me every time, and I tested this well, so no we don't need it, I removed it. > @@ +466,5 @@ > > + * Information about setting the home page > > + * @param {string} [aSpec.method="default"] > > + * Method to use to set the category("default"|"url"|"bookmark"|"current") > > + * @param {object} [aSpec.type] > > + * Type of information we need > > Not sure what this is about, it needs a way better documentation. This was a helper method for setting the homepage inside of general category. That's outside of the scope of this library so I removed it. > @@ +601,5 @@ > > + baseInContentPage.BaseInContentPage.call(this, aBrowserWindow); > > +} > > + > > +SubDialog.prototype = Object.create(baseInContentPage.BaseInContentPage.prototype); > > +SubDialog.prototype.constructor = SubDialog; > > The dialog is not an in-content page, so it cannot subclassed from it. It is, it's not really a dialog, it's a in-page element that get's displayed. I couldn't inherit from unknown-content-type-dialog.js because that it will require it to have a real different window, but that's not the case. The in-content classes are the only one that doesn't require a different window object so I thought of it to be correct one to inherit from. Henrik please check the above answers so I can pass this to Andreea for review.

Attachment #8538434 - Attachment is obsolete: true

Attachment #8539260 - Flags: feedback?(hskupin)

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

Comment 97

•

11 years ago

Comment on attachment 8539260 [details] [diff] [review] patch v8.0 Review of attachment 8539260 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Cosmin Malutan from comment #96) > > > + var preferencesButton = menuPanel.getElement({type: "panel_preferences"}); > > > + var aboutPreferencesPage = browserWindow.openAboutPreferencesPage( > > > + {type: "callback", callback: () => { preferencesButton.click(); }} > > > + ); > > > + aboutPreferencesPage.close(); > > > > The above does not open the panel, which might be necessary to dynamically > > update its content and menu element status. > The menu panel is opened at the beginning of the test. We open the > about:preferences page by clicking on the menu element. This can be seen by > human eye if you ran the test. Sure but the menu panel is closed after opening about:accounts right before. > > > + baseInContentPage.BaseInContentPage.call(this, aBrowserWindow); > > > +} > > > + > > > +SubDialog.prototype = Object.create(baseInContentPage.BaseInContentPage.prototype); > > > +SubDialog.prototype.constructor = SubDialog; > > > > The dialog is not an in-content page, so it cannot subclassed from it. > It is, it's not really a dialog, it's a in-page element that get's > displayed. I couldn't inherit from unknown-content-type-dialog.js because > that it will require it to have a real different window, but that's not the > case. The in-content classes are the only one that doesn't require a > different window object so I thought of it to be correct one to inherit > from. It's not a in-content page! It's simply an element inside such a page, but that doesn't qualify it to be subclassed from BaseInContentPage. It looks like we need a class like ContentDialog, which could also cover other tab modal dialogs later. f- only because the changes necessary for the sub dialogs. Otherwise the implementation looks way better now. Thanks for the update!

Attachment #8539260 - Flags: feedback?(hskupin) → feedback-

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 98

•

11 years ago

Attached patch patch v9.0 (obsolete) — Details — Splinter Review

(In reply to Henrik Skupin (:whimboo) from comment #97) > (In reply to Cosmin Malutan from comment #96) > > > > + var preferencesButton = menuPanel.getElement({type: "panel_preferences"}); > > > > + var aboutPreferencesPage = browserWindow.openAboutPreferencesPage( > > > > + {type: "callback", callback: () => { preferencesButton.click(); }} > > > > + ); > > > > + aboutPreferencesPage.close(); > > > > > > The above does not open the panel, which might be necessary to dynamically > > > update its content and menu element status. > > The menu panel is opened at the beginning of the test. We open the > > about:preferences page by clicking on the menu element. This can be seen by > > human eye if you ran the test. > > Sure but the menu panel is closed after opening about:accounts right before. It's not, if it will the test itself would fail, I can explicitly close it and open it again, but that will be useless. > > > > + baseInContentPage.BaseInContentPage.call(this, aBrowserWindow); > > > > +} > > > > + > > > > +SubDialog.prototype = Object.create(baseInContentPage.BaseInContentPage.prototype); > > > > +SubDialog.prototype.constructor = SubDialog; > > > > > > The dialog is not an in-content page, so it cannot subclassed from it. > > It is, it's not really a dialog, it's a in-page element that get's > > displayed. I couldn't inherit from unknown-content-type-dialog.js because > > that it will require it to have a real different window, but that's not the > > case. The in-content classes are the only one that doesn't require a > > different window object so I thought of it to be correct one to inherit > > from. > > It's not a in-content page! It's simply an element inside such a page, but > that doesn't qualify it to be subclassed from BaseInContentPage. It looks > like we need a class like ContentDialog, which could also cover other tab > modal dialogs later. > > f- only because the changes necessary for the sub dialogs. Otherwise the > implementation looks way better now. Thanks for the update! I made a new generic class, the only thing that we needed from inheritance was the getter for browserWindow and the getElement method for returning a single element. If you like the change of the dialog class, I'll pass this over to Andreea for a closer review. Thanks!

Attachment #8539260 - Attachment is obsolete: true

Attachment #8542947 - Flags: feedback?(hskupin)

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

Comment 99

•

11 years ago

Comment on attachment 8542947 [details] [diff] [review] patch v9.0 Review of attachment 8542947 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Cosmin Malutan from comment #98) > > > > The above does not open the panel, which might be necessary to dynamically > > > > update its content and menu element status. > > > The menu panel is opened at the beginning of the test. We open the > > > about:preferences page by clicking on the menu element. This can be seen by > > > human eye if you ran the test. > > > > Sure but the menu panel is closed after opening about:accounts right before. > It's not, if it will the test itself would fail, I can explicitly close it > and open it again, but that will be useless. If you test this manually it will be closed. So that is a general problem with popups and simple clicks on menu entries. That's why we have the MenuAPI in Mozmill which actually closes the popup after the action to ensure that no focus issues might come up. Also please see my other feedback regarding subdialogs inline. Looks like we can help ourselves a lot when updating the hierarchy of classes and let those subdialogs inherit most of the properties and methods from the BaseWindow class. ::: firefox/lib/ui/base-in-content-dialog.js @@ +15,5 @@ > + * > + * @param {object} aBrowserWindow > + * Browser window where the dialog lives > + */ > +function BaseInContentDialog(aBrowserWindow) { This is called SubDialog in the Firefox code, so I think we should use BaseSubDialog as class name. Also I looked in how those dialogs are actually getting called and have seen that they are basic dialogs simply opened in content. So what we might need here is the following class hierarchy: BaseWindow -> BaseDialog -> BaseSubDialog -> specific classes Whereby in BaseSubDialog the open and close methods have to be overwritten to make use of the new events. Otherwise it looks like we can inherit everything else. Then for BaseDialog we could add the handling for all the default buttons available in dialogs as you already did here, so it gets inherited to any subclass. For that type of class we already have bug 879972. @@ +56,5 @@ > + > + aCallback(); > + break; > + case "shortcut": > + this.browserWindow.controller.keypress(null, "VK_ESCAPE", {}); You retrieve the contentWindow above. You might want to use that as element to trigger the keypress on.

Attachment #8542947 - Flags: feedback?(hskupin) → feedback-

Cosmin Malutan, [:cosmin-malutan]

•

11 years ago

Comment on attachment 8545750 [details] [diff] [review] patch v10.0 Review of attachment 8545750 [details] [diff] [review]: ----------------------------------------------------------------- ::: firefox/lib/tests/testAboutPreferences.js @@ +141,5 @@ > + "Element has been found - " + aElement.name); > + }); > + }); > + > + Extra blank line, please remove ::: firefox/lib/ui/about-accounts-page.js @@ +35,5 @@ > /** > * Open the about accounts in-content page > * > + * @params {function} aCallback > + * Callback function that opens the page nit: Callback that opens the page @@ +91,5 @@ > }; > > // Export of classes > exports.AboutAccountsPage = AboutAccountsPage; > + Extra blank line at the end of the file ::: firefox/lib/ui/about-preferences-page.js @@ +456,5 @@ > +/** > + * Open the about:preferences in-content page > + * > + * @params {function} aCallback > + * Callback function that opens the page "Callback that opens the page" ::: firefox/lib/ui/browser.js @@ +178,5 @@ > + * @params {function} [aSpec.method="menu"] > + * Method to use when opening the Preferences page > + * ("menu"|"shortcut"|"menuPanel"|"callback") > + * @params {function} [aSpec.callback] > + * Callback function that opens the page "Callback that opens the page" @@ +212,5 @@ > + break; > + default: > + assert.fail("Unknown event type - " + method); > + } > + } Add ; after } @@ +228,5 @@ > + * Information about opening the page > + * @params {string} [aSpec.method] > + * Method to use when opening > + * @params {function} [aSpec.callback] > + * Callback function that opens the page "Callback that opens the page" @@ +261,5 @@ > + * Open the places organizer window > + * > + * @param {object} [aSpec] > + * Information for opening the window > + * @param {string} [aSpec.type="shortcut"] I think this should be aSpec.method, just like the way it is used in other libs. @@ +262,5 @@ > + * > + * @param {object} [aSpec] > + * Information for opening the window > + * @param {string} [aSpec.type="shortcut"] > + * How to open the Library Window ("menu", "shortcut") "Method to use to open the page" @@ +264,5 @@ > + * Information for opening the window > + * @param {string} [aSpec.type="shortcut"] > + * How to open the Library Window ("menu", "shortcut") > + * @param {string} [aSpec.location="bookmarks"] > + * Which pane to be selected after opening the window "The panel to be selected after..."

Attachment #8545750 - Flags: review?(mihaela.velimiroviciu)

Attachment #8545750 - Flags: review?(andreea.matei)

Attachment #8545750 - Flags: review-

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 104

•

11 years ago

Attached patch patch v11.0 (obsolete) — Details — Splinter Review

(In reply to Mihaela Velimiroviciu [QA] (:mihaelav) from comment #103) > > @@ +261,5 @@ > > + * Open the places organizer window > > + * > > + * @param {object} [aSpec] > > + * Information for opening the window > > + * @param {string} [aSpec.type="shortcut"] > > I think this should be aSpec.method, just like the way it is used in other > libs. > > @@ +262,5 @@ > > + * > > + * @param {object} [aSpec] > > + * Information for opening the window > > + * @param {string} [aSpec.type="shortcut"] > > + * How to open the Library Window ("menu", "shortcut") > > "Method to use to open the page" > > @@ +264,5 @@ > > + * Information for opening the window > > + * @param {string} [aSpec.type="shortcut"] > > + * How to open the Library Window ("menu", "shortcut") > > + * @param {string} [aSpec.location="bookmarks"] > > + * Which pane to be selected after opening the window > > "The panel to be selected after..." Those were not added in this patch, had an extra + at the beginning of the comment, but I won't make the patch heavier with changes that don't belong

Attachment #8545750 - Attachment is obsolete: true

Attachment #8547424 - Flags: review?(mihaela.velimiroviciu)

Attachment #8547424 - Flags: review?(andreea.matei)

Andreea Matei [:AndreeaMatei]

Comment 105

•

11 years ago

Comment on attachment 8547424 [details] [diff] [review] patch v11.0 Review of attachment 8547424 [details] [diff] [review]: ----------------------------------------------------------------- You can ask from Henrik with these nits solved. ::: firefox/lib/tests/testAboutPreferences.js @@ +127,5 @@ > + > + aModule.browserWindow.tabs.closeAllTabs(); > +} > + > +function testPreferences() { jsdoc please ::: firefox/lib/ui/browser.js @@ +17,1 @@ > var baseWindow = require("../../../lib/ui/base-window"); Please move this on a separate block @@ +243,5 @@ > + this.controller.mainMenu.click("#sync-setup"); > + break; > + case "callback": > + assert.equal(typeof aSpec.callback, "function", > + "Callback has been defined"); Switch the cases order.

Attachment #8547424 - Flags: review?(mihaela.velimiroviciu)

Attachment #8547424 - Flags: review?(andreea.matei)

Attachment #8547424 - Flags: review+

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 106

•

11 years ago

Attached patch patch v11.1 (obsolete) — Details — Splinter Review

(In reply to Andreea Matei [:AndreeaMatei] from comment #105) > ::: firefox/lib/ui/browser.js > @@ +17,1 @@ > > var baseWindow = require("../../../lib/ui/base-window"); > > Please move this on a separate block This is in the UI modules block, beside that, it was already here.

Attachment #8547424 - Attachment is obsolete: true

Attachment #8549485 - Flags: review?(hskupin)

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

Comment 107

•

11 years ago

Comment on attachment 8549485 [details] [diff] [review] patch v11.1 Review of attachment 8549485 [details] [diff] [review]: ----------------------------------------------------------------- Looks better but not complete yet. Please fix the smaller issues and re-request review from me. Thanks. ::: firefox/lib/tests/testAboutAccountsPage.js @@ +18,5 @@ > aModule.browserWindow.tabs.closeAllTabs(); > } > > function testAboutAccounts() { > + aboutAccounts = browserWindow.openAboutAccountsPage(); You missed to declare this variable. ::: firefox/lib/tests/testAboutPreferences.js @@ +125,5 @@ > +function setupModule(aModule) { > + aModule.browserWindow = new browser.BrowserWindow(); > + > + aModule.browserWindow.tabs.closeAllTabs(); > +} This test doesn't have a teardown method and does not close opened tabs. ::: firefox/lib/ui/about-preferences-page.js @@ +16,5 @@ > + }, > + ready: false, > + topic: "weave:service:ready" > +}; > +Services.obs.addObserver(weaveReadyObserver, weaveReadyObserver.topic, false); You have to do the registering in the open() method. Otherwise this will only run once and a second try to open about:preferences will fail. Also we would leak this registered observer if it doesn't fire. ::: firefox/lib/ui/browser.js @@ +201,5 @@ > + break; > + case "shortcut": > + if (mozmill.isMac) { > + var cmdKey = utils.getEntity(this.dtds, > + "preferencesCmdMac.commandkey"); this.getEntity()

Attachment #8549485 - Flags: review?(hskupin) → review-

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 108

•

11 years ago

Attached patch patch v11.2 (obsolete) — Details — Splinter Review

(In reply to Henrik Skupin (:whimboo) from comment #107) > ::: firefox/lib/ui/about-preferences-page.js > @@ +16,5 @@ > > + }, > > + ready: false, > > + topic: "weave:service:ready" > > +}; > > +Services.obs.addObserver(weaveReadyObserver, weaveReadyObserver.topic, false); > > You have to do the registering in the open() method. Otherwise this will > only run once and a second try to open about:preferences will fail. Also we > would leak this registered observer if it doesn't fire. This notification is dispatched only once, as the name says, when the service is initialized, Bug 1083094 comment 20. This is also explained in the comment above the observer.

Attachment #8549485 - Attachment is obsolete: true

Attachment #8551778 - Flags: review?(hskupin)

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

Comment 109

•

11 years ago

Comment on attachment 8551778 [details] [diff] [review] patch v11.2 Review of attachment 8551778 [details] [diff] [review]: ----------------------------------------------------------------- > (In reply to Henrik Skupin (:whimboo) from comment #107) > > ::: firefox/lib/ui/about-preferences-page.js > > @@ +16,5 @@ > > > + }, > > > + ready: false, > > > + topic: "weave:service:ready" > > > +}; > > > +Services.obs.addObserver(weaveReadyObserver, weaveReadyObserver.topic, false); > > > > You have to do the registering in the open() method. Otherwise this will > > only run once and a second try to open about:preferences will fail. Also we > > would leak this registered observer if it doesn't fire. > > This notification is dispatched only once, as the name says, when the > service is initialized, Bug 1083094 comment 20. This is also explained in > the comment above the observer. This will not help us given that we can miss to unregister the observer and leak. I digged around a bit and found the following code, which sounds way better to be used in this class given that it can always be checked and not only once for the first time. https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-syncui.js#37

Attachment #8551778 - Flags: review?(hskupin) → review-

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 110

•

11 years ago

Attached patch patch v12.0 (obsolete) — Details — Splinter Review

It's way better, thanks.

Attachment #8551778 - Attachment is obsolete: true

Attachment #8552441 - Flags: review?(hskupin)

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

Comment 111

•

11 years ago

Comment on attachment 8552441 [details] [diff] [review] patch v12.0 Review of attachment 8552441 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the remaining issue fixed. ::: firefox/lib/ui/about-preferences-page.js @@ +4,5 @@ > + > +"use strict"; > + > +Cu.import("resource://gre/modules/Services.jsm"); > +var weaveService = Cc["@mozilla.org/weave/service;1"] Please separate those into different blocks.

Attachment #8552441 - Flags: review?(hskupin) → review+

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 112

•

11 years ago

Attached patch patch v12.1 — Details — Splinter Review

Thanks.

Attachment #8552441 - Attachment is obsolete: true

Attachment #8552896 - Flags: review?(andreea.matei)

Andreea Matei [:AndreeaMatei]

Comment 113

•

11 years ago

Comment on attachment 8552896 [details] [diff] [review] patch v12.1 Review of attachment 8552896 [details] [diff] [review]: ----------------------------------------------------------------- https://hg.mozilla.org/qa/mozmill-tests/rev/dc355c60f693 (default) Yey!

Attachment #8552896 - Flags: review?(andreea.matei) → review+

Andreea Matei [:AndreeaMatei]

Updated

•

11 years ago

status-firefox35: affected → ---

status-firefox37: --- → affected

status-firefox38: --- → fixed

Cosmin Malutan, [:cosmin-malutan]

Assignee

Comment 114

•

11 years ago

Andreea we need to backport this only to aurora, on Beta the in-content preferences are not enabled by default. The patch applies cleanly and the lib tests runs well.

status-firefox36: affected → unaffected

Cosmin Malutan, [:cosmin-malutan]

Assignee

Updated

•

11 years ago

Flags: needinfo?(andreea.matei)

Andreea Matei [:AndreeaMatei]

Comment 115

•

11 years ago

https://hg.mozilla.org/qa/mozmill-tests/rev/68f513f087eb (aurora) Thanks Cosmin, lot of work here!

Status: ASSIGNED → RESOLVED

Closed: 11 years ago

status-firefox37: affected → fixed

Flags: needinfo?(andreea.matei)

Resolution: --- → FIXED

BMO Automation

Updated

•

6 years ago

Product: Mozilla QA → Mozilla QA Graveyard

bug_1005811_wa.patch 11 years ago Daniel Gherasim 21.44 KB, patch	andrei : review-	Details \| Diff \| Splinter Review
bug_1005811_wa_2.patch 11 years ago Daniel Gherasim 24.95 KB, patch	andrei : review-	Details \| Diff \| Splinter Review
bug_1005811_wa_3.patch 11 years ago Daniel Gherasim 25.44 KB, patch		Details \| Diff \| Splinter Review
bug_1005811_wa_3.1.patch 11 years ago Daniel Gherasim 25.67 KB, patch	AndreeaMatei : review-	Details \| Diff \| Splinter Review
bug_1005811_wa_3.2.patch 11 years ago Daniel Gherasim 25.69 KB, patch	AndreeaMatei : review+	Details \| Diff \| Splinter Review
WIP.patch 11 years ago Daniel Gherasim 49.92 KB, patch	andrei : feedback+ whimboo : feedback-	Details \| Diff \| Splinter Review
v1.patch 11 years ago Daniel Gherasim 93.00 KB, patch	andrei : review-	Details \| Diff \| Splinter Review
v2.patch 11 years ago Daniel Gherasim 91.74 KB, patch	andrei : review-	Details \| Diff \| Splinter Review
v3.patch 11 years ago Daniel Gherasim 107.60 KB, patch	andrei : review-	Details \| Diff \| Splinter Review
v4_WIP.patch 11 years ago Daniel Gherasim 111.60 KB, patch		Details \| Diff \| Splinter Review
v5.patch 11 years ago Daniel Gherasim 102.36 KB, patch	AndreeaMatei : review-	Details \| Diff \| Splinter Review
library_v1.patch 11 years ago Daniel Gherasim 34.07 KB, patch	andrei : review-	Details \| Diff \| Splinter Review
library_v2.patch 11 years ago Daniel Gherasim 39.04 KB, patch	andrei : review-	Details \| Diff \| Splinter Review
library_v3.patch 11 years ago Daniel Gherasim 41.88 KB, patch		Details \| Diff \| Splinter Review
library_v3.1.patch 11 years ago Daniel Gherasim 42.00 KB, patch	andrei : review-	Details \| Diff \| Splinter Review
library_v4.patch 11 years ago Daniel Gherasim 42.39 KB, patch	andrei : review+	Details \| Diff \| Splinter Review
v6.patch 11 years ago Daniel Gherasim 102.36 KB, patch		Details \| Diff \| Splinter Review
library_v4.1.patch 11 years ago Daniel Gherasim 42.40 KB, patch	whimboo : review-	Details \| Diff \| Splinter Review
structure.js 11 years ago Daniel Gherasim 3.06 KB, text/plain		Details
WIP patch v5.0 11 years ago Cosmin Malutan, [:cosmin-malutan] 23.99 KB, patch		Details \| Diff \| Splinter Review
patch v5.1 11 years ago Cosmin Malutan, [:cosmin-malutan] 40.06 KB, patch	mihaelav : review-	Details \| Diff \| Splinter Review
patch v6.0 11 years ago Cosmin Malutan, [:cosmin-malutan] 41.04 KB, patch	mihaelav : review-	Details \| Diff \| Splinter Review
patch v7.0 11 years ago Cosmin Malutan, [:cosmin-malutan] 42.85 KB, patch	AndreeaMatei : review-	Details \| Diff \| Splinter Review
patch v7.1 11 years ago Cosmin Malutan, [:cosmin-malutan] 43.29 KB, patch	AndreeaMatei : review+	Details \| Diff \| Splinter Review
patch v7.2 11 years ago Cosmin Malutan, [:cosmin-malutan] 43.92 KB, patch	whimboo : review-	Details \| Diff \| Splinter Review
patch v8.0 11 years ago Cosmin Malutan, [:cosmin-malutan] 41.74 KB, patch	whimboo : feedback-	Details \| Diff \| Splinter Review
patch v9.0 11 years ago Cosmin Malutan, [:cosmin-malutan] 43.48 KB, patch	whimboo : feedback-	Details \| Diff \| Splinter Review
patch v10.0 11 years ago Cosmin Malutan, [:cosmin-malutan] 35.90 KB, patch	mihaelav : review-	Details \| Diff \| Splinter Review
patch v11.0 11 years ago Cosmin Malutan, [:cosmin-malutan] 34.58 KB, patch	AndreeaMatei : review+	Details \| Diff \| Splinter Review
patch v11.1 11 years ago Cosmin Malutan, [:cosmin-malutan] 34.65 KB, patch	whimboo : review-	Details \| Diff \| Splinter Review
patch v11.2 11 years ago Cosmin Malutan, [:cosmin-malutan] 34.92 KB, patch	whimboo : review-	Details \| Diff \| Splinter Review
patch v12.0 11 years ago Cosmin Malutan, [:cosmin-malutan] 34.56 KB, patch	whimboo : review+	Details \| Diff \| Splinter Review
patch v12.1 11 years ago Cosmin Malutan, [:cosmin-malutan] 34.56 KB, patch	AndreeaMatei : review+	Details \| Diff \| Splinter Review