[BUG] `chromium_sandbox` doesn't default to `False`

[jamesbraza]

System info

• Playwright Version: [v1.XX] 1.41.1
• Operating System: [All, Windows 11, Ubuntu 20, macOS 13.2, etc.] macOS Ventura 13.5.2
• Browser: [All, Chromium, Firefox, WebKit] Chromium
• Other info: n/a

Source code

https://github.com/microsoft/playwright-python/blob/v1.41.1/playwright/async_api/_generated.py#L14804-L14805

class BrowserType(AsyncBase):
    ...

    async def launch(
        self,
        ...,
        chromium_sandbox: typing.Optional[bool] = None,
        ...
    ) -> "Browser":
        """BrowserType.launch
        ...
        chromium_sandbox : Union[bool, None]
            Enable Chromium sandboxing. Defaults to `false`.
        ...
        """

The chromium_sandbox docstring says it defaults to false, but it actually defaults to None. Also, it doesn't specify if None is equivalent to false.

Can we update the docstring description for chromium_sandbox?

• Specify what None means in this context
• Update docstring to say default is None?

Imo it would be more intuitive if the default were False, not None

[danphenderson]

I am digging into this issue some more to try and figure out if there is a difference between None and False.

It appears that chromium_sandbox will be passed to the launch and lanuch_persistent_context methods of BrowserType as the chromiumSandbox parameter, which is declared as a bool but defaults to None.

The more I look, I see None being declared as the default when the parameter/attribute isn't declared as optional, a different but related issue to the one at hand (e.g. all the methods in BrowserType).

@jamesbraza your report appears to just be the tip of the iceberg.

[mxschmitt]

All parameters of any Playwright API have None as a default. This is more an implementation detail which we internally use to decide if a user has passed a parameter over or not. Internally in our code we check if it was provided or not (!== null/undefined and then use it / use our default).

None means for all our methods that the default will be used / treated like nothing was specified.

We can maybe get rid of all the = None everywhere.

[danphenderson]

Using None that way is fine, but the parameter type hints should be either Union[SomeType, None], Optional[SomeType], or SomeType | None (preferred, but may cause some issues in Python 3.9)

[jamesbraza]

I am also okay with defaulting to None, I just request the docstring state:

1. The correct default of None
2. That None is equivalent to False, within the implementation

So it's changing from:

Enable Chromium sandboxing. Defaults to false.

To:

Enable Chromium sandboxing. Defaults to None, which is equivalent to False.

[danphenderson]

Great, I agree!

Regarding the other issue at hand, observe the Browser method:

class Browser(ChannelOwner):
    ...
    async def new_context(
        self,
        viewport: ViewportSize = None,
        screen: ViewportSize = None,
        noViewport: bool = None,
        ignoreHTTPSErrors: bool = None,
        javaScriptEnabled: bool = None,
        bypassCSP: bool = None,
        userAgent: str = None,
        locale: str = None,
        timezoneId: str = None,
        geolocation: Geolocation = None,
        permissions: Sequence[str] = None,
        extraHTTPHeaders: Dict[str, str] = None,
        offline: bool = None,
        httpCredentials: HttpCredentials = None,
        deviceScaleFactor: float = None,
        isMobile: bool = None,
        hasTouch: bool = None,
        colorScheme: ColorScheme = None,
        reducedMotion: ReducedMotion = None,
        forcedColors: ForcedColors = None,
        acceptDownloads: bool = None,
        defaultBrowserType: str = None,
        proxy: ProxySettings = None,
        recordHarPath: Union[Path, str] = None,
        recordHarOmitContent: bool = None,
        recordVideoDir: Union[Path, str] = None,
        recordVideoSize: ViewportSize = None,
        storageState: Union[StorageState, str, Path] = None,
        baseURL: str = None,
        strictSelectors: bool = None,
        serviceWorkers: ServiceWorkersPolicy = None,
        recordHarUrlFilter: Union[Pattern[str], str] = None,
        recordHarMode: HarMode = None,
        recordHarContent: HarContentPolicy = None,
    ) -> BrowserContext:
    ...

The type-hints for the new_context parameters are incorrect, namely, None is being used as the default despite the parameter not being declared as optional. This occurs in a lot of other areas in the code base. I realize Python typing is very optional, but I figure that if we use type hints we might as well use them properly.

An update that is compliant with Python 3.9- may look like this:

class Browser(ChannelOwner):
    ...
    async def new_context(
        self,
        viewport: Optional[ViewportSize] = None,
        screen: Optional[ViewportSize] = None,
        noViewport: Optional[bool] = None,
        ignoreHTTPSErrors: Optional[bool] = None,
        javaScriptEnabled: Optional[bool] = None,
        bypassCSP: Optional[bool] = None,
        userAgent: Optional[str] = None,
        locale: Optional[str] = None,
        timezoneId: Optional[str] = None,
        geolocation: Optional[Geolocation] = None,
        permissions: Optional[Sequence[str]] = None,
        extraHTTPHeaders: Optional[Dict[str, str]] = None,
        offline: Optional[bool] = None,
        httpCredentials: Optional[HttpCredentials] = None,
        deviceScaleFactor: Optional[float] = None,
        isMobile: Optional[bool] = None,
        hasTouch: Optional[bool] = None,
        colorScheme: Optional[ColorScheme] = None,
        reducedMotion: Optional[ReducedMotion] = None,
        forcedColors: Optional[ForcedColors] = None,
        acceptDownloads: Optional[bool] = None,
        defaultBrowserType: Optional[str] = None,
        proxy: Optional[ProxySettings] = None,
        recordHarPath: Union[Path, str, None] = None,
        recordHarOmitContent: Optional[bool] = None,
        recordVideoDir: Union[Path, str, None] = None,
        recordVideoSize: Optional[ViewportSize] = None,
        storageState: Union[StorageState, str, Path, None] = None,
        baseURL: Optional[str] = None,
        strictSelectors: Optional[bool] = None,
        serviceWorkers: Optional[ServiceWorkersPolicy] = None,
        recordHarUrlFilter: Union[Pattern[str], str, None] = None,
        recordHarMode: Optional[HarMode] = None,
        recordHarContent: Optional[HarContentPolicy] = None,
    ) -> BrowserContext:
    ...