Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shell: Remove workaround for Chrome crash #21074

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented Oct 3, 2024

Something unknown has changed during the rewrite of the shell and the
crash isn't triggered anymore. Was it ever real? Or was it just a
dream?

This could be part of #21012 if it turns out be robust.

And rewrite much of the state handling to make it easier to
understand.

Specifically, the old Index and MachineIndex classes and their
complicated interactions have been replaced with a hopefully much more
straightforward ShellState class.

However, the existing React components such as CockpitHosts and TopNav
have not been significantly touched.

The API for launching the HostModal dialogs has been changed to make
it more suitable for a later rewrite with Dialogs.run() ala
pkg/lib/cockpit-connect-ssh.
@mvollmer mvollmer mentioned this pull request Oct 3, 2024
8 tasks
pkg/shell/frames.jsx Fixed Show fixed Hide fixed
pkg/shell/frames.jsx Fixed Show fixed Hide fixed
Something unknown has changed during the rewrite of the shell and the
crash isn't triggered anymore. Was it ever real? Or was it just a
dream?
@mvollmer mvollmer force-pushed the shell-remove-chrome-crash-workaround branch from 5e94aa4 to 0ae6019 Compare October 3, 2024 12:40
frame: f,
active,
selected: active,
displayName: f.host === "localhost" ? "/" + f.path : f.host + ":/" + f.path,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

Comment on lines +113 to +116
if (machine.restarting) {
title = _("The machine is rebooting");
message = "";
} else if (connecting) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 4 added lines are not executed by any test.

if (frame.ready && iframe.getAttribute("data-ready") == null)
iframe.setAttribute("data-ready", "1");
else if (!frame.ready && iframe.getAttribute("data-ready"))
iframe.removeAttribute("data-ready");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

if (frame.loaded && iframe.getAttribute("data-loaded") == null)
iframe.setAttribute("data-loaded", "1");
else if (!frame.loaded && iframe.getAttribute("data-loaded"))
iframe.removeAttribute("data-loaded");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

Comment on lines +178 to +179
style === "auto") ||
style === "dark") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 added lines are not executed by any test.

Comment on lines +153 to +156
self.search = function(prop, value) {
for (const x in self.items) {
if (self.items[x][prop] === value)
return self.items[x];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 4 added lines are not executed by any test.

Comment on lines +188 to +192
function component_checksum(machine, path) {
const parts = path.split("/");
const pkg = parts[0];
if (machine.manifests && machine.manifests[pkg] && machine.manifests[pkg][".checksum"])
return "$" + machine.manifests[pkg][".checksum"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 5 added lines are not executed by any test.

export function compute_frame_url(machine, path) {
let base, checksum;
if (machine.manifests && machine.manifests[".checksum"])
checksum = "$" + machine.manifests[".checksum"];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

Comment on lines +202 to +204
if (checksum && checksum == component_checksum(machine, path)) {
if (machine.connection_string === "localhost")
base = "..";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 3 added lines are not executed by any test.

if (machine.connection_string === "localhost")
base = "..";
else
base = "../../" + checksum;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants