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

Simplify badge icon logic #7500

Merged
merged 1 commit into from
Mar 16, 2017
Merged

Simplify badge icon logic #7500

merged 1 commit into from
Mar 16, 2017

Conversation

jonathansampson
Copy link
Collaborator

@jonathansampson jonathansampson commented Mar 4, 2017

Removes redundant lookups, invalid considerations, and handles older patterns.

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Fixes #6193

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.

Removes redundant lookups, invalid considerations, and handles older
patterns.
@jonathansampson
Copy link
Collaborator Author

bump for review @darkdh @bridiver

@bridiver
Copy link
Collaborator

lgtm

@bsclifton bsclifton merged commit b5eb333 into master Mar 16, 2017
@bsclifton bsclifton deleted the browserActionIcons branch March 16, 2017 16:55
@darkdh
Copy link
Member

darkdh commented Mar 19, 2017

++

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

Successfully merging this pull request may close these issues.

4 participants