fix: Move to simpler message passing #268
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As per #265. This should be most of the work, and hopefully most of the bugs worked out, but there are some TODOs that remain... Plus, it's the return of sendToActiveTab() :-).
* Neaten up DevTools code in background script. * Factor out a function to return an error when an unexpected message is received. * Minor spacing/naming clarity fixes. * Log a FIXME for help for later. * Ensure DevTools is in-synch with the rest of the UI. There is still a problem whereby the content script is reporting receiving unexpected "landmarks" messages, which is odd as it only sends them to the runtime. Could it be that other content scripts are also picking this up?
* Fix logic of checking for landmarks before sending them. * Function naming clarity. * Spacing of comments. * Temporarily set aside a thing that should not happen.
* Rename toggle-state to toggle-state-is for clarity. * For !DevTools GUI message handling, check that the message relates to the active tab. * Fix a logic error where runtime.sendMessage() was used instead of sendToActiveTab(). Next: some DRYing up.
* DRY up message handling in GUI. * Address the FIXME about whether "get-toggle-state" was needed (it was). * Shorten unexpected message error function name. * Clean up and clarify some comments.
* Remove disconnectingPortErrorCheck() (not needed). * Add some debugging logging. * Try to detect and properly handle orphaned content script on Chrome. * Indication of broken DevTools connection on Chrome was buggy; fix.
Also note the problem to resolve with DevTools.
* Factor out several functions relating to message passing. * Use them to support DevTools, particularly on non-scriptable pages. * Remove some debugging via console.log. * Note a future FIXME.
This uses @lydell's method on Chrome-like browsers, of using a port's disconnect handler.
A big chunk of the extra stuff is comments; code-wise this is more tidy-y.
The logic was not quite the same across invocations.
* Content-scriptable pages are pages the extension can expect to interact with (i.e. web pages and the help page). * Content-injectable pages are pages that we can put content scripts into (i.e. web pages, but not the help page, as it is forbidden, and already loads the content script itself).
* Some of the more complex background script functions not needed. * Functions that are only called once folded back in. * Have most GUIs check if their respective page is scriptable before sending messages; the DevTools panel cannot do this, so background script checks it. * sendToActiveTab() again makes an exit.
* Ignore a splash message from the help page in the GUI script. * Refine comments about mysteriously-received messages in content script.
This removes some logging that is no longer needed, as it is working fine.
* Do not include explicit cases in switch statements for messages that are not handled in that particular place. * This has meant that the library functions to throw "unexpected message" errors, and identify sending extension components, could be removed.
* The DevTools GUI was errantly listening to browser.runtime.onMessage, not port.onMessage; oops! That took a while to find... * There is no need to update the DevTools GUI when activating a new tab, as it keeps state.
* Yes, port gets tree-shaken from GUI script. * Yes, we need the comment about the "devToolsConnectionError" message.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes as much of the long-lived port connections as possible - i.e. those between the main GUIs (but DevTools requires them, so there they stay). Amount of code is reduced and it does feel much simpler (fewer cross-browser workarounds are required, too).
It is possible to listen for navigation-related events in most GUIs, so the background script doesn't need to act on their behalf, but not for DevTools, so the background script would still need to do that. Therefore it's neater that that stuff still happens in the background script.
Fixes #265