Code review comment for lp:~abreu-alexandre/webbrowser-app/front-camera-as-default-capture

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> In src/app/BrowserView.qml, you want i18n.dtr("oxide-qt", "Front") to pick up
> the translated string from oxide.

right, done,

> Is there really a need for a currentWebviewContext property? Can’t you simply
> refer to SharedWebContext.sharedContext everywhere instead? What’s the point
> of having a null currentWebviewContext in WebApp.qml if there is no webview
> yet? The context being a singleton, its lifetime should be independent from
> that of the webviews using it, no?

no, the container does not use the shared context, and the webview is delay loaded,

> In BrowserView.qml, __internal doesn’t need to be a property, it can simply be
> a child of the FocusScope.

right, done,

> Can 'cameraIdVideoCaptureDefault' be renamed 'defaultVideoCaptureDeviceId'?
> Can 'cameraPositionVideoCaptureDefault' be renamed
> 'defaultVideoCaptureDevicePosition'?

yes, done

> In BrowserView.qml, instead of connecting to 'changed' signals in an
> imperative manner, could you do it in a declarative way (using a Connections
> element)?

thank you yes, it makes things cleaner,

« Back to merge proposal