Skip to content

Load about blank doc to set window #630

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/browser/browser.zig
Original file line number Diff line number Diff line change
Expand Up @@ -335,6 +335,8 @@ pub const Page = struct {

// if the url is about:blank, nothing to do.
if (std.mem.eql(u8, "about:blank", request_url.raw)) {
var about_blank = AboutBlank{};
Copy link
Collaborator

Choose a reason for hiding this comment

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

AboutBlank is pretty clever, but this would work too I think:

var fbs = std.io.fixedBufferStream("");
try self.loadHTMLDoc(fbs.reader(), "utf-8");

try self.loadHTMLDoc(&about_blank, "utf-8");
Copy link
Collaborator

Choose a reason for hiding this comment

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

loadHTMLDoc does a lot of stuff you probably don't need (like walking through the the DOM, finding script tabs, setting up click events, ...), but then it probably does stuff you need, let setting the window's html_doc.

I wonder if the behavior can be split into two functions, loadHTMLDoc and processHTMLDoc. You'd only call loadHTMLDoc, and the navigate flow would call loadHTMLDoc followed by processHTMLDoc...might require reorganizing things a little. I'm just thining about the few microseconds that'll be spent on this - not sure it's worth it.

return;
}

Expand Down Expand Up @@ -393,6 +395,16 @@ pub const Page = struct {
});
}

// A minimal reader for about:blank such that it can be parsed by loadHTMLDoc.
pub const AboutBlank = struct {
done: bool = false,
pub fn next(self: *AboutBlank) !?[]const u8 {
if (self.done) return null;
self.done = true;
return ""; // The contents is blank
}
};

// https://html.spec.whatwg.org/#read-html
fn loadHTMLDoc(self: *Page, reader: anytype, charset: []const u8) !void {
const arena = self.arena;
Expand Down
9 changes: 9 additions & 0 deletions src/cdp/domains/target.zig
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,10 @@ fn createTarget(cmd: anytype) !void {
bc.target_id = target_id;

var page = try bc.session.createPage();
// Navigate to about:blank such that the window / state.document are created and set
// Playwright uses these before navigating to a real URL to detect when they are removed.
const url = try @import("../../url.zig").URL.parse("about:blank", "");
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could maybe create a const URL.blank like the existing URL.empty (in url.zig)

try page.navigate(url, .{});
{
const aux_data = try std.fmt.allocPrint(cmd.arena, "{{\"isDefault\":true,\"type\":\"default\",\"frameId\":\"{s}\"}}", .{target_id});
bc.inspector.contextCreated(
Expand Down Expand Up @@ -501,6 +505,11 @@ test "cdp.target: createTarget" {
// should create a browser context
const bc = ctx.cdp().browser_context.?;
try ctx.expectSentEvent("Target.targetCreated", .{ .targetInfo = .{ .url = "about:blank", .title = "about:blank", .attached = false, .type = "page", .canAccessOpener = false, .browserContextId = bc.id, .targetId = bc.target_id.? } }, .{});

// Even about:blank should set the window and document
const page = ctx.cdp().browser_context.?.session.page.?;
try std.testing.expect(page.state.document != null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a const testing = @import("testing.zig"); in this file.

It's src/cdp/testing.zig which includes src/testing.zig, which extends std.testing. There's no expect there, but you can add it if you want.I tend to dislike it because it gives less useful errors, but I agree here it's the only real option.

In testing.zig

pub const expect = std.testing.expect;

in cdp/testing.zig

pub const expect = base.expect;

Or not, I think this code here is fine, just wanted to FYI about the existing testing files.

try std.testing.expect(page.window.document != null);
}

{
Expand Down