Closed
Bug 1056388
Opened 10 years ago
Closed 9 years ago
Add some assertions to APZTestData.h
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: botond, Assigned: kats)
Details
Attachments
(1 file, 1 obsolete file)
2.46 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•10 years ago
|
||
Try push at https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=846d48e781cf includes this patch.
Assignee: nobody → bugmail.mozilla
Assignee | ||
Comment 2•10 years ago
|
||
The second assertion failed a bunch of reftests on B2G emulator.
Assignee | ||
Comment 3•10 years ago
|
||
(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
Assignee | ||
Comment 4•9 years ago
|
||
This one works fine https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3bd72d796569
Attachment #8526722 -
Attachment is obsolete: true
Attachment #8527256 -
Flags: review?(botond)
Reporter | ||
Comment 5•9 years ago
|
||
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+
Assignee | ||
Comment 6•9 years ago
|
||
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?
Reporter | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c625a976589d
https://hg.mozilla.org/mozilla-central/rev/c625a976589d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•