Closed Bug 957588 Opened 6 years ago Closed 6 years ago

Bookmark navbar button is checked for non-metro bookmarks

Categories

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

x86_64
Windows 8.1
defect

Tracking

(firefox28 verified, firefox29 verified)

VERIFIED FIXED
Firefox 29
Tracking Status
firefox28 --- verified
firefox29 --- verified

People

(Reporter: jimm, Assigned: rsilveira)

References

Details

(Whiteboard: [release28] [defect] p=1)

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [triage] → [triage] [defect] p=0
Whiteboard: [triage] [defect] p=0 → [release28] [defect] p=0
Assignee: nobody → rsilveira
Hey Rodrigo, can you provide a point value.
Flags: needinfo?(rsilveira)
sure, p=1
Flags: needinfo?(rsilveira)
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #8360765 - Flags: review?(sfoster)
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.
Attachment #8360765 - Flags: feedback-
Comment on attachment 8360765 [details] [diff] [review]
Patch v1

Clearing r flag, will r? after addressing mbrubeck's comments.
Attachment #8360765 - Flags: review?(sfoster)
Blocks: metrov1it22
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] [defect] p=0 → [release28] [defect] p=1
Attached patch Patch v2Splinter Review
Feedback comments addressed.
Attachment #8360765 - Attachment is obsolete: true
Attachment #8361240 - Flags: review?(sfoster)
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?
Attachment #8361240 - Flags: review?(sfoster) → review+
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.
Blocks: 961159
After irc discussion leaving callback parameters empty. Filed bug 961159 for test coverage. Thanks!

https://hg.mozilla.org/integration/fx-team/rev/9f349f36c16e
Blocks: metrov1backlog
No longer blocks: metrov1it22
https://hg.mozilla.org/mozilla-central/rev/9f349f36c16e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Temporarily reopening to add to Iteration #23 (Bug 961497)
Blocks: metrov1it23
No longer blocks: metrov1backlog
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [release28] [defect] p=1 → [release28] [defect] p=1 [approval-mozilla-aurora=metro-only]
https://hg.mozilla.org/releases/mozilla-aurora/rev/8a39d0bb4c1f
Keywords: checkin-needed
Whiteboard: [release28] [defect] p=1 [approval-mozilla-aurora=metro-only] → [release28] [defect] p=1
Kamil, please verify this is fixed in the latest Nightly and Aurora builds.
Flags: needinfo?(kamiljoz)
Verified as fixed, for iteration #23, with both latest Nightly and Aurora on Win 8.1 64-bit.
Status: RESOLVED → VERIFIED
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
Flags: needinfo?(kamiljoz)
You need to log in before you can comment on or make changes to this bug.