feat: support split-view in Edge, tab-tiling in Vivaldi #608
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
github-actions
bot
added
area: content
Extension builds preview
|
){kind=icon}
){kind=icon}
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
sidvishnoi
force-pushed
the
edge-split-view
branch
from
59b2818
to
fadaa16
Compare
sidvishnoi
changed the title
sidvishnoi
force-pushed
the
edge-split-view
branch
from
fadaa16
to
a9758b2
Compare
|
Demo with Edge split view: Screencast.from.18-09-24.06.24.06.PM.IST.webm |
|
Demo with Vivaldi tab-tiling! Screencast.from.18-09-24.06.53.57.PM.IST.webm |
sidvishnoi
changed the title
sidvishnoi
force-pushed
the
edge-split-view
branch
from
edb6564
to
98e3868
Compare
sidvishnoi
changed the title
sidvishnoi
changed the title
Tymmmy
approved these changes
Sep 19, 2024
…frames)" This reverts commit f2ec374. Oops, clicking in iframe directly won't focus tab if we did this.
(looking at you, Vivaldi tab-tiling) nits
|
@sidvishnoi I have to ask, is this story ready to review? Since I reviewed it and gave approval, you pushed at least 5 new commits. I am asking so that we know when to actually review? |
|
@Tymmmy The commits I pushed are things I found with more testing, and apparently, there's enough edge cases that can cause problems with Edge (Vivaldi is fine). I'll ask for a re-review tomorrow after more testing. I would have written E2E tests for this, but there's no APIs to trigger the behavior I'm developing for here. (7c57663 was a realization that I was thinking I'd do as a follow-up PR, but decided to add to this only as two reviews are still pending) |
lengyel-arpad85
approved these changes
Sep 25, 2024
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.
Context
Closes #67
Why:
browser.tabs.query()skips tabs in right-pane of split-view, as if it don't exist. So, we need to keep track of existing tabs in a window ourselves.Changes proposed in this pull request
TAB_FOCUSEDevent from content script, on load (if focused) and on focus change (if focused).sender.tab.idfrom above event to store last focused tabId for given window.WindowStateservice.browser.action.setIcon()with a tabId, it'll only update the icon for the left pane. So, added abrowserNamevalue, and skip usingtabIdfor Edge when setting icon. This will set a global icon for Edge, but other browsers stay like before.onActivatedTabwhich is buggy in Edge with split-view, rely onTAB_FOCUSEDevents'sender.tab.idto update icon state. Also use this last focused tab to show popup content.visibilitychange, which works like before), we sendIS_TAB_IN_VIEWmessage to content script (top frame only) to know if tab is currently visible. This way, we get all visible tabs in current window.IS_WM_ENABLEDmessage as unused.For reviewers: Radu is on vacation so I was asked to ask you good folks instead.
To be decided
Right now, the rate of pay is decided per tab, using the global rate of pay. Now we have multiple active visible tabs getting monetized, so the effective global rate of pay is multiplied by number of active visible tabs - user is paying more than they initially set to as they're paying multiple websites at once. In future, we'll have exception lists (#146), which will again make rate of pay per site/tab.