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

Show brave url on status bubble #1825

Merged
merged 1 commit into from
Mar 12, 2019
Merged

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Mar 3, 2019

SetURL() is called when renderer sent ViewHostMsg_UpdateTargetURL message.
So, replacing that url should be done at bubble instead of the middle of that path.

Fix brave/brave-browser#2566

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
  • Verified that all lint errors/warnings are resolved (npm run lint)
  • 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:

yarn test brave_unit_tests --filter= BraveStatusBubbleViewsTest*

  1. Open new tab
  2. Hover settings button
  3. Check status bubble shows brave://settings instead of chrome://settings

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

@simonhong simonhong added this to the 0.63.x - Nightly milestone Mar 3, 2019
@simonhong simonhong self-assigned this Mar 3, 2019
@simonhong simonhong requested a review from petemill March 3, 2019 04:21
@simonhong simonhong added the UI label Mar 3, 2019
SetURL() is called when renderer sent ViewHostMsg_UpdateTargetURL message.
So, replacing that url should be done at bubble instead of the middle of that path.
@simonhong simonhong force-pushed the status_bubble_view_brave_url branch from 205c313 to b418116 Compare March 10, 2019 22:53
@simonhong simonhong requested a review from mkarolin March 10, 2019 22:55
@simonhong
Copy link
Member Author

kindly ping..

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, tested on Windows - works as expected.

@simonhong
Copy link
Member Author

@mkarolin Thanks for review 👍

@simonhong simonhong merged commit eba28d0 into master Mar 12, 2019
@simonhong simonhong deleted the status_bubble_view_brave_url branch March 12, 2019 21:26
bsclifton added a commit that referenced this pull request Apr 9, 2019
Unfixes brave/brave-browser#2566

This reverts commit eba28d0, reversing
changes made to 94401f9.
bsclifton added a commit that referenced this pull request Apr 9, 2019
bsclifton added a commit that referenced this pull request Apr 9, 2019
Unfixes brave/brave-browser#2566

This reverts commit eba28d0, reversing
changes made to 94401f9.
bsclifton added a commit that referenced this pull request Apr 9, 2019
…-0.64.x

Revert #1825 because of unit test failures
pilgrim-brave pushed a commit that referenced this pull request Apr 16, 2019
Unfixes brave/brave-browser#2566

This reverts commit eba28d0, reversing
changes made to 94401f9.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Tab Page uses 'chrome' links for History, Bookmarks, and Preferences
2 participants