clients(devtools): export report files as resources for devtools #7567

connorjclark · 2019-03-18T22:54:15Z

Some changes requested from Chromium side.

export as resource files, consumable via Runtime.cachedResources. mostly for theming.
use a namespace for report generator: Lighthouse.ReportGenerator

~~WIP awaiting https://chromium-review.googlesource.com/c/chromium/src/+/1510732.~~

… namespace for ReportGenerator

patrickhulce · 2019-03-19T03:52:20Z

build/dt-report-generator-bundle.js

+  fs.writeFileSync(`${distDir}/${name}`, escaped);
+}
+
+// For cached resources. Contents must be ascii.


I'm not sure what the ascii comment has to do with the rimraf :)

patrickhulce · 2019-03-19T03:53:14Z

build/dt-report-generator-bundle.js

+ * @param {string} name
+ * @param {string} content
+ */
+function cachedResource(name, content) {


it looks like this might be the ascii thing you were talking about? let's give the function a bit more descriptive name then

convertToAsciiAndWriteFile, is that what it's doing?

clients/devtools-report-assets.js

build/dt-report-generator-bundle.js

patrickhulce · 2019-03-19T03:56:08Z

build/dt-report-generator-bundle.js


-const distDir = __dirname + '/../dist';
-const outFile = `${distDir}/report-generator.js`;
+const distDir = __dirname + '/../dist/dt-resources';


nit: I know it was pre-existing but let's path.join this sucker :)

meant to do this in the last one?

patrickhulce · 2019-03-19T03:57:54Z

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

-# copy report generator
-cp dist/report-generator.js $fe_lh_dir
+# copy report generator + cached resources
+cp -r dist/dt-resources/ $fe_lh_dir


kinda unrelated but the power of the trailing / always terrifies me in cp commands...

this means we want dt-resources to now be at $fe_lh_dir/dt-resources or we want $fe_lh_dir to be dt-resources?

added comment to clarify. -r + trailing / on the source copies the contents of the folder. If the destination doesn't exist or is not a folder, it fails.

ah OK, so it's equivalent to dist/dt-resources/* in our case, right?

yes, but they'd act differently in some cases like (had to look this up):

glob does not capture hidden files

there is only one file matching the glob + destination folder does not exist => single file is copied as a file. no destination directory created

if there are multiple files matching the glob + destination folder does not exist => errors

of course, the above is a consequence of the shell expanding the glob, and the difference between cp -r f1 f2 sd and cp -r f1 sd

patrickhulce

LGTM!

patrickhulce · 2019-03-19T16:38:02Z

clients/devtools-report-assets.js

+
+/**
+ * @fileoverview Instead of loading report assets form the filesystem, in Devtools we must load
+ * them via Runtime.cachedResources. We use this module to shim ./html/html-report-assets


let's use lighthouse-core/report/html/html-report-assets.js instead of ./html/html-report-assets since it's not relative to this file :)

patrickhulce · 2019-03-19T16:40:20Z

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

-# copy report generator
-cp dist/report-generator.js $fe_lh_dir
+# copy report generator + cached resources
+cp -r dist/dt-resources/ $fe_lh_dir


ah OK, so it's equivalent to dist/dt-resources/* in our case, right?

patrickhulce · 2019-03-19T16:41:14Z

build/dt-report-generator-bundle.js


-const distDir = __dirname + '/../dist';
-const outFile = `${distDir}/report-generator.js`;
+const distDir = __dirname + '/../dist/dt-resources';


meant to do this in the last one?

brendankenny

with the changes on the devtools side, can we remove ReportGenerator.getAssets()?

couple of things, but looking good

brendankenny · 2019-03-20T22:14:47Z

build/dt-report-generator-bundle.js

+convertToAsciiAndWriteFile('report.js', htmlReportAssets.REPORT_JAVASCRIPT);
+convertToAsciiAndWriteFile('report.css', htmlReportAssets.REPORT_CSS);
+convertToAsciiAndWriteFile('template.html', htmlReportAssets.REPORT_TEMPLATE);
+convertToAsciiAndWriteFile('templates.html', htmlReportAssets.REPORT_TEMPLATES);


asserting something about htmlReportAssets in here might catch at least some issues whenever yarn build-all is run. Could be something stupid simple like Object.keys is only the ones we expect. WDYT?

how's that?

how's that?

seems good?

brendankenny · 2019-03-20T22:18:46Z

build/dt-report-generator-bundle.js


-const distDir = __dirname + '/../dist';
-const outFile = `${distDir}/report-generator.js`;
+const distDir = path.join(__dirname, '..', 'dist', 'dt-resources');


can we rename these so they match the other build scripts (and the build script name goes with the output directory name).

what about
this script -> build-dt-report-resources.js
distDir -> dist/dt-report-resources/

?

connorjclark · 2019-03-20T22:27:30Z

with the changes on the devtools side, can we remove ReportGenerator.getAssets()?

No: https://chromium-review.googlesource.com/c/chromium/src/+/1510732/13/third_party/blink/renderer/devtools/front_end/audits2/Audits2Panel.js#b142

brendankenny · 2019-03-21T19:30:05Z

No: https://chromium-review.googlesource.com/c/chromium/src/+/1510732/13/third_party/blink/renderer/devtools/front_end/audits2/Audits2Panel.js#b142

but isn't Runtime.cachedResources['audits2/lighthouse/templates.html'] going to be populated after this PR, so it won't need Lighthouse.ReportGenerator.getAssets().REPORT_TEMPLATES?

connorjclark · 2019-03-21T20:28:10Z

but isn't Runtime.cachedResources['audits2/lighthouse/templates.html'] going to be populated after this PR, so it won't need Lighthouse.ReportGenerator.getAssets().REPORT_TEMPLATES?

Oh, of course. let's kill it now.

evenstensberg · 2019-03-22T03:32:59Z

build/build-dt-report-resources.js

+ */
+'use strict';
+
+const browserify = require('browserify');


Perhaps gulp would do better here?

Just of curiousity, why are you using browserify?

Just of curiosity, why are you using browserify?

we need to run the same files in node and the browser. Even if we ran a different bundler, we'd probably run the same browserify transforms, so, might as well run the real deal until such a time something better comes along :)

Gotcha. Thanks for sharing

brendankenny

report.json included by mistake, it looks like

brendankenny · 2019-03-25T18:40:41Z

build/build-dt-report-resources.js

+convertToAsciiAndWriteFile('report.js', htmlReportAssets.REPORT_JAVASCRIPT);
+convertToAsciiAndWriteFile('report.css', htmlReportAssets.REPORT_CSS);
+convertToAsciiAndWriteFile('template.html', htmlReportAssets.REPORT_TEMPLATE);
+convertToAsciiAndWriteFile('templates.html', htmlReportAssets.REPORT_TEMPLATES);


doesn't this still result in a Runtime.cachedResources['audits2/lighthouse/templates.html'] (seems like it has to, since that's where devtools-report-assets.js loads it from), so we still don't need Lighthouse.ReportGenerator.getAssets()?

