User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 Steps to reproduce: See bug 1419947 for more discussion. I introduced the concept of tab successors into tabBrowser in a patch attached to that bug; this bug is for tracking the WebExtensions API changes needed to enable the use cases described in that bug.

Add an optional previousTabId property to the onActivated event, which is present if the previously activated tab is still open.

1. Add successorId to the Tab type, so that it will be returned in, e.g., browser.tabs.get calls 2. Extend or create the following methods on the browser.tabs API: - update: add successorTabId as an optional property on the provided updateProperties object - moveInSuccession: new method that manipulates tab successors in bulk - moveInSuccessionAfter: ditto Depends on D9271

Please note that these two patches don't have any tests yet; I'm looking for some early feedback on the design here while I figure out how to test them.

Both patches now have tests and are ready to be fully reviewed.

Hey, I think this is more or less ready to be landed—I addressed all outstanding feedback on the patches and the reviewers gave provisional approval on a previous version. Can someone do a final sanity check and nudge this forward?

Oh, that's been sitting a while. Are you able to run a try before landing? Make use of needinfo, it would have got on someones radar faster.

I tried to land this earlier today but the patch failed to apply: https://lando.services.mozilla.com/D9271/

Both patches have been rebased; try again please? (I don't think I have access to run tries?)

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/02ea631f55c3 Part 1: browser.tabs.onActivated; r=mixedpuppy,rpl,JanH

*Sigh* Apparently only part 1 got landed. Is this self-contained or is it going to fail on automation without part 2?

Part 1 should pass on its own (although I haven't actually tested it on its own since my first version of that patch).

Okay, I'll try to land part 2 separately now.

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6f209bee42c5 Part 2: expose tab successors in browser.tabs; r=mixedpuppy,rpl

backed out for failing bc at browser/components/extensions/test/browser/browser_ext_tabs_events.js and mochitest at browser/components/extensions/test/mochitest/test_ext_all_apis.html Pushes that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&&revision=02ea631f55c3ad689fff3e24df75e810cbe042ad and https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=6f209bee42c5a07163c89d21beb7e7062e2c75b1 Failure logs: bc: https://treeherder.mozilla.org/logviewer.html#?job_id=211733807&repo=autoland&lineNumber=2500 mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=211737870&repo=autoland&lineNumber=1640 backout: https://hg.mozilla.org/integration/autoland/rev/438b2437a3a7dbc542811c79f11144e8b6b62fbf

I fixed the problems flagged in those logs; once more?

(In reply to ryan.hendrickson from comment #15) > I fixed the problems flagged in those logs; once more? We just flagged the patch with a question, want to figure that out before re-landing.

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/838185d7a6e7 Part 1: browser.tabs.onActivated; r=mixedpuppy,rpl,JanH https://hg.mozilla.org/integration/autoland/rev/e93f7c2b5263 Part 2: expose tab successors in browser.tabs; r=mixedpuppy,rpl

Backed out for Backout link: https://hg.mozilla.org/integration/autoland/rev/06be7f6e26ec99e904c705ceb8a84fe2964aab7e Push link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&selectedJob=212022434&revision=e93f7c2b5263bc5faa3822114dc6b18dca7d2cb9 Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=212022434&repo=autoland&lineNumber=29195

<sigh> Okay, that mole was whacked. Sorry for the churn; some of the browser test suites seem to randomly hang for me (even before my changes), or I'd make more of an effort to run all of them. Uno mas?

Poke?

Ryan, we have the try server for running tests in automation before landing: https://wiki.mozilla.org/ReleaseEngineering/TryServer I started a run for you with your latest patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f02e21f24cc68cdb68f9b3743d2b4ef3593c471b

Sure, but I'm just a rando--if I applied for level 1 commit access (in order to submit try jobs in the future), would you or someone else on this bug be able to vouch for me, just on the basis of this interaction? Or is that just for interns/mentees/people with actual relationships with Mozilla?

There's a lot of overlap between Mozilla employees and committers (and peers etc.) but they are not 1:1. The governance model is described in great detail on the wiki. In any case, I'd be happy to vouch for you for level 1 commit access, please needinfo? me on the bug you open(ed) for commit access.

I hate to exercise your patience further, but I can't interpret the results of the run you started for me. It looks like no tests were able to run on Android 4.3, but beyond that I can't find any logs pointing to why that is or what I should do about it. All the Treeherder User Guide is able to tell me is that orange means tests failed, and the wiki doesn't seem to have a cheatsheet on what any of this alphabet soup means (I checked the page you linked and the page linked from the Treeherder User Guide). Where does a newbie go to figure out how to be productive with the try server?

The raw log (the second icon on the toolbar at the bottom view when you click on an orange [X1] contains the following near the end: [task 2018-11-20T02:31:29.155Z] 02:31:29 INFO - no tests to run using specified combination of filters: skip_if, run_if, fail_if, pathprefix(['browser/components/extensions/test']) [task 2018-11-20T02:31:29.173Z] 02:31:29 ERROR - Return code: 1 [task 2018-11-20T02:31:29.174Z] 02:31:29 ERROR - No tests run or test summary not found This means that there is not any test that matches the try syntax: > try: -b d -p linux64,android-api-16 -u mochitest-1,mochitest-browser-chrome-1,mochitest-browser-chrome-e10s-1,mochitest-e10s-1,mochitest-e10s-browser-chrome-1,xpcshell --try-test-paths browser-chrome:browser/components/extensions/test mochitest:browser/components/extensions/test xpcshell:browser/components/extensions/test --artifact The try syntax doesn't contain anything that would match the Android test that you have added here (possibly because the try job was generated on desktop). From comment 18, if you look at the test failure on Android, you can see that it is a TV failure. That means that the new test in your push was run several times under different conditions to see if there are any potential race conditions in the test or implementation. If you click on [TV], you will see the following at the left of the bottom sidebar: Job name: test-android-em-4.3-arm7-api-16/debug-test-verify At https://mozilla-releng.net/trychooser/ you can try to recreate the same task: - Select debug (let's also do opt just for a good measure) - Select an Android platform (android api-16+) - Use artifact build (you didn't edit C++ code) - Select the test-verify-e10s checkbox Result: try: -b do -p android-api-16 -u test-verify-e10s -t none --artifact Once your commit access is activated (bug 1508537), you can try it yourself. You can also run the test locally with the --verify flag; something like this should work: mach test mobile/android/components/extensions/test/mochitest/test_ext_tabs_events.html --verify For instructions to test locally, possibly an emulator, see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Rob, thanks for your help. Here's the result of running the try command you suggested: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3f988969d12f22f3b7d17c6d6ee913abf0cb932 The job succeeded, but it doesn't look like any tests actually ran. Is that right? Do I need to select more of the test suites? Is just selecting all of them considered a waste of resources, or is that frequently done?

The TV tests did not run indeed. Since the try syntax isn't helping, you can also just use the web interface of Treeherder instead; especially because you know which tests you'd like to run (from the push in comment 18). I started the TV tests in your push above; to do it yourself see: https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try If you want to retry a job, and that job has alredy been added once, then to retry you can simply select the job (to show the bottom view) and click on the "Repeat the selected job" button (it is the icon with the curved arrow) in the toolbar of the bottom view.

(In reply to ryan.hendrickson from comment #26) > Rob, thanks for your help. Here's the result of running the try command you > suggested: > > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=f3f988969d12f22f3b7d17c6d6ee913abf0cb932 The above try job does have the TV failures on android debug builds (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3f988969d12f22f3b7d17c6d6ee913abf0cb932&selectedJob=213108049) Based on the errors logged in the failure (e.g. 'Error: Type error for tab value (Unexpected property "successorTabId") for tabs.create.' or 'Error: Type error for result value (Error processing 0: Unexpected property "successorTabId") for tabs.query.'), the reason for these failures should be that we validate the properties of the objects returned/resolved from the API methods using the their related definition in the API schemas (and we do that only in debug builds, e.g. see https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/components/extensions/Schemas.jsm#2278-2284). In D9272 "successorTabId" is added to "browser/components/extensions/schemas/tabs.json", but it doesn't exist yet in the related API schema file used by Fennec (the "mobile/android/components/extensions/schemas/tabs.json" one), nevertheless "successorTabId" is also populated on Fennec (always set to -1 in D9272), and so I expect that the test fails consistently on debug builds because this property in unexpected on Fennec based on the schema. Adding "successorTabId" definition to "mobile/android/components/extensions/schemas/tabs.json" should be enough to fix that failure. > The job succeeded, but it doesn't look like any tests actually ran. Is that > right? Do I need to select more of the test suites? Is just selecting all of > them considered a waste of resources, or is that frequently done? Yeah, selecting everything is a huge waste of resources and it is definitely not the solution, "selecting the right tests in a try job" seems hard and confusing initially, but it also becomes way easier with some practice (e.g. after a bit you become able to recognize which is the try job syntax that you need more often).

Ah, thank you, that was very helpful! The fact that the schemata are only checked on debug builds was especially good to know; I didn't realize I was testing an optimized build locally, and that was the missing piece I needed to reproduce the test failure locally. I've updated and rebased the patches once more. My most recent try run is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4584c591df12e5b15888c20ef1b583812f950b07 There are two jobs that failed initially, but succeeded on a retry. One of the failures looks like an intermittent failure that is already documented; the other (the TV job) is in a test suite that I touched, but it doesn't look like something I could have caused--of course, I could be mistaken. Should I file a bug for that failure? And is this good enough to try landing the patches once more, or is the TV job cause for more concern than that?

The TV failure seems unrelated to your patch. It looks like there are already some reports of it: bug 1509755 , bug 1498390 , bug 1499621 , bug 1503096 Since another retrigger of the TV has passed, and your original debug-only issue has been resolved, I believe that it is safe to retry landing the patches.

Land please?

To land the patch, edit the bug and add the checkin-needed keyword.

Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f5bf87ab6448 Part 1: browser.tabs.onActivated; r=mixedpuppy,rpl,JanH

Pushed by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cd420001c8ea Part 2: expose tab successors in browser.tabs; r=mixedpuppy,rpl

https://hg.mozilla.org/mozilla-central/rev/f5bf87ab6448 https://hg.mozilla.org/mozilla-central/rev/cd420001c8ea

For my understanding, I've wrote effective usages of new APIs, especially about the tabs.moveInSuccession(), with many figures. https://qiita.com/piroor/items/fb969c94a41c36fd56f5 This article is written in Japanese, so I'll try to translate it to English...

I'm not a Japanese speaker, but even the Google-translated version makes it clear that this is a very detailed and carefully written article; well done! There's one detail about tab successors that I don't think you mentioned (sorry if I missed it; Google's translation was pretty rough), which helps to explain why moveInSuccession is important, besides the performance benefits which you did mention. When Firefox closes a tab, the succession order automatically adjusts around it. So if the succession order is A -> B -> C, and B is closed (or moved to another window), then A's successor will be set to C by Firefox—the add-on doesn't need to manage this. So even though it seems like you can do the same thing moveInSuccession does with the low-level get and update functions, the asynchronous communication between the Firefox parent process and the add-on makes it theoretically possible that something about the set of tabs will change while the add-on is working on setting up the successor tabs in the desired order. For example, if I'm trying to set the chain A -> B -> C without moveInSuccession, the user (or another add-on) may close B after I've set it as the successor to A, but before I've set C as the successor to B. In that case, A will take as its successor whatever B's original successor was, which is not what I'd want (I want it to be C). moveInSuccession, in contrast, is atomic—it changes all of the successors it's going to change at once, and so any tab events that might affect the succession order will only take effect entirely before or entirely after the moveInSuccession call does its work. In the example above, if B is closed before moveInSuccession is handled, then moveInSuccession will ignore the invalid tab B and just set A -> C. If B is closed after moveInSuccession is handled, then Firefox will adjust the succession order from A -> B -> C to A -> C—so it's the same result either way. So that's why there is both a low-level way and a high-level way to set tab successors. I left the low-level option in the design for two reasons: moveInSuccession *is* harder to understand, and it's possible there's some use case I haven't thought of where moveInSuccession is insufficient. But I personally use moveInSuccession only, even when just setting one tab's successor, because moveInSuccession closes up the succession order behind the moved tab(s) the same way that Firefox does when a tab is closed, and it does that atomically. And that's what I'd recommend to anyone else manipulating tab successors, unless they find some problem with it.

Thank you for comments, Ryan! I've updated/translated the article to English and published: https://qiita.com/piroor/items/ea7e727735631c45a366 Mirror: https://piro.sakura.ne.jp/latest/blosxom/mozilla/xul/2018-12-03_successor-tabs-api-en.htm

Thunderbird bug 1511838.

It is my understanding that automated tests are prepared and can be run, will you also require manual validation? if yes please specify some steps on howto correctly validate, thanks!

Vlad, I don't think manual testing is necessary for this; the automated tests cover all the actions and effects I would think of to test. (But if there's a policy encouraging manual testing for this kind of feature anyway, I could write a test plan for you.)

The main thing is for the feature to be validated and make sure it works correctly, if this is done through automation and it's comprehensive enough to be marked validated, it's good enough for me, just let me know if you need my assistance on this, I'll flag it as qe-validate- until then.

Note to MDN writers: I've added a set of notesto the Fx65 rel notes to cover this addition: https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#API_changes Hope that covers it! From those notes, the documentation to add should be fairly self explanatory.