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

clients(extension): firefox #10332

Merged
merged 8 commits into from
Feb 19, 2020
Merged

clients(extension): firefox #10332

merged 8 commits into from
Feb 19, 2020

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Feb 13, 2020

Just needed to replace some strings that only make sense in Chrome.

I'd share pictures, but I am having trouble keeping the Firefox extension open while take screengrabs.

image

image

image

image

(this is funny - the popup from the Firefox extension sticks around even when I go back to Chrome :) neat hidden "picture in picture" feature, across ~~~~~browsers 🎉 )

image

@connorjclark connorjclark requested a review from a team as a code owner February 13, 2020 00:00
@connorjclark connorjclark requested review from exterkamp and removed request for a team February 13, 2020 00:00
@connorjclark
Copy link
Collaborator Author

cc @digitarald

await Promise.all([
buildEntryPoint(),
copyAssets(),
]);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is simpler to use. just always package.

@digitarald
Copy link

This is amazing Connor, thanks for working on it. Let me know if you find any blockers that I can help with.

I'd share pictures, but I am having trouble keeping the Firefox extension open while take screengrabs.

You can use Toggle Disable Popup Auto-Hide in the Browser Toolbox, but macOS screenshots usually work fine for me.

image

@connorjclark
Copy link
Collaborator Author

Thanks @digitarald !

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

Very cool changes! Love bringing Lighthouse to more places!

This extension code seems to contain a bit of magic, a little hard to parse through tbh.

@@ -140,7 +140,7 @@ npm publish
# Publish viewer.
yarn deploy-viewer

# Publish the extension (if it changed).
# Publish the extensions (if it changed).
Copy link
Member

Choose a reason for hiding this comment

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

Are there firefox specific instructions so that someone can release this? Or will that be handled in a follow up once this has landed and can be released for the first time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I haven't uploaded to the FF store before, so IDK. I'll update when I do.

clients/extension/styles/lighthouse.css Outdated Show resolved Hide resolved
clients/extension/scripts/popup.js Outdated Show resolved Hide resolved
@@ -120,7 +124,7 @@ function getSiteUrl() {

const url = new URL(tabs[0].url);
if (url.hostname === 'localhost') {
reject(new Error('Use DevTools to audit pages on localhost.'));
reject(new Error(STRINGS.localhostErrorMessage));
} else if (/^(chrome|about)/.test(url.protocol)) {
Copy link
Member

Choose a reason for hiding this comment

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

Any firefox specific urls we need to include here now? firefox://page?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just abouts i think

about:about
image

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM!

const BROWSER_BRAND = '___BROWSER_BRAND___';

const CHROME_STRINGS = {
localhostErrorMessage: 'Use DevTools to audit pages on localhost.',
Copy link
Member

Choose a reason for hiding this comment

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

In the future, if we want to support i18n pulling these out to /locales/ (and the other user facing strings) would be good. But this is fine for now since we don't need to i18n them.

@@ -120,7 +133,7 @@ function getSiteUrl() {

const url = new URL(tabs[0].url);
if (url.hostname === 'localhost') {
reject(new Error('Use DevTools to audit pages on localhost.'));
reject(new Error(STRINGS.localhostErrorMessage));
} else if (/^(chrome|about)/.test(url.protocol)) {
reject(new Error(`Cannot audit ${url.protocol}// pages.`));

Choose a reason for hiding this comment

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

In Firefox, about: protocol pages don’t currently include the //.

@ExE-Boss
Copy link

You might want to put "fixes #6032" into the PR description.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Feb 18, 2020

We're going to keep that issue open, because it's tracking real Firefox support (as in, protocol-level). We have no plans to do that, though, atm. The extension is offered as a way to quickly generate report for the (public) url you are looking at, so at least FF users will have that.

@connorjclark connorjclark merged commit 6696ce4 into master Feb 19, 2020
@connorjclark connorjclark deleted the firefox-extension branch February 19, 2020 00:29
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.

5 participants