Following the landing of bug 870100 the pref setting no longer is affective in blocking thumbnails from being displayed. STR: add hidden pref browser.pagethumbnails.capturing_disabled Set to 'true' Note that thumbnails are still refreshed and displayed following the landing of bug 870100
I'm not really sure why we have both browser.pagethumbnails.capturing_disabled and browser.pageThumbs.enabled. Seems like the former prevents some work that the latter doesn't (adding a bunch of observers in gBrowserThumbnails.init), but they're otherwise mostly the same. Seems like we should remove one or the other?
Indeed - added the apparently hidden pref: browser.pageThumbs.enabled set to 'false' and no thumbs are displayed pref: browser.pagethumbnails.capturing_disabled is set currently to: 'true' So seems that one or the other could control everything for sure.. A quick test of setting browser.pagethumbnails.capturing_disabled to 'false' does not re-enable display of thumbs, so appears that perhaps the new hidden pref: browser.pageThumbs.enabled set to 'false' will control everything ? Further testing of just toggling browser.pageThumbs.enabled will enable/disable thumbs display in a NewTab. No coder here, but seems that the older pref: browser.pagethumbnails.capturing_disabled could indeed be removed ?
Created attachment 784803 [details] [diff] [review] 897811-remove-browser.pagethumbnails.capturing_disabled.preference.patch (In reply to :Gavin Sharp (use email@example.com for email) from comment #1) > I'm not really sure why we have both > browser.pagethumbnails.capturing_disabled and browser.pageThumbs.enabled. > Seems like the former prevents some work that the latter doesn't (adding a > bunch of observers in gBrowserThumbnails.init) I think we should remove that one that "prevents some work" for 2 reasons: * It prevents that work, but also means that Fx needs to be restarted after the preference is toggled. It would be possible to fix that, but ... * It's a hidden preference, so a tiny minority of users will ever set it. It's not worth avoiding this extra work for such users, especially if it means even more code so the pref can be toggled and have it honored without a restart. This patch removes the browser.pagethumbnails.capturing_disabled preference (it only appears in one place) and leaves the browser.pageThumbs.enabled preference alone.
hrm - on the downside, a quick google for that pref name shows quite a number of hits. We can obviously update MDN - maybe doing that and ensuring the old pref name is still there means users are still likely to find it via google?
Comment on attachment 784803 [details] [diff] [review] 897811-remove-browser.pagethumbnails.capturing_disabled.preference.patch How complicated would it be to go the other way and keep browser.pagethumbnails.capturing_disabled instead?
Created attachment 785292 [details] [diff] [review] 897811-remove-browser.pageThumbs.enabled-preference.patch (In reply to :Gavin Sharp (use firstname.lastname@example.org for email) from comment #5) > Comment on attachment 784803 [details] [diff] [review] > 897811-remove-browser.pagethumbnails.capturing_disabled.preference.patch > > How complicated would it be to go the other way and keep > browser.pagethumbnails.capturing_disabled instead? Not hard - I think we'd just still remove the check in the patch I uploaded (ie, in the init() function) and rename the other few occurrences of the pref to this. OTOH though, looking at the google results, it is almost as though browser.pagethumbnails.capturing_disabled is the pref to mean "disable the capture of sensitive pages" - which we have since fixed - so in some ways we are doing the users a favour as we will be automatically turning on thumbnails now they are "safe" ;) But yeah - that is a stretch :) So here is one that keeps only browser.pagethumbnails.capturing_disabled. Take your pick :) Try for this one at https://tbpl.mozilla.org/?tree=Try&rev=9900a351c38c
Created attachment 785464 [details] [diff] [review] 897811-remove-browser.pageThumbs.enabled-preference.patch Sorry about the previous rushed and obviously wrong patch. Try for this one at https://tbpl.mozilla.org/?tree=Try&rev=a308683f023b
Comment on attachment 785464 [details] [diff] [review] 897811-remove-browser.pageThumbs.enabled-preference.patch I would really like to know why those tests need to disable the service too. Maybe lump that in with bug 896912?
https://hg.mozilla.org/integration/mozilla-inbound/rev/b487df56bcfe I think we should uplift this to 25 so people who have already set the browser.pagethumbnails.capturing_disabled continue to work as expected in that version.
Comment on attachment 785464 [details] [diff] [review] 897811-remove-browser.pageThumbs.enabled-preference.patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 870100 User impact if declined: Users who previously set a hidden preference to disable thumbnails will see thumbnails start to appear. Testing completed (on m-c, etc.): Only on inbound at the moment, but fairly low risk and easy to manage. Risk to taking this patch (and alternatives if risky): Low String or IDL/UUID changes made by this patch: None
So, I'm confused.. which pref was removed ? browser.pagethumbnails.capturing_disabled browser.pageThumbs.enabled It appeared from a quick look that the top pref was 'removed' in favor in the 2nd pref Sorry, didn't read the actual patch. I 'reset' the top pref 'browser.pagethumnails.capturing_disabled and restarted and the pref was gone, as I suspected would happen with the removal of the pref. However, with browser.pagthumbe.enabled set to 'false' I'm now seeing thumbs again, and now readding the browser.pagethumbnails.capturing_disabled and setting to 'true' I'm still getting thumbs even after repeated deletions of the J:\Users\username\AppData\Local\Mozilla\Firefox\Profiles\default.xxxxxxx thumbnails folder. I can't seem to turn them off at all now...
both pref's retained and 'hidden' ? I have: browser.pageThumbs.enabled;false browser.pagethumbnails.capturing_disabled;true browser.pagethumbnails.storage_version;3 and after deleting the thumbnails folder - I still see thumbs...
browser.pageThumbs.enabled was removed and is now ignored by Firefox. browser.pagethumbnails.capturing_disabled is the pref you can use to disable thumbnail collection in general.
Thanks Gavin removed (reset - browser.pagethumbs.enabled) deleted cache folder deleted thumbnails folder Both with Firefox Closed - restarted - and still getting thumbs. :(
Re-opening, pref: browser.pagethumbnails.capturing_disabled no longer blocks thumbnails with this patch landing. Tested on latest m-c hourly based on cset: https://hg.mozilla.org/mozilla-central/rev/79b5c74ef97b I deleted cache and thumbnails folders several times and thumbs keep on coming back. Removed via add/remove and reinstalled from a fresh download of the build with above cset, and still seeing thumbs.
I'm now totally confused, went back to a 7/24/13 build prior to the landing of but 870100, and with the pref browser.pagethumbnails.capturing_disabled = True I'm still getting thumbs...where before I certain that was not case.
Created attachment 787216 [details] [diff] [review] bug-897811-followup.patch Pretty spectacular screwup by me. The new pref's meaning is negated: "browser.pageThumbs.enabled" -> "browser.pagethumbnails.capturing_disabled" - but the values only negated in one place - they should all have been. At least the default case (ie, no pref existing) is OK - so only when there was a value for browser.pagethumbnails.capturing_disabled did things go wrong. [Approval Request Comment] Bug caused by (feature/regressing bug #): This bug User impact if declined: Thumbnails may appear when they shouldn't, and may not when they should. Testing completed (on m-c, etc.): Risk to taking this patch (and alternatives if risky): Lower than taking the other one was ;) String or IDL/UUID changes made by this patch: None
Comment on attachment 787216 [details] [diff] [review] bug-897811-followup.patch doh!
Thanks for catching this one, Jim!
(In reply to :Gavin Sharp (use email@example.com for email) from comment #21) > Thanks for catching this one, Jim! Your Welcome. Thanks for the quick turn-around.
Backed out of inbound for b-c failures. https://tbpl.mozilla.org/php/getParsedLog.php?id=26290181&tree=Mozilla-Inbound is a typical one: https://hg.mozilla.org/integration/mozilla-inbound/rev/574c96f701d9
Created attachment 787344 [details] [diff] [review] ab9642207875 I neglected to fix head.js in the thumbnails dir as I mistakenly thought that was the only thing I didn't screw up last time. This patch fixes that too. https://tbpl.mozilla.org/?tree=Try&rev=6ecfcf7f9e58
Backed out on Aurora https://hg.mozilla.org/releases/mozilla-aurora/rev/3318c44609da
https://hg.mozilla.org/integration/fx-team/rev/0f21776e01b9 I won't ask for Aurora approval until it sticks this time ;)
Comment on attachment 787344 [details] [diff] [review] ab9642207875 [Approval Request Comment] Bug caused by (feature/regressing bug #): Where do I start with this. tl;dr, this bug. User impact if declined: Thumbnails not disabled when user sets a pref that should disable them. Testing completed (on m-c, etc.): Finally stuck on m-c. Risk to taking this patch (and alternatives if risky): Low String or IDL/UUID changes made by this patch: None