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.

Sign up for GitHub

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

Jump to bottom

Remove javascript icon from location bar #7148

Merged
merged 1 commit into from
Nov 24, 2020
Merged

Remove javascript icon from location bar #7148

merged 1 commit into from
Nov 24, 2020

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Nov 16, 2020 •
edited
Loading

Fix brave/brave-browser#199

Submitter Checklist:

Test Plan:

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.

@diracdeltas diracdeltas added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Nov 16, 2020
@diracdeltas diracdeltas requested a review from a team as a code owner November 16, 2020 18:59
@diracdeltas diracdeltas force-pushed the fix/noscript2 branch 3 times, most recently from 51f4ee2 to 15bbfce Compare November 17, 2020 03:43
darkdh
darkdh reviewed Nov 17, 2020
View reviewed changes
patches/chrome-browser-ui-content_settings-content_setting_image_model.cc.patch
@@ -15,7 +15,7 @@ index 4f8182d209673801407db8955c370a026511deed..55361a1022b7cb2b683c336894834b20
for (auto type : kContentSettingImageOrder)
result.push_back(CreateForContentType(type));
+#if defined(BRAVE_CHROMIUM_BUILD)
+ BraveGenerateContentSettingImageModels(result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's move this to chromium_src/chrome/browser/ui/content_settings/content_setting_image_model.cc along with +#include "brave/browser/ui/content_settings/brave_content_setting_image_models.h"
And define these as BRAVE_GENERATE_CONTENT_SETTING_IMAGE_MODELS macro

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering do we really need BRAVE_CHROMIUM_BUILD anywhere (cc @bridiver @simonhong) ?
we also probably can avoid patching by wrapping GenerateContentSettingImageModels with our override

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case (just adding our code), I think BRAVE_CHROMIUM_BUILD can be deleted.

@diracdeltas diracdeltas removed the request for review from bbondy November 18, 2020 22:38
@diracdeltas diracdeltas force-pushed the fix/noscript2 branch 2 times, most recently from 7861c22 to b513022 Compare November 20, 2020 23:30
@diracdeltas
Copy link
Member Author

updated! @darkdh let me know if this is what you meant

@diracdeltas diracdeltas force-pushed the fix/noscript2 branch 3 times, most recently from 583609b to d176510 Compare November 23, 2020 20:25
mkarolin
mkarolin approved these changes Nov 23, 2020
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@diracdeltas
Copy link
Member Author

CI tests are failing for a unrelated reason: https://bravesoftware.slack.com/archives/CA5FPHWLF/p1606189385254900

@diracdeltas diracdeltas merged commit 556c10e into master Nov 24, 2020
@diracdeltas diracdeltas deleted the fix/noscript2 branch November 24, 2020 04:41
@diracdeltas diracdeltas added this to the 1.19.x - Nightly milestone Nov 24, 2020
@bsclifton bsclifton mentioned this pull request Dec 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darkdh darkdh darkdh left review comments

@simonhong simonhong simonhong left review comments

@mkarolin mkarolin mkarolin approved these changes

@iefremov iefremov iefremov approved these changes

Assignees
No one assigned
Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS
Projects
None yet
Milestone
1.19.x - Release
Development

Successfully merging this pull request may close these issues.

Remove chrome's noscript UI
5 participants
@diracdeltas @iefremov @simonhong @darkdh @mkarolin