Fixes hover card bubble chrome:// urls. #3687

mkarolin · 2019-10-12T06:08:00Z

Fixes brave/brave-browser#6385

Replaces chrome:// scheme with brave://.

Submitter Checklist:

Test Plan:

Start Brave.
Navigate to brave://settings.
Hover mouse over the tab.
Observe popup showing the title Settings and subtitle brave://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

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 · 2019-10-12T06:34:30Z

Tested this on macOS; works great! 😄

mkarolin · 2019-10-14T14:22:22Z

CI failures are in lint and an unrelated browser test.

bsclifton

LGTM! 😄

bridiver · 2019-10-16T16:17:55Z

chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.h

+#define BRAVE_TAB_HOVER_CARD_BUBBLE_VIEW_H_ \
+                                            \
+ protected:                                 \
+  class BraveAnonymous;                     \


I'm not understanding what is happening here. Why are we creating an inner class that just has a single static method? Why do we need a class here at all?

I am using an inner class here because inner classes have access to private members.
Because I am redefining TabHoverCardBubbleView as TabHoverCardBubbleView_ChromiumImpl (and then declaring the subclass as TabHoverCardBubbleView) I can't add a friend class TabHoverCardBubbleView.

bridiver · 2019-10-16T16:21:27Z

chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.cc

