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,13 @@ const sitesReducer = (state, action, immutableAction) => {
  state = state.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag, action.originalSiteDetail, action.skipSync))
  }
  if (action.destinationDetail) {
- state = state.set('sites', siteUtil.moveSite(state.get('sites'),
- action.siteDetail, action.destinationDetail, false, false, true))
+ const sourceKey = siteUtil.getSiteKey(action.siteDetail)
+ const destinationKey = siteUtil.getSiteKey(action.destinationDetail)
+
+ if (sourceKey != null) {
+ state = state.set('sites', siteUtil.moveSite(state.get('sites'),
+ sourceKey, destinationKey, false, false, true))
+ }
  }
  if (syncEnabled()) {
  state = syncUtil.updateSiteCache(state, action.destinationDetail || action.siteDetail)
@@ -56,10 +61,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
  })