Closed
Bug 1288643
Opened 8 years ago
Closed 5 years ago
Implement modern context-loss detachment for objects
Categories
(Core :: Graphics: CanvasWebGL, defect, P3)
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
(Keywords: feature, Whiteboard: gfx-noted)
Attachments
(9 files)
58 bytes,
text/x-review-board-request
|
jerry
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jerry
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jerry
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jerry
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jerry
:
review-
|
Details |
58 bytes,
text/x-review-board-request
|
jerry
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jerry
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jerry
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jerry
:
review+
|
Details |
The rules changed a couple times, but they should be stable after this recent clarification. Let's take the opportunity to update and simplify some things.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66424/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66424/
Attachment #8773663 -
Flags: review?(hshih)
Attachment #8773664 -
Flags: review?(hshih)
Attachment #8773665 -
Flags: review?(hshih)
Attachment #8773666 -
Flags: review?(hshih)
Attachment #8773667 -
Flags: review?(hshih)
Attachment #8773668 -
Flags: review?(hshih)
Attachment #8773669 -
Flags: review?(hshih)
Attachment #8773670 -
Flags: review?(hshih)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66426/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66426/
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66428/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66428/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66430/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66430/
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66432/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66432/
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66434/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66434/
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66436/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66436/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66438/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66438/
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66440/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66440/
Comment 10•8 years ago
|
||
Comment on attachment 8773666 [details] Bug 1288643 - Delete on Detach. - https://reviewboard.mozilla.org/r/66430/#review63552
Attachment #8773666 -
Flags: review?(hshih) → review+
Comment hidden (obsolete) |
Comment 12•8 years ago
|
||
Comment on attachment 8773667 [details] Bug 1288643 - WebGLRefCountedObject implies WebGLContextBoundObject. - https://reviewboard.mozilla.org/r/66432/#review63556
Attachment #8773667 -
Flags: review?(hshih) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8773668 [details] Bug 1288643 - Since we detach now, just check if the object is detached to determine if lost. - https://reviewboard.mozilla.org/r/66434/#review63558
Attachment #8773668 -
Flags: review?(hshih) → review+
Comment 14•8 years ago
|
||
Comment on attachment 8773665 [details] Bug 1288643 - mContext is mutable, so is now protected. - https://reviewboard.mozilla.org/r/66428/#review63960 ::: dom/canvas/WebGLProgram.cpp:124 (Diff revision 1) > //#define DUMP_SHADERVAR_MAPPINGS > > static already_AddRefed<const webgl::LinkedProgramInfo> > QueryProgramInfo(WebGLProgram* prog, gl::GLContext* gl) > { > - WebGLContext* const webgl = prog->mContext; > + const auto& webgl = prog->Context(); Is "const auto webgl" enough? ::: dom/canvas/WebGLTextureUpload.cpp:773 (Diff revision 1) > height == imageInfo->mHeight && > depth == imageInfo->mDepth); > if (isFullUpload) { > *out_uploadWillInitialize = true; > } else { > - WebGLContext* webgl = tex->mContext; > + const auto& webgl = tex->Context(); Is "const auto webgl" enough?
Attachment #8773665 -
Flags: review?(hshih) → review+
Comment 15•8 years ago
|
||
Comment on attachment 8773664 [details] Bug 1288643 - Don't put up with macro redefinition. - https://reviewboard.mozilla.org/r/66426/#review63962
Attachment #8773664 -
Flags: review?(hshih) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8773667 [details] Bug 1288643 - WebGLRefCountedObject implies WebGLContextBoundObject. - https://reviewboard.mozilla.org/r/66432/#review64358 ::: dom/canvas/WebGLObjectModel.h:95 (Diff revision 1) > * parameter, as a means to allow DeleteOnce to call Delete() on the Derived > * class, without either method being virtual. This is a common C++ pattern > * known as the "curiously recursive template pattern (CRTP)". > */ > template<typename Derived> > -class WebGLRefCountedObject > +class WebGLRefCountedObject : public WebGLContextBoundObject The WebGLContextBoundObject definition should be placed before WebGLRefCountedObject. There is a build break here.
Attachment #8773667 -
Flags: review+ → review-
Comment 17•8 years ago
|
||
Comment on attachment 8773670 [details] Bug 1288643 - Add basic ctors for non-macro'd extensions. - https://reviewboard.mozilla.org/r/66438/#review64376 ::: dom/canvas/WebGLExtensions.h:72 (Diff revision 1) > { > public: > - explicit WebGLExtensionDebugShaders(WebGLContext*); > + explicit WebGLExtensionDebugShaders(WebGLContext* webgl) > + : WebGLExtensionBase(webgl) > + { > + IsSupported(webgl); moz_assert for all "IsSupport()" checking. ::: dom/canvas/WebGLExtensions.h:87 (Diff revision 1) > { > public: > - explicit WebGLExtensionLoseContext(WebGLContext*); > + explicit WebGLExtensionLoseContext(WebGLContext* webgl) > + : WebGLExtensionBase(webgl) > + { > + IsSupported(webgl); moz_assert ::: dom/canvas/WebGLExtensions.h:136 (Diff revision 1) > { > public: > - explicit WebGLExtensionVertexArray(WebGLContext* webgl); > + explicit WebGLExtensionVertexArray(WebGLContext* webgl) > + : WebGLExtensionBase(webgl) > + { > + IsSupported(webgl); moz_assert ::: dom/canvas/WebGLExtensions.h:154 (Diff revision 1) > { > public: > - explicit WebGLExtensionInstancedArrays(WebGLContext* webgl); > + explicit WebGLExtensionInstancedArrays(WebGLContext* webgl) > + : WebGLExtensionBase(webgl) > + { > + IsSupported(webgl); moz_assert
Attachment #8773670 -
Flags: review?(hshih) → review+
Comment 18•8 years ago
|
||
https://reviewboard.mozilla.org/r/66422/#review64402 ::: dom/canvas/WebGLExtensionCompressedTextureES3.cpp:48 (Diff revision 1) > } > > -WebGLExtensionCompressedTextureES3::~WebGLExtensionCompressedTextureES3() > +/*static*/ bool > +WebGLExtensionCompressedTextureES3::IsSupported(const WebGLContext* webgl) > { > + if (webgl->IsWebGL2()) check gfxPrefs::WebGLDraftExtensionsEnabled() ::: dom/canvas/WebGLExtensionDebugShaders.cpp:32 (Diff revision 1) > } > > +/*static*/ bool > +WebGLExtensionDebugShaders::IsSupported(const WebGLContext* webgl) > +{ > + return true; There is no switch case inside "WebGLContext::IsExtensionSupported(WebGLExtensionID ext)". If there is no gfxPrefs::WebGLPrivilegedExtensionsEnabled(), gecko return false for this. ::: dom/canvas/WebGLExtensionDisjointTimerQuery.cpp:246 (Diff revision 1) > +} > + > +/*static*/ bool > WebGLExtensionDisjointTimerQuery::IsSupported(const WebGLContext* webgl) > { > + if (webgl->IsWebGL2()) check gfxPrefs::WebGLDraftExtensionsEnabled()?
Updated•8 years ago
|
Attachment #8773663 -
Flags: review?(hshih) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8773663 [details] Bug 1288643 - Reduce extension boilerplate. - https://reviewboard.mozilla.org/r/66424/#review64366 ::: dom/canvas/WebGLExtensions.h:197 (Diff revision 1) > + { \ > + public: \ > + explicit T(WebGLContext* webgl); \ > + : WebGLExtensionBase(webgl) \ > + { \ > + IsSupported(webgl); \ MOZ_ASSERT(IsSupported(webgl))? If the ext is not supported, we should not create that one.
Comment 20•8 years ago
|
||
Comment on attachment 8773669 [details] Bug 1288643 - Bugs in WebGLExt*. - https://reviewboard.mozilla.org/r/66436/#review64428
Attachment #8773669 -
Flags: review?(hshih) → review+
Comment 21•8 years ago
|
||
Comment on attachment 8773671 [details] Bug 1288643 - Build fixes. https://reviewboard.mozilla.org/r/66440/#review64430
Attachment #8773671 -
Flags: review+
Assignee | ||
Comment 22•7 years ago
|
||
Some parts of this have been implemented in bug 1320030.
See Also: → 1320030
Assignee | ||
Updated•7 years ago
|
status-firefox50:
affected → ---
Assignee | ||
Updated•5 years ago
|
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•