Make bookmark-service less browser-specific, enabling more browsers (including mobile)

[michkot]

As per #126 (comment) I have refactored the code of chromium version of the bookmarm-service in order to make it compatible with more browsers (specifically tested myself on Opera and Kiwi).

I have then inspected the differences between the original chromium and firefox versions and upon the inspection moved the refactored code into the webext base class (with addition of getNativeBookmarksWithSeparators to the webext interface -- this could/should be resolved differently).

Currently, there are some notes and TODOs that should be resolved before merging into the upstream. The getNativeContainerIds() should be still reworked for chromium (and possibly for firefox too).

[michkot]

The linter does not like this..

[michkot]

The linter does not like this..

[michkot]

The linter does not like this..

[michkot]

The linter does not like this..

[nero120]

This will break on any non-english locale. You have to use the id to detect the container. Note that even this is not perfect (as ids can change under some circumstances) and I have discussed with the chromium team - there is no API or "supported" way of retrieving containers.

[michkot]

Got it! What about about letting the user specify the right folder during the setup?

[nero120]

Ids work 99% of the time. Implementing a UI just to select these folders is a pretty large change for little gain.

[michkot]

Ye... I guess implementing opera/kiwi specific version that just have different getNativeContainersIds would be the best approach then. It seems that Kiwi mobile bookmarks have id 3 (following chromiums id 1 and 2). Not sure what to do about opera, I read here something about the IDs not being static?

[nero120]

Thanks for the PR @michkot, I appreciate the effort. The chromium implementation is intended to work with standard chromium, browsers are different and have different functionality hence the separate classes. I'm not convinced that attempting to shoehorn all different chromium-based browsers together is a good idea. Frankly I'd rather keep them separate for now at least. Can you create sub classes for opera/kiwi where they differ from chromium and revert the changes to chromium classes?

