Refactors window actions, remove the whole frame prop (related to tabs)

Resolves brave#8857

Auditors: @bsclifton @bridiver

Test Plan:
- try closing a tab
- hover over the tab and close button should be displayed
- resize window and check if tabs are changing (when really small, text should be hidden)
- reorder tabs with dnd
- try tabs tear off
- try moving tabs from one window to another
- try moving tab into next tab page, by dropping it on the tab page indicator
- try moving bookmark into bookmark folder
- try reordering bookmarks
- check if tab preview is working

app/browser/reducers/sitesReducer.js

@@ -40,8 +40,10 @@ const sitesReducer = (state, action, immutableAction) => {
         state = state.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag, action.originalSiteDetail, action.skipSync))
       }
       if (action.destinationDetail) {
+        const sourceKey = siteUtil.getSiteKey(action.siteDetail)
+        const destinationKey = siteUtil.getSiteKey(action.destinationDetail)
         state = state.set('sites', siteUtil.moveSite(state.get('sites'),
-          action.siteDetail, action.destinationDetail, false, false, true))
+          sourceKey, destinationKey, false, false, true))
       }
       if (syncEnabled()) {
         state = syncUtil.updateSiteCache(state, action.destinationDetail || action.siteDetail)
@@ -56,10 +58,11 @@ const sitesReducer = (state, action, immutableAction) => {
       break
     case appConstants.APP_MOVE_SITE:
       state = state.set('sites', siteUtil.moveSite(state.get('sites'),
-        action.sourceDetail, action.destinationDetail, action.prepend,
+        action.sourceKey, action.destinationKey, action.prepend,
         action.destinationIsParent, false))
       if (syncEnabled()) {
-        state = syncUtil.updateSiteCache(state, action.destinationDetail)
+        const destinationDetail = state.getIn(['sites', action.destinationKey])
+        state = syncUtil.updateSiteCache(state, destinationDetail)
       }
       break
     case appConstants.APP_APPLY_SITE_RECORDS:

app/renderer/components/bookmarks/bookmarksToolbar.js

@@ -64,7 +64,10 @@ class BookmarksToolbar extends ImmutableComponent {
       if (droppedOn.selectedRef) {
         const isLeftSide = dnd.isLeftSide(ReactDOM.findDOMNode(droppedOn.selectedRef), e.clientX)
         const droppedOnSiteDetail = droppedOn.selectedRef.props.bookmark || droppedOn.selectedRef.props.bookmarkFolder
-        appActions.moveSite(bookmark, droppedOnSiteDetail, isLeftSide, droppedOnSiteDetail.get('tags').includes(siteTags.BOOKMARK_FOLDER) && droppedOn && droppedOn.isDroppedOn)
+        const isDestinationParent = droppedOnSiteDetail.get('tags').includes(siteTags.BOOKMARK_FOLDER) && droppedOn && droppedOn.isDroppedOn
+        const bookmarkSiteKey = siteUtil.getSiteKey(bookmark)
+        const droppedOnSiteKey = siteUtil.getSiteKey(droppedOnSiteDetail)
+        appActions.moveSite(bookmarkSiteKey, droppedOnSiteKey, isLeftSide, isDestinationParent)
       }
       return
     }

app/renderer/components/tabs/pinnedTabs.js

@@ -13,9 +13,6 @@ const Tab = require('./tab')
 const appActions = require('../../../../js/actions/appActions')
 const windowActions = require('../../../../js/actions/windowActions')
 
-// Store
-const windowStore = require('../../../../js/stores/windowStore')
-
 // Constants
 const siteTags = require('../../../../js/constants/siteTags')
 const dragTypes = require('../../../../js/constants/dragTypes')
@@ -48,13 +45,14 @@ class PinnedTabs extends ImmutableComponent {
       let droppedOnTab = dnd.closestFromXOffset(this.tabRefs.filter((node) => node && node.props.frame.get('key') !== key), clientX).selectedRef
       if (droppedOnTab) {
         const isLeftSide = dnd.isLeftSide(ReactDOM.findDOMNode(droppedOnTab), clientX)
-        const droppedOnFrameProps = windowStore.getFrame(droppedOnTab.props.frame.get('key'))
-        windowActions.moveTab(sourceDragData, droppedOnFrameProps, isLeftSide)
+        windowActions.moveTab(key, droppedOnTab.props.frame.get('key'), isLeftSide)
         if (!sourceDragData.get('pinnedLocation')) {
           appActions.tabPinned(sourceDragData.get('tabId'), true)
         } else {
-          appActions.moveSite(siteUtil.getDetailFromFrame(sourceDragData, siteTags.PINNED),
-            siteUtil.getDetailFromFrame(droppedOnFrameProps, siteTags.PINNED),
+          const sourceDetails = siteUtil.getDetailFromFrame(sourceDragData, siteTags.PINNED)
+          const destinationDetails = siteUtil.getDetailFromFrame(droppedOnTab.props.frame, siteTags.PINNED)
+          appActions.moveSite(siteUtil.getSiteKey(sourceDetails),
+            siteUtil.getSiteKey(destinationDetails),
             isLeftSide)
         }
       }

app/renderer/components/tabs/tab.js

@@ -168,7 +168,7 @@ class Tab extends ImmutableComponent {
       window.clearTimeout(this.hoverTimeout)
       windowActions.setPreviewFrame(null)
     }
-    windowActions.setTabHoverState(this.frame, false)
+    windowActions.setTabHoverState(this.props.frame.get('key'), false)
   }
 
   onMouseEnter (e) {
@@ -181,9 +181,9 @@ class Tab extends ImmutableComponent {
     // as reported here: https://github.com/brave/browser-laptop/issues/1434
     if (this.props.previewTabs) {
       this.hoverTimeout =
-        window.setTimeout(windowActions.setPreviewFrame.bind(null, this.frame), previewMode ? 0 : 200)
+        window.setTimeout(windowActions.setPreviewFrame.bind(null, this.props.frame.get('key')), previewMode ? 0 : 200)
     }
-    windowActions.setTabHoverState(this.frame, true)
+    windowActions.setTabHoverState(this.props.frame.get('key'), true)
   }
 
   onAuxClick (e) {
@@ -233,7 +233,7 @@ class Tab extends ImmutableComponent {
     setImmediate(() => {
       const currentSize = getTabBreakpoint(this.tabSize)
       // Avoid updating breakpoint when user enters fullscreen (see #7301)
-      !this.props.hasTabInFullScreen && windowActions.setTabBreakpoint(this.frame, currentSize)
+      !this.props.hasTabInFullScreen && windowActions.setTabBreakpoint(this.props.frame.get('key'), currentSize)
     })
   }
 

app/renderer/components/tabs/tabPage.js

@@ -58,7 +58,7 @@ class TabPage extends ImmutableComponent {
     setTimeout(() => {
       // If we're moving to a right page, then we're already shifting everything to the left by one, so we want
       // to drop it on the right.
-      windowActions.moveTab(sourceDragData, moveToFrame,
+      windowActions.moveTab(sourceDragData.get('key'), moveToFrame.get('key'),
         // Has -1 value for pinned tabs
         sourceDragFromPageIndex === -1 ||
         sourceDragFromPageIndex >= this.props.index)

app/renderer/components/tabs/tabs.js

@@ -14,9 +14,6 @@ const Tab = require('./tab')
 const appActions = require('../../../../js/actions/appActions')
 const windowActions = require('../../../../js/actions/windowActions')
 
-// Store
-const windowStore = require('../../../../js/stores/windowStore')
-
 // Constants
 const dragTypes = require('../../../../js/constants/dragTypes')
 
@@ -75,29 +72,29 @@ class Tabs extends ImmutableComponent {
     const clientX = e.clientX
     const sourceDragData = dndData.getDragData(e.dataTransfer, dragTypes.TAB)
     if (sourceDragData) {
+      // If this is a different window ID than where the drag started, then
+      // the tear off will be done by tab.js
+      if (this.props.dragData.get('windowId') !== getCurrentWindowId()) {
+        return
+      }
+
       // This must be executed async because the state change that this causes
       // will cause the onDragEnd to never run
       setTimeout(() => {
         const key = sourceDragData.get('key')
         let droppedOnTab = dnd.closestFromXOffset(this.tabRefs.filter((node) => node && node.props.frame.get('key') !== key), clientX).selectedRef
         if (droppedOnTab) {
           const isLeftSide = dnd.isLeftSide(ReactDOM.findDOMNode(droppedOnTab), clientX)
-          const droppedOnFrameProps = windowStore.getFrame(droppedOnTab.props.frame.get('key'))
 
-          // If this is a different window ID than where the drag started, then
-          // the tear off will be done by tab.js
-          if (this.props.dragData.get('windowId') !== getCurrentWindowId()) {
-            return
-          }
-
-          windowActions.moveTab(sourceDragData, droppedOnFrameProps, isLeftSide)
+          windowActions.moveTab(key, droppedOnTab.props.frame.get('key'), isLeftSide)
           if (sourceDragData.get('pinnedLocation')) {
             appActions.tabPinned(sourceDragData.get('tabId'), false)
           }
         }
       }, 0)
       return
     }
+
     if (e.dataTransfer.files) {
       Array.from(e.dataTransfer.files).forEach((file) => {
         const path = encodeURI(file.path)

app/renderer/reducers/frameReducer.js

@@ -24,7 +24,7 @@ const setFullScreen = (state, action) => {
 
 const closeFrame = (state, action) => {
   // Use the frameProps we passed in, or default to the active frame
-  const frameProps = action.frameProps
+  const frameProps = frameStateUtil.getFrameByKey(state, action.frameKey)
   const index = frameStateUtil.getFramePropsIndex(state.get('frames'), frameProps)
   if (index === -1) {
     return state
@@ -51,7 +51,7 @@ const closeFrame = (state, action) => {
   // Copy the hover state if tab closed with mouse as long as we have a next frame
   // This allow us to have closeTab button visible  for sequential frames closing, until onMouseLeave event happens.
   if (hoverState && nextFrame) {
-    windowActions.setTabHoverState(nextFrame, hoverState)
+    windowActions.setTabHoverState(nextFrame.get('key'), hoverState)
   }
 
   return state
@@ -75,7 +75,7 @@ const frameReducer = (state, action) => {
       })
       break
     case windowConstants.WINDOW_CLOSE_FRAME:
-      if (!action.frameProps) {
+      if (action.frameKey < 0) {
         break
       }
       // Unless a caller explicitly specifies to close a pinned frame, then

docs/appActions.md

@@ -192,15 +192,15 @@ Removes a site from the site list
 
 
 
-### moveSite(sourceDetail, destinationDetail, prepend, destinationIsParent) 
+### moveSite(sourceKey, destinationKey, prepend, destinationIsParent) 
 
 Dispatches a message to move a site locations.
 
 **Parameters**
 
-**sourceDetail**: `string`, the location, partitionNumber, etc of the source moved site
+**sourceKey**: `string`, the source key of the source moved site
 
-**destinationDetail**: `string`, the location, partitionNumber, etc of the destination moved site
+**destinationKey**: `string`, the destination key of the destination moved site
 
 **prepend**: `boolean`, Whether or not to prepend to the destinationLocation
   If false, the destinationDetail is considered a sibling.

docs/windowActions.md

@@ -179,15 +179,13 @@ Dispatches a message to the store to indicate that the webview entered full scre
 
 
 
-### closeFrame(frames, frameProps) 
+### closeFrame(frameKey) 
 
 Dispatches a message to close a frame
 
 **Parameters**
 
-**frames**: `Array.<Object>`, Immutable list of of all the frames
-
-**frameProps**: `Object`, The properties of the frame to close
+**frameKey**: `Object`, Frame key of the frame to close
 
 
 
@@ -224,14 +222,14 @@ Dispatches a message to the store when the frame is active and the window is foc
 
 
 
-### setPreviewFrame(frameProps) 
+### setPreviewFrame(frameKey) 
 
 Dispatches a message to the store to set a preview frame.
 This is done when hovering over a tab.
 
 **Parameters**
 
-**frameProps**: `Object`, the frame properties for the webview in question.
+**frameKey**: `Object`, the frame key for the webview in question.
 
 
 
@@ -245,25 +243,25 @@ Dispatches a message to the store to set the tab page index.
 
 
 
-### setTabBreakpoint(frameProps, breakpoint) 
+### setTabBreakpoint(frameKey, breakpoint) 
 
 Dispatches a message to the store to set the tab breakpoint.
 
 **Parameters**
 
-**frameProps**: `Object`, the frame properties for the webview in question.
+**frameKey**: `Object`, the frame key for the webview in question.
 
 **breakpoint**: `string`, the tab breakpoint to change to
 
 
 
-### setTabHoverState(frameProps, hoverState) 
+### setTabHoverState(frameKey, hoverState) 
 
 Dispatches a message to the store to set the current tab hover state.
 
 **Parameters**
 
-**frameProps**: `Object`, the frame properties for the webview in question.
+**frameKey**: `Object`, the frame key for the webview in question.
 
 **hoverState**: `boolean`, whether or not mouse is over tab
 
@@ -289,15 +287,15 @@ Dispatches a message to the store to set the tab page index.
 
 
 
-### moveTab(sourceFrameProps, destinationFrameProps, prepend) 
+### moveTab(sourceFrameKey, destinationFrameKey, prepend) 
 
 Dispatches a message to the store to indicate that the specified frame should move locations.
 
 **Parameters**
 
-**sourceFrameProps**: `Object`, the frame properties for the webview to move.
+**sourceFrameKey**: `Object`, the frame key for the webview to move.
 
-**destinationFrameProps**: `Object`, the frame properties for the webview to move to.
+**destinationFrameKey**: `Object`, the frame key for the webview to move to.
 
 **prepend**: `boolean`, Whether or not to prepend to the destinationFrameProps
 

js/about/aboutActions.js

@@ -188,11 +188,11 @@ const aboutActions = {
     ipc.sendToHost(messages.CONTEXT_MENU_OPENED, nodeProps, contextMenuType)
   },
 
-  moveSite: function (sourceDetail, destinationDetail, prepend, destinationIsParent) {
+  moveSite: function (sourceKey, destinationKey, prepend, destinationIsParent) {
     aboutActions.dispatchAction({
       actionType: appConstants.APP_MOVE_SITE,
-      sourceDetail,
-      destinationDetail,
+      sourceKey,
+      destinationKey,
       prepend,
       destinationIsParent
     })

js/about/bookmarks.js

@@ -59,8 +59,11 @@ class BookmarkFolderItem extends React.Component {
    */
   moveBookmark (e, bookmark) {
     if (siteUtil.isMoveAllowed(this.props.allBookmarkFolders, bookmark, this.props.bookmarkFolder)) {
-      aboutActions.moveSite(bookmark.toJS(),
-        this.props.bookmarkFolder.toJS(),
+      const bookmarkSiteKey = siteUtil.getSiteKey(bookmark.toJS())
+      const bookmarkFolderSiteKey = siteUtil.getSiteKey(this.props.bookmarkFolder.toJS())
+
+      aboutActions.moveSite(bookmarkSiteKey,
+        bookmarkFolderSiteKey,
         dndData.shouldPrependVerticalItem(e.target, e.clientY),
         true)
     }
@@ -296,8 +299,11 @@ class BookmarksList extends ImmutableComponent {
     }
 
     if (siteUtil.isMoveAllowed(this.props.allBookmarkFolders, bookmark, siteDetail)) {
-      aboutActions.moveSite(bookmark.toJS(),
-        siteDetail.toJS(),
+      const bookmarkSiteKey = siteUtil.getSiteKey(bookmark.toJS())
+      const siteKey = siteUtil.getSiteKey(siteDetail.toJS())
+
+      aboutActions.moveSite(bookmarkSiteKey,
+        siteKey,
         dndData.shouldPrependVerticalItem(e.target, e.clientY),
         destinationIsParent)
     }

js/actions/appActions.js

@@ -254,17 +254,17 @@ const appActions = {
   /**
    * Dispatches a message to move a site locations.
    *
-   * @param {string} sourceDetail - the location, partitionNumber, etc of the source moved site
-   * @param {string} destinationDetail - the location, partitionNumber, etc of the destination moved site
+   * @param {string} sourceKey - the source key of the source moved site
+   * @param {string} destinationKey - the destination key of the destination moved site
    * @param {boolean} prepend - Whether or not to prepend to the destinationLocation
    *   If false, the destinationDetail is considered a sibling.
    * @param {boolean} destinationIsParent - Whether or not the destinationDetail should be considered the new parent.
    */
-  moveSite: function (sourceDetail, destinationDetail, prepend, destinationIsParent) {
+  moveSite: function (sourceKey, destinationKey, prepend, destinationIsParent) {
     AppDispatcher.dispatch({
       actionType: appConstants.APP_MOVE_SITE,
-      sourceDetail,
-      destinationDetail,
+      sourceKey,
+      destinationKey,
       prepend,
       destinationIsParent
     })