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

Fix toolbar icons. #1180

Merged
merged 1 commit into from
Dec 22, 2018
Merged

Fix toolbar icons. #1180

merged 1 commit into from
Dec 22, 2018

Conversation

mkarolin
Copy link
Collaborator

@mkarolin mkarolin commented Dec 22, 2018

Relocated toolbar icons to omnibox since toolbar is no longer used in Chromium. Also, relocated settings.icon to chrome/app.

Fixes brave/brave-browser#2641

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Verify Brave icon:
  • Start Brave browser and navigate to brave://version,
  • Validate that the Brave icon is shown in the location bar to the left of the word "Brave".
  1. Verify bookmark icons:
  • Start Brave browser and navigate to any site,
  • Validate that the bookmark button to the left of the location bar is a ribbon (not a star),
  • Click on the bookmark button and verify that it shows as a filled in ribbon (not a filled in star).
  1. Verify find in page icon:
  • Start Brave browser and navigate to any site,
  • Press Ctrl + F to start find in page,
  • Validate that the find icon that shows in the location bar looks like a square with a magnifying glass, not a filled in page with magnifying glass.
  1. Verify http icon:
  • Start Brave browser and navigate to any http site,
  • Validate that the icon to the left of "Not secure" is an exclamation point in a filled white circle (not letter i in an unfilled circle).
  1. Verify https icons:
  • Start Brave browser and navigate to any valid https site,
  • Validate that the icon to the left of the address is an unfilled lock with a dot in the middle (not a filled lock).
  • Navigate to a bad https site (e.g. https://untrusted-root.badssl.com/),
  • Validate that the icon to the left of "Not secure" is a white circle with an x inside (not a red triangle with an exclamation point).
  1. Verify settings icon:
  • Start Brave browser,
  • Click on the Avatar icon and validate that the Settings menu item icon is an unfilled gear (not a filled in gear).

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

Chromium. Also, relocated settings.icon to chrome/app.

Fixes brave/brave-browser#2641
@mkarolin mkarolin self-assigned this Dec 22, 2018
@mkarolin mkarolin changed the title WIP: Fix toolbar icons. Fix toolbar icons. Dec 22, 2018
@rossmoody
Copy link
Contributor

👍

@rebron
Copy link
Collaborator

rebron commented Dec 22, 2018

Approving uplift request. cc: @kjozwiak @srirambv. Thanks @mkarolin. Please merge to master and dev as well after reviews are completed.

@bbondy
Copy link
Member

bbondy commented Dec 22, 2018

Thanks!

@mkarolin mkarolin merged commit 6751645 into master Dec 22, 2018
@mkarolin mkarolin deleted the maxk-fix-toolbar-icons branch December 22, 2018 22:34
mkarolin added a commit that referenced this pull request Dec 22, 2018
mkarolin added a commit that referenced this pull request Dec 22, 2018
@mkarolin
Copy link
Collaborator Author

master: 6751645
0.60.x: 83cb307
0.59.x: 5d5a155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chromium icons: in url bar; bookmark this page.
4 participants