[webkit-reviews] review canceled: [Bug 89035] Add flushes to CCTextureUpdater::update : [Attachment 150699] Patch

[bugzilla-daemon at webkit.org]

Nat Duca <nduca at chromium.org> has canceled Brian Anderson
<brianderson at chromium.org>'s request for review:
Bug 89035: Add flushes to CCTextureUpdater::update
https://bugs.webkit.org/show_bug.cgi?id=89035

Attachment 150699: Patch
https://bugs.webkit.org/attachment.cgi?id=150699&action=review

------- Additional Comments from Nat Duca <nduca at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=150699&action=review


Sorry, the test isn't looking quite right, yet. You've got to do something
about this global tracker, which is a non-normative design pattern. I think you
can just make each of the MyXXX classess have pointers to the
CCTextureUpdaterTest object and move all the GlobalTracker fields to the Test
body.

You also still need to make the tests "say what they're testing." Right now,
they set up different situations, which is good. But they are just saying
"verify" but its not clear what they're verifying. Maybe see what the
verification code looks like when you hand-type-it-out each time? Many of the
test cases dont need every single item in your verify() method to be done, too.
You only need to verify things that are interestnig.

I do see that you have a lot of *very* similar cases for these remainder tests
[which I dont quite grok]. For those, you could make a subclass of
CCTextureUpdaterTest, eg. CCTextureUpdaterRemainderTest that has the verify
function it. That would be maybe be okay.

As I note below, I'm a bit worried that all the tests pass kMax to the updater.
The common use case is "lots of updates, and kMax=40 [or 12]". Do we have good
coverage of that case?

Any idea why its not cleanly applying to ToT? Those purple indicators on the
patch usually imply patch failure.

> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:107
> +	   int uploadCount = 0;

fullUploadCount?

>> Source/WebCore/platform/graphics/chromium/cc/CCTextureUpdater.cpp:135
>> +	    int partialUploadCount = 0;
> 
> I didn't use index here, because there were a lot of ugly off-by-one
adjustments needed without a separate partialUploadCount variable.

Erm, just for my education, when does (index + 1) % kUploaderFlushPeriod go
wrong?

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:142
> +class MyFakeWebGraphicsContext3D : public FakeWebGraphicsContext3D {

Why not construct this with a backpointer to CCTextureUpdaterTest? Then this
"GlobalTracker" can be the CCTextureUpdaterTest? I don't see value add in this
GlobalTracker object.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:236
> +    m_globalTracker.verify();

Just verify inline. This one doesn't need to verify all the pieces... here, you
just need to make sure no flushes were done.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:241
> +TEST_F(CCTextureUpdaterTest, OneTextureUploadA)

OneFullTextureUpload

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:246
> +    m_globalTracker.verify();

Here, you want really to verify that what, one flush was done? I dont think you
need any other verifications here.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:249
> +TEST_F(CCTextureUpdaterTest, OneTextureUploadB)

OnePartialTexturUPload

xxxA and xxxB is not informative to a person not familiar with this code
desperately trying to understand why a test is failing.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:257
> +TEST_F(CCTextureUpdaterTest, OneTextureUploadC)

OnePartialOneFull

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:262
> +    m_globalTracker.verify();

Again, I think the number of things you really care about here is that we did
one flush, right? Or two? Or none?

By way of example, in this case, I know your code pretty well but I can't even
quite remember how many flushes we do here. And, to figure out what the correct
expectation is, I've actually got to study the code in detail. Versus just
reading a number.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:266
> +// NO REMAINDER TESTS

You should explain what this family of tests is doing. I'm a bit puzzled what
you're trying to test here, I suspect it has to do with some quirk with %
logic, but its not obvious to me from inspection.

> Source/WebKit/chromium/tests/CCTextureUpdaterTest.cpp:274
> +    m_updater.update(m_context.get(), &m_allocator, &m_copier, &m_uploader,
kMaxUploads);

Why are you passing kMaxUploads to all of these test methods? It seems to me
that somewhere, you want to pass like something smaller [like the value we pass
into the updater from CCThreadProxy?]