Closed
Bug 1286161
Opened 8 years ago
Closed 8 years ago
Add a GTest suite for SourceBuffer to ImageLib
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(2 files, 2 obsolete files)
4.79 KB,
patch
|
eflores
:
review+
|
Details | Diff | Splinter Review |
32.15 KB,
patch
|
Details | Diff | Splinter Review |
SourceBuffer is of fundamental importance in ImageLib and it really needs a GTest suite. Now is a good time to do it since I'm adding a couple of small features. (The bugs will block this one.)
Assignee | ||
Comment 1•8 years ago
|
||
This patch just exposes some constants which are useful for the test suite in part 2.
Attachment #8770046 -
Flags: review?(edwin)
Assignee | ||
Comment 2•8 years ago
|
||
This patch adds a fairly comprehensive test suite for SourceBuffer. It includes coverage for the support for null IResumables and advancing by specific amounts that have been added in recent patches. Apologies for how much code is here; a new GTest suite is always a bear to review. =)
Attachment #8770048 -
Flags: review?(edwin)
Attachment #8770046 -
Flags: review?(edwin) → review+
Comment on attachment 8770048 [details] [diff] [review] (Part 2) - Add a GTest suite for SourceBuffer. Review of attachment 8770048 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/test/gtest/TestSourceBuffer.cpp @@ +89,5 @@ > + { > + EXPECT_TRUE(NS_SUCCEEDED(mSourceBuffer->Append(aData, aLength))); > + } > + > + void CheckedAppendToBufferLastByeForLength(size_t aLength) nit: "Bye" @@ +627,5 @@ > + CheckedAdvanceIteratorStateOnly(iterator, 1, 2, totalLength, > + AdvanceMode::eAdvanceByLengthExactly); > + CheckData(iterator.Data(), offset++, iterator.Length()); > + } > + nit: whitespace @@ +680,5 @@ > + > + // Advance past the new data and wait again. > + CheckedAdvanceIterator(iterator, firstWriteLength); > + CheckIteratorMustWait(iterator, mCountResumes); > + nit: whitespace
Attachment #8770048 -
Flags: review?(edwin) → review+
Assignee | ||
Comment 4•8 years ago
|
||
Thanks for the review! I've addressed the issues above and also added one new test, which ensures that we don't advance to a new chunk if the iterator is at a chunk boundary and we do a zero-byte read. (In other words, it tests the tweak I added in the updated patch in bug 1286175.)
Assignee | ||
Updated•8 years ago
|
Attachment #8770048 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8770704 [details] [diff] [review] (Part 2) - Add a GTest suite for SourceBuffer. Review of attachment 8770704 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/test/gtest/TestSourceBuffer.cpp @@ +632,5 @@ > + // Make sure we reached the end. > + CheckIteratorIsComplete(iterator, 2, totalLength); > +} > + > +TEST_F(ImageSourceBuffer, SubchunkZeroByteAdvance) Actually, it's two new tests. This one... @@ +687,5 @@ > + // Make sure we reached the end. > + CheckIteratorIsComplete(iterator, 2, totalLength); > +} > + > +TEST_F(ImageSourceBuffer, SubchunkZeroByteAdvanceWithNoData) ... and this one. They're both very straightforward variants of existing tests.
Assignee | ||
Comment 6•8 years ago
|
||
Added one more quick test that checks that appending to a SourceBuffer after calling Complete() has no effect.
Assignee | ||
Updated•8 years ago
|
Attachment #8770704 -
Attachment is obsolete: true
Pushed by mfowler@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d457ff02be (Part 1) - Expose SourceBuffer and SurfaceCache constants which are useful for testing. r=edwin https://hg.mozilla.org/integration/mozilla-inbound/rev/762323d53afa (Part 2) - Add a GTest suite for SourceBuffer. r=edwin
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a6d457ff02be https://hg.mozilla.org/mozilla-central/rev/762323d53afa
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•