1440491 - There are [persist] attributes on three elements in browser.xul that don't have IDs

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Brian Grinstead [:bgrins]]

I was looking at a profile's xulstore.json file and saw this inside it:

"chrome://browser/content/browser.xul": {
  [snip]

  "": {
     "width": "54"
   }
}

Went to figure out why and found these two <hbox.titlebar-placeholder> elements: 
- https://searchfox.org/mozilla-central/rev/bd05e3853c6e982e2a35c1cc404b987b2bc914d6/browser/base/content/browser.xul#623 - https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#705

The XULStore is keyed on ID so setting [persist] on an element without an ID seems invalid. I think we can remove the attribute from these two elements.

Comment 1

[Brian Grinstead [:bgrins]]

There's actually three, the two above and https://searchfox.org/mozilla-central/source/browser/base/content/browser.xul#697

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1440491 - Remove [persist] attribute from elements without IDs in browser.xul;

The XULStore is keyed on URL first, then element ID second. Having an empty ID
leads to bad data in xulstore.json.

Review commit: https://reviewboard.mozilla.org/r/222548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222548/

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8953261 [details]
Bug 1440491 - Remove [persist] attribute from elements without IDs in browser.xul;

Gijs, is there any reason not to do this? AFAICT having these here will just end up writing an empty string into XULStore for the ID (https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/dom/xul/XULDocument.cpp#1173)

Comment 4

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Comment on attachment 8953261 [details]
> Bug 1440491 - Remove [persist] attribute from elements without IDs in
> browser.xul;
> 
> Gijs, is there any reason not to do this? AFAICT having these here will just
> end up writing an empty string into XULStore for the ID
> (https://searchfox.org/mozilla-central/rev/
> 9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/dom/xul/XULDocument.cpp#1173)

Looks like this regressed because of bug 1390025 removing the ID from these. Dão, do you know if keeping these persisted gains us anything?

Comment 5

[:Gijs (he/him)]

Comment on attachment 8953261 [details]
Bug 1440491 - Remove [persist] attribute from elements without IDs in browser.xul;

Going to clear review while waiting for Dão.

Comment 6

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Comment on attachment 8953261 [details]
> > Bug 1440491 - Remove [persist] attribute from elements without IDs in
> > browser.xul;
> > 
> > Gijs, is there any reason not to do this? AFAICT having these here will just
> > end up writing an empty string into XULStore for the ID
> > (https://searchfox.org/mozilla-central/rev/
> > 9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/dom/xul/XULDocument.cpp#1173)
> 
> Looks like this regressed because of bug 1390025 removing the ID from these.
> Dão, do you know if keeping these persisted gains us anything?

https://hg.mozilla.org/mozilla-central/rev/738845c4c146fdb65dd3d18413b41425bf570f31#l1.10 made it ineffective from another angle so I guess we can remove this.

Comment 7

[:Gijs (he/him)]

Comment on attachment 8953261 [details]
Bug 1440491 - Remove [persist] attribute from elements without IDs in browser.xul;

https://reviewboard.mozilla.org/r/222548/#review228598

Thanks, Dão. Yeah, let's just remove this then.

Comment 8

[:Gijs (he/him)]

How bad is the bad data? Is it worth cleaning up in a migration or something? I don't think so, but just making sure we've considered that...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d49f13abd54a
Remove [persist] attribute from elements without IDs in browser.xul; r=Gijs

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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