1274046 - Add FailureId to gfxConfig (FeatureState)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Girard (:BenWa)]

We need a clear failureId to report failures to telemetry. We should have an explicit field for it.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review commit: https://reviewboard.mozilla.org/r/53706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53706/

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Can we this to Instance instead of FeatureState? In theory failure IDs could get attached at multiple levels (blocklist fail, force-enable, runtime fail).

Are you think that only the most recent failure ID matters? If so, that seems okay, though it'd be nice if we could hide that and add a failure ID parameter to the relevant state functions (like SetFailed, Disable) anyway.

Comment 3

[Benoit Girard (:BenWa)]

Yea I was on the fence about it. (On the side it's not immediately obvious how Failed/Disable match with each Instance, I think we could pick clearer names.) Right now I only want to report the most relevant failureId and I don't see how we would deal with multiple failure ids in the future so I opted for this way.

It's not a bad idea to set the API properly however. I'll take a look.

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53706/diff/1-2/

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53706/diff/2-3/

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53706/diff/3-4/

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

https://reviewboard.mozilla.org/r/53706/#review51694

Thanks!

Comment 8

[Benoit Girard (:BenWa)]

This patch is adding a string to a static type which our leak checker hates:
16 bytes leaked (nsStringBuffer)

Comment 9

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53706/diff/4-5/

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53706/diff/5-6/

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53706/diff/6-7/

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53706/diff/7-8/

Comment 13

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

I had to change how gfxConfig was init because otherwise nsCString was reported as being leaked.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

https://reviewboard.mozilla.org/r/53706/#review53212

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9111a0d9a44
Add FailureId to gfxConfig (FeatureState). r=dvander

Comment 16

[Makoto Kato [:m_kato]]

this landing causes bustage.  But taskcluster has problem now, so treeherder won't be able to get error log.


 0:06.07 gfxWindowsPlatform.cpp
 0:06.07 c:/Development/hg.mozilla.org/mozilla-win64/gfx/thebes/gfxWindowsPlatform.cpp(1942): error C2660: 'mozilla::gfx::FeatureState::DisableByDefault': function does not take 2 arguments
 0:06.07 c:/Development/hg.mozilla.org/mozilla-win64/gfx/thebes/gfxWindowsPlatform.cpp(1949): error C2660: 'IsGfxInfoStatusOkay': function does not take 2 arguments
 0:06.07 c:/Development/hg.mozilla.org/mozilla-win64/gfx/thebes/gfxWindowsPlatform.cpp(1950): error C2660: 'mozilla::gfx::FeatureState::Disable': function does not take 2 arguments

Comment 17

[Benoit Girard (:BenWa)]

ni? for re-landing reminder

Comment 18

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/394039f1525288b070565eec43e384c37ef76aa7
Backout d9111a0d9a44(bug 1274046) for windows bustage on a CLOSED TREE

Comment 19

[Benoit Girard (:BenWa)]

Comment on attachment 8754075 [details]
Bug 1274046 - Add FailureId to gfxConfig (FeatureState).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53706/diff/8-9/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f382e0ae3c2b
Add FailureId to gfxConfig (FeatureState). r=dvander

Comment 21

[Carsten Book [:Tomcat]]

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