Add Importer support #4049

darkdh · 2016-09-16T08:51:16Z

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

Test Plan:

fix #428
partial fix #2107

Auditors: @bridiver, @bbondy, @bsclifton

bsclifton · 2016-09-16T20:50:58Z

app/browser/menu.js

-    CommonMenu.bookmarksToolbarMenuItem(),
-    CommonMenu.separatorMenuItem,
-    CommonMenu.importBookmarksMenuItem()
+    CommonMenu.bookmarksToolbarMenuItem()
  ]


there is some commented out code here in the menu.js that I think you can delete, now that you've added import functionality. Search for locale.translation('importFrom')

bsclifton · 2016-09-16T20:56:00Z

app/importer.js

+importer.on('add-homepage', (e, detail) => {
+})
+
+importer.on('add-bookmarks', (e, bookmarks, topLevelFolder) => {


this handler has a LOT going on in it... Any chance we could break this (or parts of it) out into a method? Looks generic enough (not dependent on electron) to put into a utils class, which means we could eventually unit test it too 😄

But the handler is for electron dependent bookmarks object and whole block is meant to handle it specifically. So I am not sure that we can pull out a generic method from it.

ah- makes sense. I'm OK w/ it as-is 😄

bsclifton · 2016-09-16T20:57:14Z

docs/appActions.md

+
+**Parameters**
+
+**selected**: `object`, the browser data to import as per doc/state.md's importBroserDataSelected


definitely not a big deal, just a misspelling here:
s/importBroserDataSelected/importBrowserDataSelected

bsclifton · 2016-09-16T20:57:51Z

docs/state.md

+      cookies: boolean
+    }
+  ],
+  importBroserDataSelected: {


whoops, another one:
s/importBroserDataSelected/importBrowserDataSelected

bsclifton · 2016-09-16T21:05:21Z

js/components/importBrowserDataPanel.js

@@ -0,0 +1,108 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


would it be possible to move this to the new directory structure?
https://github.com/brave/browser-laptop/blob/master/docs/directoryStructure.md

So for example, this would move to:
app/renderer/components

Great job on the control 😄

bsclifton · 2016-09-16T21:12:24Z

Left some comments... great work 😄

darkdh · 2016-09-17T12:20:33Z

Thanks for your feedback, @bsclifton. I've addressed it in ec45bdf.

bridiver · 2016-09-21T00:58:09Z

app/filtering.js

@@ -693,3 +693,14 @@ module.exports.clearAutofillData = () => {
    ses.autofill.clearAutofillData()
  }
 }
+
+module.exports.setCookie = (cookie) => {


this method should be moved out of filtering and only called on the default session

addressed it in 1dee63d.

bridiver · 2016-09-21T00:58:54Z

app/common/commonMenu.js

-            module.exports.sendToFocusedWindow(BrowserWindow.getAllWindows()[0], [messages.IMPORT_BOOKMARKS]), 100)
-        })
-        return
+      if (process.type === 'browser') {


you can avoid this check by making it an appAction
see https://github.com/brave/browser-laptop/blob/master/app/browser/basicAuth.js

bridiver · 2016-09-21T01:02:00Z

js/components/main.js

@@ -374,6 +376,11 @@ class Main extends ImmutableComponent {
      windowActions.setContextMenuDetail()
    })

