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

Decrease download shelf height and show origin url on hover #9521

Merged
merged 1 commit into from
Aug 2, 2021
Merged

Decrease download shelf height and show origin url on hover #9521

merged 1 commit into from
Aug 2, 2021

Conversation

nullhook
Copy link
Contributor

@nullhook nullhook commented Jul 21, 2021 •
edited
Loading

This decreases the whole download shelf height and hides the origin url by default. The origin url is only visible on hover and on tab focus.

TransparentButton is overlaid on top of DownloadItemView and the static cast is needed to access specific events such as: OnViewFocused and OnViewBlurred which isn't part of parent()View itself. It's part of ViewObserver class.

Here is the definition of TransparentButton: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/download/download_item_view.cc;l=146

Resolves brave/brave-browser#1638

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@nullhook nullhook requested review from a team as code owners July 21, 2021 18:54
bsclifton
bsclifton approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

These updates look great! 😄 I tested a few scenarios (downloading a file quick, large file, resize, etc). The chevron area shows the filename when moused over which is perfect (since person would usually use that for open in folder). Rest of shelf item shows the origin in a very readable way

The patches LGTM! Would be great to get review from @brave/patch-reviewers in case I missed something

@bsclifton
Copy link
Member

@nullhook can you take a look at brave/brave-browser#5533? I would think this is fixed; but curious if you're able to repro before your change

@nullhook nullhook force-pushed the bug/download-shelf branch 2 times, most recently from 3041756 to 556fa40 Compare July 28, 2021 20:12
goodov
goodov requested changes Jul 29, 2021
View reviewed changes
Copy link
Member

@goodov goodov left a comment

Choose a reason for hiding this comment

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

#include need to be fixed.

@nullhook nullhook force-pushed the bug/download-shelf branch 2 times, most recently from 52e6505 to 6dfd540 Compare July 31, 2021 04:18
@nullhook nullhook requested a review from goodov July 31, 2021 19:38
@nullhook nullhook merged commit e849205 into master Aug 2, 2021
@nullhook nullhook deleted the bug/download-shelf branch August 2, 2021 05:35
@nullhook nullhook added this to the 1.29.x - Nightly milestone Aug 2, 2021
mariospr added a commit that referenced this pull request Aug 6, 2021
@mariospr
Fix crash downloading large files due to the DownloadItemView
7a7565f 
When downloading a large file the DownloadItemView is supposed to show
certain information about the item being downloaded, such as the name
and the current status. Additionally, it's also meant to show the URL
but, due to changes done recently in PR 9521 [1] (see issue in [2]),
Brave contains changes to only show the URL when hovering over the
download item view, to save space.

Turns out that these changes were causing a crash after a the change
below landed upstream and the reason seems to be that our chromium_src
override was wrongly redefining the Button symbol way beyond the scope
of download_item_view.cc, causing the crash.

This change includes the necessary headers from download_item_view.cc
to avoid this Button redefinition from propagating any further, and
also makes the most of the effort to fix some small things in the
override file, such as calling the parent methods of the overriden
methods from the Button class, or adding some extra documentation.

Last, this change also fixes 2 crashing browser tests:

  * BraveCheckClientDownloadRequestBaseBrowserTest.FilterRequest
  * SavePackageFinishedObserverBrowserTest.Success

[1] #9521
[2] brave/brave-browser#1638

Chromium change:

https://source.chromium.org/chromium/chromium/src/+/03f006384e02fb387c177af07a3df9792e364e18

commit 03f006384e02fb387c177af07a3df9792e364e18
Author: Peter Kasting <[email protected]>
Date:   Thu Jul 22 00:45:59 2021 +0000

    Clicking the download item context button should close any open menu.

    This is what other buttons in Chrome do: repeatedly clicking the button
    toggles it between open and closed, instead of flickering it closed and
    instantly reopening (the current behavior).

    Bug: 1031682
