Code review comment for lp:~system-apps-team/webbrowser-app/multiple-windows

Revision history for this message
Andrew Hayzen (ahayzen) wrote :

Look good so far, two inline comments.
L1609 1) Does this case need a window.requestActivate() like the others? Eg in onOpenLinkInWindowRequested below
L1906 2) is window always valid? Or should this have a check around it?

There are a few TODO, FIXME and XXX's added are these all being fixed in future branches?
L1469 // FIXME: do this asynchronously
L1715 // TODO: do we want to save/restore window positions too (https://launchpad.net/bugs/1312892)?
L1888 // XXX: Ideally, we would use the Window.window
       // attached property, but it is new in Qt 5.7.

I don't see any tests specifically for multiple window session restore, do you think there should be some or are the general session storage tests OK?

review: Needs Information

« Back to merge proposal