Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Retrieve the page favicons from the browser and display it on the tab selector #5166
base: main
Are you sure you want to change the base?
Retrieve the page favicons from the browser and display it on the tab selector #5166
Changes from 8 commits
5bcabdd
7200193
b087655
b92ad9a
f643aab
fff37cb
0c4a2c5
40e3068
fb8a8e3
21944be
e603c26
61f7417
d3b9734
269d3ff
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
(optional)
The goal of this function was to provide always the same class name for the same URL, so that we wouldn't try to insert a lot of identical CSS classes. The other goal was an idempotent operation, so that calling it with the same parameter in 2 different places would generate the same result.
But now that you have this Map, we have a way to ensure this unicity without this trick.
So I wonder if we could do a simple counter with a common prefix instead of a possibly expensive sha1 (expensive depending on the size of the input).
What do you think?
The problem with a static counter is for testing, you need to provide a function to reset it for the tests only. Also it might need to change more than just this because of how the class is used currently (but I'm not sure).
(not optional) IMO you can do the sha1 operation for both data urls and and normal urls. Also as a comment you could check for
data:
only (but you don't need this check anyway).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.
Also I believe you moved how the classname was generated here just becase of the fact you made it async. But I don't mind, I feel like it's better to generate the classnames once rather than regenerating them always. And it makes things easier with the counter.
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.
Hmm yeah, you are right about the favicon class name. We don't really need to sha1 the whole url and just can have a counter. I changed it to that one, thanks!
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.
nit: instead of passing
dispatch
as a parameter, I'd make this a thunk action, so thatdispatch
is injected by the thunk action middleware.Then in the caller, you'd do
await dispatch(retrievePageFaviconsFromBrowser(...))
I think it's better because it makes it clearer in the caller that this will ultimately dispatch an action.
Currently it feels like
await retrievePageFaviconsFromBrowser
ought to return something.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.
I wanted to do similarly to what we already do for
doSymbolicateProfile
above:profiler/src/actions/receive-profile.js
Lines 986 to 1018 in 1170042
Both of them are called right after each other. So I would prefer to keep their implementation similar to keep them uniform in the function where we call them:
profiler/src/actions/receive-profile.js
Lines 280 to 285 in 40e3068
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.
mmm I see (and I was the one who implemented
doSymbolicateProfile
initially so I can't blame anyone else :-) )The difference I see though, is that
doSymbolicateProfile
starts the symbolication proces which can take long, whileretrievePageFaviconsFromBrowser
is a more direct process. But I admit the difference is thin.I guess seeing that we pass
dispatch
is good enough to know that actions may be dispatched.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.
This makes me think that we might want to run
doSymbolicateProfile
andretrievePageFaviconsFromBrowser
in parallel. Or at least doretrievePageFaviconsFromBrowser
first, because it's likely much faster than the symbolication, and it may happen that users share their profile before the symbolication ends.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.
if that's not normal, it would be good to output a warning or an error to the console so that it's not silently swallowed.
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.
This could also happen in the case where the backend doesn't support the new webchannel request yet and we fallback to an empty array in the webchannel logic. I would prefer to not pollute the console for now. But we might want to add for later.
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.
(optional)
so that we don't add unneeded content to the profile.
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.
If you follow my suggestion from https://phabricator.services.mozilla.com/D225197, that is the WebChannel should return the binary data + the mimetype, then here is the location where you'll generate base64 data using the FIleReader.
I was also wondering it we should store the data url, or a pair (base64 + mimetype), in the profile JSON. But I think the data url makes sense as it contains both these information.
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.
Yeah it think it's easier/simpler to keep a single data url string that contains the both information.
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.
I still think we should have some nice placeholder when there's no icon, instead of emptiness. But we can look at this later.
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.
Yeah, I would like to have a placeholder as well. I can work on it as a followup.
Do you have any ideas for possible icons that we can put it here?
Maybe "Globe" for the websites: https://icons.design.firefox.com/viewer/#globe
and "Extensions" icon for the webextensions: https://icons.design.firefox.com/viewer/#extensions
What do you think?