Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fix extension icons appearing in about: pages and context menu #12412

Merged
merged 1 commit into from
Dec 27, 2017
Merged

Fix extension icons appearing in about: pages and context menu #12412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/common/state/extensionState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
const { makeImmutable } = require('./immutableUtil')
const Immutable = require('immutable')
const platformUtil = require('../lib/platformUtil')
const {chromeUrl} = require('../../../js/lib/appUrlUtil')

const browserActionDefaults = Immutable.fromJS({
tabs: {}
Expand Down Expand Up @@ -77,7 +78,7 @@ const extensionState = {
browserActionBackgroundImage: (browserAction, tabId) => {
tabId = tabId ? tabId.toString() : '-1'
let path = browserAction.getIn(['tabs', tabId, 'path']) || browserAction.get('path')
let basePath = browserAction.get('base_path')
let basePath = chromeUrl(browserAction.get('base_path'))
if (path && basePath) {
// Older extensions may provide a string path
if (typeof path === 'string') {
Expand Down
4 changes: 2 additions & 2 deletions app/extensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const contextMenus = require('./browser/extensions/contextMenus')
const extensionActions = require('./common/actions/extensionActions')
const config = require('../js/constants/config')
const appConfig = require('../js/constants/appConfig')
const {chromeUrl} = require('../js/lib/appUrlUtil')
const {fileUrl} = require('../js/lib/appUrlUtil')
const {getExtensionsPath, getBraveExtUrl, getBraveExtIndexHTML} = require('../js/lib/appUrlUtil')
const {getSetting} = require('../js/settings')
const settings = require('../js/constants/settings')
Expand Down Expand Up @@ -408,7 +408,7 @@ module.exports.init = () => {
extensionInfo.setInstallInfo(installInfo.id, installInfo)
installInfo.filePath = installInfo.base_path

installInfo.base_path = chromeUrl(installInfo.base_path)
installInfo.base_path = fileUrl(installInfo.base_path)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinlawler i un-did this change from a51b7ea because chrome://brave URLs are not loadable in about: pages and also the code that generated context menu icons assumed a file:// URL. if i'm not mistaken, chrome://brave is only needed for the browser action.

Copy link
Member

@bsclifton bsclifton Dec 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diracdeltas with @kevinlawler 's PR before fixed the icons being broken in the top right (part of the UI Chrome). I wonder if we need to store both the Chrome URL + file URL?

Copy link
Member

@bsclifton bsclifton Dec 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just verified that with this change, the icons still show up as expected 😄 I now see the change you did by wrapping browserAction! 👍


extensionActions.extensionInstalled(installInfo.id, installInfo)
extensionActions.extensionEnabled(installInfo.id)
Expand Down
10 changes: 5 additions & 5 deletions js/lib/appUrlUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ module.exports.fileUrl = (filePath) => {
return encodeURI('file://' + fileUrlPath)
}

module.exports.chromeUrl = (filePath = '') => {
filePath = module.exports.fileUrl(filePath)
filePath = filePath.replace('file://', 'chrome://brave')

return filePath
/**
* Converts file URL to chrome:// URL
*/
module.exports.chromeUrl = (fileUrl = '') => {
return fileUrl.replace('file://', 'chrome://brave')
}

/**
Expand Down
2 changes: 1 addition & 1 deletion test/unit/lib/appUrlUtilTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ describe('appUrlUtil test', function () {
})
describe('chromeUrl', function () {
it('can convert file paths', function () {
const filePath = '/users/bbondy/space here/tesT.html'
const filePath = 'file:///users/bbondy/space%20here/tesT.html'
const chromeUrl = appUrlUtil.chromeUrl(filePath)
const expected = 'chrome://brave/users/bbondy/space%20here/tesT.html'
assert.equal(chromeUrl, expected)
Expand Down