yup. already removed getAssets

oops. now i did :)

brendankenny

LGTM!

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}

clients(devtools): export report files as resources for devtools. use…

bd23251

… namespace for ReportGenerator

connorjclark changed the title ~~clients(devtools): export report files as resources for devtools. use…~~ clients(devtools): export report files as resources for devtools Mar 18, 2019

connorjclark marked this pull request as ready for review March 18, 2019 23:46

connorjclark requested review from patrickhulce and paulirish as code owners March 18, 2019 23:46

use just one output location

c5d0d97

patrickhulce reviewed Mar 19, 2019

View reviewed changes

connorjclark added 2 commits March 19, 2019 09:15

comments, rename fn

10cec82

clarify build

3be96be

patrickhulce approved these changes Mar 19, 2019

View reviewed changes

path, comment

ba36460

brendankenny reviewed Mar 20, 2019

View reviewed changes

rename to match other build scripts, assert on contents

782639d

evenstensberg reviewed Mar 22, 2019

View reviewed changes

connorjclark added 2 commits March 25, 2019 08:54

Merge branch 'master' into dt-report-generator-2

b02fda8

do not copy over renderer files to dt

39af432

brendankenny reviewed Mar 25, 2019

View reviewed changes

connorjclark added 2 commits March 25, 2019 11:50

remove mistaken file

a2f10ef

remove getAssets

f62ed0d

brendankenny approved these changes Mar 25, 2019

View reviewed changes

brendankenny merged commit 9790337 into master Mar 25, 2019

brendankenny deleted the dt-report-generator-2 branch March 25, 2019 19:47

connorjclark mentioned this pull request Jul 10, 2019

clients(devtools): audits2 -> audits1 and defer reading resources #9344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clients(devtools): export report files as resources for devtools #7567

clients(devtools): export report files as resources for devtools #7567

connorjclark commented Mar 18, 2019 •

edited

Loading

patrickhulce Mar 19, 2019

patrickhulce Mar 19, 2019

patrickhulce Mar 19, 2019

patrickhulce Mar 19, 2019

connorjclark Mar 20, 2019

patrickhulce Mar 19, 2019

connorjclark Mar 19, 2019 •

edited

Loading

patrickhulce Mar 19, 2019

connorjclark Mar 19, 2019

connorjclark Mar 19, 2019

patrickhulce left a comment

patrickhulce Mar 19, 2019

patrickhulce Mar 19, 2019

patrickhulce Mar 19, 2019

brendankenny left a comment

brendankenny Mar 20, 2019

connorjclark Mar 20, 2019

brendankenny Mar 21, 2019

brendankenny Mar 20, 2019

connorjclark Mar 20, 2019

connorjclark commented Mar 20, 2019

brendankenny commented Mar 21, 2019

connorjclark commented Mar 21, 2019

evenstensberg Mar 22, 2019

paulirish Mar 22, 2019

evenstensberg Mar 22, 2019

brendankenny Mar 25, 2019

evenstensberg Mar 27, 2019

brendankenny left a comment

brendankenny Mar 25, 2019

connorjclark Mar 25, 2019 •

edited

Loading

connorjclark Mar 25, 2019

brendankenny left a comment

clients(devtools): export report files as resources for devtools #7567

clients(devtools): export report files as resources for devtools #7567

Conversation

connorjclark commented Mar 18, 2019 • edited Loading

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

connorjclark Mar 19, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Mar 20, 2019

brendankenny commented Mar 21, 2019

connorjclark commented Mar 21, 2019

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Mar 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

connorjclark commented Mar 18, 2019 •

edited

Loading

connorjclark Mar 19, 2019 •

edited

Loading

connorjclark Mar 25, 2019 •

edited

Loading