Closed Bug 970291 Opened 6 years ago Closed 6 years ago

Remove serializeNodeAsJSONToOutputStream

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla31

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, Whiteboard: p=1 s=it-31c-30a-29b.2 [qa-])

Attachments

(1 file)

we should kill this API in favor of a better way to get an object representing a single bookmark.
Depends on: 987682
if bug 987682 removes the Sync usage, then this bug should just handle test_leftpane_corruption_handling.js
Assignee: nobody → mak77
Status: NEW → ASSIGNED
the reason for the removal is that this API causes jank and it's also slightly broken...
Keywords: perf
Blocks: fxdesktopbacklog
No longer blocks: fxdesktoptriage
Whiteboard: p=0
Attached patch patch v1Splinter Review
while I was on the train I found some time to do this.

I was right, this API sucks, indeed by removing it I found a real bug... now fixed and tested :)
Attachment #8404859 - Flags: review?(mano)
this would be a p=1, likely.
No longer blocks: fxdesktopbacklog
Flags: firefox-backlog+
Whiteboard: p=0 → p=1 s=it-31c-30a-29b.2 [qa?]
Marco, what implications does this have in terms of potential for Places and/or Add-ons regressions?
Flags: needinfo?(mak77)
Comment on attachment 8404859 [details] [diff] [review]
patch v1

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

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +826,5 @@
>          let parentId = bs.getFolderIdForItem(query.itemId);
>          if (items.indexOf(parentId) == -1 && parentId != leftPaneRoot) {
>            // The parent is not part of the left pane, bail out and create a new
>            // left pane root.
> +          corrupt = true; 

Nit: trailing white space ;)
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #5)
> Marco, what implications does this have in terms of potential for Places
> and/or Add-ons regressions?

Places doesn't use this API from some time, mostly cause it's broken. And I cannot find any add-ons using it. I just found a couple using an old version of this from PlacesUtils, that are broken already. I'd call this a qa- since it was only used by an automated test. The small fix I found thanks to this is tested by that same test (now properly).
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [:mak] from comment #7)
> Places doesn't use this API from some time, mostly cause it's broken. And I
> cannot find any add-ons using it. I just found a couple using an old version
> of this from PlacesUtils, that are broken already. I'd call this a qa- since
> it was only used by an automated test. The small fix I found thanks to this
> is tested by that same test (now properly).

Thanks!
Whiteboard: p=1 s=it-31c-30a-29b.2 [qa?] → p=1 s=it-31c-30a-29b.2 [qa-]
Whiteboard: p=1 s=it-31c-30a-29b.2 [qa-] → p=1 s=it-31c-30a-29b.3 [qa-]
https://hg.mozilla.org/integration/fx-team/rev/9e92a6a293fa
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/9e92a6a293fa
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Whiteboard: p=1 s=it-31c-30a-29b.3 [qa-] → p=1 s=it-31c-30a-29b.2 [qa-]
You need to log in before you can comment on or make changes to this bug.