Closed Bug 1506782 Opened 5 years ago Closed 1 year ago

Add support for opening and closing of tabs for GeckoView applications (Marionette and WebDriver BiDi)

Categories

(Remote Protocol :: Agent, task, P1)

Product:
Remote Protocol
Component:
Agent
Platform:
Unspecified
Android
Type:
task
Points:
2

Tracking

(firefox105 wontfix, firefox106 fixed)

Status:
RESOLVED FIXED
Milestone:
106 Branch
Tracking Flags:
Tracking Status
firefox105 --- wontfix
firefox106 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [webdriver:m4][webdriver:relnote])

Attachments

(1 file)

Bug 1506782 - [remote] Add support for opening and closing tabs on Android.
48 bytes, text/x-phabricator-request
Details | Review 
On bug 1506643 I set the `browser.newtabpage.enabled` pref to false to prevent loading the `about:newtab` page when opening new tabs. I would like to see something similar for Fennec, which also shows such a page, but the preference from above doesn't seem to be related.

James, mind telling me how this page can be disabled? Thanks.
Flags: needinfo?(snorp)
I don't really have knowledge of how this works in the Fennec UI. Someone on Susheel's team should be able to help.
Flags: needinfo?(snorp) → needinfo?(sdaswani)
As a P3 this will show up in triage at some point.
Flags: needinfo?(sdaswani)
(In reply to :sdaswani only needinfo from comment #2)
> As a P3 this will show up in triage at some point.

No, because this bug is not part of the Fennec product but Marionette. As such it won't appear in any triage you do.
Flags: needinfo?(sdaswani)
Sorry - can you help me with priority here? What does this work block, and how important is it?
Flags: needinfo?(sdaswani)
With this work I want to make navigation for Marionette tests more reliable, and faster. The tests don't need the `about:newtab` experience including activity stream, but only have to open `about:blank`.

The only information needed here is how to turn off activity stream so that just `about:blank` gets loaded when opening new tabs.
Flags: needinfo?(sdaswani)
Liz how can we prioritize this against our other Fennec needs?
Flags: needinfo?(sdaswani) → needinfo?(lhenry)
Probably pretty easy to do, and it's good to improve automated test reliability - why not make it a P2. Vlad can your team take a look?
Flags: needinfo?(lhenry) → needinfo?(vbacia.work)
Priority: P3 → P2
I actually don't understand why we make it such a complicated process from just a question about which preference to use to display `about:blank` and not the activity stream in Fennec. Is there no such preference yet?

Again, this is not a bug for Fennec but Marionette and as such any whiteboard/severity setting doesn't apply.
Priority: P2 → P3
Flags: needinfo?(vbacia.work)

So I found that through the setting I can set the home page to "about:blank", and to let Fennec use it for each new tab. But I have problems figuring out how Marionette could set it, because those don't seem to be preferences.

Both are android:key entries here:

https://searchfox.org/mozilla-central/rev/227f5329f75bd8b16c6b146a7414598a420260cb/mobile/android/app/src/main/res/xml/preferences_home.xml#14,20

Where are those preferences set? James, could you please help? Thanks

Flags: needinfo?(snorp)

We weren't able to get this done via bug 1533058, because of the following call:

tab = this.tabBrowser.addTab(null);

Usually it would allow to pass in about:blank as URL, but as jgraham said null was necessary to use for GeckoView to kill the race condition. I haven't further investigated yet, but I hope that with being able to set the above preferences that we won't need neither of those.

Type: enhancement → task
Whiteboard: [geckoview:p3]
OS: Unspecified → Android

Parachuting into this conversation. In general there's a big split between App-level settings and Gecko-level settings, and in general Mozilla's Apps (Fennec, Fenix, r-b, etc) don't have a uniform way to manage App-level settings in automation. I have considered writing harness code to set Android SharedPreferences (which is what :whimboo is ultimately referring to in #c9), but it's very fiddly and requires either support from within Marionette/Gecko or the App or to have android:debuggable="true".

I have had success adding per-App Intent arguments to control these types of things from automation: e.g., https://github.com/mozilla-mobile/fenix/issues/1525.

For this particular issue, in my browsertime harness, I use the "skipstartpane" extra for Fennec and launch with a URL before driving Marionette: see https://searchfox.org/mozilla-central/search?q=EXTRA_SKIP_STARTPANE&redirect=false for the Intent argument.

Yeah, comment #11 captures the problem -- an intent argument is a good way to add this kind of thing to Fennec.

Flags: needinfo?(snorp)

This bug should also cover GeckoView given that Fennec goes away soon. So I assume that an intent argument will also work with GeckoView based browsers?

Summary: Replace `about:newtab` like page for Fennec tests with `about:blank` → Replace `about:newtab` like page for Fennec and GeckoView with `about:blank`

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #13)

This bug should also cover GeckoView given that Fennec goes away soon. So I assume that an intent argument will also work with GeckoView based browsers?

Each product would need special attention, but yes, it's possible -- GV doesn't have any role in this.

I would like to get back to this bug again. How is it possible these days to set about:blank for a new tab that gets e.g manually opened via the tabs pane / view? The intent arguments -a android.intent.action.VIEW -d about:blank only seem to affect the initial start page but not any tab that gets opened later. Is that a GeckoView specific setting or related to the Android app embedding GeckoView?

Note that we won't add support for that in Fennec anymore.

Nick maybe you can help or forward the question appropriately? Thanks.

Flags: needinfo?(nalexander)
Summary: Replace `about:newtab` like page for Fennec and GeckoView with `about:blank` → Replace `about:newtab` like page for GeckoView applications with `about:blank`

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #15)

I would like to get back to this bug again. How is it possible these days to set about:blank for a new tab that gets e.g manually opened via the tabs pane / view? The intent arguments -a android.intent.action.VIEW -d about:blank only seem to affect the initial start page but not any tab that gets opened later. Is that a GeckoView specific setting or related to the Android app embedding GeckoView?

That's correct: these intent arguments just impact the initial launch.

Note that we won't add support for that in Fennec anymore.

Nick maybe you can help or forward the question appropriately? Thanks.

My guess is that the App (Fenix, Reference Browser) controls what happens when a new tab is opened, and that we probably display some kind of "search/recall" UI like we do on iOS. So my guess is that you should start talking to the Fenix people more than the GV people. A quick skim of relevant GV sources strongly suggests that GV is doing the sensible thing and starting every tab with a navigation to about:blank. If you need to confirm, #geckoview on Matrix -- I expect many folks to know the details here, including :agi.

Good luck!

Flags: needinfo?(nalexander)

(In reply to Nick Alexander :nalexander [he/him] from comment #16)

My guess is that the App (Fenix, Reference Browser) controls what happens when a new tab is opened, and that we probably display some kind of "search/recall" UI like we do on iOS. So my guess is that you should start talking to the Fenix people more than the GV people. A quick skim of relevant GV sources strongly suggests that GV is doing the sensible thing and starting every tab with a navigation to about:blank. If you need to confirm, #geckoview on Matrix -- I expect many folks to know the details here, including :agi.

Thanks Nick. After a break of working on Android support for Marionette we would like to get back to this request.

Agi, could you maybe tell us which API we could use to create a new tab in a geckoview application which will then also present the about:blank page? A lot of tests meanwhile make use of this WebDriver API and it would be great to see it implemented for GeckoView. Thanks!

Flags: needinfo?(agi)

In general GeckoView does not deal with tabs, so we don't have an API for that.

What some places of GV do (e.g. tests) is pretending to be an extension and calling onNewTab, would that be a workable soulution for you?

I also wanna note that I don't think Fenix has a about:newtab equivalent, new tabs are always created to about:blank AFAIK.

Flags: needinfo?(agi)

Thanks Agi! I'll have a look in how we can get the tab opened. With bug 1723919 we will get better support for our tab handling wrappers and I might want to get this implemented via the MobileTabBrowser that you implemented a while ago. Also it's good to hear that about:newtab is not used.

Depends on: 1723919
Summary: Replace `about:newtab` like page for GeckoView applications with `about:blank` → Add support for opening a new tab with `about:blank` for GeckoView applications

We will enable the wdspec tests on Android first and then I hope to find the time to work on this issue hopefully soon.

No longer blocks: 1749444
Depends on: 1749444

Adding for triage so that we can discuss to get this included into our backlog and maybe get it even fixed in the M3 milestone. The bug should probably have at least 8 points.

Whiteboard: [geckoview:p3] → [webdriver:triage]
Points: --- → 8
Whiteboard: [webdriver:triage] → [bidi-m3-mvp]

I was trying to get opening a new tab implemented. As suggested by Agi I tried with a custom web extension by just copying what test-runner-support has. But I'm not able to get it installed most likely because I put it into the remote.jar file, whereby built-in webextensions might have to be located at resource://android/assets.

Agi, how could I package our extension so that it's getting correctly installed? Do I need some special packaging outside of Remote Agent to accomplish that? I'm somewhat blocked given that I cannot find any rules like in jar.mn files for Java packages. Thanks.

Flags: needinfo?(agi)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #23)

I was trying to get opening a new tab implemented. As suggested by Agi I tried with a custom web extension by just copying what test-runner-support has. But I'm not able to get it installed most likely because I put it into the remote.jar file, whereby built-in webextensions might have to be located at resource://android/assets.

Agi, how could I package our extension so that it's getting correctly installed? Do I need some special packaging outside of Remote Agent to accomplish that? I'm somewhat blocked given that I cannot find any rules like in jar.mn files for Java packages. Thanks.

I believe that as long as it's included in mobile/android/geckoview/src/main/assets it will work, no need to add jar.mn rules for that.

Flags: needinfo?(agi)

(In reply to Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from comment #24)

Agi, how could I package our extension so that it's getting correctly installed? Do I need some special packaging outside of Remote Agent to accomplish that? I'm somewhat blocked given that I cannot find any rules like in jar.mn files for Java packages. Thanks.

I believe that as long as it's included in mobile/android/geckoview/src/main/assets it will work, no need to add jar.mn rules for that.

Interesting. So there is no such assets folder for GeckoView in general, but as it looks like each application needs their own assets. That wouldn't be ideal for us given that we would like to ship it by default for any GeckoView application. Agi, do you think that having a mobile/android/geckoview/src/main/assets folder is fine? If yes, how / where it needs to be embedded into the build process? Maybe it's just /mobile/android/geckoview/build.gradle and everything in there will automatically be picked-up? Thanks a lot.

Flags: needinfo?(agi)

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #25)

(In reply to Agi Sferro | :agi | [slow ni? rn sorry] | ⏰ PST | he/him from comment #24)

Agi, how could I package our extension so that it's getting correctly installed? Do I need some special packaging outside of Remote Agent to accomplish that? I'm somewhat blocked given that I cannot find any rules like in jar.mn files for Java packages. Thanks.

I believe that as long as it's included in mobile/android/geckoview/src/main/assets it will work, no need to add jar.mn rules for that.

Interesting. So there is no such assets folder for GeckoView in general, but as it looks like each application needs their own assets. That wouldn't be ideal for us given that we would like to ship it by default for any GeckoView application.

This is not quite correct: all libraries, and the consuming application, contribute their assets/ to the final application package. This accommodates library assets (such as this one) in a distributed manner, without consuming application authors having to stitch together their dependents' assets.

Agi, do you think that having a mobile/android/geckoview/src/main/assets folder is fine? If yes, how / where it needs to be embedded into the build process? Maybe it's just /mobile/android/geckoview/build.gradle and everything in there will automatically be picked-up? Thanks a lot.

Yes, this should be fine, and yes, it's all automatic. I.e., convention over configuration in this case.

I'm sorry I should've been more explicit, it's totally fine to create the assets folder and it will behave like Nick is describing.

Flags: needinfo?(agi)

Discussed with Agi on Element and using a ServiceWorkerDelegate instead might be more ergonomic given that it wouldn't require to have to install the WebExtension delegate in each and every application. I filed bug 1771012 for it.

Priority: P3 → P1
Whiteboard: [bidi-m3-mvp] → [webdriver:m4]

Slightly updating the summary of the bug given that this affects both Marionette and WebDriver BiDi whereby the related code already lives in a shared module as used by both protocols.

Component: Marionette → Agent
Product: Testing → Remote Protocol
Summary: Add support for opening a new tab with `about:blank` for GeckoView applications → Add support for opening a new tab with `about:blank` for GeckoView applications (Marionette and WebDriver BiDi)

Actually when opening tabs we have to also add support for closing tabs at the same time. Otherwise we would leave all the newly opened tabs open and won't be able to actually enable the wdspec tests.

Summary: Add support for opening a new tab with `about:blank` for GeckoView applications (Marionette and WebDriver BiDi) → Add support for opening and closing of tabs for GeckoView applications (Marionette and WebDriver BiDi)

Note that this bug is currently blocked on the GeckoView implementation (bug 1771012).

Whiteboard: [webdriver:m4] → [webdriver:m4:blocked]
Priority: P1 → P2
Whiteboard: [webdriver:m4:blocked] → [webdriver:backlog]

I started with enabling the WebDriver classic tests and most of them are passing which is great. But I can also see a couple of new failures coming up, or related to bugs that got closed incomplete a while ago.

Tomorrow I'm going to enable the feature as well for WebDriver BiDi + some refactoring so that Marionette will use the same TabManager.addTab() API.

The work is not that complex as initially thought but it's still quite a lot of meta data work necessary. As such I'm decreasing the points to 2.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Points: 8 → 2
Whiteboard: [webdriver:backlog] → [webdriver:m4]
Priority: P2 → P1
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/144d4be497c6
[remote] Add support for opening and closing tabs on Android. r=webdriver-reviewers,jdescottes

Backed out 2 changesets (Bug 1789357, Bug 1506782) for causing xpcshell failures on test_Capabilities.js.
Backout link
Push with failures <--> X1
Failure Log
Also Wd1 log

Flags: needinfo?(hskupin)

I missed to correctly rebase against the /webdriver/tests/bidi/browsing_context/load/load.py | test_new_context[window] test, which should also never run on Android.

Going to push again once it's fixed and verified locally.

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fc04dba3df30
[remote] Add support for opening and closing tabs on Android. r=webdriver-reviewers,jdescottes

https://hg.mozilla.org/mozilla-central/rev/fc04dba3df30

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox106: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 106 Branch

Adding webdriver:relnote

Added support for opening and closing tabs on GeckoView applications (eg. Firefox for Android).

Whiteboard: [webdriver:m4] → [webdriver:m4][webdriver:relnote]
You need to log in before you can comment on or make changes to this bug.