Closed Bug 1154504 Opened 9 years ago Closed 9 years ago

Reconsider exposing paint flashing to the user

Categories

(Firefox for Android Graveyard :: Settings and Preferences, defect)

All
Android
defect
Not set
normal

Tracking

(firefox37 affected, firefox38 affected, firefox39 affected, firefox40 fixed, fennec+)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox37 --- affected
firefox38 --- affected
firefox39 --- affected
firefox40 --- fixed
fennec + ---

People

(Reporter: kbrosnan, Assigned: Margaret)

References

Details

Attachments

(2 files, 1 obsolete file)

People seem to enable this and don't know what happened. I get a couple questions a month on SUMO and Input seems to show even more users that hit this.

https://input.mozilla.org/en-US/?q=colour&product=Firefox+for+Android&date_end=2015-04-14&date_start=2015-01-14

https://input.mozilla.org/en-US/?q=color&product=Firefox+for+Android&date_start=2015-01-14&selected=90d

https://www.google.com/?gws_rd=ssl#q=site:support.mozilla.org+android+paint+flashing

Is this feature valuable enough to annoy users?
Sigh... this is probably why Android makes you jump through some hoops to even get to the developer options. Stupid users!

Could we instead add more description text around the "Paint flashing" option? I feel like developers are an important user base, and it would be nice to continue to try to appeal to them.

Or maybe we should only ship this on beta?
Nick added a fairly simple tap-five-times for FxA/Sync debugging. We could do something similar here to hide our more dangerous options. Stick a version info entry in Settings and let 'em tap.
Component: General → Settings and Preferences
Summary: reconsider exposing paint debugging to the user → Reconsider exposing paint debugging to the user
We talked about this in triage, and we think we should just remove this.

