Code review comment for lp://staging/~artmello/webbrowser-app/webbrowser-app-bookmark_folders

Revision history for this message
Olivier Tilloy (osomon) wrote :

There are two remaining places in Browser.qml where a bookmark is created without the popover being displayed: HUD actions (which is defunct code, but until we remove it it should be kept up-to-date) and contextual actions.

The BookmarkOptions component should gain an additional 'url' property, whose value would be set to browser.currentWebview.url at construction time. This would ensure that the url doesn’t change while it’s shown. If the webview’s current URL changes while the popover is shown, the user can potentially try to unbookmark a different URL than the one that was bookmarked.

It’s wrong to skip test_set_bookmark_title. It’s not just the test that fails, it’s the actual functionality that’s broken. So we can’t land this branch until the UITK fix lands. By the way, have you tested the UITK branch (the fix appears to be merged in staging) to confirm that it fixes our issue?

If I press Ctrl+D to create a bookmark, then Ctrl+D again to remove it, the BookmarkOptions popover doesn’t disappear. If I then press Ctrl+D again to re-create the bookmark, I end up with two instances of the same popover.

review: Needs Fixing

« Back to merge proposal