@mariospr mariospr mentioned this pull request Aug 6, 2021
Closed
mariospr added a commit that referenced this pull request Aug 6, 2021
@mariospr
Fix crash downloading large files due to the DownloadItemView
faac518 
When downloading a large file the DownloadItemView is supposed to show
certain information about the item being downloaded, such as the name
and the current status. Additionally, it's also meant to show the URL
but, due to changes done recently in PR 9521 [1] (see issue in [2]),
Brave contains changes to only show the URL when hovering over the
download item view, to save space.

Turns out that these changes were causing a crash after a the change
below landed upstream and the reason seems to be that our chromium_src
override was wrongly redefining the Button symbol way beyond the scope
of download_item_view.cc, causing the crash.

This change includes the necessary headers from download_item_view.cc
to avoid this Button redefinition from propagating any further, and
also makes the most of the effort to fix some small things in the
override file, such as calling the parent methods of the overriden
methods from the Button class, or adding some extra documentation.

Last, this change also fixes 2 crashing browser tests:

  * BraveCheckClientDownloadRequestBaseBrowserTest.FilterRequest
  * SavePackageFinishedObserverBrowserTest.Success

[1] #9521
[2] brave/brave-browser#1638

Chromium change:

https://source.chromium.org/chromium/chromium/src/+/03f006384e02fb387c177af07a3df9792e364e18

commit 03f006384e02fb387c177af07a3df9792e364e18
Author: Peter Kasting <[email protected]>
Date:   Thu Jul 22 00:45:59 2021 +0000

    Clicking the download item context button should close any open menu.

    This is what other buttons in Chrome do: repeatedly clicking the button
    toggles it between open and closed, instead of flickering it closed and
    instantly reopening (the current behavior).

    Bug: 1031682
@mariospr mariospr mentioned this pull request Aug 6, 2021
Merged
24 tasks
mariospr added a commit that referenced this pull request Aug 9, 2021
@mariospr
Fix crash downloading large files due to the DownloadItemView
691f1e9 
When downloading a large file the DownloadItemView is supposed to show
certain information about the item being downloaded, such as the name
and the current status. Additionally, it's also meant to show the URL
but, due to changes done recently in PR 9521 [1] (see issue in [2]),
Brave contains changes to only show the URL when hovering over the
download item view, to save space.

Turns out that these changes were causing a crash after a the change
below landed upstream and the reason seems to be that our chromium_src
override was wrongly redefining the Button symbol way beyond the scope
of download_item_view.cc, causing the crash.

This change includes the necessary headers from download_item_view.cc
to avoid this Button redefinition from propagating any further, and
also makes the most of the effort to fix some small things in the
override file, such as calling the parent methods of the overriden
methods from the Button class, or adding some extra documentation.

Last, this change also fixes 2 crashing browser tests:

  * BraveCheckClientDownloadRequestBaseBrowserTest.FilterRequest
  * SavePackageFinishedObserverBrowserTest.Success

[1] #9521
[2] brave/brave-browser#1638

Chromium change:

https://source.chromium.org/chromium/chromium/src/+/03f006384e02fb387c177af07a3df9792e364e18

commit 03f006384e02fb387c177af07a3df9792e364e18
Author: Peter Kasting <[email protected]>
Date:   Thu Jul 22 00:45:59 2021 +0000

    Clicking the download item context button should close any open menu.

    This is what other buttons in Chrome do: repeatedly clicking the button
    toggles it between open and closed, instead of flickering it closed and
    instantly reopening (the current behavior).

    Bug: 1031682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goodov goodov goodov approved these changes

@petemill petemill Awaiting requested review from petemill

@bridiver bridiver Awaiting requested review from bridiver

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@nullhook nullhook

Labels
bug UI
Projects
None yet
Milestone
1.29.x - Release
Development

Successfully merging this pull request may close these issues.

Decrease height of download bar by showing source on hover
3 participants
@nullhook @bsclifton @goodov