From 0cc0120d766dc2d554b4f4db136b3b64c43b1859 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 14 May 2019 16:03:18 +0000 Subject: [PATCH 01/22] Bug 1549812 - ScrollFrameRectIntoView should handle the frame going away. r=mats ScrollToShowRect already considers that possibility, so not doing it on the caller is a bug. Ideally scroll observers shouldn't be able to run script, more to that in a second. Differential Revision: https://phabricator.services.mozilla.com/D31088 --HG-- extra : moz-landing-system : lando --- layout/base/PresShell.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp index 4171984903ef0..1880bf0872fa8 100644 --- a/layout/base/PresShell.cpp +++ b/layout/base/PresShell.cpp @@ -3583,8 +3583,14 @@ bool PresShell::ScrollFrameRectIntoView(nsIFrame* aFrame, const nsRect& aRect, targetRect = targetRect.Intersect(sf->GetScrolledRect()); } - ScrollToShowRect(this, sf, targetRect, aVertical, aHorizontal, - aScrollFlags); + { + AutoWeakFrame wf(container); + ScrollToShowRect(this, sf, targetRect, aVertical, aHorizontal, + aScrollFlags); + if (!wf.IsAlive()) { + return didScroll; + } + } nsPoint newPosition = sf->LastScrollDestination(); // If the scroll position increased, that means our content moved up, From 9880e651ac6e8e12b5b178ced74abd9ef2a92532 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 17 May 2019 03:25:02 +0000 Subject: [PATCH 02/22] Bug 1549812 - Don't run arbitrary script from AccessibleCaretManager callbacks. r=TYLin Instead, post the event for the next turn of the event loop. In this case, what killed the frame is ActionBarHandler.jsm via Selection.toString(). Depends on D31088 Differential Revision: https://phabricator.services.mozilla.com/D31089 --HG-- extra : moz-landing-system : lando --- layout/base/AccessibleCaretManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/layout/base/AccessibleCaretManager.cpp b/layout/base/AccessibleCaretManager.cpp index 774336cbb44d2..4aad63348abdc 100644 --- a/layout/base/AccessibleCaretManager.cpp +++ b/layout/base/AccessibleCaretManager.cpp @@ -1354,7 +1354,7 @@ void AccessibleCaretManager::DispatchCaretStateChangedEvent( __FUNCTION__, static_cast(init.mReason), init.mCollapsed, static_cast(init.mCaretVisible)); - (new AsyncEventDispatcher(doc, event))->RunDOMEventWhenSafe(); + (new AsyncEventDispatcher(doc, event))->PostDOMEvent(); } } // namespace mozilla From 8376a58be88802a39207f52ffcbcc9ee5bfc49cb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Fri, 17 May 2019 13:22:39 +0000 Subject: [PATCH 03/22] Bug 1549812 - Try to assert a bit harder about stuff not flushing under our nose. r=TYLin,mats I think these should hold, everything that runs under them should just schedule other stuff to some later date: * Synth mouse events -> scheduled as refresh driver observers. * Scroll events -> Scheduled as well. * Caret state change events -> Also scheduled after last patch. * IME and accessibility stuff -> I don't think they can reenter layout. We can always revert this if it causes troubles, plus it shouldn't crash on release so should be fine. Differential Revision: https://phabricator.services.mozilla.com/D31090 --HG-- extra : moz-landing-system : lando --- layout/base/AccessibleCaretManager.cpp | 20 ++++++++++++++++++++ layout/base/PresShell.cpp | 4 ++-- layout/generic/nsGfxScrollFrame.cpp | 5 +++-- 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/layout/base/AccessibleCaretManager.cpp b/layout/base/AccessibleCaretManager.cpp index 4aad63348abdc..84136430519c2 100644 --- a/layout/base/AccessibleCaretManager.cpp +++ b/layout/base/AccessibleCaretManager.cpp @@ -586,6 +586,11 @@ void AccessibleCaretManager::OnScrollStart() { AutoRestore saveAllowFlushingLayout(mAllowFlushingLayout); mAllowFlushingLayout = false; + Maybe assert; + if (mPresShell) { + assert.emplace(*mPresShell); + } + mIsScrollStarted = true; if (mFirstCaret->IsLogicallyVisible() || mSecondCaret->IsLogicallyVisible()) { @@ -603,6 +608,11 @@ void AccessibleCaretManager::OnScrollEnd() { AutoRestore saveAllowFlushingLayout(mAllowFlushingLayout); mAllowFlushingLayout = false; + Maybe assert; + if (mPresShell) { + assert.emplace(*mPresShell); + } + mIsScrollStarted = false; if (GetCaretMode() == CaretMode::Cursor) { @@ -633,6 +643,11 @@ void AccessibleCaretManager::OnScrollPositionChanged() { AutoRestore saveAllowFlushingLayout(mAllowFlushingLayout); mAllowFlushingLayout = false; + Maybe assert; + if (mPresShell) { + assert.emplace(*mPresShell); + } + if (mFirstCaret->IsLogicallyVisible() || mSecondCaret->IsLogicallyVisible()) { if (mIsScrollStarted) { // We don't want extra CaretStateChangedEvents dispatched when user is @@ -656,6 +671,11 @@ void AccessibleCaretManager::OnReflow() { AutoRestore saveAllowFlushingLayout(mAllowFlushingLayout); mAllowFlushingLayout = false; + Maybe assert; + if (mPresShell) { + assert.emplace(*mPresShell); + } + if (mFirstCaret->IsLogicallyVisible() || mSecondCaret->IsLogicallyVisible()) { AC_LOG("%s: UpdateCarets(RespectOldAppearance)", __FUNCTION__); UpdateCarets(UpdateCaretsHint::RespectOldAppearance); diff --git a/layout/base/PresShell.cpp b/layout/base/PresShell.cpp index 1880bf0872fa8..e7c09a8b302a6 100644 --- a/layout/base/PresShell.cpp +++ b/layout/base/PresShell.cpp @@ -9057,8 +9057,8 @@ void PresShell::WillDoReflow() { void PresShell::DidDoReflow(bool aInterruptible) { HandlePostedReflowCallbacks(aInterruptible); - nsCOMPtr docShell = mPresContext->GetDocShell(); - if (docShell) { + AutoAssertNoFlush noReentrantFlush(*this); + if (nsCOMPtr docShell = mPresContext->GetDocShell()) { DOMHighResTimeStamp now = GetPerformanceNowUnclamped(); docShell->NotifyReflowObservers(aInterruptible, mLastReflowStart, now); } diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp index a8c6a3529c063..56a01126c170d 100644 --- a/layout/generic/nsGfxScrollFrame.cpp +++ b/layout/generic/nsGfxScrollFrame.cpp @@ -2917,6 +2917,8 @@ void ScrollFrameHelper::ScrollToImpl(nsPoint aPt, const nsRect& aRange, ScheduleSyntheticMouseMove(); + PresShell::AutoAssertNoFlush noFlush(*mOuter->PresShell()); + { // scope the AutoScrollbarRepaintSuppression AutoScrollbarRepaintSuppression repaintSuppression(this, !schedulePaint); AutoWeakFrame weakFrame(mOuter); @@ -2946,8 +2948,7 @@ void ScrollFrameHelper::ScrollToImpl(nsPoint aPt, const nsRect& aRange, mListeners[i]->ScrollPositionDidChange(pt.x, pt.y); } - nsCOMPtr docShell = presContext->GetDocShell(); - if (docShell) { + if (nsCOMPtr docShell = presContext->GetDocShell()) { docShell->NotifyScrollObservers(); } } From b6a082b1670d67a48725cc5721c8e55bf9345015 Mon Sep 17 00:00:00 2001 From: Agi Sferro Date: Fri, 17 May 2019 13:43:44 +0000 Subject: [PATCH 04/22] Bug 1552328 - Add missing unregisterListener in GeckoViewProgress. r=droeh Differential Revision: https://phabricator.services.mozilla.com/D31538 --HG-- extra : moz-landing-system : lando --- mobile/android/modules/geckoview/GeckoViewProgress.jsm | 1 + 1 file changed, 1 insertion(+) diff --git a/mobile/android/modules/geckoview/GeckoViewProgress.jsm b/mobile/android/modules/geckoview/GeckoViewProgress.jsm index 30b7943b7d5ba..af8e83ddd9de2 100644 --- a/mobile/android/modules/geckoview/GeckoViewProgress.jsm +++ b/mobile/android/modules/geckoview/GeckoViewProgress.jsm @@ -169,6 +169,7 @@ class GeckoViewProgress extends GeckoViewModule { } Services.obs.removeObserver(this, "oop-frameloader-crashed"); + this.unregisterListener("GeckoView:FlushSessionState"); } onEvent(aEvent, aData, aCallback) { From 1324e8b03632043dc41b8bea6aa4bdfc4259a20e Mon Sep 17 00:00:00 2001 From: Agi Sferro Date: Fri, 17 May 2019 13:43:44 +0000 Subject: [PATCH 05/22] Bug 1550877 - Use correct E10SUtils in GeckoViewNavigation. r=snorp This fixes a crash in `browser.tabs.update` when used with WebExtension pages. Differential Revision: https://phabricator.services.mozilla.com/D31453 --HG-- extra : moz-landing-system : lando --- .../background-script.js | 7 +++ .../extension-page-update/manifest.json | 24 ++++++++++ .../extension-page-update/tab.html | 1 + .../extension-page-update/tabs.js | 1 + .../geckoview/test/WebExtensionTest.kt | 47 +++++++++++++++++++ .../modules/geckoview/GeckoViewNavigation.jsm | 2 +- 6 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/background-script.js create mode 100644 mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/manifest.json create mode 100644 mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/tab.html create mode 100644 mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/tabs.js diff --git a/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/background-script.js b/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/background-script.js new file mode 100644 index 0000000000000..aaf1b319d6a9b --- /dev/null +++ b/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/background-script.js @@ -0,0 +1,7 @@ +browser.runtime.onMessage.addListener(notify); + +function notify(message) { + if (message.action == "showTab") { + browser.tabs.update({url: "/tab.html"}); + } +} diff --git a/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/manifest.json b/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/manifest.json new file mode 100644 index 0000000000000..b493f0fd1b0be --- /dev/null +++ b/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/manifest.json @@ -0,0 +1,24 @@ +{ + "manifest_version": 2, + "name": "Mozilla Android Components - Tabs Update Test", + "version": "1.0", + "background": { + "scripts": ["background-script.js"] + }, + "content_scripts": [ + { + "matches": ["*://*.example.com/*"], + "js": ["tabs.js"], + "run_at": "document_idle" + } + ], + "web_accessible_resources": [ + "tab.html" + ], + "permissions": [ + "geckoViewAddons", + "nativeMessaging", + "tabs", + "" + ] +} diff --git a/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/tab.html b/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/tab.html new file mode 100644 index 0000000000000..ba7c290eb9eb9 --- /dev/null +++ b/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/tab.html @@ -0,0 +1 @@ +

Hello World!

