[trace engine] Add Lighthouse's Lantern library [devtools/devtools-frontend : main]
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
*/Someone either knowledgable about software licenses, or at least with an opinion, please weigh in on if I should simply delete these License comments from the original source files.
A potential option might be to put the license in a LICENSE file in the `lantern` folder.
I think outright deleting it is not correct, because Chromium != Google, right? So while the CLA we have LH contributors sign assigns ownership to Google, does that mean a Chromium tree can treat it like it owns it? shrug.
Related details
- Adam Raine
- Jack Franklin
- Paul Irish
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
Someone either knowledgable about software licenses, or at least with an opinion, please weigh in on if I should simply delete these License comments from the original source files.
A potential option might be to put the license in a LICENSE file in the `lantern` folder.
I think outright deleting it is not correct, because Chromium != Google, right? So while the CLA we have LH contributors sign assigns ownership to Google, does that mean a Chromium tree can treat it like it owns it? shrug.
btw "treat it like it owns it" in this case mean, ignoring the requirements of the license to include the license text in derivative works. If one owns the rights to the code, that requirement doesn't apply.
Related details
- Adam Raine
- Jack Franklin
- Paul Irish
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
// TODO: why isn't "assertMatchesJSONSnapshot" working? get 404 for snapshots.jsCan anyone help me figure out why I can't get the snapshots module to work in my tests?
Related details
- Adam Raine
- Andres Olivares
- Paul Irish
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
// TODO: why isn't "assertMatchesJSONSnapshot" working? get 404 for snapshots.jsCan anyone help me figure out why I can't get the snapshots module to work in my tests?
paul helped. we learned unittests can't use this module, only node tests.
I replaced with deep strict asserts.
Related details
- Adam Raine
- Andres Olivares
- Paul Irish
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 1 comment![]( "Open in Gerrit")
Connor ClarkSomeone either knowledgable about software licenses, or at least with an opinion, please weigh in on if I should simply delete these License comments from the original source files.
A potential option might be to put the license in a LICENSE file in the `lantern` folder.
I think outright deleting it is not correct, because Chromium != Google, right? So while the CLA we have LH contributors sign assigns ownership to Google, does that mean a Chromium tree can treat it like it owns it? shrug.
btw "treat it like it owns it" in this case mean, ignoring the requirements of the license to include the license text in derivative works. If one owns the rights to the code, that requirement doesn't apply.
@yan...@chromium.org might be able to aid here or know the best person to talk to
Related details
- Adam Raine
- Andres Olivares
- Connor Clark
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 1 comment![]( "Open in Gerrit")
this is exciting! I am not going to review the code in detail but it's awesome to see this coming in 😊
Andres Olivares (Gerrit)
Andres Olivares voted and added 5 comments![]( "Open in Gerrit")
Votes added by Andres Olivares
| Code-Review | +1 |
5 comments
cool to see this happening!
LGTM % license headers and tracking pending tasks for integrating with repo ts and lint config
// TODO: off due to Lantern needing more refactoring.maybe it's worth filing a bug for this to ensure we don't forget to resolve this (or tying it to an existing bug)
"../../../core/common:bundle",
"../../../core/platform:bundle",
"../../../core/root:bundle",
"../../../core/sdk:bundle",do we expect to use these deps? If we do so it might cause issues when rolling to the standalone library, but I think we don't need them.
// @ts-nocheckditto: tracking bug (and other ts-nochecks)
// "incremental": true, /* Save .tsbuildinfo files to allow for incremental compilation of projects. */do we want to eventually enable these or could we remove them completely (or something in between)?
Related details
- Adam Raine
- Connor Clark
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Irish (Gerrit)
Paul Irish added 3 comments![]( "Open in Gerrit")
"../../../core/common:bundle",
"../../../core/platform:bundle",
"../../../core/root:bundle",
"../../../core/sdk:bundle",do we expect to use these deps? If we do so it might cause issues when rolling to the standalone library, but I think we don't need them.
If we do so it might cause issues when rolling to the standalone library,
I checked this and it doesn't. (But... I don't know why. (Nor do I know why they're here ;))
Connor ClarkSomeone either knowledgable about software licenses, or at least with an opinion, please weigh in on if I should simply delete these License comments from the original source files.
A potential option might be to put the license in a LICENSE file in the `lantern` folder.
I think outright deleting it is not correct, because Chromium != Google, right? So while the CLA we have LH contributors sign assigns ownership to Google, does that mean a Chromium tree can treat it like it owns it? shrug.
Jack Franklinbtw "treat it like it owns it" in this case mean, ignoring the requirements of the license to include the license text in derivative works. If one owns the rights to the code, that requirement doesn't apply.
@yan...@chromium.org might be able to aid here or know the best person to talk to
gemini, at least, says this is fine, but has guidance around attribution: https://g.co/gemini/share/bfdb4e58552c
Lantern,Given the new home do we want to rename this?
I'm happy with `Lantern`, but... I'd also be happy with `Simulator`
Related details
- Adam Raine
- Andres Olivares
- Connor Clark
- Jack Franklin
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Raine (Gerrit)
Adam Raine added 2 comments![]( "Open in Gerrit")
/**Why is this file outside the `trace/lantern` folder?
Lantern,Given the new home do we want to rename this?
I'm happy with `Lantern`, but... I'd also be happy with `Simulator`
I think renaming would be more work than normal since we need to sync between this and the LH repo. I think we should just keep it.
Related details
- Andres Olivares
- Connor Clark
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Irish (Gerrit)
Paul Irish voted and added 1 comment![]( "Open in Gerrit")
Votes added by Paul Irish
| Code-Review | +1 |
1 comment
Lantern,Adam RaineGiven the new home do we want to rename this?
I'm happy with `Lantern`, but... I'd also be happy with `Simulator`
I think renaming would be more work than normal since we need to sync between this and the LH repo. I think we should just keep it.
- Adam Raine
- Andres Olivares
- Connor Clark
- Jack Franklin
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 5 comments![]( "Open in Gerrit")
Patchset 7 (labeled lint), these are my granular local commit messages:
lint
add bug ref to todos
handle naming-convention lint rule
address no-unused-vars lint rule
remove some ts-nochecks
remove unneeded tsconfig.json
re-enable no-explicit-any lint rule
remove unused deps
Why is this file outside the `trace/lantern` folder?
Circular dependencies. I forget the details.
// TODO: off due to Lantern needing more refactoring.maybe it's worth filing a bug for this to ensure we don't forget to resolve this (or tying it to an existing bug)
I addressed most of them here and added https://g-issues.chromium.org/issues/348449529 for the remaining.
"../../../core/common:bundle",
"../../../core/platform:bundle",
"../../../core/root:bundle",
"../../../core/sdk:bundle",Paul Irishdo we expect to use these deps? If we do so it might cause issues when rolling to the standalone library, but I think we don't need them.
If we do so it might cause issues when rolling to the standalone library,
I checked this and it doesn't. (But... I don't know why. (Nor do I know why they're here ;))
Removed - weren't needed as you said. Not sure why I placed them here.
// "incremental": true, /* Save .tsbuildinfo files to allow for incremental compilation of projects. */do we want to eventually enable these or could we remove them completely (or something in between)?
this file wasn't doing anything and should not have been included, removed.
Related details
- Adam Raine
- Andres Olivares
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Raine (Gerrit)
Adam Raine voted Code-Review+1![]( "Open in Gerrit")
- Andres Olivares
- Connor Clark
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
ditto: tracking bug (and other ts-nochecks)
- Andres Olivares
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Olivares (Gerrit)
Andres Olivares voted Code-Review+1![]( "Open in Gerrit")
- Connor Clark
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 3 comments![]( "Open in Gerrit")
Not going to leave a -1 review, because I am OOO next week and do not want to block, but please make sure the imports are updated to follow the DevTools convention before this lands.
All imports that import from a different module must use the entry point to do so. In a release build a module is bundled up to all be contained in its entry point, and hence in our code we have to follow-suit. It might work in dev (where we don't bundle) but broken imports will cause release build breakage.
Connor ClarkWhy is this file outside the `trace/lantern` folder?
Circular dependencies. I forget the details.
can we log a bug for this?
import {MetricName} from './handlers/PageLoadMetricsHandler.js';
import {type TraceParseData} from './handlers/types.js';
import * as Lantern from './lantern/lantern.js';
import {type MicroSeconds} from './types/Timing.js';
import {type TraceEvents} from './types/types.js';
these imports don't follow the DevTools pattern (and I think will break in a release build).
Any time you import a file from another module (e.g. folder) you need to use that folder's entry point (the file named the same as the folder)
So these need to be:
```
import * as Handlers from './handlers/handlers.js'
import * as Lantern from './lantern/lantern.js'
import * as Types from './types/types.js'
```
I am surprised the ESLint rule we have didn't detect this case.
Unfortunately this does lead to more verbose code...but without this our prod bundle will likely not work.
(on a separate note, I agree this file should ideally be within the lantern module. If it can't be for now, let's file a bug and try to move it in a follow-up CL)
Related details
- Connor Clark
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 1 comment![]( "Open in Gerrit")
import {BaseNode, type Node} from '../BaseNode.js';
import {type Extras, Metric} from '../Metric.js';
import {BLOCKING_TIME_THRESHOLD, calculateSumOfBlockingTime} from '../TBTUtils.js';I think these imports may be problematic in DevTools too because of how release builds get bundled - these files will not exist in a release build
I'd suggest moving the contents of the metrics/ folder up a level (you can import files from the same folder at the same level), or putting these files into another module (lantern/core or something) to enable you to do `import * as Core from '../core/core.js'`
(yes this frustrating...but not something that is going to change)
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
import {BaseNode, type Node} from '../BaseNode.js';
import {type Extras, Metric} from '../Metric.js';
import {BLOCKING_TIME_THRESHOLD, calculateSumOfBlockingTime} from '../TBTUtils.js';I think these imports may be problematic in DevTools too because of how release builds get bundled - these files will not exist in a release build
I'd suggest moving the contents of the metrics/ folder up a level (you can import files from the same folder at the same level), or putting these files into another module (lantern/core or something) to enable you to do `import * as Core from '../core/core.js'`
(yes this frustrating...but not something that is going to change)
I assume this is why the layout tests are failing, but the other tests are fine in CI? I wasn't able to get an error message to debug that further.
Related details
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 1 comment![]( "Open in Gerrit")
import {BaseNode, type Node} from '../BaseNode.js';
import {type Extras, Metric} from '../Metric.js';
import {BLOCKING_TIME_THRESHOLD, calculateSumOfBlockingTime} from '../TBTUtils.js';Connor ClarkI think these imports may be problematic in DevTools too because of how release builds get bundled - these files will not exist in a release build
I'd suggest moving the contents of the metrics/ folder up a level (you can import files from the same folder at the same level), or putting these files into another module (lantern/core or something) to enable you to do `import * as Core from '../core/core.js'`
(yes this frustrating...but not something that is going to change)
I assume this is why the layout tests are failing, but the other tests are fine in CI? I wasn't able to get an error message to debug that further.
Yeah I expect so.
You could run a release build locally and you would probably see the error if you try to run the performance panel
Related details
- Connor Clark
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
import {BaseNode, type Node} from '../BaseNode.js';
import {type Extras, Metric} from '../Metric.js';
import {BLOCKING_TIME_THRESHOLD, calculateSumOfBlockingTime} from '../TBTUtils.js';Connor ClarkI think these imports may be problematic in DevTools too because of how release builds get bundled - these files will not exist in a release build
I'd suggest moving the contents of the metrics/ folder up a level (you can import files from the same folder at the same level), or putting these files into another module (lantern/core or something) to enable you to do `import * as Core from '../core/core.js'`
(yes this frustrating...but not something that is going to change)
Jack FranklinI assume this is why the layout tests are failing, but the other tests are fine in CI? I wasn't able to get an error message to debug that further.
Yeah I expect so.
You could run a release build locally and you would probably see the error if you try to run the performance panel
Release is the default right? My args.gn is empty. Not seeing any errors running DevTools locally.
Related details
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
Connor ClarkWhy is this file outside the `trace/lantern` folder?
Jack FranklinCircular dependencies. I forget the details.
can we log a bug for this?
Semantically I think this file belongs here. It is taking the result of the trace engine and forming it into inputs for Lantern.
Trying to get the lantern module to depend on the trace module, which itself exports Lantern, is the dep. cycle:
```
ERROR Dependency cycle:
//front_end/models/trace:trace ->
//front_end/models/trace/lantern:bundle ->
//front_end/models/trace/lantern:devtools_entrypoint-bundle-typescript ->
//front_end/models/trace/lantern:lantern ->
//front_end/models/trace:trace
```
Am I missing something that would allow code to be structured this way? I think this needs to be where it currently resides.
Related details
- Adam Raine
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
Not going to leave a -1 review, because I am OOO next week and do not want to block, but please make sure the imports are updated to follow the DevTools convention before this lands.
All imports that import from a different module must use the entry point to do so. In a release build a module is bundled up to all be contained in its entry point, and hence in our code we have to follow-suit. It might work in dev (where we don't bundle) but broken imports will cause release build breakage.
I'm unclear exactly what's disallowed since the linter isn't being helpful, but can we assume that if the layout tests are passing then this issue is resolved?
Jack Franklin (Gerrit)
Jack Franklin added 2 comments![]( "Open in Gerrit")
Connor ClarkNot going to leave a -1 review, because I am OOO next week and do not want to block, but please make sure the imports are updated to follow the DevTools convention before this lands.
All imports that import from a different module must use the entry point to do so. In a release build a module is bundled up to all be contained in its entry point, and hence in our code we have to follow-suit. It might work in dev (where we don't bundle) but broken imports will cause release build breakage.
I'm unclear exactly what's disallowed since the linter isn't being helpful, but can we assume that if the layout tests are passing then this issue is resolved?
See my two other comments for examples in context, but TL;DR is:
1. You must import from the entry point if you are importing from another module (aka folder)
GOOD: import * as X from './x/x.js';
BAD: import {Something} from './x/some-sub-module.js';
2. You can only import from the non-entrypoint if you are within that module's folder.
So `x/one.js` can import directly from `x/two.js`, if you want to import from `y/one.js`, you have to access it via `y/y.js` (the entrypoint)
Connor ClarkWhy is this file outside the `trace/lantern` folder?
Jack FranklinCircular dependencies. I forget the details.
Connor Clarkcan we log a bug for this?
Semantically I think this file belongs here. It is taking the result of the trace engine and forming it into inputs for Lantern.
Trying to get the lantern module to depend on the trace module, which itself exports Lantern, is the dep. cycle:
```
ERROR Dependency cycle:
//front_end/models/trace:trace ->
//front_end/models/trace/lantern:bundle ->
//front_end/models/trace/lantern:devtools_entrypoint-bundle-typescript ->
//front_end/models/trace/lantern:lantern ->
//front_end/models/trace:trace
```Am I missing something that would allow code to be structured this way? I think this needs to be where it currently resides.
Ah sorry for causing confusion - I was going off Adam's comment and thought maybe this wasn't the right home for it. If it is then let's leave it!
Related details
- Adam Raine
- Connor Clark
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jack Franklin (Gerrit)
Jack Franklin added 1 comment![]( "Open in Gerrit")
import {BaseNode, type Node} from '../BaseNode.js';
import {type Extras, Metric} from '../Metric.js';
import {BLOCKING_TIME_THRESHOLD, calculateSumOfBlockingTime} from '../TBTUtils.js';Connor ClarkI think these imports may be problematic in DevTools too because of how release builds get bundled - these files will not exist in a release build
I'd suggest moving the contents of the metrics/ folder up a level (you can import files from the same folder at the same level), or putting these files into another module (lantern/core or something) to enable you to do `import * as Core from '../core/core.js'`
(yes this frustrating...but not something that is going to change)
Jack FranklinI assume this is why the layout tests are failing, but the other tests are fine in CI? I wasn't able to get an error message to debug that further.
Connor ClarkYeah I expect so.
You could run a release build locally and you would probably see the error if you try to run the performance panel
Release is the default right? My args.gn is empty. Not seeing any errors running DevTools locally.
I think `is_debug` defaults to true.
So you need to do a build with is_debug = false
Jack Franklin (Gerrit)
Jack Franklin added 1 comment![]( "Open in Gerrit")
import {MetricName} from './handlers/PageLoadMetricsHandler.js';
import {type TraceParseData} from './handlers/types.js';
import * as Lantern from './lantern/lantern.js';
import {type MicroSeconds} from './types/Timing.js';
import {type TraceEvents} from './types/types.js';
these imports don't follow the DevTools pattern (and I think will break in a release build).
Any time you import a file from another module (e.g. folder) you need to use that folder's entry point (the file named the same as the folder)
So these need to be:
```
import * as Handlers from './handlers/handlers.js'
import * as Lantern from './lantern/lantern.js'
import * as Types from './types/types.js'
```I am surprised the ESLint rule we have didn't detect this case.
Unfortunately this does lead to more verbose code...but without this our prod bundle will likely not work.
(on a separate note, I agree this file should ideally be within the lantern module. If it can't be for now, let's file a bug and try to move it in a follow-up CL)
"(on a separate note, I agree this file should ideally be within the lantern module. If it can't be for now, let's file a bug and try to move it in a follow-up CL)"
ignore this bit, I don't think this now 😄
Connor Clark (Gerrit)
Connor Clark added 2 comments![]( "Open in Gerrit")
import {MetricName} from './handlers/PageLoadMetricsHandler.js';
import {type TraceParseData} from './handlers/types.js';
import * as Lantern from './lantern/lantern.js';
import {type MicroSeconds} from './types/Timing.js';
import {type TraceEvents} from './types/types.js';
Jack Franklinthese imports don't follow the DevTools pattern (and I think will break in a release build).
Any time you import a file from another module (e.g. folder) you need to use that folder's entry point (the file named the same as the folder)
So these need to be:
```
import * as Handlers from './handlers/handlers.js'
import * as Lantern from './lantern/lantern.js'
import * as Types from './types/types.js'
```I am surprised the ESLint rule we have didn't detect this case.
Unfortunately this does lead to more verbose code...but without this our prod bundle will likely not work.
(on a separate note, I agree this file should ideally be within the lantern module. If it can't be for now, let's file a bug and try to move it in a follow-up CL)
"(on a separate note, I agree this file should ideally be within the lantern module. If it can't be for now, let's file a bug and try to move it in a follow-up CL)"
ignore this bit, I don't think this now 😄
Done
traverseGenerator(getNextNodes?: (arg0: Node) => Node[]):what formatter does cdt use?
note how it mangled the iterator function syntax here
Related details
- Adam Raine
- Andres Olivares
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 2 comments![]( "Open in Gerrit")
import {BaseNode, type Node} from '../BaseNode.js';
import {type Extras, Metric} from '../Metric.js';
import {BLOCKING_TIME_THRESHOLD, calculateSumOfBlockingTime} from '../TBTUtils.js';Connor ClarkI think these imports may be problematic in DevTools too because of how release builds get bundled - these files will not exist in a release build
I'd suggest moving the contents of the metrics/ folder up a level (you can import files from the same folder at the same level), or putting these files into another module (lantern/core or something) to enable you to do `import * as Core from '../core/core.js'`
(yes this frustrating...but not something that is going to change)
Jack FranklinI assume this is why the layout tests are failing, but the other tests are fine in CI? I wasn't able to get an error message to debug that further.
Connor ClarkYeah I expect so.
You could run a release build locally and you would probably see the error if you try to run the performance panel
Jack FranklinRelease is the default right? My args.gn is empty. Not seeing any errors running DevTools locally.
I think `is_debug` defaults to true.
So you need to do a build with is_debug = false
This is resolved - layout tests are working now.
Note that this file and the ones it imports are all within the same devtools module.
Connor ClarkSomeone either knowledgable about software licenses, or at least with an opinion, please weigh in on if I should simply delete these License comments from the original source files.
A potential option might be to put the license in a LICENSE file in the `lantern` folder.
I think outright deleting it is not correct, because Chromium != Google, right? So while the CLA we have LH contributors sign assigns ownership to Google, does that mean a Chromium tree can treat it like it owns it? shrug.
Jack Franklinbtw "treat it like it owns it" in this case mean, ignoring the requirements of the license to include the license text in derivative works. If one owns the rights to the code, that requirement doesn't apply.
Paul Irish@yan...@chromium.org might be able to aid here or know the best person to talk to
gemini, at least, says this is fine, but has guidance around attribution: https://g.co/gemini/share/bfdb4e58552c
I removed the extra license comments and wrote a notice in the directory README.
Related details
- Adam Raine
- Andres Olivares
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 1 comment![]( "Open in Gerrit")
seems random layout tests (just ~5) are failing in CI, but not locally for me. expected?
Related details
- Adam Raine
- Andres Olivares
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Olivares (Gerrit)
Andres Olivares added 4 comments![]( "Open in Gerrit")
seems random layout tests (just ~5) are failing in CI, but not locally for me. expected?
mmm this is weird. Reading the failures at https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true, it doesn't seem expected and it's likely something specific to this cl.
what are the layout tests you refer to?
traverseGenerator(getNextNodes?: (arg0: Node) => Node[]):what formatter does cdt use?
note how it mangled the iterator function syntax here
we use clang (https://chromium.googlesource.com/external/github.com/llvm/llvm-project/clang/tools/clang-format.git).
When undesired stuff like this happens you can surround the affected code with
```
// clang-format off
...
// clang-format on
```
to prevent the mangling
bundle files in dt usually export the content of all files in the module like
```
import * as FileName1 from 'Filename1.ts'
import * as FileName2 from 'Filename2.ts'
...
export {
FileName1,
FileName2
...
}
```I think the way we are doing it here should work with dt bundling so using this pattern is not strictly required, but the way we import stuff from this module would look a bit different. I don't feel strongly about it tho but maybe it's worth considering if the refactoring to accomodate to this is straight forward. If it is turns out being too much refactoring then feel free to ignore. (sorry for not catching this before). (same for metrics.ts)
/**
* @license
* Copyright 2021 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
per the previous comments, I think you meant to remove these license headers? (there are other files with these remaining too)
Related details
- Adam Raine
- Connor Clark
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Olivares (Gerrit)
Andres Olivares added 1 comment![]( "Open in Gerrit")
Connor ClarkNot going to leave a -1 review, because I am OOO next week and do not want to block, but please make sure the imports are updated to follow the DevTools convention before this lands.
All imports that import from a different module must use the entry point to do so. In a release build a module is bundled up to all be contained in its entry point, and hence in our code we have to follow-suit. It might work in dev (where we don't bundle) but broken imports will cause release build breakage.
Jack FranklinI'm unclear exactly what's disallowed since the linter isn't being helpful, but can we assume that if the layout tests are passing then this issue is resolved?
See my two other comments for examples in context, but TL;DR is:
1. You must import from the entry point if you are importing from another module (aka folder)
GOOD: import * as X from './x/x.js';
BAD: import {Something} from './x/some-sub-module.js';2. You can only import from the non-entrypoint if you are within that module's folder.
So `x/one.js` can import directly from `x/two.js`, if you want to import from `y/one.js`, you have to access it via `y/y.js` (the entrypoint)
Interesting. The way it's implemented right now, all files belong to the same single build module (even if inside different folders). This allows to organize the files nicely while preventing cirular dependencies at build time when imports are added across folders. It also makes it possible for files from one folder to import files from another directly (not via a entrypoint file / bundle file).
My guess would be that this should be fine because this will all end up being bundled into lantern.js. However the devtools browser test failures seem to indicate otherwise, because they error when devtools modules are tried to be loaded in the test environment (https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true).
I wonder if there's a set of imports that violates how the bundler works that we can fix easily. Otherwise, I think we would need to stick to the usual pattern of defining one entry point per folder/module (we have the `ts` entry point files but are missing the corresponding BUILD.gn file for each) and ensure we use a module's entrypoint when importing from another module. Doing this would likely cause circular dependencies so we would have to split the stuff into more submodules, which is annoying but likely needed if we don't find the culprit in the current implementation.
Andres Olivares (Gerrit)
Andres Olivares added 1 comment![]( "Open in Gerrit")
Andres Olivaresseems random layout tests (just ~5) are failing in CI, but not locally for me. expected?
mmm this is weird. Reading the failures at https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true, it doesn't seem expected and it's likely something specific to this cl.
what are the layout tests you refer to?
resolving as this is discussed in another comment thread
Andres Olivares (Gerrit)
Andres Olivares added 1 comment![]( "Open in Gerrit")
export * as Metrics from './metrics/metrics.js';
export * as Simulation from './simulation/simulation.js';after investigating with @szu...@chromium.org, we found out these imports are the culprit of the test failures. Because these entrypoints don't have an associated devtools module (no build.gn file on those folders) they don't make it to the runtime.
two options are available:
1. moving `/metrics/metrics.js` and `./simulation/simulation.js` into the parent folder (`lantern/`)
or
2. creating actual devtools modules for metrics/ and simulation/ (by creating the corresponding BUILD.gn files.
I think 2 is likely going to cause circular dependencies and as such will be more painful to do, but I don't have a strong preferrence!
Connor Clark (Gerrit)
Connor Clark added 3 comments![]( "Open in Gerrit")
Connor ClarkNot going to leave a -1 review, because I am OOO next week and do not want to block, but please make sure the imports are updated to follow the DevTools convention before this lands.
All imports that import from a different module must use the entry point to do so. In a release build a module is bundled up to all be contained in its entry point, and hence in our code we have to follow-suit. It might work in dev (where we don't bundle) but broken imports will cause release build breakage.
Jack FranklinI'm unclear exactly what's disallowed since the linter isn't being helpful, but can we assume that if the layout tests are passing then this issue is resolved?
Andres OlivaresSee my two other comments for examples in context, but TL;DR is:
1. You must import from the entry point if you are importing from another module (aka folder)
GOOD: import * as X from './x/x.js';
BAD: import {Something} from './x/some-sub-module.js';2. You can only import from the non-entrypoint if you are within that module's folder.
So `x/one.js` can import directly from `x/two.js`, if you want to import from `y/one.js`, you have to access it via `y/y.js` (the entrypoint)
Interesting. The way it's implemented right now, all files belong to the same single build module (even if inside different folders). This allows to organize the files nicely while preventing cirular dependencies at build time when imports are added across folders. It also makes it possible for files from one folder to import files from another directly (not via a entrypoint file / bundle file).
My guess would be that this should be fine because this will all end up being bundled into lantern.js. However the devtools browser test failures seem to indicate otherwise, because they error when devtools modules are tried to be loaded in the test environment (https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true).
I wonder if there's a set of imports that violates how the bundler works that we can fix easily. Otherwise, I think we would need to stick to the usual pattern of defining one entry point per folder/module (we have the `ts` entry point files but are missing the corresponding BUILD.gn file for each) and ensure we use a module's entrypoint when importing from another module. Doing this would likely cause circular dependencies so we would have to split the stuff into more submodules, which is annoying but likely needed if we don't find the culprit in the current implementation.
I'm thinking my naming of `lowercase/lowercase.ts` files that weren't really devtools entrypoints was messing up the bundler. Perhaps it uses the file naming convention to assume things that are not codified in the BUILD files.
Andres Olivaresseems random layout tests (just ~5) are failing in CI, but not locally for me. expected?
Andres Olivaresmmm this is weird. Reading the failures at https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true, it doesn't seem expected and it's likely something specific to this cl.
what are the layout tests you refer to?
resolving as this is discussed in another comment thread
export * as Metrics from './metrics/metrics.js';
export * as Simulation from './simulation/simulation.js';after investigating with @szu...@chromium.org, we found out these imports are the culprit of the test failures. Because these entrypoints don't have an associated devtools module (no build.gn file on those folders) they don't make it to the runtime.
two options are available:
1. moving `/metrics/metrics.js` and `./simulation/simulation.js` into the parent folder (`lantern/`)
or
2. creating actual devtools modules for metrics/ and simulation/ (by creating the corresponding BUILD.gn files.
I think 2 is likely going to cause circular dependencies and as such will be more painful to do, but I don't have a strong preferrence!
Huh, I thought inclusion of those two files in the lantern BUILD devtools_module would have allowed for this.
100% right about 2) being tough b/c of circular dependencies (between metrics and simulation) - would have to make a third Core module I think (like Jack suggested) but the additional churn is not ideal from the perspective of consuming in Lighthouse. But I wouldn't be against a future refactor down the line.
I wonder why only a handful of tests seem to fail... prior I had similar bundling issues that failed _every_ layout test, but seeing just 5 fail now made me think it must be something else. Also debugging the web test locally[1] didn't reproduce for me like it did when I forgot a `type` on an TS type export. oh well
[1] npm run debug-webtest -- --target=Default http/tests/devtools/console/console-preserve-log-x-process-navigation.js
Related details
- Adam Raine
- Andres Olivares
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 3 comments![]( "Open in Gerrit")
traverseGenerator(getNextNodes?: (arg0: Node) => Node[]):Andres Olivareswhat formatter does cdt use?
note how it mangled the iterator function syntax here
we use clang (https://chromium.googlesource.com/external/github.com/llvm/llvm-project/clang/tools/clang-format.git).
When undesired stuff like this happens you can surround the affected code with
```
// clang-format off
...
// clang-format on
```to prevent the mangling
bundle files in dt usually export the content of all files in the module like
```
import * as FileName1 from 'Filename1.ts'
import * as FileName2 from 'Filename2.ts'
...export {
FileName1,
FileName2
...
}
```I think the way we are doing it here should work with dt bundling so using this pattern is not strictly required, but the way we import stuff from this module would look a bit different. I don't feel strongly about it tho but maybe it's worth considering if the refactoring to accomodate to this is straight forward. If it is turns out being too much refactoring then feel free to ignore. (sorry for not catching this before). (same for metrics.ts)
I modified these module files a bit, but anything further is a lot more refactoring so I've left it as a TODO.
* @license
* Copyright 2021 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/
per the previous comments, I think you meant to remove these license headers? (there are other files with these remaining too)
Done
Related details
- Adam Raine
- Andres Olivares
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Olivares (Gerrit)
Andres Olivares voted and added 3 comments![]( "Open in Gerrit")
Votes added by Andres Olivares
| Code-Review | +1 |
Connor ClarkNot going to leave a -1 review, because I am OOO next week and do not want to block, but please make sure the imports are updated to follow the DevTools convention before this lands.
All imports that import from a different module must use the entry point to do so. In a release build a module is bundled up to all be contained in its entry point, and hence in our code we have to follow-suit. It might work in dev (where we don't bundle) but broken imports will cause release build breakage.
Jack FranklinI'm unclear exactly what's disallowed since the linter isn't being helpful, but can we assume that if the layout tests are passing then this issue is resolved?
Andres OlivaresSee my two other comments for examples in context, but TL;DR is:
1. You must import from the entry point if you are importing from another module (aka folder)
GOOD: import * as X from './x/x.js';
BAD: import {Something} from './x/some-sub-module.js';2. You can only import from the non-entrypoint if you are within that module's folder.
So `x/one.js` can import directly from `x/two.js`, if you want to import from `y/one.js`, you have to access it via `y/y.js` (the entrypoint)
Connor ClarkInteresting. The way it's implemented right now, all files belong to the same single build module (even if inside different folders). This allows to organize the files nicely while preventing cirular dependencies at build time when imports are added across folders. It also makes it possible for files from one folder to import files from another directly (not via a entrypoint file / bundle file).
My guess would be that this should be fine because this will all end up being bundled into lantern.js. However the devtools browser test failures seem to indicate otherwise, because they error when devtools modules are tried to be loaded in the test environment (https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true).
I wonder if there's a set of imports that violates how the bundler works that we can fix easily. Otherwise, I think we would need to stick to the usual pattern of defining one entry point per folder/module (we have the `ts` entry point files but are missing the corresponding BUILD.gn file for each) and ensure we use a module's entrypoint when importing from another module. Doing this would likely cause circular dependencies so we would have to split the stuff into more submodules, which is annoying but likely needed if we don't find the culprit in the current implementation.
I'm thinking my naming of `lowercase/lowercase.ts` files that weren't really devtools entrypoints was messing up the bundler. Perhaps it uses the file naming convention to assume things that are not codified in the BUILD files.
yes this should also work! Nonetheless, I'd suggest that as a non-blocker we really try to stick as closely as we can to devtools pattern in the near future
export * as Metrics from './metrics/metrics.js';
export * as Simulation from './simulation/simulation.js';Connor Clarkafter investigating with @szu...@chromium.org, we found out these imports are the culprit of the test failures. Because these entrypoints don't have an associated devtools module (no build.gn file on those folders) they don't make it to the runtime.
two options are available:
1. moving `/metrics/metrics.js` and `./simulation/simulation.js` into the parent folder (`lantern/`)
or
2. creating actual devtools modules for metrics/ and simulation/ (by creating the corresponding BUILD.gn files.
I think 2 is likely going to cause circular dependencies and as such will be more painful to do, but I don't have a strong preferrence!
Huh, I thought inclusion of those two files in the lantern BUILD devtools_module would have allowed for this.
100% right about 2) being tough b/c of circular dependencies (between metrics and simulation) - would have to make a third Core module I think (like Jack suggested) but the additional churn is not ideal from the perspective of consuming in Lighthouse. But I wouldn't be against a future refactor down the line.
I wonder why only a handful of tests seem to fail... prior I had similar bundling issues that failed _every_ layout test, but seeing just 5 fail now made me think it must be something else. Also debugging the web test locally[1] didn't reproduce for me like it did when I forgot a `type` on an TS type export. oh well
[1] npm run debug-webtest -- --target=Default http/tests/devtools/console/console-preserve-log-x-process-navigation.js
it seems no tests are failing in cq now?
Connor Clarkbundle files in dt usually export the content of all files in the module like
```
import * as FileName1 from 'Filename1.ts'
import * as FileName2 from 'Filename2.ts'
...export {
FileName1,
FileName2
...
}
```I think the way we are doing it here should work with dt bundling so using this pattern is not strictly required, but the way we import stuff from this module would look a bit different. I don't feel strongly about it tho but maybe it's worth considering if the refactoring to accomodate to this is straight forward. If it is turns out being too much refactoring then feel free to ignore. (sorry for not catching this before). (same for metrics.ts)
I modified these module files a bit, but anything further is a lot more refactoring so I've left it as a TODO.
- Adam Raine
- Connor Clark
- Jack Franklin
- Paul Irish
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andres Olivares (Gerrit)
Andres Olivares added 1 comment![]( "Open in Gerrit")
Andres Olivaresseems random layout tests (just ~5) are failing in CI, but not locally for me. expected?
Andres Olivaresmmm this is weird. Reading the failures at https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true, it doesn't seem expected and it's likely something specific to this cl.
what are the layout tests you refer to?
Connor Clarkresolving as this is discussed in another comment thread
ah I missed this. These seem to be flakes and unrelated to this CL. They have been failing in other CLs too:
Andres Olivares (Gerrit)
Andres Olivares added 1 comment![]( "Open in Gerrit")
Andres Olivaresseems random layout tests (just ~5) are failing in CI, but not locally for me. expected?
Andres Olivaresmmm this is weird. Reading the failures at https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true, it doesn't seem expected and it's likely something specific to this cl.
what are the layout tests you refer to?
Connor Clarkresolving as this is discussed in another comment thread
Andres Olivares
ah I missed this. These seem to be flakes and unrelated to this CL. They have been failing in other CLs too:
Paul Irish (Gerrit)
Paul Irish voted Code-Review+1![]( "Open in Gerrit")
- Adam Raine
- Connor Clark
- Jack Franklin
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark added 2 comments![]( "Open in Gerrit")
Connor ClarkNot going to leave a -1 review, because I am OOO next week and do not want to block, but please make sure the imports are updated to follow the DevTools convention before this lands.
All imports that import from a different module must use the entry point to do so. In a release build a module is bundled up to all be contained in its entry point, and hence in our code we have to follow-suit. It might work in dev (where we don't bundle) but broken imports will cause release build breakage.
Jack FranklinI'm unclear exactly what's disallowed since the linter isn't being helpful, but can we assume that if the layout tests are passing then this issue is resolved?
Andres OlivaresSee my two other comments for examples in context, but TL;DR is:
1. You must import from the entry point if you are importing from another module (aka folder)
GOOD: import * as X from './x/x.js';
BAD: import {Something} from './x/some-sub-module.js';2. You can only import from the non-entrypoint if you are within that module's folder.
So `x/one.js` can import directly from `x/two.js`, if you want to import from `y/one.js`, you have to access it via `y/y.js` (the entrypoint)
Connor ClarkInteresting. The way it's implemented right now, all files belong to the same single build module (even if inside different folders). This allows to organize the files nicely while preventing cirular dependencies at build time when imports are added across folders. It also makes it possible for files from one folder to import files from another directly (not via a entrypoint file / bundle file).
My guess would be that this should be fine because this will all end up being bundled into lantern.js. However the devtools browser test failures seem to indicate otherwise, because they error when devtools modules are tried to be loaded in the test environment (https://chromium-swarm.appspot.com/task?id=6a6a2c02f5559710&w=true).
I wonder if there's a set of imports that violates how the bundler works that we can fix easily. Otherwise, I think we would need to stick to the usual pattern of defining one entry point per folder/module (we have the `ts` entry point files but are missing the corresponding BUILD.gn file for each) and ensure we use a module's entrypoint when importing from another module. Doing this would likely cause circular dependencies so we would have to split the stuff into more submodules, which is annoying but likely needed if we don't find the culprit in the current implementation.
Andres OlivaresI'm thinking my naming of `lowercase/lowercase.ts` files that weren't really devtools entrypoints was messing up the bundler. Perhaps it uses the file naming convention to assume things that are not codified in the BUILD files.
yes this should also work! Nonetheless, I'd suggest that as a non-blocker we really try to stick as closely as we can to devtools pattern in the near future
I'll start on a follow-up CL to resolve this today.
export * as Metrics from './metrics/metrics.js';
export * as Simulation from './simulation/simulation.js';Connor Clarkafter investigating with @szu...@chromium.org, we found out these imports are the culprit of the test failures. Because these entrypoints don't have an associated devtools module (no build.gn file on those folders) they don't make it to the runtime.
two options are available:
1. moving `/metrics/metrics.js` and `./simulation/simulation.js` into the parent folder (`lantern/`)
or
2. creating actual devtools modules for metrics/ and simulation/ (by creating the corresponding BUILD.gn files.
I think 2 is likely going to cause circular dependencies and as such will be more painful to do, but I don't have a strong preferrence!
Andres OlivaresHuh, I thought inclusion of those two files in the lantern BUILD devtools_module would have allowed for this.
100% right about 2) being tough b/c of circular dependencies (between metrics and simulation) - would have to make a third Core module I think (like Jack suggested) but the additional churn is not ideal from the perspective of consuming in Lighthouse. But I wouldn't be against a future refactor down the line.
I wonder why only a handful of tests seem to fail... prior I had similar bundling issues that failed _every_ layout test, but seeing just 5 fail now made me think it must be something else. Also debugging the web test locally[1] didn't reproduce for me like it did when I forgot a `type` on an TS type export. oh well
[1] npm run debug-webtest -- --target=Default http/tests/devtools/console/console-preserve-log-x-process-navigation.js
it seems no tests are failing in cq now?
- Adam Raine
- Jack Franklin
- Yang Guo
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Connor Clark (Gerrit)
Connor Clark voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Devtools-frontend LUCI CQ (Gerrit)
Devtools-frontend LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[trace engine] Add Lighthouse's Lantern library
This is the culmination of isolating Lighthouse's Lantern library
(https://github.com/GoogleChrome/lighthouse/issues/15841). The code in
this CL took Lantern as it now exists in Lighthouse, converted to
TypeScript, integrated its unit tests (Jest -> Karma conversion), and
resolved the majority of CDT's JavaScript linter errors.
There is no behavior change to CDT with this CL, besides TraceModel
exporting the Lantern module.
There is some refactoring work remaining, captured by
crbug.com/348449529
- M config/gni/devtools_grd_files.gni
- M front_end/BUILD.gn
- M front_end/models/trace/BUILD.gn
- A front_end/models/trace/LanternComputationData.ts
- A front_end/models/trace/lantern/.eslintrc.js
- A front_end/models/trace/lantern/BUILD.gn
- A front_end/models/trace/lantern/BaseNode.test.ts
- A front_end/models/trace/lantern/BaseNode.ts
- A front_end/models/trace/lantern/CPUNode.ts
- A front_end/models/trace/lantern/LanternError.ts
- A front_end/models/trace/lantern/MetricsModule.ts
- A front_end/models/trace/lantern/NetworkNode.ts
- A front_end/models/trace/lantern/PageDependencyGraph.test.ts
- A front_end/models/trace/lantern/PageDependencyGraph.ts
- A front_end/models/trace/lantern/README.md
- A front_end/models/trace/lantern/SimulationModule.ts
- A front_end/models/trace/lantern/lantern.ts
- A front_end/models/trace/lantern/metrics/FirstContentfulPaint.test.ts
- A front_end/models/trace/lantern/metrics/FirstContentfulPaint.ts
- A front_end/models/trace/lantern/metrics/Interactive.test.ts
- A front_end/models/trace/lantern/metrics/Interactive.ts
- A front_end/models/trace/lantern/metrics/LargestContentfulPaint.test.ts
- A front_end/models/trace/lantern/metrics/LargestContentfulPaint.ts
- A front_end/models/trace/lantern/metrics/MaxPotentialFID.ts
- A front_end/models/trace/lantern/metrics/Metric.ts
- A front_end/models/trace/lantern/metrics/SpeedIndex.test.ts
- A front_end/models/trace/lantern/metrics/SpeedIndex.ts
- A front_end/models/trace/lantern/metrics/TBTUtils.test.ts
- A front_end/models/trace/lantern/metrics/TBTUtils.ts
- A front_end/models/trace/lantern/metrics/TotalBlockingTime.ts
- A front_end/models/trace/lantern/simulation/ConnectionPool.test.ts
- A front_end/models/trace/lantern/simulation/ConnectionPool.ts
- A front_end/models/trace/lantern/simulation/Constants.ts
- A front_end/models/trace/lantern/simulation/DNSCache.test.ts
- A front_end/models/trace/lantern/simulation/DNSCache.ts
- A front_end/models/trace/lantern/simulation/NetworkAnalyzer.test.ts
- A front_end/models/trace/lantern/simulation/NetworkAnalyzer.ts
- A front_end/models/trace/lantern/simulation/SimulationTimingMap.ts
- A front_end/models/trace/lantern/simulation/Simulator.test.ts
- A front_end/models/trace/lantern/simulation/Simulator.ts
- A front_end/models/trace/lantern/simulation/TCPConnection.test.ts
- A front_end/models/trace/lantern/simulation/TCPConnection.ts
- A front_end/models/trace/lantern/testing/MetricTestUtils.ts
- A front_end/models/trace/lantern/types/lantern.ts
- M front_end/models/trace/trace.ts
- M front_end/panels/timeline/fixtures/traces/BUILD.gn
- A front_end/panels/timeline/fixtures/traces/lantern/README.md
- A front_end/panels/timeline/fixtures/traces/lantern/iframe/trace.json.gz
- A front_end/panels/timeline/fixtures/traces/lantern/paul/trace.json.gz
- A front_end/panels/timeline/fixtures/traces/lantern/progressive-app/trace.json.gz
- A front_end/panels/timeline/fixtures/traces/lantern/redirect/trace.json.gz
Code-Review: +1 by Andres Olivares, +1 by Paul Irish
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |