This is probably the silliest pref I've ever seen.

Note that SeaMonkey has UI for this. If you plan to remove it, might want to give them a heads-up first....

Note that now there is a img.srcset attribute/property, which makes "nsContentUtils::IsImageSrcSetDisabled" rather misleading, so that should be changed if we don't plan remove this

Giving Callek the heads-up in comment 1.

Comment on attachment 8685578 [details] [diff] [review] Remove dom.disable_image_src_set. v1 Not sure how I did that twice.

Comment on attachment 8685581 [details] [diff] [review] Remove dom.disable_image_src_set. v1 We should get this on aurora given bug We should get this on aurora given bug 1222280. The basic issue here is that we tightened down the security infrastructure in bug 1072150 in a way that will cause crashes if it's used incorrectly. The code checking this obsolete pref uses that infrastructure incorrectly, and so the most expedient thing to do here is to just remove the pref and the code that checks it.

passing off to ewong and philip chee who have been doing more UX work in SeaMonkey than me as of late. (thanks for the heads up in c#1)

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #0) > This is probably the silliest pref I've ever seen. This pref controls whether to allow images to be changed/animated, i.e. during mouse over. Should it be renamed to clarify its purpose? As it stands, disable_image_src_set, isn't quite self-explanatory. I had to look up SeaMonkey's pref UI to find out where it is.

The fundamental question here is whether you guys want to keep this preference or not. If you do, somebody needs to make it work correctly (i.e. comment 7). Otherwise we'll just rip it out of Gecko and you can delete the UI.

Note that fixing is not that hard in the short term: switch to LegacyIsCallerChromeOrNativeCode. A long-term fix would involve pushing the check up closer to the entry points, but that involves finding all the entry points. In practice, maybe just doing this for HTMLImageElement::SetSrc is good enough for practical purposes.

(In reply to Boris Zbarsky [:bz] from comment #11) > Note that fixing is not that hard in the short term: switch to > LegacyIsCallerChromeOrNativeCode. A long-term fix would involve pushing the > check up closer to the entry points, but that involves finding all the entry > points. In practice, maybe just doing this for HTMLImageElement::SetSrc is > good enough for practical purposes. I'm not really wild about that. LegacyIsCallerChromeOrNativeCode is supposed to go away (so that we can eventually remove it), and I've largely been making good on that promise by doing the legwork to fix the code properly in the crashes that have come up since then. I don't want to hold this case to a lower standard. The bigger issue here is that invoking "IsCallerChrome" in this method is somewhat nonsensical, and probably just a hacky stab at the desired semantics. Fixing this up properly would probably introduce a fair amount of complexity, and I'm not really willing to do that unless this pref is mission-critical for SeaMonkey (which it almost certainly isn't).

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > The fundamental question here is whether you guys want to keep this > preference or not. If you do, somebody needs to make it work correctly (i.e. > comment 7). And just to clarify here, I'm not convinced that SM preferring to keep the pref is sufficient justification for keeping it, since making it work right would introduce complexity.

(In reply to Bobby Holley (busy) from comment #7) > Comment on attachment 8685581 [details] [diff] [review] > Remove dom.disable_image_src_set. v1 > > We should get this on aurora given bug We should get this on aurora given > bug 1222280. > > The basic issue here is that we tightened down the security infrastructure > in bug 1072150 in a way that will cause crashes if it's used incorrectly. > The code checking this obsolete pref uses that infrastructure incorrectly, > and so the most expedient thing to do here is to just remove the pref and > the code that checks it. I don't have access to that bug, but just out of curiosity, is it possible to get the code checking use the infrastructure correctly?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > The fundamental question here is whether you guys want to keep this > preference or not. If you do, somebody needs to make it work correctly (i.e. > comment 7). Otherwise we'll just rip it out of Gecko and you can delete the > UI. Since this is a pref issue, I'm going to have to NI IanN. IanN, what should we do?

> is it possible to get the code checking use the infrastructure correctly? The basic issue is that IsCallerChrome should only be called on codepaths that are known to be called from script. So doing it inside SetAttr() is obviously wrong in that world. I still think just doing it in SetSrc would be fine.

I am waiting for the patch to be r+'d and the discussion about the impact of removing this pref to be completed before reviewing the aurora uplift.

Comment on attachment 8685581 [details] [diff] [review] Remove dom.disable_image_src_set. v1 Review of attachment 8685581 [details] [diff] [review]: ----------------------------------------------------------------- I waited two weeks. With no decision from the Seamonkey folks, I think we should proceed here.

(In reply to Edmund Wong (:ewong) from comment #15) > (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10) > > The fundamental question here is whether you guys want to keep this > > preference or not. If you do, somebody needs to make it work correctly (i.e. > > comment 7). Otherwise we'll just rip it out of Gecko and you can delete the > > UI. > > Since this is a pref issue, I'm going to have to NI IanN. > > IanN, what should we do? Just because there is pref controlling the behaviour does not mean that is a pref issue. Neil would know a lot more than me about what this pref controls.

FYI I filed Bug 1228385 to remove this from the SeaMonkey Preferences UI

Comment on attachment 8685581 [details] [diff] [review] Remove dom.disable_image_src_set. v1 Aurora44+

Bobby, do we need to add this to relnotes? I think so but I wanted to confirm with you.

Release Note Request (optional, but appreciated) [Why is this notable]: Removed dom.disable_image_src_set pref [Suggested wording]: Removed pref dom.disable_image_src_set [Links (documentation, blog post, etc)]: None

I don't think we need it in the relnotes. It was only user-visible in seamonkey.

(In reply to Bobby Holley (busy) from comment #27) > I don't think we need it in the relnotes. It was only user-visible in > seamonkey. Thanks. I will not add it to 44 relnotes if you think it's not needed. FWIW, I was able to see this pref in DevEd44 in about:config.

(In reply to Ritu Kothari (:ritu) from comment #29) > (In reply to Bobby Holley (busy) from comment #27) > > I don't think we need it in the relnotes. It was only user-visible in > > seamonkey. > > Thanks. I will not add it to 44 relnotes if you think it's not needed. FWIW, > I was able to see this pref in DevEd44 in about:config. Yeah, but I believe we don't generally consider about:config prefs to be "user-facing", unless we have a reason to believe that they're popular. SM had user-facing UI for this, whereas FF did not.

(In reply to Ian Neal from comment #21) > Just because there is pref controlling the behaviour does not mean that is a > pref issue. Neil would know a lot more than me about what this pref controls. It was probably just a means of disabling image rollovers. Nowadays most hover effects are written in CSS anyway so this preference has probably lost its significance.