devtools: import html generator code #7421

connorjclark · 2019-03-08T02:39:57Z

Bundle the HTML generation code. No longer import templates/stylesheets, as they are already in the new bundled fine.

patrickhulce

seems like we should just try to make report-generator more easily consumable by devtools rather than create this console.log'd file.

were there particular challenges there that prevented us from taking that route?

patrickhulce · 2019-03-08T15:30:43Z

lighthouse-core/scripts/roll-to-devtools.sh

+  console.log('const htmlReportAssets =', JSON.stringify(htmlReportAssets, null, 2), ';');
+  console.log('class ReportGenerator {');
+  console.log('  static Assets = htmlReportAssets;');
+  console.log('  static', reportGenerator.generateReportHtml.toString());


this feels....fragile to say the least :)

What about bundling w/ browserify + this plugin? https://github.com/browserify/brfs

The file system access is what made it tricky to consume in DT.

Not doing above, but I did a refactor. Changed the report generator module to load from Runtime.cachedAssets if the env is Devtools, and replaced the node bundling code with just copying those assets. Chromium CL not updated yet.

Bonus - fewer bytes wasted from string escaping :)

connorjclark · 2019-03-08T17:36:35Z

lighthouse-core/scripts/roll-to-devtools.sh

-cp -pPR $report_dir/{report-styles.css,templates.html,renderer} "$fe_lh_dir"
+cp -pPR $report_dir/report-generator.js "$fe_lh_dir"
+cp -pPR $report_dir/html/renderer "$fe_lh_dir"/renderer
+# import report assets


say "import report" 10 times quickly :)

brendankenny · 2019-03-08T17:53:08Z

Seems like this has a large overlap with what viewer has to do to save the HTML report. Is it possible we could share that between the two at build time rather than check at runtime?

connorjclark · 2019-03-08T18:02:40Z

Seems like this has a large overlap with what viewer has to do to save the HTML report. Is it possible we could share that between the two at build time rather than check at runtime?

good idea, I'll see what could be done.

connorjclark · 2019-03-08T18:28:17Z

Seems like this has a large overlap with what viewer has to do to save the HTML report. Is it possible we could share that between the two at build time rather than check at runtime?

good idea, I'll see what could be done.

At first glance, it seems like it'd be a large refactor and wouldn't be as simple to understand as the current code in roll-to-devtools.sh. Maybe wait for a future cleanup? At the least, tackle in a follow up PR.

brendankenny · 2019-03-08T21:23:28Z

we crafted report generation to work with devtools before, going through a few solutions until finding a nice one where we could just cut off the dependency tree at html-report-assets. It would be great to make this work within that same solution rather than having the devtools worker be build-time and the panel be run-time.

Some ideas:

the original commit was pretty much just report-generator.js browserified but dropping the csv and json output lines. Those methods are pretty small, could we just include them unused and browserify with brfs as you mentioned? build-viewer.js does this with just

const generatorFilename = `./lighthouse-core/report/report-generator.js`;
const generatorBrowserify = browserify(generatorFilename, {standalone: 'ReportGenerator'})
  .transform('brfs');

(which puts ReportGenerator on window/global/self)

if we really want to reuse assets loaded in the devtools bundle, we could write a custom html-report-assets.js just for devtools, which neatly keeps that division point. Something like

// clients/devtools-report-assets.js
'use strict';

module.exports = {
  REPORT_TEMPLATE: Runtime.cachedResources['audits2/lighthouse/report-template.html'],
  REPORT_TEMPLATES: Runtime.cachedResources['audits2/lighthouse/report-templates.html'],
  REPORT_JAVASCRIPT: Runtime.cachedResources['audits2/lighthouse/report.js'],
  REPORT_CSS: Runtime.cachedResources['audits2/lighthouse/report.css'],
};

and then have browserify insert the replacement by adding

const pathToReportAssets = require.resolve('./clients/devtools-report-assets.js');
generatorBrowserify.require(pathToURLShim, {expose: './html/html-report-assets'});

connorjclark · 2019-03-09T00:05:22Z

the original commit was pretty much just report-generator.js browserified but dropping the csv and json output lines. Those methods are pretty small, could we just include them unused and browserify with brfs as you mentioned? build-viewer.js does this with just

Did this. 2) would be nice in terms of reducing bytes (would remove tons of escaping), but would require the build script to copy the assets. but should we bother with the extra complexity?

brendankenny

method change question, but otherwise LGTM!

brendankenny · 2019-03-11T20:07:23Z

lighthouse-core/report/report-generator.js

