#7622: Add support for MV3 to setToolbarIcon via chrome.offscreen
#7634
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7634 +/- ##
=======================================
Coverage 72.77% 72.77%
=======================================
Files 1225 1226 +1
Lines 38212 38213 +1
Branches 7185 7184 -1
=======================================
+ Hits 27808 27809 +1
Misses 10404 10404 ☔ View full report in Codecov by Sentry. |
|
I'll close this until #7632 is done and other related work is finished. I thought it was |
|
@fregante you should be good to re-open this now. |
| @@ -30,12 +29,12 @@ async function setToolbarIcon(): Promise<void> { | |||
| const activeTheme = await getActiveTheme(); | |||
|
|
|||
| if (activeTheme === DEFAULT_THEME) { | |||
| await activateBrowserActionIcon(); | |||
| // Not necessary. If the theme is ever unset, just reload the extension to reset the icon. | |||
There was a problem hiding this comment.
Why not reset to the default logo though? Can it hurt? It seems more convenient to just have it set here rather then having to go out of your way to reset the extension. e.g. I'm thinking about the use-case of the sales team doing demos with custom branding and switching themes often
There was a problem hiding this comment.
Keep in mind that this PR is old and based on old theme code. I need to rebase it and rework it. I just reopened it to keep track of my tasks this week.
| assertNotNullish(context, "Failed to get 2D context for canvas"); // Impossible | ||
|
|
||
| // Note: Images that are not square will be stretched to fit the canvas, unless | ||
| // they're SVGs and the preserveAspectRatio attribute is set. |
There was a problem hiding this comment.
unless they're SVGs and the preserveAspectRatio attribute is set.
In which case, what happens? Does it get truncated?
There was a problem hiding this comment.
Raster images are just stretched. SVG images are mostly fine I think as long as there's a viewBox attribute. The preserveAspectRatio thing might only be required in some browsers. I didn't investigate all the scenarios, but in short SVGs can be authored with a specific scaling behavior.
| const imageBitmap = await createImageBitmap(blob); | ||
|
|
||
| const context = new OffscreenCanvas(width, height).getContext("2d"); | ||
| assertNotNullish(context, "Failed to get 2D context for canvas"); // Impossible |
There was a problem hiding this comment.
Impossible
Is typescript strictNull complaining here?
There was a problem hiding this comment.
Yes. The context can be null if you request an unsupported type I suppose. I could alternatively fix the global types but I think here I just copied an existing null check.
There was a problem hiding this comment.
I could alternatively fix the global types but I think here I just copied an existing null check.
I don't have a strong opinion that you go out of your way to do that. Obviously only makes the code marginally less verbose
fregante
changed the title
setToolbarIcon and fix theme supportsetToolbarIcon via chrome.offscreen
|
I'll just close this for now and open a new one. Most of the underlying code changed here. The tracking ticket is #7622 |
What does this PR do?
Demo
wip
Checklist
src/tsconfig.strictNullChecks.json(if possible)