-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
getBrowserName performance improvement #89
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me!
I'm not entirely convinced it will treat browsers the same as before, however, so some tests would be even better :)
src/client.js
Outdated
getBrowserName: function () { | ||
if ( | ||
navigator.userAgent.includes("Opera")|| | ||
navigator.userAgent.includes("OPR") | ||
) { | ||
return "opera"; | ||
} else if (navigator.userAgent.includes("Firefox")) { | ||
return "firefox"; | ||
} else if (navigator.userAgent.includes("Brave")) { | ||
return "brave"; | ||
} else if (navigator.userAgent.includes("Chrome")) { | ||
return "chrome"; | ||
} else if (navigator.userAgent.includes("Safari")) { | ||
return "safari"; | ||
} else { | ||
return "unknown"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these in the correct order? (looks correct, but not sure)
Some example User-Agent strings (by GPT-4):
Google Chrome: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.124 Safari/537.36
Mozilla Firefox: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:89.0) Gecko/20100101 Firefox/89.0
Apple Safari: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.3 Safari/605.1.15
Microsoft Edge: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.124 Safari/537.36 Edg/91.0.864.48
Opera: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.120 Safari/537.36 OPR/64.0.3417.83
Internet Explorer: Mozilla/5.0 (Windows NT 10.0; WOW64; Trident/7.0; AS; rv:11.0) like Gecko
Brave: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/89.0.4389.82 Safari/537.36 Brave/1.21.73
I would have a lot more confidence in this big if-statement if we had some tests for the above examples.
Also:
- Add Microsoft Edge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also a list of browsers in aw-webui's queries.ts
: https://github.com/ActivityWatch/aw-webui/blob/1f194ff6886d82ac5928e8cb944888d378c54157/src/queries.ts#L216
We should check so that the entirety of that list is covered. Right now I see it's missing Edge, Orion, and Vivaldi.
Might want to add a comment referring to queries.ts
so that it can easily be cross-checked when modifying it in the future.
I updated the code to support Edge.
|
Performance improvement on the getBrowserName function.
Negative impact being that aw-watcher-web will not detect more rarely used browsers. This might be justified due to the application only being on the Firefox and Chrome addon store and the ease of adding support for a missing browser. The browser name "unknown" is used in the case of an unsupported browser.
I tested this on Firefox only.