From 687189b4079c69ad33ba7448c218700ba74962dc Mon Sep 17 00:00:00 2001 From: Timothy Nikkel Date: Mon, 9 Sep 2019 23:44:58 +0000 Subject: [PATCH 01/24] Bug 1284651. Allow the image surface cache to grow beyond 1GB on 64bit builds with sufficient memory. r=aosmond Before this patch: surface cache size = min(1GB, system_memory/4) After this patch: surface cache size = min(32bit ? 1GB : 2GB , system_memory/4) sizeof(uintptr_t) is the best I can figure out to detect 32bit builds. Differential Revision: https://phabricator.services.mozilla.com/D45092 --HG-- extra : moz-landing-system : lando --- image/SurfaceCache.cpp | 5 +++++ modules/libpref/init/StaticPrefList.yaml | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/image/SurfaceCache.cpp b/image/SurfaceCache.cpp index babdf45c951a..ae24cde6b82a 100644 --- a/image/SurfaceCache.cpp +++ b/image/SurfaceCache.cpp @@ -1391,6 +1391,11 @@ void SurfaceCache::Initialize() { uint64_t surfaceCacheMaxSizeKB = StaticPrefs::image_mem_surfacecache_max_size_kb_AtStartup(); + if (sizeof(uintptr_t) <= 4) { + // Limit surface cache to 1 GB if our address space is 32 bit. + surfaceCacheMaxSizeKB = 1024 * 1024; + } + // A knob determining the actual size of the surface cache. Currently the // cache is (size of main memory) / (surface cache size factor) KB // or (surface cache max size) KB, whichever is smaller. The formula diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 595a0dd39e21..c71cde329164 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -3750,7 +3750,7 @@ # Maximum size for the surface cache, in kilobytes. - name: image.mem.surfacecache.max_size_kb type: uint32_t - value: 1024 * 1024 + value: 2024 * 1024 mirror: once # Minimum timeout for expiring unused images from the surface cache, in From 62a5c58f09f474e7e54f257c0aa8c928c95fe676 Mon Sep 17 00:00:00 2001 From: Nathan Froyd Date: Mon, 9 Sep 2019 23:54:34 +0000 Subject: [PATCH 02/24] Bug 1579430 - mark some elfhack methods as `override`; r=glandium This change silences a couple of clang warnings. Differential Revision: https://phabricator.services.mozilla.com/D45007 --HG-- extra : moz-landing-system : lando --- build/unix/elfhack/elfhack.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/build/unix/elfhack/elfhack.cpp b/build/unix/elfhack/elfhack.cpp index 2c24cbac4b57..ec01e54674b2 100644 --- a/build/unix/elfhack/elfhack.cpp +++ b/build/unix/elfhack/elfhack.cpp @@ -194,7 +194,7 @@ class ElfRelHackCode_Section : public ElfSection { ~ElfRelHackCode_Section() { delete elf; } - void serialize(std::ofstream& file, char ei_class, char ei_data) { + void serialize(std::ofstream& file, char ei_class, char ei_data) override { // Readjust code offsets for (std::vector::iterator c = code.begin(); c != code.end(); ++c) @@ -217,7 +217,7 @@ class ElfRelHackCode_Section : public ElfSection { ElfSection::serialize(file, ei_class, ei_data); } - bool isRelocatable() { return false; } + bool isRelocatable() override { return false; } unsigned int getEntryPoint() { return entry_point; } From e4e7986acbb452c591bbc5e8fad2ed482c2e9154 Mon Sep 17 00:00:00 2001 From: Neil Deakin Date: Mon, 9 Sep 2019 15:42:43 +0000 Subject: [PATCH 03/24] Bug 1577388, use the top browser when receiving zoom messages, r=mconley Differential Revision: https://phabricator.services.mozilla.com/D45178 --HG-- extra : moz-landing-system : lando --- toolkit/actors/ZoomParent.jsm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/toolkit/actors/ZoomParent.jsm b/toolkit/actors/ZoomParent.jsm index 36c871b0d31b..53e131bbddc0 100644 --- a/toolkit/actors/ZoomParent.jsm +++ b/toolkit/actors/ZoomParent.jsm @@ -8,7 +8,7 @@ var EXPORTED_SYMBOLS = ["ZoomParent"]; class ZoomParent extends JSWindowActorParent { receiveMessage(message) { - let browser = this.browsingContext.embedderElement; + let browser = this.browsingContext.top.embedderElement; if (!browser) { return; } From 15535c9309f3fd944f78fd909dad4e8961aa01e8 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Tue, 10 Sep 2019 00:09:52 +0000 Subject: [PATCH 04/24] Bug 1575921 - store the FxA device ID in an FxA-specific pref branch. r=eoger Differential Revision: https://phabricator.services.mozilla.com/D43897 --HG-- extra : moz-landing-system : lando --- .../components/preferences/in-content/sync.js | 5 +- services/fxaccounts/FxAccounts.jsm | 64 +++----- services/fxaccounts/FxAccountsCommon.js | 4 + services/fxaccounts/FxAccountsDevice.jsm | 150 ++++++++++++++++++ services/fxaccounts/moz.build | 1 + .../test_accounts_device_registration.js | 81 +++------- .../fxaccounts/tests/xpcshell/test_device.js | 92 +++++++++++ .../fxaccounts/tests/xpcshell/xpcshell.ini | 1 + services/sync/modules/engines/clients.js | 21 +-- services/sync/modules/util.js | 69 -------- services/sync/tests/unit/head_helpers.js | 9 -- .../sync/tests/unit/test_clients_engine.js | 36 ++++- services/sync/tests/unit/test_utils_misc.js | 33 ---- services/sync/tests/unit/xpcshell.ini | 1 - 14 files changed, 335 insertions(+), 232 deletions(-) create mode 100644 services/fxaccounts/FxAccountsDevice.jsm create mode 100644 services/fxaccounts/tests/xpcshell/test_device.js delete mode 100644 services/sync/tests/unit/test_utils_misc.js diff --git a/browser/components/preferences/in-content/sync.js b/browser/components/preferences/in-content/sync.js index 9741e43e8cff..1beba1cbc2dd 100644 --- a/browser/components/preferences/in-content/sync.js +++ b/browser/components/preferences/in-content/sync.js @@ -568,7 +568,10 @@ var gSyncPane = { _populateComputerName(value) { let textbox = document.getElementById("fxaSyncComputerName"); if (!textbox.hasAttribute("placeholder")) { - textbox.setAttribute("placeholder", Weave.Utils.getDefaultDeviceName()); + textbox.setAttribute( + "placeholder", + fxAccounts.device.getDefaultLocalName() + ); } textbox.value = value; }, diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm index b532f2b6f2e2..ef139fc4866c 100644 --- a/services/fxaccounts/FxAccounts.jsm +++ b/services/fxaccounts/FxAccounts.jsm @@ -50,6 +50,7 @@ const { ON_DEVICE_DISCONNECTED_NOTIFICATION, ON_NEW_DEVICE_ID, POLL_SESSION, + PREF_ACCOUNT_ROOT, PREF_LAST_FXA_USER, SERVER_ERRNO_TO_ERROR, SCOPE_OLD_SYNC, @@ -89,16 +90,20 @@ ChromeUtils.defineModuleGetter( ChromeUtils.defineModuleGetter( this, - "FxAccountsProfile", - "resource://gre/modules/FxAccountsProfile.jsm" + "FxAccountsDevice", + "resource://gre/modules/FxAccountsDevice.jsm" ); ChromeUtils.defineModuleGetter( this, - "Utils", - "resource://services-sync/util.js" + "FxAccountsProfile", + "resource://gre/modules/FxAccountsProfile.jsm" ); +XPCOMUtils.defineLazyModuleGetters(this, { + Preferences: "resource://gre/modules/Preferences.jsm", +}); + XPCOMUtils.defineLazyPreferenceGetter( this, "FXA_ENABLED", @@ -113,9 +118,9 @@ var publicProperties = [ "canGetKeys", "checkVerificationStatus", "commands", + "device", "getAccountsClient", "getAssertion", - "getDeviceId", "getDeviceList", "getKeys", "authorizeOAuthCode", @@ -518,6 +523,14 @@ FxAccountsInternal.prototype = { return this._commands; }, + _device: null, + get device() { + if (!this._device) { + this._device = new FxAccountsDevice(this); + } + return this._device; + }, + _oauthClient: null, get oauthClient() { if (!this._oauthClient) { @@ -723,6 +736,7 @@ FxAccountsInternal.prototype = { if (!FXA_ENABLED) { throw new Error("Cannot call setSignedInUser when FxA is disabled."); } + Preferences.resetBranch(PREF_ACCOUNT_ROOT); log.debug("setSignedInUser - aborting any existing flows"); const signedInUser = await this.getSignedInUser(); if (signedInUser) { @@ -841,33 +855,6 @@ FxAccountsInternal.prototype = { return this.currentAccountState.updateUserAccountData({ cert: null }); }, - async getDeviceId() { - let data = await this.currentAccountState.getUserAccountData(); - if (!data) { - // Without a signed-in user, there can be no device id. - return null; - } - // Try migrating first. Remove this in Firefox 65+. - if (data.deviceId) { - log.info("Migrating from deviceId to device."); - await this.currentAccountState.updateUserAccountData({ - deviceId: null, - deviceRegistrationVersion: null, - device: { - id: data.deviceId, - registrationVersion: data.deviceRegistrationVersion, - }, - }); - data = await this.currentAccountState.getUserAccountData(); - } - const { device } = data; - if (await this.checkDeviceUpdateNeeded(device)) { - return this._registerOrUpdateDevice(data); - } - // Return the device id that we already registered with the server. - return device.id; - }, - async checkDeviceUpdateNeeded(device) { // There is no device registered or the device registration is outdated. // Either way, we should register the device with FxA @@ -1022,6 +1009,7 @@ FxAccountsInternal.prototype = { }, async _signOutLocal() { + Preferences.resetBranch(PREF_ACCOUNT_ROOT); await this.currentAccountState.signOut(); // this "aborts" this.currentAccountState but doesn't make a new one. await this.abortExistingFlow(); @@ -2083,7 +2071,7 @@ FxAccountsInternal.prototype = { try { const subscription = await this.fxaPushService.registerPushEndpoint(); - const deviceName = this._getDeviceName(); + const deviceName = this.device.getLocalName(); let deviceOptions = {}; // if we were able to obtain a subscription @@ -2115,7 +2103,7 @@ FxAccountsInternal.prototype = { device = await this.fxAccountsClient.registerDevice( sessionToken, deviceName, - this._getDeviceType(), + this.device.getLocalType(), deviceOptions ); Services.obs.notifyObservers(null, ON_NEW_DEVICE_ID); @@ -2137,14 +2125,6 @@ FxAccountsInternal.prototype = { } }, - _getDeviceName() { - return Utils.getDeviceName(); - }, - - _getDeviceType() { - return Utils.getDeviceType(); - }, - _handleDeviceError(error, sessionToken) { return Promise.resolve() .then(() => { diff --git a/services/fxaccounts/FxAccountsCommon.js b/services/fxaccounts/FxAccountsCommon.js index 19f520a3b9ad..a43c3e9c36e2 100644 --- a/services/fxaccounts/FxAccountsCommon.js +++ b/services/fxaccounts/FxAccountsCommon.js @@ -109,6 +109,10 @@ exports.COMMAND_CHANGE_PASSWORD = "fxaccounts:change_password"; exports.COMMAND_FXA_STATUS = "fxaccounts:fxa_status"; exports.COMMAND_PAIR_PREFERENCES = "fxaccounts:pair_preferences"; +// The pref branch where any prefs which relate to a specific account should +// be stored. This branch will be reset on account signout and signin. +exports.PREF_ACCOUNT_ROOT = "identity.fxaccounts.account."; + exports.PREF_LAST_FXA_USER = "identity.fxaccounts.lastSignedInUserHash"; exports.PREF_REMOTE_PAIRING_URI = "identity.fxaccounts.remote.pairing.uri"; diff --git a/services/fxaccounts/FxAccountsDevice.jsm b/services/fxaccounts/FxAccountsDevice.jsm new file mode 100644 index 000000000000..9eb4d61ca9cc --- /dev/null +++ b/services/fxaccounts/FxAccountsDevice.jsm @@ -0,0 +1,150 @@ +/* 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"; + +const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); + +const { XPCOMUtils } = ChromeUtils.import( + "resource://gre/modules/XPCOMUtils.jsm" +); + +const { log } = ChromeUtils.import( + "resource://gre/modules/FxAccountsCommon.js" +); + +const { DEVICE_TYPE_DESKTOP } = ChromeUtils.import( + "resource://services-sync/constants.js" +); + +const { PREF_ACCOUNT_ROOT } = ChromeUtils.import( + "resource://gre/modules/FxAccountsCommon.js" +); + +const PREF_LOCAL_DEVICE_NAME = PREF_ACCOUNT_ROOT + "device.name"; +XPCOMUtils.defineLazyPreferenceGetter( + this, + "pref_localDeviceName", + PREF_LOCAL_DEVICE_NAME, + "" +); + +const PREF_DEPRECATED_DEVICE_NAME = "services.sync.client.name"; + +// Everything to do with FxA devices. +// TODO: Move more device stuff from FxAccounts.jsm into here - eg, device +// registration, device lists, etc. +class FxAccountsDevice { + constructor(fxa) { + this._fxa = fxa; + } + + async getLocalId() { + let data = await this._fxa.currentAccountState.getUserAccountData(); + if (!data) { + // Without a signed-in user, there can be no device id. + return null; + } + const { device } = data; + if (await this._fxa.checkDeviceUpdateNeeded(device)) { + return this._fxa._registerOrUpdateDevice(data); + } + // Return the device id that we already registered with the server. + return device.id; + } + + // Generate a client name if we don't have a useful one yet + getDefaultLocalName() { + let env = Cc["@mozilla.org/process/environment;1"].getService( + Ci.nsIEnvironment + ); + let user = env.get("USER") || env.get("USERNAME"); + // Note that we used to fall back to the "services.sync.username" pref here, + // but that's no longer suitable in a world where sync might not be + // configured. However, we almost never *actually* fell back to that, and + // doing so sanely here would mean making this function async, which we don't + // really want to do yet. + + // A little hack for people using the the moz-build environment on Windows + // which sets USER to the literal "%USERNAME%" (yes, really) + if (user == "%USERNAME%" && env.get("USERNAME")) { + user = env.get("USERNAME"); + } + + let brand = Services.strings.createBundle( + "chrome://branding/locale/brand.properties" + ); + let brandName; + try { + brandName = brand.GetStringFromName("brandShortName"); + } catch (O_o) { + // this only fails in tests and markh can't work out why :( + brandName = Services.appinfo.name; + } + + // The DNS service may fail to provide a hostname in edge-cases we don't + // fully understand - bug 1391488. + let hostname; + try { + // hostname of the system, usually assigned by the user or admin + hostname = Cc["@mozilla.org/network/dns-service;1"].getService( + Ci.nsIDNSService + ).myHostName; + } catch (ex) { + Cu.reportError(ex); + } + let system = + // 'device' is defined on unix systems + Services.sysinfo.get("device") || + hostname || + // fall back on ua info string + Cc["@mozilla.org/network/protocol;1?name=http"].getService( + Ci.nsIHttpProtocolHandler + ).oscpu; + + // It's a little unfortunate that this string is defined as being weave/sync, + // but it's not worth moving it. + let syncStrings = Services.strings.createBundle( + "chrome://weave/locale/sync.properties" + ); + return syncStrings.formatStringFromName("client.name2", [ + user, + brandName, + system, + ]); + } + + getLocalName() { + // We used to store this in services.sync.client.name, but now store it + // under an fxa-specific location. + let deprecated_value = Services.prefs.getStringPref( + PREF_DEPRECATED_DEVICE_NAME, + "" + ); + if (deprecated_value) { + Services.prefs.setStringPref(PREF_LOCAL_DEVICE_NAME, deprecated_value); + Services.prefs.clearUserPref(PREF_DEPRECATED_DEVICE_NAME); + } + let name = pref_localDeviceName; + if (!name) { + name = this.getDefaultLocalName(); + Services.prefs.setStringPref(PREF_LOCAL_DEVICE_NAME, name); + } + return name; + } + + setLocalName(newName) { + Services.prefs.clearUserPref(PREF_DEPRECATED_DEVICE_NAME); + Services.prefs.setStringPref(PREF_LOCAL_DEVICE_NAME, newName); + // Update the registration in the background. + this._fxa.updateDeviceRegistration().catch(error => { + log.warn("failed to update fxa device registration", error); + }); + } + + getLocalType() { + return DEVICE_TYPE_DESKTOP; + } +} + +var EXPORTED_SYMBOLS = ["FxAccountsDevice"]; diff --git a/services/fxaccounts/moz.build b/services/fxaccounts/moz.build index 530eb2f9bcb3..398bd715827a 100644 --- a/services/fxaccounts/moz.build +++ b/services/fxaccounts/moz.build @@ -22,6 +22,7 @@ EXTRA_JS_MODULES += [ 'FxAccountsCommands.js', 'FxAccountsCommon.js', 'FxAccountsConfig.jsm', + 'FxAccountsDevice.jsm', 'FxAccountsOAuthGrantClient.jsm', 'FxAccountsPairing.jsm', 'FxAccountsPairingChannel.js', diff --git a/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js b/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js index d9246609fb76..9369a8bb995d 100644 --- a/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js +++ b/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js @@ -111,17 +111,14 @@ MockFxAccountsClient.prototype = { __proto__: FxAccountsClient.prototype, }; -function MockFxAccounts(device = {}) { - return new FxAccounts({ - newAccountState(credentials) { +async function MockFxAccounts(credentials, device = {}) { + let fxa = new FxAccounts({ + newAccountState(creds) { // we use a real accountState but mocked storage. let storage = new MockStorageManager(); - storage.initialize(credentials); + storage.initialize(creds); return new AccountState(storage); }, - _getDeviceName() { - return device.name || "mock device name"; - }, async availableCommands() { return {}; }, @@ -146,6 +143,12 @@ function MockFxAccounts(device = {}) { }, DEVICE_REGISTRATION_VERSION, }); + await fxa.internal.setSignedInUser(credentials); + Services.prefs.setStringPref( + "identity.fxaccounts.account.device.name", + device.name || "mock device name" + ); + return fxa; } add_task(async function test_updateDeviceRegistration_with_new_device() { @@ -153,8 +156,7 @@ add_task(async function test_updateDeviceRegistration_with_new_device() { const deviceType = "bar"; const credentials = getTestUser("baz"); - const fxa = new MockFxAccounts({ name: deviceName }); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials, { name: deviceName }); // Remove the current device registration (setSignedInUser does one!). await fxa.updateUserAccountData({ uid: credentials.uid, device: null }); @@ -213,8 +215,7 @@ add_task(async function test_updateDeviceRegistration_with_existing_device() { const deviceName = "phil's device"; const credentials = getTestUser("pb"); - const fxa = new MockFxAccounts({ name: deviceName }); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials, { name: deviceName }); await fxa.updateUserAccountData({ uid: credentials.uid, device: { @@ -278,8 +279,7 @@ add_task( const currentDeviceId = "my device id"; const credentials = getTestUser("baz"); - const fxa = new MockFxAccounts({ name: deviceName }); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials, { name: deviceName }); await fxa.updateUserAccountData({ uid: credentials.uid, device: { @@ -350,8 +350,7 @@ add_task( const conflictingDeviceId = "conflicting device id"; const credentials = getTestUser("baz"); - const fxa = new MockFxAccounts({ name: deviceName }); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials, { name: deviceName }); await fxa.updateUserAccountData({ uid: credentials.uid, device: { @@ -439,8 +438,7 @@ add_task( const deviceName = "foo"; const credentials = getTestUser("baz"); - const fxa = new MockFxAccounts({ name: deviceName }); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials, { name: deviceName }); await fxa.updateUserAccountData({ uid: credentials.uid, device: null }); const spy = { @@ -486,8 +484,7 @@ add_task( async function test_getDeviceId_with_no_device_id_invokes_device_registration() { const credentials = getTestUser("foo"); credentials.verified = true; - const fxa = new MockFxAccounts(); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials); await fxa.updateUserAccountData({ uid: credentials.uid, device: null }); const spy = { count: 0, args: [] }; @@ -502,7 +499,7 @@ add_task( return Promise.resolve("bar"); }; - const result = await fxa.internal.getDeviceId(); + const result = await fxa.internal.device.getLocalId(); Assert.equal(spy.count, 1); Assert.equal(spy.args[0].length, 1); @@ -516,8 +513,7 @@ add_task( async function test_getDeviceId_with_registration_version_outdated_invokes_device_registration() { const credentials = getTestUser("foo"); credentials.verified = true; - const fxa = new MockFxAccounts(); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials); const spy = { count: 0, args: [] }; fxa.internal.currentAccountState.getUserAccountData = () => @@ -534,7 +530,7 @@ add_task( return Promise.resolve("wibble"); }; - const result = await fxa.internal.getDeviceId(); + const result = await fxa.internal.device.getLocalId(); Assert.equal(spy.count, 1); Assert.equal(spy.args[0].length, 1); @@ -547,8 +543,7 @@ add_task( async function test_getDeviceId_with_device_id_and_uptodate_registration_version_doesnt_invoke_device_registration() { const credentials = getTestUser("foo"); credentials.verified = true; - const fxa = new MockFxAccounts(); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials); const spy = { count: 0 }; fxa.internal.currentAccountState.getUserAccountData = async () => ({ @@ -563,7 +558,7 @@ add_task( return Promise.resolve("bar"); }; - const result = await fxa.internal.getDeviceId(); + const result = await fxa.internal.device.getLocalId(); Assert.equal(spy.count, 0); Assert.equal(result, "foo's device id"); @@ -574,8 +569,7 @@ add_task( async function test_getDeviceId_with_device_id_and_with_no_registration_version_invokes_device_registration() { const credentials = getTestUser("foo"); credentials.verified = true; - const fxa = new MockFxAccounts(); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials); const spy = { count: 0, args: [] }; fxa.internal.currentAccountState.getUserAccountData = () => @@ -586,7 +580,7 @@ add_task( return Promise.resolve("wibble"); }; - const result = await fxa.internal.getDeviceId(); + const result = await fxa.internal.device.getLocalId(); Assert.equal(spy.count, 1); Assert.equal(spy.args[0].length, 1); @@ -595,38 +589,11 @@ add_task( } ); -add_task(async function test_migration_toplevel_deviceId_to_device() { - const credentials = getTestUser("foo"); - credentials.verified = true; - const fxa = new MockFxAccounts(); - await fxa.internal.setSignedInUser(credentials); - await fxa.updateUserAccountData({ uid: credentials.uid, device: null }); - // Can't use updateUserAccountData here since it won't accept deprecated fields! - const accountData = - fxa.internal.currentAccountState.storageManager.accountData; - accountData.deviceId = "mydeviceid"; - accountData.deviceRegistrationVersion = DEVICE_REGISTRATION_VERSION; - - const result = await fxa.internal.getDeviceId(); - Assert.equal(result, "mydeviceid"); - - const state = fxa.internal.currentAccountState; - const data = await state.getUserAccountData(); - Assert.deepEqual(data.device, { - id: "mydeviceid", - registrationVersion: DEVICE_REGISTRATION_VERSION, - registeredCommandsKeys: [], - }); - Assert.ok(!data.deviceId); - Assert.ok(!data.deviceRegistrationVersion); -}); - add_task(async function test_devicelist_pushendpointexpired() { const deviceId = "mydeviceid"; const credentials = getTestUser("baz"); credentials.verified = true; - const fxa = new MockFxAccounts(); - await fxa.internal.setSignedInUser(credentials); + const fxa = await MockFxAccounts(credentials); await fxa.updateUserAccountData({ uid: credentials.uid, device: { diff --git a/services/fxaccounts/tests/xpcshell/test_device.js b/services/fxaccounts/tests/xpcshell/test_device.js new file mode 100644 index 000000000000..afc49d338f33 --- /dev/null +++ b/services/fxaccounts/tests/xpcshell/test_device.js @@ -0,0 +1,92 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +const { fxAccounts } = ChromeUtils.import( + "resource://gre/modules/FxAccounts.jsm" +); + +const { PREF_ACCOUNT_ROOT } = ChromeUtils.import( + "resource://gre/modules/FxAccountsCommon.js" +); + +_("Misc tests for FxAccounts.device"); + +add_test(function test_default_device_name() { + // Note that head_helpers overrides getDefaultDeviceName - this test is + // really just to ensure the actual implementation is sane - we can't + // really check the value it uses is correct. + // We are just hoping to avoid a repeat of bug 1369285. + let def = fxAccounts.device._getDefaultLocalName(); // make sure it doesn't throw. + _("default value is " + def); + ok(def.length > 0); + + // This is obviously tied to the implementation, but we want early warning + // if any of these things fail. + // We really want one of these 2 to provide a value. + let hostname = + Services.sysinfo.get("device") || + Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService) + .myHostName; + _("hostname is " + hostname); + ok(hostname.length > 0); + // the hostname should be in the default. + ok(def.includes(hostname)); + // We expect the following to work as a fallback to the above. + let fallback = Cc["@mozilla.org/network/protocol;1?name=http"].getService( + Ci.nsIHttpProtocolHandler + ).oscpu; + _("UA fallback is " + fallback); + ok(fallback.length > 0); + // the fallback should not be in the default + ok(!def.includes(fallback)); + + run_next_test(); +}); + +add_test(function test_migration() { + Services.prefs.clearUserPref("identity.fxaccounts.account.device.name"); + Services.prefs.setStringPref("services.sync.client.name", "my client name"); + // calling getLocalName() should move the name to the new pref and reset the old. + equal(fxAccounts.device.getLocalName(), "my client name"); + equal( + Services.prefs.getStringPref("identity.fxaccounts.account.device.name"), + "my client name" + ); + ok(!Services.prefs.prefHasUserValue("services.sync.client.name")); + run_next_test(); +}); + +add_test(function test_migration_set_before_get() { + Services.prefs.setStringPref("services.sync.client.name", "old client name"); + fxAccounts.device.setLocalName("new client name"); + equal(fxAccounts.device.getLocalName(), "new client name"); + run_next_test(); +}); + +add_task(async function test_reset() { + // We don't test the client name specifically here because the client name + // is set as part of signing the user in via the attempt to register the + // device. + const testPref = PREF_ACCOUNT_ROOT + "test-pref"; + Services.prefs.setStringPref(testPref, "whatever"); + let credentials = { + email: "foo@example.com", + uid: "1234@lcip.org", + assertion: "foobar", + sessionToken: "dead", + kSync: "beef", + kXCS: "cafe", + kExtSync: "bacon", + kExtKbHash: "cheese", + verified: true, + }; + await fxAccounts.setSignedInUser(credentials); + ok(!Services.prefs.prefHasUserValue(testPref)); + // signing the user out should reset the name pref. + const namePref = PREF_ACCOUNT_ROOT + "device.name"; + ok(Services.prefs.prefHasUserValue(namePref)); + await fxAccounts.signOut(/* remoteOnly = */ true); + ok(!Services.prefs.prefHasUserValue(namePref)); +}); diff --git a/services/fxaccounts/tests/xpcshell/xpcshell.ini b/services/fxaccounts/tests/xpcshell/xpcshell.ini index 2e3cc1ef72a2..1c1b7bc4ee15 100644 --- a/services/fxaccounts/tests/xpcshell/xpcshell.ini +++ b/services/fxaccounts/tests/xpcshell/xpcshell.ini @@ -11,6 +11,7 @@ support-files = [test_client.js] [test_commands.js] [test_credentials.js] +[test_device.js] [test_loginmgr_storage.js] [test_oauth_grant_client.js] [test_oauth_grant_client_server.js] diff --git a/services/sync/modules/engines/clients.js b/services/sync/modules/engines/clients.js index 793ad04a9a8a..433040349294 100644 --- a/services/sync/modules/engines/clients.js +++ b/services/sync/modules/engines/clients.js @@ -234,21 +234,14 @@ ClientEngine.prototype = { }, get localName() { - return Utils.getDeviceName(); + return this.fxAccounts.device.getLocalName(); }, set localName(value) { - Svc.Prefs.set("client.name", value); - // Update the registration in the background. - this.fxAccounts.updateDeviceRegistration().catch(error => { - this._log.warn("failed to update fxa device registration", error); - }); + this.fxAccounts.device.setLocalName(value); }, get localType() { - return Utils.getDeviceType(); - }, - set localType(value) { - Svc.Prefs.set("client.type", value); + return this.fxAccounts.device.getLocalType(); }, getClientName(id) { @@ -363,7 +356,7 @@ ClientEngine.prototype = { this._log.debug("Updating the known stale clients"); // _fetchFxADevices side effect updates this._knownStaleFxADeviceIds. await this._fetchFxADevices(); - let localFxADeviceId = await fxAccounts.getDeviceId(); + let localFxADeviceId = await fxAccounts.device.getLocalId(); // Process newer records first, so that if we hit a record with a device ID // we've seen before, we can mark it stale immediately. let clientList = Object.values(this._store._remoteClients).sort( @@ -452,7 +445,7 @@ ClientEngine.prototype = { await this._removeRemoteClient(id); } } - let localFxADeviceId = await fxAccounts.getDeviceId(); + let localFxADeviceId = await fxAccounts.device.getLocalId(); // Bug 1264498: Mobile clients don't remove themselves from the clients // collection when the user disconnects Sync, so we mark as stale clients // with the same name that haven't synced in over a week. @@ -622,7 +615,7 @@ ClientEngine.prototype = { }; let excludedIds = null; if (!ids) { - const localFxADeviceId = await fxAccounts.getDeviceId(); + const localFxADeviceId = await fxAccounts.device.getLocalId(); excludedIds = [localFxADeviceId]; } try { @@ -1110,7 +1103,7 @@ ClientStore.prototype = { // Package the individual components into a record for the local client if (id == this.engine.localID) { try { - record.fxaDeviceId = await this.engine.fxAccounts.getDeviceId(); + record.fxaDeviceId = await this.engine.fxAccounts.device.getLocalId(); } catch (error) { this._log.warn("failed to get fxa device id", error); } diff --git a/services/sync/modules/util.js b/services/sync/modules/util.js index 1113e52d044b..0f893dd7a1ea 100644 --- a/services/sync/modules/util.js +++ b/services/sync/modules/util.js @@ -47,13 +47,6 @@ XPCOMUtils.defineLazyServiceGetter( "nsILoginManagerCrypto" ); -XPCOMUtils.defineLazyPreferenceGetter( - this, - "localDeviceName", - "services.sync.client.name", - "" -); - XPCOMUtils.defineLazyPreferenceGetter( this, "localDeviceType", @@ -693,68 +686,6 @@ var Utils = { return result; }, - getDefaultDeviceName() { - // Generate a client name if we don't have a useful one yet - let env = Cc["@mozilla.org/process/environment;1"].getService( - Ci.nsIEnvironment - ); - let user = - env.get("USER") || - env.get("USERNAME") || - Svc.Prefs.get("account") || - Svc.Prefs.get("username"); - // A little hack for people using the the moz-build environment on Windows - // which sets USER to the literal "%USERNAME%" (yes, really) - if (user == "%USERNAME%" && env.get("USERNAME")) { - user = env.get("USERNAME"); - } - - let brand = Services.strings.createBundle( - "chrome://branding/locale/brand.properties" - ); - let brandName = brand.GetStringFromName("brandShortName"); - - // The DNS service may fail to provide a hostname in edge-cases we don't - // fully understand - bug 1391488. - let hostname; - try { - // hostname of the system, usually assigned by the user or admin - hostname = Cc["@mozilla.org/network/dns-service;1"].getService( - Ci.nsIDNSService - ).myHostName; - } catch (ex) { - Cu.reportError(ex); - } - let system = - // 'device' is defined on unix systems - Services.sysinfo.get("device") || - hostname || - // fall back on ua info string - Cc["@mozilla.org/network/protocol;1?name=http"].getService( - Ci.nsIHttpProtocolHandler - ).oscpu; - - let syncStrings = Services.strings.createBundle( - "chrome://weave/locale/sync.properties" - ); - return syncStrings.formatStringFromName("client.name2", [ - user, - brandName, - system, - ]); - }, - - getDeviceName() { - let deviceName = localDeviceName; - - if (deviceName === "") { - deviceName = this.getDefaultDeviceName(); - Svc.Prefs.set("client.name", deviceName); - } - - return deviceName; - }, - /** * Helper to implement a more efficient version of fairly common pattern: * diff --git a/services/sync/tests/unit/head_helpers.js b/services/sync/tests/unit/head_helpers.js index abfdcf55ba65..2e0a5b4eb7e9 100644 --- a/services/sync/tests/unit/head_helpers.js +++ b/services/sync/tests/unit/head_helpers.js @@ -509,15 +509,6 @@ function promiseOneObserver(topic, callback) { }); } -// Avoid an issue where `client.name2` containing unicode characters causes -// a number of tests to fail, due to them assuming that we do not need to utf-8 -// encode or decode data sent through the mocked server (see bug 1268912). -// We stash away the original implementation so test_utils_misc.js can test it. -Utils._orig_getDefaultDeviceName = Utils.getDefaultDeviceName; -Utils.getDefaultDeviceName = function() { - return "Test device name"; -}; - async function registerRotaryEngine() { let { RotaryEngine } = ChromeUtils.import( "resource://testing-common/services/sync/rotaryengine.js" diff --git a/services/sync/tests/unit/test_clients_engine.js b/services/sync/tests/unit/test_clients_engine.js index ebc1fb7abd19..4b50bd5b0e68 100644 --- a/services/sync/tests/unit/test_clients_engine.js +++ b/services/sync/tests/unit/test_clients_engine.js @@ -980,8 +980,16 @@ add_task(async function test_clients_not_in_fxa_list() { notifyDevices() { return Promise.resolve(true); }, - getDeviceId() { - return fxAccounts.getDeviceId(); + device: { + getLocalId() { + return fxAccounts.device.getLocalId(); + }, + getLocalName() { + return fxAccounts.device.getLocalName(); + }, + getLocalType() { + return fxAccounts.device.getLocalType(); + }, }, getDeviceList() { return Promise.resolve([{ id: remoteId }]); @@ -1051,8 +1059,16 @@ add_task(async function test_dupe_device_ids() { notifyDevices() { return Promise.resolve(true); }, - getDeviceId() { - return fxAccounts.getDeviceId(); + device: { + getLocalId() { + return fxAccounts.device.getLocalId(); + }, + getLocalName() { + return fxAccounts.device.getLocalName(); + }, + getLocalType() { + return fxAccounts.device.getLocalType(); + }, }, getDeviceList() { return Promise.resolve([{ id: remoteDeviceId }]); @@ -2057,8 +2073,16 @@ add_task(async function test_other_clients_notified_on_first_sync() { const fxAccounts = engine.fxAccounts; let calls = 0; engine.fxAccounts = { - getDeviceId() { - return fxAccounts.getDeviceId(); + device: { + getLocalId() { + return fxAccounts.device.getLocalId(); + }, + getLocalName() { + return fxAccounts.device.getLocalName(); + }, + getLocalType() { + return fxAccounts.device.getLocalType(); + }, }, notifyDevices() { calls++; diff --git a/services/sync/tests/unit/test_utils_misc.js b/services/sync/tests/unit/test_utils_misc.js deleted file mode 100644 index 6e926e2cc2ca..000000000000 --- a/services/sync/tests/unit/test_utils_misc.js +++ /dev/null @@ -1,33 +0,0 @@ -_("Misc tests for utils.js"); - -add_test(function test_default_device_name() { - // Note that head_helpers overrides getDefaultDeviceName - this test is - // really just to ensure the actual implementation is sane - we can't - // really check the value it uses is correct. - // We are just hoping to avoid a repeat of bug 1369285. - let def = Utils._orig_getDefaultDeviceName(); // make sure it doesn't throw. - _("default value is " + def); - ok(def.length > 0); - - // This is obviously tied to the implementation, but we want early warning - // if any of these things fail. - // We really want one of these 2 to provide a value. - let hostname = - Services.sysinfo.get("device") || - Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService) - .myHostName; - _("hostname is " + hostname); - ok(hostname.length > 0); - // the hostname should be in the default. - ok(def.includes(hostname)); - // We expect the following to work as a fallback to the above. - let fallback = Cc["@mozilla.org/network/protocol;1?name=http"].getService( - Ci.nsIHttpProtocolHandler - ).oscpu; - _("UA fallback is " + fallback); - ok(fallback.length > 0); - // the fallback should not be in the default - ok(!def.includes(fallback)); - - run_next_test(); -}); diff --git a/services/sync/tests/unit/xpcshell.ini b/services/sync/tests/unit/xpcshell.ini index e23d91a38bdd..da2b56c778d8 100644 --- a/services/sync/tests/unit/xpcshell.ini +++ b/services/sync/tests/unit/xpcshell.ini @@ -29,7 +29,6 @@ support-files = [test_utils_lock.js] [test_utils_makeGUID.js] run-sequentially = Disproportionately slows down full test run, bug 1450316 -[test_utils_misc.js] [test_utils_notify.js] [test_utils_passphrase.js] From 4f6f7b7336dd020ef20a12788e1f770263c60a17 Mon Sep 17 00:00:00 2001 From: Bogdan Tara Date: Tue, 10 Sep 2019 03:49:53 +0300 Subject: [PATCH 05/24] Backed out changeset 3519c255f86e (bug 1284651) for gtest failures CLOSED TREE --- image/SurfaceCache.cpp | 5 ----- modules/libpref/init/StaticPrefList.yaml | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/image/SurfaceCache.cpp b/image/SurfaceCache.cpp index ae24cde6b82a..babdf45c951a 100644 --- a/image/SurfaceCache.cpp +++ b/image/SurfaceCache.cpp @@ -1391,11 +1391,6 @@ void SurfaceCache::Initialize() { uint64_t surfaceCacheMaxSizeKB = StaticPrefs::image_mem_surfacecache_max_size_kb_AtStartup(); - if (sizeof(uintptr_t) <= 4) { - // Limit surface cache to 1 GB if our address space is 32 bit. - surfaceCacheMaxSizeKB = 1024 * 1024; - } - // A knob determining the actual size of the surface cache. Currently the // cache is (size of main memory) / (surface cache size factor) KB // or (surface cache max size) KB, whichever is smaller. The formula diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index c71cde329164..595a0dd39e21 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -3750,7 +3750,7 @@ # Maximum size for the surface cache, in kilobytes. - name: image.mem.surfacecache.max_size_kb type: uint32_t - value: 2024 * 1024 + value: 1024 * 1024 mirror: once # Minimum timeout for expiring unused images from the surface cache, in From e08eb066c3fddd907b3946febf395f83aff367b3 Mon Sep 17 00:00:00 2001 From: David Major Date: Mon, 9 Sep 2019 23:51:35 +0000 Subject: [PATCH 06/24] Bug 1579502 - Move libxul's Makefile.in to the right place after bug 1573566 r=glandium `hg mv toolkit/library/Makefile.in toolkit/library/build/` to account for https://hg.mozilla.org/integration/autoland/rev/14329e051f16. Otherwise the count-ctors flag and the orderfile flags won't be picked up. Differential Revision: https://phabricator.services.mozilla.com/D45127 --HG-- rename : toolkit/library/Makefile.in => toolkit/library/build/Makefile.in extra : moz-landing-system : lando --- toolkit/library/{ => build}/Makefile.in | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename toolkit/library/{ => build}/Makefile.in (100%) diff --git a/toolkit/library/Makefile.in b/toolkit/library/build/Makefile.in similarity index 100% rename from toolkit/library/Makefile.in rename to toolkit/library/build/Makefile.in From 5af13d7fdd2c622702df4973ba21595bf8463a49 Mon Sep 17 00:00:00 2001 From: Kris Taeleman Date: Tue, 10 Sep 2019 00:32:33 +0000 Subject: [PATCH 07/24] Bug 1571977 - Add wrench test for textureRect. r=gw Differential Revision: https://phabricator.services.mozilla.com/D44925 --HG-- extra : moz-landing-system : lando --- gfx/wr/webrender/src/device/gl.rs | 12 +- gfx/wr/webrender/src/lib.rs | 4 +- gfx/wr/webrender/src/renderer.rs | 1 + gfx/wr/wrench/reftests/image/colorrect.png | Bin 0 -> 256 bytes gfx/wr/wrench/reftests/image/reftest.list | 1 + .../reftests/image/texture-rect-ref.yaml | 15 ++ .../wrench/reftests/image/texture-rect.yaml | 7 + gfx/wr/wrench/src/wrench.rs | 1 + gfx/wr/wrench/src/yaml_frame_reader.rs | 144 ++++++++++++++++-- 9 files changed, 167 insertions(+), 18 deletions(-) create mode 100644 gfx/wr/wrench/reftests/image/colorrect.png create mode 100644 gfx/wr/wrench/reftests/image/texture-rect-ref.yaml create mode 100644 gfx/wr/wrench/reftests/image/texture-rect.yaml diff --git a/gfx/wr/webrender/src/device/gl.rs b/gfx/wr/webrender/src/device/gl.rs index 1019b33f6511..2a88e6e79756 100644 --- a/gfx/wr/webrender/src/device/gl.rs +++ b/gfx/wr/webrender/src/device/gl.rs @@ -3311,7 +3311,7 @@ impl Device { } } - fn gl_describe_format(&self, format: ImageFormat) -> FormatDesc { + pub fn gl_describe_format(&self, format: ImageFormat) -> FormatDesc { match format { ImageFormat::R8 => FormatDesc { internal: gl::R8, @@ -3392,16 +3392,16 @@ impl Device { } } -struct FormatDesc { +pub struct FormatDesc { /// Format the texel data is internally stored in within a texture. - internal: gl::GLenum, + pub internal: gl::GLenum, /// Format that we expect the data to be provided when filling the texture. - external: gl::GLuint, + pub external: gl::GLuint, /// Format to read the texels as, so that they can be uploaded as `external` /// later on. - read: gl::GLuint, + pub read: gl::GLuint, /// Associated pixel type. - pixel_type: gl::GLuint, + pub pixel_type: gl::GLuint, } struct UploadChunk { diff --git a/gfx/wr/webrender/src/lib.rs b/gfx/wr/webrender/src/lib.rs index c7e2b93d0972..5e9934304496 100644 --- a/gfx/wr/webrender/src/lib.rs +++ b/gfx/wr/webrender/src/lib.rs @@ -199,8 +199,8 @@ pub extern crate api; extern crate webrender_build; #[doc(hidden)] -pub use crate::device::{build_shader_strings, UploadMethod, VertexUsageHint}; -pub use crate::device::{ProgramBinary, ProgramCache, ProgramCacheObserver}; +pub use crate::device::{build_shader_strings, UploadMethod, VertexUsageHint, get_gl_target}; +pub use crate::device::{ProgramBinary, ProgramCache, ProgramCacheObserver, FormatDesc}; pub use crate::device::Device; pub use crate::frame_builder::ChasePrimitive; pub use crate::profiler::{ProfilerHooks, set_profiler_hooks}; diff --git a/gfx/wr/webrender/src/renderer.rs b/gfx/wr/webrender/src/renderer.rs index 120576e3cf8b..d3e4b3349056 100644 --- a/gfx/wr/webrender/src/renderer.rs +++ b/gfx/wr/webrender/src/renderer.rs @@ -5939,6 +5939,7 @@ impl Renderer { let mut image_handler = DummyExternalImageHandler { data: FastHashMap::default(), }; + // Note: this is a `SCENE` level population of the external image handlers // It would put both external buffers and texture into the map. // But latter are going to be overwritten later in this function diff --git a/gfx/wr/wrench/reftests/image/colorrect.png b/gfx/wr/wrench/reftests/image/colorrect.png new file mode 100644 index 0000000000000000000000000000000000000000..75283ee1f13cac6235ba879f537cc8d098009eec GIT binary patch literal 256 zcmeAS@N?(olHy`uVBq!ia0vp^DIm2xNn<$k-o@FZ;D3JEM$gBw z>P@WNVmc8USiocgCy1SJPzX#mse;%|tlnUfdm4xhR0bgEVDCzU{_GI+ZBxvX, +} + +impl LocalExternalImageHandler { + pub fn new() -> LocalExternalImageHandler { + LocalExternalImageHandler { + texture_ids: Vec::new(), + } + } + + fn init_gl_texture( + id: gl::GLuint, + gl_target: gl::GLuint, + format_desc: webrender::FormatDesc, + width: gl::GLint, + height: gl::GLint, + bytes: &[u8], + gl: &dyn gl::Gl, + ) { + gl.bind_texture(gl_target, id); + gl.tex_parameter_i(gl_target, gl::TEXTURE_MAG_FILTER, gl::LINEAR as gl::GLint); + gl.tex_parameter_i(gl_target, gl::TEXTURE_MIN_FILTER, gl::LINEAR as gl::GLint); + gl.tex_parameter_i(gl_target, gl::TEXTURE_WRAP_S, gl::CLAMP_TO_EDGE as gl::GLint); + gl.tex_parameter_i(gl_target, gl::TEXTURE_WRAP_T, gl::CLAMP_TO_EDGE as gl::GLint); + gl.tex_image_2d( + gl_target, + 0, + format_desc.internal as gl::GLint, + width, + height, + 0, + format_desc.external, + format_desc.pixel_type, + Some(bytes), + ); + gl.bind_texture(gl_target, 0); + } + + pub fn add_image(&mut self, + device: &webrender::Device, + desc: ImageDescriptor, + target: TextureTarget, + image_data: ImageData, + ) -> ImageData { + + let (image_id, channel_idx) = match image_data { + ImageData::Raw(ref data) => { + let gl = device.gl(); + let texture_ids = gl.gen_textures(1); + let format_desc = device.gl_describe_format(desc.format); + + LocalExternalImageHandler::init_gl_texture( + texture_ids[0], + webrender::get_gl_target(target), + format_desc, + desc.size.width as gl::GLint, + desc.size.height as gl::GLint, + &data, + gl, + ); + self.texture_ids.push((texture_ids[0], desc)); + (ExternalImageId((self.texture_ids.len() - 1) as u64), 0) + }, + _ => panic!("unsupported!"), + }; + + ImageData::External( + ExternalImageData { + id: image_id, + channel_index: channel_idx, + image_type: ExternalImageType::TextureHandle(target) + } + ) + } +} + +impl webrender::ExternalImageHandler for LocalExternalImageHandler { + fn lock(&mut self, key: ExternalImageId, _channel_index: u8, _rendering: ImageRendering) -> webrender::ExternalImage { + let (id, desc) = self.texture_ids[key.0 as usize]; + webrender::ExternalImage { + uv: TexelRect::new(0.0, 0.0, desc.size.width as f32, desc.size.height as f32), + source: webrender::ExternalImageSource::NativeTexture(id), + } + } + fn unlock(&mut self, _key: ExternalImageId, _channel_index: u8) {} +} + fn broadcast(base_vals: &[T], num_items: usize) -> Vec { if base_vals.len() == num_items { return base_vals.to_vec(); @@ -246,6 +335,8 @@ pub struct YamlFrameReader { yaml_string: String, keyframes: Option, + + external_image_handler: Option>, } impl YamlFrameReader { @@ -272,6 +363,7 @@ impl YamlFrameReader { requested_frame: 0, built_frame: usize::MAX, keyframes: None, + external_image_handler: Some(Box::new(LocalExternalImageHandler::new())), } } @@ -372,6 +464,8 @@ impl YamlFrameReader { self.add_stacking_context_from_yaml(&mut builder, wrench, yaml, true, &mut info); self.display_lists.push(builder.finalize()); + wrench.renderer.set_external_image_handler(self.external_image_handler.take().unwrap()); + assert_eq!(self.clip_id_stack.len(), 1); assert_eq!(self.spatial_id_stack.len(), 1); } @@ -511,6 +605,7 @@ impl YamlFrameReader { &mut self, file: &Path, tiling: Option, + item: &Yaml, wrench: &mut Wrench, ) -> (ImageKey, LayoutSize) { let key = (file.to_owned(), tiling); @@ -625,7 +720,36 @@ impl YamlFrameReader { let tiling = tiling.map(|tile_size| tile_size as u16); let image_key = wrench.api.generate_image_key(); let mut txn = Transaction::new(); - txn.add_image(image_key, descriptor, image_data, tiling); + + let external = item["external"].as_bool().unwrap_or(false); + if external { + // This indicates we want to simulate an external texture, + // ensure it gets created as such + let external_target = match item["external-target"].as_str() { + Some(ref s) => match &s[..] { + "2d" => TextureTarget::Default, + "array" => TextureTarget::Array, + "rect" => TextureTarget::Rect, + _ => panic!("Unsupported external texture target."), + } + None => { + TextureTarget::Default + } + }; + + let external_image_data = + self.external_image_handler.as_mut().unwrap().add_image( + &wrench.renderer.device, + descriptor, + external_target, + image_data + ); + txn.add_image(image_key, descriptor, external_image_data, tiling); + } + else { + txn.add_image(image_key, descriptor, image_data, tiling); + } + wrench.api.update_resources(txn.resource_updates); let val = ( image_key, @@ -708,7 +832,7 @@ impl YamlFrameReader { } else { let mut file = self.aux_dir.clone(); file.push(filename); - self.add_or_get_image(&file, tiling, wrench) + self.add_or_get_image(&file, tiling, item, wrench) } } None => { @@ -1068,7 +1192,7 @@ impl YamlFrameReader { "image" => { let file = rsrc_path(&item["image-source"], &self.aux_dir); let (image_key, _) = self - .add_or_get_image(&file, None, wrench); + .add_or_get_image(&file, None, item, wrench); NinePatchBorderSource::Image(image_key) } "gradient" => { @@ -1169,28 +1293,28 @@ impl YamlFrameReader { let yuv_data = match item["format"].as_str().expect("no format supplied") { "planar" => { let y_path = rsrc_path(&item["src-y"], &self.aux_dir); - let (y_key, _) = self.add_or_get_image(&y_path, None, wrench); + let (y_key, _) = self.add_or_get_image(&y_path, None, item, wrench); let u_path = rsrc_path(&item["src-u"], &self.aux_dir); - let (u_key, _) = self.add_or_get_image(&u_path, None, wrench); + let (u_key, _) = self.add_or_get_image(&u_path, None, item, wrench); let v_path = rsrc_path(&item["src-v"], &self.aux_dir); - let (v_key, _) = self.add_or_get_image(&v_path, None, wrench); + let (v_key, _) = self.add_or_get_image(&v_path, None, item, wrench); YuvData::PlanarYCbCr(y_key, u_key, v_key) } "nv12" => { let y_path = rsrc_path(&item["src-y"], &self.aux_dir); - let (y_key, _) = self.add_or_get_image(&y_path, None, wrench); + let (y_key, _) = self.add_or_get_image(&y_path, None, item, wrench); let uv_path = rsrc_path(&item["src-uv"], &self.aux_dir); - let (uv_key, _) = self.add_or_get_image(&uv_path, None, wrench); + let (uv_key, _) = self.add_or_get_image(&uv_path, None, item, wrench); YuvData::NV12(y_key, uv_key) } "interleaved" => { let yuv_path = rsrc_path(&item["src"], &self.aux_dir); - let (yuv_key, _) = self.add_or_get_image(&yuv_path, None, wrench); + let (yuv_key, _) = self.add_or_get_image(&yuv_path, None, item, wrench); YuvData::InterleavedYCbCr(yuv_key) } @@ -1231,7 +1355,7 @@ impl YamlFrameReader { let tiling = item["tile-size"].as_i64(); let file = rsrc_path(filename, &self.aux_dir); let (image_key, image_dims) = - self.add_or_get_image(&file, tiling, wrench); + self.add_or_get_image(&file, tiling, item, wrench); let bounds_raws = item["bounds"].as_vec_f32().unwrap(); let bounds = if bounds_raws.len() == 2 { From 706a9fa84c8b470758d66ca270554b01aa7aa0a7 Mon Sep 17 00:00:00 2001 From: Nathan Froyd Date: Mon, 9 Sep 2019 20:33:55 +0000 Subject: [PATCH 08/24] Bug 1579873 - use the aarch64 cross toolchain for aarch64-linux builds; r=nalexander The aarch64 cross toolchain is unused otherwise. The aarch64-linux builds also exist for the express purpose of eventually standing up some kind of fuzzing/ccov build, so we might as well start using a toolchain that supports those use cases. Differential Revision: https://phabricator.services.mozilla.com/D45210 --HG-- extra : moz-landing-system : lando --- taskcluster/ci/build/linux.yml | 2 +- taskcluster/ci/toolchain/clang.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/taskcluster/ci/build/linux.yml b/taskcluster/ci/build/linux.yml index 4c82f2cf68b3..f677b4c66541 100644 --- a/taskcluster/ci/build/linux.yml +++ b/taskcluster/ci/build/linux.yml @@ -1171,7 +1171,7 @@ linux64-aarch64/opt: fetches: toolchain: - linux64-binutils - - linux64-clang + - linux64-clang-aarch64-cross - linux64-rust - linux64-rust-size - linux64-cbindgen diff --git a/taskcluster/ci/toolchain/clang.yml b/taskcluster/ci/toolchain/clang.yml index 81ac5eb7665e..215dc99d183b 100644 --- a/taskcluster/ci/toolchain/clang.yml +++ b/taskcluster/ci/toolchain/clang.yml @@ -145,7 +145,7 @@ linux64-clang-8-aarch64-cross: - 'build/build-clang/clang-8-linux64-aarch64-cross.json' resources: - 'build/build-clang/clang-8-linux64-aarch64-cross.json' - toolchain-alias: linux64-aarch64-cross + toolchain-alias: linux64-clang-aarch64-cross toolchain-artifact: public/build/clang.tar.xz fetches: fetch: From 1735c8570e76914d700b01b291e8411f7eff4cb7 Mon Sep 17 00:00:00 2001 From: Bogdan Tara Date: Tue, 10 Sep 2019 04:23:19 +0300 Subject: [PATCH 09/24] Backed out changeset bde7561bd171 (bug 1571977) for wrench amd tidy failures CLOSED TREE --- gfx/wr/webrender/src/device/gl.rs | 12 +- gfx/wr/webrender/src/lib.rs | 4 +- gfx/wr/webrender/src/renderer.rs | 1 - gfx/wr/wrench/reftests/image/colorrect.png | Bin 256 -> 0 bytes gfx/wr/wrench/reftests/image/reftest.list | 1 - .../reftests/image/texture-rect-ref.yaml | 15 -- .../wrench/reftests/image/texture-rect.yaml | 7 - gfx/wr/wrench/src/wrench.rs | 1 - gfx/wr/wrench/src/yaml_frame_reader.rs | 144 ++---------------- 9 files changed, 18 insertions(+), 167 deletions(-) delete mode 100644 gfx/wr/wrench/reftests/image/colorrect.png delete mode 100644 gfx/wr/wrench/reftests/image/texture-rect-ref.yaml delete mode 100644 gfx/wr/wrench/reftests/image/texture-rect.yaml diff --git a/gfx/wr/webrender/src/device/gl.rs b/gfx/wr/webrender/src/device/gl.rs index 2a88e6e79756..1019b33f6511 100644 --- a/gfx/wr/webrender/src/device/gl.rs +++ b/gfx/wr/webrender/src/device/gl.rs @@ -3311,7 +3311,7 @@ impl Device { } } - pub fn gl_describe_format(&self, format: ImageFormat) -> FormatDesc { + fn gl_describe_format(&self, format: ImageFormat) -> FormatDesc { match format { ImageFormat::R8 => FormatDesc { internal: gl::R8, @@ -3392,16 +3392,16 @@ impl Device { } } -pub struct FormatDesc { +struct FormatDesc { /// Format the texel data is internally stored in within a texture. - pub internal: gl::GLenum, + internal: gl::GLenum, /// Format that we expect the data to be provided when filling the texture. - pub external: gl::GLuint, + external: gl::GLuint, /// Format to read the texels as, so that they can be uploaded as `external` /// later on. - pub read: gl::GLuint, + read: gl::GLuint, /// Associated pixel type. - pub pixel_type: gl::GLuint, + pixel_type: gl::GLuint, } struct UploadChunk { diff --git a/gfx/wr/webrender/src/lib.rs b/gfx/wr/webrender/src/lib.rs index 5e9934304496..c7e2b93d0972 100644 --- a/gfx/wr/webrender/src/lib.rs +++ b/gfx/wr/webrender/src/lib.rs @@ -199,8 +199,8 @@ pub extern crate api; extern crate webrender_build; #[doc(hidden)] -pub use crate::device::{build_shader_strings, UploadMethod, VertexUsageHint, get_gl_target}; -pub use crate::device::{ProgramBinary, ProgramCache, ProgramCacheObserver, FormatDesc}; +pub use crate::device::{build_shader_strings, UploadMethod, VertexUsageHint}; +pub use crate::device::{ProgramBinary, ProgramCache, ProgramCacheObserver}; pub use crate::device::Device; pub use crate::frame_builder::ChasePrimitive; pub use crate::profiler::{ProfilerHooks, set_profiler_hooks}; diff --git a/gfx/wr/webrender/src/renderer.rs b/gfx/wr/webrender/src/renderer.rs index d3e4b3349056..120576e3cf8b 100644 --- a/gfx/wr/webrender/src/renderer.rs +++ b/gfx/wr/webrender/src/renderer.rs @@ -5939,7 +5939,6 @@ impl Renderer { let mut image_handler = DummyExternalImageHandler { data: FastHashMap::default(), }; - // Note: this is a `SCENE` level population of the external image handlers // It would put both external buffers and texture into the map. // But latter are going to be overwritten later in this function diff --git a/gfx/wr/wrench/reftests/image/colorrect.png b/gfx/wr/wrench/reftests/image/colorrect.png deleted file mode 100644 index 75283ee1f13cac6235ba879f537cc8d098009eec..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 256 zcmeAS@N?(olHy`uVBq!ia0vp^DIm2xNn<$k-o@FZ;D3JEM$gBw z>P@WNVmc8USiocgCy1SJPzX#mse;%|tlnUfdm4xhR0bgEVDCzU{_GI+ZBxvX, -} - -impl LocalExternalImageHandler { - pub fn new() -> LocalExternalImageHandler { - LocalExternalImageHandler { - texture_ids: Vec::new(), - } - } - - fn init_gl_texture( - id: gl::GLuint, - gl_target: gl::GLuint, - format_desc: webrender::FormatDesc, - width: gl::GLint, - height: gl::GLint, - bytes: &[u8], - gl: &dyn gl::Gl, - ) { - gl.bind_texture(gl_target, id); - gl.tex_parameter_i(gl_target, gl::TEXTURE_MAG_FILTER, gl::LINEAR as gl::GLint); - gl.tex_parameter_i(gl_target, gl::TEXTURE_MIN_FILTER, gl::LINEAR as gl::GLint); - gl.tex_parameter_i(gl_target, gl::TEXTURE_WRAP_S, gl::CLAMP_TO_EDGE as gl::GLint); - gl.tex_parameter_i(gl_target, gl::TEXTURE_WRAP_T, gl::CLAMP_TO_EDGE as gl::GLint); - gl.tex_image_2d( - gl_target, - 0, - format_desc.internal as gl::GLint, - width, - height, - 0, - format_desc.external, - format_desc.pixel_type, - Some(bytes), - ); - gl.bind_texture(gl_target, 0); - } - - pub fn add_image(&mut self, - device: &webrender::Device, - desc: ImageDescriptor, - target: TextureTarget, - image_data: ImageData, - ) -> ImageData { - - let (image_id, channel_idx) = match image_data { - ImageData::Raw(ref data) => { - let gl = device.gl(); - let texture_ids = gl.gen_textures(1); - let format_desc = device.gl_describe_format(desc.format); - - LocalExternalImageHandler::init_gl_texture( - texture_ids[0], - webrender::get_gl_target(target), - format_desc, - desc.size.width as gl::GLint, - desc.size.height as gl::GLint, - &data, - gl, - ); - self.texture_ids.push((texture_ids[0], desc)); - (ExternalImageId((self.texture_ids.len() - 1) as u64), 0) - }, - _ => panic!("unsupported!"), - }; - - ImageData::External( - ExternalImageData { - id: image_id, - channel_index: channel_idx, - image_type: ExternalImageType::TextureHandle(target) - } - ) - } -} - -impl webrender::ExternalImageHandler for LocalExternalImageHandler { - fn lock(&mut self, key: ExternalImageId, _channel_index: u8, _rendering: ImageRendering) -> webrender::ExternalImage { - let (id, desc) = self.texture_ids[key.0 as usize]; - webrender::ExternalImage { - uv: TexelRect::new(0.0, 0.0, desc.size.width as f32, desc.size.height as f32), - source: webrender::ExternalImageSource::NativeTexture(id), - } - } - fn unlock(&mut self, _key: ExternalImageId, _channel_index: u8) {} -} - fn broadcast(base_vals: &[T], num_items: usize) -> Vec { if base_vals.len() == num_items { return base_vals.to_vec(); @@ -335,8 +246,6 @@ pub struct YamlFrameReader { yaml_string: String, keyframes: Option, - - external_image_handler: Option>, } impl YamlFrameReader { @@ -363,7 +272,6 @@ impl YamlFrameReader { requested_frame: 0, built_frame: usize::MAX, keyframes: None, - external_image_handler: Some(Box::new(LocalExternalImageHandler::new())), } } @@ -464,8 +372,6 @@ impl YamlFrameReader { self.add_stacking_context_from_yaml(&mut builder, wrench, yaml, true, &mut info); self.display_lists.push(builder.finalize()); - wrench.renderer.set_external_image_handler(self.external_image_handler.take().unwrap()); - assert_eq!(self.clip_id_stack.len(), 1); assert_eq!(self.spatial_id_stack.len(), 1); } @@ -605,7 +511,6 @@ impl YamlFrameReader { &mut self, file: &Path, tiling: Option, - item: &Yaml, wrench: &mut Wrench, ) -> (ImageKey, LayoutSize) { let key = (file.to_owned(), tiling); @@ -720,36 +625,7 @@ impl YamlFrameReader { let tiling = tiling.map(|tile_size| tile_size as u16); let image_key = wrench.api.generate_image_key(); let mut txn = Transaction::new(); - - let external = item["external"].as_bool().unwrap_or(false); - if external { - // This indicates we want to simulate an external texture, - // ensure it gets created as such - let external_target = match item["external-target"].as_str() { - Some(ref s) => match &s[..] { - "2d" => TextureTarget::Default, - "array" => TextureTarget::Array, - "rect" => TextureTarget::Rect, - _ => panic!("Unsupported external texture target."), - } - None => { - TextureTarget::Default - } - }; - - let external_image_data = - self.external_image_handler.as_mut().unwrap().add_image( - &wrench.renderer.device, - descriptor, - external_target, - image_data - ); - txn.add_image(image_key, descriptor, external_image_data, tiling); - } - else { - txn.add_image(image_key, descriptor, image_data, tiling); - } - + txn.add_image(image_key, descriptor, image_data, tiling); wrench.api.update_resources(txn.resource_updates); let val = ( image_key, @@ -832,7 +708,7 @@ impl YamlFrameReader { } else { let mut file = self.aux_dir.clone(); file.push(filename); - self.add_or_get_image(&file, tiling, item, wrench) + self.add_or_get_image(&file, tiling, wrench) } } None => { @@ -1192,7 +1068,7 @@ impl YamlFrameReader { "image" => { let file = rsrc_path(&item["image-source"], &self.aux_dir); let (image_key, _) = self - .add_or_get_image(&file, None, item, wrench); + .add_or_get_image(&file, None, wrench); NinePatchBorderSource::Image(image_key) } "gradient" => { @@ -1293,28 +1169,28 @@ impl YamlFrameReader { let yuv_data = match item["format"].as_str().expect("no format supplied") { "planar" => { let y_path = rsrc_path(&item["src-y"], &self.aux_dir); - let (y_key, _) = self.add_or_get_image(&y_path, None, item, wrench); + let (y_key, _) = self.add_or_get_image(&y_path, None, wrench); let u_path = rsrc_path(&item["src-u"], &self.aux_dir); - let (u_key, _) = self.add_or_get_image(&u_path, None, item, wrench); + let (u_key, _) = self.add_or_get_image(&u_path, None, wrench); let v_path = rsrc_path(&item["src-v"], &self.aux_dir); - let (v_key, _) = self.add_or_get_image(&v_path, None, item, wrench); + let (v_key, _) = self.add_or_get_image(&v_path, None, wrench); YuvData::PlanarYCbCr(y_key, u_key, v_key) } "nv12" => { let y_path = rsrc_path(&item["src-y"], &self.aux_dir); - let (y_key, _) = self.add_or_get_image(&y_path, None, item, wrench); + let (y_key, _) = self.add_or_get_image(&y_path, None, wrench); let uv_path = rsrc_path(&item["src-uv"], &self.aux_dir); - let (uv_key, _) = self.add_or_get_image(&uv_path, None, item, wrench); + let (uv_key, _) = self.add_or_get_image(&uv_path, None, wrench); YuvData::NV12(y_key, uv_key) } "interleaved" => { let yuv_path = rsrc_path(&item["src"], &self.aux_dir); - let (yuv_key, _) = self.add_or_get_image(&yuv_path, None, item, wrench); + let (yuv_key, _) = self.add_or_get_image(&yuv_path, None, wrench); YuvData::InterleavedYCbCr(yuv_key) } @@ -1355,7 +1231,7 @@ impl YamlFrameReader { let tiling = item["tile-size"].as_i64(); let file = rsrc_path(filename, &self.aux_dir); let (image_key, image_dims) = - self.add_or_get_image(&file, tiling, item, wrench); + self.add_or_get_image(&file, tiling, wrench); let bounds_raws = item["bounds"].as_vec_f32().unwrap(); let bounds = if bounds_raws.len() == 2 { From 0f5e9e5206a46749655738ea3b6ed4f5ee8f6eef Mon Sep 17 00:00:00 2001 From: Bogdan Tara Date: Tue, 10 Sep 2019 04:42:15 +0300 Subject: [PATCH 10/24] Backed out changeset 93d2d4bbe263 (bug 1575921) for xpcshell failures on test_device.js CLOSED TREE --- .../components/preferences/in-content/sync.js | 5 +- services/fxaccounts/FxAccounts.jsm | 66 +++++--- services/fxaccounts/FxAccountsCommon.js | 4 - services/fxaccounts/FxAccountsDevice.jsm | 150 ------------------ services/fxaccounts/moz.build | 1 - .../test_accounts_device_registration.js | 81 +++++++--- .../fxaccounts/tests/xpcshell/test_device.js | 92 ----------- .../fxaccounts/tests/xpcshell/xpcshell.ini | 1 - services/sync/modules/engines/clients.js | 21 ++- services/sync/modules/util.js | 69 ++++++++ services/sync/tests/unit/head_helpers.js | 9 ++ .../sync/tests/unit/test_clients_engine.js | 36 +---- services/sync/tests/unit/test_utils_misc.js | 33 ++++ services/sync/tests/unit/xpcshell.ini | 1 + 14 files changed, 233 insertions(+), 336 deletions(-) delete mode 100644 services/fxaccounts/FxAccountsDevice.jsm delete mode 100644 services/fxaccounts/tests/xpcshell/test_device.js create mode 100644 services/sync/tests/unit/test_utils_misc.js diff --git a/browser/components/preferences/in-content/sync.js b/browser/components/preferences/in-content/sync.js index 1beba1cbc2dd..9741e43e8cff 100644 --- a/browser/components/preferences/in-content/sync.js +++ b/browser/components/preferences/in-content/sync.js @@ -568,10 +568,7 @@ var gSyncPane = { _populateComputerName(value) { let textbox = document.getElementById("fxaSyncComputerName"); if (!textbox.hasAttribute("placeholder")) { - textbox.setAttribute( - "placeholder", - fxAccounts.device.getDefaultLocalName() - ); + textbox.setAttribute("placeholder", Weave.Utils.getDefaultDeviceName()); } textbox.value = value; }, diff --git a/services/fxaccounts/FxAccounts.jsm b/services/fxaccounts/FxAccounts.jsm index ef139fc4866c..b532f2b6f2e2 100644 --- a/services/fxaccounts/FxAccounts.jsm +++ b/services/fxaccounts/FxAccounts.jsm @@ -50,7 +50,6 @@ const { ON_DEVICE_DISCONNECTED_NOTIFICATION, ON_NEW_DEVICE_ID, POLL_SESSION, - PREF_ACCOUNT_ROOT, PREF_LAST_FXA_USER, SERVER_ERRNO_TO_ERROR, SCOPE_OLD_SYNC, @@ -88,21 +87,17 @@ ChromeUtils.defineModuleGetter( "resource://gre/modules/FxAccountsCommands.js" ); -ChromeUtils.defineModuleGetter( - this, - "FxAccountsDevice", - "resource://gre/modules/FxAccountsDevice.jsm" -); - ChromeUtils.defineModuleGetter( this, "FxAccountsProfile", "resource://gre/modules/FxAccountsProfile.jsm" ); -XPCOMUtils.defineLazyModuleGetters(this, { - Preferences: "resource://gre/modules/Preferences.jsm", -}); +ChromeUtils.defineModuleGetter( + this, + "Utils", + "resource://services-sync/util.js" +); XPCOMUtils.defineLazyPreferenceGetter( this, @@ -118,9 +113,9 @@ var publicProperties = [ "canGetKeys", "checkVerificationStatus", "commands", - "device", "getAccountsClient", "getAssertion", + "getDeviceId", "getDeviceList", "getKeys", "authorizeOAuthCode", @@ -523,14 +518,6 @@ FxAccountsInternal.prototype = { return this._commands; }, - _device: null, - get device() { - if (!this._device) { - this._device = new FxAccountsDevice(this); - } - return this._device; - }, - _oauthClient: null, get oauthClient() { if (!this._oauthClient) { @@ -736,7 +723,6 @@ FxAccountsInternal.prototype = { if (!FXA_ENABLED) { throw new Error("Cannot call setSignedInUser when FxA is disabled."); } - Preferences.resetBranch(PREF_ACCOUNT_ROOT); log.debug("setSignedInUser - aborting any existing flows"); const signedInUser = await this.getSignedInUser(); if (signedInUser) { @@ -855,6 +841,33 @@ FxAccountsInternal.prototype = { return this.currentAccountState.updateUserAccountData({ cert: null }); }, + async getDeviceId() { + let data = await this.currentAccountState.getUserAccountData(); + if (!data) { + // Without a signed-in user, there can be no device id. + return null; + } + // Try migrating first. Remove this in Firefox 65+. + if (data.deviceId) { + log.info("Migrating from deviceId to device."); + await this.currentAccountState.updateUserAccountData({ + deviceId: null, + deviceRegistrationVersion: null, + device: { + id: data.deviceId, + registrationVersion: data.deviceRegistrationVersion, + }, + }); + data = await this.currentAccountState.getUserAccountData(); + } + const { device } = data; + if (await this.checkDeviceUpdateNeeded(device)) { + return this._registerOrUpdateDevice(data); + } + // Return the device id that we already registered with the server. + return device.id; + }, + async checkDeviceUpdateNeeded(device) { // There is no device registered or the device registration is outdated. // Either way, we should register the device with FxA @@ -1009,7 +1022,6 @@ FxAccountsInternal.prototype = { }, async _signOutLocal() { - Preferences.resetBranch(PREF_ACCOUNT_ROOT); await this.currentAccountState.signOut(); // this "aborts" this.currentAccountState but doesn't make a new one. await this.abortExistingFlow(); @@ -2071,7 +2083,7 @@ FxAccountsInternal.prototype = { try { const subscription = await this.fxaPushService.registerPushEndpoint(); - const deviceName = this.device.getLocalName(); + const deviceName = this._getDeviceName(); let deviceOptions = {}; // if we were able to obtain a subscription @@ -2103,7 +2115,7 @@ FxAccountsInternal.prototype = { device = await this.fxAccountsClient.registerDevice( sessionToken, deviceName, - this.device.getLocalType(), + this._getDeviceType(), deviceOptions ); Services.obs.notifyObservers(null, ON_NEW_DEVICE_ID); @@ -2125,6 +2137,14 @@ FxAccountsInternal.prototype = { } }, + _getDeviceName() { + return Utils.getDeviceName(); + }, + + _getDeviceType() { + return Utils.getDeviceType(); + }, + _handleDeviceError(error, sessionToken) { return Promise.resolve() .then(() => { diff --git a/services/fxaccounts/FxAccountsCommon.js b/services/fxaccounts/FxAccountsCommon.js index a43c3e9c36e2..19f520a3b9ad 100644 --- a/services/fxaccounts/FxAccountsCommon.js +++ b/services/fxaccounts/FxAccountsCommon.js @@ -109,10 +109,6 @@ exports.COMMAND_CHANGE_PASSWORD = "fxaccounts:change_password"; exports.COMMAND_FXA_STATUS = "fxaccounts:fxa_status"; exports.COMMAND_PAIR_PREFERENCES = "fxaccounts:pair_preferences"; -// The pref branch where any prefs which relate to a specific account should -// be stored. This branch will be reset on account signout and signin. -exports.PREF_ACCOUNT_ROOT = "identity.fxaccounts.account."; - exports.PREF_LAST_FXA_USER = "identity.fxaccounts.lastSignedInUserHash"; exports.PREF_REMOTE_PAIRING_URI = "identity.fxaccounts.remote.pairing.uri"; diff --git a/services/fxaccounts/FxAccountsDevice.jsm b/services/fxaccounts/FxAccountsDevice.jsm deleted file mode 100644 index 9eb4d61ca9cc..000000000000 --- a/services/fxaccounts/FxAccountsDevice.jsm +++ /dev/null @@ -1,150 +0,0 @@ -/* 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"; - -const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm"); - -const { XPCOMUtils } = ChromeUtils.import( - "resource://gre/modules/XPCOMUtils.jsm" -); - -const { log } = ChromeUtils.import( - "resource://gre/modules/FxAccountsCommon.js" -); - -const { DEVICE_TYPE_DESKTOP } = ChromeUtils.import( - "resource://services-sync/constants.js" -); - -const { PREF_ACCOUNT_ROOT } = ChromeUtils.import( - "resource://gre/modules/FxAccountsCommon.js" -); - -const PREF_LOCAL_DEVICE_NAME = PREF_ACCOUNT_ROOT + "device.name"; -XPCOMUtils.defineLazyPreferenceGetter( - this, - "pref_localDeviceName", - PREF_LOCAL_DEVICE_NAME, - "" -); - -const PREF_DEPRECATED_DEVICE_NAME = "services.sync.client.name"; - -// Everything to do with FxA devices. -// TODO: Move more device stuff from FxAccounts.jsm into here - eg, device -// registration, device lists, etc. -class FxAccountsDevice { - constructor(fxa) { - this._fxa = fxa; - } - - async getLocalId() { - let data = await this._fxa.currentAccountState.getUserAccountData(); - if (!data) { - // Without a signed-in user, there can be no device id. - return null; - } - const { device } = data; - if (await this._fxa.checkDeviceUpdateNeeded(device)) { - return this._fxa._registerOrUpdateDevice(data); - } - // Return the device id that we already registered with the server. - return device.id; - } - - // Generate a client name if we don't have a useful one yet - getDefaultLocalName() { - let env = Cc["@mozilla.org/process/environment;1"].getService( - Ci.nsIEnvironment - ); - let user = env.get("USER") || env.get("USERNAME"); - // Note that we used to fall back to the "services.sync.username" pref here, - // but that's no longer suitable in a world where sync might not be - // configured. However, we almost never *actually* fell back to that, and - // doing so sanely here would mean making this function async, which we don't - // really want to do yet. - - // A little hack for people using the the moz-build environment on Windows - // which sets USER to the literal "%USERNAME%" (yes, really) - if (user == "%USERNAME%" && env.get("USERNAME")) { - user = env.get("USERNAME"); - } - - let brand = Services.strings.createBundle( - "chrome://branding/locale/brand.properties" - ); - let brandName; - try { - brandName = brand.GetStringFromName("brandShortName"); - } catch (O_o) { - // this only fails in tests and markh can't work out why :( - brandName = Services.appinfo.name; - } - - // The DNS service may fail to provide a hostname in edge-cases we don't - // fully understand - bug 1391488. - let hostname; - try { - // hostname of the system, usually assigned by the user or admin - hostname = Cc["@mozilla.org/network/dns-service;1"].getService( - Ci.nsIDNSService - ).myHostName; - } catch (ex) { - Cu.reportError(ex); - } - let system = - // 'device' is defined on unix systems - Services.sysinfo.get("device") || - hostname || - // fall back on ua info string - Cc["@mozilla.org/network/protocol;1?name=http"].getService( - Ci.nsIHttpProtocolHandler - ).oscpu; - - // It's a little unfortunate that this string is defined as being weave/sync, - // but it's not worth moving it. - let syncStrings = Services.strings.createBundle( - "chrome://weave/locale/sync.properties" - ); - return syncStrings.formatStringFromName("client.name2", [ - user, - brandName, - system, - ]); - } - - getLocalName() { - // We used to store this in services.sync.client.name, but now store it - // under an fxa-specific location. - let deprecated_value = Services.prefs.getStringPref( - PREF_DEPRECATED_DEVICE_NAME, - "" - ); - if (deprecated_value) { - Services.prefs.setStringPref(PREF_LOCAL_DEVICE_NAME, deprecated_value); - Services.prefs.clearUserPref(PREF_DEPRECATED_DEVICE_NAME); - } - let name = pref_localDeviceName; - if (!name) { - name = this.getDefaultLocalName(); - Services.prefs.setStringPref(PREF_LOCAL_DEVICE_NAME, name); - } - return name; - } - - setLocalName(newName) { - Services.prefs.clearUserPref(PREF_DEPRECATED_DEVICE_NAME); - Services.prefs.setStringPref(PREF_LOCAL_DEVICE_NAME, newName); - // Update the registration in the background. - this._fxa.updateDeviceRegistration().catch(error => { - log.warn("failed to update fxa device registration", error); - }); - } - - getLocalType() { - return DEVICE_TYPE_DESKTOP; - } -} - -var EXPORTED_SYMBOLS = ["FxAccountsDevice"]; diff --git a/services/fxaccounts/moz.build b/services/fxaccounts/moz.build index 398bd715827a..530eb2f9bcb3 100644 --- a/services/fxaccounts/moz.build +++ b/services/fxaccounts/moz.build @@ -22,7 +22,6 @@ EXTRA_JS_MODULES += [ 'FxAccountsCommands.js', 'FxAccountsCommon.js', 'FxAccountsConfig.jsm', - 'FxAccountsDevice.jsm', 'FxAccountsOAuthGrantClient.jsm', 'FxAccountsPairing.jsm', 'FxAccountsPairingChannel.js', diff --git a/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js b/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js index 9369a8bb995d..d9246609fb76 100644 --- a/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js +++ b/services/fxaccounts/tests/xpcshell/test_accounts_device_registration.js @@ -111,14 +111,17 @@ MockFxAccountsClient.prototype = { __proto__: FxAccountsClient.prototype, }; -async function MockFxAccounts(credentials, device = {}) { - let fxa = new FxAccounts({ - newAccountState(creds) { +function MockFxAccounts(device = {}) { + return new FxAccounts({ + newAccountState(credentials) { // we use a real accountState but mocked storage. let storage = new MockStorageManager(); - storage.initialize(creds); + storage.initialize(credentials); return new AccountState(storage); }, + _getDeviceName() { + return device.name || "mock device name"; + }, async availableCommands() { return {}; }, @@ -143,12 +146,6 @@ async function MockFxAccounts(credentials, device = {}) { }, DEVICE_REGISTRATION_VERSION, }); - await fxa.internal.setSignedInUser(credentials); - Services.prefs.setStringPref( - "identity.fxaccounts.account.device.name", - device.name || "mock device name" - ); - return fxa; } add_task(async function test_updateDeviceRegistration_with_new_device() { @@ -156,7 +153,8 @@ add_task(async function test_updateDeviceRegistration_with_new_device() { const deviceType = "bar"; const credentials = getTestUser("baz"); - const fxa = await MockFxAccounts(credentials, { name: deviceName }); + const fxa = new MockFxAccounts({ name: deviceName }); + await fxa.internal.setSignedInUser(credentials); // Remove the current device registration (setSignedInUser does one!). await fxa.updateUserAccountData({ uid: credentials.uid, device: null }); @@ -215,7 +213,8 @@ add_task(async function test_updateDeviceRegistration_with_existing_device() { const deviceName = "phil's device"; const credentials = getTestUser("pb"); - const fxa = await MockFxAccounts(credentials, { name: deviceName }); + const fxa = new MockFxAccounts({ name: deviceName }); + await fxa.internal.setSignedInUser(credentials); await fxa.updateUserAccountData({ uid: credentials.uid, device: { @@ -279,7 +278,8 @@ add_task( const currentDeviceId = "my device id"; const credentials = getTestUser("baz"); - const fxa = await MockFxAccounts(credentials, { name: deviceName }); + const fxa = new MockFxAccounts({ name: deviceName }); + await fxa.internal.setSignedInUser(credentials); await fxa.updateUserAccountData({ uid: credentials.uid, device: { @@ -350,7 +350,8 @@ add_task( const conflictingDeviceId = "conflicting device id"; const credentials = getTestUser("baz"); - const fxa = await MockFxAccounts(credentials, { name: deviceName }); + const fxa = new MockFxAccounts({ name: deviceName }); + await fxa.internal.setSignedInUser(credentials); await fxa.updateUserAccountData({ uid: credentials.uid, device: { @@ -438,7 +439,8 @@ add_task( const deviceName = "foo"; const credentials = getTestUser("baz"); - const fxa = await MockFxAccounts(credentials, { name: deviceName }); + const fxa = new MockFxAccounts({ name: deviceName }); + await fxa.internal.setSignedInUser(credentials); await fxa.updateUserAccountData({ uid: credentials.uid, device: null }); const spy = { @@ -484,7 +486,8 @@ add_task( async function test_getDeviceId_with_no_device_id_invokes_device_registration() { const credentials = getTestUser("foo"); credentials.verified = true; - const fxa = await MockFxAccounts(credentials); + const fxa = new MockFxAccounts(); + await fxa.internal.setSignedInUser(credentials); await fxa.updateUserAccountData({ uid: credentials.uid, device: null }); const spy = { count: 0, args: [] }; @@ -499,7 +502,7 @@ add_task( return Promise.resolve("bar"); }; - const result = await fxa.internal.device.getLocalId(); + const result = await fxa.internal.getDeviceId(); Assert.equal(spy.count, 1); Assert.equal(spy.args[0].length, 1); @@ -513,7 +516,8 @@ add_task( async function test_getDeviceId_with_registration_version_outdated_invokes_device_registration() { const credentials = getTestUser("foo"); credentials.verified = true; - const fxa = await MockFxAccounts(credentials); + const fxa = new MockFxAccounts(); + await fxa.internal.setSignedInUser(credentials); const spy = { count: 0, args: [] }; fxa.internal.currentAccountState.getUserAccountData = () => @@ -530,7 +534,7 @@ add_task( return Promise.resolve("wibble"); }; - const result = await fxa.internal.device.getLocalId(); + const result = await fxa.internal.getDeviceId(); Assert.equal(spy.count, 1); Assert.equal(spy.args[0].length, 1); @@ -543,7 +547,8 @@ add_task( async function test_getDeviceId_with_device_id_and_uptodate_registration_version_doesnt_invoke_device_registration() { const credentials = getTestUser("foo"); credentials.verified = true; - const fxa = await MockFxAccounts(credentials); + const fxa = new MockFxAccounts(); + await fxa.internal.setSignedInUser(credentials); const spy = { count: 0 }; fxa.internal.currentAccountState.getUserAccountData = async () => ({ @@ -558,7 +563,7 @@ add_task( return Promise.resolve("bar"); }; - const result = await fxa.internal.device.getLocalId(); + const result = await fxa.internal.getDeviceId(); Assert.equal(spy.count, 0); Assert.equal(result, "foo's device id"); @@ -569,7 +574,8 @@ add_task( async function test_getDeviceId_with_device_id_and_with_no_registration_version_invokes_device_registration() { const credentials = getTestUser("foo"); credentials.verified = true; - const fxa = await MockFxAccounts(credentials); + const fxa = new MockFxAccounts(); + await fxa.internal.setSignedInUser(credentials); const spy = { count: 0, args: [] }; fxa.internal.currentAccountState.getUserAccountData = () => @@ -580,7 +586,7 @@ add_task( return Promise.resolve("wibble"); }; - const result = await fxa.internal.device.getLocalId(); + const result = await fxa.internal.getDeviceId(); Assert.equal(spy.count, 1); Assert.equal(spy.args[0].length, 1); @@ -589,11 +595,38 @@ add_task( } ); +add_task(async function test_migration_toplevel_deviceId_to_device() { + const credentials = getTestUser("foo"); + credentials.verified = true; + const fxa = new MockFxAccounts(); + await fxa.internal.setSignedInUser(credentials); + await fxa.updateUserAccountData({ uid: credentials.uid, device: null }); + // Can't use updateUserAccountData here since it won't accept deprecated fields! + const accountData = + fxa.internal.currentAccountState.storageManager.accountData; + accountData.deviceId = "mydeviceid"; + accountData.deviceRegistrationVersion = DEVICE_REGISTRATION_VERSION; + + const result = await fxa.internal.getDeviceId(); + Assert.equal(result, "mydeviceid"); + + const state = fxa.internal.currentAccountState; + const data = await state.getUserAccountData(); + Assert.deepEqual(data.device, { + id: "mydeviceid", + registrationVersion: DEVICE_REGISTRATION_VERSION, + registeredCommandsKeys: [], + }); + Assert.ok(!data.deviceId); + Assert.ok(!data.deviceRegistrationVersion); +}); + add_task(async function test_devicelist_pushendpointexpired() { const deviceId = "mydeviceid"; const credentials = getTestUser("baz"); credentials.verified = true; - const fxa = await MockFxAccounts(credentials); + const fxa = new MockFxAccounts(); + await fxa.internal.setSignedInUser(credentials); await fxa.updateUserAccountData({ uid: credentials.uid, device: { diff --git a/services/fxaccounts/tests/xpcshell/test_device.js b/services/fxaccounts/tests/xpcshell/test_device.js deleted file mode 100644 index afc49d338f33..000000000000 --- a/services/fxaccounts/tests/xpcshell/test_device.js +++ /dev/null @@ -1,92 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -"use strict"; - -const { fxAccounts } = ChromeUtils.import( - "resource://gre/modules/FxAccounts.jsm" -); - -const { PREF_ACCOUNT_ROOT } = ChromeUtils.import( - "resource://gre/modules/FxAccountsCommon.js" -); - -_("Misc tests for FxAccounts.device"); - -add_test(function test_default_device_name() { - // Note that head_helpers overrides getDefaultDeviceName - this test is - // really just to ensure the actual implementation is sane - we can't - // really check the value it uses is correct. - // We are just hoping to avoid a repeat of bug 1369285. - let def = fxAccounts.device._getDefaultLocalName(); // make sure it doesn't throw. - _("default value is " + def); - ok(def.length > 0); - - // This is obviously tied to the implementation, but we want early warning - // if any of these things fail. - // We really want one of these 2 to provide a value. - let hostname = - Services.sysinfo.get("device") || - Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService) - .myHostName; - _("hostname is " + hostname); - ok(hostname.length > 0); - // the hostname should be in the default. - ok(def.includes(hostname)); - // We expect the following to work as a fallback to the above. - let fallback = Cc["@mozilla.org/network/protocol;1?name=http"].getService( - Ci.nsIHttpProtocolHandler - ).oscpu; - _("UA fallback is " + fallback); - ok(fallback.length > 0); - // the fallback should not be in the default - ok(!def.includes(fallback)); - - run_next_test(); -}); - -add_test(function test_migration() { - Services.prefs.clearUserPref("identity.fxaccounts.account.device.name"); - Services.prefs.setStringPref("services.sync.client.name", "my client name"); - // calling getLocalName() should move the name to the new pref and reset the old. - equal(fxAccounts.device.getLocalName(), "my client name"); - equal( - Services.prefs.getStringPref("identity.fxaccounts.account.device.name"), - "my client name" - ); - ok(!Services.prefs.prefHasUserValue("services.sync.client.name")); - run_next_test(); -}); - -add_test(function test_migration_set_before_get() { - Services.prefs.setStringPref("services.sync.client.name", "old client name"); - fxAccounts.device.setLocalName("new client name"); - equal(fxAccounts.device.getLocalName(), "new client name"); - run_next_test(); -}); - -add_task(async function test_reset() { - // We don't test the client name specifically here because the client name - // is set as part of signing the user in via the attempt to register the - // device. - const testPref = PREF_ACCOUNT_ROOT + "test-pref"; - Services.prefs.setStringPref(testPref, "whatever"); - let credentials = { - email: "foo@example.com", - uid: "1234@lcip.org", - assertion: "foobar", - sessionToken: "dead", - kSync: "beef", - kXCS: "cafe", - kExtSync: "bacon", - kExtKbHash: "cheese", - verified: true, - }; - await fxAccounts.setSignedInUser(credentials); - ok(!Services.prefs.prefHasUserValue(testPref)); - // signing the user out should reset the name pref. - const namePref = PREF_ACCOUNT_ROOT + "device.name"; - ok(Services.prefs.prefHasUserValue(namePref)); - await fxAccounts.signOut(/* remoteOnly = */ true); - ok(!Services.prefs.prefHasUserValue(namePref)); -}); diff --git a/services/fxaccounts/tests/xpcshell/xpcshell.ini b/services/fxaccounts/tests/xpcshell/xpcshell.ini index 1c1b7bc4ee15..2e3cc1ef72a2 100644 --- a/services/fxaccounts/tests/xpcshell/xpcshell.ini +++ b/services/fxaccounts/tests/xpcshell/xpcshell.ini @@ -11,7 +11,6 @@ support-files = [test_client.js] [test_commands.js] [test_credentials.js] -[test_device.js] [test_loginmgr_storage.js] [test_oauth_grant_client.js] [test_oauth_grant_client_server.js] diff --git a/services/sync/modules/engines/clients.js b/services/sync/modules/engines/clients.js index 433040349294..793ad04a9a8a 100644 --- a/services/sync/modules/engines/clients.js +++ b/services/sync/modules/engines/clients.js @@ -234,14 +234,21 @@ ClientEngine.prototype = { }, get localName() { - return this.fxAccounts.device.getLocalName(); + return Utils.getDeviceName(); }, set localName(value) { - this.fxAccounts.device.setLocalName(value); + Svc.Prefs.set("client.name", value); + // Update the registration in the background. + this.fxAccounts.updateDeviceRegistration().catch(error => { + this._log.warn("failed to update fxa device registration", error); + }); }, get localType() { - return this.fxAccounts.device.getLocalType(); + return Utils.getDeviceType(); + }, + set localType(value) { + Svc.Prefs.set("client.type", value); }, getClientName(id) { @@ -356,7 +363,7 @@ ClientEngine.prototype = { this._log.debug("Updating the known stale clients"); // _fetchFxADevices side effect updates this._knownStaleFxADeviceIds. await this._fetchFxADevices(); - let localFxADeviceId = await fxAccounts.device.getLocalId(); + let localFxADeviceId = await fxAccounts.getDeviceId(); // Process newer records first, so that if we hit a record with a device ID // we've seen before, we can mark it stale immediately. let clientList = Object.values(this._store._remoteClients).sort( @@ -445,7 +452,7 @@ ClientEngine.prototype = { await this._removeRemoteClient(id); } } - let localFxADeviceId = await fxAccounts.device.getLocalId(); + let localFxADeviceId = await fxAccounts.getDeviceId(); // Bug 1264498: Mobile clients don't remove themselves from the clients // collection when the user disconnects Sync, so we mark as stale clients // with the same name that haven't synced in over a week. @@ -615,7 +622,7 @@ ClientEngine.prototype = { }; let excludedIds = null; if (!ids) { - const localFxADeviceId = await fxAccounts.device.getLocalId(); + const localFxADeviceId = await fxAccounts.getDeviceId(); excludedIds = [localFxADeviceId]; } try { @@ -1103,7 +1110,7 @@ ClientStore.prototype = { // Package the individual components into a record for the local client if (id == this.engine.localID) { try { - record.fxaDeviceId = await this.engine.fxAccounts.device.getLocalId(); + record.fxaDeviceId = await this.engine.fxAccounts.getDeviceId(); } catch (error) { this._log.warn("failed to get fxa device id", error); } diff --git a/services/sync/modules/util.js b/services/sync/modules/util.js index 0f893dd7a1ea..1113e52d044b 100644 --- a/services/sync/modules/util.js +++ b/services/sync/modules/util.js @@ -47,6 +47,13 @@ XPCOMUtils.defineLazyServiceGetter( "nsILoginManagerCrypto" ); +XPCOMUtils.defineLazyPreferenceGetter( + this, + "localDeviceName", + "services.sync.client.name", + "" +); + XPCOMUtils.defineLazyPreferenceGetter( this, "localDeviceType", @@ -686,6 +693,68 @@ var Utils = { return result; }, + getDefaultDeviceName() { + // Generate a client name if we don't have a useful one yet + let env = Cc["@mozilla.org/process/environment;1"].getService( + Ci.nsIEnvironment + ); + let user = + env.get("USER") || + env.get("USERNAME") || + Svc.Prefs.get("account") || + Svc.Prefs.get("username"); + // A little hack for people using the the moz-build environment on Windows + // which sets USER to the literal "%USERNAME%" (yes, really) + if (user == "%USERNAME%" && env.get("USERNAME")) { + user = env.get("USERNAME"); + } + + let brand = Services.strings.createBundle( + "chrome://branding/locale/brand.properties" + ); + let brandName = brand.GetStringFromName("brandShortName"); + + // The DNS service may fail to provide a hostname in edge-cases we don't + // fully understand - bug 1391488. + let hostname; + try { + // hostname of the system, usually assigned by the user or admin + hostname = Cc["@mozilla.org/network/dns-service;1"].getService( + Ci.nsIDNSService + ).myHostName; + } catch (ex) { + Cu.reportError(ex); + } + let system = + // 'device' is defined on unix systems + Services.sysinfo.get("device") || + hostname || + // fall back on ua info string + Cc["@mozilla.org/network/protocol;1?name=http"].getService( + Ci.nsIHttpProtocolHandler + ).oscpu; + + let syncStrings = Services.strings.createBundle( + "chrome://weave/locale/sync.properties" + ); + return syncStrings.formatStringFromName("client.name2", [ + user, + brandName, + system, + ]); + }, + + getDeviceName() { + let deviceName = localDeviceName; + + if (deviceName === "") { + deviceName = this.getDefaultDeviceName(); + Svc.Prefs.set("client.name", deviceName); + } + + return deviceName; + }, + /** * Helper to implement a more efficient version of fairly common pattern: * diff --git a/services/sync/tests/unit/head_helpers.js b/services/sync/tests/unit/head_helpers.js index 2e0a5b4eb7e9..abfdcf55ba65 100644 --- a/services/sync/tests/unit/head_helpers.js +++ b/services/sync/tests/unit/head_helpers.js @@ -509,6 +509,15 @@ function promiseOneObserver(topic, callback) { }); } +// Avoid an issue where `client.name2` containing unicode characters causes +// a number of tests to fail, due to them assuming that we do not need to utf-8 +// encode or decode data sent through the mocked server (see bug 1268912). +// We stash away the original implementation so test_utils_misc.js can test it. +Utils._orig_getDefaultDeviceName = Utils.getDefaultDeviceName; +Utils.getDefaultDeviceName = function() { + return "Test device name"; +}; + async function registerRotaryEngine() { let { RotaryEngine } = ChromeUtils.import( "resource://testing-common/services/sync/rotaryengine.js" diff --git a/services/sync/tests/unit/test_clients_engine.js b/services/sync/tests/unit/test_clients_engine.js index 4b50bd5b0e68..ebc1fb7abd19 100644 --- a/services/sync/tests/unit/test_clients_engine.js +++ b/services/sync/tests/unit/test_clients_engine.js @@ -980,16 +980,8 @@ add_task(async function test_clients_not_in_fxa_list() { notifyDevices() { return Promise.resolve(true); }, - device: { - getLocalId() { - return fxAccounts.device.getLocalId(); - }, - getLocalName() { - return fxAccounts.device.getLocalName(); - }, - getLocalType() { - return fxAccounts.device.getLocalType(); - }, + getDeviceId() { + return fxAccounts.getDeviceId(); }, getDeviceList() { return Promise.resolve([{ id: remoteId }]); @@ -1059,16 +1051,8 @@ add_task(async function test_dupe_device_ids() { notifyDevices() { return Promise.resolve(true); }, - device: { - getLocalId() { - return fxAccounts.device.getLocalId(); - }, - getLocalName() { - return fxAccounts.device.getLocalName(); - }, - getLocalType() { - return fxAccounts.device.getLocalType(); - }, + getDeviceId() { + return fxAccounts.getDeviceId(); }, getDeviceList() { return Promise.resolve([{ id: remoteDeviceId }]); @@ -2073,16 +2057,8 @@ add_task(async function test_other_clients_notified_on_first_sync() { const fxAccounts = engine.fxAccounts; let calls = 0; engine.fxAccounts = { - device: { - getLocalId() { - return fxAccounts.device.getLocalId(); - }, - getLocalName() { - return fxAccounts.device.getLocalName(); - }, - getLocalType() { - return fxAccounts.device.getLocalType(); - }, + getDeviceId() { + return fxAccounts.getDeviceId(); }, notifyDevices() { calls++; diff --git a/services/sync/tests/unit/test_utils_misc.js b/services/sync/tests/unit/test_utils_misc.js new file mode 100644 index 000000000000..6e926e2cc2ca --- /dev/null +++ b/services/sync/tests/unit/test_utils_misc.js @@ -0,0 +1,33 @@ +_("Misc tests for utils.js"); + +add_test(function test_default_device_name() { + // Note that head_helpers overrides getDefaultDeviceName - this test is + // really just to ensure the actual implementation is sane - we can't + // really check the value it uses is correct. + // We are just hoping to avoid a repeat of bug 1369285. + let def = Utils._orig_getDefaultDeviceName(); // make sure it doesn't throw. + _("default value is " + def); + ok(def.length > 0); + + // This is obviously tied to the implementation, but we want early warning + // if any of these things fail. + // We really want one of these 2 to provide a value. + let hostname = + Services.sysinfo.get("device") || + Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService) + .myHostName; + _("hostname is " + hostname); + ok(hostname.length > 0); + // the hostname should be in the default. + ok(def.includes(hostname)); + // We expect the following to work as a fallback to the above. + let fallback = Cc["@mozilla.org/network/protocol;1?name=http"].getService( + Ci.nsIHttpProtocolHandler + ).oscpu; + _("UA fallback is " + fallback); + ok(fallback.length > 0); + // the fallback should not be in the default + ok(!def.includes(fallback)); + + run_next_test(); +}); diff --git a/services/sync/tests/unit/xpcshell.ini b/services/sync/tests/unit/xpcshell.ini index da2b56c778d8..e23d91a38bdd 100644 --- a/services/sync/tests/unit/xpcshell.ini +++ b/services/sync/tests/unit/xpcshell.ini @@ -29,6 +29,7 @@ support-files = [test_utils_lock.js] [test_utils_makeGUID.js] run-sequentially = Disproportionately slows down full test run, bug 1450316 +[test_utils_misc.js] [test_utils_notify.js] [test_utils_passphrase.js] From a2304c7a65e87d57db1ce8468e9b85de717e8078 Mon Sep 17 00:00:00 2001 From: Jason Laster Date: Mon, 9 Sep 2019 23:28:36 +0000 Subject: [PATCH 11/24] Bug 1580033 - Fix saveAs. r=davidwalsh Differential Revision: https://phabricator.services.mozilla.com/D45265 --HG-- extra : moz-landing-system : lando --- devtools/client/debugger/src/utils/utils.js | 4 ++-- devtools/client/shared/build/build-debugger.js | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/devtools/client/debugger/src/utils/utils.js b/devtools/client/debugger/src/utils/utils.js index b188c8a67b45..c4a9a023cbf0 100644 --- a/devtools/client/debugger/src/utils/utils.js +++ b/devtools/client/debugger/src/utils/utils.js @@ -5,7 +5,7 @@ // @flow import type { SourceContent } from "../types"; -import { saveAs } from "devtools-modules"; +import { DevToolsUtils } from "devtools-modules"; /** * Utils for utils, by utils @@ -62,5 +62,5 @@ export function downloadFile(content: SourceContent, fileName: string) { } const data = new TextEncoder().encode(content.value); - saveAs(window, data, fileName); + DevToolsUtils.saveAs(window, data, fileName); } diff --git a/devtools/client/shared/build/build-debugger.js b/devtools/client/shared/build/build-debugger.js index 78af39fd7e24..a96dbb0e8ae3 100644 --- a/devtools/client/shared/build/build-debugger.js +++ b/devtools/client/shared/build/build-debugger.js @@ -72,6 +72,7 @@ const moduleMapping = { asyncStoreHelper: "devtools/client/shared/async-store-helper", asyncStorage: "devtools/shared/async-storage", PluralForm: "devtools/shared/plural-form", + DevToolsUtils: "devtools/shared/DevToolsUtils", }; /* From 04e8f93b98a2a466bdab3486c63b58d9e956c5b9 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Mon, 9 Sep 2019 02:48:56 +0000 Subject: [PATCH 12/24] Bug 1574852 - part 85: Move `HTMLEditRules::AlignBlock()` to `HTMLEditor` r=m_kato With guaranteeing the argument element type with `MOZ_ASSERT()`, we can make it really simpler. Differential Revision: https://phabricator.services.mozilla.com/D44789 --HG-- extra : moz-landing-system : lando --- editor/libeditor/HTMLEditRules.cpp | 100 ++++++++++++----------------- editor/libeditor/HTMLEditRules.h | 16 ----- editor/libeditor/HTMLEditor.h | 22 +++++++ 3 files changed, 62 insertions(+), 76 deletions(-) diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index f359e340a854..c0c9bed7cddb 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -6336,13 +6336,13 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { // The node is a table element, an hr, a paragraph, a div or a section // header; in HTML 4, it can directly carry the ALIGN attribute and we // don't need to make a div! If we are in CSS mode, all the work is done - // in AlignBlock - rv = AlignBlock(MOZ_KnownLive(*node->AsElement()), aAlignType, - ResetAlignOf::OnlyDescendants); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - return NS_OK; + // in SetBlockElementAlign(). + nsresult rv = MOZ_KnownLive(HTMLEditorRef()) + .SetBlockElementAlign( + MOZ_KnownLive(*node->AsElement()), aAlignType, + HTMLEditor::EditTarget::OnlyDescendantsExceptTable); + NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "SetBlockElementAlign() failed"); + return rv; } if (TextEditUtils::IsBreak(node)) { @@ -6429,7 +6429,10 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { // Remember our new block for postprocessing HTMLEditorRef().TopLevelEditSubActionDataRef().mNewBlockElement = div; // Set up the alignment on the div, using HTML or CSS - rv = AlignBlock(*div, aAlignType, ResetAlignOf::OnlyDescendants); + rv = MOZ_KnownLive(HTMLEditorRef()) + .SetBlockElementAlign( + *div, aAlignType, + HTMLEditor::EditTarget::OnlyDescendantsExceptTable); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } @@ -6480,10 +6483,13 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { // The node is a table element, an hr, a paragraph, a div or a section // header; in HTML 4, it can directly carry the ALIGN attribute and we // don't need to nest it, just set the alignment. In CSS, assign the - // corresponding CSS styles in AlignBlock + // corresponding CSS styles in SetBlockElementAlign(). if (HTMLEditUtils::SupportsAlignAttr(*curNode)) { - rv = AlignBlock(MOZ_KnownLive(*curNode->AsElement()), aAlignType, - ResetAlignOf::ElementAndDescendants); + nsresult rv = + MOZ_KnownLive(HTMLEditorRef()) + .SetBlockElementAlign( + MOZ_KnownLive(*curNode->AsElement()), aAlignType, + HTMLEditor::EditTarget::NodeAndDescendantsExceptTable); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } @@ -6579,11 +6585,15 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { // Remember our new block for postprocessing HTMLEditorRef().TopLevelEditSubActionDataRef().mNewBlockElement = curDiv; // Set up the alignment on the div - rv = AlignBlock(*curDiv, aAlignType, ResetAlignOf::OnlyDescendants); + rv = MOZ_KnownLive(HTMLEditorRef()) + .SetBlockElementAlign( + *curDiv, aAlignType, + HTMLEditor::EditTarget::OnlyDescendantsExceptTable); if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) { return NS_ERROR_EDITOR_DESTROYED; } - NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Failed to align the
"); + NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), + "SetBlockElementAlign() failed, but ignored"); } // Tuck the node into the end of the active div @@ -10772,61 +10782,31 @@ nsresult HTMLEditor::EnsureHardLineEndsWithLastChildOf( return NS_OK; } -nsresult HTMLEditRules::AlignBlock(Element& aElement, - const nsAString& aAlignType, - ResetAlignOf aResetAlignOf) { - MOZ_ASSERT(IsEditorDataAvailable()); +nsresult HTMLEditor::SetBlockElementAlign(Element& aBlockOrHRElement, + const nsAString& aAlignType, + EditTarget aEditTarget) { + MOZ_ASSERT(IsEditActionDataAvailable()); + MOZ_ASSERT(HTMLEditor::NodeIsBlockStatic(aBlockOrHRElement) || + aBlockOrHRElement.IsHTMLElement(nsGkAtoms::hr)); + MOZ_ASSERT(IsCSSEnabled() || + HTMLEditUtils::SupportsAlignAttr(aBlockOrHRElement)); - if (!HTMLEditor::NodeIsBlockStatic(aElement) && - !aElement.IsHTMLElement(nsGkAtoms::hr)) { - // We deal only with blocks; early way out - return NS_OK; - } - - if (!aElement.IsHTMLElement(nsGkAtoms::table)) { + if (!aBlockOrHRElement.IsHTMLElement(nsGkAtoms::table)) { nsresult rv = - MOZ_KnownLive(HTMLEditorRef()) - .RemoveAlignFromDescendants( - aElement, aAlignType, - aResetAlignOf == ResetAlignOf::OnlyDescendants - ? HTMLEditor::EditTarget::OnlyDescendantsExceptTable - : HTMLEditor::EditTarget::NodeAndDescendantsExceptTable); + RemoveAlignFromDescendants(aBlockOrHRElement, aAlignType, aEditTarget); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } } - if (HTMLEditorRef().IsCSSEnabled()) { - // Let's use CSS alignment; we use margin-left and margin-right for tables - // and text-align for other block-level elements - nsresult rv = MOZ_KnownLive(HTMLEditorRef()) - .SetAttributeOrEquivalent(&aElement, nsGkAtoms::align, - aAlignType, false); - if (NS_WARN_IF(!CanHandleEditAction())) { - return NS_ERROR_EDITOR_DESTROYED; - } - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - return NS_OK; - } - - // HTML case; this code is supposed to be called ONLY if the element - // supports the align attribute but we'll never know... - if (NS_WARN_IF(!HTMLEditUtils::SupportsAlignAttr(aElement))) { - // XXX error? - return NS_OK; - } - - nsresult rv = MOZ_KnownLive(HTMLEditorRef()) - .SetAttributeOrEquivalent(&aElement, nsGkAtoms::align, - aAlignType, false); - if (NS_WARN_IF(!CanHandleEditAction())) { + nsresult rv = SetAttributeOrEquivalent(&aBlockOrHRElement, nsGkAtoms::align, + aAlignType, false); + if (NS_WARN_IF(Destroyed())) { return NS_ERROR_EDITOR_DESTROYED; } - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - return NS_OK; + NS_WARNING_ASSERTION( + NS_SUCCEEDED(rv), + "SetAttributeOrEquivalent() failed to set `align` attribute or property"); + return rv; } nsresult HTMLEditor::ChangeMarginStart(Element& aElement, diff --git a/editor/libeditor/HTMLEditRules.h b/editor/libeditor/HTMLEditRules.h index 6809c5e7eceb..af570d04428c 100644 --- a/editor/libeditor/HTMLEditRules.h +++ b/editor/libeditor/HTMLEditRules.h @@ -289,22 +289,6 @@ class HTMLEditRules : public TextEditRules { */ MOZ_MUST_USE nsresult ConfirmSelectionInBody(); - /** - * AlignBlock() resets align attribute, text-align property, etc first. - * Then, aligns contents of aElement on aAlignType. - * - * @param aElement The element whose contents will be aligned. - * @param aAlignType Boundary or "center" which contents should be - * aligned on. - * @param aResetAlignOf Resets align of whether element and its - * descendants or only descendants. - */ - enum class ResetAlignOf { ElementAndDescendants, OnlyDescendants }; - MOZ_CAN_RUN_SCRIPT - MOZ_MUST_USE nsresult AlignBlock(Element& aElement, - const nsAString& aAlignType, - ResetAlignOf aResetAlignOf); - /** * DocumentModifiedWorker() is called by DocumentModified() either * synchronously or asynchronously. diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h index ac869558198d..9fd7fdfc694e 100644 --- a/editor/libeditor/HTMLEditor.h +++ b/editor/libeditor/HTMLEditor.h @@ -2501,6 +2501,28 @@ class HTMLEditor final : public TextEditor, MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult RemoveAlignFromDescendants( Element& aElement, const nsAString& aAlignType, EditTarget aEditTarget); + /** + * SetBlockElementAlign() resets `align` attribute, `text-align` property + * of descendants of aBlockOrHRElement except `` element descendants. + * Then, set `align` attribute or `text-align` property of aBlockOrHRElement. + * + * @param aBlockOrHRElement The element whose contents will be aligned. + * This must be a block element or `
` element. + * If we're not in CSS mode, this element has + * to support `align` attribute (i.e., + * `HTMLEditUtils::SupportsAlignAttr()` must + * return true). + * @param aAlignType Boundary or "center" which contents should be + * aligned on. + * @param aEditTarget If `OnlyDescendantsExceptTable`, modifies only + * descendants of aBlockOrHRElement. + * If `NodeAndDescendantsExceptTable`, modifies + * aBlockOrHRElement and its descendants. + */ + MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult + SetBlockElementAlign(Element& aBlockOrHRElement, const nsAString& aAlignType, + EditTarget aEditTarget); + protected: // Called by helper classes. virtual void OnStartToHandleTopLevelEditSubAction( EditSubAction aEditSubAction, nsIEditor::EDirection aDirection) override; From b1ead5a6f385bc3d54fdbbe25bcb3f8249391eb8 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Mon, 9 Sep 2019 04:57:50 +0000 Subject: [PATCH 13/24] Bug 1574852 - part 86: Move `HTMLEditRules::WillAlign()` and `HTMLEditRules::AlignContentsAtSelection()` to `HTMLEditor` r=m_kato And also this splits large 2 blocks of `HTMLEditRules::AlignContentsAtSelection()` to 2 methods. Differential Revision: https://phabricator.services.mozilla.com/D44790 --HG-- extra : moz-landing-system : lando --- editor/libeditor/HTMLEditRules.cpp | 359 +++++++++++++++-------------- editor/libeditor/HTMLEditRules.h | 27 --- editor/libeditor/HTMLEditor.cpp | 25 +- editor/libeditor/HTMLEditor.h | 50 +++- editor/nsIHTMLEditor.idl | 1 + 5 files changed, 238 insertions(+), 224 deletions(-) diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index c0c9bed7cddb..dfa098ed6eb7 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -788,8 +788,6 @@ nsresult HTMLEditRules::WillDoAction(EditSubActionInfo& aInfo, bool* aCancel, return WillAbsolutePosition(aCancel, aHandled); case EditSubAction::eSetPositionToStatic: return WillRemoveAbsolutePosition(aCancel, aHandled); - case EditSubAction::eSetOrClearAlignment: - return WillAlign(*aInfo.alignType, aCancel, aHandled); case EditSubAction::eInsertElement: case EditSubAction::eInsertQuotedText: { nsresult rv = MOZ_KnownLive(HTMLEditorRef()).WillInsert(aCancel); @@ -813,6 +811,7 @@ nsresult HTMLEditRules::WillDoAction(EditSubActionInfo& aInfo, bool* aCancel, case EditSubAction::eUndo: case EditSubAction::eRedo: case EditSubAction::eRemoveList: + case EditSubAction::eSetOrClearAlignment: MOZ_ASSERT_UNREACHABLE("This path should've been dead code"); return NS_ERROR_UNEXPECTED; default: @@ -835,9 +834,6 @@ nsresult HTMLEditRules::DidDoAction(EditSubActionInfo& aInfo, return NS_OK; case EditSubAction::eDeleteSelectedContent: return DidDeleteSelection(); - case EditSubAction::eSetOrClearAlignment: - return MOZ_KnownLive(HTMLEditorRef()) - .MaybeInsertPaddingBRElementForEmptyLastLineAtSelection(); case EditSubAction::eSetPositionToAbsolute: { nsresult rv = MOZ_KnownLive(HTMLEditorRef()) @@ -860,6 +856,7 @@ nsresult HTMLEditRules::DidDoAction(EditSubActionInfo& aInfo, case EditSubAction::eUndo: case EditSubAction::eRedo: case EditSubAction::eRemoveList: + case EditSubAction::eSetOrClearAlignment: MOZ_ASSERT_UNREACHABLE("This path should've been dead code"); return NS_ERROR_UNEXPECTED; default: @@ -6273,54 +6270,61 @@ bool HTMLEditor::IsEmptyBlockElement(Element& aElement, return NS_SUCCEEDED(rv) && isEmpty; } -nsresult HTMLEditRules::WillAlign(const nsAString& aAlignType, bool* aCancel, - bool* aHandled) { - MOZ_ASSERT(IsEditorDataAvailable()); - MOZ_ASSERT(aCancel && aHandled); +EditActionResult HTMLEditor::AlignAsSubAction(const nsAString& aAlignType) { + MOZ_ASSERT(IsEditActionDataAvailable()); - *aCancel = false; - *aHandled = false; + AutoPlaceholderBatch treatAsOneTransaction(*this); + AutoEditSubActionNotifier startToHandleEditSubAction( + *this, EditSubAction::eSetOrClearAlignment, nsIEditor::eNext); + + EditActionResult result = CanHandleHTMLEditSubAction(); + if (NS_WARN_IF(result.Failed()) || result.Canceled()) { + return result; + } // FYI: Ignore cancel result of WillInsert(). - nsresult rv = MOZ_KnownLive(HTMLEditorRef()).WillInsert(); + nsresult rv = WillInsert(); if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) { - return NS_ERROR_EDITOR_DESTROYED; + return EditActionResult(NS_ERROR_EDITOR_DESTROYED); } - NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "WillInsert() failed"); + NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "WillInsert() failed, but ignored"); if (!SelectionRefPtr()->IsCollapsed()) { - nsresult rv = MOZ_KnownLive(HTMLEditorRef()) - .MaybeExtendSelectionToHardLineEdgesForBlockEditAction(); + nsresult rv = MaybeExtendSelectionToHardLineEdgesForBlockEditAction(); if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; + return EditActionResult(rv); } } - *aHandled = true; + // AlignContentsAtSelection() creates AutoSelectionRestorer. Therefore, + // we need to check whether we've been destroyed or not even if it returns + // NS_OK. rv = AlignContentsAtSelection(aAlignType); - if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED) || - NS_WARN_IF(!CanHandleEditAction())) { - return NS_ERROR_EDITOR_DESTROYED; + if (NS_WARN_IF(Destroyed())) { + return EditActionHandled(NS_ERROR_EDITOR_DESTROYED); } if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; + return EditActionHandled(rv); } - return NS_OK; + + rv = MaybeInsertPaddingBRElementForEmptyLastLineAtSelection(); + NS_WARNING_ASSERTION( + NS_SUCCEEDED(rv), + "MaybeInsertPaddingBRElementForEmptyLastLineAtSelection() failed"); + return EditActionHandled(rv); } -nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { - AutoSelectionRestorer restoreSelectionLater(HTMLEditorRef()); +nsresult HTMLEditor::AlignContentsAtSelection(const nsAString& aAlignType) { + AutoSelectionRestorer restoreSelectionLater(*this); // Convert the selection ranges into "promoted" selection ranges: This // basically just expands the range to include the immediate block parent, // and then further expands to include any ancestors whose children are all // in the range AutoTArray, 64> nodeArray; - nsresult rv = - MOZ_KnownLive(HTMLEditorRef()) - .SplitInlinesAndCollectEditTargetNodesInExtendedSelectionRanges( - nodeArray, EditSubAction::eSetOrClearAlignment, - HTMLEditor::CollectNonEditableNodes::Yes); + nsresult rv = SplitInlinesAndCollectEditTargetNodesInExtendedSelectionRanges( + nodeArray, EditSubAction::eSetOrClearAlignment, + CollectNonEditableNodes::Yes); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } @@ -6328,7 +6332,7 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { // If we don't have any nodes, or we have only a single br, then we are // creating an empty alignment div. We have to do some different things for // these. - bool emptyDiv = nodeArray.IsEmpty(); + bool createEmptyDivElement = nodeArray.IsEmpty(); if (nodeArray.Length() == 1) { OwningNonNull node = nodeArray[0]; @@ -6337,26 +6341,27 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { // header; in HTML 4, it can directly carry the ALIGN attribute and we // don't need to make a div! If we are in CSS mode, all the work is done // in SetBlockElementAlign(). - nsresult rv = MOZ_KnownLive(HTMLEditorRef()) - .SetBlockElementAlign( - MOZ_KnownLive(*node->AsElement()), aAlignType, - HTMLEditor::EditTarget::OnlyDescendantsExceptTable); + nsresult rv = + SetBlockElementAlign(MOZ_KnownLive(*node->AsElement()), aAlignType, + EditTarget::OnlyDescendantsExceptTable); NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "SetBlockElementAlign() failed"); return rv; } if (TextEditUtils::IsBreak(node)) { - // The special case emptyDiv code (below) that consumes BRs can cause - // tables to split if the start node of the selection is not in a table - // cell or caption, for example parent is a . Avoid this unnecessary - // splitting if possible by leaving emptyDiv FALSE so that we fall - // through to the normal case alignment code. + // The special case createEmptyDivElement code (below) that consumes + // `
` elements can cause tables to split if the start node of the + // selection is not in a table cell or caption, for example parent is a + // ``. Avoid this unnecessary splitting if possible by leaving + // createEmptyDivElement false so that we fall through to the normal case + // alignment code. // - // XXX: It seems a little error prone for the emptyDiv special case code - // to assume that the start node of the selection is the parent of the - // single node in the nodeArray, as the paragraph above points out. Do we - // rely on the selection start node because of the fact that nodeArray - // can be empty? We should probably revisit this issue. - kin + // XXX: It seems a little error prone for the createEmptyDivElement + // special case code to assume that the start node of the selection + // is the parent of the single node in the nodeArray, as the + // paragraph above points out. Do we rely on the selection start + // node because of the fact that nodeArray can be empty? We should + // probably revisit this issue. - kin nsRange* firstRange = SelectionRefPtr()->GetRangeAt(0); if (NS_WARN_IF(!firstRange)) { @@ -6367,116 +6372,128 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { return NS_ERROR_FAILURE; } nsINode* parent = atStartOfSelection.Container(); - emptyDiv = !HTMLEditUtils::IsTableElement(parent) || - HTMLEditUtils::IsTableCellOrCaption(*parent); + createEmptyDivElement = !HTMLEditUtils::IsTableElement(parent) || + HTMLEditUtils::IsTableCellOrCaption(*parent); } } - if (emptyDiv) { - nsRange* firstRange = SelectionRefPtr()->GetRangeAt(0); - if (NS_WARN_IF(!firstRange)) { - return NS_ERROR_FAILURE; - } - EditorDOMPoint atStartOfSelection(firstRange->StartRef()); - if (NS_WARN_IF(!atStartOfSelection.IsSet())) { - return NS_ERROR_FAILURE; + if (createEmptyDivElement) { + EditActionResult result = + AlignContentsAtSelectionWithEmptyDivElement(aAlignType); + NS_WARNING_ASSERTION( + result.Succeeded(), + "AlignContentsAtSelectionWithEmptyDivElement() failed"); + if (result.Handled()) { + restoreSelectionLater.Abort(); } + return rv; + } - SplitNodeResult splitNodeResult = - MOZ_KnownLive(HTMLEditorRef()) - .MaybeSplitAncestorsForInsertWithTransaction(*nsGkAtoms::div, - atStartOfSelection); - if (NS_WARN_IF(splitNodeResult.Failed())) { - return splitNodeResult.Rv(); - } + rv = AlignNodesAndDescendants(nodeArray, aAlignType); + NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "AlignNodesAndDescendants() failed"); + return rv; +} - // Consume a trailing br, if any. This is to keep an alignment from - // creating extra lines, if possible. - nsCOMPtr brContent = - HTMLEditorRef().GetNextEditableHTMLNodeInBlock( - splitNodeResult.SplitPoint()); - EditorDOMPoint pointToInsertDiv(splitNodeResult.SplitPoint()); - if (brContent && TextEditUtils::IsBreak(brContent)) { +EditActionResult HTMLEditor::AlignContentsAtSelectionWithEmptyDivElement( + const nsAString& aAlignType) { + MOZ_ASSERT(IsTopLevelEditSubActionDataAvailable()); + + nsRange* firstRange = SelectionRefPtr()->GetRangeAt(0); + if (NS_WARN_IF(!firstRange)) { + return EditActionResult(NS_ERROR_FAILURE); + } + + EditorDOMPoint atStartOfSelection(firstRange->StartRef()); + if (NS_WARN_IF(!atStartOfSelection.IsSet())) { + return EditActionResult(NS_ERROR_FAILURE); + } + + SplitNodeResult splitNodeResult = MaybeSplitAncestorsForInsertWithTransaction( + *nsGkAtoms::div, atStartOfSelection); + if (NS_WARN_IF(splitNodeResult.Failed())) { + return EditActionResult(splitNodeResult.Rv()); + } + + EditorDOMPoint pointToInsertDiv(splitNodeResult.SplitPoint()); + + // Consume a trailing br, if any. This is to keep an alignment from + // creating extra lines, if possible. + if (nsCOMPtr maybeBRContent = + GetNextEditableHTMLNodeInBlock(splitNodeResult.SplitPoint())) { + if (TextEditUtils::IsBreak(maybeBRContent) && pointToInsertDiv.GetChild()) { // Making use of html structure... if next node after where we are // putting our div is not a block, then the br we found is in same block // we are, so it's safe to consume it. - nsCOMPtr sibling; - if (pointToInsertDiv.GetChild()) { - sibling = - HTMLEditorRef().GetNextHTMLSibling(pointToInsertDiv.GetChild()); - } - if (sibling && !HTMLEditor::NodeIsBlockStatic(*sibling)) { - AutoEditorDOMPointChildInvalidator lockOffset(pointToInsertDiv); - rv = MOZ_KnownLive(HTMLEditorRef()) - .DeleteNodeWithTransaction(*brContent); - if (NS_WARN_IF(!CanHandleEditAction())) { - return NS_ERROR_EDITOR_DESTROYED; - } - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; + if (nsIContent* nextEditableSibling = + GetNextHTMLSibling(pointToInsertDiv.GetChild())) { + if (!HTMLEditor::NodeIsBlockStatic(*nextEditableSibling)) { + AutoEditorDOMPointChildInvalidator lockOffset(pointToInsertDiv); + nsresult rv = DeleteNodeWithTransaction(*maybeBRContent); + if (NS_WARN_IF(Destroyed())) { + return EditActionResult(NS_ERROR_EDITOR_DESTROYED); + } + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionResult(rv); + } } } } - RefPtr div = - MOZ_KnownLive(HTMLEditorRef()) - .CreateNodeWithTransaction(*nsGkAtoms::div, pointToInsertDiv); - if (NS_WARN_IF(!CanHandleEditAction())) { - return NS_ERROR_EDITOR_DESTROYED; - } - if (NS_WARN_IF(!div)) { - return NS_ERROR_FAILURE; - } - // Remember our new block for postprocessing - HTMLEditorRef().TopLevelEditSubActionDataRef().mNewBlockElement = div; - // Set up the alignment on the div, using HTML or CSS - rv = MOZ_KnownLive(HTMLEditorRef()) - .SetBlockElementAlign( - *div, aAlignType, - HTMLEditor::EditTarget::OnlyDescendantsExceptTable); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - // Put in a padding
element for empty last line so that it won't get - // deleted. - CreateElementResult createPaddingBRResult = - MOZ_KnownLive(HTMLEditorRef()) - .InsertPaddingBRElementForEmptyLastLineWithTransaction( - EditorDOMPoint(div, 0)); - if (NS_WARN_IF(createPaddingBRResult.Failed())) { - return createPaddingBRResult.Rv(); - } - EditorRawDOMPoint atStartOfDiv(div, 0); - // Don't restore the selection - restoreSelectionLater.Abort(); - ErrorResult error; - SelectionRefPtr()->Collapse(atStartOfDiv, error); - if (NS_WARN_IF(!CanHandleEditAction())) { - error.SuppressException(); - return NS_ERROR_EDITOR_DESTROYED; - } - if (NS_WARN_IF(error.Failed())) { - return error.StealNSResult(); - } - return NS_OK; } - // Next we detect all the transitions in the array, where a transition - // means that adjacent nodes in the array don't have the same parent. + RefPtr divElement = + CreateNodeWithTransaction(*nsGkAtoms::div, pointToInsertDiv); + if (NS_WARN_IF(Destroyed())) { + return EditActionResult(NS_ERROR_EDITOR_DESTROYED); + } + if (NS_WARN_IF(!divElement)) { + return EditActionResult(NS_ERROR_FAILURE); + } + // Remember our new block for postprocessing + TopLevelEditSubActionDataRef().mNewBlockElement = divElement; + // Set up the alignment on the div, using HTML or CSS + nsresult rv = SetBlockElementAlign(*divElement, aAlignType, + EditTarget::OnlyDescendantsExceptTable); + if (NS_WARN_IF(NS_FAILED(rv))) { + return EditActionResult(rv); + } + // Put in a padding
element for empty last line so that it won't get + // deleted. + CreateElementResult createPaddingBRResult = + InsertPaddingBRElementForEmptyLastLineWithTransaction( + EditorDOMPoint(divElement, 0)); + if (NS_WARN_IF(createPaddingBRResult.Failed())) { + return EditActionResult(createPaddingBRResult.Rv()); + } + EditorRawDOMPoint atStartOfDiv(divElement, 0); + ErrorResult error; + SelectionRefPtr()->Collapse(atStartOfDiv, error); + if (NS_WARN_IF(Destroyed())) { + error.SuppressException(); + return EditActionHandled(NS_ERROR_EDITOR_DESTROYED); + } + NS_WARNING_ASSERTION(!error.Failed(), "Selection::Collapse() failed"); + return EditActionHandled(error.StealNSResult()); +} +nsresult HTMLEditor::AlignNodesAndDescendants( + nsTArray>& aArrayOfNodes, + const nsAString& aAlignType) { + // Detect all the transitions in the array, where a transition means that + // adjacent nodes in the array don't have the same parent. AutoTArray transitionList; - HTMLEditor::MakeTransitionList(nodeArray, transitionList); + HTMLEditor::MakeTransitionList(aArrayOfNodes, transitionList); // Okay, now go through all the nodes and give them an align attrib or put // them in a div, or whatever is appropriate. Woohoo! - nsCOMPtr curDiv; - bool useCSS = HTMLEditorRef().IsCSSEnabled(); + RefPtr createdDivElement; + bool useCSS = IsCSSEnabled(); int32_t indexOfTransitionList = -1; - for (OwningNonNull& curNode : nodeArray) { + for (OwningNonNull& curNode : aArrayOfNodes) { ++indexOfTransitionList; // Ignore all non-editable nodes. Leave them be. - if (!HTMLEditorRef().IsEditable(curNode)) { + if (!IsEditable(curNode)) { continue; } @@ -6486,15 +6503,14 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { // corresponding CSS styles in SetBlockElementAlign(). if (HTMLEditUtils::SupportsAlignAttr(*curNode)) { nsresult rv = - MOZ_KnownLive(HTMLEditorRef()) - .SetBlockElementAlign( - MOZ_KnownLive(*curNode->AsElement()), aAlignType, - HTMLEditor::EditTarget::NodeAndDescendantsExceptTable); + SetBlockElementAlign(MOZ_KnownLive(*curNode->AsElement()), aAlignType, + EditTarget::NodeAndDescendantsExceptTable); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } - // Clear out curDiv so that we don't put nodes after this one into it - curDiv = nullptr; + // Clear out createdDivElement so that we don't put nodes after this one + // into it + createdDivElement = nullptr; continue; } @@ -6506,12 +6522,11 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { // Skip insignificant formatting text nodes to prevent unnecessary // structure splitting! bool isEmptyTextNode = false; - if (curNode->GetAsText() && + if (curNode->IsText() && ((HTMLEditUtils::IsTableElement(atCurNode.GetContainer()) && !HTMLEditUtils::IsTableCellOrCaption(*atCurNode.GetContainer())) || HTMLEditUtils::IsList(atCurNode.GetContainer()) || - (NS_SUCCEEDED( - HTMLEditorRef().IsEmptyNode(curNode, &isEmptyTextNode)) && + (NS_SUCCEEDED(IsEmptyNode(curNode, &isEmptyTextNode)) && isEmptyTextNode))) { continue; } @@ -6521,74 +6536,73 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { if (HTMLEditUtils::IsListItem(curNode) || HTMLEditUtils::IsList(curNode)) { Element* listOrListItemElement = curNode->AsElement(); AutoEditorDOMPointOffsetInvalidator lockChild(atCurNode); - rv = MOZ_KnownLive(HTMLEditorRef()) - .RemoveAlignFromDescendants( - MOZ_KnownLive(*listOrListItemElement), aAlignType, - HTMLEditor::EditTarget::OnlyDescendantsExceptTable); + nsresult rv = RemoveAlignFromDescendants( + MOZ_KnownLive(*listOrListItemElement), aAlignType, + EditTarget::OnlyDescendantsExceptTable); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } + if (useCSS) { - HTMLEditorRef().mCSSEditUtils->SetCSSEquivalentToHTMLStyle( + mCSSEditUtils->SetCSSEquivalentToHTMLStyle( MOZ_KnownLive(listOrListItemElement), nullptr, nsGkAtoms::align, &aAlignType, false); - if (NS_WARN_IF(!CanHandleEditAction())) { + if (NS_WARN_IF(Destroyed())) { return NS_ERROR_EDITOR_DESTROYED; } - curDiv = nullptr; + createdDivElement = nullptr; continue; } + if (HTMLEditUtils::IsList(atCurNode.GetContainer())) { // If we don't use CSS, add a content to list element: they have to // be inside another list, i.e., >= second level of nesting. // XXX AlignContentsInAllTableCellsAndListItems() handles only list // item elements and table cells. Is it intentional? Why don't // we need to align contents in other type blocks? - rv = MOZ_KnownLive(HTMLEditorRef()) - .AlignContentsInAllTableCellsAndListItems( - MOZ_KnownLive(*listOrListItemElement), aAlignType); + nsresult rv = AlignContentsInAllTableCellsAndListItems( + MOZ_KnownLive(*listOrListItemElement), aAlignType); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } - curDiv = nullptr; + createdDivElement = nullptr; continue; } - // Clear out curDiv so that we don't put nodes after this one into it + + // Clear out createdDivElement so that we don't put nodes after this one + // into it } // Need to make a div to put things in if we haven't already, or if this // node doesn't go in div we used earlier. - if (!curDiv || transitionList[indexOfTransitionList]) { + if (!createdDivElement || transitionList[indexOfTransitionList]) { // First, check that our element can contain a div. - if (!HTMLEditorRef().CanContainTag(*atCurNode.GetContainer(), - *nsGkAtoms::div)) { - // Cancelled + if (!CanContainTag(*atCurNode.GetContainer(), *nsGkAtoms::div)) { + // XXX Why do we return NS_OK here rather than returning error or + // doing continue? return NS_OK; } SplitNodeResult splitNodeResult = - MOZ_KnownLive(HTMLEditorRef()) - .MaybeSplitAncestorsForInsertWithTransaction(*nsGkAtoms::div, - atCurNode); + MaybeSplitAncestorsForInsertWithTransaction(*nsGkAtoms::div, + atCurNode); if (NS_WARN_IF(splitNodeResult.Failed())) { return splitNodeResult.Rv(); } - curDiv = MOZ_KnownLive(HTMLEditorRef()) - .CreateNodeWithTransaction(*nsGkAtoms::div, - splitNodeResult.SplitPoint()); - if (NS_WARN_IF(!CanHandleEditAction())) { + createdDivElement = CreateNodeWithTransaction( + *nsGkAtoms::div, splitNodeResult.SplitPoint()); + if (NS_WARN_IF(Destroyed())) { return NS_ERROR_EDITOR_DESTROYED; } - if (NS_WARN_IF(!curDiv)) { + if (NS_WARN_IF(!createdDivElement)) { return NS_ERROR_FAILURE; } // Remember our new block for postprocessing - HTMLEditorRef().TopLevelEditSubActionDataRef().mNewBlockElement = curDiv; + TopLevelEditSubActionDataRef().mNewBlockElement = createdDivElement; // Set up the alignment on the div - rv = MOZ_KnownLive(HTMLEditorRef()) - .SetBlockElementAlign( - *curDiv, aAlignType, - HTMLEditor::EditTarget::OnlyDescendantsExceptTable); + nsresult rv = + SetBlockElementAlign(*createdDivElement, aAlignType, + EditTarget::OnlyDescendantsExceptTable); if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) { return NS_ERROR_EDITOR_DESTROYED; } @@ -6597,10 +6611,9 @@ nsresult HTMLEditRules::AlignContentsAtSelection(const nsAString& aAlignType) { } // Tuck the node into the end of the active div - rv = MOZ_KnownLive(HTMLEditorRef()) - .MoveNodeToEndWithTransaction(MOZ_KnownLive(*curNode->AsContent()), - *curDiv); - if (NS_WARN_IF(!CanHandleEditAction())) { + nsresult rv = MoveNodeToEndWithTransaction( + MOZ_KnownLive(*curNode->AsContent()), *createdDivElement); + if (NS_WARN_IF(Destroyed())) { return NS_ERROR_EDITOR_DESTROYED; } if (NS_WARN_IF(NS_FAILED(rv))) { diff --git a/editor/libeditor/HTMLEditRules.h b/editor/libeditor/HTMLEditRules.h index af570d04428c..14cac1ea3aff 100644 --- a/editor/libeditor/HTMLEditRules.h +++ b/editor/libeditor/HTMLEditRules.h @@ -113,19 +113,6 @@ class HTMLEditRules : public TextEditRules { */ MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult DidDeleteSelection(); - /** - * Called before aligning contents around Selection. This method actually - * sets align attributes to align contents. - * - * @param aAlignType New align attribute value where the contents - * should be aligned to. - * @param aCancel Returns true if the operation is canceled. - * @param aHandled Returns true if the edit action is handled. - */ - MOZ_CAN_RUN_SCRIPT - nsresult WillAlign(const nsAString& aAlignType, bool* aCancel, - bool* aHandled); - /** * Called before changing absolute positioned element to static positioned. * This method actually changes the position property of nearest absolute @@ -191,20 +178,6 @@ class HTMLEditRules : public TextEditRules { MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult DidAbsolutePosition(); - /** - * AlignContentsAtSelection() aligns contents around Selection to aAlignType. - * This creates AutoSelectionRestorer. Therefore, even if this returns - * NS_OK, CanHandleEditAction() may return false if the editor is destroyed - * during restoring the Selection. So, every caller needs to check if - * CanHandleEditAction() returns true before modifying the DOM tree or - * changing Selection. - * - * @param aAlignType New align attribute value where the contents - * should be aligned to. - */ - MOZ_CAN_RUN_SCRIPT - MOZ_MUST_USE nsresult AlignContentsAtSelection(const nsAString& aAlignType); - nsresult AppendInnerFormatNodes(nsTArray>& aArray, nsINode* aNode); nsresult GetFormatString(nsINode* aNode, nsAString& outFormat); diff --git a/editor/libeditor/HTMLEditor.cpp b/editor/libeditor/HTMLEditor.cpp index 6c088ad92f71..318855ff8264 100644 --- a/editor/libeditor/HTMLEditor.cpp +++ b/editor/libeditor/HTMLEditor.cpp @@ -2205,28 +2205,9 @@ nsresult HTMLEditor::AlignAsAction(const nsAString& aAlignType, return NS_ERROR_NOT_INITIALIZED; } - // Protect the edit rules object from dying - RefPtr rules(mRules); - - AutoPlaceholderBatch treatAsOneTransaction(*this); - AutoEditSubActionNotifier startToHandleEditSubAction( - *this, EditSubAction::eSetOrClearAlignment, nsIEditor::eNext); - - bool cancel, handled; - - // Find out if the selection is collapsed: - EditSubActionInfo subActionInfo(EditSubAction::eSetOrClearAlignment); - subActionInfo.alignType = &aAlignType; - nsresult rv = rules->WillDoAction(subActionInfo, &cancel, &handled); - if (cancel || NS_FAILED(rv)) { - return EditorBase::ToGenericNSResult(rv); - } - - rv = rules->DidDoAction(subActionInfo, rv); - if (NS_WARN_IF(NS_FAILED(rv))) { - return EditorBase::ToGenericNSResult(rv); - } - return NS_OK; + EditActionResult result = AlignAsSubAction(aAlignType); + NS_WARNING_ASSERTION(result.Succeeded(), "AlignAsSubAction() failed"); + return EditorBase::ToGenericNSResult(result.Rv()); } Element* HTMLEditor::GetElementOrParentByTagName(const nsAtom& aTagName, diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h index 9fd7fdfc694e..243150cbd027 100644 --- a/editor/libeditor/HTMLEditor.h +++ b/editor/libeditor/HTMLEditor.h @@ -259,8 +259,8 @@ class HTMLEditor final : public TextEditor, MOZ_CAN_RUN_SCRIPT nsresult SetParagraphFormatAsAction( const nsAString& aParagraphFormat, nsIPrincipal* aPrincipal = nullptr); - nsresult AlignAsAction(const nsAString& aAlignType, - nsIPrincipal* aPrincipal = nullptr); + MOZ_CAN_RUN_SCRIPT nsresult AlignAsAction(const nsAString& aAlignType, + nsIPrincipal* aPrincipal = nullptr); MOZ_CAN_RUN_SCRIPT nsresult RemoveListAsAction( const nsAString& aListType, nsIPrincipal* aPrincipal = nullptr); @@ -2523,6 +2523,52 @@ class HTMLEditor final : public TextEditor, SetBlockElementAlign(Element& aBlockOrHRElement, const nsAString& aAlignType, EditTarget aEditTarget); + /** + * AlignContentsAtSelectionWithEmptyDivElement() inserts new `
` element + * at `Selection` to align selected contents. This returns as "handled" + * if this modifies `Selection` so that callers shouldn't modify `Selection` + * in such case especially when using AutoSelectionRestorer. + * + * @param aAlignType New align attribute value where the contents + * should be aligned to. + */ + MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE EditActionResult + AlignContentsAtSelectionWithEmptyDivElement(const nsAString& aAlignType); + + /** + * AlignNodesAndDescendants() make contents of nodes in aArrayOfNodes and + * their descendants aligned to aAlignType. + * + * @param aAlignType New align attribute value where the contents + * should be aligned to. + */ + MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult + AlignNodesAndDescendants(nsTArray>& aArrayOfNodes, + const nsAString& aAlignType); + + /** + * AlignContentsAtSelection() aligns contents around Selection to aAlignType. + * This creates AutoSelectionRestorer. Therefore, even if this returns + * NS_OK, we might have been destroyed. So, every caller needs to check if + * Destroyed() returns false before modifying the DOM tree or changing + * Selection. + * NOTE: Call AlignAsSubAction() instead. + * + * @param aAlignType New align attribute value where the contents + * should be aligned to. + */ + MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult + AlignContentsAtSelection(const nsAString& aAlignType); + + /** + * AlignAsSubAction() handles "align" command with `Selection`. + * + * @param aAlignType New align attribute value where the contents + * should be aligned to. + */ + MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE EditActionResult + AlignAsSubAction(const nsAString& aAlignType); + protected: // Called by helper classes. virtual void OnStartToHandleTopLevelEditSubAction( EditSubAction aEditSubAction, nsIEditor::EDirection aDirection) override; diff --git a/editor/nsIHTMLEditor.idl b/editor/nsIHTMLEditor.idl index 2aba22a346d8..d0d650317b3b 100644 --- a/editor/nsIHTMLEditor.idl +++ b/editor/nsIHTMLEditor.idl @@ -307,6 +307,7 @@ interface nsIHTMLEditor : nsISupports * Document me! * */ + [can_run_script] void align(in AString aAlign); /** From a8504046a4c3c4a7305eae745d60f75a1fe03d12 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Mon, 9 Sep 2019 05:21:04 +0000 Subject: [PATCH 14/24] Bug 1574852 - part 87: Move `HTMLEditRules::SelectionEndpointInNode()` to `HTMLEditor` r=m_kato Differential Revision: https://phabricator.services.mozilla.com/D44791 --HG-- extra : moz-landing-system : lando --- editor/libeditor/HTMLEditRules.cpp | 45 ++++++++++-------------------- editor/libeditor/HTMLEditRules.h | 2 -- editor/libeditor/HTMLEditor.h | 6 ++++ 3 files changed, 21 insertions(+), 32 deletions(-) diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index dfa098ed6eb7..df6f88c630fc 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -10017,14 +10017,8 @@ nsresult HTMLEditRules::RemoveEmptyNodesInChangedRange() { // These node types are candidates if selection is not in them. If // it is one of these, don't delete if selection inside. This is so // we can create empty headings, etc., for the user to type into. - bool isSelectionEndInNode; - rv = SelectionEndpointInNode(node, &isSelectionEndInNode); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - if (!isSelectionEndInNode) { - isCandidate = true; - } + isCandidate = !HTMLEditorRef().StartOrEndOfSelectionRangesIsIn( + *node->AsContent()); } } @@ -10098,25 +10092,18 @@ nsresult HTMLEditRules::RemoveEmptyNodesInChangedRange() { return NS_OK; } -nsresult HTMLEditRules::SelectionEndpointInNode(nsINode* aNode, bool* aResult) { - MOZ_ASSERT(IsEditorDataAvailable()); +bool HTMLEditor::StartOrEndOfSelectionRangesIsIn(nsIContent& aContent) const { + MOZ_ASSERT(IsEditActionDataAvailable()); - NS_ENSURE_TRUE(aNode && aResult, NS_ERROR_NULL_POINTER); - - *aResult = false; - - uint32_t rangeCount = SelectionRefPtr()->RangeCount(); - for (uint32_t rangeIdx = 0; rangeIdx < rangeCount; ++rangeIdx) { - RefPtr range = SelectionRefPtr()->GetRangeAt(rangeIdx); + for (uint32_t i = 0; i < SelectionRefPtr()->RangeCount(); ++i) { + nsRange* range = SelectionRefPtr()->GetRangeAt(i); nsINode* startContainer = range->GetStartContainer(); if (startContainer) { - if (aNode == startContainer) { - *aResult = true; - return NS_OK; + if (&aContent == startContainer) { + return true; } - if (EditorUtils::IsDescendantOf(*startContainer, *aNode)) { - *aResult = true; - return NS_OK; + if (EditorUtils::IsDescendantOf(*startContainer, aContent)) { + return true; } } nsINode* endContainer = range->GetEndContainer(); @@ -10124,17 +10111,15 @@ nsresult HTMLEditRules::SelectionEndpointInNode(nsINode* aNode, bool* aResult) { continue; } if (endContainer) { - if (aNode == endContainer) { - *aResult = true; - return NS_OK; + if (&aContent == endContainer) { + return true; } - if (EditorUtils::IsDescendantOf(*endContainer, *aNode)) { - *aResult = true; - return NS_OK; + if (EditorUtils::IsDescendantOf(*endContainer, aContent)) { + return true; } } } - return NS_OK; + return false; } nsresult HTMLEditor::LiftUpListItemElement( diff --git a/editor/libeditor/HTMLEditRules.h b/editor/libeditor/HTMLEditRules.h index 14cac1ea3aff..864522f61da0 100644 --- a/editor/libeditor/HTMLEditRules.h +++ b/editor/libeditor/HTMLEditRules.h @@ -249,8 +249,6 @@ class HTMLEditRules : public TextEditRules { MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult RemoveEmptyNodesInChangedRange(); - nsresult SelectionEndpointInNode(nsINode* aNode, bool* aResult); - /** * ConfirmSelectionInBody() makes sure that Selection is in editor root * element typically element (see HTMLEditor::UpdateRootElement()) diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h index 243150cbd027..6840ffc19fc9 100644 --- a/editor/libeditor/HTMLEditor.h +++ b/editor/libeditor/HTMLEditor.h @@ -2569,6 +2569,12 @@ class HTMLEditor final : public TextEditor, MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE EditActionResult AlignAsSubAction(const nsAString& aAlignType); + /** + * StartOrEndOfSelectionRangesIsIn() returns true if start or end of one + * of selection ranges is in aContent. + */ + bool StartOrEndOfSelectionRangesIsIn(nsIContent& aContent) const; + protected: // Called by helper classes. virtual void OnStartToHandleTopLevelEditSubAction( EditSubAction aEditSubAction, nsIEditor::EDirection aDirection) override; From 9a7cb5f3c88403d634667fa54b755c94705f3f16 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Mon, 9 Sep 2019 10:09:38 +0000 Subject: [PATCH 15/24] Bug 1574852 - part 88: Move `HTMLEditRules::FindNearEditableNode()` to `HTMLEditor` r=m_kato Differential Revision: https://phabricator.services.mozilla.com/D44792 --HG-- extra : moz-landing-system : lando --- editor/libeditor/HTMLEditRules.cpp | 46 +++++++++++++++--------------- editor/libeditor/HTMLEditRules.h | 17 ----------- editor/libeditor/HTMLEditor.h | 17 +++++++++++ 3 files changed, 40 insertions(+), 40 deletions(-) diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index df6f88c630fc..ca00210fcc20 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -223,6 +223,10 @@ template Element* HTMLEditor::GetInvisibleBRElementAt( const EditorDOMPoint& aPoint); template Element* HTMLEditor::GetInvisibleBRElementAt( const EditorRawDOMPoint& aPoint); +template nsIContent* HTMLEditor::FindNearEditableContent( + const EditorDOMPoint& aPoint, nsIEditor::EDirection aDirection); +template nsIContent* HTMLEditor::FindNearEditableContent( + const EditorRawDOMPoint& aPoint, nsIEditor::EDirection aDirection); HTMLEditRules::HTMLEditRules() : mHTMLEditor(nullptr), mInitialized(false) { mIsHTMLEditRules = true; @@ -9850,7 +9854,7 @@ nsresult HTMLEditRules::AdjustSelection(nsIEditor::EDirection aAction) { // look for a nearby text node. // prefer the correct direction. - nearNode = FindNearEditableNode(point, aAction); + nearNode = HTMLEditorRef().FindNearEditableContent(point, aAction); if (!nearNode) { return NS_OK; } @@ -9869,25 +9873,21 @@ nsresult HTMLEditRules::AdjustSelection(nsIEditor::EDirection aAction) { } template -nsIContent* HTMLEditRules::FindNearEditableNode( +nsIContent* HTMLEditor::FindNearEditableContent( const EditorDOMPointBase& aPoint, nsIEditor::EDirection aDirection) { - MOZ_ASSERT(IsEditorDataAvailable()); - - if (NS_WARN_IF(!aPoint.IsSet())) { - return nullptr; - } + MOZ_ASSERT(IsEditActionDataAvailable()); MOZ_ASSERT(aPoint.IsSetAndValid()); - nsIContent* nearNode = nullptr; + nsIContent* editableContent = nullptr; if (aDirection == nsIEditor::ePrevious) { - nearNode = HTMLEditorRef().GetPreviousEditableHTMLNode(aPoint); - if (!nearNode) { + editableContent = GetPreviousEditableHTMLNode(aPoint); + if (!editableContent) { return nullptr; // Not illegal. } } else { - nearNode = HTMLEditorRef().GetNextEditableHTMLNode(aPoint); - if (NS_WARN_IF(!nearNode)) { + editableContent = GetNextEditableHTMLNode(aPoint); + if (NS_WARN_IF(!editableContent)) { // Perhaps, illegal because the node pointed by aPoint isn't editable // and nobody of previous nodes is editable. return nullptr; @@ -9896,32 +9896,32 @@ nsIContent* HTMLEditRules::FindNearEditableNode( // scan in the right direction until we find an eligible text node, // but don't cross any breaks, images, or table elements. - // XXX This comment sounds odd. |nearNode| may have already crossed breaks - // and/or images. - while (nearNode && !(EditorBase::IsTextNode(nearNode) || - TextEditUtils::IsBreak(nearNode) || - HTMLEditUtils::IsImage(nearNode))) { + // XXX This comment sounds odd. editableContent may have already crossed + // breaks and/or images if they are non-editable. + while (editableContent && !EditorBase::IsTextNode(editableContent) && + !TextEditUtils::IsBreak(editableContent) && + !HTMLEditUtils::IsImage(editableContent)) { if (aDirection == nsIEditor::ePrevious) { - nearNode = HTMLEditorRef().GetPreviousEditableHTMLNode(*nearNode); - if (NS_WARN_IF(!nearNode)) { + editableContent = GetPreviousEditableHTMLNode(*editableContent); + if (NS_WARN_IF(!editableContent)) { return nullptr; } } else { - nearNode = HTMLEditorRef().GetNextEditableHTMLNode(*nearNode); - if (NS_WARN_IF(!nearNode)) { + editableContent = GetNextEditableHTMLNode(*editableContent); + if (NS_WARN_IF(!editableContent)) { return nullptr; } } } // don't cross any table elements - if (HTMLEditor::NodesInDifferentTableElements(*nearNode, + if (HTMLEditor::NodesInDifferentTableElements(*editableContent, *aPoint.GetContainer())) { return nullptr; } // otherwise, ok, we have found a good spot to put the selection - return nearNode; + return editableContent; } // static diff --git a/editor/libeditor/HTMLEditRules.h b/editor/libeditor/HTMLEditRules.h index 864522f61da0..b55400a906aa 100644 --- a/editor/libeditor/HTMLEditRules.h +++ b/editor/libeditor/HTMLEditRules.h @@ -219,23 +219,6 @@ class HTMLEditRules : public TextEditRules { MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult AdjustSelection(nsIEditor::EDirection aAction); - /** - * FindNearEditableNode() tries to find an editable node near aPoint. - * - * @param aPoint The DOM point where to start to search from. - * @param aDirection If nsIEditor::ePrevious is set, this searches an - * editable node from next nodes. Otherwise, from - * previous nodes. - * @return If found, returns non-nullptr. Otherwise, nullptr. - * Note that if found node is in different table element, - * this returns nullptr. - * And also if aDirection is not nsIEditor::ePrevious, - * the result may be the node pointed by aPoint. - */ - template - nsIContent* FindNearEditableNode(const EditorDOMPointBase& aPoint, - nsIEditor::EDirection aDirection); - /** * RemoveEmptyNodesInChangedRange() removes all empty nodes in * TopLevelEditSubActionData::mChangedRange. However, if mail-cite node has diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h index 6840ffc19fc9..7ffb86f3a0d9 100644 --- a/editor/libeditor/HTMLEditor.h +++ b/editor/libeditor/HTMLEditor.h @@ -2575,6 +2575,23 @@ class HTMLEditor final : public TextEditor, */ bool StartOrEndOfSelectionRangesIsIn(nsIContent& aContent) const; + /** + * FindNearEditableContent() tries to find an editable node near aPoint. + * + * @param aPoint The DOM point where to start to search from. + * @param aDirection If nsIEditor::ePrevious is set, this searches an + * editable node from next nodes. Otherwise, from + * previous nodes. + * @return If found, returns non-nullptr. Otherwise, nullptr. + * Note that if found node is in different table element, + * this returns nullptr. + * And also if aDirection is not nsIEditor::ePrevious, + * the result may be the node pointed by aPoint. + */ + template + nsIContent* FindNearEditableContent(const EditorDOMPointBase& aPoint, + nsIEditor::EDirection aDirection); + protected: // Called by helper classes. virtual void OnStartToHandleTopLevelEditSubAction( EditSubAction aEditSubAction, nsIEditor::EDirection aDirection) override; From e4076e2b2f370e8be5205cfe30d30f9a3430f283 Mon Sep 17 00:00:00 2001 From: Mike Hommey Date: Tue, 10 Sep 2019 02:55:03 +0000 Subject: [PATCH 16/24] Bug 1579156 - Fix binding tests after bug 1573566. r=chmanchester The build system has no clue that there is something to build in dom/bindings/test. It's currently all handled via make rules generated by the backend, but ideally, this would all be handled by the frontend emitting appropriate GeneratedFiles and Sources objects. In the meanwhile, we just force the make backend to recurse through dom/bindings/test. Differential Revision: https://phabricator.services.mozilla.com/D45124 --HG-- extra : moz-landing-system : lando --- python/mozbuild/mozbuild/backend/recursivemake.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/python/mozbuild/mozbuild/backend/recursivemake.py b/python/mozbuild/mozbuild/backend/recursivemake.py index 9baf334504df..872cd0f21c60 100644 --- a/python/mozbuild/mozbuild/backend/recursivemake.py +++ b/python/mozbuild/mozbuild/backend/recursivemake.py @@ -1827,3 +1827,9 @@ class RecursiveMakeBackend(CommonBackend): webidls_mk = mozpath.join(bindings_dir, 'webidlsrcs.mk') with self._write_file(webidls_mk) as fh: mk.dump(fh, removal_guard=False) + + # Add the test directory to the compile graph. + if self.environment.substs.get('ENABLE_TESTS'): + self._compile_graph[mozpath.join( + mozpath.relpath(bindings_dir, self.environment.topobjdir), + 'test', 'target-objects')] From 4d1c6efb73fa5774f3feeda79ed2382489723df7 Mon Sep 17 00:00:00 2001 From: Mike Shal Date: Mon, 9 Sep 2019 23:56:10 +0000 Subject: [PATCH 17/24] Bug 1579969 - Don't add export dependency on .cargo/config when testing; r=glandium Bug 1578254 added a dependency on .cargo/config to export, which happens to work in the python/mozbuild/mozbuild/test/backend/test_build.py test cases because DEPTH=., and make finds the checked-in file .cargo/config in topsrcdir because VPATH points there. After removing VPATH in bug 1496746, the test no longer finds the file in topsrcdir, and doesn't have a rule to build it since that exists in the top-level moz.build file. Differential Revision: https://phabricator.services.mozilla.com/D45261 --HG-- extra : moz-landing-system : lando --- config/recurse.mk | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/recurse.mk b/config/recurse.mk index 91cbc2a1d68c..7e979e47bf59 100644 --- a/config/recurse.mk +++ b/config/recurse.mk @@ -203,7 +203,9 @@ $(addprefix build/unix/stdc++compat/,target host) build/clang-plugin/host: confi # export, which ensures it exists before recursing the rust targets, tricking # Make into keeping them early. $(rust_targets): $(DEPTH)/.cargo/config +ifndef TEST_MOZBUILD export:: $(DEPTH)/.cargo/config +endif # When building gtest as part of the build (LINK_GTEST_DURING_COMPILE), # force the build system to get to it first, so that it can be linked From c6f647e9c9764d0f5e92f8c06ec4f82e01b0704f Mon Sep 17 00:00:00 2001 From: Dzmitry Malyshau Date: Tue, 10 Sep 2019 02:04:32 +0000 Subject: [PATCH 18/24] Bug 1579318 - Switch mozangle dependency of Wrench to 0.2 r=jrmuizel Differential Revision: https://phabricator.services.mozilla.com/D44934 --HG-- extra : moz-landing-system : lando --- gfx/wr/Cargo.lock | 41 ++++++++++------------------ gfx/wr/direct-composition/Cargo.toml | 2 +- gfx/wr/servo-tidy.toml | 2 -- gfx/wr/webrender/Cargo.toml | 2 +- gfx/wr/wrench/Cargo.toml | 2 +- 5 files changed, 18 insertions(+), 31 deletions(-) diff --git a/gfx/wr/Cargo.lock b/gfx/wr/Cargo.lock index af7a494d4ea0..b9f38edc437d 100644 --- a/gfx/wr/Cargo.lock +++ b/gfx/wr/Cargo.lock @@ -174,6 +174,9 @@ dependencies = [ name = "cc" version = "1.0.15" source = "registry+https://github.com/rust-lang/crates.io-index" +dependencies = [ + "rayon 1.0.1 (registry+https://github.com/rust-lang/crates.io-index)", +] [[package]] name = "cfg-if" @@ -420,7 +423,7 @@ version = "0.1.0" dependencies = [ "euclid 0.20.0 (registry+https://github.com/rust-lang/crates.io-index)", "gleam 0.6.17 (registry+https://github.com/rust-lang/crates.io-index)", - "mozangle 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)", + "mozangle 0.2.7 (registry+https://github.com/rust-lang/crates.io-index)", "webrender 0.60.0", "winapi 0.3.6 (registry+https://github.com/rust-lang/crates.io-index)", "winit 0.19.1 (registry+https://github.com/rust-lang/crates.io-index)", @@ -592,17 +595,17 @@ dependencies = [ [[package]] name = "gl_generator" -version = "0.9.0" +version = "0.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ - "khronos_api 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)", + "khronos_api 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)", - "xml-rs 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)", + "xml-rs 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] name = "gl_generator" -version = "0.11.0" +version = "0.13.1" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "khronos_api 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -781,11 +784,6 @@ dependencies = [ "winapi-build 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)", ] -[[package]] -name = "khronos_api" -version = "2.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" - [[package]] name = "khronos_api" version = "3.1.0" @@ -939,12 +937,13 @@ dependencies = [ [[package]] name = "mozangle" -version = "0.1.7" +version = "0.2.7" source = "registry+https://github.com/rust-lang/crates.io-index" dependencies = [ "cc 1.0.15 (registry+https://github.com/rust-lang/crates.io-index)", - "gl_generator 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)", + "gl_generator 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)", + "walkdir 2.2.8 (registry+https://github.com/rust-lang/crates.io-index)", ] [[package]] @@ -1834,7 +1833,7 @@ dependencies = [ "libc 0.2.58 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)", "malloc_size_of_derive 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)", - "mozangle 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)", + "mozangle 0.2.7 (registry+https://github.com/rust-lang/crates.io-index)", "num-traits 0.2.4 (registry+https://github.com/rust-lang/crates.io-index)", "plane-split 0.14.1 (registry+https://github.com/rust-lang/crates.io-index)", "png 0.15.0 (registry+https://github.com/rust-lang/crates.io-index)", @@ -1995,7 +1994,7 @@ dependencies = [ "image 0.22.1 (registry+https://github.com/rust-lang/crates.io-index)", "lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)", "log 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)", - "mozangle 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)", + "mozangle 0.2.7 (registry+https://github.com/rust-lang/crates.io-index)", "osmesa-src 0.1.1 (git+https://github.com/servo/osmesa-src)", "osmesa-sys 0.1.2 (registry+https://github.com/rust-lang/crates.io-index)", "ron 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)", @@ -2050,14 +2049,6 @@ name = "xdg" version = "2.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" -[[package]] -name = "xml-rs" -version = "0.7.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -dependencies = [ - "bitflags 1.0.3 (registry+https://github.com/rust-lang/crates.io-index)", -] - [[package]] name = "xml-rs" version = "0.8.0" @@ -2148,7 +2139,7 @@ dependencies = [ "checksum generic-array 0.12.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3c0f28c2f5bfb5960175af447a2da7c18900693738343dc896ffbcabd9839592" "checksum gif 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ff3414b424657317e708489d2857d9575f4403698428b040b609b9d1c1a84a2c" "checksum gl_generator 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)" = "39a23d5e872a275135d66895d954269cf5e8661d234eb1c2480f4ce0d586acbd" -"checksum gl_generator 0.9.0 (registry+https://github.com/rust-lang/crates.io-index)" = "7a795170cbd85b5a7baa58d6d7525cae6a03e486859860c220f7ebbbdd379d0a" +"checksum gl_generator 0.13.1 (registry+https://github.com/rust-lang/crates.io-index)" = "ca98bbde17256e02d17336a6bdb5a50f7d0ccacee502e191d3e3d0ec2f96f84a" "checksum gleam 0.6.17 (registry+https://github.com/rust-lang/crates.io-index)" = "7f46fd8874e043ffac0d638ed1567a2584f7814f6d72b4db37ab1689004a26c4" "checksum glutin 0.21.0 (registry+https://github.com/rust-lang/crates.io-index)" = "cb26027a84c3b9e1949ef0df0b6a3db8d0c124243a5c161ea25c7def90cb1474" "checksum glutin_egl_sys 0.1.3 (registry+https://github.com/rust-lang/crates.io-index)" = "23f48987ab6cb2b61ad903b59e54a2fd0c380a7baff68cffd6826b69a73dd326" @@ -2166,7 +2157,6 @@ dependencies = [ "checksum itoa 0.4.1 (registry+https://github.com/rust-lang/crates.io-index)" = "c069bbec61e1ca5a596166e55dfe4773ff745c3d16b700013bcaff9a6df2c682" "checksum jpeg-decoder 0.1.14 (registry+https://github.com/rust-lang/crates.io-index)" = "0dfe27a6c0dabd772d0f9b9f8701c4ca12c4d1eebcadf2be1f6f70396f6a1434" "checksum kernel32-sys 0.2.2 (registry+https://github.com/rust-lang/crates.io-index)" = "7507624b29483431c0ba2d82aece8ca6cdba9382bff4ddd0f7490560c056098d" -"checksum khronos_api 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "037ab472c33f67b5fbd3e9163a2645319e5356fcd355efa6d4eb7fff4bbcb554" "checksum khronos_api 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "e2db585e1d738fc771bf08a151420d3ed193d9d895a36df7f6f8a9456b911ddc" "checksum lazy_static 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)" = "76f033c7ad61445c5b347c7382dd1237847eb1bce590fe50365dcb33d546be73" "checksum lazy_static 1.3.0 (registry+https://github.com/rust-lang/crates.io-index)" = "bc5729f27f159ddd61f4df6228e827e86643d4d3e7c32183cb30a1c08f604a14" @@ -2187,7 +2177,7 @@ dependencies = [ "checksum mio 0.6.16 (registry+https://github.com/rust-lang/crates.io-index)" = "71646331f2619b1026cc302f87a2b8b648d5c6dd6937846a16cc8ce0f347f432" "checksum mio-extras 2.0.5 (registry+https://github.com/rust-lang/crates.io-index)" = "46e73a04c2fa6250b8d802134d56d554a9ec2922bf977777c805ea5def61ce40" "checksum miow 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "8c1f2f3b1cf331de6896aabf6e9d55dca90356cc9960cca7eaaf408a355ae919" -"checksum mozangle 0.1.7 (registry+https://github.com/rust-lang/crates.io-index)" = "45a8a18a41cfab0fde25cc2f43ea89064d211a0fbb33225b8ff93ab20406e0e7" +"checksum mozangle 0.2.7 (registry+https://github.com/rust-lang/crates.io-index)" = "75a61b5a06b6f362eb45590ddf2643c255768a7039bcde1dc70320b97e7f9651" "checksum net2 0.2.32 (registry+https://github.com/rust-lang/crates.io-index)" = "9044faf1413a1057267be51b5afba8eb1090bd2231c693664aa1db716fe1eae0" "checksum nix 0.12.1 (registry+https://github.com/rust-lang/crates.io-index)" = "319fffb13b63c0f4ff5a4e1c97566e7e741561ff5d03bf8bbf11653454bbd70b" "checksum nix 0.14.1 (registry+https://github.com/rust-lang/crates.io-index)" = "6c722bee1037d430d0f8e687bbdbf222f27cc6e4e68d5caf630857bb2b6dbdce" @@ -2301,7 +2291,6 @@ dependencies = [ "checksum ws2_32-sys 0.2.1 (registry+https://github.com/rust-lang/crates.io-index)" = "d59cefebd0c892fa2dd6de581e937301d8552cb44489cdff035c6187cb63fa5e" "checksum x11-dl 2.18.3 (registry+https://github.com/rust-lang/crates.io-index)" = "940586acb859ea05c53971ac231685799a7ec1dee66ac0bccc0e6ad96e06b4e3" "checksum xdg 2.2.0 (registry+https://github.com/rust-lang/crates.io-index)" = "d089681aa106a86fade1b0128fb5daf07d5867a509ab036d99988dec80429a57" -"checksum xml-rs 0.7.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3c1cb601d29fe2c2ac60a2b2e5e293994d87a1f6fa9687a31a15270f909be9c2" "checksum xml-rs 0.8.0 (registry+https://github.com/rust-lang/crates.io-index)" = "541b12c998c5b56aa2b4e6f18f03664eef9a4fd0a246a55594efae6cc2d964b5" "checksum yaml-rust 0.3.5 (registry+https://github.com/rust-lang/crates.io-index)" = "e66366e18dc58b46801afbf2ca7661a9f59cc8c5962c29892b6039b4f86fa992" "checksum yaml-rust 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "95acf0db5515d07da9965ec0e0ba6cc2d825e2caeb7303b66ca441729801254e" diff --git a/gfx/wr/direct-composition/Cargo.toml b/gfx/wr/direct-composition/Cargo.toml index d21934a36c7b..ce8d598b002e 100644 --- a/gfx/wr/direct-composition/Cargo.toml +++ b/gfx/wr/direct-composition/Cargo.toml @@ -8,7 +8,7 @@ edition = "2018" [target.'cfg(windows)'.dependencies] euclid = "0.20" gleam = "0.6.2" -mozangle = {version = "0.1", features = ["egl"]} +mozangle = {version = "0.2.7", features = ["egl"]} webrender = {path = "../webrender"} winapi = {version = "0.3", features = ["winerror", "d3d11", "dcomp"]} winit = "0.19" diff --git a/gfx/wr/servo-tidy.toml b/gfx/wr/servo-tidy.toml index 13189aa7ba28..30bfde55233d 100644 --- a/gfx/wr/servo-tidy.toml +++ b/gfx/wr/servo-tidy.toml @@ -8,7 +8,6 @@ check-alphabetical-order = false packages = [ "crossbeam-utils", "gl_generator", - "khronos_api", "lazy_static", "nix", "percent-encoding", @@ -17,7 +16,6 @@ packages = [ "winapi", "core-graphics", "core-text", - "xml-rs", "yaml-rust", ] diff --git a/gfx/wr/webrender/Cargo.toml b/gfx/wr/webrender/Cargo.toml index cde7c0f582a6..f7abc1713b40 100644 --- a/gfx/wr/webrender/Cargo.toml +++ b/gfx/wr/webrender/Cargo.toml @@ -54,7 +54,7 @@ ws = { optional = true, version = "0.9" } svg_fmt = "0.4" [dev-dependencies] -mozangle = "0.1" +mozangle = "0.2" rand = "0.4" [target.'cfg(any(target_os = "android", all(unix, not(target_os = "macos"))))'.dependencies] diff --git a/gfx/wr/wrench/Cargo.toml b/gfx/wr/wrench/Cargo.toml index a0424ef129da..60be1bdcc417 100644 --- a/gfx/wr/wrench/Cargo.toml +++ b/gfx/wr/wrench/Cargo.toml @@ -42,7 +42,7 @@ headless = [ "osmesa-sys", "osmesa-src" ] [target.'cfg(target_os = "windows")'.dependencies] dwrote = "0.9" -mozangle = {version = "0.1.5", features = ["egl"]} +mozangle = {version = "0.2", features = ["egl"]} [target.'cfg(all(unix, not(target_os = "android")))'.dependencies] font-loader = "0.7" From 8cc57c8048d615a8f7b688c5b274450b65212925 Mon Sep 17 00:00:00 2001 From: Mats Palmgren Date: Tue, 10 Sep 2019 03:36:29 +0000 Subject: [PATCH 19/24] Bug 1248227 - Add crashtest. r=dholbert Differential Revision: https://phabricator.services.mozilla.com/D45278 --HG-- extra : moz-landing-system : lando --- layout/generic/crashtests/1248227.html | 13 +++++++++++++ layout/generic/crashtests/crashtests.list | 1 + 2 files changed, 14 insertions(+) create mode 100644 layout/generic/crashtests/1248227.html diff --git a/layout/generic/crashtests/1248227.html b/layout/generic/crashtests/1248227.html new file mode 100644 index 000000000000..2b2f3ff98249 --- /dev/null +++ b/layout/generic/crashtests/1248227.html @@ -0,0 +1,13 @@ + + + + + + +
+ +
+
+
+ + diff --git a/layout/generic/crashtests/crashtests.list b/layout/generic/crashtests/crashtests.list index d2bf6128a64f..b66c889cf527 100644 --- a/layout/generic/crashtests/crashtests.list +++ b/layout/generic/crashtests/crashtests.list @@ -653,6 +653,7 @@ load 1233191.html load 1233607.html load 1234701-1.html load 1234701-2.html +load 1248227.html load 1271765.html pref(layout.css.xul-box-display-values.content.enabled,true) asserts(2) asserts-if(Android,1) load 1272983-1.html # bug 586628 pref(layout.css.xul-box-display-values.content.enabled,true) asserts(2) asserts-if(Android,1) load 1272983-2.html # bug 586628 From a2c5d981ee553814915ed32ca4e6819791d07c21 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Tue, 10 Sep 2019 03:38:04 +0000 Subject: [PATCH 20/24] Bug 1574852 - part 89: Move `HTMLEditRules::AdjustSelection()` to `HTMLEditor` r=m_kato Differential Revision: https://phabricator.services.mozilla.com/D44793 --HG-- extra : moz-landing-system : lando --- editor/libeditor/HTMLEditRules.cpp | 253 +++++++++++++++-------------- editor/libeditor/HTMLEditRules.h | 10 -- editor/libeditor/HTMLEditor.h | 11 ++ 3 files changed, 142 insertions(+), 132 deletions(-) diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index ca00210fcc20..1da985b0f85c 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -618,7 +618,8 @@ nsresult HTMLEditRules::AfterEditInner() { // did it. if (!HTMLEditorRef() .TopLevelEditSubActionDataRef() - .mDidDeleteEmptyParentBlocks) { + .mDidDeleteEmptyParentBlocks && + SelectionRefPtr()->IsCollapsed()) { switch (HTMLEditorRef().GetTopLevelEditSubAction()) { case EditSubAction::eInsertText: case EditSubAction::eInsertTextComingFromIME: @@ -627,8 +628,13 @@ nsresult HTMLEditRules::AfterEditInner() { case EditSubAction::eInsertParagraphSeparator: case EditSubAction::ePasteHTMLContent: case EditSubAction::eInsertHTMLSource: - rv = AdjustSelection( - HTMLEditorRef().GetDirectionOfTopLevelEditSubAction()); + // XXX AdjustCaretPositionAndEnsurePaddingBRElement() intentionally + // does not create padding `
` element for empty editor. + // Investigate which is better that whether this should does it + // or wait MaybeCreatePaddingBRElementForEmptyEditor(). + rv = MOZ_KnownLive(HTMLEditorRef()) + .AdjustCaretPositionAndEnsurePaddingBRElement( + HTMLEditorRef().GetDirectionOfTopLevelEditSubAction()); if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } @@ -9708,70 +9714,62 @@ void HTMLEditRules::CheckInterlinePosition() { } } -nsresult HTMLEditRules::AdjustSelection(nsIEditor::EDirection aAction) { - MOZ_ASSERT(IsEditorDataAvailable()); +nsresult HTMLEditor::AdjustCaretPositionAndEnsurePaddingBRElement( + nsIEditor::EDirection aDirectionAndAmount) { + MOZ_ASSERT(IsEditActionDataAvailable()); + MOZ_ASSERT(SelectionRefPtr()->IsCollapsed()); - // if the selection isn't collapsed, do nothing. - // moose: one thing to do instead is check for the case of - // only a single break selected, and collapse it. Good thing? Beats me. - if (!SelectionRefPtr()->IsCollapsed()) { - return NS_OK; - } - - // get the (collapsed) selection location EditorDOMPoint point(EditorBase::GetStartPoint(*SelectionRefPtr())); if (NS_WARN_IF(!point.IsSet())) { return NS_ERROR_FAILURE; } - // are we in an editable node? - while (!HTMLEditorRef().IsEditable(point.GetContainer())) { - // scan up the tree until we find an editable place to be + // If selection start is not editable, climb up the tree until editable one. + while (!IsEditable(point.GetContainer())) { point.Set(point.GetContainer()); if (NS_WARN_IF(!point.IsSet())) { return NS_ERROR_FAILURE; } } - // make sure we aren't in an empty block - user will see no cursor. If this - // is happening, put a
in the block if allowed. - RefPtr theblock = HTMLEditorRef().GetBlock(*point.GetContainer()); - - if (theblock && HTMLEditorRef().IsEditable(theblock)) { - bool isEmptyNode; - nsresult rv = - HTMLEditorRef().IsEmptyNode(theblock, &isEmptyNode, false, false); - if (NS_WARN_IF(NS_FAILED(rv))) { - return rv; - } - // check if br can go into the destination node - if (isEmptyNode && - HTMLEditorRef().CanContainTag(*point.GetContainer(), *nsGkAtoms::br)) { - Element* rootElement = HTMLEditorRef().GetRoot(); - if (NS_WARN_IF(!rootElement)) { - return NS_ERROR_FAILURE; + // If caret is in empty block element, we need to insert a `
` element + // because the block should have one-line height. + if (RefPtr blockElement = GetBlock(*point.GetContainer())) { + if (IsEditable(blockElement)) { + bool isEmptyNode; + nsresult rv = IsEmptyNode(blockElement, &isEmptyNode, false, false); + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; } - if (point.GetContainer() == rootElement) { - // Our root node is completely empty. Don't add a
here. - // AfterEditInner() will add one for us when it calls - // TextEditor::MaybeCreatePaddingBRElementForEmptyEditor()! + if (isEmptyNode && CanContainTag(*point.GetContainer(), *nsGkAtoms::br)) { + Element* bodyOrDocumentElement = GetRoot(); + if (NS_WARN_IF(!bodyOrDocumentElement)) { + return NS_ERROR_FAILURE; + } + if (point.GetContainer() == bodyOrDocumentElement) { + // Our root node is completely empty. Don't add a
here. + // AfterEditInner() will add one for us when it calls + // TextEditor::MaybeCreatePaddingBRElementForEmptyEditor(). + // XXX This kind of dependency between methods makes us spaghetti. + // Let's handle it here later. + // XXX This looks odd check. If active editing host is not a + // ``, what are we doing? + return NS_OK; + } + CreateElementResult createPaddingBRResult = + InsertPaddingBRElementForEmptyLastLineWithTransaction(point); + if (NS_WARN_IF(createPaddingBRResult.Failed())) { + return createPaddingBRResult.Rv(); + } return NS_OK; } - - // we know we can skip the rest of this routine given the cirumstance - CreateElementResult createPaddingBRResult = - MOZ_KnownLive(HTMLEditorRef()) - .InsertPaddingBRElementForEmptyLastLineWithTransaction(point); - if (NS_WARN_IF(createPaddingBRResult.Failed())) { - return createPaddingBRResult.Rv(); - } - return NS_OK; } } - // are we in a text node? + // XXX Perhaps, we should do something if we're in a data node but not + // a text node. if (point.IsInTextNode()) { - return NS_OK; // we LIKE it when we are in a text node. that RULZ + return NS_OK; } // Do we need to insert a padding
element for empty last line? We do @@ -9780,96 +9778,107 @@ nsresult HTMLEditRules::AdjustSelection(nsIEditor::EDirection aAction) { // 2) prior node is a br AND // 3) that br is not visible - nsCOMPtr nearNode = - HTMLEditorRef().GetPreviousEditableHTMLNode(point); - if (nearNode) { - // is nearNode also a descendant of same block? - RefPtr block = HTMLEditorRef().GetBlock(*point.GetContainer()); - RefPtr nearBlock = HTMLEditorRef().GetBlockNodeParent(nearNode); - if (block && block == nearBlock) { - if (nearNode && TextEditUtils::IsBreak(nearNode)) { - if (!HTMLEditorRef().IsVisibleBRElement(nearNode)) { - // need to insert special moz BR. Why? Because if we don't - // the user will see no new line for the break. Also, things - // like table cells won't grow in height. - CreateElementResult createPaddingBRResult = - MOZ_KnownLive(HTMLEditorRef()) - .InsertPaddingBRElementForEmptyLastLineWithTransaction(point); - if (NS_WARN_IF(createPaddingBRResult.Failed())) { - return createPaddingBRResult.Rv(); - } - point.Set(createPaddingBRResult.GetNewNode()); - // Selection stays *before* padding
element for empty last - // line, sticking to it. - ErrorResult error; - SelectionRefPtr()->SetInterlinePosition(true, error); - if (NS_WARN_IF(!CanHandleEditAction())) { - error.SuppressException(); - return NS_ERROR_EDITOR_DESTROYED; - } - NS_WARNING_ASSERTION(!error.Failed(), + if (nsCOMPtr previousEditableContent = + GetPreviousEditableHTMLNode(point)) { + RefPtr blockElementAtCaret = GetBlock(*point.GetContainer()); + RefPtr blockElementParentAtPreviousEditableContent = + GetBlockNodeParent(previousEditableContent); + // If previous editable content of caret is in same block and a `
` + // element, we need to adjust interline position. + if (blockElementAtCaret && + blockElementAtCaret == blockElementParentAtPreviousEditableContent && + previousEditableContent && + TextEditUtils::IsBreak(previousEditableContent)) { + // If it's an invisible `
` element, we need to insert a padding + // `
` element for making empty line have one-line height. + if (!IsVisibleBRElement(previousEditableContent)) { + CreateElementResult createPaddingBRResult = + InsertPaddingBRElementForEmptyLastLineWithTransaction(point); + if (NS_WARN_IF(createPaddingBRResult.Failed())) { + return createPaddingBRResult.Rv(); + } + point.Set(createPaddingBRResult.GetNewNode()); + // Selection stays *before* padding `
` element for empty last + // line, sticking to it. + IgnoredErrorResult ignoredError; + SelectionRefPtr()->SetInterlinePosition(true, ignoredError); + if (NS_WARN_IF(Destroyed())) { + return NS_ERROR_EDITOR_DESTROYED; + } + NS_WARNING_ASSERTION(!ignoredError.Failed(), + "Failed to set interline position"); + ErrorResult error; + SelectionRefPtr()->Collapse(point, error); + if (NS_WARN_IF(Destroyed())) { + error.SuppressException(); + return NS_ERROR_EDITOR_DESTROYED; + } + if (NS_WARN_IF(error.Failed())) { + return error.StealNSResult(); + } + } + // If it's a visible `
` element and next editable content is a + // padding `
` element, we need to set interline position. + else if (nsIContent* nextEditableContentInBlock = + GetNextEditableHTMLNodeInBlock(*previousEditableContent)) { + if (EditorBase::IsPaddingBRElementForEmptyLastLine( + *nextEditableContentInBlock)) { + // Make it stick to the padding `
` element so that it will be + // on blank line. + IgnoredErrorResult ignoredError; + SelectionRefPtr()->SetInterlinePosition(true, ignoredError); + NS_WARNING_ASSERTION(!ignoredError.Failed(), "Failed to set interline position"); - error = NS_OK; - SelectionRefPtr()->Collapse(point, error); - if (NS_WARN_IF(!CanHandleEditAction())) { - error.SuppressException(); - return NS_ERROR_EDITOR_DESTROYED; - } - if (NS_WARN_IF(error.Failed())) { - return error.StealNSResult(); - } - } else { - nsCOMPtr nextNode = - HTMLEditorRef().GetNextEditableHTMLNodeInBlock(*nearNode); - if (nextNode && - EditorBase::IsPaddingBRElementForEmptyLastLine(*nextNode)) { - // Selection between a
element and a padding
element for - // empty last line. Make it stick to the padding
element so - // that it will be on blank line. - IgnoredErrorResult ignoredError; - SelectionRefPtr()->SetInterlinePosition(true, ignoredError); - NS_WARNING_ASSERTION(!ignoredError.Failed(), - "Failed to set interline position"); - } } } } } - // we aren't in a textnode: are we adjacent to text or a break or an image? - nearNode = HTMLEditorRef().GetPreviousEditableHTMLNodeInBlock(point); - if (nearNode && - (TextEditUtils::IsBreak(nearNode) || EditorBase::IsTextNode(nearNode) || - HTMLEditUtils::IsImage(nearNode) || - nearNode->IsHTMLElement(nsGkAtoms::hr))) { - // this is a good place for the caret to be - return NS_OK; + // If previous editable content in same block is `
`, text node, `` + // or `
`, current caret position is fine. + if (nsIContent* previousEditableContentInBlock = + GetPreviousEditableHTMLNodeInBlock(point)) { + if (TextEditUtils::IsBreak(previousEditableContentInBlock) || + EditorBase::IsTextNode(previousEditableContentInBlock) || + HTMLEditUtils::IsImage(previousEditableContentInBlock) || + previousEditableContentInBlock->IsHTMLElement(nsGkAtoms::hr)) { + return NS_OK; + } } - nearNode = HTMLEditorRef().GetNextEditableHTMLNodeInBlock(point); - if (nearNode && - (TextEditUtils::IsBreak(nearNode) || EditorBase::IsTextNode(nearNode) || - nearNode->IsAnyOfHTMLElements(nsGkAtoms::img, nsGkAtoms::hr))) { - return NS_OK; // this is a good place for the caret to be + // If next editable content in same block is `
`, text node, `` or + // `
`, current caret position is fine. + if (nsIContent* nextEditableContentInBlock = + GetNextEditableHTMLNodeInBlock(point)) { + if (TextEditUtils::IsBreak(nextEditableContentInBlock) || + EditorBase::IsTextNode(nextEditableContentInBlock) || + nextEditableContentInBlock->IsAnyOfHTMLElements(nsGkAtoms::img, + nsGkAtoms::hr)) { + return NS_OK; + } } - // look for a nearby text node. - // prefer the correct direction. - nearNode = HTMLEditorRef().FindNearEditableContent(point, aAction); - if (!nearNode) { + // Otherwise, look for a near editable content towards edit action direction. + + // If there is no editable content, keep current caret position. + nsIContent* nearEditableContent = + FindNearEditableContent(point, aDirectionAndAmount); + if (!nearEditableContent) { return NS_OK; } - EditorDOMPoint pt = HTMLEditorRef().GetGoodCaretPointFor(*nearNode, aAction); + EditorDOMPoint pointToPutCaret = + GetGoodCaretPointFor(*nearEditableContent, aDirectionAndAmount); + if (NS_WARN_IF(!pointToPutCaret.IsSet())) { + return NS_ERROR_FAILURE; + } ErrorResult error; - SelectionRefPtr()->Collapse(pt, error); - if (NS_WARN_IF(!CanHandleEditAction())) { + SelectionRefPtr()->Collapse(pointToPutCaret, error); + if (NS_WARN_IF(Destroyed())) { error.SuppressException(); return NS_ERROR_EDITOR_DESTROYED; } - if (NS_WARN_IF(error.Failed())) { - return error.StealNSResult(); - } - return NS_OK; + NS_WARNING_ASSERTION(!error.Failed(), "Selection::Collapse() failed"); + return error.StealNSResult(); } template diff --git a/editor/libeditor/HTMLEditRules.h b/editor/libeditor/HTMLEditRules.h index b55400a906aa..0de409d226f6 100644 --- a/editor/libeditor/HTMLEditRules.h +++ b/editor/libeditor/HTMLEditRules.h @@ -209,16 +209,6 @@ class HTMLEditRules : public TextEditRules { void CheckInterlinePosition(); - /** - * AdjustSelection() may adjust Selection range to nearest editable content. - * Despite of the name, this may change the DOM tree. If it needs to create - * a
to put caret, this tries to create a
element. - * - * @param aAction Maybe used to look for a good point to put caret. - */ - MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult - AdjustSelection(nsIEditor::EDirection aAction); - /** * RemoveEmptyNodesInChangedRange() removes all empty nodes in * TopLevelEditSubActionData::mChangedRange. However, if mail-cite node has diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h index 7ffb86f3a0d9..7c190e88d076 100644 --- a/editor/libeditor/HTMLEditor.h +++ b/editor/libeditor/HTMLEditor.h @@ -2592,6 +2592,17 @@ class HTMLEditor final : public TextEditor, nsIContent* FindNearEditableContent(const EditorDOMPointBase& aPoint, nsIEditor::EDirection aDirection); + /** + * AdjustCaretPositionAndEnsurePaddingBRElement() may adjust caret + * position to nearest editable content and if padding `
` element is + * necessary at caret position, this creates it. + * + * @param aDirectionAndAmount Direction of the edit action. + */ + MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult + AdjustCaretPositionAndEnsurePaddingBRElement( + nsIEditor::EDirection aDirectionAndAmount); + protected: // Called by helper classes. virtual void OnStartToHandleTopLevelEditSubAction( EditSubAction aEditSubAction, nsIEditor::EDirection aDirection) override; From 24f1d9a9b32d00e8f1344ce268175f59d32657c4 Mon Sep 17 00:00:00 2001 From: Masayuki Nakano Date: Tue, 10 Sep 2019 03:52:44 +0000 Subject: [PATCH 21/24] Bug 1574852 - part 90: Move `HTMLEditRules::ConfirmSelectionInBody()` to `HTMLEditor` r=m_kato Differential Revision: https://phabricator.services.mozilla.com/D44794 --HG-- extra : moz-landing-system : lando --- editor/libeditor/HTMLEditRules.cpp | 43 ++++++++++++++++++++---------- editor/libeditor/HTMLEditRules.h | 11 -------- editor/libeditor/HTMLEditor.h | 8 ++++++ 3 files changed, 37 insertions(+), 25 deletions(-) diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp index 1da985b0f85c..676fe56600d9 100644 --- a/editor/libeditor/HTMLEditRules.cpp +++ b/editor/libeditor/HTMLEditRules.cpp @@ -365,10 +365,14 @@ nsresult HTMLEditRules::BeforeEdit() { } // Check that selection is in subtree defined by body node - nsresult rv = ConfirmSelectionInBody(); + nsresult rv = + MOZ_KnownLive(HTMLEditorRef()).EnsureSelectionInBodyOrDocumentElement(); if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) { return NS_ERROR_EDITOR_DESTROYED; } + NS_WARNING_ASSERTION( + NS_SUCCEEDED(rv), + "EnsureSelectionInBodyOrDocumentElement() failed, but ignored"); return NS_OK; } @@ -419,11 +423,14 @@ nsresult HTMLEditRules::AfterEdit() { nsresult HTMLEditRules::AfterEditInner() { MOZ_ASSERT(IsEditorDataAvailable()); - nsresult rv = ConfirmSelectionInBody(); + nsresult rv = + MOZ_KnownLive(HTMLEditorRef()).EnsureSelectionInBodyOrDocumentElement(); if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) { return NS_ERROR_EDITOR_DESTROYED; } - NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Failed to normalize Selection"); + NS_WARNING_ASSERTION( + NS_SUCCEEDED(rv), + "EnsureSelectionInBodyOrDocumentElement() failed, but ignored"); switch (HTMLEditorRef().GetTopLevelEditSubAction()) { case EditSubAction::eReplaceHeadWithHTMLSource: case EditSubAction::eCreatePaddingBRElementForEmptyEditor: @@ -10292,12 +10299,12 @@ nsresult HTMLEditor::DestroyListStructureRecursively(Element& aListElement) { return NS_OK; } -nsresult HTMLEditRules::ConfirmSelectionInBody() { - MOZ_ASSERT(IsEditorDataAvailable()); +nsresult HTMLEditor::EnsureSelectionInBodyOrDocumentElement() { + MOZ_ASSERT(IsEditActionDataAvailable()); - Element* rootElement = HTMLEditorRef().GetRoot(); - if (NS_WARN_IF(!rootElement)) { - return NS_ERROR_UNEXPECTED; + RefPtr bodyOrDocumentElement = GetRoot(); + if (NS_WARN_IF(!bodyOrDocumentElement)) { + return NS_ERROR_FAILURE; } EditorRawDOMPoint selectionStartPoint( @@ -10306,6 +10313,12 @@ nsresult HTMLEditRules::ConfirmSelectionInBody() { return NS_ERROR_FAILURE; } + // XXX This does wrong things. Web apps can put any elements as sibling + // of `` element. Therefore, this collapses `Selection` into + // the `` element which `HTMLDocument.body` is set to. So, + // this makes users impossible to modify content outside of the + // `` element even if caret is in an editing host. + // Check that selection start container is inside the element. // XXXsmaug this code is insane. nsINode* temp = selectionStartPoint.GetContainer(); @@ -10316,13 +10329,14 @@ nsresult HTMLEditRules::ConfirmSelectionInBody() { // If we aren't in the element, force the issue. if (!temp) { IgnoredErrorResult ignoredError; - SelectionRefPtr()->Collapse(RawRangeBoundary(rootElement, 0), ignoredError); - if (NS_WARN_IF(!CanHandleEditAction())) { + SelectionRefPtr()->Collapse(RawRangeBoundary(bodyOrDocumentElement, 0), + ignoredError); + if (NS_WARN_IF(Destroyed())) { return NS_ERROR_EDITOR_DESTROYED; } NS_WARNING_ASSERTION( !ignoredError.Failed(), - "Failed to collapse selection at start of the root element"); + "Selection::Collapse() with start of editing host failed, but ignored"); return NS_OK; } @@ -10342,13 +10356,14 @@ nsresult HTMLEditRules::ConfirmSelectionInBody() { // If we aren't in the element, force the issue. if (!temp) { IgnoredErrorResult ignoredError; - SelectionRefPtr()->Collapse(RawRangeBoundary(rootElement, 0), ignoredError); - if (NS_WARN_IF(!CanHandleEditAction())) { + SelectionRefPtr()->Collapse(RawRangeBoundary(bodyOrDocumentElement, 0), + ignoredError); + if (NS_WARN_IF(Destroyed())) { return NS_ERROR_EDITOR_DESTROYED; } NS_WARNING_ASSERTION( !ignoredError.Failed(), - "Failed to collapse selection at start of the root element"); + "Selection::Collapse() with start of editing host failed, but ignored"); } return NS_OK; diff --git a/editor/libeditor/HTMLEditRules.h b/editor/libeditor/HTMLEditRules.h index 0de409d226f6..893b5da7779a 100644 --- a/editor/libeditor/HTMLEditRules.h +++ b/editor/libeditor/HTMLEditRules.h @@ -222,17 +222,6 @@ class HTMLEditRules : public TextEditRules { MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult RemoveEmptyNodesInChangedRange(); - /** - * ConfirmSelectionInBody() makes sure that Selection is in editor root - * element typically element (see HTMLEditor::UpdateRootElement()) - * and only one Selection range. - * XXX This method is not necessary because even if selection is outside the - * element, elements outside the element should be - * editable, e.g., any element can be inserted siblings as element - * and other browsers allow to edit such elements. - */ - MOZ_MUST_USE nsresult ConfirmSelectionInBody(); - /** * DocumentModifiedWorker() is called by DocumentModified() either * synchronously or asynchronously. diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h index 7c190e88d076..9a27baf2cf71 100644 --- a/editor/libeditor/HTMLEditor.h +++ b/editor/libeditor/HTMLEditor.h @@ -2603,6 +2603,14 @@ class HTMLEditor final : public TextEditor, AdjustCaretPositionAndEnsurePaddingBRElement( nsIEditor::EDirection aDirectionAndAmount); + /** + * EnsureSelectionInBodyOrDocumentElement() collapse `Selection` to the + * primary `` element or document element when `Selection` crosses + * `` element's boundary. + */ + MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult + EnsureSelectionInBodyOrDocumentElement(); + protected: // Called by helper classes. virtual void OnStartToHandleTopLevelEditSubAction( EditSubAction aEditSubAction, nsIEditor::EDirection aDirection) override; From 6ac9c7bb88754cacd832c39930492291d2ed06ea Mon Sep 17 00:00:00 2001 From: Matthew Noorenberghe Date: Mon, 9 Sep 2019 22:42:32 +0000 Subject: [PATCH 22/24] Bug 1579512 - Don't include the real password in the markup of about:logins when masked. r=jaws Differential Revision: https://phabricator.services.mozilla.com/D45075 --HG-- extra : moz-landing-system : lando --- .../aboutlogins/content/aboutLogins.html | 1 + .../content/components/login-item.js | 23 ++++++++++++++++--- .../browser_aaa_eventTelemetry_run_first.js | 2 +- .../tests/browser/browser_openSite.js | 2 +- .../tests/browser/browser_updateLogin.js | 18 +++++++++++++-- .../aboutlogins/tests/browser/head.js | 12 +++++----- .../tests/chrome/test_login_item.html | 13 +++++++---- 7 files changed, 54 insertions(+), 17 deletions(-) diff --git a/browser/components/aboutlogins/content/aboutLogins.html b/browser/components/aboutlogins/content/aboutLogins.html index 7a5b3d0b0c2d..88bb54f13e9b 100644 --- a/browser/components/aboutlogins/content/aboutLogins.html +++ b/browser/components/aboutlogins/content/aboutLogins.html @@ -198,6 +198,7 @@
diff --git a/browser/components/aboutlogins/content/components/login-item.js b/browser/components/aboutlogins/content/components/login-item.js index 556dc2c37d37..95d87926a8c9 100644 --- a/browser/components/aboutlogins/content/components/login-item.js +++ b/browser/components/aboutlogins/content/components/login-item.js @@ -146,7 +146,8 @@ export default class LoginItem extends HTMLElement { this._title.textContent = this._login.title; this._originInput.defaultValue = this._login.origin || ""; this._usernameInput.defaultValue = this._login.username || ""; - this._passwordInput.defaultValue = this._login.password || ""; + // The password gets filled in _updatePasswordRevealState + if (this.dataset.editing) { this._usernameInput.removeAttribute("data-l10n-id"); this._usernameInput.placeholder = ""; @@ -246,7 +247,8 @@ export default class LoginItem extends HTMLElement { case "click": { let classList = event.currentTarget.classList; if (classList.contains("reveal-password-checkbox")) { - if (this._revealCheckbox.checked) { + // We prompt for the master password when entering edit mode already. + if (this._revealCheckbox.checked && !this.dataset.editing) { let masterPasswordAuth = await new Promise(resolve => { window.AboutLoginsUtils.promptForMasterPassword(resolve); }); @@ -344,6 +346,13 @@ export default class LoginItem extends HTMLElement { return; } if (classList.contains("edit-button")) { + let masterPasswordAuth = await new Promise(resolve => { + window.AboutLoginsUtils.promptForMasterPassword(resolve); + }); + if (!masterPasswordAuth) { + return; + } + this._toggleEditing(); this.render(); @@ -680,7 +689,15 @@ export default class LoginItem extends HTMLElement { let { checked } = this._revealCheckbox; let inputType = checked ? "text" : "password"; - this._passwordInput.setAttribute("type", inputType); + this._passwordInput.type = inputType; + // Don't include the password value in the attribute when it's supposed to be + // masked so that it's not trivial to bypass the Master Password prompt with + // the inspector in devtools. + let password = this._login.password || ""; + // We prompt for the master password before entering edit mode so we can use + // the password in the markup then. + this._passwordInput.defaultValue = + checked || this.dataset.editing ? password : " ".repeat(password.length); } } customElements.define("login-item", LoginItem); diff --git a/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js b/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js index ce4e72137cb9..9c7e80ccfe16 100644 --- a/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js +++ b/browser/components/aboutlogins/tests/browser/browser_aaa_eventTelemetry_run_first.js @@ -79,7 +79,7 @@ add_task(async function test_telemetry_events() { let promiseNewTab = BrowserTestUtils.waitForNewTab( gBrowser, - TEST_LOGIN2.origin + TEST_LOGIN2.origin + "/" ); await ContentTask.spawn(gBrowser.selectedBrowser, null, async function() { let loginItem = content.document.querySelector("login-item"); diff --git a/browser/components/aboutlogins/tests/browser/browser_openSite.js b/browser/components/aboutlogins/tests/browser/browser_openSite.js index 8c8aa45378ed..a747be387c05 100644 --- a/browser/components/aboutlogins/tests/browser/browser_openSite.js +++ b/browser/components/aboutlogins/tests/browser/browser_openSite.js @@ -16,7 +16,7 @@ add_task(async function setup() { add_task(async function test_launch_login_item() { let promiseNewTab = BrowserTestUtils.waitForNewTab( gBrowser, - TEST_LOGIN1.origin + TEST_LOGIN1.origin + "/" ); let browser = gBrowser.selectedBrowser; diff --git a/browser/components/aboutlogins/tests/browser/browser_updateLogin.js b/browser/components/aboutlogins/tests/browser/browser_updateLogin.js index 8c002093d79f..8298a8357d82 100644 --- a/browser/components/aboutlogins/tests/browser/browser_updateLogin.js +++ b/browser/components/aboutlogins/tests/browser/browser_updateLogin.js @@ -63,6 +63,10 @@ add_task(async function test_login_item() { async function test_discard_dialog(exitPoint) { editButton.click(); + await ContentTaskUtils.waitForCondition( + () => loginItem.dataset.editing, + "Entering edit mode" + ); await Promise.resolve(); usernameInput.value += "-undome"; @@ -102,7 +106,7 @@ add_task(async function test_login_item() { ); is( passwordInput.value, - login.password, + " ".repeat(login.password.length), "Password change should be reverted" ); is( @@ -118,6 +122,10 @@ add_task(async function test_login_item() { await test_discard_dialog(cancelButton); editButton.click(); + await ContentTaskUtils.waitForCondition( + () => loginItem.dataset.editing, + "Entering edit mode" + ); await Promise.resolve(); let revealCheckbox = loginItem.shadowRoot.querySelector( @@ -132,6 +140,8 @@ add_task(async function test_login_item() { usernameInput.value += "-saveme"; passwordInput.value += "-saveme"; + // Cache the value since it will change upon leaving edit mode. + let passwordInputValue = passwordInput.value; ok(loginItem.dataset.editing, "LoginItem should be in 'edit' mode"); let saveChangesButton = loginItem.shadowRoot.querySelector( @@ -145,7 +155,7 @@ add_task(async function test_login_item() { return ( updatedLogin && updatedLogin.username == usernameInput.value && - updatedLogin.password == passwordInput.value + updatedLogin.password == passwordInputValue ); }, "Waiting for corresponding login in login list to update"); @@ -164,6 +174,10 @@ add_task(async function test_login_item() { ); editButton.click(); + await ContentTaskUtils.waitForCondition( + () => loginItem.dataset.editing, + "Entering edit mode" + ); await Promise.resolve(); ok(loginItem.dataset.editing, "LoginItem should be in 'edit' mode"); diff --git a/browser/components/aboutlogins/tests/browser/head.js b/browser/components/aboutlogins/tests/browser/head.js index c1e548529840..26d97d990ffb 100644 --- a/browser/components/aboutlogins/tests/browser/head.js +++ b/browser/components/aboutlogins/tests/browser/head.js @@ -8,8 +8,8 @@ let nsLoginInfo = new Components.Constructor( ); let TEST_LOGIN1 = new nsLoginInfo( - "https://example.com/", - "https://example.com/", + "https://example.com", + "https://example.com", null, "user1", "pass1", @@ -17,8 +17,8 @@ let TEST_LOGIN1 = new nsLoginInfo( "password" ); let TEST_LOGIN2 = new nsLoginInfo( - "https://2.example.com/", - "https://2.example.com/", + "https://2.example.com", + "https://2.example.com", null, "user2", "pass2", @@ -27,8 +27,8 @@ let TEST_LOGIN2 = new nsLoginInfo( ); let TEST_LOGIN3 = new nsLoginInfo( - "https://breached.com/", - "https://breached.com/", + "https://breached.com", + "https://breached.com", null, "breachedLogin1", "pass3", diff --git a/browser/components/aboutlogins/tests/chrome/test_login_item.html b/browser/components/aboutlogins/tests/chrome/test_login_item.html index 10cdc42b1018..131c543a382b 100644 --- a/browser/components/aboutlogins/tests/chrome/test_login_item.html +++ b/browser/components/aboutlogins/tests/chrome/test_login_item.html @@ -89,7 +89,7 @@ add_task(async function test_set_login() { let usernameInput = gLoginItem.shadowRoot.querySelector("input[name='username']"); is(usernameInput.value, TEST_LOGIN_1.username, "username should be populated"); is(document.l10n.getAttributes(usernameInput).id, "about-logins-login-item-username", "username field should have default placeholder when not editing"); - is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, TEST_LOGIN_1.password, "password should be populated"); + is(gLoginItem.shadowRoot.querySelector("input[name='password']").value.length, TEST_LOGIN_1.password.length, "password mask text should be populated"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, TEST_LOGIN_1.timeCreated, "time-created should be populated"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, TEST_LOGIN_1.timePasswordChanged, "time-changed should be populated"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, TEST_LOGIN_1.timeLastUsed, "time-used should be populated"); @@ -126,6 +126,7 @@ add_task(async function test_set_login() { usernameInput.placeholder = "dummy placeholder"; gLoginItem.shadowRoot.querySelector(".edit-button").click(); + await asyncElementRendered(); is( document.l10n.getAttributes(usernameInput).id, null, @@ -160,6 +161,7 @@ add_task(async function test_edit_login() { usernameInput.placeholder = "dummy placeholder"; gLoginItem.shadowRoot.querySelector(".edit-button").click(); await asyncElementRendered(); + await asyncElementRendered(); ok(gLoginItem.dataset.editing, "loginItem should be in 'edit' mode"); ok(isHidden(gLoginItem.shadowRoot.querySelector(".edit-button")), "edit button should be hidden in 'edit' mode"); @@ -199,6 +201,7 @@ add_task(async function test_edit_login() { add_task(async function test_edit_login_cancel() { gLoginItem.setLogin(TEST_LOGIN_1); gLoginItem.shadowRoot.querySelector(".edit-button").click(); + await asyncElementRendered(); ok(gLoginItem.dataset.editing, "loginItem should be in 'edit' mode"); is(!!gLoginItem.dataset.isNewLogin, false, @@ -231,6 +234,8 @@ add_task(async function test_reveal_password_change_selected_login() { let editButton = gLoginItem.shadowRoot.querySelector(".edit-button"); editButton.click(); + await asyncElementRendered(); + ok(revealCheckbox.checked, "reveal-checkbox should remain checked when entering 'edit' mode"); gLoginItem.shadowRoot.querySelector(".cancel-button").click(); ok(!revealCheckbox.checked, "reveal-checkbox should be unchecked after canceling 'edit' mode"); @@ -307,7 +312,7 @@ add_task(async function test_different_login_modified() { is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, TEST_LOGIN_1.origin, "origin should be unchanged"); is(gLoginItem.shadowRoot.querySelector("input[name='username']").value, TEST_LOGIN_1.username, "username should be unchanged"); - is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, TEST_LOGIN_1.password, "password should be unchanged"); + is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(TEST_LOGIN_1.password.length), "password length should be unchanged"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, TEST_LOGIN_1.timeCreated, "time-created should be unchanged"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, TEST_LOGIN_1.timePasswordChanged, "time-changed should be unchanged"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, TEST_LOGIN_1.timeLastUsed, "time-used should be unchanged"); @@ -321,7 +326,7 @@ add_task(async function test_different_login_removed() { is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, TEST_LOGIN_1.origin, "origin should be unchanged"); is(gLoginItem.shadowRoot.querySelector("input[name='username']").value, TEST_LOGIN_1.username, "username should be unchanged"); - is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, TEST_LOGIN_1.password, "password should be unchanged"); + is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(TEST_LOGIN_1.password.length), "password length should be unchanged"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, TEST_LOGIN_1.timeCreated, "time-created should be unchanged"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, TEST_LOGIN_1.timePasswordChanged, "time-changed should be unchanged"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, TEST_LOGIN_1.timeLastUsed, "time-used should be unchanged"); @@ -335,7 +340,7 @@ add_task(async function test_login_modified() { is(gLoginItem.shadowRoot.querySelector("input[name='origin']").value, modifiedLogin.origin, "origin should be updated"); is(gLoginItem.shadowRoot.querySelector("input[name='username']").value, modifiedLogin.username, "username should be updated"); - is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, modifiedLogin.password, "password should be updated"); + is(gLoginItem.shadowRoot.querySelector("input[name='password']").value, " ".repeat(modifiedLogin.password.length), "password length should be updated"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-created")).args.timeCreated, modifiedLogin.timeCreated, "time-created should be updated"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-changed")).args.timeChanged, modifiedLogin.timePasswordChanged, "time-changed should be updated"); is(document.l10n.getAttributes(gLoginItem.shadowRoot.querySelector(".time-used")).args.timeUsed, modifiedLogin.timeLastUsed, "time-used should be updated"); From 9a8d3ce98285866f39e9c540d6a5014c4570689f Mon Sep 17 00:00:00 2001 From: Jared Wein Date: Tue, 10 Sep 2019 00:57:43 +0000 Subject: [PATCH 23/24] Bug 1579169 - Remove font-weight from fxaccount-email. r=MattN Differential Revision: https://phabricator.services.mozilla.com/D45271 --HG-- extra : moz-landing-system : lando --- .../aboutlogins/content/components/fxaccounts-button.css | 1 - 1 file changed, 1 deletion(-) diff --git a/browser/components/aboutlogins/content/components/fxaccounts-button.css b/browser/components/aboutlogins/content/components/fxaccounts-button.css index 139bf974cd57..03a0a455b24b 100644 --- a/browser/components/aboutlogins/content/components/fxaccounts-button.css +++ b/browser/components/aboutlogins/content/components/fxaccounts-button.css @@ -28,7 +28,6 @@ } .fxaccount-email { - font-weight: 600; font-size: .9em; vertical-align: middle; } From 081827e5486a2134b4b00b5da4efb57600d841ab Mon Sep 17 00:00:00 2001 From: Jared Wein Date: Tue, 10 Sep 2019 05:31:56 +0000 Subject: [PATCH 24/24] Bug 1579262 - Increase the margin-bottom on the field-labels. r=MattN Differential Revision: https://phabricator.services.mozilla.com/D45273 --HG-- extra : moz-landing-system : lando --- .../components/aboutlogins/content/components/login-item.css | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/browser/components/aboutlogins/content/components/login-item.css b/browser/components/aboutlogins/content/components/login-item.css index 8a01debd3c11..d8f35bf01213 100644 --- a/browser/components/aboutlogins/content/components/login-item.css +++ b/browser/components/aboutlogins/content/components/login-item.css @@ -148,7 +148,7 @@ input[type="url"][readOnly]:hover:active { display: block; font-size: smaller; color: var(--in-content-deemphasized-text); - margin-bottom: 5px; + margin-bottom: 8px; } :host([data-editing]) .detail-cell input:not([readOnly]):not([type="checkbox"]) {