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

Use Messenger for executeInTarget #2044

Closed
wants to merge 3 commits into from
Closed

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Dec 4, 2021

Tests done

200 Successful

  1. brick: open tab
  2. brick: alert in Target:target

404 Missing target

  1. brick: alert in Target:target
    • it doesn't exist, so it will receive BusinessError("Sender tab has no target")

410 Outdated target

  1. brick: open tab
  2. brick: wait 2000 ms
  3. manually close tab
  4. brick: alert in Target:target
    • it no longer exists exist, so it will receive BusinessError("Sender tab has no target")

Comment on lines -114 to -116
throw new BusinessError(
`Tab ${tabId} was not ready after ${maxWaitMillis}ms`
);
Copy link
Contributor Author

@fregante fregante Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific error no longer exists because the retries are handled automatically by webext-messenger when forwarding it. The max retry time is fixed/global to around 5s, but it only applies "per hop", i.e. the background can take 4s to answer, then the content script can wait 4s more, etc.

I suppose this is an issue on large pages that have document_idle late.

If you think this is an issue, I could add an option¹ for this or increase the default timeout.

1 options are kind of difficult at the moment, 🤭 which is why I split getMethod from getNotifier instead of using isNotification: true

Copy link
Contributor Author

@fregante fregante Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I'm bringing the handler back to this context, I will restore this function 🎉

Comment on lines +27 to +29
export const target = {
id: -1,
};
Copy link
Contributor Author

@fregante fregante Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The target was previous stored in the background page and the messaging solution expected the background page to match "sender" with "target".

With the messenger we can message the tab "directly" (i.e. the messenger takes care of forwarding) so we can store it locally (which is where the tab is created anyway).

This might not be the best solution but it's clean-ish (when messaging the target, you'll have to import this block, which shows the direct dependency between the two)

Copy link
Contributor

@twschiller twschiller Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The target was previous stored in the background page and the messaging solution expected the background page to match "sender" with "target".

This change actually changes the behavior. The old code also tracked if a normal link opened up a new tab. With this new code the "target" relationship will only be tracked through this block?

  • Check impact on tabs opened through 'a'

See the bookkeeping via browser.tabs.onCreated at initExecutor: http://github.com/pixiebrix/pixiebrix-extension/blob/main/src/background/executor.ts#L300-L300

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. In that case we can exclusively send messages via the background page because that's the only context that can know that information. I'll have to restore the previous manual forwarding

@fregante fregante marked this pull request as ready for review December 4, 2021 10:47
Comment on lines 43 to 45
const MESSAGE_RUN_BLOCK_OPENER = `${MESSAGE_PREFIX}RUN_BLOCK_OPENER`;
const MESSAGE_RUN_BLOCK_TARGET = `${MESSAGE_PREFIX}RUN_BLOCK_TARGET`;
const MESSAGE_RUN_BLOCK_BROADCAST = `${MESSAGE_PREFIX}RUN_BLOCK_BROADCAST`;
const MESSAGE_RUN_BLOCK_FRAME_NONCE = `${MESSAGE_PREFIX}RUN_BLOCK_FRAME_NONCE`;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A note about the other targets:

Copy link
Contributor

@twschiller twschiller Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadcast likely similar implementation to Replace notifyContentScripts with Messenger #1329

Unlike notifications, broadcast actually returns the results as the array. You mean in terms of how it notifies all the tabs?

frame_nonce I haven't looked at it yet, but maybe via {tabId: "this", page: "?nonce=ABC"}

I think using the more general page matching capability here to support our nonce based frame approach makes sense here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using the more general page matching capability here to support our nonce based frame approach makes sense here

I'm starting to think that all of these methods will continue existing in the background page because that's the only context that can do this.

Copy link
Contributor

@twschiller twschiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about the "target" book keeping. How does the target in redirectPage file relate to tabToTarget in the executor?

I might be missing something?

@fregante
Copy link
Contributor Author

fregante commented Dec 7, 2021

You're right, I left a comment in the review. I'll preserve the background-based manual forwarding as before, except with the messenger instead of sendMessage

@fregante fregante marked this pull request as draft December 7, 2021 04:14
@fregante
Copy link
Contributor Author

fregante commented Dec 8, 2021

None of this is useful unfortunately, I'll reopen a PR with new code that only replaces the sendMessage with the messenger.

@fregante fregante closed this Dec 8, 2021
@fregante fregante deleted the F/messenger/run-in-target branch December 8, 2021 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants