Retrieve the page favicons from the browser and display it on the tab selector #5166

canova · 2024-10-16T11:37:03Z

This requires changes from Bug 1921778 since it adds the webchannel changes in the backend.

This PR adds the ability to retrieve the favicons for pages as a data url, so we can display them easily in the frontend without needing to fetch them from their url every time we open a profile.

This also changes how we use the Icon component and how we compute the css class names for them. Let me know what you think!

Note that this doesn't change the other icons that we have in places like call tree. I would prefer to do it as a followup as there are some questions to answer there.

Deploy preview with the page data retrieved already

In the following commit, we are going to be using the sha1 code that we have that relies on window.crypto. We have to add it to jest as well to make sure that we don't break the tests.

julienw

Thanks for the patch!
I haven't tried it, but I trust you that you tested it.

I think this should be improved now, because it impacts the web channel protocol:

make the web channel return binary data instead of the data url. I left these comments in the phabricator patch as well. See below for more specific comments about that.

(+ some nits)

Possibly later, but my preference is that it should be done now:

I think the icons from pages don't need to go through all the machinery in actions/icons. This machinery was made so that we wouldn't try to display the favicons that are inexistant, which was unavoidable because we were trying to guess a URL. Without this machinery, some browsers would display the "broken image" icon instead of a blank image.

So I think you could have 2 different paths for these 2 types of icons: the ones where we have a data-url, and the ones where we have a normal URL. We know that the data-url works, so we don't need to check for it.

By having these 2 different paths, you could also batch adding all the icons from pages to the state in just one operation, avoiding the cost of regenerating the map for each icon (like mentioned below).

This way you could also avoid the sha1 operation and instead use a counter like mentioned below.

Tell me what you think!

Please add new commits on top of these ones and do not change the current ones (except for rebase if necessary of cousre).

julienw · 2024-10-18T09:37:04Z

src/actions/receive-profile.js

@@ -1017,6 +1022,36 @@ export async function doSymbolicateProfile(
  dispatch(doneSymbolicating());
 }

+export async function retrievePageFaviconsFromBrowser(
+  dispatch: Dispatch,


nit: instead of passing dispatch as a parameter, I'd make this a thunk action, so that dispatch 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.

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

export async function doSymbolicateProfile(

dispatch: Dispatch,

profile: Profile,

symbolStore: SymbolStore

) {

dispatch(startSymbolicating());

const completionPromises = [];

await symbolicateProfile(

profile,

symbolStore,

(

threadIndex: ThreadIndex,

symbolicationStepInfo: SymbolicationStepInfo

) => {

completionPromises.push(

new Promise((resolve) => {

_symbolicationStepQueueSingleton.enqueueSingleSymbolicationStep(

dispatch,

threadIndex,

symbolicationStepInfo,

resolve

);

})

);

}

);

await Promise.all(completionPromises);

dispatch(doneSymbolicating());

}

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

await doSymbolicateProfile(dispatch, profile, symbolStore);

}

}

