[Tabs]: Add BraveTabFeatures #27322
[Tabs]: Add BraveTabFeatures #27322
Conversation
github-actions
bot
added
CI/run-network-audit
fallaciousreasoning
force-pushed
the
brave-tab-featuresx
branch
from
8ba47b0 to
a0e8135
Compare
browser/sources.gni
Outdated
| "//brave/browser/android/brave_tab_features_android.cc", | ||
| "//brave/browser/android/brave_tab_features_android.h", |
There was a problem hiding this comment.
note: this is what upstream does. I imagine they'll change it at some point in the future but I think its good to match them for now.
There was a problem hiding this comment.
That's probably because there is a circular dependency in theirs? If we can avoid adding new sources to sources.gni directly, we should
There was a problem hiding this comment.
I can put these in their own target, but I'll need to add a dep on //chrome/browser/android and allow_circular_includes_from - is that okay?
| public: | ||
| TabFeaturesAndroid(content::WebContents* web_contents, Profile* profile); | ||
| - ~TabFeaturesAndroid(); | ||
| + virtual ~TabFeaturesAndroid(); |
There was a problem hiding this comment.
Maybe we should rename the base class so we don't need this patch. So change TabFeaturesAndroid -> TabFeaturesAndroid_ChromiumImpl and call our subclass TabFeaturesAndroid so there are no base class pointers? If we do that here we should do the same for TabFeatures for consistency
There was a problem hiding this comment.
actually we can still name ours BraveTabFeatures* and typedef/using to also be TabFeatures*
There was a problem hiding this comment.
I think its better to subclass here - that way if upstream adds some new public methods (highly likely, judging by the look of TabFeatures on desktop) it won't break our overrides.
Additionally if we do that we'll need a std::unique_ptr<TabFeaturesAndroid_ChromiumImpl> on our class to ensure everything its instantiating still exists?
There was a problem hiding this comment.
we are still subclassing, we're just changing the name of the base class
There was a problem hiding this comment.
we do this in other places as well when having a subclass with a different name is problematic for some reason
There was a problem hiding this comment.
We'll still need to make the destructor virtual though, right? Or would it be safe because all the upstream code would be dealing with our instance of the class?
There was a problem hiding this comment.
Hey, so I had a quick go at converting this over - we can't declare ours to be BraveTabFeatures and have a using statement for TabFeatures because it conflicts with the forward declarations in TabAndroid. Naming ours TabFeaturesAndroid works but I'm not a huge fan if we can avoid it.
Personally I think I'd prefer having the patch - especially because I suspect that they'll convert this constructor to virtual once they flesh out the functionality of TabFeaturesAndroid a bit more.
I suspect that class is going to see a bit of churn because:
- It doesn't support being replaced in tests
- It has extremely limited functionality (when compared to the desktop equivalent)
- It doesn't seem like it can replace TabHelpers yet
There was a problem hiding this comment.
I've pushed the replacement - I wasn't sure whether to leave the filenames as brave_ or not so I've left it for now.
FWIW, I still have a preference for the patching approach for the reasons above
fallaciousreasoning
force-pushed
the
brave-tab-featuresx
branch
2 times, most recently
from
8cde2ed to
e8d84d5
Compare
fallaciousreasoning
force-pushed
the
brave-tab-featuresx
branch
from
e8d84d5 to
452ed92
Compare
Resolves brave/brave-browser#43437
Submitter Checklist:
QA/YesorQA/No;release-notes/includeorrelease-notes/exclude;OS/...) to the associated issuenpm run test -- brave_browser_tests,npm run test -- brave_unit_testswikinpm run presubmitwiki,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
N/A - no changes to existing behavior