Replace empty bookmark title with best guess

[jun1x]

Some users remove the title and only keep the favicon when adding
links to the bookmark toolbar, to improve appearance and save space.

The bookmark library and sidebar will come up with a best guess
for a bookmark title, if it's left empty. The title is constructed
using the URI.

Added this default Firefox behavior to Bookmark Search Plus 2.

(See comment on commit for visual examples
jun1x@1b9a416#comments)

[aaFn]

Hello @jun1x , thank you for the contribution. I will look at integrating.

However, the code is currently under major overhaul for 2.0.27 in order to realize the next optimization step of #5 which requires to split and move big chunks of code to the background task,
So I won't be able to take that pull request, and I will have to reproduce it by hand when finished (which is taking some time). I hope you don't mind.

Also, two considerations to ponder:

• when using urlObj = new URL (something), we need to release it with a URL.revokeObjectURL(urlObj);
  Indeed, the URL objects are hooked to the main document and never get released, therefore creating a memory leak if we get a high number of them.
• we need to be careful on the display code = for users who have big numbers of URL's (10000, etc ..), this is one of the major sources of delay to display (see On elaboration of Bookmark-search-plus-2 #5 again). So it needs to be very simple and fast.
  In the case you are addressing, how many bookmarks with no title do you expect to see ? Is that a large number or a small one ?
  If small, I guess the form you propose in getBestTitle() should be ok, but if big, we'll need another and faster way to process the URL text to display a title.

Let me know your thoughts.
Thanks, aaFn.

[jun1x]

Hello @aaFn ,

First off, thank you for converting BMSP2 to a Web-extension.

I understand this change will have to wait for now, it's more of a cosmetic improvement anyway.
Looking at the space available on the bookmark toolbar I expect less then 50 bookmarks.

As for the memory concern. I did notice "Removed a memory leak due to URL object" in the changelog.txt
I do however think that the way I created the URL object shouldn't cause a leak.

There are 2 ways according to the MDN documentation, as I understand it.

1. Constructing a new object:

	let urlObj = new URL("https://www.github.com");	
	urlObj = null; 	

urlObj is a local URL object with properties, href / host / pathname / etc.
Its lifetime is tied to the scope in which it was created e.g. the 'fetchFavicon' function
When it goes out of scope the gc should handle it. Setting it explicitly to null might not even be necessary.

2. Calling a static method:

	let urlStr = URL.createObjectURL(file/blob/mediasource); 
	URL.revokeObjectURL(urlStr); 

urlStr is just a DOMString, no properties.
The actual URL object is handled internally by the static function and its lifetime is tied to the document, like you mentioned.
In order for it to get released the document (sidebar) needs to be unloaded or revokeObjectUrl needs to be called explicitly. If not done properly it will result in memory leaks.

Currently:
The 'fetchFavicon' function is mixing these two ways.
First creating a URL object with 'new' and then using the static URL function to revoke.

I could be wrong, but I don't think it can work because the return value of 'new URL(...)', a URL object,
does not match the input parameter of URL.revokeObjectURL(...), a DOMString.

Let me know what you think.

Thanks, Jun1x

[aaFn]

Hi @jun1x , indeed re-reading the URL MDN page I believe you are correct ... so my URL.revokeObjectURL(...) don't have any effect I believe ;-)

I did that when I was chasing big memory consumptions and thought this could be a source .. I found later that apparently this is browser.storage.local.set() which was making big memory peaks (it was consuming my whole 16 GB for long experiments ...) when called too often and too close ..
By spacing them I am now able to reduce much the hungry memory monster .. I guess the spacing gives enough time to the GC to act ... and I didn't find any way to trigger the GC on demand nor to reduce the browser.storage.local.set() consumption ...

So I will probably clean fetchFavicon .. no need to keep anything unuseful.

Back to your subject, ok if we are on things like 50 bookmarks, we are good by far .. :-)
Then that's only an additional test on the path, that will be without impact on the duration I think .. the alternate path will be exercised max 50 times so won't add anything for those having normal bookmarks.

Ok, so I'll look at integrating when I aml finished with the current optimization step for #5.
I will keep you posted here.

Thank you much, aaFn.

[aaFn]

Hello @jun1x , this is out tonight with 2.0.27.

I had to adapt it to the new form of the code which in this case is differing in bkmkChanged() now.
Rest is where you had placed it, but I combined things and streamlined a little for the sake of efficiency since this is on the main path to display, and 2.0.27 is about again opitmizing bookmark tree availabliity.

In terms of code management, I will see if I can accept that pull request and override it totally with 2.0.27 code just behind in GitHub, for the sake of acknowledging that suggestion in the records.

Thank you, aaFn.

[jun1x]

Thank you, I'm looking forward to using version 2.0.28

[aaFn]

It is published now, let me know if the function does not work as you expected.