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.

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

[113.0.5672.63 follow up] Fix build failure on master branch #30060

Closed
mkarolin opened this issue Apr 28, 2023 · 6 comments · Fixed by brave/brave-core#18301
Closed

[113.0.5672.63 follow up] Fix build failure on master branch #30060

mkarolin opened this issue Apr 28, 2023 · 6 comments · Fixed by brave/brave-core#18301

Comments

@mkarolin
Copy link
Contributor

mkarolin commented Apr 28, 2023

Merge of brave/brave-core#18271 and brave/brave-core#18243 conflicted on master branch:

In file included from ../../brave/chromium_src/chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:16:
In file included from ../../../src/chrome/browser/ui/views/download/download_shelf_context_menu_view.cc:11:
In file included from ../../chrome/browser/download/bubble/download_bubble_ui_controller.h:11:
In file included from ../../chrome/browser/download/bubble/download_bubble_update_service.h:15:
../../brave/chromium_src/chrome/browser/download/bubble/download_display_controller.h:32:66: error: non-virtual member function marked 'override' hides virtual member function
      std::vector>& all_models) override;
                                                                 ^

and more...

QA:
Please, verify that brave/brave-core#18243 still works.

@stephendonner
Copy link

@brave/qa-team probably best to verify this once #30882 gets uplifted to 1.53.x - does that sound prudent/necessary, @mkarolin @simonhong given the expected results?

@simonhong
Copy link
Member

@brave/qa-team probably best to verify this once #30882 gets uplifted to 1.53.x - does that sound prudent/necessary, @mkarolin @simonhong given the expected results?

@stephendonner #30882 came from another fix (brave/brave-core#17832).

@stephendonner
Copy link

stephendonner commented Jun 8, 2023

Verified PASSED using

Brave 1.53.83 Chromium: 114.0.5735.110 (Official Build) beta (64-bit)
Revision 1c828682b85bbc70230a48f5e345489ec447373e-refs/branch-heads/5735_90@{#13}
OS Windows 10 Version 22H2 (Build 19045.3031)

Steps:

  1. installed 1.53.83
  2. launched Brave
  3. opened brave://settings/security
  4. confirmed default of Standard protection for Safe Browsing
  5. toggled it to No protection (not recommended)
  6. loaded https://www.majorgeeks.com/mg/get/fort_firewall,2.html
  7. waited
  8. confirmed auto-download Save file as... file picker was invoked
  9. clicked Save
  10. confirmed the downloads-bubble animation pointed to the downloads bubble
  11. clicked on the downloads bubble
  12. confirmed the file was marked as Blocked - dangerous
  13. repeated the above, alternating Discard and Keep scenarios

Confirmed I was prompted for and shown the downloads-bubble animation, regardless of the previous-download's blocked/unblocked status

Screencast:

major-geeks

Screenshots:

example example example example example example example example
image image (1) image (2) image (3) image (5) image (4) image (7) image (6)

@kjozwiak
Copy link
Member

@mkarolin does this need to get checked on Android? Looks like #30060 (comment) mentions to run through brave/brave-core#18243 which fixes #29651 and is labelled as OS/Desktop only. Android is a lot different re: how you download. Should we just check and make sure that you can still download without issues? Or is there any specific STR/Cases we can run through?

@simonhong
Copy link
Member

simonhong commented Jun 14, 2023

@mkarolin does this need to get checked on Android? Looks like #30060 (comment) mentions to run through brave/brave-core#18243 which fixes #29651 and is labelled as OS/Desktop only. Android is a lot different re: how you download. Should we just check and make sure that you can still download without issues? Or is there any specific STR/Cases we can run through?

@kjozwiak I think we don't need to test this more on Android. it's desktop specific issue and above @stephendonner 's test result is sufficient.

@kjozwiak
Copy link
Member

kjozwiak commented Jul 6, 2023

@mkarolin does this need to get checked on Android? Looks like #30060 (comment) mentions to run through brave/brave-core#18243 which fixes #29651 and is labelled as OS/Desktop only. Android is a lot different re: how you download. Should we just check and make sure that you can still download without issues? Or is there any specific STR/Cases we can run through?

@kjozwiak I think we don't need to test this more on Android. it's desktop specific issue and above @stephendonner 's test result is sufficient.

Thanks! I'll add QA/Pass for Android so this is removed from our list of issues that still need be verified. I won't want to remove OS/Android as this also fixes Android as per the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants