From a9b6b7cff86f8c7e354a9050c7e4cb133ed24a08 Mon Sep 17 00:00:00 2001 From: Bogdan Tara Date: Wed, 27 Jan 2021 02:46:06 +0200 Subject: [PATCH] Backed out changeset a77ccd1c1659 (bug 1687358) for browser_lazyload_telemetry.js failures CLOSED TREE --- dom/base/DOMIntersectionObserver.cpp | 21 +------ dom/base/DOMIntersectionObserver.h | 2 - dom/base/Document.cpp | 41 ------------- dom/base/Document.h | 29 --------- dom/html/HTMLImageElement.cpp | 38 +++--------- dom/html/HTMLImageElement.h | 3 +- layout/base/tests/browser.ini | 4 -- .../base/tests/browser_lazyload_telemetry.js | 61 ------------------- .../base/tests/file_lazyload_telemetry.html | 9 --- toolkit/components/telemetry/Histograms.json | 60 ------------------ 10 files changed, 9 insertions(+), 259 deletions(-) delete mode 100644 layout/base/tests/browser_lazyload_telemetry.js delete mode 100644 layout/base/tests/file_lazyload_telemetry.html diff --git a/dom/base/DOMIntersectionObserver.cpp b/dom/base/DOMIntersectionObserver.cpp index 2aa1494de36e..5fb70f5c25a2 100644 --- a/dom/base/DOMIntersectionObserver.cpp +++ b/dom/base/DOMIntersectionObserver.cpp @@ -152,18 +152,7 @@ static void LazyLoadCallback( MOZ_ASSERT(entry->Target()->IsHTMLElement(nsGkAtoms::img)); if (entry->IsIntersecting()) { static_cast(entry->Target()) - ->StopLazyLoadingAndStartLoadIfNeeded(true); - } - } -} - -static void LazyLoadCallbackReachViewport( - const Sequence>& aEntries) { - for (const auto& entry : aEntries) { - MOZ_ASSERT(entry->Target()->IsHTMLElement(nsGkAtoms::img)); - if (entry->IsIntersecting()) { - static_cast(entry->Target()) - ->LazyLoadImageReachedViewport(); + ->StopLazyLoadingAndStartLoadIfNeeded(); } } } @@ -200,14 +189,6 @@ DOMIntersectionObserver::CreateLazyLoadObserver(Document& aDocument) { return observer.forget(); } -already_AddRefed -DOMIntersectionObserver::CreateLazyLoadObserverViewport(Document& aDocument) { - RefPtr observer = - new DOMIntersectionObserver(aDocument, LazyLoadCallbackReachViewport); - observer->mThresholds.AppendElement(std::numeric_limits::min()); - return observer.forget(); -} - bool DOMIntersectionObserver::SetRootMargin(const nsACString& aString) { return Servo_IntersectionObserverRootMargin_Parse(&aString, &mRootMargin); } diff --git a/dom/base/DOMIntersectionObserver.h b/dom/base/DOMIntersectionObserver.h index 1a50dd043def..627c41ecbe0d 100644 --- a/dom/base/DOMIntersectionObserver.h +++ b/dom/base/DOMIntersectionObserver.h @@ -127,8 +127,6 @@ class DOMIntersectionObserver final : public nsISupports, static already_AddRefed CreateLazyLoadObserver( Document&); - static already_AddRefed - CreateLazyLoadObserverViewport(Document&); protected: void Connect(); diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp index 43ab6b9614dd..945e61f6261f 100644 --- a/dom/base/Document.cpp +++ b/dom/base/Document.cpp @@ -1403,10 +1403,6 @@ Document::Document(const char* aContentType) mOnloadBlockCount(0), mAsyncOnloadBlockCount(0), mWriteLevel(0), - mLazyLoadImageCount(0), - mLazyLoadImageStarted(0), - mLazyLoadImageReachViewportLoading(0), - mLazyLoadImageReachViewportLoaded(0), mContentEditableCount(0), mEditingState(EditingState::eOff), mCompatMode(eCompatibility_FullStandards), @@ -2378,7 +2374,6 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(Document) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOnloadBlocker) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLazyLoadImageObserver) - NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLazyLoadImageObserverViewport) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDOMImplementation) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mImageMaps) NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOrientationPendingPromise) @@ -2501,7 +2496,6 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Document) NS_IMPL_CYCLE_COLLECTION_UNLINK(mSecurityInfo) NS_IMPL_CYCLE_COLLECTION_UNLINK(mDisplayDocument) NS_IMPL_CYCLE_COLLECTION_UNLINK(mLazyLoadImageObserver) - NS_IMPL_CYCLE_COLLECTION_UNLINK(mLazyLoadImageObserverViewport) NS_IMPL_CYCLE_COLLECTION_UNLINK(mFontFaceSet) NS_IMPL_CYCLE_COLLECTION_UNLINK(mReadyForIdle) NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocumentL10n) @@ -15415,25 +15409,6 @@ void Document::ReportDocumentUseCounters() { (" > %s\n", Telemetry::GetHistogramName(id))); Telemetry::Accumulate(id, 1); } - - ReportDocumentLazyLoadCounters(); -} - -void Document::ReportDocumentLazyLoadCounters() { - if (!mLazyLoadImageCount) { - return; - } - Telemetry::Accumulate(Telemetry::LAZYLOAD_IMAGE_TOTAL, mLazyLoadImageCount); - Telemetry::Accumulate(Telemetry::LAZYLOAD_IMAGE_STARTED, - mLazyLoadImageStarted); - Telemetry::Accumulate(Telemetry::LAZYLOAD_IMAGE_NOT_VIEWPORT, - mLazyLoadImageStarted - - mLazyLoadImageReachViewportLoading - - mLazyLoadImageReachViewportLoaded); - Telemetry::Accumulate(Telemetry::LAZYLOAD_IMAGE_VIEWPORT_LOADING, - mLazyLoadImageReachViewportLoading); - Telemetry::Accumulate(Telemetry::LAZYLOAD_IMAGE_VIEWPORT_LOADED, - mLazyLoadImageReachViewportLoaded); } void Document::SendPageUseCounters() { @@ -15562,22 +15537,6 @@ DOMIntersectionObserver& Document::EnsureLazyLoadImageObserver() { return *mLazyLoadImageObserver; } -DOMIntersectionObserver& Document::EnsureLazyLoadImageObserverViewport() { - if (!mLazyLoadImageObserverViewport) { - mLazyLoadImageObserverViewport = - DOMIntersectionObserver::CreateLazyLoadObserverViewport(*this); - } - return *mLazyLoadImageObserverViewport; -} - -void Document::IncLazyLoadImageReachViewport(bool aLoading) { - if (aLoading) { - ++mLazyLoadImageReachViewportLoading; - } else { - ++mLazyLoadImageReachViewportLoaded; - } -} - void Document::NotifyLayerManagerRecreated() { EnumerateActivityObservers(NotifyActivityChanged); EnumerateSubDocuments([](Document& aSubDoc) { diff --git a/dom/base/Document.h b/dom/base/Document.h index aabb4b2789aa..8e5cd23676c2 100644 --- a/dom/base/Document.h +++ b/dom/base/Document.h @@ -3616,9 +3616,6 @@ class Document : public nsINode, // effect once per document, and so is called during document destruction. void ReportDocumentUseCounters(); - // Report how lazyload performs for this document. - void ReportDocumentLazyLoadCounters(); - // Sends page use counters to the parent process to accumulate against the // top-level document. Must be called while we still have access to our // WindowContext. This method has an effect each time it is called, and we @@ -3732,18 +3729,7 @@ class Document : public nsINode, DOMIntersectionObserver* GetLazyLoadImageObserver() { return mLazyLoadImageObserver; } - DOMIntersectionObserver* GetLazyLoadImageObserverViewport() { - return mLazyLoadImageObserverViewport; - } DOMIntersectionObserver& EnsureLazyLoadImageObserver(); - DOMIntersectionObserver& EnsureLazyLoadImageObserverViewport(); - void IncLazyLoadImageCount() { ++mLazyLoadImageCount; } - void DecLazyLoadImageCount() { - MOZ_DIAGNOSTIC_ASSERT(mLazyLoadImageCount > 0); - --mLazyLoadImageCount; - } - void IncLazyLoadImageStarted() { ++mLazyLoadImageStarted; } - void IncLazyLoadImageReachViewport(bool aLoading); // Dispatch a runnable related to the document. nsresult Dispatch(TaskCategory aCategory, @@ -4728,19 +4714,6 @@ class Document : public nsINode, // finishes processing that script. uint32_t mWriteLevel; - // The amount of images that have `loading="lazy"` on the page or have loaded - // lazily already. - uint32_t mLazyLoadImageCount; - // Number of lazy-loaded images that we've started loading as a result of - // triggering the lazy-load observer threshold. - uint32_t mLazyLoadImageStarted; - // Number of lazy-loaded images that reached the viewport which were not done - // loading when they did so. - uint32_t mLazyLoadImageReachViewportLoading; - // Number of lazy-loaded images that reached the viewport and were done - // loading when they did so. - uint32_t mLazyLoadImageReachViewportLoaded; - uint32_t mContentEditableCount; EditingState mEditingState; @@ -5036,8 +5009,6 @@ class Document : public nsINode, nsTHashtable> mIntersectionObservers; RefPtr mLazyLoadImageObserver; - // Used to measure how effective the lazyload thresholds are. - RefPtr mLazyLoadImageObserverViewport; // Stack of top layer elements. nsTArray mTopLayer; diff --git a/dom/html/HTMLImageElement.cpp b/dom/html/HTMLImageElement.cpp index 0c061aedacf0..670513ca0545 100644 --- a/dom/html/HTMLImageElement.cpp +++ b/dom/html/HTMLImageElement.cpp @@ -352,7 +352,7 @@ nsresult HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName, static_cast( aOldValue->GetEnumValue()) == Loading::Lazy && !ImageState().HasState(NS_EVENT_STATE_LOADING)) { - StopLazyLoadingAndStartLoadIfNeeded(false); + StopLazyLoadingAndStartLoadIfNeeded(); } } else if (aName == nsGkAtoms::src && !aValue) { // NOTE: regular src value changes are handled in AfterMaybeChangeAttr, so @@ -653,18 +653,8 @@ EventStates HTMLImageElement::IntrinsicState() const { void HTMLImageElement::NodeInfoChanged(Document* aOldDoc) { nsGenericHTMLElement::NodeInfoChanged(aOldDoc); - // Unlike the LazyLoadImageObserver, the intersection observer - // for the viewport could contain the element even if - // it's not lazy-loading. For instance, the element has - // started to load, but haven't reached to the viewport. - // So here we always try to unobserve it. - if (auto* observer = aOldDoc->GetLazyLoadImageObserverViewport()) { - observer->Unobserve(*this); - } - if (mLazyLoading) { aOldDoc->GetLazyLoadImageObserver()->Unobserve(*this); - aOldDoc->DecLazyLoadImageCount(); mLazyLoading = false; SetLazyLoading(); } @@ -1266,9 +1256,10 @@ void HTMLImageElement::SetLazyLoading() { return; } - doc->EnsureLazyLoadImageObserver().Observe(*this); - doc->EnsureLazyLoadImageObserverViewport().Observe(*this); - doc->IncLazyLoadImageCount(); + // There (maybe) is a race condition that we have no LazyLoadImageObserver + // when the root document has been removed from the docshell. + // In the case we don't need to worry about lazy-loading. + OwnerDoc()->EnsureLazyLoadImageObserver().Observe(*this); mLazyLoading = true; UpdateImageState(true); } @@ -1288,28 +1279,13 @@ void HTMLImageElement::StartLoadingIfNeeded() { } } -void HTMLImageElement::StopLazyLoadingAndStartLoadIfNeeded( - bool aFromIntersectionObserver) { +void HTMLImageElement::StopLazyLoadingAndStartLoadIfNeeded() { if (!mLazyLoading) { return; } mLazyLoading = false; - Document* doc = OwnerDoc(); - doc->GetLazyLoadImageObserver()->Unobserve(*this); + OwnerDoc()->GetLazyLoadImageObserver()->Unobserve(*this); StartLoadingIfNeeded(); - - if (aFromIntersectionObserver) { - doc->IncLazyLoadImageStarted(); - } else { - doc->DecLazyLoadImageCount(); - doc->GetLazyLoadImageObserverViewport()->Unobserve(*this); - } -} - -void HTMLImageElement::LazyLoadImageReachedViewport() { - Document* doc = OwnerDoc(); - doc->GetLazyLoadImageObserverViewport()->Unobserve(*this); - doc->IncLazyLoadImageReachViewport(!Complete()); } } // namespace mozilla::dom diff --git a/dom/html/HTMLImageElement.h b/dom/html/HTMLImageElement.h index 8d78b466f999..45ee2234c55e 100644 --- a/dom/html/HTMLImageElement.h +++ b/dom/html/HTMLImageElement.h @@ -261,8 +261,7 @@ class HTMLImageElement final : public nsGenericHTMLElement, const nsAString& aTypeAttr, const nsAString& aMediaAttr, nsAString& aResult); - void StopLazyLoadingAndStartLoadIfNeeded(bool aFromIntersectionObserver); - void LazyLoadImageReachedViewport(); + void StopLazyLoadingAndStartLoadIfNeeded(); protected: virtual ~HTMLImageElement(); diff --git a/layout/base/tests/browser.ini b/layout/base/tests/browser.ini index 41f79638cf54..258c9f8852bb 100644 --- a/layout/base/tests/browser.ini +++ b/layout/base/tests/browser.ini @@ -22,7 +22,3 @@ skip-if = (verify && (os == 'mac')) # bug 1627874 support-files = !/gfx/layers/apz/test/mochitest/apz_test_native_event_utils.js !/browser/base/content/test/forms/head.js -[browser_lazyload_telemetry.js] -support-files = - file_lazyload_telemetry.html - image_rgrg-256x256.png diff --git a/layout/base/tests/browser_lazyload_telemetry.js b/layout/base/tests/browser_lazyload_telemetry.js deleted file mode 100644 index 4990965e761f..000000000000 --- a/layout/base/tests/browser_lazyload_telemetry.js +++ /dev/null @@ -1,61 +0,0 @@ -const baseURL = getRootDirectory(gTestPath).replace( - "chrome://mochitests/content", - "http://example.com" -); - -const testFileURL = `${baseURL}file_lazyload_telemetry.html`; - -const { TelemetryTestUtils } = ChromeUtils.import( - "resource://testing-common/TelemetryTestUtils.jsm" -); - -function dataIsReported() { - const snapshot = Services.telemetry.getSnapshotForHistograms("main", false) - .content; - return ( - snapshot.LAZYLOAD_IMAGE_TOTAL && snapshot.LAZYLOAD_IMAGE_TOTAL.values[4] - ); -} - -add_task(async function testTelemetryCollection() { - Services.telemetry.getHistogramById("LAZYLOAD_IMAGE_TOTAL").clear(); - Services.telemetry.getHistogramById("LAZYLOAD_IMAGE_STARTED").clear(); - Services.telemetry.getHistogramById("LAZYLOAD_IMAGE_NOT_VIEWPORT").clear(); - Services.telemetry - .getHistogramById("LAZYLOAD_IMAGE_VIEWPORT_LOADING") - .clear(); - Services.telemetry.getHistogramById("LAZYLOAD_IMAGE_VIEWPORT_LOADED").clear(); - - const testTab = await BrowserTestUtils.openNewForegroundTab( - gBrowser, - testFileURL, - true - ); - - await SpecialPowers.spawn(testTab.linkedBrowser.browsingContext, [], () => { - const image2 = content.document.getElementById("image2"); - image2.scrollIntoView(); - }); - - gBrowser.removeTab(testTab); - - await BrowserTestUtils.waitForCondition(dataIsReported); - - const snapshot = Services.telemetry.getSnapshotForHistograms("main", false) - .content; - - // Ensures we have 4 lazyload images. - is(snapshot.LAZYLOAD_IMAGE_TOTAL.values[4], 1); - // All 4 images should be lazy-loaded. - is(snapshot.LAZYLOAD_IMAGE_STARTED.values[4], 1); - // The last image didn't reach to the viewport. - is(snapshot.LAZYLOAD_IMAGE_NOT_VIEWPORT.values[1], 1); - // The sum of the images that reached to the viewport - // should be three. This includes all images except - // the last one. - is( - snapshot.LAZYLOAD_IMAGE_VIEWPORT_LOADING.sum + - snapshot.LAZYLOAD_IMAGE_VIEWPORT_LOADED.sum, - 3 - ); -}); diff --git a/layout/base/tests/file_lazyload_telemetry.html b/layout/base/tests/file_lazyload_telemetry.html deleted file mode 100644 index 636090a19e16..000000000000 --- a/layout/base/tests/file_lazyload_telemetry.html +++ /dev/null @@ -1,9 +0,0 @@ - - - - - - - - - diff --git a/toolkit/components/telemetry/Histograms.json b/toolkit/components/telemetry/Histograms.json index 6cc571552bd5..dfc9b88c736e 100644 --- a/toolkit/components/telemetry/Histograms.json +++ b/toolkit/components/telemetry/Histograms.json @@ -4733,66 +4733,6 @@ "releaseChannelCollection": "opt-out", "description": "Number of tabs opened across all windows, collected at most every 5 minutes whenever the user interacts with the browser in the following ways: open tab/window, page load." }, - "LAZYLOAD_IMAGE_TOTAL": { - "record_in_processes": ["main", "content"], - "products": ["firefox"], - "alert_emails": ["sefeng@mozilla.com"], - "bug_numbers": [1687358], - "expires_in_version": "never", - "kind": "exponential", - "high": 2000, - "n_buckets": 100, - "releaseChannelCollection": "opt-out", - "description": "The amount of images that have `loading='lazy'` on the page or have loaded lazily already." - }, - "LAZYLOAD_IMAGE_STARTED": { - "record_in_processes": ["main", "content"], - "products": ["firefox"], - "alert_emails": ["sefeng@mozilla.com"], - "bug_numbers": [1687358], - "expires_in_version": "never", - "kind": "exponential", - "high": 2000, - "n_buckets": 100, - "releaseChannelCollection": "opt-out", - "description": "Number of lazy-loaded images that we've started loading as a result of triggering the lazy-load observer threshold." - }, - "LAZYLOAD_IMAGE_NOT_VIEWPORT": { - "record_in_processes": ["main", "content"], - "products": ["firefox"], - "alert_emails": ["sefeng@mozilla.com"], - "bug_numbers": [1687358], - "expires_in_version": "never", - "kind": "exponential", - "high": 2000, - "n_buckets": 100, - "releaseChannelCollection": "opt-out", - "description": "Number of lazy-loaded images that were started to load but didn't reach to the viewport eventually" - }, - "LAZYLOAD_IMAGE_VIEWPORT_LOADING": { - "record_in_processes": ["main", "content"], - "products": ["firefox"], - "alert_emails": ["sefeng@mozilla.com"], - "bug_numbers": [1687358], - "expires_in_version": "never", - "kind": "exponential", - "high": 2000, - "n_buckets": 100, - "releaseChannelCollection": "opt-out", - "description": "Number of lazy-loaded images that reached the viewport which were not done loading when they did so." - }, - "LAZYLOAD_IMAGE_VIEWPORT_LOADED": { - "record_in_processes": ["main", "content"], - "products": ["firefox"], - "alert_emails": ["sefeng@mozilla.com"], - "bug_numbers": [1687358], - "expires_in_version": "never", - "kind": "exponential", - "high": 2000, - "n_buckets": 100, - "releaseChannelCollection": "opt-out", - "description": "Number of lazy-loaded images that reached the viewport and were done loading when they did so." - }, "LOADED_TAB_COUNT": { "record_in_processes": ["main"], "products": ["firefox"],