If it's not already part of WebIDE, I think it would be most appropriate to have this be something developers can enable/disable remotely. jryans, do you know if we've thought about this at all?
Assignee: nobody → margaret.leibovic
tracking-fennec: ? → +
Flags: needinfo?(jryans)
(In reply to :Margaret Leibovic from comment #3)
> We talked about this in triage, and we think we should just remove this.
> 
> If it's not already part of WebIDE, I think it would be most appropriate to
> have this be something developers can enable/disable remotely. jryans, do
> you know if we've thought about this at all?

It is controllable from WebIDE via Device Preferences, but you'd have to know what pref name maps to this debugging feature.

Bug 1128988 (to be landed soonish) will allow the toolbox "command buttons" (like the paint debugging button) to function for remote devices as well.  So, you could connect via WebIDE, and click this button to enable it for Fx on Android.

I would guess that's probably enough (eventual) paths that it's fine to remove from Fennec UI, especially to avoid confusing people.
Flags: needinfo?(jryans)
See Also: → 1128988
antlam, based on the discussion in this bug, I want to remove "Paint flashing" from the "Developer tools" in settings page. However, that would leave us with only "Remote debugging" in the "Developer tools" page.

Should we remove the "Developer tools" page altogether and replace it with just the remote debugging pref? That doesn't feel like something that should be on the main settings screen, so maybe we should stick that somewhere else, but I can't think of any good place. Maybe it is fine to just leave it under "Developer tools". What do you think?

I don't even see this option in Chrome's settings, I wonder how remote debugging works there.
Flags: needinfo?(alam)
(In reply to :Margaret Leibovic from comment #5)
> antlam, based on the discussion in this bug, I want to remove "Paint
> flashing" from the "Developer tools" in settings page. However, that would
> leave us with only "Remote debugging" in the "Developer tools" page.
> 
> Should we remove the "Developer tools" page altogether and replace it with
> just the remote debugging pref? That doesn't feel like something that should
> be on the main settings screen, so maybe we should stick that somewhere
> else, but I can't think of any good place. Maybe it is fine to just leave it
> under "Developer tools". What do you think?
> 
> I don't even see this option in Chrome's settings, I wonder how remote
> debugging works there.

With Chrome for Android, I believe their remote debugging is always on if the Android OS level "USB Debugging" option is enabled.  There does not appear[1] to be an additional, in-app setting.

Firefox could take the same approach, if you wanted to.

[1]: https://developer.chrome.com/devtools/docs/remote-debugging
(In reply to J. Ryan Stinnett [:jryans] from comment #6)
> (In reply to :Margaret Leibovic from comment #5)
> > antlam, based on the discussion in this bug, I want to remove "Paint
> > flashing" from the "Developer tools" in settings page. However, that would
> > leave us with only "Remote debugging" in the "Developer tools" page.
> > 
> > Should we remove the "Developer tools" page altogether and replace it with
> > just the remote debugging pref? That doesn't feel like something that should
> > be on the main settings screen, so maybe we should stick that somewhere
> > else, but I can't think of any good place. Maybe it is fine to just leave it
> > under "Developer tools". What do you think?
> > 
> > I don't even see this option in Chrome's settings, I wonder how remote
> > debugging works there.
> 
> With Chrome for Android, I believe their remote debugging is always on if
> the Android OS level "USB Debugging" option is enabled.  There does not
> appear[1] to be an additional, in-app setting.
> 
> Firefox could take the same approach, if you wanted to.
> 
> [1]: https://developer.chrome.com/devtools/docs/remote-debugging

Oh, interesting! Eliminating the need to toggle this pref in Fennec sounds like a win to me. Then we could get rid of this "Developer tools" settings page altogether.

My one concern would be that in the past we had performance issues related to turning on remote debugging by default on Nightly (bug 979101), and I wouldn't want to regress performance for all users that have remote debugging enabled on the Android level. However, I would hope that any additional memory use would take effect when your phone is connected over USB, which is not really a real world use case for regular browser usage.
(In reply to :Margaret Leibovic from comment #7)
> My one concern would be that in the past we had performance issues related
> to turning on remote debugging by default on Nightly (bug 979101), and I
> wouldn't want to regress performance for all users that have remote
> debugging enabled on the Android level. However, I would hope that any
> additional memory use would take effect when your phone is connected over
> USB, which is not really a real world use case for regular browser usage.

Alex Poirot has done a lot of work to clamp down on reduce memory usage, make things lazier, etc.  I would expect the situation to be better than last time you tried.  But of course, we should definitely measure carefully to be sure it's safe.  There is now also a DevTools test to ensure memory remains below certain thresholds[1].

If just turning it on as-is for all USB debugging users is still too much memory, I think we could change the way we start things for Fennec.  Instead of starting the DebuggerServer first, we could start only the port listener.  Then, the first time someone connects to the port, we would start the DebuggerServer, delaying the larger memory cost until that point.  I guess we should discuss this stuff further in a separate bug...

[1]: https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/unit/test_memory_footprint.js
/r/7473 - Bug 1154504 - Remove paint flashing option from settings. r=liuche
/r/7475 - Bug 1154504 - (Part 2) Add proper browser UI migration logic to reset paint flashing pref. r=liuche

Pull down these commits:

hg pull -r 5c17e1eb7383a4e08ce1a82a7d62ac097c66921e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8596233 - Flags: review?(liuche)
For now I decided to just go ahead and remove this paint flashing pref from the UI, and we can file a separate bug to further discuss removing the developer tools settings altogether in favor of deferring to system debugging settings.

I also took this opportunity to add better logic (modeled after what desktop does) for migrating the browser on update. I believe we can safely remove all this pre-existing pref migration logic (most of which I added :/), since it will have run in older app updates.

Although thinking now, maybe this will miss the case of a user updating from a very old version to a new version... maybe I should add back this old logic after all :(
Comment on attachment 8596233 [details]
MozReview Request: bz://1154504/margaret

https://reviewboard.mozilla.org/r/7471/#review6231
Attachment #8596233 - Flags: review?(liuche)
https://reviewboard.mozilla.org/r/7475/#review6229

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> -    if (Services.prefs.prefHasUserValue("plugins.click_to_play")) {

Why is it okay to remove all this code? What if a user is upgrading from many versions back, don't we still need to migrate these prefs?

::: mobile/android/chrome/content/browser.js:917
(Diff revision 1)
> -      Reader.migrateCache().catch(e => Cu.reportError("Error migrating Reader cache: " + e));
> +    const UI_VERSION = 1;

Nice, I like that you're adding versioning!
Comment on attachment 8596233 [details]
MozReview Request: bz://1154504/margaret

/r/7473 - Bug 1154504 - Remove paint flashing option from settings. r=liuche
/r/7475 - Bug 1154504 - (Part 2) Add proper browser UI migration logic to reset paint flashing pref. r=liuche

Pull down these commits:

hg pull -r 9d4f92577dd58e98e38416dad2ec3cffcc42d17d https://reviewboard-hg.mozilla.org/gecko/
Attachment #8596233 - Flags: review?(liuche)
(In reply to J. Ryan Stinnett [:jryans] from comment #8)
> (In reply to :Margaret Leibovic from comment #7)
> > My one concern would be that in the past we had performance issues related
> > to turning on remote debugging by default on Nightly (bug 979101), and I
> > wouldn't want to regress performance for all users that have remote
> > debugging enabled on the Android level. However, I would hope that any
> > additional memory use would take effect when your phone is connected over
> > USB, which is not really a real world use case for regular browser usage.
> 
> Alex Poirot has done a lot of work to clamp down on reduce memory usage,
> make things lazier, etc.  I would expect the situation to be better than
> last time you tried.  But of course, we should definitely measure carefully
> to be sure it's safe.  There is now also a DevTools test to ensure memory
> remains below certain thresholds[1].
> 
> If just turning it on as-is for all USB debugging users is still too much
> memory, I think we could change the way we start things for Fennec.  Instead
> of starting the DebuggerServer first, we could start only the port listener.
> Then, the first time someone connects to the port, we would start the
> DebuggerServer, delaying the larger memory cost until that point.  I guess
> we should discuss this stuff further in a separate bug...
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/tests/
> unit/test_memory_footprint.js

I filed bug 1157531. I think it would be worth exploring this.
Do we have telemetry on the usage of this feature?

I agree that there should also probably be some description along with a few hoops the user needs to jump through to "promise they won't break anything".
Flags: needinfo?(alam) → needinfo?(margaret.leibovic)
(In reply to Anthony Lam (:antlam) from comment #16)
> Do we have telemetry on the usage of this feature?
> 
> I agree that there should also probably be some description along with a few
> hoops the user needs to jump through to "promise they won't break anything".

We do have telemetry, I don't know that it's very helpful if users are accidentally enabling this when they didn't mean to.

Given that web developers are advanced users and could enable this in about:config, I don't think it's worth the risk of exposing this.

Also, if a button for this is landing in WebIDE soon, that seems like a much better way to use this feature.
Flags: needinfo?(margaret.leibovic)
Yeah I can't say I feel strongly about it in either case as I don't even use this that often myself. 

But I do want to make this area a bit more difficult to get into (like the developer tools in Android). Perhaps we should use a simple dialog for confirmation that prevents the user from accidentally enabling this stuff? 

I suggest:

 - Add a simple dialog for confirmation to access these settings "Are you sure?"/"Promise not to break anything?"
 - Add descriptions to underneath each item inside as well.
(In reply to Anthony Lam (:antlam) from comment #18)
> Yeah I can't say I feel strongly about it in either case as I don't even use
> this that often myself. 
> 
> But I do want to make this area a bit more difficult to get into (like the
> developer tools in Android). Perhaps we should use a simple dialog for
> confirmation that prevents the user from accidentally enabling this stuff? 
> 
> I suggest:
> 
>  - Add a simple dialog for confirmation to access these settings "Are you
> sure?"/"Promise not to break anything?"
>  - Add descriptions to underneath each item inside as well.

Yeah, I agree we should make it more clear that this is "advanced". I think that fixing bug 1157531 would be the simplest way to do this.
Comment on attachment 8596233 [details]
MozReview Request: bz://1154504/margaret

https://reviewboard.mozilla.org/r/7471/#review6521

Nice, I like this migration pathway.
Attachment #8596233 - Flags: review?(liuche) → review+
Summary: Reconsider exposing paint debugging to the user → Reconsider exposing paint flashing to the user
Attachment #8596233 - Attachment is obsolete: true
Attachment #8620043 - Flags: review+
Attachment #8620044 - Flags: review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.