970291 - Remove serializeNodeAsJSONToOutputStream

Status: VERIFIED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

we should kill this API in favor of a better way to get an object representing a single bookmark.

Comment 1

[Marco Bonardo [:mak]]

if bug 987682 removes the Sync usage, then this bug should just handle test_leftpane_corruption_handling.js

Comment 2

[Marco Bonardo [:mak]]

the reason for the removal is that this API causes jank and it's also slightly broken...

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1

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 :)

Comment 4

[Marco Bonardo [:mak]]

this would be a p=1, likely.

Comment 5

[u279076]

Marco, what implications does this have in terms of potential for Places and/or Add-ons regressions?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

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 ;)

Comment 7

[Marco Bonardo [:mak]]

(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).

Comment 8

[u279076]

(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!

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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