Edit: If you subclass ChromiumAppModule, ChromiumBackgroundModule and ChromiumBookmarkService then you can add menuBookmarksNodeId and mobileBookmarksNodeId and any other containers those browsers have and update unsupportedContainers (but remember, other browsers have to sync any new containers so you'll have to update those implementations as well if necessary). You can then override getNativeContainerIds and any other relevant functions for that browser implementation. Remember to add a new webpack config and npm scripts for the new browser so it builds your new classes to a separate output.

[michkot]

I will see how much time there will be in the next days, I hope I'll be able to refresh this PR before I forget all the details :)

[nero120]

No worries, adding a new platform is a complex task. Not only coding but lots of testing to ensure that other browsers properly sync the data for the browser you've added! It won't make it in time for the next release, so let's not rush it.

[michkot]

...The chromium implementation is intended to work with standard chromium, browsers are different and have different functionality hence the separate classes. I'm not convinced that attempting to shoehorn all different chromium-based browsers together is a good idea....

Respecting that opinion would be crucial if the changes are to get merged, but it got me thinking. What if, instead, there would be a single small "place" (DI service, a big switch statement, whatever technical solution would be best) that would provide all the necessary browser-specific information for the same extension package to work across different chromium- or firefox- based browsers? And when I say that, the only thing that comes to my mind is the list of natively supported bookmark containers and their IDs.

Creating separate .crx files for various platforms might get quite messy, especially since many people probably get addons for Opera/Kiwi via the chrome store.

Slightly unrelated observation: I have tried the 1.6.0 on my desktop Firefox, and the Mobile bookmarks are nowhere to be found via the browsers bookmark manager.

[michkot]

... revert the changes to chromium classes...

I am more then willing to remove the hacks that will break with non-english locale etc. However, I would love to keep these changes 8a4fdaa (+ the later reuse of the same code for firefox too b29ad13), because it eliminated the dependency between the browser configuration (the set of natively supported bookmarks root nodes/containers in a particular browser) and the code gluing the native and extension bookmarks together. Maintaining the same code in two places might have worked, but forking it into 3 or 4 copies seems highly error pronse

[michkot]

And I would need a small help

app/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts

Lines 464 to 473 in 317e697

	.isSyncEnabled()
	.then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined))
	.then((bookmarks) => {
	// Initialise container ids object using containers defined in bookmarks
	const containerIds = new Map<BookmarkContainer, string>();
	if (!angular.isUndefined(bookmarks)) {
	bookmarks.forEach((x) => {
	containerIds.set(x.title as BookmarkContainer, undefined);
	});
	}

What is the purpose of this code?
With the code there, the return value of the getNativeContainerIds() can vary. Without the cache, it will always return a map of entries that always have a defined value. With the cache, it might return entries with null values for unsupported containers. Was there any gain in that (before my changes, I mean), when there is already the unsupportedContainers field?

edit: I see that there is a place in the code, which depends on this logic:

app/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts

Lines 1033 to 1044 in 317e697

	wasContainerChanged(changedNativeBookmark: NativeBookmarks.BookmarkTreeNode): ng.IPromise<boolean> {
	return this.getNativeContainerIds().then((nativeContainerIds) => {
	// If parent is not other bookmarks, no container was changed
	const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other);
	if ((changedNativeBookmark as NativeBookmarks.BookmarkTreeNode).parentId !== otherBookmarksId) {
	return false;
	}
	// If any native container ids are undefined, container was removed
	if (Array.from(nativeContainerIds.values()).includes(undefined)) {
	return true;
	}

- it used in processChangeType{Add,Modify.Remove}OnBookmarks
and the same check that's in the method is used directly in processChangeTypeMoveOnBookmarks
at

app/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts

Lines 681 to 690 in 317e697

	processChangeTypeMoveOnBookmarks(
	bookmarks: Bookmark[],
	changeData: MoveNativeBookmarkChangeData
	): ng.IPromise<Bookmark[]> {
	// Get native container ids
	return this.getNativeContainerIds().then((nativeContainerIds) => {
	// Check if container was moved to a different folder
	if (Array.from(nativeContainerIds.values()).includes(undefined)) {
	throw new Exceptions.ContainerChangedException();
	}

[nero120]

Respecting that opinion would be crucial if the changes are to get merged, but it got me thinking. What if, instead, there would be a single small "place" (DI service, a big switch statement, whatever technical solution would be best) that would provide all the necessary browser-specific information for the same extension package to work across different chromium- or firefox- based browsers? And when I say that, the only thing that comes to my mind is the list of natively supported bookmark containers and their IDs.

That's exactly what xBrowserSync is doing. You understand angular code right? These classes are instantiated via angular's dependency injection, hence different subclass implementations (where necessary) for different browsers where their native implementations differ. Taking bookmarks as an example, all desktop implementations descend from WebExtBookmarkService there is as much common code as possible without breaking things.

Creating separate .crx files for various platforms might get quite messy, especially since many people probably get addons for Opera/Kiwi via the chrome store.

I am more then willing to remove the hacks that will break with non-english locale etc. However, I would love to keep these changes 8a4fdaa (+ the later reuse of the same code for firefox too b29ad13), because it eliminated the dependency between the browser configuration (the set of natively supported bookmarks root nodes/containers in a particular browser) and the code gluing the native and extension bookmarks together. Maintaining the same code in two places might have worked, but forking it into 3 or 4 copies seems highly error pronse

Lol I didn't do it because I like making my life difficult!! You don't understand, chromium/chrome, opera and kiwi (and firefox, brave, edge etc) are different browsers, you cannot lump them together with one implementation. xBrowserSync handles syncing across multiple browsers where they all have different bookmark containers (i.e. the root folders). If a user creates a sync with Firefox and adds a bookmark in "Menu Bookmarks" then what happens to that bookmark when they subsequently sync to Chrome? xBrowserSync handles this by creating folders for unsupported containers, and it can only do that because I've separated the implementations with subclasses.

For example, look at Opera's native containers. Run this in xBrowserSync background console:

chrome.bookmarks.getTree((tree) => { console.log(JSON.stringify(tree[0].children.map((container) => ({ id: container.id, title: container.title })))); });

the result is:

[
  {
    "id": "1",
    "title": "Bookmarks bar"
  },
  {
    "id": "2",
    "title": "Imported bookmarks"
  },
  {
    "id": "6",
    "title": "Speed Dials"
  },
  {
    "id": "7",
    "title": "Trash"
  },
  {
    "id": "4",
    "title": "Unsorted bookmarks"
  },
  {
    "id": "5",
    "title": "Other bookmarks"
  }
]

Compare that to Chrome:

[
  {
    "id": "1",
    "title": "Bookmarks bar"
  },
  {
    "id": "2",
    "title": "Other bookmarks"
  }
]

How are you going to handle syncing all those extra unsupported containers? The current architecture solves this problem after much real world trial and error. This is a very complex project trying to solve a very complex problem, you can't take shortcuts with it. And that's just bookmarks! When you start looking at other custom/different aspects of browser runtime/api implementations then things can get even more crazy.

Slightly unrelated observation: I have tried the 1.6.0 on my desktop Firefox, and the Mobile bookmarks are nowhere to be found via the browsers bookmark manager.

Mobile bookmarks only appear when a user enables native sync with a mobile device. I am actually thinking about deprecating syncing of mobile bookmarks since xBrowserSync does not support native bookmark sync and most users won't have them.

[nero120]

What is the purpose of this code?

getNativeContainerIds creates a map of BookmarkContainer to the string ID of the native bookmark node that represents that container. Remember, not all containers that xBrowserSync syncs are supported so, in chromium for example, it would get Other and Toolbar by look at the root children of the native tree, but since Menu and Mobile are unsupported if they have been synced they will be folders in Other so it needs to check that they exist and if so, add the node IDs to the map.

[michkot]

Before I go on with the technical chitchat and reply to the previous messages (I hope you do not mind that the chat takes places here in the PR, I am happy to discuss whatever needs to be discussed in order to improve the extension in maintainable way), I have a question:

How did you handle addition of new containers into the extension in the past? Or has the current set (other, toolbar, menu, mobile) never been different before?. (Its relevant to my question about getNativeContainerIds -> it seems that the current v1.6 code might throw ContainerChangedException if it would load syncdata with unexpected (new) container in it)

[michkot]

So, what are the latest changes about:

• the ID detection for chromium browsers is restored (added ID 3 for mobile bookmarks as an ALFA feature)
• Opera-specific ID detection using their specific API
• refactoring / sharing more code, with the same goal as before: remove repetitive references to BookmarkContainer constant and instead use run-time logic to do the same thing = get the lists of all / natively supported /natively unsupported containers and iterate over those.
• fixing strictNullCheck errors in parts of code

The change details are summarized in the commit messages (sorry for the TYPOs, I am reading it now again and its horrible sometimes)

[michkot]

Respecting that opinion would be crucial if the changes are to get merged, but it got me thinking. What if, instead, there would be a single small "place" (DI service, a big switch statement, whatever technical solution would be best) that would provide all the necessary browser-specific information for the same extension package to work across different chromium- or firefox- based browsers? And when I say that, the only thing that comes to my mind is the list of natively supported bookmark containers and their IDs.

That's exactly what xBrowserSync is doing. You understand angular code right? These classes are instantiated via angular's dependency injection, hence different subclass implementations (where necessary) for different browsers where their native implementations differ. Taking bookmarks as an example, all desktop implementations descend from WebExtBookmarkService there is as much common code as possible without breaking things.

I am not really familiar with old Angular (Angular.JS nowadays) but since I was not adding stuff, general DI understanding was enough for me :).
In fact, I wonder if there is a practical reason to use angular's $q.stuff over ES6 Promise or even async methods (I guess its primarily about what has been chosen for this project at some point and being consistent?)

Creating separate .crx files for various platforms might get quite messy, especially since many people probably get addons for Opera/Kiwi via the chrome store.
I am more then willing to remove the hacks that will break with non-english locale etc. However, I would love to keep these changes 8a4fdaa (+ the later reuse of the same code for firefox too b29ad13), because it eliminated the dependency between the browser configuration (the set of natively supported bookmarks root nodes/containers in a particular browser) and the code gluing the native and extension bookmarks together. Maintaining the same code in two places might have worked, but forking it into 3 or 4 copies seems highly error pronse

Lol I didn't do it because I like making my life difficult!! You don't understand, chromium/chrome, opera and kiwi (and firefox, brave, edge etc) are different browsers, you cannot lump them together with one implementation. xBrowserSync handles syncing across multiple browsers where they all have different bookmark containers (i.e. the root folders). If a user creates a sync with Firefox and adds a bookmark in "Menu Bookmarks" then what happens to that bookmark when they subsequently sync to Chrome? xBrowserSync handles this by creating folders for unsupported containers, and it can only do that because I've separated the implementations with subclasses.

If I did offend you by that message, I am sorry 😓. I can imagine that browser development can be really hard due their specifics quirks. I understood the concept of the supported/unsupported containers after a few hours of toying with the code. What I am trying to achieve, in order to make adding more browsers less error-prone, is code deduplication in the bookmark-service code. Obviously, if some new obscure browser is to be added, some code might need to get un-shared again.

I will not try to paraphrase the before/after change of the bookmark-service code, in a slightly humorous way (had a bad day, so will try to relax a bit while writing this, feel free to skip it :D ):

chromium code before:

We known that we support native toolbar (id:1) and other(id:2) bookmarks containers, and menu and mobile using "virtual containers" in the "other bookmarks" container. First, let's write the createNativeBookmarks-ish method - write the code fragment for one of the containers, copy paste it three times, then add special "virtual containers" handling stuff for the "other bookmarks" copy and special "is toolbar sync enabled" to the "bookmark bark" copy and special "I am not a real native container" handling to the "menu" and "mobile" containers. Repeat the whole thing 2 or 3 times for other methods.

firefox code before:

We support all four containers!. It's like chrome, but simpler, because there are no virtual containers and no special convertSeparatorsCall. (( actually, this is one of the TODOs in my code I am hoping to get your help with later).

chrome and firefox code after:

Oh, we are free of having to solve that container-related stuff, and we don't even care how many different containers you want us to support! We can focus on our browser-specific quirks, that we have hooks for in the generic webext implementation!

webext code after:

Uhgghhh... I got kinda heavy in the last week... Did I eat too much pizza-code from the restaurant next doors 🤨? But now, I can handle all the native<->extension containers converting stuff for both of my kids! Also, I learned to dynamically find out what containers does the current browser support via asking my children, so I don't have to write it down everywhere. I can ask the kids about particular BookmarkContainer whenever I need to know, whether it is natively supported or not right now, and whether I should blow-up with an exception if I cant find a specific container.

--------- end of the hopefully funny part ---------

For example, look at Opera's native containers. Run this in xBrowserSync background console:

Yes, I was well aware of that, scouted the issues on this project ;)

How are you going to handle syncing all those extra unsupported containers?

1. By creating new base-class (abstract) methods in bookmark-service, which handle the minimum necessary browser-specific behaviour.
2. By replacing all direct references of BookmarkContainer constants with iteration over sets of supported/unsupported containers + a notion of default container, all resolved at extension init-time. ^: there is an exception for BookmarkContainer .Toolbar where "shouldSyncToolbar" check needs to be done.

I admit there is one behaviour I did not consider yet: what does the current implementation do, if the browser has more native containers that are extension-supported now by the BookmarkContainer enum -- specifically, what happens if I move bookmarks between extension-supported and extension-unsupported bookmark containers, will it result to deletion/creation every time, or will it crash? But I believe/hope this can be also resolved in a browser-agnostic way 😃 .

The current architecture solves this problem after much real world trial and error.

Sure... I am not expecting my changes to be "shipped immediately", but I think I can do (and already did) some decent manual testing and there could be people willing to try out an alfa version on a before-unsupported browser? But I might be too optimistic with that, I guess.

And that's just bookmarks! When you start looking at other custom/different aspects of browser runtime/api implementations then things can get even more crazy.

For know, I had to use one Opera-specific bookmark method. Are you aware of any other chromium-based browser that would require something more then base/chromium API? I am willing to look into those to consider the impact of providing support for them too 👍
// edit: I see there is an outstanding request to support Vivaldi, I will test it together with Opera since they both use Trash (I wonder if you have any idea how to deal with that?)

Slightly unrelated observation: I have tried the 1.6.0 on my desktop Firefox, and the Mobile bookmarks are nowhere to be found via the browsers bookmark manager.

Mobile bookmarks only appear when a user enables native sync with a mobile device. I am actually thinking about deprecating syncing of mobile bookmarks since xBrowserSync does not support native bookmark sync and most users won't have them.

Thanks for the explanation! That's rather unfortunate, because while syncing and accessibility of Mobile bookmarks between my Kiwi-phone and Chrome/Opera is flawless right now, Mobile bookmarks can not be access from Firefox-UI...

[michkot]

Right now, the code compiles, but I need to do more testing and there is a lot of code-quaility TODOs that I would like to resolve before asking you to merge this. (Unless you state that you really don't like the direction this is going -- which is perfectly legit variant -- I would probably abandon this PR and keep it for personal-use only).

Tasklist (I could copy it to the PR description and update it regularly)

• is there a way to test the extension without creating new and new sync-data on remote server in some "offline mode", or I am better to just start-up my own server instance for spamming? (I am missing an option to erase the existing server-side data and push whatever I have currently in browser)
• the base code for detecting (un)supported containers at "init-time" currently resides in BookmarkService, but I am not sure it should stay there or whether to create move it do different (or new) service.

  app/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts

  Lines 640 to 702 in 018fe8b

  	/**
  	* to be overridden; used in getNativeContainerIds()
  	*
  	* id: the native id, if it is supported \
  	* throwIfNotFound: whether getNativeContainerIds should throw an exception, when the id is undefined
  	*/
  	abstract getNativeContainerInfo(
  	containerName: BookmarkContainer
  	): ng.IPromise<{ id?: string; throwIfNotFound: boolean }>;
  	/**
  	* to be overridden; used in getNativeContainerIds()
  	*/
  	abstract getDefaultNativeContainerCandidates(): BookmarkContainer[];
  	unsupportedNativeContainerCache: BookmarkContainer[];
  	supportedNativeContainerCache: BookmarkContainer[];
  	supportedNativeContainerIdsCache: Map<BookmarkContainer, string>;
  	/** wrapper for unsupportedNativeContainerCache */
  	getUnsupportedContainers(): BookmarkContainer[] {
  	return this.unsupportedNativeContainerCache;
  	}
  	/** wrapper for supportedNativeContainerCache */
  	getSupportedContainers(): BookmarkContainer[] {
  	return this.supportedNativeContainerCache;
  	}
  	/**
  	* must be called before any getSupportedContainers() / getUnsupportedContainers() calls
  	*/
  	identifySupportedContainers(): ng.IPromise<void> {
  	let promise: ng.IPromise<any>;
  	if (this.supportedNativeContainerCache === undefined) {
  	// initialize
  	this.supportedNativeContainerCache = [];
  	this.supportedNativeContainerIdsCache = new Map();
  	const promises = Object.values(BookmarkContainer).map((containerName) => {
  	return this.getNativeContainerInfo(containerName).then((info) => {
  	if (info.id) {
  	// add to supported cache
  	this.supportedNativeContainerCache.push(containerName);
  	this.supportedNativeContainerIdsCache.set(containerName, info.id);
  	} else {
  	this.logSvc.logWarning(`Missing container for: ${containerName}`);
  	if (info.throwIfNotFound) {
  	throw new Exceptions.ContainerNotFoundException();
  	}
  	}
  	});
  	});
  	promise = this.$q.all(promises).then(() => {
  	this.unsupportedNativeContainerCache = Object.values(BookmarkContainer).filter(
  	(bc) => !this.supportedNativeContainerCache.includes(bc)
  	);
  	});
  	return promise;
  	}
  	// else
  	return this.$q.resolve();
  	}

• I probably should rewrite the for(.. of ..) loops to... probably array(x).forEach / [...x].forEach - unless you are fine with relaxing the eslint rules to allow iterators (forbidden in the airbnb style guide)
• I probably should unify the added Promise.all calls to the angular version - had some type interface problems with it, so I marked it with TODO for now.
• need to heavily test moving/removing/adding bookmarks in chrome/firefox/opera/kiwi to see if I did not break something and all the operations are properly synced (it seems moving in opera throws errors right now, I need to investigate)
• resolve non-style TODOs

[nero120]

Again I appreciate the effort, and I'm not offended by suggesting alternate approaches. It's nice to have someone interested enough to even have a discussion!! So thank you very much.

However, I really think making such sweeping changes in one go is not the right approach. You have to understand, this is a very complex codebase that has been refined over years of encountering and fixing edge case bugs. You're making changes without testing them extensively as you go and you're going to find that many of those bugs return. The architecture is as it is in order to get multiple platforms (i.e. Chromium and Firefox) syncing together without issue. Unsupported containers are handled carefully to avoid many bugs that users have discovered over the years. There's also the lack of automated tests which makes this process more difficult - on a side note, if you're willing to help me add unit tests I would really appreciate it!

I don't think I am willing merge such a huge PR, however I do think we can add the extra containers to the Chromium subclasses so that Chromium-based browsers that have extra containers like Opera and Kiwi will sync correctly. We just need to ensure that Chromium-based browsers that have the standard two containers (Other Bookmarks and Bookmarks Bar) sync these unsupported containers correctly as folders under Other Bookmarks. I think we can get this in for the v1.6.0 release.

Just for info, the platform architecture of the code looks like this:

So Chromium-based browsers can share more code, but Firefox and Chromium can only share the WebExt code.

At this point, you can either carry with the work you're doing here but keep the code bases separate, or start a new PR tackling the above task of updating the Chromium subclasses to support the known extra containers used by Opera/Kiwi/Vivaldi/etc. Would love to have your help with this, but either way thanks for your time and effort!

[nero120]

is there a way to test the extension without creating new and new sync-data on remote server in some "offline mode", or I am better to just start-up my own server instance for spamming? (I am missing an option to erase the existing server-side data and push whatever I have currently in browser)

I use a local xBrowserSync service (running in docker) for testing, easy enough to provision.

[michkot]

is there a way to test the extension without creating new and new sync-data on remote server in some "offline mode", or I am better to just start-up my own server instance for spamming? (I am missing an option to erase the existing server-side data and push whatever I have currently in browser)

I use a local xBrowserSync service (running in docker) for testing, easy enough to provision.

I know this is unrelated but -- do you maybe have a docker compose file or some script that would get xbs-api + mongo and any other containers up and running? I tried, but when I start the xbs-api container, there is no output and it immediately stops. (I am running docker inside a one-off VM just to do this)

Never mind, I think I got it working, probably some copy paste errors. I ended up with a trimmed down compose file for the full setup. It might be nice to provide a docker compose without the reverse proxy, if you need to run it in a Let's encrypt unfriendly env ;)

[michkot]

Just for info, the platform architecture of the code looks like this:

Yep, already figured it out from the code. Actually I really like the structure!

I don't think I am willing merge such a huge PR ...

At this point, you can either carry with the work you're doing here but keep the code bases separate, or start a new PR tackling the above task of updating the Chromium subclasses to support the known extra containers used by Opera/Kiwi/Vivaldi/etc

To put in bullets:

• this PR can definitely be heavily rebased and possibly split into multiple related PR...
• I don't mind if "all of this would get into "main branch" in a month, two or so, as long as we are committed to do it at some point, no rush from my side (I can always build a personal version to use, as anybody else...)
• however, the first step would still need to be to refactor the hard-to-extend-for-more-browsers parts of code. I'd rather spend time testing that it behaves the same as before, then blow-up the implementation with code-copying while adding support for more browsers without this first step.
• the second step would probably adding support for more chromium browsers
• if you would be up for it, we could set-up an online meeting (heh, I year ago that might sound really awkward, but I have so many online meetings nowadays that it seems completely OK) where we could go over the main change I am proposing
• I don't mind adding unit tests - would be a nice way to check that the code is still doing the some thing even after refactor... however, we will need to mock the browser calls somehow -- do you have a technical plan for this, since I am not technically strong in JS/TS/node.js platform?