Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify message-passing #201

Closed
matatk opened this issue Aug 23, 2018 · 1 comment
Closed

Unify message-passing #201

matatk opened this issue Aug 23, 2018 · 1 comment
Assignees
@matatk
Labels
enhancement
Milestone
2.4.0

Comments

@matatk
Copy link
Owner

matatk commented Aug 23, 2018 •
edited

The current simple way of doing message-passing doesn't work for the DevTools part because only long-lived connections via ports to the background page are supported.

The techniques given in Messaging from Content Scripts to the DevTools Page should be used to allow messages from the content script to get to the correct DevTools panel.

Might as well unify the comms across Landmarks and use them to get messages from the content script to the background and pop-up and sidebar panel too. This will remove some of the present 'ping-pong' of messages, whereby the background script instructs the GUIs to ask the content script for the latest landmarks when required, rather than managing the connections itself and sending things as they are updated). Effort to set up, as it will require state-handling, but could bring robustosity benefits, and have clearer paths of communications, in future.
@matatk matatk added this to the 2.4.0 milestone Aug 23, 2018
@matatk matatk self-assigned this Aug 23, 2018
matatk added a commit that referenced this issue Aug 29, 2018
@matatk
Port-based communications; Build tweaks; Commands; Start-up
40dad05 
Partly addresses #201

* Catch Rollup errors in build script.
* Neaten up bundle specifying in build script.
* Build DevTools panel bits for, and load them on, Chrome and Opera.
* Add port-based communications from content script to background
  script; remove blanket message-sending.
* Add port-based communications from DevTools scripts to background
  script; remove blanket message-sending.
* Add port-based communications from pop-up and sidebar scripts to
  background script; remove blanket message-sending.
* Remove sendToActiveTab.js.
* Add a library function to check for disconnecting port errors.
* Sort out calling of the keyboard settings window cross-browser; fixes
  #200; thanks
  openstyles/stylus#52 (comment)
  for the URL.
* Inject the content script when the extension background page loads on
  Chrome/Opera - previously it was only injected when the extension was
  installed, so would not work as expected when the extension was
  disabled and then re-enabled.
* Re-jig the Logger to not require the init() function. This was done to
  make it possible to use the Logger to log messages relating to the new
  port-based communication. However that is not presently being used
  and, now the odd Firefox issues with content-to-background comms have
  been addressed, it may not be (and the Logger could be removed in
  future if the DevTools panel(s) get(s) good.

Due to the unified messaging system, the DevTools panel can now focus
landmarks just like the other GUIs. The inspection of landmarks can be
done on a separate UI elment that's only added to the DevTools panel.
This should make the DevTools panel more intuitive.
matatk added a commit that referenced this issue Aug 30, 2018
@matatk
Content-to-background logic; less verbose; neaten
b766e49 
* Fix the logic around connections from the content script to the
  background script, and dealing with orphaned.
* Remove various log messages from background script now it seems to be
  working fine.
* Simplify the function to set the badge text.
* Logger queues up messages until it knows if the user wants to display
  them or not. When the browser returns the user's preference, the
  queued messages are either displayed or dropped.
* Make the scanning-related messages softer (less shouty case).

Partly addresses #201 - what remains is to make the error messages about
not having a connection more user-friendly, and complete the FIXMEs.
@matatk
Copy link
Owner Author

matatk commented Sep 4, 2018 •
edited

Looks like the technique from Simon Lydell for dealing with the background script not being loaded yet on Firefox might be better (neater to factor out for unaffected browsers?) than the "try 10 times in quick succession then give up" approach I am currently using.

matatk added a commit that referenced this issue Sep 14, 2018
@matatk
Attempt to fix background comms and navigation on Firefox
57e955b 
Note: there are still problems with the workaround for content script
	  non-reinjection on Firefox, so whilst the solution below seems
	  good, it doesn't actually work yet; still getting "WebExtension
	  context not found!" errors:
	  <https://bugzilla.mozilla.org/show_bug.cgi?id=1390715#c6>.

* Change the reconnection logic to use @lydell's method for coping with
  the background script not already being loaded on Firefox -
  https://bugzilla.mozilla.org/show_bug.cgi?id=1474727#c3 - thanks; I
  had been using an approach whereby it would try to reconnect n times
  in quick succession, but timing-based solutions are a bit yucky,
  whereas your method is elegant :-). Addresses #201.
* At least temporarily reverse the changes in 6ea4e56 to make the code
  simpler.
* Use @rpl's method for supporting the content script not being
  re-injected when navigating back to a previous page on Firefox -
  https://bugzilla.mozilla.org/show_bug.cgi?id=1390715#c4 - also thanks;
  I was really stuck with this and would certainly not arrived at the
  same conclusion and clean approach to the workaround :-). Ref #202.
* Reinstate some of the background script logging from b766e49 to help
  with the content script re-injection/context errors ongoing
  investigation.
* Add a FIXME to sort regarding DevTools being persistent across reloads
  on Chrome-like browsers.
@matatk matatk mentioned this issue Oct 20, 2018
Merged
@matatk matatk mentioned this issue Jan 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@matatk matatk

Labels
enhancement
Projects
None yet
Milestone
2.4.0
Development

No branches or pull requests
1 participant
@matatk