912855 - Can't remove check-mark on "Load this bookmark in the sidebar" in Firefox 25 and 26

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[dickvl]

It is possible to set a check-mark on "Load this bookmark in the sidebar" in the Library, but it isn't possible to remove this check-mark.

A check of places.sqlite with the SQLite Manager shows that a bookmarkProperties/loadInSidebar entry in moz_items_annos is created with a value of 1.

When I try to remove the check-mark then the value is changed to 0 and the check-box is still checked (pressing the space bar gives a short off/on blink).

This still works correctly in the current Firefox 24 beta version, but doesn't work in Aurora (25a2) and Nightly (26a1) builds.

Comment 1

[Alice0775 White]

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5197eef777dd
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 ID:20130803010030
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/714a27789e9e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:25.0) Gecko/20100101 Firefox/25.0 ID:20130803040718
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5197eef777dd&tochange=714a27789e9e
Regressed by 
714a27789e9e	Asaf Romano — Bug 895839 - Remove support for binary annotations. r=mak. sr=gavin.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes, this is an unexpected regression

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 802411 [details] [diff] [review]
patch

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

::: browser/components/places/content/editBookmarkOverlay.js
@@ +589,5 @@
>  
>    onLoadInSidebarCheckboxCommand:
>    function EIO_onLoadInSidebarCheckboxCommand() {
>      var loadInSidebarChecked = this._element("loadInSidebarCheckbox").checked;
> +    var annoObj = { name   : PlacesUIUtils.LOAD_IN_SIDEBAR_ANNO };

please fix indentation before ":", now that there's only one property

@@ +590,5 @@
>    onLoadInSidebarCheckboxCommand:
>    function EIO_onLoadInSidebarCheckboxCommand() {
>      var loadInSidebarChecked = this._element("loadInSidebarCheckbox").checked;
> +    var annoObj = { name   : PlacesUIUtils.LOAD_IN_SIDEBAR_ANNO };
> +    if (this._element("loadInSidebarCheckbox").checked)

you already have loadInSidebarChecked... maybe you wanted to remove it?

@@ +592,5 @@
>      var loadInSidebarChecked = this._element("loadInSidebarCheckbox").checked;
> +    var annoObj = { name   : PlacesUIUtils.LOAD_IN_SIDEBAR_ANNO };
> +    if (this._element("loadInSidebarCheckbox").checked)
> +      annoObj.value = true;
> +    var txn = new PlacesSetItemAnnotationTransaction(this._itemId, annoObj);

while here, s/var/let/

Comment 5

[Alex Keybl [:akeybl]]

Ready to land?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2

updated patch addressing review comments

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 814900 [details] [diff] [review]
patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 895839
User impact if declined: Load in sidebar options can't be changed in the ui
Testing completed (on m-c, etc.): local testing
Risk to taking this patch (and alternatives if risky): low, trivial patch
String or IDL/UUID changes made by this patch: none

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b088976006ef

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de66802f05aa
https://hg.mozilla.org/releases/mozilla-beta/rev/883512212ee0

Comment 11

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Windows NT 6.1; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (X11; Linux i686; rv:25.0) Gecko/20100101 Firefox/25.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0

Verified as fixed on Firefox 25 beta 7 (Build ID: 20131010180222), the "Load this bookmark in the sidebar" check-mark can be unchecked.

Comment 12

[Petruta Horea [:phorea], Desktop QA]

Verified as fixed on latest Aurora (20131016004005):

Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:26.0) Gecko/20100101 Firefox/26.0
Mozilla/5.0 (Windows NT 6.2; rv:26.0) Gecko/20100101 Firefox/26.0

Comment 13

[Ada [:adalucinet]]

Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:27.0) Gecko/20100101 Firefox/27.0
Mozilla/5.0 (Windows NT 6.1; rv:27.0) Gecko/20100101 Firefox/27.0

Verified as fixed on latest Aurora 27.0a2 (Build ID: 20131104004000) and latest Nightly 28.0a1 (Build ID: 20131104044747).