[go: nahoru, domu]

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鈥檒l 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鈥檛聽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.

None yet

5 participants