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.

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

New named targets solution + add some notes/docs #53

Merged
merged 25 commits into from
Nov 28, 2021
Merged

Conversation

fregante
Copy link
Collaborator

@fregante fregante commented Nov 17, 2021

New attempt at implementing:

The logic has been linearized as much as possible here and in the past PR, but it's still a bit hard to follow and reason about because there are so many small things to consider. I added some (partial) notes to docs/notes.md in this PR.

In short:

  • messages received by the content scripts and background page can be identified immediately
  • messages received by other extension pages require a "whoAmI" request first, regardless of whether they're frame: 0 or some other sub-frame.

This is what happens specifically in the latter part, for example:

Sender (background) Receiver (sidebar)
send message to `{tabId: 1, page: "action.html"}`
sidebar might still be loading, so message is ignored
retries
sidebar loads
sidebar receives message, but doesn't know `whoAmI`, so it's again ignored
retries
sidebar knows it's `{tabId: 1, frameId: 100, page: "action.html"}`
sidebar receives message, every key of the `target` matches, so it responds
receives message

Here I just spread the messages a little bit to highlight the "race condition" but in reality things will probably be faster, i.e. the sidebar will already know whoAmI at the first message.

Missing from this PR

  • Existing tests pass
  • Match messages from own tab even if target doesn't mention its tab ID, i.e. {page: "action.html"} from the content script should be handled in the same tab
  • Add tests for named targets
  • Add to pixiebrix
  • Maybe add support for {frameId: 0} message so that the sidebar can message the top frame directly

@fregante
Copy link
Collaborator Author

This solution has advantages over the previous one:

  • Messages can go directly from the content script to the sidebar, without passing through the background page, without having to register oneself with the background page (which may fall out of date)
  • Opening and closing a sidebar doesn't mean querying the background page again for a new target; if a new sidebar is created, it will just respond automatically exactly like the first one.

fregante added a commit that referenced this pull request Nov 24, 2021
@twschiller
Copy link
Collaborator

I read over the notes, I think the approach makes sense to try. A couple thoughts/questions:

  • For page contexts that might appear multiple times on a page, e.g., page: "/iframe.html" can the frameId be passed also? (This sort of negates the usefulness of named targets, but is necessary?). In practice the only context that's guaranteed to be unique on a tab is the sidebar
  • Whenever a page receives its whoami, it could notify via chrome.runtime so all the extension pages know a new context is available. The sender could use that signal to retry faster than its normal retry interval?

@fregante
Copy link
Collaborator Author

  • can the frameId be passed also?

If you know the frame ID, then you don't need the page name, tab/frame tuples are unique.

As we verify and expand this feature, we could also add UUID-based targets, e.g. {page: '/form.html?uuid=abc'}, which would make it easy to create an iframe and immediately target it unequivocally.

  • all the extension pages know a new context is available

That would only work between extension pages, but this PR’s solution is mainly for direct brick -> frame communication. If we notice a large delay between retries we can consider this implementation, but I think it's easier to just reduce the delay.

@fregante fregante marked this pull request as ready for review November 28, 2021 16:14
@fregante
Copy link
Collaborator Author

fregante commented Nov 28, 2021

Maybe add support for {frameId: 0} message so that the sidebar can message the top frame directly

I'll address this in a future PR, after this solution is confirmed to work and merged into PB

@fregante
Copy link
Collaborator Author

Will merge and release as 0.15 since it was merged in PB 🚢

@fregante fregante merged commit bb3b50f into main Nov 28, 2021
@fregante fregante deleted the new-named-targets branch November 28, 2021 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants