1521893 - Support SVGs instead of PDFs on touchbar

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement)

Description

[Tim Nguyen :ntim]

I believe this code can do this: https://searchfox.org/mozilla-central/source/widget/cocoa/nsMenuItemIconX.mm

Comment 1

[Tim Nguyen :ntim]

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

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1521893 - Part 1 - Split off nsMenuItemIconX image loading into a new utility class, MacIconLoader and be its first consumer. r?spohl

Comment 3

[Stephen A Pohl [:spohl] (OOO until 4/29)]

Adding n-i here for the question in phabricator.

Comment 4

[Harry Twyford]

Attachment: Bug 1521893 - Part 1 - Split off nsMenuItemIconX image loading into a new utility class, nsIconLoaderService, and be its first consumer. r?spohl

Comment 5

[Harry Twyford]

Attachment: Bug 1521893 - Part 2 - Support loading SVG icons on the Touch Bar through a new nsTouchBarInputIcon service class. r?spohl

Depends on D34018

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ab282375f89b

Comment 8

[Jonathan Watt [:jwatt]]

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

[Timothy Nikkel (: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?

Comment 10

[Jonathan Watt [: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

Comment 11

[Timothy Nikkel (: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.

Comment 12

[Jonathan Watt [:jwatt]]

Patch in bug 1567962.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Andrei Ciure[:aciure]]

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

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=257786095&fromchange=a858e44115324243417d830fafa8a4a8eb57e5a0&searchStr=os%2Cx%2C10.10%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx1010-64-shippable%2Fopt-mochitest-browser-chrome-e10s-4%2Cm%28bc4%29

https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=257748489&fromchange=a858e44115324243417d830fafa8a4a8eb57e5a0&searchStr=os%2Cx%2C10.10%2Cdebug%2Cmochitests%2Ctest-macosx1010-64%2Fdebug-mochitest-browser-chrome-e10s-8%2Cm%28bc8%29

backout: https://hg.mozilla.org/integration/autoland/rev/fd712790ec030ddc0e8c0555428779132e919a60

Comment 15

[Harry Twyford]

I updated the revision on Phabricator. Green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5da7db4b8b992d05c603dc7e9ee591a439ff5fd1

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/4aead1eca8c6

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out part 2 to fix crashes (Bug 1568862):

https://hg.mozilla.org/integration/autoland/rev/4b23bb4e274c0c0d69b7be2d3cd6090a971a837e

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4b23bb4e274c

Comment 20

[Harry Twyford]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Dorel Luca [:dluca]]

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

Comment 23

[Harry Twyford]

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Cristian Brindusan [:cbrindusan]]

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

Comment 26

[Harry Twyford]

Clearing ni since I pushed a new revision to Phabricator.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/38b1b272884d