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.

Sign up for GitHub

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

Jump to bottom

Drop patch for profile_io_data.cc and use chromium_src instead #7995

Merged
merged 1 commit into from
Feb 23, 2021
Merged

Drop patch for profile_io_data.cc and use chromium_src instead #7995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions chromium_src/chrome/browser/profiles/profile_io_data.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,29 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#include "chrome/browser/profiles/profile_io_data.h"
#include "brave/common/url_constants.h"

#define IsHandledProtocol IsHandledProtocol_ChromiumImpl
#define IsHandledURL IsHandledURL_ChromiumImpl
#include "../../../../../chrome/browser/profiles/profile_io_data.cc"
#undef IsHandledURL
#undef IsHandledProtocol

bool ProfileIOData::IsHandledProtocol(const std::string& scheme) {
if (scheme == kBraveUIScheme)
return true;
return IsHandledProtocol_ChromiumImpl(scheme);
}

// 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(). Otherwise the old version will still be called since the
// renaming of IsHandledProtocol above does also modify the call point.
bool ProfileIOData::IsHandledURL(const GURL& url) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't quite follow what this is for

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, I just didn't look into what's happening in IsHandledURL(). Probably worth adding a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, comment added. Will merge once CI finishes running again. Thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mariospr mariospr Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (dhollowa@chromium.org      2013-03-07 12:30:06 +0000 279) // static
a8c1e745a571a (willchan@chromium.org      2011-05-14 06:17:07 +0000 280) bool ProfileIOData::IsHandledURL(const GURL& url) {
a8c1e745a571a (willchan@chromium.org      2011-05-14 06:17:07 +0000 281)   if (!url.is_valid()) {
a8c1e745a571a (willchan@chromium.org      2011-05-14 06:17:07 +0000 282)     // We handle error cases.
a8c1e745a571a (willchan@chromium.org      2011-05-14 06:17:07 +0000 283)     return true;
a8c1e745a571a (willchan@chromium.org      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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, not a strong concern.

if (!url.is_valid()) {
// We handle error cases.
return true;
}
return IsHandledProtocol(url.scheme());
}
25 changes: 25 additions & 0 deletions chromium_src/chrome/browser/profiles/profile_io_data.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/* Copyright (c) 2021 The Brave Authors. All rights reserved.
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

#ifndef BRAVE_CHROMIUM_SRC_CHROME_BROWSER_PROFILES_PROFILE_IO_DATA_H_
#define BRAVE_CHROMIUM_SRC_CHROME_BROWSER_PROFILES_PROFILE_IO_DATA_H_

#include <string>
#include "url/gurl.h"

#define IsHandledProtocol \
IsHandledProtocol_ChromiumImpl(const std::string& scheme); \
static bool IsHandledProtocol

#define IsHandledURL \
IsHandledURL_ChromiumImpl(const GURL& url); \
static bool IsHandledURL

#include "../../../../../chrome/browser/profiles/profile_io_data.h"

#undef IsHandledURL
#undef IsHandledProtocol

#endif // BRAVE_CHROMIUM_SRC_CHROME_BROWSER_PROFILES_PROFILE_IO_DATA_H_
12 changes: 0 additions & 12 deletions patches/chrome-browser-profiles-profile_io_data.cc.patch
View file
Open in desktop

This file was deleted.