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

Undefined properties and nested folders handling #5298

Merged
merged 1 commit into from
Nov 1, 2016

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Oct 31, 2016

  • 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).

fix #5291
fix #5293

requires brave/muon#85

Auditors: @bridiver, @bbondy

Test Plan:

  1. Go to about:preferences#security
  2. Select Lastpass as password manager
  3. Login to Lastpass
  4. Right click to show context menu
  5. Nested menu of Lastpass should show

Screen shots


brave
screen shot 2016-10-31 at 18 23 51

chrome
screen shot 2016-10-31 at 14 16 23

@darkdh darkdh added this to the 0.12.8dev milestone Oct 31, 2016
@darkdh darkdh force-pushed the context-menus-refined branch from 8970035 to 64d594e Compare October 31, 2016 22:21
@@ -1093,7 +1093,8 @@ function mainTemplateInit (nodeProps, frame) {
})

const passwordManager = getActivePasswordManager()
if (passwordManager.get('extensionId')) {
if (passwordManager.get('extensionId') &&
passwordManager.get('displayName') === 'Dashlane') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this only for dashlane?

Copy link
Member Author

Choose a reason for hiding this comment

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

because dashlane doesn't create context menus and other's do which cause repeated entries shows in #5291

Copy link
Collaborator

Choose a reason for hiding this comment

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

we can just remove these then. The only reason we created context menus for the password managers was because we didn't have contextMenu api support.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I see. removed!

fix #5291
fix #5293

requires brave/muon#85

Auditors: @bridiver, @bbondy

Test Plan:
1. Go to about:preferences#security
2. Select Lastpass as password manager
3. Login to Lastpass
4. Right click to show context menu
5. Nested menu of Lastpass should show
@darkdh darkdh force-pushed the context-menus-refined branch from 64d594e to 6fb27a1 Compare November 1, 2016 00:11
@bbondy
Copy link
Member

bbondy commented Nov 1, 2016

++

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