Add extensionsTab to preferences page #8065

cezaraugusto · 2017-04-04T19:12:48Z

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

Add extensionsTab to preferences page

Test plan:

Automated test must pass:

extension state

npm run test -- --grep="extensionUninstalled"

extensionsReducer

npm run test -- --grep="extensionsReducer"

extensionsTab

npm run test -- --grep="ExtensionsTab component"

QA Steps:

Go to extensions tab
Enabling an extension should enable it
Disabling an extension should disable it
Removing an extension should remove its folder (you can see its path logged on console)
Removed extension should not appear in UI, and should be disabled after removal
Enabling a removed extension should install it again
Enabling another password manager should disable the first
Disabling all password managers should set "built-in" password manager to active
You can't remove built-in (webtorrent and sync) extensions -- update per @bradleyrichter request Sync is no longer included

jonathansampson · 2017-04-06T17:47:49Z

@cezaraugusto Rebase, please? :)

jonathansampson · 2017-04-06T18:00:58Z

Perhaps about:extensions should redirect to about:preferences#extensions with this change?

jonathansampson · 2017-04-06T18:22:15Z

I don't feel too great about including these extensions images in the browser itself. I feel as though we should get those from the extension store instead. Otherwise, we'll have to do extra work each time we support a new extension.

cezaraugusto · 2017-04-07T18:45:29Z

rebased. Added more tests.

cezaraugusto · 2017-04-07T18:46:56Z

@jonathansampson I agree with you regarding extensions images but at this moment there's no way of having their data unless it's installed by default.

Ideally they would have their store with name, description, and image, even when they're not installed by default (built-in). At that moment we can also remove dummyContent I included, and process of adding new extensions would be easier

jonathansampson · 2017-04-08T01:13:57Z

@cezaraugusto Good point. Can't show an extension's image if the image hasn't yet been loaded with the extension. We may need to have a bigger-vision discussion about this. Current approach is clearly necessary, but won't scale. My apologies :)

Can you add an image for Honey?

jonathansampson · 2017-04-08T01:20:47Z

app/extensions/brave/locales/en-US/extensions.properties

+l10nSyncDesc=
+l10nWebtorrent= Torrent Viewer
+l10nWebtorrentDesc=
+l10nVimium= Vimium


We'll be shipping Honey soon (perhaps in 0.14.2). Can we include it?
https://chrome.google.com/webstore/detail/honey/bmnlcjabgnpnenekpadlanbbkooimhnj

not sure which is going to land first but will include once Honey lands

jonathansampson · 2017-04-08T01:28:28Z

app/renderer/components/settings.js

+      // switch back password to unmanaged (null)
+      e.target.value
+        ? aboutActions.changeSetting(settings.ACTIVE_PASSWORD_MANAGER, this.props.prefKey)
+        : aboutActions.changeSetting(settings.ACTIVE_PASSWORD_MANAGER, null)


Similar pattern in extensionsTab.js. There false was used; here null is being used. Can we go with one or the other, so there isn't any confusion about whether or not the two behave differently?

+ isPasswordManager(extensionId) + ? aboutActions.changeSetting(settings.ACTIVE_PASSWORD_MANAGER, extensionId) + : aboutActions.changeSetting(getExtensionSetting(extensionId), false)

yes I'll update, thanks for noticing

jonathansampson · 2017-04-08T01:29:26Z

app/renderer/lib/extensionsUtil.js

+const pocket = config.PocketExtensionId
+const sync = config.syncExtensionId
+const webtorrent = config.torrentExtensionId
+const vimium = config.vimiumExtensionId


Please add Honey :)

jonathansampson · 2017-04-08T01:29:52Z

app/renderer/lib/extensionsUtil.js

+    description: 'l10nPocketDesc',
+    icon: 'img/extensions/pocket-128.png'
+  }
+  // { id: 'vimium' // TBD }


Please add Honey.

jonathansampson · 2017-04-08T01:30:49Z

app/renderer/lib/extensionsUtil.js

+    case vimium:
+      extensionSetting = settings.VIMIUM_ENABLED
+      break
+    default:


Please add Honey.

jonathansampson · 2017-04-08T01:35:42Z

js/components/main.js

@@ -1339,7 +1339,7 @@ class Main extends ImmutableComponent {
                        .includes(siteTags.BOOKMARK)) || emptyMap
                  : null}
                history={this.bindHistory(frame)}
