EIP-1102 updates #6006
EIP-1102 updates #6006
Conversation
|
Hey @bitpshr, Status here. What's your target date lately for enabling privacy mode by default? We were planning to do so in our next release (~early Feb). |
| @@ -65,9 +65,11 @@ class ExtensionPlatform { | |||
| } | |||
|
|
|||
| sendMessage (message, query = {}) { | |||
| extension.tabs.query(query, tabs => { | |||
| const id = query.id | |||
| delete query.id | |||
There was a problem hiding this comment.
Why are we deleting the id from the query here?
There was a problem hiding this comment.
Querying tabs worked oddly in Chrome and Firefox. If the id was not deleted before querying, no tabs would ever be returned. This is why this roundabout method of sending to a specific ID is used: if an id is present on a query, we cache it, delete it, run the rest of the query, then only send the resulting message to tabs matching the cached id.
| tabs.forEach(tab => { | ||
| extension.tabs.sendMessage(tab.id, message) | ||
| extension.tabs.sendMessage(id || tab.id, message) |
There was a problem hiding this comment.
If the id is passed in and we're querying all the tabs, will this send the same message to the same tab multiple times?
There was a problem hiding this comment.
Should we be sending a message to tab id if that's passed in, else iterate all the tabs?
There was a problem hiding this comment.
I feel like a query with multiple properties should hit tabs that satisfy all properties, not just some. This is why we 1) query without the ID, then 2) iterate through the results and further filter by ID. If we detected ID first then bailed, that implies an ID takes precedence over all other query properties.
There was a problem hiding this comment.
Hmm, maybe I don't understand what these APIs are doing well enough. If we call platform.sendMessage(foo, 42), we'll in turn be calling extension.tabs.sendMessage(42, foo) multiple times (once for each tab returned in the query), no?
There was a problem hiding this comment.
Whymarrh's concern made sense to me, and then I tried to word it and understood Paul's point:
The tabs.query method does not take an id parameter and filter for it, and so this basically allows our own platform.sendMessage() method to accept an id in its query, and then only send to that tab.
| approveProviderRequest: origin => dispatch(approveProviderRequest(origin)), | ||
| rejectProviderRequest: origin => dispatch(rejectProviderRequest(origin)), | ||
| approveProviderRequest: tabID => dispatch(approveProviderRequest(tabID)), | ||
| rejectProviderRequest: tabID => dispatch(rejectProviderRequest(tabID)), |
There was a problem hiding this comment.
I would normally raise a concern about mixing platform-specific details (like tab ID) in with the UI, but I have a proposal for a revised approach to this I want to share with you soon (maybe tomorrow?), that I think will help clean up a lot of this tab management logic.
|
You'll have to pull in latest |
whymarrh
force-pushed
the
1102-updates
branch
from
9320c38 to
13258ea
Compare
This pull request includes a few bugfixes around EIP-1102:
Fixes #5941
Fixes #5951
Fixes #5959
Fixes #5993