From ccbe453ae8033638813e61ea5427a3ee1415602b Mon Sep 17 00:00:00 2001
From: Matthew Tylee Atkinson <matatk@gmail.com>
Date: Fri, 12 Jan 2018 15:41:16 +0000
Subject: [PATCH] Complete guarding (fixes #127)

* Remove magic numbers in all setTimeout() calls.
* If landmarks might be out-of-date, re-scan when using the button or
  keyboard shortcuts.
* Make injected landmarks test page less verbose.
---
 src/static/content.focusing.js          |  3 +-
 src/static/content.overall.js           | 15 +++++
 src/static/content.pausing.js           |  5 +-
 test/manual-test-injected-landmark.html | 82 +++++++++----------------
 4 files changed, 51 insertions(+), 54 deletions(-)

diff --git a/src/static/content.focusing.js b/src/static/content.focusing.js
index 07036afa..a2fa0409 100644
--- a/src/static/content.focusing.js
+++ b/src/static/content.focusing.js
@@ -2,6 +2,7 @@
 /* exported ElementFocuser */
 
 function ElementFocuser() {
+	const momentaryBorderTime = 2000
 	let currentlySelectedElement
 
 
@@ -38,7 +39,7 @@ function ElementFocuser() {
 				addBorder(element)
 
 				if (borderTypePref === 'momentary') {
-					setTimeout(() => removeBorder(element), 2000)
+					setTimeout(() => removeBorder(element), momentaryBorderTime)
 				}
 			}
 
diff --git a/src/static/content.overall.js b/src/static/content.overall.js
index 82065066..67595db5 100644
--- a/src/static/content.overall.js
+++ b/src/static/content.overall.js
@@ -1,5 +1,6 @@
 'use strict'
 /* global LandmarksFinder ElementFocuser PauseHandler */
+const outOfDateTime = 2000
 const logger = new Logger()
 let observer = null
 
@@ -61,22 +62,27 @@ function messageHandler(message, sender, sendResponse) {
 	switch (message.request) {
 		case 'get-landmarks':
 			// The pop-up is requesting the list of landmarks on the page
+			handleOutdatedResults()
 			sendResponse(lf.filter())
 			break
 		case 'focus-landmark':
 			// Triggered by clicking on an item in the pop-up, or indirectly
 			// via one of the keyboard shortcuts (if landmarks are present)
+			handleOutdatedResults()
 			checkFocusElement(() => lf.getLandmarkElement(message.index))
 			break
 		case 'next-landmark':
 			// Triggered by keyboard shortcut
+			handleOutdatedResults()
 			checkFocusElement(lf.getNextLandmarkElement)
 			break
 		case 'prev-landmark':
 			// Triggered by keyboard shortcut
+			handleOutdatedResults()
 			checkFocusElement(lf.getPreviousLandmarkElement)
 			break
 		case 'main-landmark': {
+			handleOutdatedResults()
 			const mainElement = lf.getMainElement()
 			if (mainElement) {
 				ef.focusElement(mainElement)
@@ -102,6 +108,13 @@ function messageHandler(message, sender, sendResponse) {
 	}
 }
 
+function handleOutdatedResults() {
+	if (ph.getPauseTime() > outOfDateTime) {
+		logger.log(`Landmarks may be out of date (pause: ${ph.getPauseTime()}); scanning now...`)
+		findLandmarksAndUpdateBadge()
+	}
+}
+
 function checkFocusElement(callbackReturningElement) {
 	if (lf.getNumberOfLandmarks() === 0) {
 		alert(browser.i18n.getMessage('noLandmarksFound') + '.')
@@ -132,6 +145,7 @@ function sendUpdateBadgeMessage() {
 		// been retired because the extension was unloaded/reloaded. In which
 		// case, we don't want to keep handling mutations.
 		if (observer) {
+			logger.log('Disconnecting observer from retired content script')
 			observer.disconnect()
 		} else {
 			throw error
@@ -161,6 +175,7 @@ function setUpMutationObserver() {
 		ph.run(
 			function() {
 				if (shouldRefreshLandmarkss(mutations)) {
+					logger.log('SCAN mutation')
 					findLandmarksAndUpdateBadge()
 				}
 			},
diff --git a/src/static/content.pausing.js b/src/static/content.pausing.js
index 2b7695f2..9bf1a661 100644
--- a/src/static/content.pausing.js
+++ b/src/static/content.pausing.js
@@ -65,7 +65,6 @@ function PauseHandler(logger) {
 	this.run = function(guardedTask, scheduledTask) {
 		const now = Date.now()
 		if (now > lastEvent + pause) {
-			logger.log('SCAN mutation')
 			guardedTask()
 			lastEvent = now
 		} else if (!haveIncreasedPauseAndScheduledTask) {
@@ -80,4 +79,8 @@ function PauseHandler(logger) {
 			haveIncreasedPauseAndScheduledTask = true
 		}
 	}
+
+	this.getPauseTime = function() {
+		return pause
+	}
 }
diff --git a/test/manual-test-injected-landmark.html b/test/manual-test-injected-landmark.html
index 0e5d4a55..e62d67eb 100644
--- a/test/manual-test-injected-landmark.html
+++ b/test/manual-test-injected-landmark.html
@@ -9,14 +9,14 @@
 			<h1>Test injected landmark</h1>
 		</header>
 		<main>
-			<p>Main content.</p>
-			<button id="outer-injector">Inject a landmark</button>
-			<button id="inner-injector" disabled>
-				Inject a landmark inside the other
-			</button>
-			<button id="the-cleaner" disabled>
-				Remove all injected bits and bobs
-			</button>
+		<p>Main content.</p>
+		<button id="outer-injector">Inject a landmark</button>
+		<button id="inner-injector" disabled>
+			Inject a landmark inside the other
+		</button>
+		<button id="the-cleaner" disabled>
+			Remove all injected bits and bobs
+		</button>
 		</main>
 		<footer>
 			<p>This is the footer.</p>
@@ -30,70 +30,48 @@ <h1>Test injected landmark</h1>
 
 function makeSection(level, heading, text) {
 	const section = document.createElement('section')
-	const sectionHeader = document.createElement(`h${level}`)
-	const sectionHeaderText = document.createTextNode(heading)
-	const sectionPara = document.createElement('p')
-	const sectionParaText = document.createTextNode(text)
+		const sectionHeader = document.createElement(`h${level}`)
+		const sectionHeaderText = document.createTextNode(heading)
+		const sectionPara = document.createElement('p')
+		const sectionParaText = document.createTextNode(text)
 
-	sectionHeader.appendChild(sectionHeaderText)
-	section.appendChild(sectionHeader)
+		sectionHeader.appendChild(sectionHeaderText)
+		section.appendChild(sectionHeader)
 
-	sectionPara.appendChild(sectionParaText)
-	section.appendChild(sectionPara)
+		sectionPara.appendChild(sectionParaText)
+		section.appendChild(sectionPara)
 
-	sectionHeader.id = `section-header-${idCounter++}`
-	section.setAttribute('aria-labelledby', sectionHeader.id)
+		sectionHeader.id = `section-header-${idCounter++}`
+		section.setAttribute('aria-labelledby', sectionHeader.id)
 
-	return section
+		return section
 }
 
 document.getElementById(btnOuterInjectorId).onclick = function() {
 	const section = makeSection(2, 'Outer injected section',
-		'A section nested within the main region.')
-	section.id = outerInjectedSectionId
-	document.getElementsByTagName('main')[0].appendChild(section)
+			'A section nested within the main region.')
+		section.id = outerInjectedSectionId
+		document.getElementsByTagName('main')[0].appendChild(section)
 
-	this.disabled = true
-	document.getElementById(btnInnerInjectorId).removeAttribute('disabled')
+		this.disabled = true
+		document.getElementById(btnInnerInjectorId).removeAttribute('disabled')
 }
 
 document.getElementById(btnInnerInjectorId).onclick = function() {
 	const innerSection = makeSection(3, 'Inner injected section',
-		'A further nested section.')
-	document.getElementById(outerInjectedSectionId).appendChild(innerSection)
+			'A further nested section.')
+		document.getElementById(outerInjectedSectionId).appendChild(innerSection)
 
-	this.disabled = true
-	document.getElementById(btnCleanerId).removeAttribute('disabled')
+		this.disabled = true
+		document.getElementById(btnCleanerId).removeAttribute('disabled')
 }
 
 document.getElementById(btnCleanerId).onclick = function() {
 	document.getElementById(outerInjectedSectionId).remove()
 
-	this.disabled = true
-	document.getElementById(btnOuterInjectorId).removeAttribute('disabled')
+		this.disabled = true
+		document.getElementById(btnOuterInjectorId).removeAttribute('disabled')
 }
-
-const observer = new MutationObserver(function(mutations) {
-	mutations.forEach(function(mutation) {
-		console.log(mutation)
-		if (mutation.type === 'childList') {
-			mutation.addedNodes.forEach(function(addedNode) {
-				if (addedNode.nodeType === Node.ELEMENT_NODE) {
-					console.log('Added node:', addedNode)
-				}
-			})
-			mutation.removedNodes.forEach(function(removedNode) {
-				console.log('Removed node:', removedNode)
-			})
-		}
-	})
-})
-
-observer.observe(document, {
-	attributes: true,
-	childList: true,
-	subtree: true
-})
 		</script>
 	</body>
 </html>