[meta] Fix remaining issues with GeckoView in Marionette
Categories
(Remote Protocol :: Marionette, defect, P2)
Tracking
(Not tracked)
People
(Reporter: jgraham, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: meta)
TypeError: this.tabBrowser.addTab is not a function
Comment 1•5 years ago
|
||
This comes from: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=251607369&repo=try&lineNumber=1225
Here the JS stack:
[task 2019-06-13T10:10:48.665Z] 10:10:48 WARNING - openTab@chrome://marionette/content/browser.js:430:31
[task 2019-06-13T10:10:48.665Z] 10:10:48 WARNING - GeckoDriver.prototype.newWindow@chrome://marionette/content/driver.js:2761:39
It's called here:
What I wonder is why Services.appinfo.name.toLowerCase()
returns fennec
and not something else.
Beside that it means we have to stop using it because geckoview is embedded and can have multiple names. As such we will have to do method sniffing instead.
Note that there are also other areas where updates are necessary. So updating the summary accordingly.
Comment 2•5 years ago
|
||
As James said it works in general, but there are some commands which need to be fixed.
Comment 3•5 years ago
|
||
I’m confused. Is this a tracking bug for remaining GeckoView issues,
about supporting the New Window command, or for changing the product
comparison code? (We’ve had similar woes about using the app ID in
the past, for the same reasons.)
tl;dr: What is this bug about?
Comment 4•5 years ago
|
||
As it has been mentioned on comment 0 browser.js will be one spot. And there might be others.
Actually I missed in my original comment to also add the link to the try build James was running with the necessary patches to be applied, so here is the list:
https://hg.mozilla.org/try/rev/ff78f063e42cc503fd6e79525c54c09a2a45437a
https://hg.mozilla.org/try/rev/41c1c942dfcb4f37580ff0a860e38e9d1dba9c66
Comment 5•5 years ago
|
||
Note that there is Services.androidBridge.isFennec
available to check if it is Fennec or a GeckoView based browser:
Here the interface definition:
https://searchfox.org/mozilla-central/rev/8ed8474757695cdae047150a0eaf94a5f1c96dbe/widget/android/nsIAndroidBridge.idl#83
Comment 6•5 years ago
|
||
There is one behavior of GeckoView based applications which isn't clear to me yet. What should happen when window.open()
gets called? Is a new tab opened or a new window. Or does it even depend on the vehicle itself how it handles that? If it is the latter what is the best way to wait for the new window/tab to have been opened? What kind of event or observer could be used here? Or would we have to poll a specific state?
Comment 7•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #6)
There is one behavior of GeckoView based applications which isn't clear to me yet. What should happen when
window.open()
gets called? Is a new tab opened or a new window. Or does it even depend on the vehicle itself how it handles that?
I think it must depend on the vehicle itself. I'm free-handing this, but the mental model I'm using is by comparison with an Android WebView. For a browser-like vehicle, window.open()
should do what browsers do on Android, which is generally open a new tab (with window.opener
set, etc). But for, say, browsing inside the Facebook App, there's no concept of tabs, so I don't know what happens. Maybe the current window.location
is replaced (so we load in the same WebView)? Maybe the function is just ignored? That depends on the vehicle, but what is clear is that WebView has a callback interface for the consumer to say what they want; see, for example this discussion (which itself links to API docs). GeckoView must (or already does) provide some version of that callback interface API.
If it is the latter what is the best way to wait for the new window/tab to have been opened? What kind of event or observer could be used here? Or would we have to poll a specific state?
My expectation is that we can have GeckoView's callback interface API include enough richness that Marionette (and other automation-y things) can discover that the window.open
has been processed and either a new window has been opened (or a new tab?), or the request has been denied. Here I really am free-handing, 'cuz I don't know what GeckoView mechanisms there are for this. I really hope we can avoid polling!
What is it that Marionette actually needs to know about the window.open
? Is the goal for Marionette to be able to open tabs/windows to implement the Web Driver API (or equivalent)? Or does Marionette need to know when arbitrary content does such opening, so that it can instrument the opened Web Contexts in some way?
Looking at #c0, the answer must be "don't use tabBrowser
" 'cuz that's Fennec-specific. But that doesn't actually help you :(
Comment 8•5 years ago
|
||
(In reply to Nick Alexander :nalexander [he/him] from comment #7)
What is it that Marionette actually needs to know about the
window.open
? Is the goal for Marionette to be able to open tabs/windows to implement the Web Driver API (or equivalent)? Or does Marionette need to know when arbitrary content does such opening, so that it can instrument the opened Web Contexts in some way?
It's the former. And here especially to get the New Window command implemented. For Firefox there are dedicated methods to open a new browser window or a tab, so that I know what I have to wait for.
Looking at #c0, the answer must be "don't use
tabBrowser
" 'cuz that's Fennec-specific. But that doesn't actually help you :(
Very helpful fact, but it is still available and accessible. Shouldn't it be disallowed to be used? Or what's the use case for it in GeckoView then?
Comment 9•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #8)
Looking at #c0, the answer must be "don't use
tabBrowser
" 'cuz that's Fennec-specific. But that doesn't actually help you :(Very helpful fact, but it is still available and accessible. Shouldn't it be disallowed to be used? Or what's the use case for it in GeckoView then?
Just to clarify. The term tabBrowser
is a wrapper around gBrowser
for Firefox, and BrowserApp
for Fennec. So you are saying that BrowserApp
should not be used, right?
Comment 10•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #9)
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #8)
Looking at #c0, the answer must be "don't use
tabBrowser
" 'cuz that's Fennec-specific. But that doesn't actually help you :(Very helpful fact, but it is still available and accessible. Shouldn't it be disallowed to be used? Or what's the use case for it in GeckoView then?
Just to clarify. The term
tabBrowser
is a wrapper aroundgBrowser
for Firefox, andBrowserApp
for Fennec. So you are saying thatBrowserApp
should not be used, right?
Now I don't even know, 'cuz (very recently!) a BrowserApp
was added to GeckoView for certain Web Extension API support: see https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewTab.jsm#81 and the commits that added it.
Agi, can you suggest how Henrik might use the newly added support for tab (and window?) management built into GeckoView from Marionette? I'm not familiar with the new changes.
Comment 11•5 years ago
|
||
We can most likely reuse the infrastructure we just built for WebExtension support. I believe we do use BrowserApp
for it, but that was supposed to change soon? robwu should know.
As an aside, window.open
calls into onNewSession
in GeckoView and then it's up to the embedder to decide what to do. Looks like the TestRunnerActivity
creates a background session: https://searchfox.org/mozilla-central/rev/e0b0c38ee83f99d3cf868bad525ace4a395039f1/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestRunnerActivity.java#67-69
Comment 12•5 years ago
|
||
BrowserApp
and gBrowser
should not be used on GeckoView. They are very minimal transitional aids to help with getting the extension API to work. Its implementation is at: https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/mobile/android/modules/geckoview/GeckoViewTab.jsm
To support the tabs.create
extension API, we added new delegates in the app side, so that apps can customize how to handle tab open requests.
If you're able to implement (part of) Marionette's functionality with a WebExtension, then the same infrastructure can be reused.
Otherwise, you have to reuse something else (maybe methods on the nsIBrowserDOMWindow
interface?), or roll your own. If you're able to call window.open
from a content page, then that may also serve your needs.
(window.open
doesn't work in the browser process, see bug 1560842)
Comment 13•5 years ago
|
||
I don't think that we will change parts of Marionette to let it depend on the WebExtension APIs. Does that mean we shouldn't use anything from the GeckoViewTab.jsm
module in Marionette?
The code of Marionette which is basically affected is in:
https://searchfox.org/mozilla-central/source/testing/marionette/browser.js
What we mostly use it for is to get the linked browser for a tab, and to add, switch, and close different tabs.
If there is no gBrower or BrowserApp some questions come up:
-
Is there a way to retrieve all available instances of GeckoView (whether those are windows or tabs) within a vehicle? To test it which applications would you suggest? As best I would need one which has tabs, and one which only allows windows; not sure if there is one for both.
-
If we don't use gBrowser and BrowserApp for GeckoView, how could we determine if tabs are supported? If yes, what would be the best way to retrieve the currently selected tab, or to change that?
-
How do we get to the linked browser (and its browsing context) of the current tab/window from inside the parent process where the extension code is running?
-
The
nsIBrowserDOMWindow
interface looks fine, but different open methods still expect awhere
argument. So based on my question 1) we would have to know if tabs/windows are supported, and open it appropriately.
Comment 14•5 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #13)
I don't think that we will change parts of Marionette to let it depend on the WebExtension APIs. Does that mean we shouldn't use anything from the
GeckoViewTab.jsm
module in Marionette?
Indeed, don't use anything from GeckoViewTab.jsm.
What we mostly use it for is to get the linked browser for a tab, and to add, switch, and close different tabs.
GeckoView doesn't have tabs, only windows. Each window contains one browser, see geckoview.xul/geckoview.js.
- Is there a way to retrieve all available instances of GeckoView (whether those are windows or tabs) within a vehicle? To test it which applications would you suggest? As best I would need one which has tabs, and one which only allows windows; not sure if there is one for both.
Use Services.wm
- see nsIWindowMediator.idl. Use "geckoview:browser" instead of "navigator:browser".
- If we don't use gBrowser and BrowserApp for GeckoView, how could we determine if tabs are supported? If yes, what would be the best way to retrieve the currently selected tab, or to change that?
GeckoView doesn't know about tabs. An app may be extended to have tabs UI (via an android component), but afaik it is not exposed to GeckoView. Someone from the GeckoView or mobile teams may offer a more complete answer here.
I don't know if it is possible to switch windows from frontend (JS) code in GeckoView. This is still a to-do in the extension API, and dependent on bug 1565536.
- How do we get to the linked browser (and its browsing context) of the current tab/window from inside the parent process where the extension code is running?
let win = Services.wm.getMostRecentWindow("navigator:geckoview");
let browser = win.browser;
- The
nsIBrowserDOMWindow
interface looks fine, but different open methods still expect awhere
argument. So based on my question 1) we would have to know if tabs/windows are supported, and open it appropriately.
You can look up the implementation at https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/mobile/android/modules/geckoview/GeckoViewNavigation.jsm
Comment 15•5 years ago
|
||
Currently I'm not working on this bug.
Comment 16•5 years ago
|
||
Bug 1628117 makes some changes to browser.js, which might already help here. Lets see what else is remaining once that other patch got landed!
Comment 17•4 years ago
|
||
Note that reftests are currently always setting window.gBrowser
and that leads to a wrong detection of the tabBrowser
on Android. A fix for that is part of my patch series on bug 1661495.
Updated•4 years ago
|
Comment 18•3 years ago
|
||
On bug 1749444 I'm actually going to enable the tests first. Once done it will be easier to get the remaining features added given that we have a test bed.
Comment 19•2 years ago
|
||
All dependencies are finally fixed. As such I would close this meta bug, and for other upcoming issues and enhancements we will create individual bugs.
Updated•2 years ago
|
Description
•