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
Shirk my SDK code #605
Shirk my SDK code #605
Conversation
958ac1e
to
3b20ef9
Compare
3b20ef9
to
b33b99c
Compare
1bbbb53
to
d07d371
Compare
I verified this mostly works, and the most noticeable bugs are already listed in #486. Needs a merge from master to resolve conflicts. |
@@ -0,0 +1,64 @@ | |||
const tabPageCounter = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove this ... either now, or in a separate branch to prepare for AMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do later I wasn't sure if we wanted to account for opt in telemetry at any point. Was a little tricky to reengineer this and other telemetry bits (this was pre decision to drop it totally that I did the work) So I would rather leave it in as totally non functioning for now. We could consider supporting telemetry for people who want to help us also.
d07d371
to
e6ba916
Compare
@kjozwiak looks like you were already assigned anyway |
Things still look good. But when I installed this onto my existing 2.4.0 add-on, I lost all my container tabs? 😢 |
@groovecoder just the tabs or the containers too? |
@groovecoder I couldn't replicate this when renumbering this to 3.0.0 after rebasing. My STR was making the two XPIs from master and this branch:
|
f58efb4
to
2af75f5
Compare
@groovecoder I was able to replicate #703 following the same STR from about:addons though |
Okay, I re-tried again on 54, 55, 56, and 57 Nightly and I wasn't able to reproduce losing tabs & containers. So I think this is ready as soon as the travis-ci/ |
Merged #707 so this needs rebase & |
2af75f5
to
fe77c89
Compare
@groovecoder should be good to go merged and dead headed the code. I tested my TODO comments I removed, works with less code 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and works well. Let's plan to push this to production on Monday.
No description provided.