CRX: persistent background page -> event page #1487

ebidel · 2017-01-18T07:49:17Z

R: all

I'm not sure why the crx uses a persistent background page, but it doesn't need it. They are generally discouraged in favor of on-demand event pages, which consume less memory in Chrome and are better for perf.

The changes here are mostly:

Moving to an event page. This requires using chrome.runtime.getBackgroundPage which is async. I've created a promise and wrapped code that needs the background page in it.
Moving code around (aka doing less work in DOMContentLoaded). We were defining a bunch of functions and delaying all work until DOMContentLoaded.

brendankenny · 2017-01-18T22:37:58Z

I'm not sure why the crx uses a persistent background page

It might just be because of the generator we originally used to get an extension up and running.

How does this affect the extension debugging story?

brendankenny · 2017-01-18T22:53:09Z

How does this affect the extension debugging story?

looks like the answer is that the extension is persisted while being debugged. Not sure if that's good or bad for accurately debugging :)

ebidel · 2017-01-18T23:20:33Z

How does this affect the extension debugging story?

You mean dev'ing the extension? I only saw it go inactive a few times during testing but it fired right back up when I pressed the browser action button.

Apart from that, the background page should remain live as long as LH is doing its thing during a run.

brendankenny

mostly looking good. Made some style suggestions on old variable names and completing the flattening of things out of DOMContentLoaded as long as you're in there.

Testing the extension everything appears to be working. The one difference I see is that when running lighthouse, clicking outside of the popup to close it, then reopening the popup it now does the very brief opacity transition to loading the status subpage every time, while before it appeared to be visible instantly. Fixable?

brendankenny · 2017-01-18T23:38:07Z

lighthouse-extension/app/src/popup.js

+
+      siteURL = new URL(tabs[0].url);
+
+      document.querySelector('header h2').textContent = siteURL.origin;


comment here? or intermediate originEl (or whatever) variable

brendankenny · 2017-01-18T23:39:04Z

lighthouse-extension/app/src/popup.js


-    siteURL = new URL(tabs[0].url);
-    siteNameEl.textContent = siteURL.origin;
+    const generateReportEl = document.getElementById('generate-report');


generateReportButton?

brendankenny · 2017-01-18T23:45:31Z

lighthouse-extension/app/src/popup.js

