Merge pull request #14384 from brave/fix/14207-immediate-tab-loading-…

…indicator Show tab loading icon when navigating from about:* content to http content
brave · Jun 12, 2018 · 0b70d84 · 0b70d84
1 parent f5d7d50
commit 0b70d84
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 21 deletions.
diff --git a/app/common/state/tabContentState/faviconState.js b/app/common/state/tabContentState/faviconState.js
@@ -3,7 +3,7 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 // Utils
-const {isSourceAboutUrl} = require('../../../../js/lib/appUrlUtil')
+const {isTargetAboutUrl} = require('../../../../js/lib/appUrlUtil')
 const frameStateUtil = require('../../../../js/state/frameStateUtil')
 const {isEntryIntersected} = require('../../../../app/renderer/lib/observerUtil')
 
@@ -35,7 +35,8 @@ module.exports.showFavicon = (state, frameKey) => {
   }
 
   // new tab page is the only tab we do not show favicon
-  return !isNewTabPage
+  // unless we are loading the next page
+  return !isNewTabPage || module.exports.showLoadingIcon(state, frameKey)
 }
 
 module.exports.getFavicon = (state, frameKey) => {
@@ -55,15 +56,14 @@ module.exports.showLoadingIcon = (state, frameKey) => {
   if (frame == null) {
     return false
   }
-
-  if (frame.get('loading') == null) {
+  const isLoading = frame.get('loading')
+  // handle false or falsy value
+  if (!isLoading) {
     return false
   }
 
-  return (
-    !isSourceAboutUrl(frame.get('location')) &&
-    frame.get('loading')
-  )
+  const isLoadingAboutUrl = isTargetAboutUrl(frame.get('provisionalLocation'))
+  return !isLoadingAboutUrl
 }
 
 module.exports.showIconWithLessMargin = (state, frameKey) => {

diff --git a/app/common/state/tabUIState.js b/app/common/state/tabUIState.js
@@ -8,6 +8,7 @@ const settings = require('../../../js/constants/settings')
 // State helpers
 const closeState = require('../../common/state/tabContentState/closeState')
 const frameStateUtil = require('../../../js/state/frameStateUtil')
+const faviconState = require('../../common/state/tabContentState/faviconState')
 
 // Utils
 const {isEntryIntersected} = require('../../../app/renderer/lib/observerUtil')
@@ -84,7 +85,10 @@ module.exports.addExtraGutterToTitle = (state, frameKey) => {
     return false
   }
 
-  return frameStateUtil.frameLocationMatch(frame, 'about:newtab')
+  return (
+    frameStateUtil.frameLocationMatch(frame, 'about:newtab') &&
+    !faviconState.showLoadingIcon(state, frameKey)
+  )
 }
 
 module.exports.centralizeTabIcons = (state, frameKey, isPinned) => {

diff --git a/test/unit/app/common/state/tabContentStateTest/faviconStateTest.js b/test/unit/app/common/state/tabContentStateTest/faviconStateTest.js
@@ -9,6 +9,7 @@ const Immutable = require('immutable')
 const mockery = require('mockery')
 const fakeElectron = require('../../../../lib/fakeElectron')
 const {intersection} = require('../../../../../../app/renderer/components/styles/global')
+const appUrlUtil = require('../../../../../../js/lib/appUrlUtil')
 
 const frameKey = 1
 const index = 0
@@ -101,36 +102,60 @@ describe('faviconState unit tests', function () {
       assert.equal(faviconState.showLoadingIcon(), false)
     })
 
-    it('returns false if source is about page', function * () {
+    it('returns false if destination is about page and page is not loading', function * () {
       const state = defaultState
-        .setIn(['frames', index, 'location'], 'about:blank')
+        .mergeIn(['frames', index], {
+          loading: false,
+          location: 'http://www.example.com',
+          provisionalLocation: appUrlUtil.aboutUrls.get('about:newtab')
+        })
       const result = faviconState.showLoadingIcon(state, frameKey)
       assert.equal(result, false)
     })
 
-    it('returns true if source is not about page', function * () {
-      const state = defaultState.setIn(['frames', index, 'loading'], true)
+    it('returns false if loading an about page', function * () {
+      const state = defaultState
+        .mergeIn(['frames', index], {
+          loading: true,
+          location: 'http://www.example.com',
+          provisionalLocation: appUrlUtil.aboutUrls.get('about:newtab')
+        })
+      const result = faviconState.showLoadingIcon(state, frameKey)
+      assert.equal(result, false)
+    })
+
+    it('returns true if destination is not about page', function * () {
+      const state = defaultState
+      .mergeIn(['frames', index], {
+        loading: true,
+        location: 'http://www.example.com',
+        provisionalLocation: 'https://www.brave.com/'
+      })
       const result = faviconState.showLoadingIcon(state, frameKey)
       assert.equal(result, true)
     })
 
     it('returns false if page is not loading', function * () {
-      const state = defaultState.setIn(['frames', index, 'loading'], false)
+      const state = defaultState
+      .mergeIn(['frames', index], {
+        loading: false,
+        location: 'http://www.example.com',
+        provisionalLocation: 'https://www.brave.com/'
+      })
       const result = faviconState.showLoadingIcon(state, frameKey)
       assert.equal(result, false)
     })
 
     it('returns false if loading is undefined', function * () {
-      const state = defaultState.setIn(['frames', index, 'loading'], undefined)
+      const state = defaultState
+      .mergeIn(['frames', index], {
+        loading: undefined,
+        location: 'http://www.example.com',
+        provisionalLocation: 'https://www.brave.com/'
+      })
       const result = faviconState.showLoadingIcon(state, frameKey)
       assert.equal(result, false)
     })
-
-    it('returns true if page is loading', function * () {
-      const state = defaultState.setIn(['frames', index, 'loading'], true)
-      const result = faviconState.showLoadingIcon(state, frameKey)
-      assert.equal(result, true)
-    })
   })
 
   describe('showIconWithLessMargin', function () {