957588 - Bookmark navbar button is checked for non-metro bookmarks

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Bookmarks, defect, P2)

Description

[Jim Mathies [:jimm]]

If you use the navbar autocomplete to load up a page that you have bookmarked in desktop bookmarks, the bookmark button will be checked even though the site is not in the metro bookmarks on about:start.

Comment 1

[Marco Mucci [:MarcoM]]

Hey Rodrigo, can you provide a point value.

Comment 2

[Rodrigo Silveira [:rsilveira] [:rodms]]

sure, p=1

Comment 3

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v1

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8360765 [details] [diff] [review]
Patch v1

Drive-by comment: We should also change what happens in Browser.unstarSite -> Bookmarks.removeForURI.  Instead of removing all matching bookmarks from all bookmark folders, we should only remove matching bookmarks in the Metro folder.

Comment 5

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 8360765 [details] [diff] [review]
Patch v1

Clearing r flag, will r? after addressing mbrubeck's comments.

Comment 6

[Rodrigo Silveira [:rsilveira] [:rodms]]

Attachment: Patch v2

Feedback comments addressed.

Comment 7

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8361240 [details] [diff] [review]
Patch v2

Review of attachment 8361240 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works well. We have no test coverage for this though, can we file a follow-up to add tests for the expected behavior - that non-metro bookmarks are not considered "bookmarked" from the UI's point of view. That'll make the intention clear and help us keep on top of regressions. We also need tests for UI behavior when adding and removing bookmarks - the appbar star button should be in the right state, and the bookmark should show up and disappear as appropriate in the startUI bookmarks grid.

::: browser/metro/base/content/bookmarks.js
@@ +67,2 @@
>        if (callback)
> +        callback();

Nit: Is there a reason for this function to explicitly not return the URI and item ids? Seems like it doesn't hurt - the callback can just ignore them?

Comment 8

[Matt Brubeck (:mbrubeck)]

Comment on attachment 8361240 [details] [diff] [review]
Patch v2

Review of attachment 8361240 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/metro/base/content/bookmarks.js
@@ +67,2 @@
>        if (callback)
> +        callback();

The caller already has the URI, and the relevant IDs are for deleted bookmarks and so they are no longer valid... so I think it's fair to remove this.

Comment 9

[Rodrigo Silveira [:rsilveira] [:rodms]]

After irc discussion leaving callback parameters empty. Filed bug 961159 for test coverage. Thanks!

https://hg.mozilla.org/integration/fx-team/rev/9f349f36c16e

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9f349f36c16e

Comment 11

[Marco Mucci [:MarcoM]]

Temporarily reopening to add to Iteration #23 (Bug 961497)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8a39d0bb4c1f

Comment 13

[u279076]

Kamil, please verify this is fixed in the latest Nightly and Aurora builds.

Comment 14

[Manuela Muntean [Away]]

Verified as fixed, for iteration #23, with both latest Nightly and Aurora on Win 8.1 64-bit.

Comment 15

[Kamil Jozwiak [:kjozwiak]]

Adding some other test cases to consider when going through this issue during the next iteration cycle:

- Ensure that websites being opened via "Top Sites" don't appear as bookmarked on fxmetro if they have been bookmarked under fxdesktop
- Ensure that websites being opened via "Recent History" don't appear as bookmarked on fxmetro if they have been bookmarked under fxdesktop
- Ensure that bookmarking a websites under fxmetro that is already bookmarked under fxdesktop work without any issues
- Ensure that you can bookmark/unbookmark websites and the bookmark icon under the Navigation App Bar is changed accordingly
- Ensured that copy/pasting a URL in the Navigation App Bar that is already bookmarked under desktop doesn't appear bookmarked under metrofx
- Ensured that fxdesktop bookmarks are not removed if you bookmark/unbook mark from fxmetro and switch over to the desktop environment