Closed Bug 1521893 Opened 9 months ago Closed 11 days ago

Support SVGs instead of PDFs on touchbar

Categories

(Core :: Widget: Cocoa, enhancement)

enhancement
Not set
Points:
8

Tracking

()

RESOLVED FIXED
mozilla71
Iteration:
71.3 - Sept 30 - Oct 13
Tracking Status
firefox71 --- fixed

People

(Reporter: ntim, Assigned: harry)

References

Details

Attachments

(2 files, 1 obsolete file)

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.

Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(mstange)

Adding n-i here for the question in phabricator.

Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(mdeboer)
Flags: needinfo?(mdeboer)
Flags: needinfo?(spohl.mozilla.bugs)
Flags: needinfo?(spohl.mozilla.bugs)
Attachment #9038521 - Attachment is obsolete: true
Attachment #9038521 - Attachment is obsolete: false
Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Attachment #9038521 - Attachment is obsolete: true
Points: --- → 8
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab282375f89b
Part 1 - Split off nsMenuItemIconX image loading into a new utility class, nsIconLoaderService, and be its first consumer. r=spohl
Keywords: leave-open

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?

Flags: needinfo?(tnikkel)

(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?

Flags: needinfo?(tnikkel)
Flags: needinfo?(jwatt)

(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
Flags: needinfo?(jwatt)
Flags: needinfo?(tnikkel)

(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 used context-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.

Flags: needinfo?(tnikkel)
Flags: needinfo?(jwatt)

Patch in bug 1567962.

Flags: needinfo?(jwatt)
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6bad91b2f198
Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r=spohl
Keywords: leave-open
Flags: needinfo?(mstange)
Flags: needinfo?(htwyford)
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4aead1eca8c6
Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r=spohl
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
Regressions: 1569012
Status: RESOLVED → REOPENED
Flags: needinfo?(htwyford)
Resolution: FIXED → ---
Target Milestone: mozilla70 → ---
Attachment #9071982 - Attachment description: Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r?spohl! → Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r=spohl

Clearing needinfo since there's an updated revision on Phabricator.

Flags: needinfo?(htwyford)
Attachment #9071982 - Attachment description: Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r=spohl → Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r?spohl
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3ab29c05ea2f
Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r=spohl

Clearing ni since I posted a new revision on Phabricator with no bustages: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b7281d48f3d9b7756d615da4e8a4af877925749

Flags: needinfo?(htwyford)
Iteration: --- → 71.1 - Sept 2 - 15
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8975a4aebc0
Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r=spohl

Clearing ni since I pushed a new revision to Phabricator.

Iteration: 71.1 - Sept 2 - 15 → ---
Flags: needinfo?(htwyford)
Attachment #9071982 - Attachment description: Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class r?spohl → Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class.
Attachment #9071982 - Attachment description: Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class. → Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class. r?spohl
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38b1b272884d
Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class. r=spohl
Status: REOPENED → RESOLVED
Closed: 3 months ago11 days ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
Iteration: --- → 71.3 - Sept 30 - Oct 13
You need to log in before you can comment on or make changes to this bug.