Performance improvement in case of empty mutations

[Reinmar]

Extracted from ckeditor/ckeditor5-widget#108

---

Regarding this part

I think we could get rid of it completely

if we optimized _onMutations() that calls view.forceRender() whether there are view mutations or not.

For instance, when the image is resized, DOM (character) mutations are triggered for the label displaying the current size. We filter them out (which makes sense) but we call view.forceRender() anyway later on.

I'm not an expert in this area but I guess we could terminate earlier if mutations come from within an UIElement. Is there any use–case for calling view.forceRender() in such a situation?

I did this

diff --git a/src/view/observer/mutationobserver.js b/src/view/observer/mutationobserver.js
index 2362afff..0bb2e190 100644
--- a/src/view/observer/mutationobserver.js
+++ b/src/view/observer/mutationobserver.js
@@ -248,7 +248,9 @@ export default class MutationObserver extends Observer {

                   // If nothing changes on `mutations` event, at this point we have "dirty DOM" (changed) and de-synched
                   // view (which has not been changed). In order to "reset DOM" we render the view again.
-                  this.view.forceRender();
+                  if ( viewMutations.length ) {
+                           this.view.forceRender();
+                  }

                   function sameNodes( child1, child2 ) {
                            // First level of comparison (array of children vs array of children) – use the Lodash's default behavior.

and engine tests were all green.

cc @scofalik

[Reinmar]

IMO, we shouldn't also fire the viewDoc#mutations event when there are no view mutations, so I tihnk it should look like this:

		if ( viewMutations.length ) {
			this.document.fire( 'mutations', viewMutations, viewSelection );

			// If nothing changes on `mutations` event, at this point we have "dirty DOM" (changed) and de-synched
			// view (which has not been changed). In order to "reset DOM" we render the view again.
			this.view.forceRender();
		}

This helps a bit with @mlewand's image resizer performance issue too.

[Reinmar]

I want to make sure about one thing – that no elements are "markToSynced" when we abort from calling forceRender() . I'll add a warning about this situation. Let's see if we'll spot it.

[Reinmar]

The first thing I checked is that Renderer#mark*ToSync() is called only when something is added to viewMutations. So, at least currently, that's kept in sync.

Now, Renderer#render() bases on marked elements but it also takes care of the filler and selection regardless of what was marked. This means that even if nothing was marked it may fix some things. 

Therefore, I started wondering if skipping forceRender() is safe. Perhaps there are scenarios when it should be called anyway. We can hope to catch them manually, but the reality is that without a message on the console that we skipped forceRender this may be really tricky. We may face issues and not even connect those things.

So, are we able to log something to the console in fishy situations? Unfortunately, I can't see a condition on which we could base that message.

I made some quick checks though. I added a warning in the else block of the condition that we've just added and ran all the tests. I got errors only from MutationObserver and WidgetResize tests (as expected). Manual tests also never logged this warning except of resizing (as expected).

So, then I started removing bogus <br> from the console (in a setInterval() to avoid selection changes) to check if the editor is able to restore that. It turned out that it wasn't able to restore that neither on current master or before we did any changes. 

Then I checked the same for:

setInterval( () => { x = document.getSelection().anchorNode; if ( x && x.data && x.data.length === 7 ) { x.remove(); console.log( 'removed', x ); } }, 3000 );

To check if removal of 7ZWS (the inline filling character) can be restored. I was pressing Cmd+B and waited for the filler to be removed. It wasn't then restored correctly plus some error was thrown from the renderer. That's very similar on master and before our changes.

renderer.js:394 Uncaught TypeError: Cannot read property 'removeChild' of null
    at Renderer._removeInlineFiller (renderer.js:394)
    at Renderer.render (renderer.js:183)
    at View._render (view.js:682)
    at View.<anonymous> (view.js:187)
    at View.fire (emittermixin.js:209)
    at View.change (view.js:479)
    at View.forceRender (view.js:502)

Anyway, as we can see, we're not handling those situations anyway. Apparently, they didn't occur in real life often enough for us to focus on them. 

Let's see then. If we'll hit some regressions, we'll know where to look for for a moment despite lack of a console warning.

[Reinmar]

I reported #5991 because @scofalik made me realize that the above error makes a lot of sense and it should actually be corrected. However, I checked that it'd still not work correctly.