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

[jamesbraza]

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

[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.

[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:
    ...

[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.