-
Notifications
You must be signed in to change notification settings - Fork 888
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
fixed download item filename overlap #9759
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, although wondering if there's a lint issue. 👍 when all CI passes
bcc708f
to
a62f6f3
Compare
@petemill does look wierd; but I ran clang-format on the above patch and it came out the same 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a62f6f3
to
29bac7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice find - we just need to make it a bit more robust with the view lifecycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful! Thanks for taking the time to move the logic into UpdateLabels
😄
I didn't see any artifacts (although didn't notice those before either) after patching this in / compiling / running it - and code is nice and clean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small refactor, otherwise looks good
29bac7c
to
114e143
Compare
Verification PASSED on
Example from https://testsafebrowsing.appspot.com via the |
Resolves brave/brave-browser#17313
The overlap was caused by a recent change we made on the UX of the download item view.
The duped filename was setting itself visible in all modes. There are various modes to take in to consideration however, we only care about the normal mode.
This is what upstream is doing: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/download/download_item_view.cc;l=816;bpv=0;bpt=1
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-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 tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
A detailed test plan is described in the issue.