+     * @param {!Object<boolean>} selectedAggregations
+     * @return {!DocumentFragment}
+     */
+    function generateOptionsList(list, selectedAggregations) {


move this outside of the DOMContentLoaded block? It's already used asynchronously, so fine if it's asyc as well

done. but needed to add a background page param to make that work.

brendankenny · 2017-01-18T23:46:24Z

lighthouse-extension/app/src/popup.js

-    const checkedAggregations = Array.from(optionsEl.querySelectorAll(':checked'))
-        .map(input => input.value);
-    background.saveSelectedAggregations(checkedAggregations);
+    if (background.isRunning()) {


move all the rest of this to a named setup function?

brendankenny · 2017-01-18T23:51:17Z

lighthouse-extension/app/src/popup.js

-  function buildReportErrorLink(err) {
-    const reportErrorEl = document.createElement('a');
-    reportErrorEl.className = 'button button--report-error';
+function startSpinner() {


Not just a spinner...showStatusSubpage/hideStatusSubpage?

brendankenny · 2017-01-18T23:51:36Z

lighthouse-extension/app/src/popup.js

-    qsBody += '**Error Message**: ' + err.message + '\n';
-    qsBody += '**Stack Trace**:\n ```' + err.stack + '```';
+function buildReportErrorLink(err) {
+  const MAX_ISSUE_ERROR_LENGTH = 60;


top level const is normal for this

brendankenny · 2017-01-18T23:52:47Z

lighthouse-extension/app/src/popup.js

-    const reportErrorEl = document.createElement('a');
-    reportErrorEl.className = 'button button--report-error';
+function startSpinner() {
+  statusEl.classList.add(subpageVisibleClass);


maybe do document.querySelector('.status') rather than keeping global around?

brendankenny · 2017-01-19T00:09:16Z

also @paulirish had a big refactor involving this file to make aggregation categories play well with devtools, so this will be fun to come home to :)

ebidel · 2017-01-19T03:24:31Z

PTAL

brendankenny · 2017-01-19T04:49:28Z

lighthouse-extension/app/src/popup.js

+  });
+
+  return frag;
+}


this doesn't use the first list argument in here.

I was thinking you could also just have it fetch the background itself (though it doesn't really matter), and since selectedAggregations is only used in here (not in the parent), you could even absorb that too:

/** * Generates a document fragment containing a list of checkboxes and labels * for the aggregation categories. * @return {!Promise<!DocumentFragment>} */ function generateOptionsList() { const frag = document.createDocumentFragment(); return getBackgroundPage.then(background => { return background.loadSelectedAggregations().then(selectedAggregations => { const defaultAggregations = background.getDefaultAggregations(); defaultAggregations.forEach(aggregation => { const isChecked = selectedAggregations[aggregation.name]; frag.appendChild(createOptionItem(aggregation.name, isChecked)); }); return frag; }

then below it would be
generateOptionsList().then(frag => optionsList.appendChild(frag));

(could still just pass in background, of course, since the caller already has a reference to it)

redid this function a bit.

brendankenny · 2017-01-19T04:50:59Z

more important than bikeshedding that :) did you see my earlier comment:

The one difference I see is that when running lighthouse, clicking outside of the popup to close it, then reopening the popup it now does the very brief opacity transition to loading the status subpage every time, while before it appeared to be visible instantly. Fixable?

ebidel · 2017-01-19T20:05:59Z

I decided to pas background and selectedAggregations references where they were needed. Felt more DRY and less wasteful then having extra promise ticks inside separate functions:

function initPage() {
...
    background.loadSelectedAggregations().then(selectedAggregations => {
      generateOptionsList(background, selectedAggregations);

      const generateReportButton = document.getElementById('generate-report');
      generateReportButton.addEventListener('click', () => {
        onGenerateReportButtonClick(background, selectedAggregations);
      });
    });
...
}

I think the changes also fix the visual jank you were seeing. Because the background page is async, it's a bit harder to control the transitions when the popup first starts and you're already running LH. I think we always had that jank, but it was unnoticeable until now.

brendankenny · 2017-01-19T21:32:23Z

ah, good call with selectedAggregations. One weird thing is I'm really not sure why loadSelectedAggregations is in the background page. Seems like it should be in the popup since that's where it's generated and used. But that can wait for another day

brendankenny · 2017-01-19T21:52:12Z

I think the changes also fix the visual jank you were seeing. Because the background page is async, it's a bit harder to control the transitions when the popup first starts and you're already running LH. I think we always had that jank, but it was unnoticeable until now.

Yeah, this traded it for a fade in at first launch, but it's pretty unnoticeable either way. I like this better, though others can argue :) I guess we could put the transition in a class we add after load, but I wonder if a flash of the wrong subpage is better than a fast fadeout of the wrong subpage.

brendankenny

📆📄

paulirish · 2017-01-23T22:52:22Z

also @paulirish had a big refactor involving this file to make aggregation categories play well with devtools, so this will be fun to come home to :)

INDEED. 😁

but overall a worthwhile change, so it's all good.

CRX: persistent background page -> event page

2d4bf2a

ebidel force-pushed the crxeventpage branch from 7a2d70b to 2d4bf2a Compare January 18, 2017 17:35

brendankenny requested changes Jan 19, 2017

View reviewed changes

ebidel added 2 commits January 18, 2017 19:22

feedack

ce507a0

comment

6e1141b

brendankenny requested changes Jan 19, 2017

View reviewed changes

visual jank

5c5785c

brendankenny approved these changes Jan 19, 2017

View reviewed changes

brendankenny merged commit 6ad45ac into master Jan 19, 2017

brendankenny deleted the crxeventpage branch January 19, 2017 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRX: persistent background page -> event page #1487

CRX: persistent background page -> event page #1487

ebidel commented Jan 18, 2017 •

edited

Loading

brendankenny commented Jan 18, 2017

brendankenny commented Jan 18, 2017

ebidel commented Jan 18, 2017

brendankenny left a comment

brendankenny Jan 18, 2017

ebidel Jan 19, 2017

brendankenny Jan 18, 2017

ebidel Jan 19, 2017

brendankenny Jan 18, 2017

ebidel Jan 19, 2017

brendankenny Jan 18, 2017

ebidel Jan 19, 2017

brendankenny Jan 18, 2017

ebidel Jan 19, 2017

brendankenny Jan 18, 2017

ebidel Jan 19, 2017

brendankenny Jan 18, 2017

ebidel Jan 19, 2017

brendankenny commented Jan 19, 2017

ebidel commented Jan 19, 2017

brendankenny Jan 19, 2017

brendankenny Jan 19, 2017

ebidel Jan 19, 2017

brendankenny commented Jan 19, 2017

ebidel commented Jan 19, 2017 •

edited

Loading

brendankenny commented Jan 19, 2017

brendankenny commented Jan 19, 2017

brendankenny left a comment

paulirish commented Jan 23, 2017


		siteURL = new URL(tabs[0].url);

		document.querySelector('header h2').textContent = siteURL.origin;

CRX: persistent background page -> event page #1487

CRX: persistent background page -> event page #1487

Conversation

ebidel commented Jan 18, 2017 • edited Loading

brendankenny commented Jan 18, 2017

brendankenny commented Jan 18, 2017

ebidel commented Jan 18, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 19, 2017

ebidel commented Jan 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 19, 2017

ebidel commented Jan 19, 2017 • edited Loading

brendankenny commented Jan 19, 2017

brendankenny commented Jan 19, 2017

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Jan 23, 2017

ebidel commented Jan 18, 2017 •

edited

Loading

ebidel commented Jan 19, 2017 •

edited

Loading