- Manually applied patch (had conflicts) from #1993

- Small refactor in siteUtil.js; trying to make sure bookmark code is consistent, because bookmarks menu will use similar code to the bookmarks toolbar.
- Subfolders now shown in menu
- Folder menu items with 0 children no longer include empty submenu
- Refactored code in menu.js; all submenus are defined above the template definition.
- Bookmark menu selections now respect secondary keys
- Fixes #1993
- Created siteUtilTest file, moved some existing tests over, and added more tests

js/commonMenu.js

@@ -13,6 +13,9 @@ const settings = require('./constants/settings')
 const getSetting = require('./settings').getSetting
 const issuesUrl = 'https://github.com/brave/browser-laptop/issues'
 const isDarwin = process.platform === 'darwin'
+const siteTags = require('./constants/siteTags')
+const siteUtil = require('./state/siteUtil')
+const eventUtil = require('./lib/eventUtil')
 
 let electron
 try {
@@ -175,7 +178,7 @@ module.exports.preferencesMenuItem = () => {
   }
 }
 
-module.exports.bookmarksMenuItem = () => {
+module.exports.bookmarksManagerMenuItem = () => {
   return {
     label: locale.translation('bookmarksManager'),
     accelerator: isDarwin ? 'CmdOrCtrl+Alt+B' : 'Ctrl+Shift+O',
@@ -250,6 +253,46 @@ module.exports.importBookmarksMenuItem = () => {
   */
 }
 
+module.exports.createBookmarkMenuItems = (bookmarks, parentFolderId) => {
+  let filteredBookmarks
+  if (parentFolderId) {
+    filteredBookmarks = bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === parentFolderId)
+  } else {
+    filteredBookmarks = bookmarks.filter((bookmark) => !bookmark.get('parentFolderId'))
+  }
+
+  var payload = []
+  filteredBookmarks.forEach((site) => {
+    if (site.get('tags').includes(siteTags.BOOKMARK) && site.get('location')) {
+      payload.push({
+        // TODO include label made from favicon. It needs to be of type NativeImage
+        // which can be made using a Buffer / DataURL / local image
+        // the image will likely need to be included in the site data
+        // there was potentially concern about the size of the app state
+        // and as such there may need to be another mechanism or cache
+        //
+        // see: https://github.com/brave/browser-laptop/issues/3050
+        label: site.get('customTitle') || site.get('title'),
+        click: (item, focusedWindow, e) => {
+          if (eventUtil.isForSecondaryAction(e)) {
+            module.exports.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_NEW_FRAME, site.get('location'), { openInForeground: !!e.shiftKey }])
+          } else {
+            module.exports.sendToFocusedWindow(focusedWindow, [messages.SHORTCUT_ACTIVE_FRAME_LOAD_URL, site.get('location')])
+          }
+        }
+      })
+    } else if (siteUtil.isFolder(site)) {
+      const folderId = site.get('folderId')
+      const submenuItems = bookmarks.filter((bookmark) => bookmark.get('parentFolderId') === folderId)
+      payload.push({
+        label: site.get('customTitle') || site.get('title'),
+        submenu: submenuItems.count() > 0 ? module.exports.createBookmarkMenuItems(bookmarks, folderId) : null
+      })
+    }
+  })
+  return payload
+}
+
 module.exports.reportAnIssueMenuItem = () => {
   return {
     label: locale.translation('reportAnIssue'),

js/components/addEditBookmark.js

@@ -28,7 +28,7 @@ class AddEditBookmark extends ImmutableComponent {
     return ['about:blank', 'about:newtab'].includes(this.props.currentDetail.get('location'))
   }
   get isFolder () {
-    return this.props.currentDetail.get('tags').includes(siteTags.BOOKMARK_FOLDER)
+    return siteUtil.isFolder(this.props.currentDetail)
   }
   updateFolders (props) {
     this.folders = siteUtil.getFolders(this.props.sites, props.currentDetail.get('folderId'))

js/components/bookmarksToolbar.js

@@ -116,7 +116,7 @@ class BookmarkToolbarButton extends ImmutableComponent {
   }
 
   get isFolder () {
-    return this.props.bookmark.get('tags').includes(siteTags.BOOKMARK_FOLDER)
+    return siteUtil.isFolder(this.props.bookmark)
   }
 
   onContextMenu (e) {
@@ -259,8 +259,8 @@ class BookmarksToolbar extends ImmutableComponent {
     contextMenus.onShowBookmarkFolderMenu(this.bookmarks, bookmark, this.activeFrame, e)
   }
   updateBookmarkData (props) {
-    this.bookmarks = props.sites
-      .filter((site) => site.get('tags').includes(siteTags.BOOKMARK) || site.get('tags').includes(siteTags.BOOKMARK_FOLDER))
+    this.bookmarks = siteUtil.getBookmarks(props.sites)
+
     const noParentItems = this.bookmarks
       .filter((bookmark) => !bookmark.get('parentFolderId'))
     let widthAccountedFor = 0

js/components/navigationBar.js

@@ -11,7 +11,7 @@ const Button = require('./button')
 const UrlBar = require('./urlBar')
 const appActions = require('../actions/appActions')
 const windowActions = require('../actions/windowActions')
-const {isSiteInList} = require('../state/siteUtil')
+const {isSiteBookmarked} = require('../state/siteUtil')
 const siteTags = require('../constants/siteTags')
 const messages = require('../constants/messages')
 const settings = require('../constants/settings')
@@ -76,11 +76,11 @@ class NavigationBar extends ImmutableComponent {
 
   get bookmarked () {
     return this.props.activeFrameKey !== undefined &&
-      isSiteInList(this.props.sites, Immutable.fromJS({
+      isSiteBookmarked(this.props.sites, Immutable.fromJS({
         location: this.props.location,
         partitionNumber: this.props.partitionNumber,
         title: this.props.title
-      }), siteTags.BOOKMARK)
+      }))
   }
 
   get titleMode () {
@@ -109,11 +109,12 @@ class NavigationBar extends ImmutableComponent {
   componentDidUpdate (prevProps) {
     // Update the app menu to reflect whether the current page is bookmarked
     const prevBookmarked = this.props.activeFrameKey !== undefined &&
-      isSiteInList(prevProps.sites, Immutable.fromJS({
+      isSiteBookmarked(prevProps.sites, Immutable.fromJS({
         location: prevProps.location,
         partitionNumber: prevProps.partitionNumber,
         title: prevProps.title
-      }), siteTags.BOOKMARK)
+      }))
+
     if (this.bookmarked !== prevBookmarked) {
       ipc.send(messages.UPDATE_APP_MENU, {bookmarked: this.bookmarked})
     }

js/contextMenus.js

@@ -110,7 +110,7 @@ function urlBarTemplateInit (searchDetail, activeFrame, e) {
 
 function tabsToolbarTemplateInit (activeFrame, closestDestinationDetail, isParent) {
   const menu = [
-    CommonMenu.bookmarksMenuItem(),
+    CommonMenu.bookmarksManagerMenuItem(),
     CommonMenu.bookmarksToolbarMenuItem(),
     CommonMenu.separatorMenuItem
   ]
@@ -213,7 +213,7 @@ function downloadsToolbarTemplateInit (downloadId, downloadItem) {
 
 function bookmarkTemplateInit (siteDetail, activeFrame) {
   const location = siteDetail.get('location')
-  const isFolder = siteDetail.get('tags').includes(siteTags.BOOKMARK_FOLDER)
+  const isFolder = siteUtil.isFolder(siteDetail)
   const template = []
 
   if (!isFolder) {
@@ -573,7 +573,7 @@ function hamburgerTemplateInit (location, e) {
     {
       label: locale.translation('bookmarks'),
       submenu: [
-        CommonMenu.bookmarksMenuItem(),
+        CommonMenu.bookmarksManagerMenuItem(),
         CommonMenu.bookmarksToolbarMenuItem(),
         CommonMenu.separatorMenuItem,
         CommonMenu.importBookmarksMenuItem()

js/state/siteUtil.js

@@ -8,6 +8,18 @@ const settings = require('../constants/settings')
 const getSetting = require('../settings').getSetting
 const urlParse = require('url').parse
 
+const isBookmark = (tags) => {
+  return tags && tags.includes(siteTags.BOOKMARK)
+}
+
+const isBookmarkFolder = (tags) => {
+  if (!tags) {
+    return false
+  }
+  return typeof tags === 'string' && tags === siteTags.BOOKMARK_FOLDER ||
+    tags && typeof tags !== 'string' && tags.includes(siteTags.BOOKMARK_FOLDER)
+}
+
 /**
  * Obtains the index of the location in sites
  *
@@ -16,10 +28,11 @@ const urlParse = require('url').parse
  * @return index of the site or -1 if not found.
  */
 module.exports.getSiteIndex = function (sites, siteDetail, tags) {
-  let isBookmarkFolder = typeof tags === 'string' && tags === siteTags.BOOKMARK_FOLDER ||
-    tags && typeof tags !== 'string' && tags.includes(siteTags.BOOKMARK_FOLDER)
-  if (isBookmarkFolder) {
-    return sites.findIndex((site) => site.get('folderId') === siteDetail.get('folderId') && site.get('tags').includes(siteTags.BOOKMARK_FOLDER))
+  if (!sites || !siteDetail) {
+    return -1
+  }
+  if (isBookmarkFolder(tags)) {
+    return sites.findIndex((site) => isBookmarkFolder(site.get('tags')) && site.get('folderId') === siteDetail.get('folderId'))
   }
   return sites.findIndex((site) => site.get('location') === siteDetail.get('location') && (site.get('partitionNumber') || 0) === (siteDetail.get('partitionNumber') || 0))
 }
@@ -31,12 +44,12 @@ module.exports.getSiteIndex = function (sites, siteDetail, tags) {
  * @param siteDetail The site to check if it's in the specified tag
  * @return true if the location is already bookmarked
  */
-module.exports.isSiteInList = function (sites, siteDetail, tag) {
-  const index = module.exports.getSiteIndex(sites, siteDetail, tag)
+module.exports.isSiteBookmarked = function (sites, siteDetail) {
+  const index = module.exports.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK)
   if (index === -1) {
     return false
   }
-  return sites.get(index).get('tags').includes(tag)
+  return isBookmark(sites.get(index).get('tags'))
 }
 
 const getNextFolderIdItem = (sites) =>
@@ -56,6 +69,10 @@ const getNextFolderIdItem = (sites) =>
   })
 
 module.exports.getNextFolderId = (sites) => {
+  const defaultFolderId = 0
+  if (!sites) {
+    return defaultFolderId
+  }
   const maxIdItem = getNextFolderIdItem(sites)
   return (maxIdItem ? (maxIdItem.get('folderId') || 0) : 0) + 1
 }
@@ -97,7 +114,7 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
     location: siteDetail.get('location'),
     // We don't want bookmarks and other site info being renamed on users if they already exist
     // The name should remain the same while it is bookmarked forever.
-    title: oldSite && tags.includes(siteTags.BOOKMARK) ? oldSite.get('title') : siteDetail.get('title')
+    title: oldSite && isBookmark(tags) ? oldSite.get('title') : siteDetail.get('title')
   })
   if (folderId) {
     site = site.set('folderId', Number(folderId))
@@ -261,7 +278,10 @@ module.exports.isEquivalent = function (siteDetail1, siteDetail2) {
  * @return true if the site detail is a folder.
  */
 module.exports.isFolder = function (siteDetail) {
-  return siteDetail.get('tags').includes(siteTags.BOOKMARK_FOLDER)
+  if (siteDetail) {
+    return isBookmarkFolder(siteDetail.get('tags'))
+  }
+  return false
 }
 
 /**
@@ -330,6 +350,15 @@ module.exports.hasNoTagSites = function (sites) {
   return sites.findIndex((site) => !site.get('tags') || site.get('tags').size === 0) !== -1
 }
 
+/**
+ * Returns all sites that have a bookmark tag.
+ * @param sites The application state's Immutable sites list.
+ */
+
+module.exports.getBookmarks = function (sites) {
+  return sites.filter((site) => isBookmarkFolder(site.get('tags')) || isBookmark(site.get('tags')))
+}
+
 /**
  * Gets a site origin (scheme + hostname + port) from a URL or null if not
  * available.

test/unit/state/siteSettingsTest.js

@@ -1,7 +1,6 @@
 /* global describe, before, it */
 
 const siteSettings = require('../../../js/state/siteSettings')
-const siteUtil = require('../../../js/state/siteUtil')
 const assert = require('assert')
 const Immutable = require('immutable')
 let siteSettingsMap = new Immutable.Map()
@@ -161,32 +160,3 @@ describe('siteSettings', function () {
     })
   })
 })
-
-describe('siteUtil', function () {
-  describe('gets URL origin', function () {
-    it('gets URL origin for simple url', function () {
-      assert.strictEqual(siteUtil.getOrigin('https://abc.bing.com'), 'https://abc.bing.com')
-    })
-    it('gets URL origin for url with port', function () {
-      assert.strictEqual(siteUtil.getOrigin('https://bing.com:443/?test=1#abc'), 'https://bing.com:443')
-    })
-    it('gets URL origin for IP host', function () {
-      assert.strictEqual(siteUtil.getOrigin('http://127.0.0.1:443/?test=1#abc'), 'http://127.0.0.1:443')
-    })
-    it('gets URL origin for slashless protocol URL', function () {
-      assert.strictEqual(siteUtil.getOrigin('about:test/foo'), 'about:test')
-    })
-    it('returns null for invalid URL', function () {
-      assert.strictEqual(siteUtil.getOrigin('abc'), null)
-    })
-    it('returns null for empty URL', function () {
-      assert.strictEqual(siteUtil.getOrigin(''), null)
-    })
-    it('returns null for null URL', function () {
-      assert.strictEqual(siteUtil.getOrigin(null), null)
-    })
-    it('returns correct result for URL with hostname that is a scheme', function () {
-      assert.strictEqual(siteUtil.getOrigin('http://http/test'), 'http://http')
-    })
-  })
-})