Drop patch for profile_io_data.cc and use chromium_src instead #7995
Conversation
| return IsHandledProtocol_ChromiumImpl(scheme); | ||
| } | ||
|
|
||
| bool ProfileIOData::IsHandledURL(const GURL& url) { |
There was a problem hiding this comment.
don't quite follow what this is for
There was a problem hiding this comment.
IsHandledProtocol() is referenced from the body of IsHandledURL() in this profile_io_data.cc file, and thus the renaming via #define IsHandledProtocol IsHandledProtocol_ChromiumImpl done above this lines also changes that line inside IsHandledURL()'s body.
Therefore, I understand that we need to provide our own version of IsHandledURL() as well to make sure that we get our own version of IsHandledProtocol() called when invoked via IsHandledURL(), no?
There was a problem hiding this comment.
gotcha, I just didn't look into what's happening in IsHandledURL(). Probably worth adding a comment
There was a problem hiding this comment.
No problem, comment added. Will merge once CI finishes running again. Thanks!
There was a problem hiding this comment.
This method seems to be missing
if (!url.is_valid()) {
// We handle error cases.
return true;
}
I am a bit hesitant about replacing methods in this way: if the original method is further modified we'd never know.
There was a problem hiding this comment.
This method seems to be missing
if (!url.is_valid()) {
// We handle error cases.
return true;
}
Oops! True, added now.
I am a bit hesitant about replacing methods in this way: if the original method is further modified we'd never know.
I share the same concern to be honest. In the other hand, I think this risk already exists in other overrides since renaming a symbol via define will change not just the name of the method in its definition, but also any callpoint inside the same .cc file. That is, imagine another that we already have where we rename an internal method that was originally not called from any other point in that .cc file, and then upstream decides to reference it at some point from another method in that file: our override might not be correct now since that newly added callpoint would now be invoking the unmodified logic, not ours... and we don't have an easy way to detect that, I think?
Now back to this particular case, I did check what git blame had to say about IsHandledURL() and found this:
672c8c1e10e9b ([email protected] 2013-03-07 12:30:06 +0000 279) // static
a8c1e745a571a ([email protected] 2011-05-14 06:17:07 +0000 280) bool ProfileIOData::IsHandledURL(const GURL& url) {
a8c1e745a571a ([email protected] 2011-05-14 06:17:07 +0000 281) if (!url.is_valid()) {
a8c1e745a571a ([email protected] 2011-05-14 06:17:07 +0000 282) // We handle error cases.
a8c1e745a571a ([email protected] 2011-05-14 06:17:07 +0000 283) return true;
a8c1e745a571a ([email protected] 2011-05-14 06:17:07 +0000 284) }
That is, it seems that the last time this IsHandledURL() method was modified was 10 years ago, so perhaps the risk in this particular case is low.
@mkarolin WDYT?
PS: FWIW, I'm not pushing for this to be done, I'm happy to go back to having a .patch and re-think how to get rid about it (or keep it), just mentioning this because it was the reasoning I followed
There was a problem hiding this comment.
Agree, not a strong concern.
mariospr
force-pushed
the
issues/14238
branch
from
9c82ec9
to
af9c351
Compare
mariospr
force-pushed
the
issues/14238
branch
4 times, most recently
from
5c0869c
to
5c534f3
Compare
|
@brave/patch-reviewers Looks like I still need a review from you, PTAL when you have a moment, thanks! |
mariospr
force-pushed
the
issues/14238
branch
from
5c534f3
to
aa5d008
Compare
The idea of this patch was simply to make sure that the brave:// scheme gets considered by ProfileIOData::IsHandledProtocol() along with other schemes, but we can achieve the same goal just using chromium_src overrides, so let's do that instead. Fixes brave/brave-browser#14238
mariospr
force-pushed
the
issues/14238
branch
from
aa5d008
to
0a7a69b
Compare
|
The failure in the post-init CI bot is unrelated to this PR and this can be safely merged now, so doing it now. Thanks! |
The idea of this patch was simply to make sure that the brave://
scheme gets considered by ProfileIOData::IsHandledProtocol() along
with other schemes, but we can achieve the same goal just using
chromium_src overrides, so let's do that instead.
Fixes brave/brave-browser#14238
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_tests,npm run lint,npm run gn_check,npm run tslintgit rebase master(if needed)Reviewer Checklist:
gnAfter-merge Checklist:
changes has landed on
Test Plan:
See Test plan from #1196