[blink] feat: add devtools document.cookie performance issue [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mpde...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mpde...@chromium.org
Reviewer source(s):
mpde...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Benedikt Meurer
- Dmitry Gozman
- Mason Freed
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
- Benedikt Meurer
- Chanran Lan
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benedikt Meurer (Gerrit)
Benedikt Meurer added 1 comment![]( "Open in Gerrit")
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
Please fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 3 comments![]( "Open in Gerrit")
I assume I'm on here mostly for document.*. That looks ok other than the need to move most of the code out of document.c.
// Capture the source location of the JavaScript code that called
// document.cookie
SourceLocation* source_location =
CaptureSourceLocation(GetExecutionContext());
// Get the file path, line number, and column number
String file_path = source_location->Url();
unsigned line_number = source_location->LineNumber();
unsigned column_number = source_location->ColumnNumber();I think the usual thing is to add a helper here:
rather than hard code it all here. Or at least - it seems like the code here should be refactored into a helper *somewhere*.
affected_location->line = line_number > 0 ? line_number - 1 : 0;This seems odd.
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 2 comments![]( "Open in Gerrit")
// Capture the source location of the JavaScript code that called
// document.cookie
SourceLocation* source_location =
CaptureSourceLocation(GetExecutionContext());
// Get the file path, line number, and column number
String file_path = source_location->Url();
unsigned line_number = source_location->LineNumber();
unsigned column_number = source_location->ColumnNumber();I think the usual thing is to add a helper here:
rather than hard code it all here. Or at least - it seems like the code here should be refactored into a helper *somewhere*.
Refactor as a helper function inside document.cc.
affected_location->line = line_number > 0 ? line_number - 1 : 0;This seems odd.
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 2 comments![]( "Open in Gerrit")
// Capture the source location of the JavaScript code that called
// document.cookie
SourceLocation* source_location =
CaptureSourceLocation(GetExecutionContext());
// Get the file path, line number, and column number
String file_path = source_location->Url();
unsigned line_number = source_location->LineNumber();
unsigned column_number = source_location->ColumnNumber();Chanran LanI think the usual thing is to add a helper here:
rather than hard code it all here. Or at least - it seems like the code here should be refactored into a helper *somewhere*.
Refactor as a helper function inside document.cc.
Done
affected_location->line = line_number > 0 ? line_number - 1 : 0;Chanran LanThis seems odd.
Changed. Same to https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/inspector/inspector_audits_issue.cc
Done
Related details
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
// Capture the source location of the JavaScript code that called
// document.cookie
SourceLocation* source_location =
CaptureSourceLocation(GetExecutionContext());
// Get the file path, line number, and column number
String file_path = source_location->Url();
unsigned line_number = source_location->LineNumber();
unsigned column_number = source_location->ColumnNumber();Chanran LanI think the usual thing is to add a helper here:
rather than hard code it all here. Or at least - it seems like the code here should be refactored into a helper *somewhere*.
Chanran LanRefactor as a helper function inside document.cc.
Done
Any reason you can't move most/all of this to inspector_audits_issue.cc? Then you wouldn't have to copy/paste the logic for `createAffectedLocation`, and you could just re-use `CreateProtocolLocation` from there.
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 4 comments![]( "Open in Gerrit")
We would need some tests for this
mojom::blink::AffectedLocationPtr createAffectedLocation(Start the name with a capital letter and move to anonymous namespace.
if (!location->ScriptId()) {Should this be `if (location->ScriptId()) {`?
CaptureSourceLocation(GetExecutionContext());Won't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
Related details
- Chanran Lan
- Dmitry Gozman
Chanran Lan (Gerrit)
Chanran Lan added 4 comments![]( "Open in Gerrit")
mojom::blink::AffectedLocationPtr createAffectedLocation(Start the name with a capital letter and move to anonymous namespace.
Removed
Should this be `if (location->ScriptId()) {`?
Removed
// Capture the source location of the JavaScript code that called
// document.cookie
SourceLocation* source_location =
CaptureSourceLocation(GetExecutionContext());
// Get the file path, line number, and column number
String file_path = source_location->Url();
unsigned line_number = source_location->LineNumber();
unsigned column_number = source_location->ColumnNumber();Chanran LanI think the usual thing is to add a helper here:
rather than hard code it all here. Or at least - it seems like the code here should be refactored into a helper *somewhere*.
Chanran LanRefactor as a helper function inside document.cc.
Mason FreedDone
Any reason you can't move most/all of this to inspector_audits_issue.cc? Then you wouldn't have to copy/paste the logic for `createAffectedLocation`, and you could just re-use `CreateProtocolLocation` from there.
AffectedLocation removed. As Danil Somsikov said, it may be even worse performance issue.
Won't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
// Capture the source location of the JavaScript code that called
// document.cookie
SourceLocation* source_location =
CaptureSourceLocation(GetExecutionContext());
// Get the file path, line number, and column number
String file_path = source_location->Url();
unsigned line_number = source_location->LineNumber();
unsigned column_number = source_location->ColumnNumber();Chanran LanI think the usual thing is to add a helper here:
rather than hard code it all here. Or at least - it seems like the code here should be refactored into a helper *somewhere*.
Chanran LanRefactor as a helper function inside document.cc.
Mason FreedDone
Chanran LanAny reason you can't move most/all of this to inspector_audits_issue.cc? Then you wouldn't have to copy/paste the logic for `createAffectedLocation`, and you could just re-use `CreateProtocolLocation` from there.
AffectedLocation removed. As Danil Somsikov said, it may be even worse performance issue.
Done
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 2 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
We would need some tests for this
+1
CaptureSourceLocation(GetExecutionContext());Chanran LanWon't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
Ok, affected_location code removed.
...isn't the code location kind of the most interesting part of this warning? Without that, how is a developer to figure out where they're doing the bad thing?
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
CaptureSourceLocation(GetExecutionContext());Chanran LanWon't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
Mason FreedOk, affected_location code removed.
...isn't the code location kind of the most interesting part of this warning? Without that, how is a developer to figure out where they're doing the bad thing?
Is there any ways to catch code location with better performance?
Related details
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
The devtools part cr link:
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/7258674
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
CaptureSourceLocation(GetExecutionContext());Chanran LanWon't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
Mason FreedOk, affected_location code removed.
Chanran Lan...isn't the code location kind of the most interesting part of this warning? Without that, how is a developer to figure out where they're doing the bad thing?
Is there any ways to catch code location with better performance?
In my opinion, since we're trying to keep people from using an already-bad API (performance-wise), it's maybe ok to take a little longer to give them more actionable feedback. That would include the code location. Without that, the most likely outcome will be developers ignoring the warning, since they can't figure out where the problem comes from.
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov added 1 comment![]( "Open in Gerrit")
CaptureSourceLocation(GetExecutionContext());Chanran LanWon't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
Mason FreedOk, affected_location code removed.
Chanran Lan...isn't the code location kind of the most interesting part of this warning? Without that, how is a developer to figure out where they're doing the bad thing?
Mason FreedIs there any ways to catch code location with better performance?
In my opinion, since we're trying to keep people from using an already-bad API (performance-wise), it's maybe ok to take a little longer to give them more actionable feedback. That would include the code location. Without that, the most likely outcome will be developers ignoring the warning, since they can't figure out where the problem comes from.
- Benedikt Meurer
- Chanran Lan
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benedikt Meurer (Gerrit)
Benedikt Meurer added 1 comment![]( "Open in Gerrit")
CaptureSourceLocation(GetExecutionContext());Chanran LanWon't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
Mason FreedOk, affected_location code removed.
Chanran Lan...isn't the code location kind of the most interesting part of this warning? Without that, how is a developer to figure out where they're doing the bad thing?
Mason FreedIs there any ways to catch code location with better performance?
Danil SomsikovIn my opinion, since we're trying to keep people from using an already-bad API (performance-wise), it's maybe ok to take a little longer to give them more actionable feedback. That would include the code location. Without that, the most likely outcome will be developers ignoring the warning, since they can't figure out where the problem comes from.
@bme...@chromium.org wdyt?
I agree with Mason: The code location is the most interesting part. We should definitely have it.
Related details
- Chanran Lan
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Chanran Lan
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
CaptureSourceLocation(GetExecutionContext());Chanran LanWon't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
Mason FreedOk, affected_location code removed.
Chanran Lan...isn't the code location kind of the most interesting part of this warning? Without that, how is a developer to figure out where they're doing the bad thing?
Mason FreedIs there any ways to catch code location with better performance?
Danil SomsikovIn my opinion, since we're trying to keep people from using an already-bad API (performance-wise), it's maybe ok to take a little longer to give them more actionable feedback. That would include the code location. Without that, the most likely outcome will be developers ignoring the warning, since they can't figure out where the problem comes from.
Benedikt Meurer@bme...@chromium.org wdyt?
I agree with Mason: The code location is the most interesting part. We should definitely have it.
- Benedikt Meurer
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 2 comments![]( "Open in Gerrit")
Mason FreedWe would need some tests for this
+1
Done
CaptureSourceLocation(GetExecutionContext());Chanran LanWon't this involve stack walking and allocating Mojo structs/IPC messages inside the Document::cookie getter, making the performance problem even worse?
Mason FreedOk, affected_location code removed.
Chanran Lan...isn't the code location kind of the most interesting part of this warning? Without that, how is a developer to figure out where they're doing the bad thing?
Mason FreedIs there any ways to catch code location with better performance?
Danil SomsikovIn my opinion, since we're trying to keep people from using an already-bad API (performance-wise), it's maybe ok to take a little longer to give them more actionable feedback. That would include the code location. Without that, the most likely outcome will be developers ignoring the warning, since they can't figure out where the problem comes from.
Benedikt Meurer@bme...@chromium.org wdyt?
Chanran LanI agree with Mason: The code location is the most interesting part. We should definitely have it.
Okay, I will reset to patchset 9 and add tests.
- Benedikt Meurer
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
- Benedikt Meurer
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benedikt Meurer (Gerrit)
Benedikt Meurer voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
// Capture the source location of the JavaScript code that called
// document.cookie
SourceLocation* source_location =
CaptureSourceLocation(GetExecutionContext());
// Get the file path, line number, and column number
String file_path = source_location->Url();
unsigned line_number = source_location->LineNumber();
unsigned column_number = source_location->ColumnNumber();Chanran LanI think the usual thing is to add a helper here:
rather than hard code it all here. Or at least - it seems like the code here should be refactored into a helper *somewhere*.
Chanran LanRefactor as a helper function inside document.cc.
Mason FreedDone
Chanran LanAny reason you can't move most/all of this to inspector_audits_issue.cc? Then you wouldn't have to copy/paste the logic for `createAffectedLocation`, and you could just re-use `CreateProtocolLocation` from there.
Chanran LanAffectedLocation removed. As Danil Somsikov said, it may be even worse performance issue.
Done
Coming back to this comment, can you please move **as much of this new code as possible** over to `third_party/blink/renderer/core/inspector/inspector_audits_issue.cc`? The new `createAffectedLocation()` method here is almost an exact copy/paste of `CreateProtocolLocation()` in `inspector_audits_issue.cc`. If there's a reason you can't do that, let me know.
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
- Benedikt Meurer
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
- Benedikt Meurer
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
// Capture the source location of the JavaScript code that called
// document.cookie
SourceLocation* source_location =
CaptureSourceLocation(GetExecutionContext());
// Get the file path, line number, and column number
String file_path = source_location->Url();
unsigned line_number = source_location->LineNumber();
unsigned column_number = source_location->ColumnNumber();Chanran LanI think the usual thing is to add a helper here:
rather than hard code it all here. Or at least - it seems like the code here should be refactored into a helper *somewhere*.
Chanran LanRefactor as a helper function inside document.cc.
Mason FreedDone
Chanran LanAny reason you can't move most/all of this to inspector_audits_issue.cc? Then you wouldn't have to copy/paste the logic for `createAffectedLocation`, and you could just re-use `CreateProtocolLocation` from there.
Chanran LanAffectedLocation removed. As Danil Somsikov said, it may be even worse performance issue.
Mason FreedDone
Chanran LanComing back to this comment, can you please move **as much of this new code as possible** over to `third_party/blink/renderer/core/inspector/inspector_audits_issue.cc`? The new `createAffectedLocation()` method here is almost an exact copy/paste of `CreateProtocolLocation()` in `inspector_audits_issue.cc`. If there's a reason you can't do that, let me know.
Sorry about missing this. Just done.
Done
Related details
- Benedikt Meurer
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
auto* source_location = CaptureSourceLocation(GetExecutionContext());
auto performance_issue_details =
protocol::Audits::PerformanceIssueDetails::create()
.setSourceCodeLocation(CreateProtocolLocation(*source_location))
.setPerformanceIssueType(
protocol::Audits::PerformanceIssueTypeEnum::DocumentCookie)
.build();
auto issue_details =
protocol::Audits::InspectorIssueDetails::create()
.setPerformanceIssueDetails(std::move(performance_issue_details))
.build();
auto issue =
protocol::Audits::InspectorIssue::create()
.setCode(protocol::Audits::InspectorIssueCodeEnum::PerformanceIssue)
.setDetails(std::move(issue_details))
.build();
GetExecutionContext()->AddInspectorIssue(AuditsIssue(std::move(issue)));
}```
// Report PerformanceIssue for DevTools
if (auto* execution_context = GetExecutionContext()) {
AuditsIssue::ReportQuirksModeIssue(execution_context);
}
...
void AuditsIssue::ReportDocumentCookieIssue(ExecutionContext* execution_context) {
auto* source_location = CaptureSourceLocation(execution_context);
auto performance_issue_details =
protocol::Audits::PerformanceIssueDetails::create()
.setSourceCodeLocation(CreateProtocolLocation(*source_location))
.setPerformanceIssueType(
protocol::Audits::PerformanceIssueTypeEnum::DocumentCookie)
.build();
auto issue_details =
protocol::Audits::InspectorIssueDetails::create()
.setPerformanceIssueDetails(std::move(performance_issue_details))
.build();
auto issue =
protocol::Audits::InspectorIssue::create()
.setCode(protocol::Audits::InspectorIssueCodeEnum::PerformanceIssue)
.setDetails(std::move(issue_details))
.build();
execution_context->AddInspectorIssue(AuditsIssue(std::move(issue)));
}
```Related details
- Benedikt Meurer
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan removed browser-comp...@chromium.org from this change![]( "Open in Gerrit")
- Benedikt Meurer
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
auto* source_location = CaptureSourceLocation(GetExecutionContext());- Benedikt Meurer
- Danil Somsikov
- Dmitry Gozman
- Mason Freed
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
auto* source_location = CaptureSourceLocation(GetExecutionContext());
auto performance_issue_details =
protocol::Audits::PerformanceIssueDetails::create()
.setSourceCodeLocation(CreateProtocolLocation(*source_location))
.setPerformanceIssueType(
protocol::Audits::PerformanceIssueTypeEnum::DocumentCookie)
.build();
auto issue_details =
protocol::Audits::InspectorIssueDetails::create()
.setPerformanceIssueDetails(std::move(performance_issue_details))
.build();
auto issue =
protocol::Audits::InspectorIssue::create()
.setCode(protocol::Audits::InspectorIssueCodeEnum::PerformanceIssue)
.setDetails(std::move(issue_details))
.build();
GetExecutionContext()->AddInspectorIssue(AuditsIssue(std::move(issue)));
}Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted Code-Review+1![]( "Open in Gerrit")
- Benedikt Meurer
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benedikt Meurer (Gerrit)
Benedikt Meurer voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Matthew Denton
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peng Zhou (Gerrit)
Peng Zhou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Matthew Denton (Gerrit)
Matthew Denton voted Code-Review+1![]( "Open in Gerrit")
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Peng Zhou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Benedikt Meurer
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
- Matthew Denton
- Peng Zhou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peng Zhou (Gerrit)
Peng Zhou voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Benedikt Meurer
- Chanran Lan
- Danil Somsikov
- Dmitry Gozman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peng Zhou (Gerrit)
Peng Zhou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[blink] feat: add devtools document.cookie performance issue
We encountered a major source of jank because front-end developers often use the synchronous API document.cookie. Currently, the asynchronous API CookieStore serves as an alternative, so it would be better to add a warning message when document.cookie is used. In the future, it could be gradually marked as deprecated.
- M third_party/blink/public/devtools_protocol/domains/Audits.pdl
- M third_party/blink/public/mojom/devtools/inspector_issue.mojom
- M third_party/blink/renderer/core/dom/document.cc
- M third_party/blink/renderer/core/inspector/inspector_audits_issue.cc
- M third_party/blink/renderer/core/inspector/inspector_audits_issue.h
- M third_party/blink/renderer/core/inspector/inspector_issue_conversion.cc
- A third_party/blink/web_tests/http/tests/inspector-protocol/issues/document-cookie-performance-issue-expected.txt
- A third_party/blink/web_tests/http/tests/inspector-protocol/issues/document-cookie-performance-issue.js
Code-Review: +1 by Mason Freed, +1 by Peng Zhou
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chanran Lan (Gerrit)
Chanran Lan added 1 comment![]( "Open in Gerrit")
Should I ignore that V8 Perf detected this cr performance? https://issues.chromium.org/issues/484305638. What do you think? @d...@chromium.org @mas...@chromium.org @bme...@chromium.org
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Should I ignore that V8 Perf detected this cr performance? https://issues.chromium.org/issues/484305638. What do you think? @d...@chromium.org @mas...@chromium.org @bme...@chromium.org
Seems ok to me - I commented on the bug.