Support SVGs instead of PDFs on touchbar
Categories
(Core :: Widget: Cocoa, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox71 | --- | fixed |
People
(Reporter: ntim, Assigned: bugzilla)
References
Details
Attachments
(2 files, 1 obsolete file)
I believe this code can do this: https://searchfox.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm
Reporter | ||
Comment 1•6 years ago
|
||
I looked a bit into how nsMenuItemIconX works, it seems to asynchronously load the icon using imgLoader->LoadImage (1) and then receives it in the imgINotificationObserver::Notify method (2) then transforms it to an NSImage (3) before setting it as menuitem icon in OnFrameComplete (4).
This code is actually pretty old, it was introduced in bug 363574, so I guess all this logic could be refactored as a helper that does steps 1-3.
A possible plan would be:
-
copy nsMenuItemIconX's code into a new nsChromeIcon class (I'm terrible at naming)
-
remove all the menuitem specific stuff
-
make nsChromeIcon expose an
onNSImageLoaded
callback on its interface. -
make nsMenuItemIconX implement that new nsChromeIcon interface instead of imgINotificationObserver
-
create a new nsTouchBarItemIcon class implementing that interface
-
use that new class in nsTouchBar
Some questions:
- Can we support dynamic touchbar input icon updates with this approach ? The bookmark item needs to support this.
- It's very likely that I'm over-complicating things because of my inexperience.
So: Is this the most simple approach ? Is there an easier alternative ?
Markus, Stephen, what do you think ? I might not work on this right away, but this info might be useful to anyone who wants to.
Comment 2•6 years ago
|
||
Comment 3•6 years ago
|
||
Adding n-i here for the question in phabricator.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 4•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 5•6 years ago
|
||
Depends on D34018
Assignee | ||
Updated•6 years ago
|
Reporter | ||
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 7•6 years ago
|
||
bugherder |
![]() |
||
Comment 8•6 years ago
|
||
Tim, D34926 is being held up because nsSVGUtils::GetOpacity triggers warnings while processing the contents of a VectorImage under nsIconLoaderService::OnFrameComplete. It would be preferable to have the warning code warn only if the VectorImage is actually embedded (by an HTML/SVG document). I.e. not warn if it is being viewed directly, or is referenced by nsIconLoaderService for the macbook touchbar. Right now we can't use whether an SVGContextPaint object was passed or not to determine whether the image is embedded or directly referenced (we don't pass an SVGContextPaint in most cases even when an image is embedded because referencing context properties is so rare). Would you know if there's a general mechanism we could use to determine this instead?
Comment 9•6 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #8)
It would be preferable to have the
warning code warn only if the VectorImage is actually embedded (by an
HTML/SVG document). I.e. not warn if it is being viewed directly, or is
referenced by nsIconLoaderService for the macbook touchbar.
Would you know if there's a
general mechanism we could use to determine this instead?
I don't think that is going to be possible as the svg image could be used in both ways at the same time. And the issue in D34926 seems to be wanting to use the same images in the touchbar and in a regular window.
How hard is it to pass a SVGContextPaint to fix this warning for most cases?
Out of curiousity (or perhaps it may help solve this issue), what is the path from nsIconLoaderService::OnFrameComplete to nsSVGUtils::GetOpacity?
Updated•6 years ago
|
![]() |
||
Comment 10•6 years ago
|
||
(In reply to Timothy Nikkel (:tnikkel) from comment #9)
I don't think that is going to be possible as the svg image could be used in both ways at the same time. And the issue in D34926 seems to be wanting to use the same images in the touchbar and in a regular window.
That makes sense.
How hard is it to pass a SVGContextPaint to fix this warning for most cases?
It would be a little annoying, but also wasteful. On balance, I think we should just remove these warnings. We don't have similar warnings for the far more commonly used context-fill
etc. and given that the image will visually appear differently that's probably fine.
Out of curiousity (or perhaps it may help solve this issue), what is the path from nsIconLoaderService::OnFrameComplete to nsSVGUtils::GetOpacity?
nsSVGUtils::GetOpacity
nsSVGUtils::MakeFillPatternFor
mozilla::SVGGeometryFrame::Render
mozilla::SVGGeometryFrame::PaintSVG
nsDisplaySVGGeometry::Paint
mozilla::FrameLayerBuilder::PaintItems
mozilla::FrameLayerBuilder::DrawPaintedLayer
mozilla::layers::BasicLayerManager::PaintSelfOrChildren
mozilla::layers::BasicLayerManager::PaintSelfOrChildren
nsDisplayList::PaintRoot
nsLayoutUtils::PaintFrame
mozilla::PresShell::RenderDocument
gfxCallbackDrawable::Draw
gfxUtils::DrawPixelSnapped
mozilla::image::imgFrame::InitWithDrawable
mozilla::image::VectorImage::CreateSurface
mozilla::image::VectorImage::GetFrameInternal
mozilla::image::VectorImage::GetFrameAtSize
mozilla::image::VectorImage::GetFrame
nsIconLoaderService::OnFrameComplete
Updated•6 years ago
|
Comment 11•6 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #10)
How hard is it to pass a SVGContextPaint to fix this warning for most cases?
It would be a little annoying, but also wasteful. On balance, I think we
should just remove these warnings. We don't have similar warnings for the
far more commonly usedcontext-fill
etc. and given that the image will
visually appear differently that's probably fine.
That seems reasonable to me.
Out of curiousity (or perhaps it may help solve this issue), what is the path from nsIconLoaderService::OnFrameComplete to nsSVGUtils::GetOpacity?
nsSVGUtils::GetOpacity nsSVGUtils::MakeFillPatternFor mozilla::SVGGeometryFrame::Render mozilla::SVGGeometryFrame::PaintSVG nsDisplaySVGGeometry::Paint mozilla::FrameLayerBuilder::PaintItems mozilla::FrameLayerBuilder::DrawPaintedLayer mozilla::layers::BasicLayerManager::PaintSelfOrChildren mozilla::layers::BasicLayerManager::PaintSelfOrChildren nsDisplayList::PaintRoot nsLayoutUtils::PaintFrame mozilla::PresShell::RenderDocument gfxCallbackDrawable::Draw gfxUtils::DrawPixelSnapped mozilla::image::imgFrame::InitWithDrawable mozilla::image::VectorImage::CreateSurface mozilla::image::VectorImage::GetFrameInternal mozilla::image::VectorImage::GetFrameAtSize mozilla::image::VectorImage::GetFrame nsIconLoaderService::OnFrameComplete
We could maybe pass a flag to GetFrame and then make sure that flag gets all the way to the nsDisplayListBuilder created in nsLayoutUtils::PaintFrame, and then make sure that info gets from nsDisplaySVGGeometry::Paint to nsSVGUtils::GetOpacity. But that seems like a lot of code just for a warning that we aren't super interested in keeping around.
Updated•6 years ago
|
Comment 13•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
Backed out changeset 6bad91b2f198 (bug 1521893) for causing browser_touchbar_tests.js to permafail
push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=257786095&searchStr=os%2Cx%2C10.10%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx1010-64-shippable%2Fopt-mochitest-browser-chrome-e10s-4%2Cm%28bc4%29&revision=6bad91b2f1989e35f5dbcaa2a850102a0bcb3078
backout: https://hg.mozilla.org/integration/autoland/rev/fd712790ec030ddc0e8c0555428779132e919a60
Assignee | ||
Comment 15•6 years ago
|
||
I updated the revision on Phabricator. Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5da7db4b8b992d05c603dc7e9ee591a439ff5fd1
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
bugherder |
![]() |
||
Comment 18•6 years ago
|
||
Backed out part 2 to fix crashes (Bug 1568862):
https://hg.mozilla.org/integration/autoland/rev/4b23bb4e274c0c0d69b7be2d3cd6090a971a837e
![]() |
||
Comment 19•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 20•6 years ago
|
||
Clearing needinfo since there's an updated revision on Phabricator.
Updated•6 years ago
|
Comment 21•6 years ago
|
||
Comment 22•6 years ago
|
||
Backed out changeset 3ab29c05ea2f (bug 1521893) build bustage in /builds/worker/workspace/build/src/widget/cocoa/nsTouchBarInputIcon.h. CLOSED TREE
Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=263986766&repo=autoland&lineNumber=25896
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=3ab29c05ea2f80400fee727500dd4563854779a3
Backout:
https://hg.mozilla.org/integration/autoland/rev/3abcd517522920b1cdd9efc21ff475d7b013e179
Assignee | ||
Comment 23•6 years ago
|
||
Clearing ni since I posted a new revision on Phabricator with no bustages: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b7281d48f3d9b7756d615da4e8a4af877925749
Assignee | ||
Updated•6 years ago
|
Comment 24•6 years ago
|
||
Comment 25•6 years ago
|
||
Backed out changeset a8975a4aebc0 (bug 1521893) for bc leasks on (nsIconLoaderObserver, nsIconLoaderService, nsTouchBarInputIcon).
Backout: https://hg.mozilla.org/integration/autoland/rev/00e5d7de0b146fc9c0868933a8970742ca404a0e
Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=a8975a4aebc079f0d943d3d65dc42b7a1371975b&selectedJob=265399059
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=265399059&repo=autoland&lineNumber=13346
Assignee | ||
Comment 26•6 years ago
|
||
Clearing ni since I pushed a new revision to Phabricator.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 27•6 years ago
|
||
Comment 28•6 years ago
|
||
bugherder |
Assignee | ||
Updated•6 years ago
|
Description
•