Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Description • 6 years ago

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.

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 1 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 2 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 3 • 6 years ago

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.

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 4 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 5 • 6 years ago

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?

	Shane Caraveo (:mixedpuppy)
	Comment 6 • 6 years ago

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.

	Dão Gottwald [:dao]
	Comment 7 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 8 • 6 years ago

Both patches have been rebased; try again please?

(I don't think I have access to run tries?)

	Pulsebot
	Comment 9 • 6 years ago

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

	Dão Gottwald [:dao]
	Comment 10 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 11 • 6 years ago

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

	Dão Gottwald [:dao]
	Comment 12 • 6 years ago

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

	Pulsebot
	Comment 13 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 14 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 15 • 6 years ago

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

	Shane Caraveo (:mixedpuppy)
	Comment 16 • 6 years ago

(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.

	Pulsebot
	Comment 17 • 6 years ago

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

	Narcis Beleuzu [:NarcisB]
	Comment 18 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 19 • 6 years ago

<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?

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 20 • 6 years ago

Poke?

	Andrew Swan [:aswan]
	Comment 21 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 22 • 6 years ago

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?

	Andrew Swan [:aswan]
	Comment 23 • 6 years ago

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.

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 24 • 6 years ago

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?

	Rob Wu [:robwu]
	Comment 25 • 6 years ago

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

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 26 • 6 years ago

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?

	Rob Wu [:robwu]
	Comment 27 • 6 years ago

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.

	Luca Greco [:rpl] [:luca] [:lgreco]
	Comment 28 • 6 years ago

(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).

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 29 • 6 years ago

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?

	Rob Wu [:robwu]
	Comment 30 • 6 years ago

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.

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 31 • 6 years ago

Land please?

	Rob Wu [:robwu]
	Comment 32 • 6 years ago

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

	Pulsebot
	Comment 33 • 6 years ago

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

	Pulsebot
	Comment 34 • 6 years ago

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

	Cosmin Sabou [:CosminS]
	Comment 35 • 6 years ago

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

	YUKI "Piro" Hiroshi
	Comment 36 • 6 years ago

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...

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 37 • 6 years ago

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.

	YUKI "Piro" Hiroshi
	Comment 38 • 6 years ago

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

	Ben Bucksch (:BenB)
	Comment 39 • 6 years ago

Thunderbird bug 1511838.

	Vlad Jiman
	Comment 40 • 6 years ago

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!

	Ryan Hendrickson [rhendric on GitLab, GitHub] Assignee
	Comment 41 • 6 years ago

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.)

	Vlad Jiman
	Comment 42 • 6 years ago

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.

	Chris Mills [:cmills]
	Comment 43 • 5 years ago

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.