Bugzilla

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
> Actually... I think the fix for bug 1795901 is incomplete. We should've also removed the persisted attributes in a migration. Otherwise old persistent attributes might still apply. Or at least I don't see what wouldn't make them apply, [this code](https://searchfox.org/mozilla-central/rev/b6d0ad6ba24b3d075e357e52160cd8fc7f911ba9/dom/xul/XULPersist.cpp#273-293) doesn't check for `ShouldPersistAttribute`.

I thought the code would not persist attributes if they are not in persist="", not sure why it didn't happen in my testing, nor in QA.... I wonder if we ended up testing bookmarkProperties2 and not the remaining one?
That sounds like a bug in xulpersist... wdyt? Otherwise we'd need a ui migration, but I'd prefer avoiding the same issue in the future.
ShouldPersistAttribute is likely not the right thing to check though, we should rather get a list from the persist attributes (like [here](https://searchfox.org/mozilla-central/rev/b6d0ad6ba24b3d075e357e52160cd8fc7f911ba9/dom/xul/XULPersist.cpp#79-82), and apply the attr only if it appears in the list. That requires some additional string find though, would we be ok with paying that price?

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)
> Actually... I think the fix for bug 1795901 is incomplete. We should've also removed the persisted attributes in a migration. Otherwise old persistent attributes might still apply. Or at least I don't see what wouldn't make them apply, [this code](https://searchfox.org/mozilla-central/rev/b6d0ad6ba24b3d075e357e52160cd8fc7f911ba9/dom/xul/XULPersist.cpp#273-293) doesn't check for `ShouldPersistAttribute`.

I thought the code would not persist attributes if they are not in persist="", not sure why it didn't happen in my testing, nor in QA.... I wonder if we ended up testing bookmarkProperties2 and not the remaining one?
That sounds like a bug in xulpersist... wdyt? Otherwise we'd need a ui migration, but I'd prefer avoiding the same issue in the future.
ShouldPersistAttribute is likely not the right thing to check though, we should rather get a list from the persist attributes (like [here](https://searchfox.org/mozilla-central/rev/b6d0ad6ba24b3d075e357e52160cd8fc7f911ba9/dom/xul/XULPersist.cpp#79-82)), and apply the attr only if it appears in the list. That requires some additional string find though, would we be ok with paying that price?