Add a GTest suite for SourceBuffer to ImageLib

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

3 years ago
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

Updated

3 years ago
Depends on: 1286165
Assignee

Updated

3 years ago
Depends on: 1286170
Assignee

Updated

3 years ago
Depends on: 1286175
Assignee

Comment 1

3 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

3 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)
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

3 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

3 years ago
Attachment #8770048 - Attachment is obsolete: true
Assignee

Comment 5

3 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

3 years ago
Added one more quick test that checks that appending to a SourceBuffer after
calling Complete() has no effect.
Assignee

Updated

3 years ago
Attachment #8770704 - Attachment is obsolete: true

Comment 7

3 years ago
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a6d457ff02be
https://hg.mozilla.org/mozilla-central/rev/762323d53afa
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Assignee

Updated

3 years ago
Duplicate of this bug: 1284033
You need to log in before you can comment on or make changes to this bug.