Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote debugging can now be enabled via brave://settings/privacy #4044

Merged
merged 3 commits into from
Nov 25, 2019

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Nov 20, 2019

Remote debugging can now be enabled via brave://settings/privacy

  • Default for this is FALSE (meaning remote debugging is disabled).
  • Setting is global (using local_state), not per-profile.

Fixes brave/brave-browser#5640
Fixes brave/brave-browser#3199

NOTE: if this is merged, I will need to update https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)

Screen Shot 2019-11-21 at 3 47 54 PM

Submitter Checklist:

Test Plan:

Test remote debugging feature (brave/brave-browser#5640)

  1. New profile
  2. Connect Android device to Desktop
  3. Open brave://inspect#devices in Desktop
  4. Visit a site on brave Android
  5. Ensure the site lists on brave://inspect#devices
  6. Click on inspect under the device name in desktop
  7. Developer tool window launches but never loads
  8. Quit Brave and relaunch
  9. Visit brave://settings/privacy
  10. Enable Remote debugging (it should have been turned off)
  11. With device still attached, visit a site on brave Android
  12. Click on inspect under the device name in desktop
  13. Developer tool window launches
  14. It should work 🎉

Test Lighthouse (brave/brave-browser#3199)

  1. New profile
  2. Launch Brave from command line (so you can see stdout)
  3. Visit https://clifton.io
  4. Open developer tools (F12 / Cmd+Opt+I)
  5. Open Audits
  6. Click Run audits
  7. stdout should output Remote debugging is DISABLED. If you want to use it, please enable in brave://settings/privacy (may show 2 or 3 times)
  8. Verify that Audit gets stuck
  9. Quit Brave and relaunch
  10. Visit brave://settings/privacy
  11. Enable Remote debugging (it should have been turned off)
  12. Visit https://clifton.io
  13. Open developer tools (F12 / Cmd+Opt+I)
  14. Open Audits
  15. Click Run audits
  16. Lighthouse functionality should work 🎉

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bsclifton
Copy link
Member Author

bsclifton commented Nov 21, 2019

@rebron this section is getting a bit crowded- we have an opportunity to maybe have a Brave specific section... maybe something like Optional features, Extended Privacy, Services that reach out, etc
Screen Shot 2019-11-20 at 10 31 07 PM

If there was a new area, that would be where this setting, P3A, Push Messaging, WebRTC, and other coming soon settings (like disabling autocomplete, disabling tor) could live. Basically leaving the Chromium ones alone

@simonhong
Copy link
Member

It would be good if settings change can be synced across multiple settings page.

@rebron
Copy link
Collaborator

rebron commented Nov 21, 2019

Let's go with Remote debugging and current placement for now, we need to redesign this section and make it more user friendly.

@bsclifton bsclifton added this to the 1.3.x - Nightly milestone Nov 22, 2019
@bsclifton
Copy link
Member Author

Text updated, security review approved! 🎉

@jumde
Copy link
Contributor

jumde commented Nov 22, 2019

enabling remote debugging sends requests to google, are we okay not proxying those requests?

@bsclifton
Copy link
Member Author

bsclifton commented Nov 22, 2019

@jumde since it's opt-in, this approach is OK and was security review approved. But I do think we can create an enhancement to proxy that in the future

Also: there was a lint error (my bad) - just checked in a fix for that. Otherwise, all CI passed 🎉

@bsclifton
Copy link
Member Author

bsclifton commented Nov 22, 2019

@simonhong do you mean for the scenario where multiple windows with different profiles are open? (and setting is updated in one?)

@simonhong
Copy link
Member

simonhong commented Nov 22, 2019

@bsclifton For this prefs, profiles are not important because it's local state.
If I opened multiple settings page(same profile or not) and I changed this settings in one window, other settings page should show same newly changed option value.
(If profile prefs are used, this is done w/o dev effort for same profile's windows.)

@bsclifton
Copy link
Member Author

@simonhong ah- yes, I forgot you can manually open more instances of settings (if you keep trying via hamburger, it opens an already existing one)

@simonhong
Copy link
Member

Yes, we can open many settings pages. but I think it is not the blocker for this PR :)

@bsclifton bsclifton force-pushed the bsc-re-enable-remote-debugging branch 2 times, most recently from 6e16d6f to 29979dc Compare November 22, 2019 07:29
@bsclifton
Copy link
Member Author

Updated PR to cover the multiple window scenario (also, fixed this for P3A which was missing this too). Tested locally on macOS w/ Debug/Release. Rebased and CI running again.

@bsclifton bsclifton force-pushed the bsc-re-enable-remote-debugging branch 2 times, most recently from 8f10390 to e8305c9 Compare November 22, 2019 20:21
@bsclifton bsclifton force-pushed the bsc-re-enable-remote-debugging branch from e8305c9 to 846a1cf Compare November 25, 2019 17:58
@bsclifton
Copy link
Member Author

Only failure was on iOS compilation (which was fixed/merged in master with #4070). I'll label accordingly, rebase, and restart CI

@bsclifton bsclifton added CI/skip-android Do not run CI builds for Android CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 labels Nov 25, 2019
Default for this is FALSE (meaning remote debugging is disabled). Setting is global
(using local_state), not per-profile.

Fixes brave/brave-browser#5640
Fixes brave/brave-browser#3199
…value is in sync

For example, if you had two windows with brave://settings/privacy open
@bsclifton bsclifton force-pushed the bsc-re-enable-remote-debugging branch from 846a1cf to e7b2d99 Compare November 25, 2019 21:51
@bsclifton
Copy link
Member Author

There we go- after rebase, iOS looks good 🎉

@bsclifton bsclifton merged commit 78ec598 into master Nov 25, 2019
@bsclifton bsclifton deleted the bsc-re-enable-remote-debugging branch November 25, 2019 22:16
@bsclifton
Copy link
Member Author

Forgot to update documentation! Doing that now 😄

@bsclifton
Copy link
Member Author

updated!

@bsclifton
Copy link
Member Author

Possibly also fixed brave/brave-browser#5618 when this landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
5 participants