+    ipc.on(messages.IMPORTER_LIST, (e, detail) => {


this would also be better as an appAction. We want to avoid adding more ipc handlers

bridiver · 2016-09-21T01:05:45Z

js/components/main.js

@@ -374,6 +376,11 @@ class Main extends ImmutableComponent {
      windowActions.setContextMenuDetail()
    })

+    ipc.on(messages.IMPORTER_LIST, (e, detail) => {
+      windowActions.setImportBrowserDataDetail(detail)


ideally actions should not be setters, they should describe the actual action. In this case something like appAction.supportedBrowsersUpdated which would then be handled in the windowStore and should call helper methods to update the state (don't call actions from actions).

bridiver · 2016-09-21T01:06:45Z

js/components/main.js

@@ -532,6 +539,10 @@ class Main extends ImmutableComponent {
    windowActions.setClearBrowsingDataDetail()
  }

+  onHideImportBrowserDataPanel () {
+    windowActions.setImportBrowserDataDetail()


along the same lines as the previous comment this would be something like windowActions.importDataPanelHidden

darkdh · 2016-09-21T02:35:35Z

I will do appAction refactoring in follow-up commit. recorded in #2107 (comment)

bridiver · 2016-09-21T04:14:59Z

sounds good! Only the setCookie thing is critical for merging

@bridiver

fix #2107 requires brave/muon#57 Auditors: @bridiver, @bbondy, @bsclifton

bbondy · 2016-09-21T15:49:23Z

merging!

bsclifton reviewed Sep 16, 2016

View reviewed changes

cndouglas mentioned this pull request Sep 17, 2016

Bookmark issue #4052

Closed

bridiver reviewed Sep 21, 2016

View reviewed changes

luixxiul added this to the 0.12.2dev milestone Sep 21, 2016

darkdh force-pushed the import-browser-data branch from 1dee63d to 3b94788 Compare September 21, 2016 02:34

bbondy assigned darkdh Sep 21, 2016

Add Importer support

2d91041

fix #2107 requires brave/muon#57 Auditors: @bridiver, @bbondy, @bsclifton

darkdh force-pushed the import-browser-data branch from 3b94788 to 2d91041 Compare September 21, 2016 04:43

bbondy merged commit 7352955 into master Sep 21, 2016

This was referenced Sep 22, 2016

Manual tests for OSX 0.12.2 RC1 #4163

Closed

Manual tests for Windows ia-32 0.12.2 RC1 #4164

Closed

Manual tests for Ubuntu 0.12.2 RC1 #4165

Closed

Manual tests for Windows x64 0.12.2 RC1 #4166

Closed

luixxiul added the QA/checked-Win32 label Sep 22, 2016

This was referenced Sep 22, 2016

Manual tests for OS X 0.12.2 RC2 #4204

Closed

Manual tests for Windows ia-32 0.12.2 RC2 #4205

Closed

Manual tests for Windows x64 0.12.2 RC2 #4206

Closed

Manual tests for Ubuntu 0.12.2 RC2 #4207

Closed

aekeus added the QA/checked-Linux label Sep 23, 2016

darkdh deleted the import-browser-data branch September 23, 2016 02:30

alexwykoff added the QA/checked-macOS label Sep 23, 2016

This was referenced Sep 24, 2016

Provide a way to export bookmarks #1002

Closed

Manual tests for Windows ia-32 0.12.2 RC4 #4255

Closed

Manual tests for Windows x64 0.12.2 RC4 #4256

Closed

Manual tests for OS X 0.12.2 RC4 #4257

Closed

Manual tests for Ubuntu 0.12.2 RC4 #4258

Closed

cndouglas mentioned this pull request Sep 24, 2016

Imported Bookmarks from Firefox do not appear in Bookmarks Menu #2830

Closed

alexwykoff added the QA/checked-Win64 label Sep 24, 2016

luixxiul added the feature/importer label Sep 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Importer support #4049

Add Importer support #4049

darkdh commented Sep 16, 2016 •

edited

Loading

bsclifton Sep 16, 2016

bsclifton Sep 16, 2016

darkdh Sep 17, 2016

bsclifton Sep 18, 2016

bsclifton Sep 16, 2016

bsclifton Sep 16, 2016

bsclifton Sep 16, 2016

bsclifton commented Sep 16, 2016

darkdh commented Sep 17, 2016

bridiver Sep 21, 2016

darkdh Sep 21, 2016

bridiver Sep 21, 2016

bridiver Sep 21, 2016

bridiver Sep 21, 2016

bridiver Sep 21, 2016

darkdh commented Sep 21, 2016 •

edited

Loading

bridiver commented Sep 21, 2016

bbondy commented Sep 21, 2016


		Parameters

		selected: `object`, the browser data to import as per doc/state.md's importBroserDataSelected

		@@ -0,0 +1,108 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

Add Importer support #4049

Add Importer support #4049

Conversation

darkdh commented Sep 16, 2016 • 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

bsclifton commented Sep 16, 2016

darkdh commented Sep 17, 2016

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

darkdh commented Sep 21, 2016 • edited Loading

bridiver commented Sep 21, 2016

bbondy commented Sep 21, 2016

darkdh commented Sep 16, 2016 •

edited

Loading

darkdh commented Sep 21, 2016 •

edited

Loading