Skip to content
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

feat: Add reference browser support to the firefox-android extension runner (#1870) #1871

Merged
merged 1 commit into from
Apr 22, 2020

Conversation

jonalmeida
Copy link
Contributor

In addition to Fenix support (#1834), we'd like to add Reference Browser as well. 🙂

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you verify that you're able to run extensions in the Reference browser after this change?

tests/unit/test-extension-runners/test.firefox-android.js Outdated Show resolved Hide resolved
@jonalmeida
Copy link
Contributor Author

Did you verify that you're able to run extensions in the Reference browser after this change?

Yes, I did. It requires a minor change in r-b to allow web-ext to launch the browser which I'll be landing soon. 🙂

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a3f06e0 on jonalmeida:rb-support into de9211a on mozilla:master.

@Rob--W
Copy link
Member

Rob--W commented Mar 27, 2020

I see that you've opened a new pull request at the Reference Browser's repository as well.

Did you know that even without any changes to web-ext and the reference browser, that you are already able to run web-ext with the Reference browser? For example:

web-ext run -t firefox-android --android-device=<device ID here> --firefox-apk=org.mozilla.reference.browser --firefox-apk-component=BrowserActivity

To avoid the need for mozilla-mobile/reference-browser#1096 , you could add

if (!apkComponent && apk === 'org.mozilla.reference.browser') {
   apkComponent = 'BrowserActivity';
}

before

web-ext/src/util/adb.js

Lines 273 to 274 in ffe5726

const component = apkComponent ?
`${apk}/.${apkComponent}` : `${apk}/.App`;

@jonalmeida
Copy link
Contributor Author

It seemed more in line with our other browser apps to follow the activity-alias for .App since we have to add it for other production browsers as well. Keeping R-B in sync with those makes testing reproducible.

@jonalmeida
Copy link
Contributor Author

Did you know that even without any changes to web-ext and the reference browser, that you are already able to run web-ext with the Reference browser?

Did this work for you? In my testing it wasn't able to launch the activity. So I opted for adding the alias as mentioned above.

@Rob--W
Copy link
Member

Rob--W commented Mar 27, 2020

I did the following:

  1. Create minimal extension that opens a new tab:

manifest.json

{
    "name": "Test ext",
    "version": "1",
    "manifest_version": 2,
    "background": {
        "scripts": ["background.js"]
    }
}

background.js

browser.tabs.create({url:"https://example.com"});
  1. Launch web-ext with R-B using the command from feat: Add reference browser support to the firefox-android extension runner (#1870) #1871 (comment)
  2. Verified that R-B launched, with example.com

(I installed R-B from the Play store just today)

@jonalmeida
Copy link
Contributor Author

Interesting, this command seems to work for release, but not debug builds. I get this error instead (without the activity-alias):

Error: Activity class {org.mozilla.reference.browser.debug/org.mozilla.reference.browser.debug.BrowserActivity} does not exist.
    at /Users/jalmeida/src/web-ext/node_modules/adbkit/lib/adb/command/host-transport/startactivity.js:57:21
    at tryCatcher (/Users/jalmeida/src/web-ext/node_modules/bluebird/js/main/util.js:26:23)
    at Promise._settlePromiseFromHandler (/Users/jalmeida/src/web-ext/node_modules/bluebird/js/main/promise.js:503:31)
    at Promise._settlePromiseAt (/Users/jalmeida/src/web-ext/node_modules/bluebird/js/main/promise.js:577:18)
    at Async._drainQueue (/Users/jalmeida/src/web-ext/node_modules/bluebird/js/main/async.js:128:12)
    at Async._drainQueues (/Users/jalmeida/src/web-ext/node_modules/bluebird/js/main/async.js:133:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/Users/jalmeida/src/web-ext/node_modules/bluebird/js/main/async.js:15:14)

That's probably because it's sending the intent with the variant (debug) in the package name:

What it's sending: org.mozilla.reference.browser.debug/org.mozilla.reference.browser.debug.BrowserActivity
What it should be sending: org.mozilla.reference.browser.debug/org.mozilla.reference.browser.BrowserActivity

I don't believe that's a bug with web-ext though as we've run into this in Fenix where the IDE would make the same error (mozilla-mobile/fenix#9462 (comment)).

@jonalmeida
Copy link
Contributor Author

Ping @Rob--W . What do you think about this? The reason it isn't a bug on Fenix variants is because we use the same activity-alias there too.

@Rob--W
Copy link
Member

Rob--W commented Apr 7, 2020

The value of component ends up being used as the value of -n in am start ... -n org.mozilla.reference.browser.debug/.BrowserActivity.

I think that the best course of action is to pass a fully-qualified name instead, without the .debug suffix.

@Rob--W
Copy link
Member

Rob--W commented Apr 16, 2020

@jonalmeida Could you update the PR to implement the above idea, and test whether it also works with other mobile products such as Fenix (and maybe even geckoview_example)?

@jonalmeida
Copy link
Contributor Author

@jonalmeida Could you update the PR to implement the above idea, and test whether it also works with other mobile products such as Fenix (and maybe even geckoview_example)?

This seems outside the scope of this issue - I can file a follow-up for it if you'd like. In R-B, we have everything we need to land this patch as is.

@Rob--W
Copy link
Member

Rob--W commented Apr 22, 2020

This seems outside the scope of this issue

That's OK, let's not block the merge on my next question.

I can file a follow-up for it if you'd like. In R-B, we have everything we need to land this patch as is.

Has the .debug issue also been solved (#1871 (comment))? If not, please file a follow-up.

@jonalmeida
Copy link
Contributor Author

Has the .debug issue also been solved (#1871 (comment))? If not, please file a follow-up.

I've filed #1891

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants