Added more tests for tabContentState and found/fixed issues along the…

… way:

- iconSize was not being exported correctly (it always had undefined). I moved to config and verified it has a value now
- the tab size methods now handle null/undefined properly
- updated tabContentState to use the braveExtensionId constant

Auditors: @NejcZdovc

app/common/state/tabContentState.js

@@ -4,6 +4,7 @@
 
 // Constants
 const settings = require('../../../js/constants/settings')
+const {braveExtensionId} = require('../../../js/constants/config')
 
 // Utils
 const locale = require('../../../js/l10n')
@@ -15,8 +16,6 @@ const {getSetting} = require('../../../js/settings')
 // Styles
 const styles = require('../../renderer/components/styles/global')
 
-module.exports.iconSize = 16
-
 const tabContentState = {
   getDisplayTitle: (state, frameKey) => {
     const frame = frameStateUtil.getFrameByKey(state, frameKey)
@@ -66,7 +65,7 @@ const tabContentState = {
       ) &&
       (
         !frame.get('provisionalLocation') ||
-        !frame.get('provisionalLocation').startsWith('chrome-extension://mnojpmjdmbbfmejpflffifhffcmidifd/')
+        !frame.get('provisionalLocation').startsWith(`chrome-extension://${braveExtensionId}/`)
       )
   },
 
@@ -79,23 +78,23 @@ const tabContentState = {
 
   isMediumView: (state, frameKey) => {
     const frame = frameStateUtil.getFrameByKey(state, frameKey)
-    const sizes = ['large', 'largeMedium']
-
-    return sizes.includes(frame.get('breakpoint'))
+    return frame
+      ? ['large', 'largeMedium'].includes(frame.get('breakpoint'))
+      : false
   },
 
   isNarrowView: (state, frameKey) => {
     const frame = frameStateUtil.getFrameByKey(state, frameKey)
-    const sizes = ['medium', 'mediumSmall', 'small', 'extraSmall', 'smallest']
-
-    return sizes.includes(frame.get('breakpoint'))
+    return frame
+      ? ['medium', 'mediumSmall', 'small', 'extraSmall', 'smallest'].includes(frame.get('breakpoint'))
+      : false
   },
 
   isNarrowestView: (state, frameKey) => {
     const frame = frameStateUtil.getFrameByKey(state, frameKey)
-    const sizes = ['extraSmall', 'smallest']
-
-    return sizes.includes(frame.get('breakpoint'))
+    return frame
+      ? ['extraSmall', 'smallest'].includes(frame.get('breakpoint'))
+      : false
   },
 
   getTabIconColor: (state, frameKey) => {

app/renderer/components/bookmarks/bookmarkToolbarButton.js

@@ -20,12 +20,12 @@ const windowStore = require('../../../../js/stores/windowStore')
 // Constants
 const siteTags = require('../../../../js/constants/siteTags')
 const dragTypes = require('../../../../js/constants/dragTypes')
+const {iconSize} = require('../../../../js/constants/config')
 
 // Utils
 const siteUtil = require('../../../../js/state/siteUtil')
 const {getCurrentWindowId} = require('../../currentWindow')
 const dnd = require('../../../../js/dnd')
-const {iconSize} = require('../../../common/state/tabContentState')
 const cx = require('../../../../js/lib/classSet')
 
 // Styles

app/renderer/components/bookmarks/bookmarksToolbar.js

@@ -22,6 +22,7 @@ const windowStore = require('../../../../js/stores/windowStore')
 // Constants
 const siteTags = require('../../../../js/constants/siteTags')
 const dragTypes = require('../../../../js/constants/dragTypes')
+const {iconSize} = require('../../../../js/constants/config')
 
 // Utils
 const siteUtil = require('../../../../js/state/siteUtil')
@@ -30,7 +31,6 @@ const cx = require('../../../../js/lib/classSet')
 const dnd = require('../../../../js/dnd')
 const dndData = require('../../../../js/dndData')
 const {calculateTextWidth} = require('../../../../js/lib/textCalculator')
-const {iconSize} = require('../../../common/state/tabContentState')
 
 // Styles
 const globalStyles = require('../styles/global')

js/about/bookmarks.js

@@ -15,7 +15,7 @@ const cx = require('../lib/classSet')
 const SortableTable = require('../../app/renderer/components/common/sortableTable')
 const siteUtil = require('../state/siteUtil')
 const formatUtil = require('../../app/common/lib/formatUtil')
-const {iconSize} = require('../../app/common/state/tabContentState')
+const {iconSize} = require('../constants/config')
 
 const ipc = window.chrome.ipcRenderer
 

js/constants/config.js

@@ -62,5 +62,6 @@ module.exports = {
   },
   tabs: {
     maxAllowedNewSessions: 9
-  }
+  },
+  iconSize: 16
 }

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

@@ -2,12 +2,14 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-/* global describe, it, before, after */
+/* global describe, it, before, after, afterEach */
 
 const assert = require('assert')
 const Immutable = require('immutable')
 const mockery = require('mockery')
+const sinon = require('sinon')
 const fakeElectron = require('../../../lib/fakeElectron')
+const {braveExtensionId} = require('../../../../../js/constants/config')
 
 const frameKey = 1
 const defaultWindowStore = Immutable.fromJS({
@@ -32,17 +34,21 @@ const defaultWindowStore = Immutable.fromJS({
 
 describe('tabContentState unit tests', function () {
   let tabContentState
+  let frameStateUtil
+  let getFrameByKeyMock
 
   before(function () {
     mockery.enable({
       warnOnReplace: false,
       warnOnUnregistered: false,
       useCleanCache: true
     })
+    frameStateUtil = require('../../../../../js/state/frameStateUtil')
     mockery.registerMock('electron', fakeElectron)
     mockery.registerMock('../../../js/l10n', {
       translation: () => 'translated'
     })
+    mockery.registerMock('../../../js/state/frameStateUtil', frameStateUtil)
     tabContentState = require('../../../../../app/common/state/tabContentState')
   })
 
@@ -51,6 +57,13 @@ describe('tabContentState unit tests', function () {
     mockery.disable()
   })
 
+  afterEach(function () {
+    if (getFrameByKeyMock) {
+      getFrameByKeyMock.restore()
+      getFrameByKeyMock = undefined
+    }
+  })
+
   describe('getDisplayTitle', function () {
     it('should return empty string if frame is not found', function * () {
       const result = tabContentState.getDisplayTitle(defaultWindowStore, 0)
@@ -95,4 +108,81 @@ describe('tabContentState unit tests', function () {
       assert.equal(result, 'Brave')
     })
   })
+
+  describe('isTabLoading', function () {
+    describe('when provisionalLocation is not set', function () {
+      it('returns true if frame.loading', function () {
+        getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', (state, frameKey) => {
+          return Immutable.fromJS({loading: true})
+        })
+        assert.equal(tabContentState.isTabLoading(), true)
+      })
+      it('returns true if location is about:blank', function () {
+        getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', (state, frameKey) => {
+          return Immutable.fromJS({location: 'about:blank'})
+        })
+        assert.equal(tabContentState.isTabLoading(), true)
+      })
+    })
+
+    describe('when provisionalLocation is set', function () {
+      it('returns false if loading and provisionalLocation is a brave about page', function () {
+        getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', (state, frameKey) => {
+          return Immutable.fromJS({
+            loading: true,
+            provisionalLocation: `chrome-extension://${braveExtensionId}/pageGoesHere`
+          })
+        })
+        assert.equal(tabContentState.isTabLoading(), false)
+      })
+      it('returns true if loading and provisionalLocation is not a brave about page', function () {
+        getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', (state, frameKey) => {
+          return Immutable.fromJS({
+            loading: true,
+            provisionalLocation: 'https://brave.com'
+          })
+        })
+        assert.equal(tabContentState.isTabLoading(), true)
+      })
+    })
+  })
+
+  describe('isMediumView', function () {
+    it('handles frame being null/undefined', function () {
+      assert.equal(tabContentState.isMediumView(), false)
+    })
+
+    it('returns true if valid', function () {
+      getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', (state, frameKey) => {
+        return Immutable.fromJS({breakpoint: 'large'})
+      })
+      assert.equal(tabContentState.isMediumView(), true)
+    })
+  })
+
+  describe('isNarrowView', function () {
+    it('returns false if null/undefined', function () {
+      assert.equal(tabContentState.isNarrowView(), false)
+    })
+
+    it('returns true if valid', function () {
+      getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', (state, frameKey) => {
+        return Immutable.fromJS({breakpoint: 'small'})
+      })
+      assert.equal(tabContentState.isNarrowView(), true)
+    })
+  })
+
+  describe('isNarrowestView', function () {
+    it('handles frame being null/undefined', function () {
+      assert.equal(tabContentState.isNarrowestView(), false)
+    })
+
+    it('returns true if valid', function () {
+      getFrameByKeyMock = sinon.stub(frameStateUtil, 'getFrameByKey', (state, frameKey) => {
+        return Immutable.fromJS({breakpoint: 'extraSmall'})
+      })
+      assert.equal(tabContentState.isNarrowestView(), true)
+    })
+  })
 })