Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix lock/unlock icon from disappearing when downloading from a site #13541

Closed
wants to merge 3 commits into from

Conversation

MargarytaChepiga
Copy link
Contributor

Fixes issue #12248

Before:
before

After:
fix 3-1

Tested on:

  • Secure websites with secure downloads
  • Secure websites with insecure downloads
  • Insecure websites
  • PDF downloads
  • Twitter ( made sure to not cause this issue)

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. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@@ -591,6 +591,10 @@ class Frame extends React.Component {
// update the load state; otherwise it will not show the security
// icon.
return
} else if (!(getBaseUrl === getTargetAboutUrl('about:newtab'))) {
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this always return true since getBaseUrl is a function and getTargetAboutUrl('about:newtab') is a string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right actually. Sorry for that, I am gonna figure it out asap and update the PR. Okay?

@MargarytaChepiga
Copy link
Contributor Author

@diracdeltas I updated the fix. According to the various console logs I had everywhere, this works. Let me know what you think 😄

@codecov-io
Copy link

codecov-io commented Mar 22, 2018

Codecov Report

Merging #13541 into master will increase coverage by <.01%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master   #13541      +/-   ##
==========================================
+ Coverage    56.6%    56.6%   +<.01%     
==========================================
  Files         284      285       +1     
  Lines       28675    28920     +245     
  Branches     4744     4782      +38     
==========================================
+ Hits        16231    16371     +140     
- Misses      12444    12549     +105
Flag Coverage Δ
#unittest 56.6% <14.28%> (ø) ⬆️
Impacted Files Coverage Δ
js/stores/windowStore.js 27.9% <14.28%> (-0.86%) ⬇️
app/renderer/components/preferences/pluginsTab.js 40.32% <0%> (-8.7%) ⬇️
js/stores/appStoreRenderer.js 91.66% <0%> (-8.34%) ⬇️
...r/components/preferences/payment/enabledContent.js 67.18% <0%> (-8.18%) ⬇️
app/renderer/components/reduxComponent.js 57.75% <0%> (-3.45%) ⬇️
app/browser/api/ledgerNotifications.js 69.87% <0%> (-1.08%) ⬇️
app/common/lib/ledgerUtil.js 93.38% <0%> (-0.82%) ⬇️
app/filtering.js 17.61% <0%> (-0.57%) ⬇️
app/browser/menu.js 52.15% <0%> (-0.57%) ⬇️
... and 13 more

@diracdeltas
Copy link
Member

please remove extra whitespace so that the linter passes: https://travis-ci.org/brave/browser-laptop/jobs/356631639

otherwise i can't find anything wrong with this PR, but it seems somewhat risky because WINDOW_WEBVIEW_LOAD_START used to be necessary for all webview loads. maybe @bsclifton or @petemill can do a review?

@diracdeltas
Copy link
Member

actually i did find a bug - it seems this PR makes the loading indicator disappear from tabs

ex:

  1. do a google search
  2. click on any link
  3. the loading indicator is not shown on the tab while the site is loading

@petemill petemill self-requested a review March 27, 2018 19:21
@MargarytaChepiga
Copy link
Contributor Author

Thank you @diracdeltas! I fixed the lint error. Now I am trying to figure out how to fix the loading indicator, or how to fix the icon from disappearing without causing a bug to the loading indicator.

@diracdeltas
Copy link
Member

@MargarytaChepiga the issue is that https://github.com/brave/browser-laptop/pull/13541/files#diff-c3c556f1ee31674844d6d0be95748d8eR596 is too general (it applies to all loads that aren't from the newtab page). you want to look for some condition that only applies to downloads

@MargarytaChepiga
Copy link
Contributor Author

MargarytaChepiga commented Apr 2, 2018

I think I found a way to fix it. Everything seems to work fine, including the loading indicator. @diracdeltas can you please take a look, and let me know what you think? Thank you 😄

P.S I checked for the lint errors and eliminated them. However, I noticed that I am failing tests.

@diracdeltas
Copy link
Member

thanks for the effort! we decided to close this PR since the issue is not present in the new codebase (brave-core)

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

Successfully merging this pull request may close these issues.

lock/unlock icon disappears when downloading from a site
4 participants