if (browserConnection && pages && pages.length > 0) {

await retrievePageFaviconsFromBrowser(dispatch, pages, browserConnection);

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, while retrievePageFaviconsFromBrowser 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.

This makes me think that we might want to run doSymbolicateProfile and retrievePageFaviconsFromBrowser in parallel. Or at least do retrievePageFaviconsFromBrowser first, because it's likely much faster than the symbolication, and it may happen that users share their profile before the symbolication ends.

src/actions/receive-profile.js

julienw · 2024-10-18T09:39:00Z

src/actions/receive-profile.js

+        // It appears that an error occurred since the pages and favicons arrays
+        // have different lengths. Return early without doing anything.


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.

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.

src/reducers/profile-view.js

src/reducers/icons.js

src/profile-logic/profile-data.js

src/test/components/TabSelectorMenu.test.js

julienw · 2024-10-18T13:00:14Z

src/actions/icons.js

+async function _classNameFromUrl(url): Promise<string> {
+  return url.startsWith('data:image/')
+    ? 'dataUrl' + (await sha1(url))
+    : url.replace(/[/:.+>< ~()#,]/g, '_');
+}


(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).

return `favicon-${++faviconCounter}`

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).

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.

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!

julienw · 2024-10-18T13:33:04Z

src/actions/receive-profile.js

+      for (let index = 0; index < favicons.length; index++) {
+        newPages[index] = {
+          ...newPages[index],
+          favicon: favicons[index],
+        };


(optional)

Suggested change

for (let index = 0; index < favicons.length; index++) {

newPages[index] = {

...newPages[index],

favicon: favicons[index],

};

for (let index = 0; index < favicons.length; index++) {

if (favicons[index]){

newPages[index] = {

...newPages[index],

favicon: favicons[index],

};

}

so that we don't add unneeded content to the profile.

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.

Yeah it think it's easier/simpler to keep a single data url string that contains the both information.

src/selectors/icons.js

canova

Thanks for the review @julienw! You can find the new commits after the [DO NOT LAND] Enable the tab selector commit.

I changed the frontend to handle the binary data and convert that into the data url once we get them. Also batched the icon/classname logic so we don't have to update the map one by one.

Let me know what you think!

canova · 2024-10-20T15:56:59Z

src/actions/receive-profile.js

@@ -1017,6 +1022,36 @@ export async function doSymbolicateProfile(
  dispatch(doneSymbolicating());
 }

+export async function retrievePageFaviconsFromBrowser(
+  dispatch: Dispatch,


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

export async function doSymbolicateProfile(

dispatch: Dispatch,

profile: Profile,

symbolStore: SymbolStore

) {

dispatch(startSymbolicating());

const completionPromises = [];

await symbolicateProfile(

profile,

symbolStore,

(

threadIndex: ThreadIndex,

symbolicationStepInfo: SymbolicationStepInfo

) => {

completionPromises.push(

new Promise((resolve) => {

_symbolicationStepQueueSingleton.enqueueSingleSymbolicationStep(

dispatch,

threadIndex,

symbolicationStepInfo,

resolve

);

})

);

}

);

await Promise.all(completionPromises);

dispatch(doneSymbolicating());

}

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

await doSymbolicateProfile(dispatch, profile, symbolStore);

}

}

if (browserConnection && pages && pages.length > 0) {

await retrievePageFaviconsFromBrowser(dispatch, pages, browserConnection);

canova · 2024-10-20T15:59:53Z

src/actions/receive-profile.js

+        // It appears that an error occurred since the pages and favicons arrays
+        // have different lengths. Return early without doing anything.


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.

canova · 2024-10-20T16:04:56Z

src/components/shared/TabSelectorMenu.js

@@ -71,6 +76,7 @@ class TabSelectorMenuImpl extends React.PureComponent<Props> {
              'aria-checked': tabFilter === tabID ? 'false' : 'true',
            }}
          >
+            {hasSomeIcons ? <Icon iconUrl={pageData.favicon} /> : null}


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?

src/profile-logic/profile-data.js

src/types/state.js

src/reducers/icons.js

src/test/components/TabSelectorMenu.test.js

src/test/store/icons.test.js

canova · 2024-10-22T08:58:49Z

src/actions/receive-profile.js

+      for (let index = 0; index < favicons.length; index++) {
+        newPages[index] = {
+          ...newPages[index],
+          favicon: favicons[index],
+        };


Yeah it think it's easier/simpler to keep a single data url string that contains the both information.

…ata urls

codecov · 2024-10-22T09:11:44Z

Codecov Report

Attention: Patch coverage is 47.05882% with 45 lines in your changes missing coverage. Please review.

Project coverage is 88.45%. Comparing base (1170042) to head (269d3ff).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/actions/icons.js	50.00%	10 Missing and 1 partial ⚠️
src/actions/receive-profile.js	42.10%	9 Missing and 2 partials ⚠️
src/utils/base64.js	0.00%	10 Missing ⚠️
src/reducers/icons.js	44.44%	5 Missing ⚠️
src/reducers/profile-view.js	0.00%	4 Missing and 1 partial ⚠️
src/app-logic/web-channel.js	0.00%	2 Missing ⚠️
src/app-logic/browser-connection.js	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5166      +/-   ##
==========================================
- Coverage   88.56%   88.45%   -0.11%     
==========================================
  Files         307      308       +1     
  Lines       27936    27997      +61     
  Branches     7560     7572      +12     
==========================================
+ Hits        24741    24766      +25     
- Misses       2978     3013      +35     
- Partials      217      218       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

canova force-pushed the page-favicons branch from 7800401 to b1e12e4 Compare October 16, 2024 12:07

canova added 8 commits October 16, 2024 14:08

Add the mechanisms to retrieve the favicons via WebChannel

5bcabdd

Retrieve favicons from the browser on the initial load

7200193

Sanitize the favicon data while sanitizing the urls

b087655

Provide window.crypto in jest

b92ad9a

In the following commit, we are going to be using the sha1 code that we have that relies on window.crypto. We have to add it to jest as well to make sure that we don't break the tests.

Make it possible to pass data urls to the Icon component

f643aab

Show the favicons in the tab selector and profile filter navigator

fff37cb

Update tests after the favicon changes

0c4a2c5

[DO NOT LAND] Enable the tab selector

40e3068

canova force-pushed the page-favicons branch from b1e12e4 to 40e3068 Compare October 16, 2024 12:08

canova requested a review from julienw October 16, 2024 12:14

julienw requested changes Oct 18, 2024

View reviewed changes

canova added 3 commits October 21, 2024 14:05

Address some small review comments

fb8a8e3

Use IconsWithClassNames everywhere instead of IconState

21944be

Convert the iconWithClassName array into an object

e603c26

canova commented Oct 22, 2024

View reviewed changes

canova requested a review from julienw October 22, 2024 09:03

canova added 3 commits October 22, 2024 11:05

Use a static icon counter for favicon classes

61f7417

Get the binary favicon data directly and batch the icon loading for d…

d3b9734

…ata urls

Remove the unneeded image mock

269d3ff

canova force-pushed the page-favicons branch from 3dcdc4d to 269d3ff Compare October 22, 2024 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve the page favicons from the browser and display it on the tab selector #5166

Retrieve the page favicons from the browser and display it on the tab selector #5166

canova commented Oct 16, 2024 •

edited

Loading

julienw left a comment

julienw Oct 18, 2024

canova Oct 20, 2024

julienw Oct 23, 2024

julienw Oct 23, 2024

julienw Oct 18, 2024

canova Oct 20, 2024

julienw Oct 18, 2024

julienw Oct 18, 2024

canova Oct 21, 2024

julienw Oct 18, 2024

julienw Oct 18, 2024

canova Oct 22, 2024

canova left a comment

canova Oct 20, 2024

canova Oct 20, 2024

canova Oct 20, 2024

canova Oct 22, 2024

codecov bot commented Oct 22, 2024 •

edited

Loading

	export async function doSymbolicateProfile(
	dispatch: Dispatch,
	profile: Profile,
	symbolStore: SymbolStore
	) {
	dispatch(startSymbolicating());

	const completionPromises = [];

	await symbolicateProfile(
	profile,
	symbolStore,
	(
	threadIndex: ThreadIndex,
	symbolicationStepInfo: SymbolicationStepInfo
	) => {
	completionPromises.push(
	new Promise((resolve) => {
	_symbolicationStepQueueSingleton.enqueueSingleSymbolicationStep(
	dispatch,
	threadIndex,
	symbolicationStepInfo,
	resolve
	);
	})
	);
	}
	);

	await Promise.all(completionPromises);

	dispatch(doneSymbolicating());
	}

	await doSymbolicateProfile(dispatch, profile, symbolStore);
	}
	}

	if (browserConnection && pages && pages.length > 0) {
	await retrievePageFaviconsFromBrowser(dispatch, pages, browserConnection);

		// It appears that an error occurred since the pages and favicons arrays
		// have different lengths. Return early without doing anything.

Retrieve the page favicons from the browser and display it on the tab selector #5166

Are you sure you want to change the base?

Retrieve the page favicons from the browser and display it on the tab selector #5166

Conversation

canova commented Oct 16, 2024 • edited Loading

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

canova commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading