Closed Bug 1728267 Opened 3 months ago Closed 2 months ago

Remove implicit ToResult from QM_TRY macros

Categories

(Core :: Storage: Quota Manager, task, P2)

task

Tracking

()

RESOLVED FIXED
94 Branch
Tracking Status
firefox94 --- fixed

People

(Reporter: janv, Assigned: janv)

References

Details

Attachments

(14 files, 2 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

Some QM_TRY macros contain implicit ToResult which makes it easier to use with expressions which don't return mozilla::Result directly. However, when we need to add for example QM_OR_ELSE_WARN, an explicit ToResult must be added. I think it would be better and more consistent if we removed the implicit ToResult and used explicit sub macros (like MOZ_TO_RESULT) for conversion to mozilla::Result instead.

There are other things in favor of the explicit sub macros:

  • an explicit conversion to mozilla::Result signals a start of an error stack
  • try! in rust requires a Result to be passed as well
  • precise error stacks (QMresult) need an explicit conversion anyway
  • better consistency with existing MOZ_TO_RESULT_INVOKE sub macros

So, ideally, if an expression doesn't return mozilla::Result we should wrap it with one of these sub macros:
MOZ_TO_RESULT
MOZ_TO_RESULT_INVOKE
MOZ_TO_RESULT_INVOKE_TYPED
MOZ_TO_RESULT_GET_TYPED

We will later add QM_ counterparts for functions returning the new OkOrErr (Result<Ok, QMResult).

Depends on D125169

This will also prevent implicit conversion of Result<V, QMResult> to
Result<V, nsresult>.

Depends on D125310

This is needed especially when QMResult is used in some global initialization
like InitializeQuotaManager.

Depends on D125312

We can use QM_TO_RESULT (instead of MOZ_TO_RESULT) because QM_WARNONLY_TRY
doesn't propagate errors, so no other adjustment is needed.

Depends on D125313

Depends on D125323

Comment on attachment 9240716 [details]
Bug 1728267 - Replace existing ToResult uses wth MOZ_TO_RESULT; r=#dom-storage

Revision D125324 was moved to bug 1731544. Setting attachment 9240716 [details] to obsolete.

Attachment #9240716 - Attachment is obsolete: true

Comment on attachment 9240717 [details]
Bug 1728267 - Replace existing ToResultGet uses with MOZ_TO_RESULT_GET_TYPED; r=#dom-storage

Revision D125325 was moved to bug 1731546. Setting attachment 9240717 [details] to obsolete.

Attachment #9240717 - Attachment is obsolete: true

(In reply to Jan Varga [:janv] from comment #0)

  • precise error stacks (QMresult) need an explicit conversion anyway

For my better understanding: does this mean that many (all?) call sites that àccording to these patches will use MOZ_TO_RESULT should better use QM_TO_RESULT in the future?

Flags: needinfo?(jvarga)

(In reply to Jens Stutte [:jstutte] from comment #18)

(In reply to Jan Varga [:janv] from comment #0)

  • precise error stacks (QMresult) need an explicit conversion anyway

For my better understanding: does this mean that many (all?) call sites that àccording to these patches will use MOZ_TO_RESULT should better use QM_TO_RESULT in the future?

Yes, in the future. One of the patches already uses QM_TO_RESULT, it's for warnings when error are not propagated, so no conversion from QMResult to nsresult is needed.

Flags: needinfo?(jvarga)
Keywords: leave-open
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dda21747235a
Introduce MOZ_TO_RESULT; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/9a6c66385edd
Remove auto conversion of QMResult to nsresult; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/9cf7fc718f8b
Remove implicit ToResult from QM_TRY_RETURN; r=dom-storage-reviewers,jstutte,jari
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e682992a9a96
Fix TestQMResult.cpp to start with the current last stack id instead of using a hard coded constant; r=dom-storage-reviewers,jstutte
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81d355503575
Remove implicit ToResult from QM_WARNONLY_TRY macros; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/bf3d498a444e
Remove implicit ToResult from QM_TRY when two extra arguments are passed; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/560f2fb34f4c
Remove implicit ToResult from QM_TRY when one extra argument is passed; r=dom-storage-reviewers,jstutte,jari
Keywords: leave-open
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cde45992f903
Remove implicit ToResult from QM_TRY when no extra argument is passed in dom/quota; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/d60cca5a9566
Remove implicit ToResult from QM_TRY when no extra argument is passed in dom/indexedDB; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/e67e7301c573
Remove implicit ToResult from QM_TRY when no extra argument is passed in dom/cache; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/26fc082dc4f3
Remove implicit ToResult from QM_TRY when no extra argument is passed in dom/simpledb; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/97daac17f849
Remove implicit ToResult from QM_TRY when no extra argument is passed in dom/localstorage; r=dom-storage-reviewers,jstutte,jari
https://hg.mozilla.org/integration/autoland/rev/226019fce13a
Remove implicit ToResult from QM_TRY when no extra argument is passed; r=dom-storage-reviewers,jstutte,jari
Regressions: 1732467
Keywords: leave-open
Keywords: leave-open
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a93548920ba7
Fix compilation when QM_ERROR_STACKS_ENABLED is not defined; r=dom-storage-reviewers,jstutte
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch
You need to log in before you can comment on or make changes to this bug.