@@ -8,6 +8,10 @@
 const htmlReportAssets = require('./html/html-report-assets');

 class ReportGenerator {
+  static get Assets() {


Suggested change

static get Assets() {

static getAssets() {

any specific reason to have it as a getter?

i thought it looked better when used: https://chromium-review.googlesource.com/c/chromium/src/+/1510732/5/third_party/blink/renderer/devtools/front_end/audits2/Audits2Panel.js

I think they're usually just lying as an API--maybe justified if you're trying to match an expected interface--but we don't really have an established style on this front so to each their own :)

Should be lowercase assets, though

brendankenny · 2019-03-11T20:12:06Z

lighthouse-core/scripts/roll-to-devtools.sh

 echo -e "\033[32m ✓\033[39m Report renderer files copied."

 # copy lighthouse-dt-bundle (potentially stale)
 cp -pPR "$lh_bg_js" "$lh_worker_dir/lighthouse-dt-bundle.js"
 echo -e "\033[96m ✓\033[39m (Potentially stale) lighthouse-dt-bundle copied."
+
+# bundle report generator
+yarn browserify lighthouse-core/report/report-generator.js -o $fe_lh_dir/report-generator.js -s ReportGenerator -t brfs


I think it could be helpful to move this into a script in build/ and output to dist/dt-report-generator-bundle.js (or whatever) and have roll-to-devtools.sh only responsible for copying things over. That way all our client building is all in one place and the requirements (e.g. browserify compatible) are plain to see.

Buuuut, this is also so succinct, if you don't want to move on that today I'm ok with that too :)

moved to build/

patrickhulce

so beautiful now! 😍

brendankenny

Nice! l like this

default.profraw accidentally committed?

build/dt-report-generator-bundle.js

brendankenny · 2019-03-15T17:30:40Z

build/dt-report-generator-bundle.js

+  .bundle((err, src) => {
+    if (err) throw err;
+    fs.writeFileSync(outFile, src.toString());
+  });


sweet, I think this might help @exterkamp as well. At least I think he was looking for a standalone report generator?

Cool! He can just remove the dt- bit then, that'd be a great PR @exterkamp :)

Changelog: GoogleChrome/lighthouse@v4.2.0...9790337 Relevant LH build changes: GoogleChrome/lighthouse#7421 GoogleChrome/lighthouse#7567 Bug: 772558 Change-Id: I50296da5059d6f90fbd69346691529a5feff15ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1510732 Commit-Queue: Connor Clark <cjamcl@google.com> Reviewed-by: Paul Irish <paulirish@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#649003} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 1df082f1ff78ab6009a972d861537484f967d3d5

Changelog: GoogleChrome/lighthouse@v4.2.0...9790337 Relevant LH build changes: GoogleChrome/lighthouse#7421 GoogleChrome/lighthouse#7567 Bug: 772558 Change-Id: I50296da5059d6f90fbd69346691529a5feff15ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1510732 Commit-Queue: Connor Clark <cjamcl@google.com> Reviewed-by: Paul Irish <paulirish@chromium.org> Reviewed-by: Dmitry Gozman <dgozman@chromium.org> Cr-Commit-Position: refs/heads/master@{#649003}

devtools: import html generator code

7eef78b

patrickhulce reviewed Mar 8, 2019

View reviewed changes

refactor

cb33112

connorjclark requested a review from paulirish as a code owner March 8, 2019 17:34

connorjclark commented Mar 8, 2019

View reviewed changes

connorjclark added 2 commits March 8, 2019 09:41

oh cp...

62f67ac

cachedResources

511278a

udpate other uses of report assets

4b1df48

connorjclark added 2 commits March 8, 2019 10:52

ensure asset files are ascii only so chrome will compile

af95b3a

lint

6e63930

connorjclark added 2 commits March 8, 2019 15:08

browserify

da90944

revert roll changes

d511f4f

lint

b766b45

brendankenny approved these changes Mar 11, 2019

View reviewed changes

patrickhulce approved these changes Mar 11, 2019

View reviewed changes

brendankenny assigned connorjclark Mar 14, 2019

getter -> method, move build code to build folder

dcf83a7

brendankenny approved these changes Mar 14, 2019

View reviewed changes

build/dt-report-generator-bundle.js Show resolved Hide resolved

connorjclark added 2 commits March 14, 2019 10:06

license, use strict

cf51a1b

go away

e44d50f

brendankenny approved these changes Mar 15, 2019

View reviewed changes

brendankenny merged commit 9efe177 into master Mar 15, 2019

brendankenny deleted the dt-save-html branch March 15, 2019 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

devtools: import html generator code #7421

devtools: import html generator code #7421

connorjclark commented Mar 8, 2019

patrickhulce left a comment

patrickhulce Mar 8, 2019

connorjclark Mar 8, 2019

connorjclark Mar 8, 2019 •

edited

Loading

connorjclark Mar 8, 2019

connorjclark Mar 8, 2019

brendankenny commented Mar 8, 2019

connorjclark commented Mar 8, 2019

connorjclark commented Mar 8, 2019

brendankenny commented Mar 8, 2019

connorjclark commented Mar 9, 2019

brendankenny left a comment

brendankenny Mar 11, 2019

connorjclark Mar 14, 2019

brendankenny Mar 14, 2019

brendankenny Mar 11, 2019

connorjclark Mar 14, 2019

patrickhulce left a comment

brendankenny left a comment

brendankenny Mar 15, 2019

connorjclark Mar 15, 2019

devtools: import html generator code #7421

devtools: import html generator code #7421

Conversation

connorjclark commented Mar 8, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Mar 8, 2019

connorjclark commented Mar 8, 2019

connorjclark commented Mar 8, 2019

brendankenny commented Mar 8, 2019

connorjclark commented Mar 9, 2019

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

patrickhulce left a comment

Choose a reason for hiding this comment

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

connorjclark Mar 8, 2019 •

edited

Loading