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

Refactor parseBrowserContextOptions to Sobek #1526

Merged
merged 6 commits into from
Nov 7, 2024

Conversation

inancgumus
Copy link
Member

@inancgumus inancgumus commented Nov 6, 2024

What?

Refactor parseBrowserContextOptions to use direct Sobek transformation.

Why?

After the refactorings we made in #1270, it's now possible to let Sobek perform this transformation alone.

Checklist

  • I have performed a self-review of my code
  • I have added tests for my changes (locally tested it—see the script)
  • I have commented on my code, particularly in hard-to-understand areas

Script

I also used the script to test it using my old friend Printf in Browser.NewContext etc..

export default async function() {
  const context = await browser.newContext({
    acceptDownloads: true,
    downloadsPath: "/tmp",
    bypassCSP: true,
    colorScheme: "dark",
    deviceScaleFactor: 1,
    extraHTTPHeaders: {
        "X-Header": "value",
    },
    geolocation: { latitude: 51.509865, longitude: -0.118092 },
    hasTouch: true,
    httpCredentials: { username: "admin", password: "password" },
    ignoreHTTPSErrors: true,
    isMobile: true,
    javaScriptEnabled: true,
    locale: "fr-FR",
    offline: true,
    permissions: ["camera", "microphone"],
    reducedMotion: true,
    screen: { width: 800, height: 600 },
    timezoneID: "Europe/Paris",
    userAgent: "my agent",
    viewport: { width: 800, height: 600 },
  });
}

Related PR(s)/Issue(s)

@inancgumus inancgumus self-assigned this Nov 6, 2024
@inancgumus inancgumus marked this pull request as draft November 6, 2024 21:49
@inancgumus inancgumus force-pushed the refactor/parse-browser-options branch 3 times, most recently from 8a891d6 to 7d1388a Compare November 6, 2024 22:11
@inancgumus inancgumus changed the title Remove parseBrowserContextOptions Refactor parseBrowserContextOptions to Sobek Nov 6, 2024
@inancgumus inancgumus force-pushed the refactor/parse-browser-options branch from 7e43386 to eea70f6 Compare November 6, 2024 22:20
@inancgumus inancgumus marked this pull request as ready for review November 6, 2024 22:21
@inancgumus inancgumus force-pushed the refactor/parse-browser-options branch from 3cd03ac to d72e163 Compare November 7, 2024 02:06
ankur22
ankur22 previously approved these changes Nov 7, 2024
Copy link
Collaborator

@ankur22 ankur22 left a comment

Choose a reason for hiding this comment

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

It seems like a good idea, but I think we should add another test to ensure that all the fields are parsed/exported/merged as we expect.

browser/browser_context_options_test.go Show resolved Hide resolved
@inancgumus inancgumus force-pushed the remove/network-manager-extra-http-headers branch from b6806de to 14d6082 Compare November 7, 2024 14:45
@inancgumus inancgumus requested a review from a team as a code owner November 7, 2024 14:45
@inancgumus inancgumus requested review from mstoykov and oleiade and removed request for a team November 7, 2024 14:45
Base automatically changed from remove/network-manager-extra-http-headers to main November 7, 2024 14:45
@inancgumus inancgumus dismissed ankur22’s stale review November 7, 2024 14:45

The base branch was changed.

NewBrowserContextOptions no longer makes sense.
DefaultBrowserContextOptions also reads better when we look at the code.
We can easily tell we're working with the default options, not just a
new BrowserContextOptions value.
@inancgumus inancgumus force-pushed the refactor/parse-browser-options branch from e132b27 to 96a3172 Compare November 7, 2024 14:45
@inancgumus inancgumus merged commit 6e8c206 into main Nov 7, 2024
21 of 22 checks passed
@inancgumus inancgumus deleted the refactor/parse-browser-options branch November 7, 2024 14:45
@inancgumus inancgumus mentioned this pull request Nov 7, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants