Perf dashboard's page title can be set to a previously visited page
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Jan 2018 20:31:06 +0000 (20:31 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 29 Jan 2018 20:31:06 +0000 (20:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182209

Rubber-stamped by Chris Dumez.

Before this patch, opening a page and navigating away from it could result in the page title
getting set to that of the previously visited page after the new page had been opened.

This bug was caused by Page.render keep setting document.title even though the page is no longer
the currently open page of the router. Fixed it by exiting early in Page.enqueueToRender when
this page is not the currently open page of the router.

Also added basic tests for Page.

* browser-tests/index.html:
* browser-tests/page-tests.js: Added.
* public/v3/pages/page.js:
(Page): Removed the unused second constructor argument.
(Page.prototype.enqueueToRender): Fixed the bug.
(Page.prototype.render): Use const instead of var.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227747 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-tests.js [new file with mode: 0644]
Websites/perf.webkit.org/public/v3/pages/page.js

index 5ed0339..fb21c3f 100644 (file)
@@ -1,5 +1,28 @@
 2018-01-29  Ryosuke Niwa  <rniwa@webkit.org>
 
+        Perf dashboard's page title can be set to a previously visited page
+        https://bugs.webkit.org/show_bug.cgi?id=182209
+
+        Rubber-stamped by Chris Dumez.
+
+        Before this patch, opening a page and navigating away from it could result in the page title
+        getting set to that of the previously visited page after the new page had been opened.
+
+        This bug was caused by Page.render keep setting document.title even though the page is no longer
+        the currently open page of the router. Fixed it by exiting early in Page.enqueueToRender when
+        this page is not the currently open page of the router.
+
+        Also added basic tests for Page.
+
+        * browser-tests/index.html:
+        * browser-tests/page-tests.js: Added.
+        * public/v3/pages/page.js:
+        (Page): Removed the unused second constructor argument.
+        (Page.prototype.enqueueToRender): Fixed the bug.
+        (Page.prototype.render): Use const instead of var.
+
+2018-01-29  Ryosuke Niwa  <rniwa@webkit.org>
+
         CommitLogViewer should not fetch commits in serial
         https://bugs.webkit.org/show_bug.cgi?id=182207
 
index b0b148b..90a6078 100644 (file)
@@ -16,6 +16,7 @@ mocha.setup('bdd');
 <body>
 <div id="mocha"></div>
 <script src="component-base-tests.js"></script>
+<script src="page-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-tests.js b/Websites/perf.webkit.org/browser-tests/page-tests.js
new file mode 100644 (file)
index 0000000..30916d0
--- /dev/null
@@ -0,0 +1,127 @@
+
+describe('Page', function() {
+
+    describe('open', () => {
+        it('must replace the content of document.body', async () => {
+            const context = new BrowsingContext();
+            const Page = await context.importScripts(['instrumentation.js', 'components/base.js', 'pages/page.js'], 'Page');
+
+            class SomePage extends Page {
+                constructor() { super('some page'); }
+            };
+
+            const somePage = new SomePage;
+
+            const document = context.document;
+            document.body.innerHTML = '<p>hello, world</p>';
+            expect(document.querySelector('p')).not.to.be(null);
+            expect(document.querySelector('page-component')).to.be(null);
+
+            somePage.open();
+
+            expect(document.querySelector('p')).to.be(null);
+            expect(document.querySelector('page-component')).to.be(somePage.element());
+        });
+
+        it('must update the document title', async () => {
+            const context = new BrowsingContext();
+            const Page = await context.importScripts(['instrumentation.js', 'components/base.js', 'pages/page.js'], 'Page');
+
+            class SomePage extends Page {
+                constructor() { super('some page'); }
+            };
+
+            const somePage = new SomePage;
+
+            const document = context.document;
+            expect(document.title).to.be('');
+            somePage.open();
+            expect(document.title).to.be('some page');
+        });
+
+        it('must enqueue itself to render', async () => {
+            const context = new BrowsingContext();
+            const [Page, ComponentBase] = await context.importScripts(['instrumentation.js', 'components/base.js', 'pages/page.js'], 'Page', 'ComponentBase');
+
+            let renderCount = 0;
+            class SomePage extends Page {
+                constructor() { super('some page'); }
+                render() { renderCount++; }
+            };
+
+            expect(renderCount).to.be(0);
+            const somePage = new SomePage;
+            await waitForComponentsToRender(context);
+            expect(renderCount).to.be(0);
+
+            somePage.open();
+            await waitForComponentsToRender(context);
+            expect(renderCount).to.be(1);
+        });
+
+        it('must update the current page of the router', 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');
+
+            class SomePage extends Page {
+                constructor() { super('some page'); }
+                routeName() { return 'some-page'; }
+            };
+
+            const router = new PageRouter;
+            const somePage = new SomePage;
+            router.addPage(somePage);
+            expect(router.currentPage()).to.be(null);
+            somePage.open();
+            expect(router.currentPage()).to.be(somePage);
+        });
+    });
+
+    describe('enqueueToRender', () => {
+        it('must not enqueue itself to render if the router is set and the current page is not itself', 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 'some-page'; }
+            };
+
+            let anotherRenderCount = 0;
+            class AnotherPage extends Page {
+                constructor() { super('another page'); }
+                render() { anotherRenderCount++; }
+                routeName() { return 'another-page'; }
+            };
+
+            const router = new PageRouter;
+            const somePage = new SomePage;
+            const anotherPage = new AnotherPage;
+            router.addPage(somePage);
+            router.addPage(anotherPage);
+            router.setDefaultPage(somePage);
+            router.route();
+            await waitForComponentsToRender(context);
+            expect(someRenderCount).to.be(1);
+            expect(anotherRenderCount).to.be(0);
+
+            anotherPage.open();
+            await waitForComponentsToRender(context);
+            expect(someRenderCount).to.be(1);
+            expect(anotherRenderCount).to.be(1);
+
+            somePage.enqueueToRender();
+            anotherPage.enqueueToRender();
+            await waitForComponentsToRender(context);
+            expect(someRenderCount).to.be(1);
+            expect(anotherRenderCount).to.be(2);
+        });
+    });
+
+});
index f887c03..77734ad 100644 (file)
@@ -1,6 +1,6 @@
 
 class Page extends ComponentBase {
-    constructor(name, container)
+    constructor(name)
     {
         super('page-component');
         this._name = name;
@@ -22,9 +22,16 @@ class Page extends ComponentBase {
         this.enqueueToRender();
     }
 
+    enqueueToRender()
+    {
+        if (this._router && this._router.currentPage() != this)
+            return;
+        super.enqueueToRender();
+    }
+
     render()
     {
-        var title = this.pageTitle();
+        const title = this.pageTitle();
         if (document.title != title)
             document.title = title;
         super.render();