From 755c9fb4f837a9f8b80d51610e86c3ba2ea1999f Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sat, 5 Mar 2016 05:52:36 +0000 Subject: Make HUD.init() idempotent. --- content_scripts/hud.coffee | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/content_scripts/hud.coffee b/content_scripts/hud.coffee index 453d6522..e56644df 100644 --- a/content_scripts/hud.coffee +++ b/content_scripts/hud.coffee @@ -13,9 +13,10 @@ HUD = # it doesn't sit on top of horizontal scrollbars like Chrome's HUD does. init: -> - @hudUI = new UIComponent "pages/hud.html", "vimiumHUDFrame", ({data}) => - this[data.name]? data - @tween = new Tween "iframe.vimiumHUDFrame.vimiumUIComponentVisible", @hudUI.shadowDOM + unless @hudUI? + @hudUI = new UIComponent "pages/hud.html", "vimiumHUDFrame", ({data}) => + this[data.name]? data + @tween = new Tween "iframe.vimiumHUDFrame.vimiumUIComponentVisible", @hudUI.shadowDOM showForDuration: (text, duration) -> @show(text) -- cgit v1.2.3 From 0822f8244d328233752c7f986a70e805ee238979 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sat, 5 Mar 2016 06:36:16 +0000 Subject: Initialize UI components only when they're needed. HUD: Initialize only when the frame receives the focus and Vimium is enabled. Vomnibar: Initialize in the top frame when Vimium is enabled in *any* frame. Warning: There may be a race condition here. Specifically, if Vimium is disabled in the main/top frame (T) but enabled in another frame (A), then the initialisation could happen in frame A before frame T is listening, so frame T would miss the initialization message (which is only sent once). Message listeners are installed early (and probably installed first in the main/top frame), and the `isEnabledForUrl` messaging takes some time, so perhaps it's OK. But it *is* a race condition. Fixes #1838. --- content_scripts/hud.coffee | 2 +- content_scripts/vimium_frontend.coffee | 21 +++++++++++++++++---- tests/dom_tests/dom_tests.coffee | 1 + tests/dom_tests/vomnibar_test.coffee | 1 + 4 files changed, 20 insertions(+), 5 deletions(-) diff --git a/content_scripts/hud.coffee b/content_scripts/hud.coffee index e56644df..aa8a6bbd 100644 --- a/content_scripts/hud.coffee +++ b/content_scripts/hud.coffee @@ -86,7 +86,7 @@ HUD = isReady: do -> ready = false DomUtils.documentReady -> ready = true - -> ready and document.body != null + -> ready and document.body != null and @hudUI? enabled: -> true diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index 2e57ee1a..5acabcfc 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -149,6 +149,7 @@ initializePreDomReady = -> # A frame has received the focus. We don't care here (the Vomnibar/UI-component handles this). frameFocused: -> checkEnabledAfterURLChange: checkEnabledAfterURLChange + initializeTopFrame: initializeTopFrame runInTopFrame: ({sourceFrameId, registryEntry}) -> Utils.invokeCommandString registryEntry.command, [sourceFrameId, registryEntry] if DomUtils.isTopFrame() @@ -218,9 +219,6 @@ initializeOnDomReady = -> isEnabledForUrl = false chrome.runtime.sendMessage = -> window.removeEventListener "focus", onFocus - # We only initialize the vomnibar in the tab's main frame, because it's only ever opened there. - Vomnibar.init() if DomUtils.isTopFrame() - HUD.init() registerFrame = -> # Don't register frameset containers; focusing them is no use. @@ -436,6 +434,17 @@ extend window, indicator: false +initializeTopFrame = (request = null) -> + initializeTopFrame = -> # Only do this initialization once. + # We only initialize the vomnibar in the tab's top/main frame, because it's only ever opened there. + if DomUtils.isTopFrame() + Vomnibar.init() + else + # Ignore requests from other frames (if we're not the top frame). + unless request? + # Tell the top frame to initialize the Vomnibar. + chrome.runtime.sendMessage handler: "sendMessageToFrames", message: name: "initializeTopFrame" + # Checks if Vimium should be enabled or not in this frame. As a side effect, it also informs the background # page whether this frame has the focus, allowing the background page to track the active frame's URL. checkIfEnabledForUrl = (frameIsFocused = windowIsFocused()) -> @@ -443,7 +452,11 @@ checkIfEnabledForUrl = (frameIsFocused = windowIsFocused()) -> chrome.runtime.sendMessage { handler: "isEnabledForUrl", url: url, frameIsFocused: frameIsFocused }, (response) -> { isEnabledForUrl, passKeys } = response installListeners() # But only if they have not been installed already. - if HUD.isReady() and not isEnabledForUrl + # Initialize UI components. We only initialize these once we know that Vimium is enabled; see #1838. + if isEnabledForUrl + initializeTopFrame() + HUD.init() if frameIsFocused + else if HUD.isReady() # Quickly hide any HUD we might already be showing, e.g. if we entered insert mode on page load. HUD.hide() normalMode?.setPassKeys passKeys diff --git a/tests/dom_tests/dom_tests.coffee b/tests/dom_tests/dom_tests.coffee index 0745e5b2..3797a37b 100644 --- a/tests/dom_tests/dom_tests.coffee +++ b/tests/dom_tests/dom_tests.coffee @@ -1,6 +1,7 @@ # Install frontend event handlers. installListeners() +HUD.init() installListener = (element, event, callback) -> element.addEventListener event, (-> callback.apply(this, arguments)), true diff --git a/tests/dom_tests/vomnibar_test.coffee b/tests/dom_tests/vomnibar_test.coffee index 3eda6234..0898e33a 100644 --- a/tests/dom_tests/vomnibar_test.coffee +++ b/tests/dom_tests/vomnibar_test.coffee @@ -1,5 +1,6 @@ vomnibarFrame = null SearchEngines.refresh "" +Vomnibar.init() context "Keep selection within bounds", -- cgit v1.2.3 From bde730eb473982203f9db9813aebd5657beeebf9 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sat, 5 Mar 2016 07:53:24 +0000 Subject: Address possible race condition in Vomnibar initialization. This addresses the potential race condition mentioned in this commit record (3542db7b6c322d803c263db641ae0b02327447ca) and in #2033. In non-top frames, wait until documentReady before sending the message to initialise the Vomnibar. This cannot be before preDomReady in the main frame, right? --- content_scripts/vimium_frontend.coffee | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index 5acabcfc..92cfea58 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -440,10 +440,12 @@ initializeTopFrame = (request = null) -> if DomUtils.isTopFrame() Vomnibar.init() else - # Ignore requests from other frames (if we're not the top frame). + # Ignore requests from other frames (we're not the top frame). unless request? - # Tell the top frame to initialize the Vomnibar. - chrome.runtime.sendMessage handler: "sendMessageToFrames", message: name: "initializeTopFrame" + # Tell the top frame to initialize the Vomnibar. We wait until "DOMContentLoaded" to ensure that the + # listener in the main/top frame (which are installed pre-DomReady) is already installed. + DomUtils.documentReady -> + chrome.runtime.sendMessage handler: "sendMessageToFrames", message: name: "initializeTopFrame" # Checks if Vimium should be enabled or not in this frame. As a side effect, it also informs the background # page whether this frame has the focus, allowing the background page to track the active frame's URL. -- cgit v1.2.3 From 7818ab8d6db8e28d6699e64b17bf3a0d63adbdf7 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sat, 5 Mar 2016 08:17:05 +0000 Subject: Do no init Vomnibar pre-DomReady. --- content_scripts/vimium_frontend.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index 92cfea58..3beab455 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -438,7 +438,7 @@ initializeTopFrame = (request = null) -> initializeTopFrame = -> # Only do this initialization once. # We only initialize the vomnibar in the tab's top/main frame, because it's only ever opened there. if DomUtils.isTopFrame() - Vomnibar.init() + DomUtils.documentReady -> Vomnibar.init() else # Ignore requests from other frames (we're not the top frame). unless request? -- cgit v1.2.3 From 2035844b1c625ca65c1b9d0fb8c9a6c6df6a0dcd Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sat, 5 Mar 2016 08:18:13 +0000 Subject: Do no init Vomnibar pre-DomReady (tweak). --- content_scripts/vimium_frontend.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index 3beab455..99d8fc4a 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -438,7 +438,7 @@ initializeTopFrame = (request = null) -> initializeTopFrame = -> # Only do this initialization once. # We only initialize the vomnibar in the tab's top/main frame, because it's only ever opened there. if DomUtils.isTopFrame() - DomUtils.documentReady -> Vomnibar.init() + DomUtils.documentReady Vomnibar.init.bind Vomnibar else # Ignore requests from other frames (we're not the top frame). unless request? -- cgit v1.2.3 From 5b2e06747f63fbb120bae527122bd2ea0af9ea7d Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sun, 6 Mar 2016 11:22:07 +0000 Subject: Don't HUD.init() until DOM is ready. --- content_scripts/vimium_frontend.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee index 99d8fc4a..1a882192 100644 --- a/content_scripts/vimium_frontend.coffee +++ b/content_scripts/vimium_frontend.coffee @@ -457,7 +457,7 @@ checkIfEnabledForUrl = (frameIsFocused = windowIsFocused()) -> # Initialize UI components. We only initialize these once we know that Vimium is enabled; see #1838. if isEnabledForUrl initializeTopFrame() - HUD.init() if frameIsFocused + DomUtils.documentReady HUD.init.bind HUD if frameIsFocused else if HUD.isReady() # Quickly hide any HUD we might already be showing, e.g. if we entered insert mode on page load. HUD.hide() -- cgit v1.2.3 From a8cd9d5b66f9fd6d46f453e50672611538c801f2 Mon Sep 17 00:00:00 2001 From: Stephen Blott Date: Sun, 6 Mar 2016 11:24:59 +0000 Subject: Tweak documentReady(). We can remove these listeners once we're done. --- lib/dom_utils.coffee | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/dom_utils.coffee b/lib/dom_utils.coffee index aab0a4df..3581bd3f 100644 --- a/lib/dom_utils.coffee +++ b/lib/dom_utils.coffee @@ -2,11 +2,13 @@ DomUtils = # # Runs :callback if the DOM has loaded, otherwise runs it on load # - documentReady: (func) -> + documentReady: (callback) -> if document.readyState == "loading" - window.addEventListener "DOMContentLoaded", func + window.addEventListener "DOMContentLoaded", handler = -> + window.removeEventListener "DOMContentLoaded", handler + callback() else - func() + callback() createElement: (tagName) -> element = document.createElement tagName -- cgit v1.2.3