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

Vimium: Icon not displayed near address bar #6193

Closed
jonathansampson opened this issue Dec 13, 2016 · 4 comments
Closed

Vimium: Icon not displayed near address bar #6193

jonathansampson opened this issue Dec 13, 2016 · 4 comments

Comments

@jonathansampson
Copy link
Collaborator

jonathansampson commented Dec 13, 2016

Test Plan:

An extension's manifest file may contain a browser_action.default_icon property. This property usually holds an object, but may (in older extensions) hold a string. String paths to images were not being handled by Brave, which caused some extensions (such as Vimium) to have no displayed icon.

To test this, install the Vimium extension in your development build of Brave. Vimium's extension ID is dbepggeogbaibhgnhhndojpepiihcmeb. With this extension registered and loaded, running Brave should reveal the Vimium icon near the Brave Shield icon.

This commit also prevents the redundant look-up of manifest.browser_action.default_icon (path is already a reference to browser_action.default_icon). Also, manifest.icons should not be the source for browser action icons.

Original issue description

Describe the issue you encountered:
Upon loading the Vimium extension, there was no icon displayed near the address bar.

Work-around:
Adding the following (borrowed from manifest.icon) to the manifest.browser_action object resolves:

"default_icon": {
  "19": "icons/icon16.png",
  "38": "icons/icon48.png"
}

Brave Version: 0.12.15

@luixxiul
Copy link
Contributor

@jonathansampson do we need QA for this issue, or is this postponed until Vinium extension becomes ready?

@luixxiul luixxiul added the needs-info Another team member needs information from the PR/issue opener. label Feb 16, 2017
@jonathansampson
Copy link
Collaborator Author

@luixxiul The root issue is resolved; this is good closed :)

@jonathansampson
Copy link
Collaborator Author

Reopening as manifest.icons is the wrong way to go about solving this problem. Seeing the comments on #7500 to better understand the correct approach.

@luixxiul luixxiul added QA/test-plan-required and removed needs-info Another team member needs information from the PR/issue opener. QA/no-qa-needed labels Mar 5, 2017
@bbondy bbondy modified the milestones: 0.13.6, 0.13.5 Mar 8, 2017
@bsclifton bsclifton modified the milestones: 0.13.7, 0.13.6 Mar 14, 2017
@bsclifton bsclifton modified the milestones: 0.14.0, 0.14.1 Mar 16, 2017
@srirambv
Copy link
Collaborator

Added no-qa-requried as the extension is not available in packaged build for testing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants