Allow same priority when registering anchor mutators. (#17634)

* use priority queue and allow same priority to be used

* forEach loop

* test

* fix annotation

* sandbox > sinon

* fix parseUrl call

* fix test

src/service/navigation.js

@@ -33,6 +33,7 @@ import {
   registerServiceBuilderForDoc,
 } from '../service';
 import {toWin} from '../types';
+import PriorityQueue from '../utils/priority-queue';
 
 const TAG = 'navigation';
 /** @private @const {string} */
@@ -131,8 +132,12 @@ export class Navigation {
      */
     this.a2aFeatures_ = null;
 
-    /** @private @const {!Array<function(!Element)>} */
-    this.anchorMutators_ = [];
+    /**
+     * @type {!PriorityQueue<function(!Element)>}
+     * @private
+     * @const
+     */
+    this.anchorMutators_ = new PriorityQueue();
   }
 
   /**
@@ -284,10 +289,10 @@ export class Navigation {
     }
 
     // Handle anchor transformations.
-    this.anchorMutators_.forEach(callback => {
-      callback(target);
-      location = this.parseUrl_(target.href);
+    this.anchorMutators_.forEach(anchorMutator => {
+      anchorMutator(target);
     });
+    location = this.parseUrl_(target.href);
 
     // Finally, handle normal click-navigation behavior.
     this.handleNavClick_(e, target, location);
@@ -450,16 +455,7 @@ export class Navigation {
    * @param {number} priority
    */
   registerAnchorMutator(callback, priority) {
-    dev().assert(priority <= 10 && priority > 0,
-        'Priority must a number from 1-10.');
-    user().assert(!this.anchorMutators_[priority],
-        'Mutator with same priority is already in use.');
-    // Note that we define a set priority, as making this boundless
-    // will create a large sparse array, which is not performant if iterating
-    // through when executing. If the number of registered anchor mutators
-    // exceeds 10 then we will need to either increase or modify the
-    // implementation. Please talk to @alabiaga @choumx @jridgewell.
-    this.anchorMutators_[priority] = callback;
+    this.anchorMutators_.enqueue(callback, priority);
   }
 
   /**

src/utils/priority-queue.js

@@ -82,6 +82,16 @@ export default class PriorityQueue {
     return i;
   }
 
+  /**
+   * @param {function(T)} callback
+   */
+  forEach(callback) {
+    let index = this.queue_.length;
+    while (index--) {
+      callback(this.queue_[index].item);
+    }
+  }
+
   /**
    * Dequeues the max priority item.
    * Items with the same priority are dequeued in FIFO order.

test/functional/test-navigation.js

@@ -145,39 +145,18 @@ describes.sandboxed('Navigation', {}, () => {
     });
 
     describe('anchor mutators', () => {
-      const priortyUsedError = /Mutator with same priority is already in use./;
-      const priorityError = /Priority must a number from 1-10./;
-      const priority = 10;
       it('should throw error if priority is already in use', () => {
+        const priority = 10;
         handler.registerAnchorMutator(element => {
           element.href += '?am=1';
         }, priority);
         allowConsoleError(() => {
           expect(() => handler.registerAnchorMutator(element => {
             element.href += '?am=2';
-          }, priority)).to.throw(priortyUsedError);
+          }, priority)).to.not.throw();
         });
       });
 
-      it('should respect confines of the priority rules', () => {
-        allowConsoleError(() => {
-          expect(() => handler.registerAnchorMutator(element => {
-            element.href += '?priority=-1';
-          }, -1)).to.throw(priorityError);
-        });
-        allowConsoleError(() => {
-          expect(() => handler.registerAnchorMutator(element => {
-            element.href += '?priority=11';
-          }, 11)).to.throw(priorityError);
-        });
-        expect(() => handler.registerAnchorMutator(element => {
-          element.href += '?priority=1';
-        }, 1)).to.not.throw();
-        expect(() => handler.registerAnchorMutator(element => {
-          element.href += '?priority=10';
-        }, 10)).to.not.throw();
-      });
-
       it('should execute in order', () => {
         anchor.href = 'https://www.testing-1-2-3.org';
         let transformedHref;
@@ -186,16 +165,22 @@ describes.sandboxed('Navigation', {}, () => {
           transformedHref = element.href;
         }, 2);
         handler.registerAnchorMutator(element => {
-          element.href += '?first=1';
+          element.href += '&first=1';
           transformedHref = element.href;
         }, 1);
         handler.registerAnchorMutator(element => {
-          element.href += '&third=3';
+          element.href += '?third=3';
+          transformedHref = element.href;
+        }, 3);
+        // If using a same priority, the order of registration is respected.
+        handler.registerAnchorMutator(element => {
+          element.href += '&third=3-1';
           transformedHref = element.href;
         }, 3);
         handler.handle_(event);
         expect(transformedHref).to.equal(
-            'https://www.testing-1-2-3.org/?first=1&second=2&third=3');
+            'https://www.testing-1-2-3.org/?third=3&third=3-1&second=2'
+            + '&first=1');
       });
 
       it('verify order of operations', () => {
@@ -213,6 +198,11 @@ describes.sandboxed('Navigation', {}, () => {
         // of the possibly mutated anchor href into the location object
         // for navigation.handleNavClick.
         sinon.assert.callOrder(expandVars, linkRuleSpy, parseUrl);
+        expect(expandVars).to.be.calledOnce;
+        // Verify that parseUrl is called once when the variables are
+        // expanded, then after the anchor mutators and then once more
+        // in handleNavClick
+        expect(parseUrl).to.be.calledThrice;
       });
     });
 

test/functional/utils/test-priority-queue.js

@@ -18,8 +18,10 @@ import PriorityQueue from '../../../src/utils/priority-queue';
 
 describe('PriorityQueue', function() {
   let pq;
+  let sandbox;
 
   beforeEach(() => {
+    sandbox = sinon.sandbox;
     pq = new PriorityQueue();
   });
 
@@ -87,4 +89,17 @@ describe('PriorityQueue', function() {
   it('should throw error when priority is NaN', () => {
     expect(() => { pq.enqueue(NaN); }).to.throw(Error);
   });
+
+  it('should iterate through queue', () => {
+    const spy = sandbox.spy();
+    pq.enqueue('p', 1);
+    pq.enqueue('m', 2);
+    pq.enqueue('a', 3);
+    pq.forEach(letter => {
+      spy(letter);
+    });
+    expect(spy.firstCall).to.be.calledWith('a');
+    expect(spy.secondCall).to.be.calledWith('m');
+    expect(spy.thirdCall).to.be.calledWith('p');
+  });
 });