-                extensions={frame.get('location') === 'about:extensions'
+                extensions={['about:extensions', 'about:preferences'].includes(getBaseUrl(frame.get('location')))


Should we continue to support about:extensions after this?

jonathansampson

Left a handful of single comments here and there. Really great PR. Primary theme is forthcoming support for Honey.

bbondy · 2017-04-10T03:05:07Z

@cezaraugusto pls rebase this one when you get a chance. Thanks!

cezaraugusto · 2017-04-10T04:06:03Z

ok rebased 👍

bsclifton

Since the header style didn't match, I created a follow up issue. We can revisit updating all the styles to this new orange color / style 😄 #8165

bsclifton · 2017-04-05T05:57:08Z

app/common/state/extensionState.js

+    // so we add a prop called 'excluded' and use it to hide extension on UI
+    if (extension) {
+      return state.setIn(['extensions', extensionId], extension.set('excluded', true))
+    } else {


instead of an else, this could be cleaner to just return state (no else) 😄

bsclifton · 2017-04-05T05:57:57Z

app/extensions/brave/locales/en-US/extensions.properties

@@ -3,3 +3,29 @@ extensions=Extensions
 extensionIdLabel=ID:
 extensionPathLabel=Path:
 extensionPermissionsLabel=Permissions:
+notifyNewExtensionsAdded=Notifiy me when new extensions are added


Misspelling here; s/Notifiy/Notify

bsclifton · 2017-04-05T06:03:13Z

app/renderer/lib/extensionsUtil.js

+ * @param {String} extensionId - The current extension ID
+ * @returns {String} The given setting option
+ */
+module.exports.getExtensionSetting = (extensionId) => {


This method name isn't entirely accurate, since it's getting the key used to do the lookup in the settings... maybe we can change the name to something like getExtensionKey?

bsclifton · 2017-04-05T06:05:42Z

test/unit/app/common/state/extensionStateTest.js

+          url: 'some_url',
+          path: 'some/path',
+          version: '1.0',
+          description: 'epic game about an android dragon dinossaur',


@bsclifton

- Auditors: @bsclifton, @bbondy - Closes #6530

bsclifton

Changes work (and look) great! 😄 Nice job on the tests too

We'll need a follow-up for Honey once it's merged also

luixxiul · 2017-04-10T07:12:32Z

@cezaraugusto does this close #6794 as well?

bsclifton · 2017-04-10T07:47:02Z

@luixxiul it sure does! 😄 I'll go ahead and close that out

cezaraugusto added feature/extensions settings labels Apr 4, 2017

cezaraugusto added this to the 0.14.2 milestone Apr 4, 2017

cezaraugusto self-assigned this Apr 4, 2017

cezaraugusto requested review from jonathansampson, bbondy, bsclifton and NejcZdovc April 4, 2017 19:12

jonathansampson reviewed Apr 8, 2017

View reviewed changes

jonathansampson suggested changes Apr 8, 2017

View reviewed changes

luixxiul added the PR/needs-rebase ۞ label Apr 10, 2017

bsclifton added QA/test-plan-specified release-notes/include and removed PR/needs-rebase ۞ labels Apr 10, 2017

bradleyrichter mentioned this pull request Apr 10, 2017

Added extensions panel to about:preferences #6530

Closed

bsclifton reviewed Apr 10, 2017

View reviewed changes

Add extensionsTab to preferences page

5b162c9

- Auditors: @bsclifton, @bbondy - Closes #6530

Add tests for extensionsTab

caa2c39

bsclifton reviewed Apr 10, 2017

View reviewed changes

bsclifton approved these changes Apr 10, 2017

View reviewed changes

bsclifton merged commit 16ac2ab into brave:master Apr 10, 2017

bsclifton mentioned this pull request Apr 10, 2017

Remove/clear extension #6794

Closed

cezaraugusto deleted the extensions/6530 branch July 25, 2017 07:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extensionsTab to preferences page #8065

Add extensionsTab to preferences page #8065

cezaraugusto commented Apr 4, 2017 •

edited by bsclifton

Loading

jonathansampson commented Apr 6, 2017

jonathansampson commented Apr 6, 2017

jonathansampson commented Apr 6, 2017

cezaraugusto commented Apr 7, 2017

cezaraugusto commented Apr 7, 2017 •

edited

Loading

jonathansampson commented Apr 8, 2017 •

edited

Loading

jonathansampson Apr 8, 2017

cezaraugusto Apr 10, 2017

jonathansampson Apr 8, 2017

cezaraugusto Apr 10, 2017

jonathansampson Apr 8, 2017

jonathansampson Apr 8, 2017

jonathansampson Apr 8, 2017

jonathansampson Apr 8, 2017

jonathansampson left a comment

bbondy commented Apr 10, 2017

cezaraugusto commented Apr 10, 2017

bsclifton left a comment

bsclifton Apr 5, 2017

bsclifton Apr 5, 2017

bsclifton Apr 5, 2017

bsclifton Apr 5, 2017

bsclifton left a comment

luixxiul commented Apr 10, 2017

bsclifton commented Apr 10, 2017

Add extensionsTab to preferences page #8065

Add extensionsTab to preferences page #8065

Conversation

cezaraugusto commented Apr 4, 2017 • edited by bsclifton Loading

Test plan:

QA Steps:

jonathansampson commented Apr 6, 2017

jonathansampson commented Apr 6, 2017

jonathansampson commented Apr 6, 2017

cezaraugusto commented Apr 7, 2017

cezaraugusto commented Apr 7, 2017 • edited Loading

jonathansampson commented Apr 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathansampson left a comment

Choose a reason for hiding this comment

bbondy commented Apr 10, 2017

cezaraugusto commented Apr 10, 2017

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

luixxiul commented Apr 10, 2017

bsclifton commented Apr 10, 2017

cezaraugusto commented Apr 4, 2017 •

edited by bsclifton

Loading

cezaraugusto commented Apr 7, 2017 •

edited

Loading

jonathansampson commented Apr 8, 2017 •

edited

Loading