Closed Bug 1056388 Opened 5 years ago Closed 5 years ago

Add some assertions to APZTestData.h

Categories

(Core :: Panning and Zooming, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: botond, Assigned: kats)

Details

Attachments

(1 file, 1 obsolete file)

APZTestData.h contains the following comments ([1], [2]):

46     // TODO(botond): MOZ_ASSERT() that we didn't already have a paint with this
47     // sequence number once we get rid ofAPZCTreeManager::UpdatePanZoomControllerTree()
48     // calls for repeat transactions (bug 1007728).

97     // TODO(botond): MOZ_ASSERT() that we don't already have this key once we
98     // get rid of APZCTreeManager::UpdatePanZoomControllerTree() calls for
99     // repeat transactions (bug 1007728).

Bug 1007728 has been fixed, so we should add the assertions the comments suggest.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/testutil/APZTestData.h?rev=fcf15eb82338#46
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/testutil/APZTestData.h?rev=fcf15eb82338#97
Attached patch Patch (obsolete) — Splinter Review
Try push at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=846d48e781cf includes this patch.
Assignee: nobody → bugmail.mozilla
The second assertion failed a bunch of reftests on B2G emulator.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> The second assertion failed a bunch of reftests on B2G emulator.

s/reftests/mochitests/

I suspect this is because of multi-layer-APZ. The call at [1] may get invoked for the same scrollId multiple times, which will trigger the assertion at [2]. Perhaps the assertion should check that the value being put in is the same as the existing value, if any?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=42c5ee6094ee#689
[2] https://hg.mozilla.org/try/file/c755646a071a/gfx/layers/apz/testutil/APZTestData.h#l95
Comment on attachment 8527256 [details] [diff] [review]
Patch

Review of attachment 8527256 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the suggested changes.

Thanks for taking this, Kats! It's one of the loose ends I meant to tie up but forgot about.

::: gfx/layers/apz/testutil/APZTestData.h
@@ +47,1 @@
>      mPaints.insert(DataStore::value_type(aSequenceNumber, Bucket()));

In debug mode, find() and insert() will each perform a search of the map. This can be avoided:

auto insertResult = mPaints.insert(DataStore::value_type(aSequenceNumber, Bucket()));
MOZ_ASSERT(insertResult.second);

@@ +97,1 @@
>      scrollFrameData.insert(ScrollFrameData::value_type(aKey, aValue));

Here, find(), operator[] and insert() would perform a total of three searches of the map :) It's again possible to make do with just one:

auto insertResult = scrollFrameData.insert(ScrollFrameData::value_type(aKey, aValue));
MOZ_ASSERT(insertResult.second 
        || insertResult.first->second == aValue);
Attachment #8527256 - Flags: review?(botond) → review+
Both of those will leave "insertResult" as unused in non-debug builds, so I would further have to wrap it in a DebugOnly<>. But even if I did that, I feel like this change sacrifices readability for the sake of debug-only optimizations, for a piece of code that is only exercised in tests to begin with. Is it really worth it?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Both of those will leave "insertResult" as unused in non-debug builds, so I
> would further have to wrap it in a DebugOnly<>. 

Ah, good point.

It would be nice to have a version of DebugOnly<> that worked well with 'auto', i.e. one where you didn't have to write out the type it's wrapping. Since we don't have such a thing yet...

> But even if I did that, I
> feel like this change sacrifices readability for the sake of debug-only
> optimizations, for a piece of code that is only exercised in tests to begin
> with. Is it really worth it?

... I'm OK with landing as-is.
https://hg.mozilla.org/mozilla-central/rev/c625a976589d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.