Should chose the best match during 'route' if there are multiple matches.
authordewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 31 Jan 2018 23:41:37 +0000 (23:41 +0000)
committerdewei_zhu@apple.com <dewei_zhu@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 31 Jan 2018 23:41:37 +0000 (23:41 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182326

Reviewed by Ryosuke Niwa.

r227749 made a change that 'analysisCategoryPage' will be added before 'analysisTaskPage'.
As route names for both pages starts with 'analysis', whichever added first will be chosen.
For a route like 'analysis/task/1'. As a result, 'analysisCategoryPage' will be chosen and
this is not expected behavior. Adding the logic on the cases when route name does not extact
match the route name, always choose the longest mathcing route name.

Also modernized the code of 'page-router.js' to use const & let instead of var.

Added a browser test to guard against this bug.

* browser-tests/index.html: Import 'page-router-tests.js'.
* browser-tests/page-router-tests.js: Added unit test to guard against this bug.
* public/v3/pages/page-router.js:
(PageRouter.prototype.route): Added logic to find best matching in the case of inexact match.
(PageRouter.prototype.pageDidOpen):
(PageRouter.prototype._updateURLState):
(PageRouter.prototype._serializeToHash):
(PageRouter.prototype._deserializeFromHash):
(PageRouter.prototype._serializeHashQueryValue):
(PageRouter.prototype._deserializeHashQueryValue):
(PageRouter.prototype._countOccurrences):
(PageRouter):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227938 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/browser-tests/index.html
Websites/perf.webkit.org/browser-tests/page-router-tests.js [new file with mode: 0644]
Websites/perf.webkit.org/public/v3/pages/page-router.js

index b86fcc6..3c88b8c 100644 (file)
@@ -1,3 +1,33 @@
+2018-01-31  Dewei Zhu  <dewei_zhu@apple.com>
+
+        Should chose the best match during 'route' if there are multiple matches.
+        https://bugs.webkit.org/show_bug.cgi?id=182326
+
+        Reviewed by Ryosuke Niwa.
+
+        r227749 made a change that 'analysisCategoryPage' will be added before 'analysisTaskPage'.
+        As route names for both pages starts with 'analysis', whichever added first will be chosen.
+        For a route like 'analysis/task/1'. As a result, 'analysisCategoryPage' will be chosen and
+        this is not expected behavior. Adding the logic on the cases when route name does not extact
+        match the route name, always choose the longest mathcing route name.
+
+        Also modernized the code of 'page-router.js' to use const & let instead of var.
+
+        Added a browser test to guard against this bug.
+
+        * browser-tests/index.html: Import 'page-router-tests.js'.
+        * browser-tests/page-router-tests.js: Added unit test to guard against this bug.
+        * public/v3/pages/page-router.js:
+        (PageRouter.prototype.route): Added logic to find best matching in the case of inexact match.
+        (PageRouter.prototype.pageDidOpen):
+        (PageRouter.prototype._updateURLState):
+        (PageRouter.prototype._serializeToHash):
+        (PageRouter.prototype._deserializeFromHash):
+        (PageRouter.prototype._serializeHashQueryValue):
+        (PageRouter.prototype._deserializeHashQueryValue):
+        (PageRouter.prototype._countOccurrences):
+        (PageRouter):
+
 2018-01-29  Dewei Zhu  <dewei_zhu@apple.com>
 
         Should fetch owner commits in build-requests-fetcher.
index 90a6078..6b25815 100644 (file)
@@ -17,6 +17,7 @@ mocha.setup('bdd');
 <div id="mocha"></div>
 <script src="component-base-tests.js"></script>
 <script src="page-tests.js"></script>
+<script src="page-router-tests.js"></script>
 <script src="close-button-tests.js"></script>
 <script src="editable-text-tests.js"></script>
 <script src="time-series-chart-tests.js"></script>
diff --git a/Websites/perf.webkit.org/browser-tests/page-router-tests.js b/Websites/perf.webkit.org/browser-tests/page-router-tests.js
new file mode 100644 (file)
index 0000000..9f42d45
--- /dev/null
@@ -0,0 +1,36 @@
+
+describe('PageRouter', () => {
+    describe('route', () => {
+        it('should choose the longest match', async () => {
+            const context = new BrowsingContext();
+            const [Page, PageRouter, ComponentBase] = await context.importScripts(
+                ['instrumentation.js', 'components/base.js', 'pages/page.js', 'pages/page-router.js'],
+                'Page', 'PageRouter', 'ComponentBase');
+
+            let someRenderCount = 0;
+            class SomePage extends Page {
+                constructor() { super('some page'); }
+                render() { someRenderCount++; }
+                routeName() { return 'page'; }
+            };
+
+            let anotherRenderCount = 0;
+            class AnotherPage extends Page {
+                constructor() { super('another page'); }
+                render() { anotherRenderCount++; }
+                routeName() { return 'page/another'; }
+            };
+
+            const router = new PageRouter;
+            const somePage = new SomePage;
+            const anotherPage = new AnotherPage;
+            router.addPage(somePage);
+            router.addPage(anotherPage);
+            context.global.location.hash = '#/page/another/1';
+            router.route();
+            await waitForComponentsToRender(context);
+            expect(someRenderCount).to.be(0);
+            expect(anotherRenderCount).to.be(1);
+        });
+    });
+});
\ No newline at end of file
index 9d1691c..c3f743b 100644 (file)
@@ -25,23 +25,31 @@ class PageRouter {
 
     route()
     {
-        var destinationPage = this._defaultPage;
-        var parsed = this._deserializeFromHash(location.hash);
+        let destinationPage = this._defaultPage;
+        const parsed = this._deserializeFromHash(location.hash);
         if (parsed.route) {
-            var hashUrl = parsed.route;
-            var queryIndex = hashUrl.indexOf('?');
+            let hashUrl = parsed.route;
+            let bestMatchingRouteName = null;
+            const queryIndex = hashUrl.indexOf('?');
             if (queryIndex >= 0)
                 hashUrl = hashUrl.substring(0, queryIndex);
 
-            for (var page of this._pages) {
-                var routeName = page.routeName();
-                if (routeName == hashUrl
-                    || (hashUrl.startsWith(routeName) && hashUrl.charAt(routeName.length) == '/')) {
-                    parsed.state.remainingRoute = hashUrl.substring(routeName.length + 1);
+            for (const page of this._pages) {
+                const routeName = page.routeName();
+
+                if (routeName == hashUrl) {
+                    bestMatchingRouteName = routeName;
                     destinationPage = page;
                     break;
+                } else if (hashUrl.startsWith(routeName) && hashUrl.charAt(routeName.length) == '/'
+                    && (!bestMatchingRouteName || bestMatchingRouteName.length < routeName.length)) {
+                    bestMatchingRouteName = routeName;
+                    destinationPage = page;
                 }
             }
+
+            if (bestMatchingRouteName)
+                parsed.state.remainingRoute = hashUrl.substring(bestMatchingRouteName.length + 1);
         }
 
         if (!destinationPage)
@@ -60,7 +68,7 @@ class PageRouter {
     pageDidOpen(page)
     {
         console.assert(page instanceof Page);
-        var pageDidChange = this._currentPage != page;
+        const pageDidChange = this._currentPage != page;
         this._currentPage = page;
         if (pageDidChange)
             this.scheduleUrlStateUpdate();
@@ -82,7 +90,7 @@ class PageRouter {
     {
         this._historyTimer = null;
         console.assert(this._currentPage);
-        var currentPage = this._currentPage;
+        const currentPage = this._currentPage;
         this._hash = this._serializeToHash(currentPage.routeName(), currentPage.serializeState());
         location.hash = this._hash;
     }
@@ -97,10 +105,10 @@ class PageRouter {
 
     _serializeToHash(route, state)
     {
-        var params = [];
-        for (var key in state)
+        const params = [];
+        for (const key in state)
             params.push(key + '=' + this._serializeHashQueryValue(state[key]));
-        var query = params.length ? ('?' + params.join('&')) : '';
+        const query = params.length ? ('?' + params.join('&')) : '';
         return `#/${route}${query}`;
     }
     
@@ -111,13 +119,13 @@ class PageRouter {
 
         hash = unescape(hash); // For Firefox.
 
-        var queryIndex = hash.indexOf('?');
-        var route;
-        var state = {};
+        const queryIndex = hash.indexOf('?');
+        let route;
+        const state = {};
         if (queryIndex >= 0) {
             route = hash.substring(2, queryIndex);
-            for (var part of hash.substring(queryIndex + 1).split('&')) {
-                var keyValuePair = part.split('=');
+            for (const part of hash.substring(queryIndex + 1).split('&')) {
+                const keyValuePair = part.split('=');
                 state[keyValuePair[0]] = this._deserializeHashQueryValue(keyValuePair[1]);
             }
         } else
@@ -129,8 +137,8 @@ class PageRouter {
     _serializeHashQueryValue(value)
     {
         if (value instanceof Array) {
-            var serializedItems = [];
-            for (var item of value)
+            const serializedItems = [];
+            for (const item of value)
                 serializedItems.push(this._serializeHashQueryValue(item));
             return '(' + serializedItems.join('-') + ')';
         }
@@ -143,11 +151,11 @@ class PageRouter {
     _deserializeHashQueryValue(value)
     {
         if (value.charAt(0) == '(') {
-            var nestingLevel = 0;
-            var end = 0;
-            var start = 1;
-            var result = [];
-            for (var character of value) {
+            let nestingLevel = 0;
+            let end = 0;
+            let start = 1;
+            const result = [];
+            for (const character of value) {
                 if (character == '(')
                     nestingLevel++;
                 else if (character == ')') {
@@ -176,7 +184,7 @@ class PageRouter {
 
     _countOccurrences(string, regex)
     {
-        var match = string.match(regex);
+        const match = string.match(regex);
         return match ? match.length : 0;
     }
 }