\ No newline at end of file diff --git a/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/tabs.js b/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/tabs.js new file mode 100644 index 0000000000000..5765db1e77a8f --- /dev/null +++ b/mobile/android/geckoview/src/androidTest/assets/web_extensions/extension-page-update/tabs.js @@ -0,0 +1 @@ +browser.runtime.sendMessage({"action": "showTab"}); diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/WebExtensionTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/WebExtensionTest.kt index ac3b14a23e430..9b841ff6f4332 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/WebExtensionTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/WebExtensionTest.kt @@ -9,6 +9,7 @@ import org.mozilla.geckoview.test.rule.GeckoSessionTestRule.ReuseSession import android.support.test.filters.MediumTest import android.support.test.runner.AndroidJUnit4 +import org.hamcrest.core.StringEndsWith.endsWith import org.hamcrest.core.IsEqual.equalTo import org.json.JSONObject import org.junit.Assert @@ -17,7 +18,9 @@ import org.junit.Assert.fail import org.junit.Test import org.junit.runner.RunWith import org.mozilla.geckoview.* +import org.mozilla.geckoview.test.rule.GeckoSessionTestRule import org.mozilla.geckoview.test.rule.GeckoSessionTestRule.WithDevToolsAPI +import org.mozilla.geckoview.test.util.Callbacks import org.mozilla.geckoview.test.util.HttpBin import java.net.URI @@ -460,6 +463,50 @@ class WebExtensionTest : BaseSessionTest() { } } + @Test + fun loadWebExtensionPage() { + val extension = WebExtension("resource://android/assets/web_extensions/extension-page-update/") + sessionRule.waitForResult(sessionRule.runtime.registerWebExtension(extension)) + + mainSession.loadUri("http://example.com"); + + mainSession.waitUntilCalled(object : Callbacks.NavigationDelegate, Callbacks.ProgressDelegate { + @GeckoSessionTestRule.AssertCalled(count = 1) + override fun onLocationChange(session: GeckoSession, url: String?) { + assertThat("Url should load example.com first", + url, equalTo("http://example.com/")) + } + + @GeckoSessionTestRule.AssertCalled(count = 1) + override fun onPageStop(session: GeckoSession, success: Boolean) { + assertThat("Page should load successfully.", + success, equalTo(true)) + } + }) + + + var page: String? = null + var pageStop = GeckoResult() + + mainSession.delegateUntilTestEnd(object : Callbacks.NavigationDelegate, Callbacks.ProgressDelegate { + override fun onLocationChange(session: GeckoSession, url: String?) { + page = url + } + + override fun onPageStop(session: GeckoSession, success: Boolean) { + if (success && page != null && page!!.endsWith("/tab.html")) { + pageStop.complete(true) + } + } + }) + + // Make sure the page loaded successfully + sessionRule.waitForResult(pageStop) + + assertThat("Url should load WebExtension page", page, endsWith("/tab.html")) + sessionRule.waitForResult(sessionRule.runtime.unregisterWebExtension(extension)) + } + @Test fun badFileType() { testRegisterError("resource://android/bad/location/error", diff --git a/mobile/android/modules/geckoview/GeckoViewNavigation.jsm b/mobile/android/modules/geckoview/GeckoViewNavigation.jsm index ea548f0e5948c..abd0d1dd342e4 100644 --- a/mobile/android/modules/geckoview/GeckoViewNavigation.jsm +++ b/mobile/android/modules/geckoview/GeckoViewNavigation.jsm @@ -10,7 +10,7 @@ const {GeckoViewModule} = ChromeUtils.import("resource://gre/modules/GeckoViewMo const {XPCOMUtils} = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm"); XPCOMUtils.defineLazyModuleGetters(this, { - E10SUtils: "resource://gre/modules/sessionstore/Utils.jsm", + E10SUtils: "resource://gre/modules/E10SUtils.jsm", LoadURIDelegate: "resource://gre/modules/LoadURIDelegate.jsm", Services: "resource://gre/modules/Services.jsm", }); From ebeb369ef4ed2cd57488fb7006e4ccd7c8e36af2 Mon Sep 17 00:00:00 2001 From: Erica Wright Date: Thu, 16 May 2019 22:05:47 +0000 Subject: [PATCH 06/22] Bug 1551675 - remove the Standard category pref and depend on default values to compute standard settings. r=johannh The standard category pref is gone. When the default values change the standard category will change it's expectations to match. Differential Revision: https://phabricator.services.mozilla.com/D31138 --HG-- extra : moz-landing-system : lando --- browser/app/profile/firefox.js | 14 +- browser/components/BrowserGlue.jsm | 120 +++++++++--------- .../preferences/in-content/privacy.js | 36 +++++- .../browser_contentblocking_categories.js | 61 +++++---- modules/libpref/init/StaticPrefList.h | 3 - modules/libpref/init/all.js | 2 - 6 files changed, 126 insertions(+), 110 deletions(-) diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js index 7c5b94bdeba36..371a2fe0a46d9 100644 --- a/browser/app/profile/firefox.js +++ b/browser/app/profile/firefox.js @@ -1188,7 +1188,6 @@ pref("services.sync.prefs.sync.addons.ignoreUserEnabledChanges", true); // source, and this would propagate automatically to other, // uncompromised Sync-connected devices. pref("services.sync.prefs.sync.browser.contentblocking.category", true); -pref("services.sync.prefs.sync.browser.contentblocking.features.standard", true); pref("services.sync.prefs.sync.browser.contentblocking.features.strict", true); pref("services.sync.prefs.sync.browser.contentblocking.introCount", true); pref("services.sync.prefs.sync.browser.crashReports.unsubmittedCheck.autoSubmit2", true); @@ -1591,7 +1590,7 @@ pref("browser.contentblocking.control-center.ui.showAllowedLabels", false); pref("browser.contentblocking.cryptomining.preferences.ui.enabled", true); pref("browser.contentblocking.fingerprinting.preferences.ui.enabled", true); -// Possible values for browser.contentblocking.features.* prefs: +// Possible values for browser.contentblocking.features.strict pref: // Tracking Protection: // "tp": tracking protection enabled // "-tp": tracking protection disabled @@ -1610,17 +1609,8 @@ pref("browser.contentblocking.fingerprinting.preferences.ui.enabled", true); // "cookieBehavior2": cookie behaviour BEHAVIOR_REJECT // "cookieBehavior3": cookie behaviour BEHAVIOR_LIMIT_FOREIGN // "cookieBehavior4": cookie behaviour BEHAVIOR_REJECT_TRACKER -// One value from each section must be included in each browser.contentblocking.features.* pref. +// One value from each section must be included in the browser.contentblocking.features.strict pref. pref("browser.contentblocking.features.strict", "tp,tpPrivate,cookieBehavior4,cm,fp"); -// Enable blocking access to storage from tracking resources only in nightly -// and early beta. By default the value is "cookieBehavior0": BEHAVIOR_ACCEPT -// Enable cryptomining blocking in standard in nightly and early beta. -// Enable fingerprinting blocking in standard in nightly and early beta. -#ifdef EARLY_BETA_OR_EARLIER -pref("browser.contentblocking.features.standard", "-tp,tpPrivate,cookieBehavior4,cm,fp"); -#else -pref("browser.contentblocking.features.standard", "-tp,tpPrivate,cookieBehavior0,-cm,-fp"); -#endif // Enable the Report Breakage UI on Nightly and Beta but not on Release yet. #ifdef EARLY_BETA_OR_EARLIER diff --git a/browser/components/BrowserGlue.jsm b/browser/components/BrowserGlue.jsm index fb9e3165d9c8b..9242494670dd0 100644 --- a/browser/components/BrowserGlue.jsm +++ b/browser/components/BrowserGlue.jsm @@ -1039,8 +1039,8 @@ BrowserGlue.prototype = { Services.prefs.removeObserver("privacy.trackingprotection", this._matchCBCategory); Services.prefs.removeObserver("network.cookie.cookieBehavior", this._matchCBCategory); Services.prefs.removeObserver(ContentBlockingCategoriesPrefs.PREF_CB_CATEGORY, this._updateCBCategory); - Services.prefs.removeObserver("browser.contentblocking.features.standard", this._setPrefExpectations); - Services.prefs.removeObserver("browser.contentblocking.features.strict", this._setPrefExpectations); + Services.prefs.removeObserver("privacy.trackingprotection", this._setPrefExpectations); + Services.prefs.removeObserver("browser.contentblocking.features.strict", this._setPrefExpectationsAndUpdate); }, // runs on startup, before the first command line handler is invoked @@ -1391,7 +1391,7 @@ BrowserGlue.prototype = { // Set the default favicon size for UI views that use the page-icon protocol. PlacesUtils.favicons.setDefaultIconURIPreferredSize(16 * aWindow.devicePixelRatio); - this._setPrefExpectations(); + this._setPrefExpectationsAndUpdate(); this._matchCBCategory(); // This observes the entire privacy.trackingprotection.* pref tree. @@ -1399,8 +1399,8 @@ BrowserGlue.prototype = { Services.prefs.addObserver("network.cookie.cookieBehavior", this._matchCBCategory); Services.prefs.addObserver(ContentBlockingCategoriesPrefs.PREF_CB_CATEGORY, this._updateCBCategory); Services.prefs.addObserver("media.autoplay.default", this._updateAutoplayPref); - Services.prefs.addObserver("browser.contentblocking.features.standard", this._setPrefExpectations); - Services.prefs.addObserver("browser.contentblocking.features.strict", this._setPrefExpectations); + Services.prefs.addObserver("privacy.trackingprotection", this._setPrefExpectations); + Services.prefs.addObserver("browser.contentblocking.features.strict", this._setPrefExpectationsAndUpdate); }, _updateAutoplayPref() { @@ -1410,6 +1410,10 @@ BrowserGlue.prototype = { _setPrefExpectations() { ContentBlockingCategoriesPrefs.setPrefExpectations(); + }, + + _setPrefExpectationsAndUpdate() { + ContentBlockingCategoriesPrefs.setPrefExpectations(); ContentBlockingCategoriesPrefs.updateCBCategory(); }, @@ -3018,13 +3022,12 @@ BrowserGlue.prototype = { var ContentBlockingCategoriesPrefs = { PREF_CB_CATEGORY: "browser.contentblocking.category", PREF_STRICT_DEF: "browser.contentblocking.features.strict", - PREF_STANDARD_DEF: "browser.contentblocking.features.standard", switchingCategory: false, setPrefExpectations() { - // The prefs inside CATEGORY_PREFS are initial values, these values then get set. - // If the pref remains null, then it will expect the default value, - // but the UI will not respond correctly. + // The prefs inside CATEGORY_PREFS are initial values. + // If the pref remains null, then it will expect the default value. + // The "standard" category is defined as expecting all 5 default values. this.CATEGORY_PREFS = { strict: { "network.cookie.cookieBehavior": null, @@ -3041,58 +3044,51 @@ var ContentBlockingCategoriesPrefs = { "privacy.trackingprotection.cryptomining.enabled": null, }, }; - let types = ["strict", "standard"]; - for (let type of types) { - let rulesArray; - if (type == "strict") { - rulesArray = Services.prefs.getStringPref(this.PREF_STRICT_DEF).split(","); - } else { - rulesArray = Services.prefs.getStringPref(this.PREF_STANDARD_DEF).split(","); - } - for (let item of rulesArray) { - switch (item) { - case "tp": - this.CATEGORY_PREFS[type]["privacy.trackingprotection.enabled"] = true; - break; - case "-tp": - this.CATEGORY_PREFS[type]["privacy.trackingprotection.enabled"] = false; - break; - case "tpPrivate": - this.CATEGORY_PREFS[type]["privacy.trackingprotection.pbmode.enabled"] = true; - break; - case "-tpPrivate": - this.CATEGORY_PREFS[type]["privacy.trackingprotection.pbmode.enabled"] = false; - break; - case "fp": - this.CATEGORY_PREFS[type]["privacy.trackingprotection.fingerprinting.enabled"] = true; - break; - case "-fp": - this.CATEGORY_PREFS[type]["privacy.trackingprotection.fingerprinting.enabled"] = false; - break; - case "cm": - this.CATEGORY_PREFS[type]["privacy.trackingprotection.cryptomining.enabled"] = true; - break; - case "-cm": - this.CATEGORY_PREFS[type]["privacy.trackingprotection.cryptomining.enabled"] = false; - break; - case "cookieBehavior0": - this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_ACCEPT; - break; - case "cookieBehavior1": - this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_REJECT_FOREIGN; - break; - case "cookieBehavior2": - this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_REJECT; - break; - case "cookieBehavior3": - this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_LIMIT_FOREIGN; - break; - case "cookieBehavior4": - this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER; - break; - default: - Cu.reportError(`Error: Unknown rule observed ${item}`); - } + let type = "strict"; + let rulesArray = Services.prefs.getStringPref(this.PREF_STRICT_DEF).split(","); + for (let item of rulesArray) { + switch (item) { + case "tp": + this.CATEGORY_PREFS[type]["privacy.trackingprotection.enabled"] = true; + break; + case "-tp": + this.CATEGORY_PREFS[type]["privacy.trackingprotection.enabled"] = false; + break; + case "tpPrivate": + this.CATEGORY_PREFS[type]["privacy.trackingprotection.pbmode.enabled"] = true; + break; + case "-tpPrivate": + this.CATEGORY_PREFS[type]["privacy.trackingprotection.pbmode.enabled"] = false; + break; + case "fp": + this.CATEGORY_PREFS[type]["privacy.trackingprotection.fingerprinting.enabled"] = true; + break; + case "-fp": + this.CATEGORY_PREFS[type]["privacy.trackingprotection.fingerprinting.enabled"] = false; + break; + case "cm": + this.CATEGORY_PREFS[type]["privacy.trackingprotection.cryptomining.enabled"] = true; + break; + case "-cm": + this.CATEGORY_PREFS[type]["privacy.trackingprotection.cryptomining.enabled"] = false; + break; + case "cookieBehavior0": + this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_ACCEPT; + break; + case "cookieBehavior1": + this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_REJECT_FOREIGN; + break; + case "cookieBehavior2": + this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_REJECT; + break; + case "cookieBehavior3": + this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_LIMIT_FOREIGN; + break; + case "cookieBehavior4": + this.CATEGORY_PREFS[type]["network.cookie.cookieBehavior"] = Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER; + break; + default: + Cu.reportError(`Error: Unknown rule observed ${item}`); } } }, @@ -3109,7 +3105,6 @@ var ContentBlockingCategoriesPrefs = { for (let pref in this.CATEGORY_PREFS[category]) { let value = this.CATEGORY_PREFS[category][pref]; if (value == null) { - Cu.reportError(`Error: ${pref} has not been defined in ${category}`); if (Services.prefs.prefHasUserValue(pref)) { return false; } @@ -3175,7 +3170,6 @@ var ContentBlockingCategoriesPrefs = { let value = this.CATEGORY_PREFS[category][pref]; if (!Services.prefs.prefIsLocked(pref)) { if (value == null) { - Cu.reportError(`Error: ${pref} has not been defined in ${category}`); Services.prefs.clearUserPref(pref); } else { switch (Services.prefs.getPrefType(pref)) { diff --git a/browser/components/preferences/in-content/privacy.js b/browser/components/preferences/in-content/privacy.js index ef4972502275b..305828ddd007f 100644 --- a/browser/components/preferences/in-content/privacy.js +++ b/browser/components/preferences/in-content/privacy.js @@ -80,7 +80,6 @@ Preferences.addAll([ { id: "network.cookie.blockFutureCookies", type: "bool" }, // Content blocking category { id: "browser.contentblocking.category", type: "string"}, - { id: "browser.contentblocking.features.standard", type: "string"}, { id: "browser.contentblocking.features.strict", type: "string"}, // Clear Private Data @@ -477,6 +476,9 @@ var gPrivacyPane = { // not be implemented until they refresh their tabs. for (let pref of CONTENT_BLOCKING_PREFS) { Preferences.get(pref).on("change", gPrivacyPane.maybeNotifyUserToReload); + // If the value changes, run populateCategoryContents, since that change might have been + // triggered by a default value changing in the standard category. + Preferences.get(pref).on("change", gPrivacyPane.populateCategoryContents); } Preferences.get("urlclassifier.trackingTable").on("change", gPrivacyPane.maybeNotifyUserToReload); for (let button of document.querySelectorAll(".reload-tabs-button")) { @@ -491,8 +493,6 @@ var gPrivacyPane = { fingerprintersOption.hidden = !Services.prefs.getBoolPref("browser.contentblocking.fingerprinting.preferences.ui.enabled"); - Preferences.get("browser.contentblocking.features.standard").on("change", - this.populateCategoryContents); Preferences.get("browser.contentblocking.features.strict").on("change", this.populateCategoryContents); this.populateCategoryContents(); @@ -513,14 +513,38 @@ var gPrivacyPane = { populateCategoryContents() { for (let type of ["strict", "standard"]) { - let rulesArray, selector; + let rulesArray = []; + let selector; if (type == "strict") { selector = "#contentBlockingOptionStrict"; rulesArray = Services.prefs.getStringPref("browser.contentblocking.features.strict").split(","); } else { selector = "#contentBlockingOptionStandard"; - let rulesString = Services.prefs.getStringPref("browser.contentblocking.features.standard"); - rulesArray = rulesString.split(","); + // In standard show/hide UI items based on the default values of the relevant prefs. + let defaults = Services.prefs.getDefaultBranch(""); + + let cookieBehavior = defaults.getIntPref("network.cookie.cookieBehavior"); + switch (cookieBehavior) { + case Ci.nsICookieService.BEHAVIOR_ACCEPT: + rulesArray.push("cookieBehavior0"); + break; + case Ci.nsICookieService.BEHAVIOR_REJECT_FOREIGN: + rulesArray.push("cookieBehavior1"); + break; + case Ci.nsICookieService.BEHAVIOR_REJECT: + rulesArray.push("cookieBehavior2"); + break; + case Ci.nsICookieService.BEHAVIOR_LIMIT_FOREIGN: + rulesArray.push("cookieBehavior3"); + break; + case Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER: + rulesArray.push("cookieBehavior4"); + break; + } + rulesArray.push(defaults.getBoolPref("privacy.trackingprotection.cryptomining.enabled") ? "cm" : "-cm"); + rulesArray.push(defaults.getBoolPref("privacy.trackingprotection.fingerprinting.enabled") ? "fp" : "-fp"); + rulesArray.push(defaults.getBoolPref("privacy.trackingprotection.enabled") ? "tp" : "-tp"); + rulesArray.push(defaults.getBoolPref("privacy.trackingprotection.pbmode.enabled") ? "tpPrivate" : "-tpPrivate"); } // Hide all cookie options first, until we learn which one should be showing. document.querySelector(selector + " .all-cookies-option").hidden = true; diff --git a/browser/components/preferences/in-content/tests/browser_contentblocking_categories.js b/browser/components/preferences/in-content/tests/browser_contentblocking_categories.js index ae9ac939ad878..57a4ed216cf07 100644 --- a/browser/components/preferences/in-content/tests/browser_contentblocking_categories.js +++ b/browser/components/preferences/in-content/tests/browser_contentblocking_categories.js @@ -13,44 +13,57 @@ const CAT_PREF = "browser.contentblocking.category"; const FP_PREF = "privacy.trackingprotection.fingerprinting.enabled"; const CM_PREF = "privacy.trackingprotection.cryptomining.enabled"; const STRICT_DEF_PREF = "browser.contentblocking.features.strict"; -const STANDARD_DEF_PREF = "browser.contentblocking.features.standard"; -// Tests that the content blocking standard category definition changes the behavior -// of the standard category pref and all prefs it controls. +// Tests that the content blocking standard category definition is based on the default settings of +// the content blocking prefs. // Changing the definition does not remove the user from the category. add_task(async function testContentBlockingStandardDefinition() { - let defaults = Services.prefs.getDefaultBranch(""); - let originalStandardPref = defaults.getStringPref(STANDARD_DEF_PREF); - defaults.setStringPref(STANDARD_DEF_PREF, "tp,tpPrivate,fp,cm,cookieBehavior4"); + Services.prefs.setStringPref(CAT_PREF, "strict"); + Services.prefs.setStringPref(CAT_PREF, "standard"); is(Services.prefs.getStringPref(CAT_PREF), "standard", `${CAT_PREF} starts on standard`); - ok(!Services.prefs.prefHasUserValue(STANDARD_DEF_PREF), `We changed the default value of ${STANDARD_DEF_PREF}`); - is(Services.prefs.getStringPref(STANDARD_DEF_PREF), "tp,tpPrivate,fp,cm,cookieBehavior4", "The pref changed to what we set."); + ok(!Services.prefs.prefHasUserValue(TP_PREF), `${TP_PREF} pref has the default value`); + ok(!Services.prefs.prefHasUserValue(TP_PBM_PREF), `${TP_PBM_PREF} pref has the default value`); + ok(!Services.prefs.prefHasUserValue(FP_PREF), `${FP_PREF} pref has the default value`); + ok(!Services.prefs.prefHasUserValue(CM_PREF), `${CM_PREF} pref has the default value`); + ok(!Services.prefs.prefHasUserValue(NCB_PREF), `${NCB_PREF} pref has the default value`); - is(Services.prefs.getBoolPref(TP_PREF), true, `${TP_PREF} pref has been set to true`); - is(Services.prefs.getBoolPref(TP_PBM_PREF), true, `${TP_PBM_PREF} pref has been set to true`); - is(Services.prefs.getBoolPref(FP_PREF), true, `${CM_PREF} pref has been set to true`); - is(Services.prefs.getBoolPref(CM_PREF), true, `${CM_PREF} pref has been set to true`); - is(Services.prefs.getIntPref(NCB_PREF), Ci.nsICookieService.BEHAVIOR_REJECT_TRACKER, `${NCB_PREF} has been set to BEHAVIOR_REJECT_TRACKER`); + let defaults = Services.prefs.getDefaultBranch(""); + let originalTP = defaults.getBoolPref(TP_PREF); + let originalTPPBM = defaults.getBoolPref(TP_PBM_PREF); + let originalFP = defaults.getBoolPref(FP_PREF); + let originalCM = defaults.getBoolPref(CM_PREF); + let originalNCB = defaults.getIntPref(NCB_PREF); + + let nonDefaultNCB; + switch (originalNCB) { + case Ci.nsICookieService.BEHAVIOR_ACCEPT: + nonDefaultNCB = Ci.nsICookieService.BEHAVIOR_REJECT; + break; + default: + nonDefaultNCB = Ci.nsICookieService.BEHAVIOR_ACCEPT; + break; + } + defaults.setIntPref(NCB_PREF, nonDefaultNCB); + defaults.setBoolPref(TP_PREF, !originalTP); + defaults.setBoolPref(TP_PBM_PREF, !originalTPPBM); + defaults.setBoolPref(FP_PREF, !originalFP); + defaults.setBoolPref(CM_PREF, !originalCM); + defaults.setIntPref(NCB_PREF, !originalNCB); - // Note, if a pref is not listed it will use the default value, however this is only meant as a - // backup if a mistake is made. The UI will not respond correctly. - defaults.setStringPref(STANDARD_DEF_PREF, ""); ok(!Services.prefs.prefHasUserValue(TP_PREF), `${TP_PREF} pref has the default value`); ok(!Services.prefs.prefHasUserValue(TP_PBM_PREF), `${TP_PBM_PREF} pref has the default value`); ok(!Services.prefs.prefHasUserValue(FP_PREF), `${FP_PREF} pref has the default value`); ok(!Services.prefs.prefHasUserValue(CM_PREF), `${CM_PREF} pref has the default value`); ok(!Services.prefs.prefHasUserValue(NCB_PREF), `${NCB_PREF} pref has the default value`); - defaults.setStringPref(STANDARD_DEF_PREF, "-tpPrivate,-fp,-cm,-tp,cookieBehavior2"); - is(Services.prefs.getBoolPref(TP_PREF), false, `${TP_PREF} pref has been set to false`); - is(Services.prefs.getBoolPref(TP_PBM_PREF), false, `${TP_PBM_PREF} pref has been set to false`); - is(Services.prefs.getBoolPref(FP_PREF), false, `${FP_PREF} pref has been set to false`); - is(Services.prefs.getBoolPref(CM_PREF), false, `${CM_PREF} pref has been set to false`); - is(Services.prefs.getIntPref(NCB_PREF), Ci.nsICookieService.BEHAVIOR_REJECT, `${NCB_PREF} has been set to BEHAVIOR_REJECT_TRACKER`); - // cleanup - defaults.setStringPref(STANDARD_DEF_PREF, originalStandardPref); + defaults.setIntPref(NCB_PREF, originalNCB); + defaults.setBoolPref(TP_PREF, originalTP); + defaults.setBoolPref(TP_PBM_PREF, originalTPPBM); + defaults.setBoolPref(FP_PREF, originalFP); + defaults.setBoolPref(CM_PREF, originalCM); + defaults.setIntPref(NCB_PREF, originalNCB); }); // Tests that the content blocking strict category definition changes the behavior diff --git a/modules/libpref/init/StaticPrefList.h b/modules/libpref/init/StaticPrefList.h index e50dde3d8ae7c..c6e868aaf0923 100644 --- a/modules/libpref/init/StaticPrefList.h +++ b/modules/libpref/init/StaticPrefList.h @@ -2175,7 +2175,6 @@ VARCACHE_PREF( // 0-Accept, 1-dontAcceptForeign, 2-dontAcceptAny, 3-limitForeign, // 4-rejectTracker // Keep the old default of accepting all cookies -// In Firefox Desktop this pref is set by browser.contentblocking.features.[standard, strict] see firefox.js for details. VARCACHE_PREF( "network.cookie.cookieBehavior", network_cookie_cookieBehavior, @@ -2428,7 +2427,6 @@ VARCACHE_PREF( ) // Block 3rd party fingerprinting resources. -// In Firefox Desktop this pref is set by browser.contentblocking.features.[standard, strict] see firefox.js for details. VARCACHE_PREF( "privacy.trackingprotection.fingerprinting.enabled", privacy_trackingprotection_fingerprinting_enabled, @@ -2443,7 +2441,6 @@ VARCACHE_PREF( ) // Block 3rd party cryptomining resources. -// In Firefox Desktop this pref is set by browser.contentblocking.features.[standard, strict] see firefox.js for details. VARCACHE_PREF( "privacy.trackingprotection.cryptomining.enabled", privacy_trackingprotection_cryptomining_enabled, diff --git a/modules/libpref/init/all.js b/modules/libpref/init/all.js index 3431c153f853b..e73d66f3b914f 100644 --- a/modules/libpref/init/all.js +++ b/modules/libpref/init/all.js @@ -1391,10 +1391,8 @@ pref("privacy.restrict3rdpartystorage.userInteractionRequiredForHosts", ""); pref("privacy.popups.maxReported", 100); // Enforce tracking protection in all modes -// In Firefox Desktop this pref is set by browser.contentblocking.features.[standard, strict] see firefox.js for details. pref("privacy.trackingprotection.enabled", false); // Enforce tracking protection in Private Browsing mode -// In Firefox Desktop this pref is set by browser.contentblocking.features.[standard, strict] see firefox.js for details. pref("privacy.trackingprotection.pbmode.enabled", true); // First Party Isolation (double keying), disabled by default pref("privacy.firstparty.isolate", false); From 1f54ba7d471c17e53cbde7a7cef12bf23ca49796 Mon Sep 17 00:00:00 2001 From: Marco Bonardo Date: Fri, 17 May 2019 14:25:47 +0000 Subject: [PATCH 07/22] Bug 1551898 - Replace UrlbarProvider.sources with a more flexible solution. r=adw Differential Revision: https://phabricator.services.mozilla.com/D31272 --HG-- extra : moz-landing-system : lando --- browser/components/urlbar/UrlbarInput.jsm | 1 - .../urlbar/UrlbarProviderOpenTabs.jsm | 26 +- .../urlbar/UrlbarProviderUnifiedComplete.jsm | 29 +- .../urlbar/UrlbarProvidersManager.jsm | 115 +++---- browser/components/urlbar/UrlbarUtils.jsm | 20 +- browser/components/urlbar/tests/unit/head.js | 21 +- .../unit/test_UrlbarController_telemetry.js | 7 +- .../unit/test_providersManager_filtering.js | 289 +++++++++++------- browser/docs/AddressBar.rst | 82 +++-- 9 files changed, 342 insertions(+), 248 deletions(-) diff --git a/browser/components/urlbar/UrlbarInput.jsm b/browser/components/urlbar/UrlbarInput.jsm index 5b7419a5b9993..182bb04ddc937 100644 --- a/browser/components/urlbar/UrlbarInput.jsm +++ b/browser/components/urlbar/UrlbarInput.jsm @@ -643,7 +643,6 @@ class UrlbarInput { isPrivate: this.isPrivate, maxResults: UrlbarPrefs.get("maxRichResults"), muxer: "UnifiedComplete", - providers: ["UnifiedComplete"], searchString, userContextId: this.window.gBrowser.selectedBrowser.getAttribute("usercontextid"), })); diff --git a/browser/components/urlbar/UrlbarProviderOpenTabs.jsm b/browser/components/urlbar/UrlbarProviderOpenTabs.jsm index ae38eb5dba407..90ffa7b6fde21 100644 --- a/browser/components/urlbar/UrlbarProviderOpenTabs.jsm +++ b/browser/components/urlbar/UrlbarProviderOpenTabs.jsm @@ -95,13 +95,27 @@ class ProviderOpenTabs extends UrlbarProvider { } /** - * Returns the sources returned by this provider. - * @returns {array} one or multiple types from UrlbarUtils.RESULT_SOURCE.* + * Whether this provider should be invoked for the given context. + * If this method returns false, the providers manager won't start a query + * with this provider, to save on resources. + * @param {UrlbarQueryContext} queryContext The query context object + * @returns {boolean} Whether this provider should be invoked for the search. */ - get sources() { - return [ - UrlbarUtils.RESULT_SOURCE.TABS, - ]; + isActive(queryContext) { + // For now we don't actually use this provider to query open tabs, instead + // we join the temp table in UnifiedComplete. + return false; + } + + /** + * Whether this provider wants to restrict results to just itself. + * Other providers won't be invoked, unless this provider doesn't + * support the current query. + * @param {UrlbarQueryContext} queryContext The query context object + * @returns {boolean} Whether this provider wants to restrict results. + */ + isRestricting(queryContext) { + return false; } /** diff --git a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm index e13837151010a..406663313e51d 100644 --- a/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm +++ b/browser/components/urlbar/UrlbarProviderUnifiedComplete.jsm @@ -63,18 +63,25 @@ class ProviderUnifiedComplete extends UrlbarProvider { } /** - * Returns the sources returned by this provider. - * @returns {array} one or multiple types from UrlbarUtils.RESULT_SOURCE.* + * Whether this provider should be invoked for the given context. + * If this method returns false, the providers manager won't start a query + * with this provider, to save on resources. + * @param {UrlbarQueryContext} queryContext The query context object + * @returns {boolean} Whether this provider should be invoked for the search. */ - get sources() { - return [ - UrlbarUtils.RESULT_SOURCE.BOOKMARKS, - UrlbarUtils.RESULT_SOURCE.HISTORY, - UrlbarUtils.RESULT_SOURCE.SEARCH, - UrlbarUtils.RESULT_SOURCE.TABS, - UrlbarUtils.RESULT_SOURCE.OTHER_LOCAL, - UrlbarUtils.RESULT_SOURCE.OTHER_NETWORK, - ]; + isActive(queryContext) { + return true; + } + + /** + * Whether this provider wants to restrict results to just itself. + * Other providers won't be invoked, unless this provider doesn't + * support the current query. + * @param {UrlbarQueryContext} queryContext The query context object + * @returns {boolean} Whether this provider wants to restrict results. + */ + isRestricting(queryContext) { + return false; } /** diff --git a/browser/components/urlbar/UrlbarProvidersManager.jsm b/browser/components/urlbar/UrlbarProvidersManager.jsm index c5549edb0fe1f..087e53f09aa16 100644 --- a/browser/components/urlbar/UrlbarProvidersManager.jsm +++ b/browser/components/urlbar/UrlbarProvidersManager.jsm @@ -41,7 +41,6 @@ var localMuxerModules = { // that we can chunk matches coming in that timeframe into a single call. const CHUNK_MATCHES_DELAY_MS = 16; -const DEFAULT_PROVIDERS = ["UnifiedComplete"]; const DEFAULT_MUXER = "UnifiedComplete"; /** @@ -52,12 +51,8 @@ const DEFAULT_MUXER = "UnifiedComplete"; class ProvidersManager { constructor() { // Tracks the available providers. - // This is a double map, first it maps by PROVIDER_TYPE, then - // registerProvider maps by provider.name: { type: { name: provider }} - this.providers = new Map(); - for (let type of Object.values(UrlbarUtils.PROVIDER_TYPE)) { - this.providers.set(type, new Map()); - } + // This is a sorted array, with IMMEDIATE providers at the top. + this.providers = []; for (let [symbol, module] of Object.entries(localProviderModules)) { let {[symbol]: provider} = ChromeUtils.import(module, {}); this.registerProvider(provider); @@ -90,7 +85,11 @@ class ProvidersManager { throw new Error(`Unknown provider type ${provider.type}`); } logger.info(`Registering provider ${provider.name}`); - this.providers.get(provider.type).set(provider.name, provider); + if (provider.type == UrlbarUtils.PROVIDER_TYPE.IMMEDIATE) { + this.providers.unshift(provider); + } else { + this.providers.push(provider); + } } /** @@ -99,7 +98,10 @@ class ProvidersManager { */ unregisterProvider(provider) { logger.info(`Unregistering provider ${provider.name}`); - this.providers.get(provider.type).delete(provider.name); + let index = this.providers.indexOf(provider); + if (index != -1) { + this.providers.splice(index, 1); + } } /** @@ -139,9 +141,12 @@ class ProvidersManager { if (!muxer) { throw new Error(`Muxer with name ${muxerName} not found`); } - // Define the list of providers to use. - let providers = queryContext.providers || DEFAULT_PROVIDERS; - providers = filterProviders(this.providers, providers); + + // If the queryContext specifies a list of providers to use, filter on it, + // otherwise just pass the full list of providers. + let providers = queryContext.providers ? + this.providers.filter(p => queryContext.providers.includes(p.name)) : + this.providers; let query = new Query(queryContext, controller, muxer, providers); this.queries.set(queryContext, query); @@ -213,10 +218,11 @@ class Query { this.started = false; this.canceled = false; this.complete = false; - // Array of acceptable RESULT_SOURCE values for this query. Providers not - // returning any of these will be skipped, as well as results not part of - // this subset (Note we still expect the provider to do its own internal - // filtering, our additional filtering will be for sanity). + + // Array of acceptable RESULT_SOURCE values for this query. Providers can + // use queryContext.acceptableSources to decide whether they want to be + // invoked or not. + // This is also used to filter results in add(). this.acceptableSources = []; } @@ -229,37 +235,36 @@ class Query { } this.started = true; UrlbarTokenizer.tokenize(this.context); + this.acceptableSources = getAcceptableMatchSources(this.context); logger.debug(`Acceptable sources ${this.acceptableSources}`); + // Pass a copy so the provider can't modify our local version. + this.context.acceptableSources = this.acceptableSources.slice(); + + // Check which providers should be queried. + let providers = this.providers.filter(p => p.isActive(this.context)); + // Check if any of the remaining providers wants to restrict the search. + let restrictProviders = providers.filter(p => p.isRestricting(this.context)); + if (restrictProviders.length) { + providers = restrictProviders; + } + // Start querying providers. let promises = []; - for (let provider of this.providers.get(UrlbarUtils.PROVIDER_TYPE.IMMEDIATE).values()) { + let delayStarted = false; + for (let provider of providers) { if (this.canceled) { break; } - // Immediate type providers may return heuristic results, that usually can - // bypass suggest.* preferences, so we always execute them, regardless of - // this.acceptableSources, and filter results in add(). - promises.push(provider.startQuery(this.context, this.add.bind(this))); - } - - // Tracks the delay timer. We will fire (in this specific case, cancel would - // do the same, since the callback is empty) the timer when the search is - // canceled, unblocking start(). - this._sleepTimer = new SkippableTimer(() => {}, UrlbarPrefs.get("delay")); - await this._sleepTimer.promise; - - for (let providerType of [UrlbarUtils.PROVIDER_TYPE.NETWORK, - UrlbarUtils.PROVIDER_TYPE.PROFILE, - UrlbarUtils.PROVIDER_TYPE.EXTENSION]) { - for (let provider of this.providers.get(providerType).values()) { - if (this.canceled) { - break; - } - if (this._providerHasAcceptableSources(provider)) { - promises.push(provider.startQuery(this.context, this.add.bind(this))); - } + if (provider.type != UrlbarUtils.PROVIDER_TYPE.IMMEDIATE && !delayStarted) { + delayStarted = true; + // Tracks the delay timer. We will fire (in this specific case, cancel + // would do the same, since the callback is empty) the timer when the + // search is canceled, unblocking start(). + this._sleepTimer = new SkippableTimer(() => {}, UrlbarPrefs.get("delay")); + await this._sleepTimer.promise; } + promises.push(provider.startQuery(this.context, this.add.bind(this))); } logger.info(`Queried ${promises.length} providers`); @@ -286,10 +291,8 @@ class Query { return; } this.canceled = true; - for (let providers of this.providers.values()) { - for (let provider of providers.values()) { - provider.cancelQuery(this.context); - } + for (let provider of this.providers) { + provider.cancelQuery(this.context); } if (this._chunkTimer) { this._chunkTimer.cancel().catch(Cu.reportError); @@ -349,15 +352,6 @@ class Query { this._chunkTimer = new SkippableTimer(notifyResults, CHUNK_MATCHES_DELAY_MS); } } - - /** - * Returns whether a provider's sources are acceptable for this query. - * @param {object} provider A provider object. - * @returns {boolean}whether the provider sources are acceptable. - */ - _providerHasAcceptableSources(provider) { - return provider.sources.some(s => this.acceptableSources.includes(s)); - } } /** @@ -480,20 +474,3 @@ function getAcceptableMatchSources(context) { } return acceptedSources; } - -/* Given a providers Map and a list of provider names, produces a filtered - * Map containing only the provided names. - * @param providersMap {Map} providers mapped by type and name - * @param names {array} list of provider names to retain - * @returns {Map} a new filtered providers Map - */ -function filterProviders(providersMap, names) { - let providers = new Map(); - for (let [type, providersByName] of providersMap) { - providers.set(type, new Map()); - for (let name of Array.from(providersByName.keys()).filter(n => names.includes(n))) { - providers.get(type).set(name, providersByName.get(name)); - } - } - return providers; -} diff --git a/browser/components/urlbar/UrlbarUtils.jsm b/browser/components/urlbar/UrlbarUtils.jsm index 9d51d3776d145..a9e216e86a29d 100644 --- a/browser/components/urlbar/UrlbarUtils.jsm +++ b/browser/components/urlbar/UrlbarUtils.jsm @@ -486,11 +486,25 @@ class UrlbarProvider { throw new Error("Trying to access the base class, must be overridden"); } /** - * List of UrlbarUtils.RESULT_SOURCE, representing the data sources used by - * the provider. + * Whether this provider should be invoked for the given context. + * If this method returns false, the providers manager won't start a query + * with this provider, to save on resources. + * @param {UrlbarQueryContext} queryContext The query context object + * @returns {boolean} Whether this provider should be invoked for the search. + * @abstract + */ + isActive(queryContext) { + throw new Error("Trying to access the base class, must be overridden"); + } + /** + * Whether this provider wants to restrict results to just itself. + * Other providers won't be invoked, unless this provider doesn't + * support the current query. + * @param {UrlbarQueryContext} queryContext The query context object + * @returns {boolean} Whether this provider wants to restrict results. * @abstract */ - get sources() { + isRestricting(queryContext) { throw new Error("Trying to access the base class, must be overridden"); } /** diff --git a/browser/components/urlbar/tests/unit/head.js b/browser/components/urlbar/tests/unit/head.js index d369a312b4634..0eb83f66a7141 100644 --- a/browser/components/urlbar/tests/unit/head.js +++ b/browser/components/urlbar/tests/unit/head.js @@ -77,20 +77,26 @@ function promiseControllerNotification(controller, notification, expected = true * A basic test provider, returning all the provided matches. */ class TestProvider extends UrlbarProvider { - constructor(matches, cancelCallback) { + constructor(matches, cancelCallback, type = UrlbarUtils.PROVIDER_TYPE.PROFILE) { super(); this._name = "TestProvider" + Math.floor(Math.random() * 100000); this._cancelCallback = cancelCallback; this._matches = matches; + this._type = type; } get name() { return this._name; } get type() { - return UrlbarUtils.PROVIDER_TYPE.PROFILE; + return this._type; } - get sources() { - return this._matches.map(r => r.source); + isActive(context) { + Assert.ok(context, "context is passed-in"); + return true; + } + isRestricting(context) { + Assert.ok(context, "context is passed-in"); + return false; } async startQuery(context, add) { Assert.ok(context, "context is passed-in"); @@ -103,7 +109,7 @@ class TestProvider extends UrlbarProvider { cancelQuery(context) { // If the query was created but didn't run, this_context will be undefined. if (this._context) { - Assert.equal(this._context, context, "context is the same"); + Assert.equal(this._context, context, "cancelQuery: context is the same"); } if (this._cancelCallback) { this._cancelCallback(); @@ -118,10 +124,11 @@ class TestProvider extends UrlbarProvider { * @param {array} matches The matches for the provider to return. * @param {function} [cancelCallback] Optional, called when the query provider * receives a cancel instruction. + * @param {UrlbarUtils.PROVIDER_TYPE} type The provider type. * @returns {string} name of the registered provider */ -function registerBasicTestProvider(matches, cancelCallback) { - let provider = new TestProvider(matches, cancelCallback); +function registerBasicTestProvider(matches = [], cancelCallback, type) { + let provider = new TestProvider(matches, cancelCallback, type); UrlbarProvidersManager.registerProvider(provider); return provider.name; } diff --git a/browser/components/urlbar/tests/unit/test_UrlbarController_telemetry.js b/browser/components/urlbar/tests/unit/test_UrlbarController_telemetry.js index b94e00d9e7826..c052b029f1d1c 100644 --- a/browser/components/urlbar/tests/unit/test_UrlbarController_telemetry.js +++ b/browser/components/urlbar/tests/unit/test_UrlbarController_telemetry.js @@ -33,8 +33,11 @@ class DelayedProvider extends UrlbarProvider { get type() { return UrlbarUtils.PROVIDER_TYPE.PROFILE; } - get sources() { - return [UrlbarUtils.RESULT_SOURCE.TABS]; + isActive(context) { + return true; + } + isRestricting(context) { + return false; } async startQuery(context, add) { Assert.ok(context, "context is passed-in"); diff --git a/browser/components/urlbar/tests/unit/test_providersManager_filtering.js b/browser/components/urlbar/tests/unit/test_providersManager_filtering.js index e5de3179b4ae9..185154e8d2ae6 100644 --- a/browser/components/urlbar/tests/unit/test_providersManager_filtering.js +++ b/browser/components/urlbar/tests/unit/test_providersManager_filtering.js @@ -3,19 +3,28 @@ "use strict"; -add_task(async function test_filtering() { +/** + * A test controller. + */ +class TestUrlbarController extends UrlbarController { + constructor() { + super({ + browserWindow: { + location: { + href: AppConstants.BROWSER_CHROME_URL, + }, + }, + }); + } +} + +add_task(async function test_filtering_disable_only_source() { let match = new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, UrlbarUtils.RESULT_SOURCE.TABS, { url: "http://mozilla.org/foo/" }); let providerName = registerBasicTestProvider([match]); let context = createContext(undefined, {providers: [providerName]}); - let controller = new UrlbarController({ - browserWindow: { - location: { - href: AppConstants.BROWSER_CHROME_URL, - }, - }, - }); + let controller = new TestUrlbarController(); info("Disable the only available source, should get no matches"); Services.prefs.setBoolPref("browser.urlbar.suggest.openpage", false); @@ -26,47 +35,61 @@ add_task(async function test_filtering() { await controller.startQuery(context); await promise; Services.prefs.clearUserPref("browser.urlbar.suggest.openpage"); + UrlbarProvidersManager.unregisterProvider({name: providerName}); +}); +add_task(async function test_filtering_disable_one_source() { let matches = [ - match, + new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, + UrlbarUtils.RESULT_SOURCE.TABS, + { url: "http://mozilla.org/foo/" }), new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, UrlbarUtils.RESULT_SOURCE.HISTORY, { url: "http://mozilla.org/foo/" }), ]; - providerName = registerBasicTestProvider(matches); - context = createContext(undefined, {providers: [providerName]}); + let providerName = registerBasicTestProvider(matches); + let context = createContext(undefined, {providers: [providerName]}); + let controller = new TestUrlbarController(); info("Disable one of the sources, should get a single match"); Services.prefs.setBoolPref("browser.urlbar.suggest.history", false); - promise = Promise.all([ + let promise = Promise.all([ promiseControllerNotification(controller, "onQueryResults"), promiseControllerNotification(controller, "onQueryFinished"), ]); await controller.startQuery(context, controller); await promise; - Assert.deepEqual(context.results, [match]); + Assert.deepEqual(context.results, matches.slice(0, 1)); Services.prefs.clearUserPref("browser.urlbar.suggest.history"); + UrlbarProvidersManager.unregisterProvider({name: providerName}); +}); + +add_task(async function test_filtering_restriction_token() { + let matches = [ + new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, + UrlbarUtils.RESULT_SOURCE.TABS, + { url: "http://mozilla.org/foo/" }), + new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, + UrlbarUtils.RESULT_SOURCE.HISTORY, + { url: "http://mozilla.org/foo/" }), + ]; + let providerName = registerBasicTestProvider(matches); + let context = createContext(`foo ${UrlbarTokenizer.RESTRICT.OPENPAGE}`, + {providers: [providerName]}); + let controller = new TestUrlbarController(); info("Use a restriction character, should get a single match"); - context = createContext(`foo ${UrlbarTokenizer.RESTRICT.OPENPAGE}`, - {providers: [providerName]}); - promise = Promise.all([ + let promise = Promise.all([ promiseControllerNotification(controller, "onQueryResults"), promiseControllerNotification(controller, "onQueryFinished"), ]); await controller.startQuery(context, controller); await promise; - Assert.deepEqual(context.results, [match]); + Assert.deepEqual(context.results, matches.slice(0, 1)); + UrlbarProvidersManager.unregisterProvider({name: providerName}); }); add_task(async function test_filter_javascript() { - let controller = new UrlbarController({ - browserWindow: { - location: { - href: AppConstants.BROWSER_CHROME_URL, - }, - }, - }); let match = new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, UrlbarUtils.RESULT_SOURCE.TABS, { url: "http://mozilla.org/foo/" }); @@ -75,6 +98,7 @@ add_task(async function test_filter_javascript() { { url: "javascript:foo" }); let providerName = registerBasicTestProvider([match, jsMatch]); let context = createContext(undefined, {providers: [providerName]}); + let controller = new TestUrlbarController(); info("By default javascript should be filtered out"); let promise = promiseControllerNotification(controller, "onQueryResults"); @@ -98,17 +122,10 @@ add_task(async function test_filter_javascript() { await promise; Assert.deepEqual(context.results, [match, jsMatch]); Services.prefs.clearUserPref("browser.urlbar.filter.javascript"); + UrlbarProvidersManager.unregisterProvider({name: providerName}); }); -add_task(async function test_filter_sources() { - let controller = new UrlbarController({ - browserWindow: { - location: { - href: AppConstants.BROWSER_CHROME_URL, - }, - }, - }); - +add_task(async function test_filter_isActive() { let goodMatches = [ new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, UrlbarUtils.RESULT_SOURCE.TABS, @@ -117,38 +134,13 @@ add_task(async function test_filter_sources() { UrlbarUtils.RESULT_SOURCE.HISTORY, { url: "http://mozilla.org/foo/" }), ]; - /** - * A test provider that should be invoked. - */ - class TestProvider extends UrlbarProvider { - get name() { - return "GoodProvider"; - } - get type() { - return UrlbarUtils.PROVIDER_TYPE.PROFILE; - } - get sources() { - return [ - UrlbarUtils.RESULT_SOURCE.TABS, - UrlbarUtils.RESULT_SOURCE.HISTORY, - ]; - } - async startQuery(context, add) { - Assert.ok(true, "expected provider was invoked"); - for (const match of goodMatches) { - add(this, match); - } - } - cancelQuery(context) {} - } - UrlbarProvidersManager.registerProvider(new TestProvider()); + let providerName = registerBasicTestProvider(goodMatches); let badMatches = [ new UrlbarResult(UrlbarUtils.RESULT_TYPE.URL, UrlbarUtils.RESULT_SOURCE.BOOKMARKS, { url: "http://mozilla.org/foo/" }), ]; - /** * A test provider that should not be invoked. */ @@ -159,8 +151,12 @@ add_task(async function test_filter_sources() { get type() { return UrlbarUtils.PROVIDER_TYPE.PROFILE; } - get sources() { - return [UrlbarUtils.RESULT_SOURCE.BOOKMARKS]; + isActive(context) { + info("Acceptable sources: " + context.acceptableSources); + return context.acceptableSources.includes(UrlbarUtils.RESULT_SOURCE.BOOKMARKS); + } + isRestricting(context) { + return false; } async startQuery(context, add) { Assert.ok(false, "Provider should no be invoked"); @@ -170,13 +166,13 @@ add_task(async function test_filter_sources() { } cancelQuery(context) {} } - UrlbarProvidersManager.registerProvider(new NoInvokeProvider()); let context = createContext(undefined, { sources: [UrlbarUtils.RESULT_SOURCE.TABS], - providers: ["GoodProvider", "BadProvider"], + providers: [providerName, "BadProvider"], }); + let controller = new TestUrlbarController(); info("Only tabs should be returned"); let promise = promiseControllerNotification(controller, "onQueryResults"); @@ -185,20 +181,50 @@ add_task(async function test_filter_sources() { Assert.deepEqual(context.results.length, 1, "Should find only one match"); Assert.deepEqual(context.results[0].source, UrlbarUtils.RESULT_SOURCE.TABS, "Should find only a tab match"); + UrlbarProvidersManager.unregisterProvider({name: providerName}); + UrlbarProvidersManager.unregisterProvider({name: "BadProvider"}); +}); + +add_task(async function test_filter_queryContext() { + let providerName = registerBasicTestProvider(); + + /** + * A test provider that should not be invoked because of queryContext.providers. + */ + class NoInvokeProvider extends UrlbarProvider { + get name() { + return "BadProvider"; + } + get type() { + return UrlbarUtils.PROVIDER_TYPE.PROFILE; + } + isActive(context) { + return true; + } + isRestricting(context) { + return false; + } + async startQuery(context, add) { + Assert.ok(false, "Provider should no be invoked"); + } + cancelQuery(context) {} + } + UrlbarProvidersManager.registerProvider(new NoInvokeProvider()); + + let context = createContext(undefined, { + providers: [providerName], + }); + let controller = new TestUrlbarController(); + + await controller.startQuery(context, controller); + UrlbarProvidersManager.unregisterProvider({name: providerName}); + UrlbarProvidersManager.unregisterProvider({name: "BadProvider"}); }); add_task(async function test_nofilter_immediate() { // Checks that even if a provider returns a result that should be filtered out // it will still be invoked if it's of type immediate, and only the heuristic // result is returned. - let controller = new UrlbarController({ - browserWindow: { - location: { - href: AppConstants.BROWSER_CHROME_URL, - }, - }, - }); - let matches = [ new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, UrlbarUtils.RESULT_SOURCE.TABS, @@ -208,39 +234,17 @@ add_task(async function test_nofilter_immediate() { { url: "http://mozilla.org/foo2/" }), ]; matches[0].heuristic = true; - - /** - * A test provider that should be invoked. - */ - class TestProvider extends UrlbarProvider { - get name() { - return "GoodProvider"; - } - get type() { - return UrlbarUtils.PROVIDER_TYPE.IMMEDIATE; - } - get sources() { - return [ - UrlbarUtils.RESULT_SOURCE.TABS, - ]; - } - async startQuery(context, add) { - Assert.ok(true, "expected provider was invoked"); - for (let match of matches) { - add(this, match); - } - } - cancelQuery(context) {} - } - UrlbarProvidersManager.registerProvider(new TestProvider()); + let providerName = registerBasicTestProvider(matches, undefined, + UrlbarUtils.PROVIDER_TYPE.IMMEDIATE); let context = createContext(undefined, { sources: [UrlbarUtils.RESULT_SOURCE.SEARCH], - providers: ["GoodProvider"], + providers: [providerName], }); + let controller = new TestUrlbarController(); + // Disable search matches through prefs. Services.prefs.setBoolPref("browser.urlbar.suggest.openpage", false); - info("Only 1 heuristic tab result should be returned"); let promise = promiseControllerNotification(controller, "onQueryResults"); await controller.startQuery(context, controller); @@ -249,19 +253,12 @@ add_task(async function test_nofilter_immediate() { Assert.deepEqual(context.results.length, 1, "Should find only one match"); Assert.deepEqual(context.results[0].source, UrlbarUtils.RESULT_SOURCE.TABS, "Should find only a tab match"); + UrlbarProvidersManager.unregisterProvider({name: providerName}); }); add_task(async function test_nofilter_restrict() { // Checks that even if a pref is disabled, we still return results on a // restriction token. - let controller = new UrlbarController({ - browserWindow: { - location: { - href: AppConstants.BROWSER_CHROME_URL, - }, - }, - }); - let matches = [ new UrlbarResult(UrlbarUtils.RESULT_TYPE.TAB_SWITCH, UrlbarUtils.RESULT_SOURCE.TABS, @@ -276,7 +273,6 @@ add_task(async function test_nofilter_restrict() { UrlbarUtils.RESULT_SOURCE.SEARCH, { engine: "noengine" }), ]; - /** * A test provider. */ @@ -287,13 +283,13 @@ add_task(async function test_nofilter_restrict() { get type() { return UrlbarUtils.PROVIDER_TYPE.IMMEDIATE; } - get sources() { - return [ - UrlbarUtils.RESULT_SOURCE.TABS, - UrlbarUtils.RESULT_SOURCE.BOOKMARKS, - UrlbarUtils.RESULT_SOURCE.HISTORY, - UrlbarUtils.RESULT_SOURCE.SEARCH, - ]; + isActive(context) { + Assert.equal(context.acceptableSources.length, 1, + "Check acceptableSources"); + return true; + } + isRestricting(context) { + return false; } async startQuery(context, add) { Assert.ok(true, "expected provider was invoked"); @@ -301,9 +297,11 @@ add_task(async function test_nofilter_restrict() { add(this, match); } } - cancelQuery(context) {} + cancelQuery(context) { + } } - UrlbarProvidersManager.registerProvider(new TestProvider()); + let provider = new TestProvider(); + UrlbarProvidersManager.registerProvider(provider); let typeToPropertiesMap = new Map([ ["HISTORY", {source: "HISTORY", pref: "history"}], @@ -320,6 +318,7 @@ add_task(async function test_nofilter_restrict() { let context = createContext(token + " foo", { providers: ["MyProvider"], }); + let controller = new TestUrlbarController(); // Disable the corresponding pref. const pref = "browser.urlbar.suggest." + properties.pref; info("Disabling " + pref); @@ -331,4 +330,62 @@ add_task(async function test_nofilter_restrict() { "Check result source"); Services.prefs.clearUserPref(pref); } + UrlbarProvidersManager.unregisterProvider(provider); +}); + +add_task(async function test_filter_isRestricting() { + /** + * A test provider that should be invoked and is restricting. + */ + class TestProvider extends UrlbarProvider { + get name() { + return "GoodProvider"; + } + get type() { + return UrlbarUtils.PROVIDER_TYPE.PROFILE; + } + isActive(context) { + return true; + } + isRestricting(context) { + return true; + } + async startQuery(context, add) { + Assert.ok(true, "expected provider was invoked"); + } + cancelQuery(context) {} + } + UrlbarProvidersManager.registerProvider(new TestProvider()); + + /** + * A test provider that should not be invoked because the other one is restricting. + */ + class NoInvokeProvider extends UrlbarProvider { + get name() { + return "BadProvider"; + } + get type() { + return UrlbarUtils.PROVIDER_TYPE.PROFILE; + } + isActive(context) { + return true; + } + isRestricting(context) { + return false; + } + async startQuery(context, add) { + Assert.ok(false, "Provider should no be invoked"); + } + cancelQuery(context) {} + } + UrlbarProvidersManager.registerProvider(new NoInvokeProvider()); + + let context = createContext(undefined, { + providers: ["GoodProvider", "BadProvider"], + }); + let controller = new TestUrlbarController(); + + await controller.startQuery(context, controller); + UrlbarProvidersManager.unregisterProvider({name: "GoodProvider"}); + UrlbarProvidersManager.unregisterProvider({name: "BadProvider"}); }); diff --git a/browser/docs/AddressBar.rst b/browser/docs/AddressBar.rst index cd6963edae35e..29cf2376851ae 100644 --- a/browser/docs/AddressBar.rst +++ b/browser/docs/AddressBar.rst @@ -79,6 +79,8 @@ It is augmented as it progresses through the system, with various information: results; // {array} list of UrlbarResult objects. tokens; // {array} tokens extracted from the searchString, each token is an // object in the form {type, value, lowerCaseValue}. + acceptableSources; // {array} list of UrlbarUtils.RESULT_SOURCE that the + // model will accept for this context. } @@ -156,46 +158,60 @@ implementation details may vary deeply among different providers. class UrlbarProvider { /** - * Unique name for the provider, used by the context to filter on providers. - * Not using a unique name will cause the newest registration to win. - * @abstract - */ + * Unique name for the provider, used by the context to filter on providers. + * Not using a unique name will cause the newest registration to win. + * @abstract + */ get name() { return "UrlbarProviderBase"; } /** - * The type of the provider, must be one of UrlbarUtils.PROVIDER_TYPE. - * @abstract - */ + * The type of the provider, must be one of UrlbarUtils.PROVIDER_TYPE. + * @abstract + */ get type() { throw new Error("Trying to access the base class, must be overridden"); } /** - * List of UrlbarUtils.RESULT_SOURCE, representing the data sources used by - * the provider. - * @abstract - */ - get sources() { + * Whether this provider should be invoked for the given context. + * If this method returns false, the providers manager won't start a query + * with this provider, to save on resources. + * @param {UrlbarQueryContext} queryContext The query context object + * @returns {boolean} Whether this provider should be invoked for the search. + * @abstract + */ + isActive(queryContext) { throw new Error("Trying to access the base class, must be overridden"); } /** - * Starts querying. - * @param {UrlbarQueryContext} queryContext The query context object - * @param {function} addCallback Callback invoked by the provider to add a new - * result. A UrlbarResult should be passed to it. - * @note Extended classes should return a Promise resolved when the provider - * is done searching AND returning results. - * @abstract - */ + * Whether this provider wants to restrict results to just itself. + * Other providers won't be invoked, unless this provider doesn't + * support the current query. + * @param {UrlbarQueryContext} queryContext The query context object + * @returns {boolean} Whether this provider wants to restrict results. + * @abstract + */ + isRestricting(queryContext) { + throw new Error("Trying to access the base class, must be overridden"); + } + /** + * Starts querying. + * @param {UrlbarQueryContext} queryContext The query context object + * @param {function} addCallback Callback invoked by the provider to add a new + * result. A UrlbarResult should be passed to it. + * @note Extended classes should return a Promise resolved when the provider + * is done searching AND returning results. + * @abstract + */ startQuery(queryContext, addCallback) { throw new Error("Trying to access the base class, must be overridden"); } /** - * Cancels a running query, - * @param {UrlbarQueryContext} queryContext The query context object to cancel - * query for. - * @abstract - */ + * Cancels a running query, + * @param {UrlbarQueryContext} queryContext The query context object to cancel + * query for. + * @abstract + */ cancelQuery(queryContext) { throw new Error("Trying to access the base class, must be overridden"); } @@ -218,18 +234,18 @@ indicated by the UrlbarQueryContext.muxer property. class UrlbarMuxer { /** - * Unique name for the muxer, used by the context to sort results. - * Not using a unique name will cause the newest registration to win. - * @abstract - */ + * Unique name for the muxer, used by the context to sort results. + * Not using a unique name will cause the newest registration to win. + * @abstract + */ get name() { return "UrlbarMuxerBase"; } /** - * Sorts UrlbarQueryContext results in-place. - * @param {UrlbarQueryContext} queryContext the context to sort results for. - * @abstract - */ + * Sorts UrlbarQueryContext results in-place. + * @param {UrlbarQueryContext} queryContext the context to sort results for. + * @abstract + */ sort(queryContext) { throw new Error("Trying to access the base class, must be overridden"); } From e997399bf4ca6565f137fe3008c048f0814c6bfa Mon Sep 17 00:00:00 2001 From: Ed Lee Date: Fri, 17 May 2019 13:25:09 +0000 Subject: [PATCH 08/22] Bug 1552366 - Add placeholder cards, wrapping buttons and bug fixes to Activity Stream r=k88hudson Differential Revision: https://phabricator.services.mozilla.com/D31549 --HG-- extra : moz-landing-system : lando --- browser/components/newtab/.mcignore | 3 + browser/components/newtab/README.md | 24 -- .../asrouter/components/RichText/RichText.jsx | 3 +- .../components/SnippetBase/_SnippetBase.scss | 20 ++ .../EOYSnippet/EOYSnippet.schema.json | 10 +- .../FXASignupSnippet.schema.json | 10 +- .../NewsletterSnippet.schema.json | 10 +- .../templates/ReturnToAMO/ReturnToAMO.jsx | 5 +- .../SendToDeviceSnippet.schema.json | 14 +- .../SimpleBelowSearchSnippet.jsx | 5 +- .../SimpleBelowSearchSnippet.schema.json | 6 +- .../templates/SimpleSnippet/SimpleSnippet.jsx | 23 +- .../SimpleSnippet/SimpleSnippet.schema.json | 12 + .../SubmitFormSnippet/SubmitFormSnippet.jsx | 13 +- .../SubmitFormSnippet.schema.json | 14 +- .../templates/Trailhead/_Trailhead.scss | 8 +- .../ASRouterAdmin/ASRouterAdmin.jsx | 4 +- .../CardGrid/CardGrid.jsx | 6 +- .../DiscoveryStreamComponents/Hero/Hero.jsx | 40 ++- .../DiscoveryStreamComponents/List/List.jsx | 6 +- .../DiscoveryStreamComponents/List/_List.scss | 5 +- .../content-src/lib/selectLayoutRender.js | 61 ++-- browser/components/newtab/contributing.md | 16 ++ .../newtab/css/activity-stream-linux.css | 22 +- .../newtab/css/activity-stream-mac.css | 22 +- .../newtab/css/activity-stream-windows.css | 22 +- .../data/content/activity-stream.bundle.js | 271 +++++++++++------- browser/components/newtab/docs/index.rst | 65 +++++ browser/components/newtab/hooks/pre-push | 11 + browser/components/newtab/lib/ASRouter.jsm | 11 +- .../lib/SnippetsTestMessageProvider.jsm | 71 +++++ browser/components/newtab/moz.build | 2 + browser/components/newtab/package-lock.json | 198 ------------- browser/components/newtab/package.json | 4 +- .../newtab/test/browser/browser.ini | 2 + .../test/browser/browser_asrouter_snippets.js | 37 +++ .../test/unit/asrouter/ASRouter.test.js | 17 ++ .../SimpleBelowSearchSnippet.test.jsx | 9 +- .../asrouter/templates/SimpleSnippet.test.jsx | 28 +- .../templates/SubmitFormSnippet.test.jsx | 8 +- .../components/ASRouterAdmin.test.jsx | 2 +- .../lib/selectLayoutRender.test.js | 62 ++-- browser/components/newtab/yamscripts.yml | 6 +- 43 files changed, 748 insertions(+), 440 deletions(-) delete mode 100644 browser/components/newtab/README.md create mode 100644 browser/components/newtab/docs/index.rst create mode 100644 browser/components/newtab/hooks/pre-push create mode 100644 browser/components/newtab/test/browser/browser_asrouter_snippets.js diff --git a/browser/components/newtab/.mcignore b/browser/components/newtab/.mcignore index a7dd711627a4f..0241ed5b4f6ec 100644 --- a/browser/components/newtab/.mcignore +++ b/browser/components/newtab/.mcignore @@ -14,6 +14,9 @@ npm-debug.log /logs/ /node_modules/ +# ignore README since it's GitHub specific +/README.md + # also ignores ping centre tests ping-centre/ diff --git a/browser/components/newtab/README.md b/browser/components/newtab/README.md deleted file mode 100644 index cd9c16f5aefe4..0000000000000 --- a/browser/components/newtab/README.md +++ /dev/null @@ -1,24 +0,0 @@ -# activity-stream - -[![Task Status](https://github.taskcluster.net/v1/repository/mozilla/activity-stream/master/badge.svg)](https://github.taskcluster.net/v1/repository/mozilla/activity-stream/master/latest) - -This system add-on replaces the new tab page in Firefox with a new design and -functionality as part of the Activity Stream project. - -The files in this directory, including vendor dependencies, are imported from the -system-addon directory in https://github.com/mozilla/activity-stream. - -Read [docs/v2-system-addon](https://github.com/mozilla/activity-stream/tree/master/docs/v2-system-addon/1.GETTING_STARTED.md) for more detail. - -## Where should I file bugs? - -We regularly check the ActivityStream:NewTab component on Bugzilla. - -## For Developers - -If you are interested in contributing, take a look at [this guide](contributing.md) on where to find us and how to contribute, -and [this guide](docs/v2-system-addon/1.GETTING_STARTED.md) for getting your development environment set up. - -## For Localizers - -Activity Stream localization is managed via [Pontoon](https://pontoon.mozilla.org/projects/activity-stream-new-tab/), not direct pull requests to the repository. If you want to fix a typo, add a new language, or simply know more about localization, please get in touch with the [existing localization team](https://pontoon.mozilla.org/teams/) for your language, or Mozilla’s [l10n-drivers](https://wiki.mozilla.org/L10n:Mozilla_Team#Mozilla_Corporation) for guidance. diff --git a/browser/components/newtab/content-src/asrouter/components/RichText/RichText.jsx b/browser/components/newtab/content-src/asrouter/components/RichText/RichText.jsx index f9eab80db240d..7f619bbe06c8b 100644 --- a/browser/components/newtab/content-src/asrouter/components/RichText/RichText.jsx +++ b/browser/components/newtab/content-src/asrouter/components/RichText/RichText.jsx @@ -24,7 +24,8 @@ export function convertLinks(links, sendClick, doNotAutoBlock, openNewWindow = f // Setting the value to false will not include the attribute in the anchor const url = action ? false : safeURI(links[linkTag].url); - acc[linkTag] = (; + const customElement = {ICON_ALT_TEXT}; return ( - + {ICON_ALT_TEXT} + {ICON_ALT_TEXT}

{this.renderText()}

{this.props.extraContent} diff --git a/browser/components/newtab/content-src/asrouter/templates/SimpleBelowSearchSnippet/SimpleBelowSearchSnippet.schema.json b/browser/components/newtab/content-src/asrouter/templates/SimpleBelowSearchSnippet/SimpleBelowSearchSnippet.schema.json index 5e469539838cd..07d9ee98d84a8 100644 --- a/browser/components/newtab/content-src/asrouter/templates/SimpleBelowSearchSnippet/SimpleBelowSearchSnippet.schema.json +++ b/browser/components/newtab/content-src/asrouter/templates/SimpleBelowSearchSnippet/SimpleBelowSearchSnippet.schema.json @@ -1,7 +1,7 @@ { "title": "SimpleBelowSearchSnippet", "description": "A simple template with just an icon and rich text. It gets inserted below the Activity Stream search box.", - "version": "1.1.0", + "version": "1.2.0", "type": "object", "definitions": { "richText": { @@ -25,6 +25,10 @@ "type": "string", "description": "Snippet icon. 64x64px. SVG or PNG preferred." }, + "icon_dark_theme": { + "type": "string", + "description": "Snippet icon. Dark theme variant. 64x64px. SVG or PNG preferred." + }, "block_button_text": { "type": "string", "description": "Tooltip text used for dismiss button.", diff --git a/browser/components/newtab/content-src/asrouter/templates/SimpleSnippet/SimpleSnippet.jsx b/browser/components/newtab/content-src/asrouter/templates/SimpleSnippet/SimpleSnippet.jsx index 41a8755b5958f..7f56c0eba1a4c 100644 --- a/browser/components/newtab/content-src/asrouter/templates/SimpleSnippet/SimpleSnippet.jsx +++ b/browser/components/newtab/content-src/asrouter/templates/SimpleSnippet/SimpleSnippet.jsx @@ -6,6 +6,8 @@ import {safeURI} from "../../template-utils"; import {SnippetBase} from "../../components/SnippetBase/SnippetBase"; const DEFAULT_ICON_PATH = "chrome://branding/content/icon64.png"; +// Alt text if available; in the future this should come from the server. See bug 1551711 +const ICON_ALT_TEXT = ""; export class SimpleSnippet extends React.PureComponent { constructor(props) { @@ -41,8 +43,16 @@ export class SimpleSnippet extends React.PureComponent { } renderTitleIcon() { - const titleIcon = safeURI(this.props.content.title_icon); - return titleIcon ? : null; + const titleIconLight = safeURI(this.props.content.title_icon); + const titleIconDark = safeURI(this.props.content.title_icon_dark_theme || this.props.content.title_icon); + if (!titleIconLight) { + return null; + } + + return ( + + + ); } renderButton() { @@ -83,14 +93,16 @@ export class SimpleSnippet extends React.PureComponent { // an icon and text must be specified to render the section header if (props.content.section_title_icon && props.content.section_title_text) { - const sectionTitleIcon = safeURI(props.content.section_title_icon); + const sectionTitleIconLight = safeURI(props.content.section_title_icon); + const sectionTitleIconDark = safeURI(props.content.section_title_icon_dark_theme || props.content.section_title_icon); const sectionTitleURL = props.content.section_title_url; return (

- + + {props.content.section_title_text}

@@ -119,7 +131,8 @@ export class SimpleSnippet extends React.PureComponent { return ( {sectionHeader} - + {ICON_ALT_TEXT} + {ICON_ALT_TEXT}
{this.renderTitle()}

{this.renderText()}

{this.props.extraContent} diff --git a/browser/components/newtab/content-src/asrouter/templates/SimpleSnippet/SimpleSnippet.schema.json b/browser/components/newtab/content-src/asrouter/templates/SimpleSnippet/SimpleSnippet.schema.json index 66f3f8d2283ef..0fec2c9a91f0c 100644 --- a/browser/components/newtab/content-src/asrouter/templates/SimpleSnippet/SimpleSnippet.schema.json +++ b/browser/components/newtab/content-src/asrouter/templates/SimpleSnippet/SimpleSnippet.schema.json @@ -35,10 +35,18 @@ "type": "string", "description": "Snippet icon. 64x64px. SVG or PNG preferred." }, + "icon_dark_theme": { + "type": "string", + "description": "Snippet icon, dark theme variant. 64x64px. SVG or PNG preferred." + }, "title_icon": { "type": "string", "description": "Small icon that shows up before the title / text. 16x16px. SVG or PNG preferred. Grayscale." }, + "title_icon_dark_theme": { + "type": "string", + "description": "Small icon that shows up before the title / text. Dark theme variant. 16x16px. SVG or PNG preferred. Grayscale." + }, "button_action": { "type": "string", "description": "The type of action the button should trigger." @@ -102,6 +110,10 @@ "type": "string", "description": "Section title icon. 16x16px. SVG or PNG preferred. section_title_text must also be specified to display." }, + "section_title_icon_dark_theme": { + "type": "string", + "description": "Section title icon, dark theme variant. 16x16px. SVG or PNG preferred. section_title_text must also be specified to display." + }, "section_title_text": { "type": "string", "description": "Section title text. section_title_icon must also be specified to display." diff --git a/browser/components/newtab/content-src/asrouter/templates/SubmitFormSnippet/SubmitFormSnippet.jsx b/browser/components/newtab/content-src/asrouter/templates/SubmitFormSnippet/SubmitFormSnippet.jsx index e7c031864e068..cd02798312deb 100644 --- a/browser/components/newtab/content-src/asrouter/templates/SubmitFormSnippet/SubmitFormSnippet.jsx +++ b/browser/components/newtab/content-src/asrouter/templates/SubmitFormSnippet/SubmitFormSnippet.jsx @@ -1,9 +1,13 @@ import {Button} from "../../components/Button/Button"; import React from "react"; import {RichText} from "../../components/RichText/RichText"; +import {safeURI} from "../../template-utils"; import {SimpleSnippet} from "../SimpleSnippet/SimpleSnippet"; import {SnippetBase} from "../../components/SnippetBase/SnippetBase"; +// Alt text if available; in the future this should come from the server. See bug 1551711 +const ICON_ALT_TEXT = ""; + export class SubmitFormSnippet extends React.PureComponent { constructor(props) { super(props); @@ -155,15 +159,18 @@ export class SubmitFormSnippet extends React.PureComponent { name="email" required={true} placeholder={placholder} - onChange={this.props.validateInput ? this.onInputChange : null} - autoFocus={true} />); + onChange={this.props.validateInput ? this.onInputChange : null} />); } renderSignupView() { const {content} = this.props; const containerClass = `SubmitFormSnippet ${this.props.className}`; return ( - {content.scene2_icon ?
: null} + {content.scene2_icon ? +
+ {ICON_ALT_TEXT} + {ICON_ALT_TEXT} +
: null}

{content.scene2_title &&

{content.scene2_title}

} diff --git a/browser/components/newtab/content-src/asrouter/templates/SubmitFormSnippet/SubmitFormSnippet.schema.json b/browser/components/newtab/content-src/asrouter/templates/SubmitFormSnippet/SubmitFormSnippet.schema.json index 538214b639d50..4b6752e8bc3e5 100644 --- a/browser/components/newtab/content-src/asrouter/templates/SubmitFormSnippet/SubmitFormSnippet.schema.json +++ b/browser/components/newtab/content-src/asrouter/templates/SubmitFormSnippet/SubmitFormSnippet.schema.json @@ -1,7 +1,7 @@ { "title": "SubmitFormSnippet", "description": "A template with two states: a SimpleSnippet and another that contains a form", - "version": "1.0.0", + "version": "1.1.0", "type": "object", "definitions": { "plainText": { @@ -55,10 +55,18 @@ "type": "string", "description": "Snippet icon. 64x64px. SVG or PNG preferred." }, + "scene1_icon_dark_theme": { + "type": "string", + "description": "Snippet icon. Dark theme variant. 64x64px. SVG or PNG preferred." + }, "scene1_title_icon": { "type": "string", "description": "Small icon that shows up before the title / text. 16x16px. SVG or PNG preferred. Grayscale." }, + "scene1_title_icon_dark_theme": { + "type": "string", + "description": "Small icon that shows up before the title / text. Dark theme variant. 16x16px. SVG or PNG preferred. Grayscale." + }, "form_action": { "type": "string", "description": "Endpoint to submit form data." @@ -103,6 +111,10 @@ "type": "string", "description": "(send to device) Image to display above the form. 98x98px. SVG or PNG preferred." }, + "scene2_icon_dark_theme": { + "type": "string", + "description": "(send to device) Image to display above the form. Dark theme variant. 98x98px. SVG or PNG preferred." + }, "scene2_newsletter": { "type": "string", "description": "Newsletter/basket id user is subscribing to. Must be a value from the 'Slug' column here: https://basket.mozilla.org/news/. Default 'mozilla-foundation'." diff --git a/browser/components/newtab/content-src/asrouter/templates/Trailhead/_Trailhead.scss b/browser/components/newtab/content-src/asrouter/templates/Trailhead/_Trailhead.scss index ca713f0ff77f2..6b64efde4c97a 100644 --- a/browser/components/newtab/content-src/asrouter/templates/Trailhead/_Trailhead.scss +++ b/browser/components/newtab/content-src/asrouter/templates/Trailhead/_Trailhead.scss @@ -371,9 +371,10 @@ color: var(--newtab-text-conditional-color); background: var(--trailhead-card-button-background-color); border: 0; - height: 30px; + margin: 14px; min-width: 70%; - padding: 0 14px; + padding: 6px 14px; + white-space: pre-wrap; &:focus, &:hover { @@ -391,9 +392,8 @@ } .onboardingButtonContainer { - height: 60px; position: absolute; - bottom: 0; + bottom: 16px; left: 0; width: 100%; text-align: center; diff --git a/browser/components/newtab/content-src/components/ASRouterAdmin/ASRouterAdmin.jsx b/browser/components/newtab/content-src/components/ASRouterAdmin/ASRouterAdmin.jsx index cd02490816660..e12b125b038c6 100644 --- a/browser/components/newtab/content-src/components/ASRouterAdmin/ASRouterAdmin.jsx +++ b/browser/components/newtab/content-src/components/ASRouterAdmin/ASRouterAdmin.jsx @@ -472,7 +472,7 @@ export class ASRouterAdminInner extends React.PureComponent { if (!this.state.providers) { return null; } - return (

Show messages from {this.state.providers.map(provider => ())}

); @@ -541,7 +541,7 @@ export class ASRouterAdminInner extends React.PureComponent {

-