+};
+
+void TabHoverCardBubbleView::UpdateCardContent(const Tab* tab) {
+  TabHoverCardBubbleView_ChromiumImpl::BraveAnonymous::UpdateCardContent(


re https://github.com/brave/brave-core/pull/3687/files#r335575232 - why aren't we just directly calling the code in TabHoverCardBubbleView_ChromiumImpl::BraveAnonymous::UpdateCardContent?

Similarly here, I am going through the inner class method to get to the private members of the base class (because I couldn't make the subclass a friend with this approach).

why rename it? Wouldn't it be simpler to subclass/friend and override in tab_strip.cc? You need a patch in TabHoverCardBubbleView either way and it would be simpler to understand imo

Mainly was just trying out a way to make changes without having to patch tab_strip.cc.

you wouldn't have to patch tab_strip.cc, you can change the class name with a chromium_src override

Yes, but with TabHoverCardBubbleView, tab_strip.cc:126 also has
TabHoverCardEventSniffer(TabHoverCardBubbleView* hover_card, TabStrip* tab_strip)
which would get redefined.

why does that matter? I saw it, but maybe I'm missing something?

ok, I see why it matters (TabHoverCardBubbleView* hover_card_ = nullptr;) in the header, but can't you just override in the header as well?

I mean, what you have works, it's just harder to follow because it's very different from what we would normally do in this case. If we can subclass and override I think it would be better for long-term maintenance

or if we went this route, wouldn't it be simpler to just directly patch in a method to TabHoverCardBubbleView_ChromiumImpl instead of creating an inner class?

bridiver · 2019-10-16T16:23:46Z

chromium_src/chrome/browser/ui/views/tabs/tab_hover_card_bubble_view.h

+ private:
+
+#define TabHoverCardBubbleView TabHoverCardBubbleView_ChromiumImpl
+#define UpdateCardContent virtual UpdateCardContent


this could be used to get rid of a lot of other patches for virtual. I didn't realize it was valid to have void virtual Method(...) cc @bbondy

@mkarolin please take advantage of this when doing chromium updates for existing patches using virtual

Yes, you can have virtual after the return type as long as the return type is not a pointer or a reference. If they are then you need to have A virtual *function, which doesn't help us.

that's the case even though the standard for chromium code is T* function()? Shouldn't the preprocessor replacement work correctly for that?

patches/chrome-browser-ui-views-tabs-tab_hover_card_bubble_view.h.patch

Fixes brave/brave-browser#6385 Replaces chrome:// scheme with brave:// in the hover card text. Added a browser test to test the same. Also, organized brave_browser_tests sources alphabetically in test/BUILD.gn.

mkarolin · 2019-10-17T18:48:12Z

CI fails in an unrelated unit test.

Fixes hover card bubble chrome:// urls.

mkarolin added this to the 0.72.x - Nightly milestone Oct 12, 2019

mkarolin self-assigned this Oct 12, 2019

mkarolin force-pushed the maxk-fix-hover-card-bubble-2 branch from 74b2012 to 3ebf237 Compare October 12, 2019 16:10

mkarolin changed the title ~~Fixes hover card bubble chrome:// urls.~~ WIP: Fixes hover card bubble chrome:// urls. Oct 12, 2019

mkarolin marked this pull request as ready for review October 12, 2019 16:35

mkarolin requested a review from bridiver as a code owner October 12, 2019 16:35

mkarolin force-pushed the maxk-fix-hover-card-bubble-2 branch 2 times, most recently from 232e25d to 14bae84 Compare October 13, 2019 00:31

bsclifton mentioned this pull request Oct 14, 2019

Upgrade from Chromium 77.0.3865.90 to Chromium 78.0.3904.50 brave/brave-browser#6067

Closed

mkarolin changed the title ~~WIP: Fixes hover card bubble chrome:// urls.~~ Fixes hover card bubble chrome:// urls. Oct 14, 2019

mkarolin requested review from petemill and bsclifton October 14, 2019 14:30

bsclifton approved these changes Oct 14, 2019

View reviewed changes

NejcZdovc modified the milestones: 0.72.x - Dev, 0.73.x - Nightly Oct 16, 2019

bridiver reviewed Oct 16, 2019

View reviewed changes

patches/chrome-browser-ui-views-tabs-tab_hover_card_bubble_view.h.patch Show resolved Hide resolved

mkarolin force-pushed the maxk-fix-hover-card-bubble-2 branch 3 times, most recently from af7ec45 to 3dbda8d Compare October 16, 2019 22:22

mkarolin added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux and removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Oct 17, 2019

mkarolin removed CI/skip-linux labels Oct 17, 2019

Fixes hover card bubble chrome:// urls.

defda68

Fixes brave/brave-browser#6385 Replaces chrome:// scheme with brave:// in the hover card text. Added a browser test to test the same. Also, organized brave_browser_tests sources alphabetically in test/BUILD.gn.

mkarolin force-pushed the maxk-fix-hover-card-bubble-2 branch from 3dbda8d to defda68 Compare October 17, 2019 16:25

bridiver approved these changes Oct 17, 2019

View reviewed changes

mkarolin merged commit 8f02f5c into master Oct 17, 2019

mkarolin deleted the maxk-fix-hover-card-bubble-2 branch October 17, 2019 18:48

mkarolin added a commit that referenced this pull request Oct 17, 2019

Merge pull request #3687 from brave/maxk-fix-hover-card-bubble-2

7cf1e39

Fixes hover card bubble chrome:// urls.

mkarolin added a commit that referenced this pull request Oct 17, 2019

Merge pull request #3687 from brave/maxk-fix-hover-card-bubble-2

bd6e6d5

Fixes hover card bubble chrome:// urls.

mkarolin added a commit that referenced this pull request Oct 17, 2019

Merge pull request #3687 from brave/maxk-fix-hover-card-bubble-2

bb728e2

Fixes hover card bubble chrome:// urls.

This was referenced Oct 17, 2019

Fixes hover card bubble chrome:// urls (0.72.x). #3732

Merged

Fixes hover card bubble chrome:// urls (0.71.x). #3733

Merged

Fixes hover card bubble chrome:// urls (0.70.x). #3734

Merged

LaurenWags mentioned this pull request Oct 21, 2019

Hover card shows chrome:// on brave:// pages (C78) brave/brave-browser#6385

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes hover card bubble chrome:// urls. #3687

Fixes hover card bubble chrome:// urls. #3687

mkarolin commented Oct 12, 2019 •

edited

Loading

bsclifton commented Oct 12, 2019

mkarolin commented Oct 14, 2019

bsclifton left a comment

bridiver Oct 16, 2019

mkarolin Oct 16, 2019

bridiver Oct 16, 2019

mkarolin Oct 16, 2019

bridiver Oct 16, 2019 •

edited

Loading

mkarolin Oct 16, 2019

bridiver Oct 16, 2019

mkarolin Oct 16, 2019

bridiver Oct 17, 2019

bridiver Oct 17, 2019

bridiver Oct 17, 2019

bridiver Oct 17, 2019

bridiver Oct 16, 2019

bridiver Oct 16, 2019

mkarolin Oct 16, 2019

bridiver Oct 16, 2019

mkarolin commented Oct 17, 2019

Fixes hover card bubble chrome:// urls. #3687

Fixes hover card bubble chrome:// urls. #3687

Conversation

mkarolin commented Oct 12, 2019 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

After-merge Checklist:

bsclifton commented Oct 12, 2019

mkarolin commented Oct 14, 2019

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkarolin commented Oct 17, 2019

mkarolin commented Oct 12, 2019 •

edited

Loading

bridiver Oct 16, 2019 •

edited

Loading