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

Use override method for ChromeBrowserMainExtraPartsView #230

Merged
merged 2 commits into from
Jul 25, 2018
Merged

Use override method for ChromeBrowserMainExtraPartsView #230

merged 2 commits into from
Jul 25, 2018

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Jul 7, 2018 •
edited
Loading

With this, we don't need to subclass ChromeBrowserMainExtraPartsView and
touch ChromeContentBrowserClient.

Issue: brave/brave-browser#461

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.

Test Plan:

yarn test brave_browser_tests Release --filter=*.GetDefaultWindowIconTest

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions

@simonhong simonhong self-assigned this Jul 7, 2018
@simonhong simonhong requested a review from bridiver July 7, 2018 05:05
@simonhong simonhong mentioned this pull request Jul 7, 2018
Merged
9 tasks
@simonhong simonhong force-pushed the use_override_for_brave_views_delegate branch from 9b4d104 to 6153ae3 Compare July 20, 2018 01:15
@simonhong simonhong requested a review from garrettr July 20, 2018 01:17
@simonhong
Use override method for ChromeBrowserMainExtraPartsView
5128710 
With this, we don't need to subclass ChromeBrowserMainExtraPartsView and
touch ChromeContentBrowserClient.
@simonhong simonhong force-pushed the use_override_for_brave_views_delegate branch from 6153ae3 to 5128710 Compare July 22, 2018 07:59
@simonhong simonhong changed the title Use override method for ChromeBrowserMainExtraPartsView WIP: Use override method for ChromeBrowserMainExtraPartsView Jul 23, 2018
@simonhong
Copy link
Member Author

simonhong commented Jul 23, 2018 •
edited
Loading

Found that this PR work with yarn start but test is failed after rebasing onto C68.

After this https://chromium-review.googlesource.com/c/chromium/src/+/981937,
ChromeViewsDelegate is set before ChromeBrowserMainExtraPartsViews sets ChromeViewsDelegate in browser test.
Because of this early setting, we can't set our BraveBrowserViewsDelegate.

@simonhong
Enable GetDefaultWindowIconTest only in official(release) build
4957da9 
Set views delegate to our BraveViewsDelegateLinux by explicitly initialize.
This test is only added in official(release) build because setting views delegate
twice isn't allowed in debug mode.
@simonhong simonhong changed the title WIP: Use override method for ChromeBrowserMainExtraPartsView Use override method for ChromeBrowserMainExtraPartsView Jul 23, 2018
@simonhong
Copy link
Member Author

Test is enabled only in release mode.
PTAL.

@simonhong
Copy link
Member Author

Kindly ping..

@simonhong simonhong requested a review from bbondy July 24, 2018 23:09
@bbondy
Copy link
Member

bbondy commented Jul 25, 2018

Thanks for cleaning up and removing an old patch

@bbondy bbondy merged commit 2265742 into master Jul 25, 2018
@simonhong simonhong deleted the use_override_for_brave_views_delegate branch August 19, 2018 23:22
NejcZdovc pushed a commit that referenced this pull request Sep 19, 2019
@ryanml @NejcZdovc
Rewards: Imported from brave-ui: Fixes #230, enforces 1024px min-widt…
9f620fd 
…h for settingsPage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bbondy bbondy bbondy approved these changes

@bridiver bridiver Awaiting requested review from bridiver bridiver is a code owner

@garrettr garrettr Awaiting requested review from garrettr

Assignees

@simonhong simonhong

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@simonhong @bbondy