Fix wrong location icon and page info bubble of brave page #1722
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
simonhong
force-pushed
the
fix_location_icon_and_page_info_bubble
branch
2 times, most recently
from
21c582a
to
85692c8
Compare
Currently we use brave schemed url as a virtual url of navigation entry. However, Location area uses virtual url to check whether it is chrome page or not. So, our webui bypassed. To show proper icon and bubble for brave page, brave scheme url should be also checked.
simonhong
force-pushed
the
fix_location_icon_and_page_info_bubble
branch
from
85692c8
to
e612f10
Compare
simonhong
changed the title
simonhong
commented
Feb 19, 2019
| @@ -0,0 +1,24 @@ | |||
| diff --git a/chrome/browser/ui/views/page_info/page_info_bubble_view.cc b/chrome/browser/ui/views/page_info/page_info_bubble_view.cc | |||
| index ebe5987d5b4239e09a9b255b0d73ec9f7ad74fe9..933b15b53b86376e63a425701ae8cb8571d8b692 100644 | |||
| --- a/chrome/browser/ui/views/page_info/page_info_bubble_view.cc | |||
There was a problem hiding this comment.
I can't avoid this patch file because multiple PageInfoBubbleView::CreatePageInfoBubble() is used in this file and InternalPageInfoBubbleView class is in anonymous namespace.
Add test cases for BraveBrowserLocationBarModelDelegate, BraveLocationIconView and PageInfoBubbleView. All new tests check whether these classes handle brave url properly.
simonhong
force-pushed
the
fix_location_icon_and_page_info_bubble
branch
from
ef98cfc
to
d399680
Compare
|
kindly ping.. |
|
@simonhong please can we squash the 3 commits since they're all the same feature |
|
...at least for the tests. For the lint if it's fixing unrelated changes that were required because you modified those files then IMO good to keep separate commit 👍 |
|
I'm going to defer review to @bridiver as long as he can get to it before 0.62.x hits beta, for the following reasons:
|
|
Wrote closed reason for record. @bridiver will fix this issue with another approach. |
fmarier
pushed a commit
that referenced
this pull request
Oct 29, 2019
docs/installing: Add new public key URL for new signing key
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we use brave schemed url as a virtual url of navigation entry.
However, Location area uses virtual url to check whether it is chrome page
or not. So, our webui bypassed. To show proper icon and bubble for brave page,
brave scheme url should be also checked.
Fix brave/brave-browser#3411
Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests) ongit rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
Please refer to STR in brave/brave-browser#3411
PageInfoBubbleViewTest.BraveURLTestBraveLocationIconViewTest.BraveURLTestBraveBrowserLocationBarModelDelegateTest.VectorIconOverrideTestReviewer Checklist: