We are increasingly using asynchronous panning and zooming (APZ) on our mobile platforms. (For an overview of APZ, see https://wiki.mozilla.org/Platform/GFX/APZ.) The C++ implementation was initially used only in the B2G browser, but we have now ported it to Metro and enabled it for other B2G apps as well, and we hope to eventually port it to Fennec (which currently has a separate Java implementation). Over the course of this work, we have expanded the scope of APZ and made many changes to it, both bug fixes and new features. However, we have written very few tests to go along with these changes, which is unfortunate because we are missing out on the benefits of automated tests catching regressions early. One reason for this is that we don't have a good mechanism for writing APZ tests. We have some gtests [1], but they only test APZCTreeManager and AsyncPanZoomController in isolation, while a lot of the interesting things to test involve interactions between APZ and the compositor, widget code, and gecko. Setting up components like the compositor, widget code, and gecko in a gtest is (according to my understanding) highly nontrivial, so we should explore an alternate avenue for testing APZ in action. A promising idea, suggested by Jeff, is to write mochitests, while putting into place the following new things: - A global buffer that can contain arbitrary data. - Conditionally-compiled C++ code in APZ, gecko, widget code etc. that logs various things to this buffer. - An API that allows accessing this buffer from JS, e.g. via nsIDOMWindowUtils. The mochitests could then examine the contents of this buffer and assert that logged values that are of interest to the test are what is expected. For example, a mochitest could load a page for which we know what the initial zoom level, initial composition bounds, and other such metrics should be, and (with the cooperation of C++ code that logs these metrics to the buffer) assert that they are indeed those values. I'm not sure when I will get a chance to implement this mechanism, but if someone would take to take point on this before I do, I would be very glad to mentor them. I believe this would be a high value-added change, given the increasing use and scope of APZ and its complexity that often gives rise to unexpected regressions. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/tests/gtest/TestAsyncPanZoomController.cpp

I would like to get in on this.

(In reply to Rodrigo Rodriguez from comment #1) > I would like to get in on this. Great! I'm happy to give you guidance with this. To start, do you have a build set up? For this work, a Metro or B2G build would be ideal, as those are the platforms that have APZ enabled. However, since a lot of the testing mechanism can be more general-purpose than APZ, we can get started with any build (e.g. desktop).

Well I do not have a Metro or B2g build, but I could get the latter if that would make things a whole bunch more convenient.

(In reply to Rodrigo Rodriguez from comment #3) > Well I do not have a Metro or B2g build, but I could get the latter if that > would make things a whole bunch more convenient. Just to be clear, I do have a desktop build.

(In reply to Rodrigo Rodriguez from comment #4) > (In reply to Rodrigo Rodriguez from comment #3) > > Well I do not have a Metro or B2g build, but I could get the latter if that > > would make things a whole bunch more convenient. > > Just to be clear, I do have a desktop build. Let's start with the desktop build, I think we can make significant progress with that. This could be a first milestone that is already useful for APZ: (1) Add a function, accessible to C++ code, that logs an arbitrary string to a buffer. (2) Add a function, accessible to JS code, that returns the contents of this buffer. (3) Whenever layout code in Gecko calculates the "composition bounds" of a scrollable element, it logs this value to the buffer via (1). (The composition bounds of a scrollable element is an important property used by APZ. It is basically the size of the scrollable element (as opposed to the size of the scrolled content inside it). It's also called "scroll port" in some parts of the code.) (4) Write a mochitest that loads a simple page like [A], looks (via (2)) at the first composition bounds value that was logged (this should be the initial composition bounds when the page loads) and checks it against some known expected value. (The actual value is not important at this stage, you can just hardcode some value for now.) A good place in the code to interface between C++ and JS is nsDOMWindowUtils. Here's an overview of this class works: - nsIDOMWindowUtils.idl [B] defines an interface. The methods in this interface can be called by JS. - As part of the build process, a C++ header file called nsIDOMWindowUtils.h is generated, which declares an abstract class called nsIDOMWindowUtils with virtual methods corresponding to the methods in the IDL file. Since this file is generated, we don't make changes to it. - A C++ header file called nsDOMWindowUtils.h [C] (note the absence of the "I") declares a concrete class nsDOMWindowUtils which implements all of the methods declared in nsIDOMWindowUtils, as well as adding new methods which will be accessible to C++ only. - nsDOMWindowUtils.cpp [D] defines all the methods declared in ns[I]DOMWindowUtils.h. So for (1), since we are writing a function accessible to C++ code, we can declare it as a method of nsDOMWindowUtils in nsDOMWindowUtils.h, and implement it in nsDOMWindowUtils.cpp. The buffer that this method writes to can also live in the nsDOMWindowUtils class. For (2), since we are writing a function accessible to JS code, we can declare it as a method of nsIDOMWindowUtils in nsIDOMWindowUtils.idl. We can then run the build process to get it to generate the corresponding C++ method declaration in nsIDOMWindowUtils.h, and then write its implementation in nsIDOMWindowUtils.cpp. For (3), the place in the code where the composition bounds is calculated is [E]. For (4), the actual writing the mochitest part is new to me too, so we will learn about this together :) I hope I've given you enough info to get started. I left some decisions up to you like how to format the data in the buffer - use your best judgment. Feel free to take liberties like having an array of messages instead of a single flat buffer, if that is more convenient. Feel free to ask me any questions you have via email or IRC (my nick is "botond" and you can find me in #gfx), or by posting in this bug. If I'm not around other people are usually happy to help too (#developers is a good IRC channel to ask general questions). [A] http://people.mozilla.org/~bballo/grid.html [B] http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl [C] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.h [D} http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp [E] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#701

Okay awesome, I will get into this tonight and read up. I was already reading the APZ wiki and some documentation to better familiarize myself with this component.

Sorry if this is redundant, I am just trying to clear up my understanding of the problem and the possible solution. GOAL: Create a mechanism that will enable test writers to more trivially create tests for APZ. Currently the solution to creating automated testing is gtest. This solution is not satisfactory because this automated testing only targets APZCTreeManager and AysncPanZoomController in isolation, and ignores interactions between APZ and the compositor, widget code, and gecko. SOLUTION: Write Mochitests. Mochitest has the ability to load a metric-specific page that will enable developrs to gauge the APZ reaction to different scenarios. APZ generated metrics are compared to the expected metrics. The APZ generated metrics will be found in a buffer that we have to implement. The solution comes in two parts. The C++ component and the JS component. THE C++ COMPONENT: This part of the solution is what actually places arbitrary data( strings ) in a buffer. The data found in this buffer is generated by the APZ. So this APZ generated data is what will be tested by the mochitests. THE JS Component: In order to compare the APZ generated data with the expected data we need to retrieve it from the buffer. This is where the JS component comes into play, the JS component is used to retrieve data. Please feel free to correct me if I misinterpreted the problem and solution.

(In reply to Rodrigo Rodriguez from comment #7) > Sorry if this is redundant, I am just trying to clear up my understanding of > the problem and the possible solution. > > GOAL: Create a mechanism that will enable test writers to more trivially > create tests for APZ. Currently the solution to creating automated testing > is gtest. This solution is not satisfactory because this automated testing > only targets APZCTreeManager and AysncPanZoomController in isolation, and > ignores interactions between APZ and the compositor, widget code, and gecko. > > SOLUTION: Write Mochitests. Mochitest has the ability to load a > metric-specific page that will enable developrs to gauge the APZ reaction to > different scenarios. APZ generated metrics are compared to the expected > metrics. The APZ generated metrics will be found in a buffer that we have to > implement. > > The solution comes in two parts. The C++ component and the JS component. > > THE C++ COMPONENT: This part of the solution is what actually places > arbitrary data( strings ) in a buffer. The data found in this buffer is > generated by the APZ. So this APZ generated data is what will be tested by > the mochitests. > > THE JS Component: In order to compare the APZ generated data with the > expected data we need to retrieve it from the buffer. This is where the JS > component comes into play, the JS component is used to retrieve data. > > > Please feel free to correct me if I misinterpreted the problem and solution. Yup, that 's the idea. Just one small clarification: while most of the tests we'll write will place values calculated by APZ into the buffer, this first test that I described will put something (composition bounds) that is calculated by Gecko's layout code into the buffer. (APZ uses this value, which is why we care about it.)

(In reply to Rodrigo Rodriguez from comment #6) > Okay awesome, I will get into this tonight and read up. I was already > reading the APZ wiki and some documentation to better familiarize myself > with this component. Hi Rodrigo, how is this going? Is there anything I can help you with? Please feel free to ping me on IRC or email me any time!

Been slightly busy trying to get my career started, but I am going to start to dedicate some time to this bug. Thank! Will do!

I have created a "placeholder" function, and I am trying to wrap my head around every component that plays a part in this. I'm probably over thinking it. Will this buffer exist only in the scope of the function? and the items we are going to be writing to it will only consist of string types?

I'll jump in since Botond is away this week. (In reply to Rodrigo Rodriguez from comment #11) > Will this buffer exist only in the scope of the function? and the items we > are going to be writing to it will only consist of string types? These are good questions. I think we'll need at least two functions, one to be called from C++ code to append things into the buffer, and one to be called from the test code to read things out of the buffer. The buffer needs to exist across both functions, so it probably makes sense to have it live on the DOMWindowUtils object. I think it might be preferable and simpler to make the buffer static, so that the one copy is shared across all the DOMWindowUtils objects for different windows. I think this would be better because many of the call sites that we're planning on using the buffer from wouldn't have a specific DOMWindowUtils that they're associated with. As to the data types we're putting in, I think for now we should go with just string types and then later if we need more complexity we can add it.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12) > I'll jump in since Botond is away this week. > > (In reply to Rodrigo Rodriguez from comment #11) > > Will this buffer exist only in the scope of the function? and the items we > > are going to be writing to it will only consist of string types? > > These are good questions. I think we'll need at least two functions, one to > be called from C++ code to append things into the buffer, and one to be > called from the test code to read things out of the buffer. The buffer needs > to exist across both functions, so it probably makes sense to have it live > on the DOMWindowUtils object. Okay that makes complete sense, should of occurred to me earlier :) > I think it might be preferable and simpler to make the buffer static, so > that the one copy is shared across all the DOMWindowUtils objects for > different windows. I think this would be better because many of the call > sites that we're planning on using the buffer from wouldn't have a specific > DOMWindowUtils that they're associated with. According to Botond, Gecko Layout code calculates "composition bounds" or "Scroll Port" and it is this value which will be saved to the buffer. There are quite a few places in source where "composition bounds" or "scroll port" are mentioned but I believe I found a place where these values are called for by the DOMWindowsUtils object, to use as a sort of reference http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils.cpp#1822 If this is actually what's happening, does this mean that the method that we will write and that will load the buffer will have to convert an object of the type "nsIScrollableFrame" into a string?

(In reply to Rodrigo Rodriguez from comment #13) > According to Botond, Gecko Layout code calculates "composition bounds" or > "Scroll Port" and it is this value which will be saved to the buffer. There > are quite a few places in source where "composition bounds" or "scroll port" > are mentioned but I believe I found a place where these values are called > for by the DOMWindowsUtils object, to use as a sort of reference > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMWindowUtils. > cpp#1822 > If this is actually what's happening, does this mean that the method that we > will write and that will load the buffer will have to convert an object of > the type "nsIScrollableFrame" into a string? The composition bounds Botond was referring to are the ones at [1] (it's a rect). They are referenced from various points in the code. For the purposes of this bug it's not particularly relevant exactly what data we want to put in the buffer though, as long as it just takes string inputs it should be fine. The composition bounds and other structures we want to log can be converted into strings and passed to your function. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/FrameMetrics.h#187

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #12) > I'll jump in since Botond is away this week. Thanks! To add to what Kats said, I think a good place to log the composition bounds is when layout calculates and sets it, which happens in RecordFrameMetrics [1]. [1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#706

So far I have this, can someone let me know if I am going in the right direction? I do realize that implementing this way would not allow any other method to access the data but since I'm quite knew with using "static" I don't know where I should be placing the buffer.

(In reply to Rodrigo Rodriguez from comment #16) > Created attachment 8375189 [details] [diff] [review] > bug961289_CplusplusAccessFunction.diff > > So far I have this, can someone let me know if I am going in the right > direction? I do realize that implementing this way would not allow any other > method to access the data but since I'm quite knew with using "static" I > don't know where I should be placing the buffer. You'll want to make the buffer a static field of the class, precisely for the reason you mentioned: to allow the other method (the one that reads from it) to access it. You can add a static field to a class like so: class C { ... static Type name; }; Also, it's better to use a dynamically resizable array class like mozilla::Vector (our analogue of std::vector) instead of a fixed size array + variable keeping track of the size.

(In reply to Botond Ballo [:botond] [c++ standards meeting Feb 10-15] from comment #17) > Also, it's better to use a dynamically resizable array class like > mozilla::Vector (our analogue of std::vector) instead of a fixed size array > + variable keeping track of the size. Where can I find out what operations are implmented by mozilla::vector? I don't suppose they would be the same as std::vector? Could I do something like this: static mozilla::vector<std::string> buffer; buffer.push_back( data );

(In reply to Rodrigo Rodriguez from comment #18) > (In reply to Botond Ballo [:botond] [c++ standards meeting Feb 10-15] from > comment #17) > > Also, it's better to use a dynamically resizable array class like > > mozilla::Vector (our analogue of std::vector) instead of a fixed size array > > + variable keeping track of the size. > > Where can I find out what operations are implmented by mozilla::vector? I > don't suppose they would be the same as std::vector? I believe the intention of the MFBT versions of standard library classes is to have the same interface as the standard library classes plus maybe additional features. There may be some exceptions, but I think you're unlikely to run into them here. If ever in doubt, you can check the header that defines mozilla::Vector [1]. > Could I do something > like this: > > static mozilla::vector<std::string> buffer; > buffer.push_back( data ); Yup. (Note the capital 'V' in mozilla::Vector though.) [1] https://mxr.mozilla.org/mozilla-central/source/mfbt/Vector.h

Fixed all syntax errors as well as added some "const" keywords to ensure that the string is not unintentionally modified.

The patch posted so far is in the right direction. Rodrigo, do you know what to do as the next step? Feel free to ping me on IRC if you'd like to chat about next steps for this bug.

Haven't heard from Rodrigo for a while, unassigning so that other people who might want to work on this see it as available. Rodrigo, feel free to reassign to yourself if you resume working on this.

I'm going to take this myself as it's becoming more and more pressing that we have this. If subtasks suitable for mentoring emerge, I will mark them as such.

After thinking about this for a while and consulting with a few people (:BenWa, :ehsan and :ahal), I have come up with an initial idea for a high-level design: - On the content thread, maintain a "paint sequence number" which is incremented on every paint. This can probably be stored in ClientLayerManager or suchlike. - When a layer transaction is forwarded to the compositor thread, send the paint sequence number along. - (In the event of multiple transactions per paint, which happens with progressive rendering, we may also want a "transaction sequence number" which resets on each paint. In the foregoing I will assume one transaction per paint for simplicity, but it is straightforward to extend this design to accommodate multiple transactions per paint.) - Both the content thread and the compositor thread will maintain an instance of a data structure which I'll call "Test Data" for now. This data structure will have a bucket for each paint sequence number, and within each bucket a map from string keys to string values. Let's call the two instances of this data structure the "Content-Side Test Data" and the "Compositor-Side Test Data", respectively. - When the content thread, over the course of painting (in which I'm including display list building, layer building, etc.) calculates values that are of interest to tests, it writes them into the Content-Side Test Data. As an example, RecordFrameMetrics() might store a rect (encoded as a string) under the key "root composition bounds" in the bucket for the current paint sequence number. - When the compositor thread, upon receiving a transaction associated with a particular paint sequence number, calculates values that are of interest to tests, it writes them into the Compositor-Side Test Data. As an example, AsyncPanZoomController::RequestContentRepaint() might store a rect (encoded as a string) under the key "root display port" in the bucket for the current paint sequence number. - Tests will be written as mochitests, and will generally operate by loading a page, perhaps doing some things on the page, and then examining the Content- and Compositor-Side Test Data, using the fact that both data structures are keyed by the same paint sequence number to correlate data between the two. For example, a test might check that the "root display port" calculated by the compositor thread in response to a particular paint, is at least as large as the "root composition bounds" calculated by the content thread during that paint. - Since the tests will run on the content thread, the content thread will have a mechanism for requesting the Compositor-Side Test Data from the compositor thread for examination. On B2G, where the content thread and the compositor thread are in different processes, this will involve transferring the data over IPDL. - If it becomes apparent over the course of writing tests that exercise this infrastructure, that additional substructure in the Test Data (beyond keying things by paint sequence number) is useful, we can do so. For example, perhaps it would be useful to have a sub-bucket for each scrollable frame, and have a key-value store within each of those. I can see two options for writing the tests themselves: OPTION 1 - The contents of the Test Data data structures can be encoded as JSON and made available to mochitest code via nsIDOMWindowUtils. The mochitest can then examine the data as make assertions on it in JS like other mochitests. OPTION 2 - We can have the mochitest JS code invoke a C++ function (via nsIDOMWindowUtils) that examines the Test Data and communicates information about whether the test should pass or fail back to JS, which can then make the test pass or fail appropriately. The API here can range from a different function for each test (probably undesirable) to a 'runTest(testid)' function which switches on an id and calls a specific C++ test function based on the id. Option 1 has the disadvantage of having to encode the Test Data into JSON. Option 2 has the disadvantage of increasing code bloat by having more C++ code in the binary, although we can conditionally compile that out for releases. I would appreciate any feedback on this design, including which of Option 1 or Option 2 is better as a means of writing the tests.

(In reply to Botond Ballo [:botond] from comment #25) > > - When the compositor thread, upon receiving a transaction associated with > a > particular paint sequence number, calculates values that are of interest > to > tests, it writes them into the Compositor-Side Test Data. > As an example, AsyncPanZoomController::RequestContentRepaint() might > store a > rect (encoded as a string) under the key "root display port" in the > bucket > for the current paint sequence number. RequestContentRepaint is not tied to the paint sequence number, and can be triggered independently. We might want to have a different sequence number for this side? > > - Tests will be written as mochitests, and will generally operate by > loading a > page, perhaps doing some things on the page, and then examining the > Content- > and Compositor-Side Test Data, using the fact that both data structures > are > keyed by the same paint sequence number to correlate data between the > two. > For example, a test might check that the "root display port" calculated > by > the compositor thread in response to a particular paint, is at least as > large > as the "root composition bounds" calculated by the content thread during > that > paint. This sounds good in general. One thing to make sure is that our test code is robust against changes in the painting heuristics. For example if two non-overlapping parts of the screen are invalidated, I believe layout could either paint them individually or just paint the union of the two rects. That sort of internal decision should not affect the result of the test, and that might be a little tricky. > - If it becomes apparent over the course of writing tests that exercise > this > infrastructure, that additional substructure in the Test Data (beyond > keying > things by paint sequence number) is useful, we can do so. For example, > perhaps > it would be useful to have a sub-bucket for each scrollable frame, and > have > a key-value store within each of those. Yeah, I think this will be needed and we might as well put it in the initial design. Perhaps sub-bucketing by content element would be the best way to go, since that is generally easy to obtain in layout (i.e. frame->GetContent()) and also easily accessible via the DOM in the test. > Option 1 has the disadvantage of having to encode the Test Data into JSON. > Option 2 has the disadvantage of increasing code bloat by having more C++ > code > in the binary, although we can conditionally compile that out for releases. I think I'm leaning towards option 1 at this point. I don't think it's hard to encode stuff into JSON, we can just write JSON-formatted strings using the printf-family of functions in C++ and unpack it in JS. For getting the test up and running quickly I think this approach is fine. If we find that it is too restricting in that we need log all sorts of stuff to get decent test coverage, and there's no way to exclude that from production builds, then we can switch to option 2 which involves building a separate binary like the gtest tests do.

Thanks for the feedback! (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #26) > (In reply to Botond Ballo [:botond] from comment #25) > > > > - When the compositor thread, upon receiving a transaction associated with > > a > > particular paint sequence number, calculates values that are of interest > > to > > tests, it writes them into the Compositor-Side Test Data. > > As an example, AsyncPanZoomController::RequestContentRepaint() might > > store a > > rect (encoded as a string) under the key "root display port" in the > > bucket > > for the current paint sequence number. > > RequestContentRepaint is not tied to the paint sequence number, and can be > triggered independently. We might want to have a different sequence number > for this side? That's a good idea! We can have a "repaint sequence number" on the compositor side, and have code in TabChild::UpdateRootFrame and ilk record information with that number. In the event of a repaint trigerred by a layers update, we can keep track of the paint sequence number triggering the repaint as well. > > - Tests will be written as mochitests, and will generally operate by > > loading a > > page, perhaps doing some things on the page, and then examining the > > Content- > > and Compositor-Side Test Data, using the fact that both data structures > > are > > keyed by the same paint sequence number to correlate data between the > > two. > > For example, a test might check that the "root display port" calculated > > by > > the compositor thread in response to a particular paint, is at least as > > large > > as the "root composition bounds" calculated by the content thread during > > that > > paint. > > This sounds good in general. One thing to make sure is that our test code is > robust against changes in the painting heuristics. For example if two > non-overlapping parts of the screen are invalidated, I believe layout could > either paint them individually or just paint the union of the two rects. > That sort of internal decision should not affect the result of the test, and > that might be a little tricky. Layout deciding how much to paint shouldn't affect things like the FrameMetrics it calculates, should it? > > - If it becomes apparent over the course of writing tests that exercise > > this > > infrastructure, that additional substructure in the Test Data (beyond > > keying > > things by paint sequence number) is useful, we can do so. For example, > > perhaps > > it would be useful to have a sub-bucket for each scrollable frame, and > > have > > a key-value store within each of those. > > Yeah, I think this will be needed and we might as well put it in the initial > design. Perhaps sub-bucketing by content element would be the best way to > go, since that is generally easy to obtain in layout (i.e. > frame->GetContent()) and also easily accessible via the DOM in the test. Sounds good. The key itself would be the scroll id for the content element (since we want the compositor-side to be able to use the same key); that's exposed to JS via nsIDOMWindowUtils, so that should be fine. > > Option 1 has the disadvantage of having to encode the Test Data into JSON. > > Option 2 has the disadvantage of increasing code bloat by having more C++ > > code > > in the binary, although we can conditionally compile that out for releases. > > I think I'm leaning towards option 1 at this point. I don't think it's hard > to encode stuff into JSON, we can just write JSON-formatted strings using > the printf-family of functions in C++ and unpack it in JS. For getting the > test up and running quickly I think this approach is fine. If we find that > it is too restricting in that we need log all sorts of stuff to get decent > test coverage, and there's no way to exclude that from production builds, > then we can switch to option 2 which involves building a separate binary > like the gtest tests do. Sounds reasonable. I'll assume option 1 for now, we can switch later if it becomes desirable as you say.

(In reply to Botond Ballo [:botond] from comment #27) > > This sounds good in general. One thing to make sure is that our test code is > > robust against changes in the painting heuristics. For example if two > > non-overlapping parts of the screen are invalidated, I believe layout could > > either paint them individually or just paint the union of the two rects. > > That sort of internal decision should not affect the result of the test, and > > that might be a little tricky. > > Layout deciding how much to paint shouldn't affect things like the > FrameMetrics it calculates, should it? Shouldn't affect FrameMetrics, but might affect the uploaded area. Not really a concern for us, I guess.

I began prototyping the design from comment 25. I'm posting some very early and incomplete WIP patches.

Posting updated patches. Still very much a WIP!

Remaining work: - Add a mechanism for transferring compositor-side APZ data to client side (This is what the Part 8 patch is anticipating.) I am thinking of using the PCompositor protocol for this. - Expose the APZ test data to JS via nsIDOMWindowUtils. This is where the yucky JSONification part comes in :/ - Write an example mochitest that exercises the framework. I am thinking of using bug 982141 as the first thing to write a test for. The Part 6 and Part 7 patches anticipate this by adding the platform-side logging that this test will require.

Ended up using PLayerTransaction rather than PCompositor for this.

Ended up using WebIDL to communicate the test data to JS rather than serializing to JSON.

Turns out you can't actually use std::tr1::unordered_map directly on all platforms because on some platforms the header is under <unordered_map>, and on others, <tr1/unordered_map>. base/hash_tables.h sorts that all out and exposes something consistent (and with an interface identical to unordered_map) as base::hash_map, so I updated my patches to use that instead.

With Ehsan's help, I got the transfer of APZTestData from C++ to JS to work correctly. Updated patch with the fixes.

Update on the status of this bug: I'm currently working on an example mochitest which tests bug 982141. I'm testing it on b2g-emulator, and I'm currenly trying to get the test assertions to run at the right time (which is to say, _after_ the events about which they wish to make assertions have happened). Among other things, I am running into bug 1005378 which prevents a displayport being set on the root scrollable frame.

Here is an initial mochitest, for bug 982141, which exercises the test framework. If this code looks like it was written by a Javascript beginner, it's because it was :) Suggestions for improvement are always welcome. The sections marked "infrastructure to get the test assertions to run at the right time", "functions that convert APZ test data into a more usable form", and "utilities for reconstructing the structure of the APZC tree" should probably be reused across all APZ tests. It's not immediately clear to me how to do that; suggestions are welcome. I'm also fine with deferring such a refactoring until additional mochitests are written. One thing I would particularly like feedback on is making sure the test doesn't depend on any timing issues and such; I wouldn't like this to be one of those tests which mysteriously fails from time to time.

Comment on attachment 8418315 [details] [diff] [review] Part 1 - Assign sequence numbers to paints on the client side and forward them to the compositor Review of attachment 8418315 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/ClientLayerManager.cpp @@ +151,5 @@ > if (aTarget && XRE_GetProcessType() == GeckoProcessType_Default) { > mShadowTarget = aTarget; > } > + > + // If this is a new paint, increment the paint sequence number. I'm not sure the comment adds information that isn't clear from the code but I don't really object. ::: gfx/layers/client/ClientLayerManager.h @@ +225,5 @@ > bool mCompositorMightResample; > bool mNeedsComposite; > > + // An incrementing sequence number for paints. > + // Incremented in BeginTransaction(), but not for begin transactions. I don't understand the second line? ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp @@ +1021,5 @@ > EXPECT_EQ(gfxPoint(15, 15), transformToApzc.Transform(gfxPoint(15, 15))); > EXPECT_EQ(gfxPoint(15, 15), transformToGecko.Transform(gfxPoint(15, 15))); > > // Now we have a sub APZC with a better fit > + ++paintSequenceNumber; Why increment these if we're not testing the value? If it's meant to be used/tested later then that's fine.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=e0188d37d06b

(In reply to Benoit Girard (:BenWa) from comment #60) > Comment on attachment 8418315 [details] [diff] [review] Thanks for the quick review! > ::: gfx/layers/client/ClientLayerManager.cpp > @@ +151,5 @@ > > if (aTarget && XRE_GetProcessType() == GeckoProcessType_Default) { > > mShadowTarget = aTarget; > > } > > + > > + // If this is a new paint, increment the paint sequence number. > > I'm not sure the comment adds information that isn't clear from the code but > I don't really object. Someone unfamiliar with the way layer transactions work reading the code may not be clear about what 'mIsRepeatTransaction' signifies. > > ::: gfx/layers/client/ClientLayerManager.h > @@ +225,5 @@ > > bool mCompositorMightResample; > > bool mNeedsComposite; > > > > + // An incrementing sequence number for paints. > > + // Incremented in BeginTransaction(), but not for begin transactions. > > I don't understand the second line? Whoops, should say "not for repeat transactions". Will fix. > ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp > @@ +1021,5 @@ > > EXPECT_EQ(gfxPoint(15, 15), transformToApzc.Transform(gfxPoint(15, 15))); > > EXPECT_EQ(gfxPoint(15, 15), transformToGecko.Transform(gfxPoint(15, 15))); > > > > // Now we have a sub APZC with a better fit > > + ++paintSequenceNumber; > > Why increment these if we're not testing the value? If it's meant to be > used/tested later then that's fine. The contract of UpdatePanZoomControllerTree() is that successive calls pass in incrementing values for the 'aPaintSequenceNumber' parameter. As a matter of principle, I like to satisfy the contract of a function when I call it, even if I know it isn't required for this particular call.

Comment on attachment 8418316 [details] [diff] [review] Part 2 - Introduce a data structure for storing data for APZ testing Review of attachment 8418316 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/testutil/APZTestData.h @@ +16,5 @@ > + > +// TODO(botond): > +// - Improve warnings > +// - Add ability to associate a repaint request triggered during a layers update > +// with the sequence number of the paint that caused the layers update For debugging purpose this is now easy to do with the profiler. If you printf_stderr the data you can easily correlate them. Even better if you add them to the layers.dump. @@ +17,5 @@ > +// TODO(botond): > +// - Improve warnings > +// - Add ability to associate a repaint request triggered during a layers update > +// with the sequence number of the paint that caused the layers update > +class APZTestData { This class needs a comment to explain it's purpose.

Comment on attachment 8418317 [details] [diff] [review] Part 3 - Introduce utilities that make logging APZ test data easier Review of attachment 8418317 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/testutil/APZTestUtil.h @@ +15,5 @@ > +namespace layers { > + > +class APZTestUtil { > +public: > + template <typename T, typename Sub, typename Point, typename SizeT, typename Margin> IMO we should come up with non APZTestUtil specific ways to print our point, rect, regions. I feel like this is a step in the wrong direction (when we go to do the right thing we will have to clean this up).

Comment on attachment 8418325 [details] [diff] [review] Part 8 - Add IPDL serialization support for base::hash_map Review of attachment 8418325 [details] [diff] [review]: ----------------------------------------------------------------- After talking to Jeff, I decided to use std::map instead of base::hash_map (apparently that's only meant to be used from within the chromium base library). Unflagging for review; the changes to the remaining patches will be trivial, as std::map and base::hash_map have identical interfaces (at least, the subset that I use).

Comment on attachment 8418330 [details] [diff] [review] Part 10 - Expose client- and compositor-side APZ test data from nsIDOMWindowUtils Review of attachment 8418330 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMWindowUtils.cpp @@ +3946,5 @@ > + if (!nsContentUtils::IsCallerChrome()) { > + return NS_ERROR_DOM_SECURITY_ERR; > + } > + > + if (nsIWidget* widget = GetWidget()) { Not sure if I feel great about defining variables in conditionals. ;-) ::: dom/interfaces/base/nsIDOMWindowUtils.idl @@ +1665,5 @@ > AString getOMTAOrComputedStyle(in nsIDOMElement aElement, > in AString aProperty); > > /** > + * Get the content- and compositor-side APZ test data instances. Please document what these jsvals are. ::: dom/webidl/APZTestData.webidl @@ +3,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this file, > + * You can obtain one at http://mozilla.org/MPL/2.0/. > + */ > + > +dictionary Entry { Please document what these dictionaries are used for. Also, remember that you need a DOM peer review on this part.

Comment on attachment 8418339 [details] [diff] [review] Part 11 - An initial mochitest (for bug 982141) that exercises the framework Review of attachment 8418339 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/test/test_bug982141.html @@ +64,5 @@ > + function convertBuckets(buckets) { > + var result = {}; > + for (var i = 0; i < buckets.length; ++i) { > + if (buckets[i].sequenceNumber in result) { > + throw("duplicate sequence number"); For mochitests, it's better form to do something more explicit here, like: ok(false, "duplicate sequence number: " + buckets[i].sequenceNumber); Including the number in the message will save you one day when this test fails only on our infra and you have to debug things without being able to reproduce the bug.

Comment on attachment 8418315 [details] [diff] [review] Part 1 - Assign sequence numbers to paints on the client side and forward them to the compositor Review of attachment 8418315 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/APZCTreeManager.h @@ +104,5 @@ > * to included a first-paint. If this is true, the part of > * the tree that is affected by the first-paint flag is > * indicated by the aFirstPaintLayersId parameter. > + * @param aPaintSequenceNumber The sequence number of the paint that triggered > + * this layer update. So each content process is going to have an independent paint sequence number, right? It should be noted here that aPaintSequenceNumber is specific to the layer subtree that's causing this update. @@ +110,5 @@ > + void UpdatePanZoomControllerTree(CompositorParent* aCompositor, > + Layer* aRoot, > + bool aIsFirstPaint, > + uint64_t aFirstPaintLayersId, > + uint32_t aPaintSequenceNumber); The other UpdatePanZoomControllerTree declaration in this file needs updating too ::: gfx/layers/ipc/PLayerTransaction.ipdl @@ +47,5 @@ > async PTexture(SurfaceDescriptor aSharedData, TextureFlags aTextureFlags); > > // The isFirstPaint flag can be used to indicate that this is the first update > // for a particular document. > + sync Update(Edit[] cset, TargetConfig targetConfig, bool isFirstPaint, nit: trailing whitespace @@ +75,5 @@ > async SetAsyncScrollOffset(PLayer layer, int32_t x, int32_t y); > > // We don't need to send a sync transaction if > // no transaction operate require a swap. > + async UpdateNoSwap(Edit[] cset, TargetConfig targetConfig, bool isFirstPaint, nit: ws ::: gfx/tests/gtest/TestAsyncPanZoomController.cpp @@ +1015,3 @@ > // Now we have a root APZC that will match the page > SetScrollableFrameMetrics(root, FrameMetrics::START_SCROLL_ID); > + manager->UpdatePanZoomControllerTree(nullptr, root, false, 0, paintSequenceNumber); It might be better to just do a post-increment on paintSequenceNumber as part of this call. Makes it less likely that somebody will copy/paste this code and forget to increment paintSequenceNumber. I don't care much either way though so if you prefer it like this that's fine too.

Comment on attachment 8418316 [details] [diff] [review] Part 2 - Introduce a data structure for storing data for APZ testing Review of attachment 8418316 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/testutil/APZTestData.h @@ +23,5 @@ > +public: > + void StartNewPaint(SequenceNumber aSequenceNumber) { > + auto insertResult = mPaints.insert(DataStore::value_type(aSequenceNumber, Bucket())); > + if (!insertResult.second) { > + NS_WARNING("Already have a paint with this sequence number"); I would change this to a MOZ_ASSERT or something. Because this code should only get run during tests, we should make it a hard fail to catch errors. @@ +60,5 @@ > + const std::string& aKey, > + const std::string& aValue) { > + auto bucketIterator = aDataStore.find(aSequenceNumber); > + if (bucketIterator == aDataStore.end()) { > + NS_WARNING("LogTestDataImpl called with nonexistent sequence number, ignoring"); Are there any cases where this is expected behaviour? If not I would rather change this to a MOZ_ASSERT as well.

Comment on attachment 8418317 [details] [diff] [review] Part 3 - Introduce utilities that make logging APZ test data easier Review of attachment 8418317 [details] [diff] [review]: ----------------------------------------------------------------- I agree with BenWa, I don't like adding this util class solely for stringifying stuff. I'd rather refactor the Log class in gfx/2d/Logging.h to allow returning a std::string as well. Willing to change this to a r+ if it's too much work to do here and you want to file a follow-up bug for it.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #68) > @@ +110,5 @@ > > + void UpdatePanZoomControllerTree(CompositorParent* aCompositor, > > + Layer* aRoot, > > + bool aIsFirstPaint, > > + uint64_t aFirstPaintLayersId, > > + uint32_t aPaintSequenceNumber); > > The other UpdatePanZoomControllerTree declaration in this file needs > updating too Oops, ignore this bit. I misread the patch.

Comment on attachment 8418321 [details] [diff] [review] Part 5 - Compositor-side instances of APZ test data and API for writing to them Review of attachment 8418321 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +127,5 @@ > + // the layers id that originated this update. > + APZTestData* testData = nullptr; > + if (CompositorParent::LayerTreeState* state = CompositorParent::GetIndirectShadowTree(aOriginatingLayersId)) { > + testData = &state->mApzTestData; > + testData->StartNewPaint(aPaintSequenceNumber); Repeat transactions will end up calling StartNewPaint with the same paint sequence number multiple times, right? Is that going to be a problem? I recall there was a warning printed when that happens, which I suggested you upgrade to an assert, but maybe it would be better to get rid of the warning entirely if this is expected and can be handled gracefully.

Comment on attachment 8418324 [details] [diff] [review] Part 7 - Log APZC tree parent links on compositor side for APZ testing Review of attachment 8418324 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/src/APZCTreeManager.cpp @@ +269,5 @@ > + // For testing, log the parent scroll id of every APZC that has a > + // parent. This allows test code to reconstruct the APZC tree. > + // Note that we currently only do this for APZCs in the layer tree > + // that originated the update, because the only identifying information > + // we are logging about APZCs in the scroll id, and otherwise we could s/in/is/ @@ +273,5 @@ > + // we are logging about APZCs in the scroll id, and otherwise we could > + // confuse APZCs from different layer trees with the same scroll id. > + if (aLayersId == aOriginatingLayersId && apzc->GetParent()) { > + aPaintLogger.LogTestData(metrics.GetScrollId(), "parentScrollId", > + apzc->GetParent()->GetGuid().mScrollId); It occurs to me that in the case where the apzc is recycled (i.e. code flow didn't take the |if (newApzc)| branch above) and also we go through the |mRootApzc = apzc| branch, then it's possible for apzc->GetParent() to be some stale pointer. I don't think this ever happens in practice but maybe we should explicitly null out the parent pointer when setting mRootApzc to apzc. Anyway, that's a pre-existing bug and unrelated to your patch. ::: gfx/layers/apz/src/AsyncPanZoomController.cpp @@ +1906,5 @@ > } > UpdateSharedCompositorFrameMetrics(); > } > > +const FrameMetrics& AsyncPanZoomController::GetFrameMetrics() const { Move this to part 0

Comment on attachment 8418326 [details] [diff] [review] Part 9 - API to request compositor-side test data from client Review of attachment 8418326 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositorParent.cpp @@ +846,5 @@ > void > +CompositorParent::GetAPZTestData(const LayerTransactionParent* aLayerTree, > + APZTestData* aOutData) > +{ > + *aOutData = sIndirectLayerTrees[mRootLayerTreeID].mApzTestData; Any reason you chose to have the out-param rather than a return value? I remember we discussed this generally before and IIRC we both preferred return values unless there's a good reason for the out-param. ::: gfx/layers/ipc/PCompositor.ipdl @@ +86,5 @@ > returns (TextureFactoryIdentifier textureFactoryIdentifier, bool success); > > // Notify the compositor that a region of the screen has been invalidated. > async NotifyRegionInvalidated(nsIntRegion region); > + spurious whitespace change ::: gfx/layers/ipc/PLayerTransaction.ipdl @@ +87,5 @@ > // Schedule a composite if one isn't already scheduled. > async ForceComposite(); > + > + // Get a copy of the compositor-side APZ test data instance for this > + // layers id. trailing ws on these three lines

Comment on attachment 8418330 [details] [diff] [review] Part 10 - Expose client- and compositor-side APZ test data from nsIDOMWindowUtils Review of attachment 8418330 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the parts I'm qualified to review (not the JS stuff) ::: gfx/layers/apz/testutil/APZTestData.cpp @@ +9,5 @@ > + > +namespace mozilla { > +namespace layers { > + > +struct APZTestDataToJSConverter { Please leave a blank line between functions here. ::: gfx/layers/client/ClientLayerManager.cpp @@ +292,5 @@ > } > } > > void > +ClientLayerManager::GetCompositorSideAPZTestData(APZTestData* aData) const { fix the trailing ws while you're here.

Comment on attachment 8418339 [details] [diff] [review] Part 11 - An initial mochitest (for bug 982141) that exercises the framework Review of attachment 8418339 [details] [diff] [review]: ----------------------------------------------------------------- I believe you need a build peer to sign off on the moz.build changes since you're adding new sections and new files. ::: gfx/layers/apz/test/test_bug982141.html @@ +123,5 @@ > + var root = makeNode(-1); > + for (var scrollId in paint) { > + if ("parentScrollId" in paint[scrollId]) { > + addLink(root, scrollId, paint[scrollId]["parentScrollId"]); > + } If "parentScrollId" doesn't exist in paint[scrollId], what does that mean? Does scrollId need to be added as a child of the root? @@ +174,5 @@ > + "expected scroll frame data for scroll id " + childScrollId); > + SimpleTest.ok("displayport" in correspondingContentPaint[childScrollId], > + "expected a displayport for scroll id " + childScrollId); > + var childDisplayport = correspondingContentPaint[childScrollId]["displayport"]; > + SimpleTest.isnot(childDisplayport, "(0,0,0,0)", "expected a nonempty displayport"); I would go one step further and extract the width and height from the displayport and verify that they are at least the size of the div (you can set a width style attribute on the div to force a specific width).

Comment on attachment 8418320 [details] [diff] [review] Part 4 - Client-side instance of APZ test data and API for writing to it Review of attachment 8418320 [details] [diff] [review]: ----------------------------------------------------------------- Minusing this one for the moment, see first comment below. ::: gfx/layers/client/ClientLayerManager.h @@ +176,5 @@ > + } > + > + // Log APZ test data for a repaint request. The sequence number must be > + // passed in from outside, and APZTestData::StartNewRepaintRequest() needs > + // to be called from the outside as well when a new repaint request is started. It's not clear to me why you want to expose these two functions to the outside world. It seems like it introduces a possible footgun, and I didn't see anything in the later patches that uses it. ::: layout/base/nsLayoutUtils.h @@ +2187,5 @@ > + const std::string& aValue); > + > + /** > + * A convenience overload of LogTestDataForPaint() that accepts any type > + * as the value, as passes it through APZTestUtil::StringifyValue() to It only accepts any type that APZTestUtil::StringifyValue accepts

The other thing that I think is missing from this patch queue is some way to disable all the logging so we don't run it in production code. I believe we have the ability to have specific prefs set while running mochitests, so I think disabling a bunch of the more expensive operations based on prefs might be wise.

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #66) > ::: dom/base/nsDOMWindowUtils.cpp > @@ +3946,5 @@ > > + if (!nsContentUtils::IsCallerChrome()) { > > + return NS_ERROR_DOM_SECURITY_ERR; > > + } > > + > > + if (nsIWidget* widget = GetWidget()) { > > Not sure if I feel great about defining variables in conditionals. ;-) We do this in many places :-) It's a nice way of de-noising null checks. http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?from=nsDisplayList.cpp#747 http://dxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?from=nsDisplayList.cpp#797 http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?from=nsLayoutUtils.cpp#2687 http://dxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?from=nsLayoutUtils.cpp#6327 http://dxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.cpp?from=AsyncPanZoomController.cpp#1048

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #74) > ::: gfx/layers/ipc/CompositorParent.cpp > @@ +846,5 @@ > > void > > +CompositorParent::GetAPZTestData(const LayerTransactionParent* aLayerTree, > > + APZTestData* aOutData) > > +{ > > + *aOutData = sIndirectLayerTrees[mRootLayerTreeID].mApzTestData; > > Any reason you chose to have the out-param rather than a return value? I > remember we discussed this generally before and IIRC we both preferred > return values unless there's a good reason for the out-param. In PLayerTransaction.ipdl, the function is declared: sync GetAPZTestData() returns (APZTestData data); The C++ signature this generates on the compositor side is: virtual bool RecvGetAPZTestData(APZTestData* aOutData); The implementation of that calls CompositorParent::GetAPZTestData. Since it needs to pass out a pointer itself, CompositorParent::GetAPZTestData might as well do so too, instead of incurring an extra move of the object.

Comment on attachment 8418320 [details] [diff] [review] Part 4 - Client-side instance of APZ test data and API for writing to it After discussion on IRC I'm changing this to a r+. I partly misread this patch, and didn't understand the repaint request half of things. It's true that these two functions aren't used but will eventually be used to track RequestContentRepaint calls to the GeckoContentController. When we have a test that exercises that we can implement that side of things. The asymmetry of the sequence number management still feels a little wrong to me but when the rest of it gets implemented it'll be easier to take a step back and see if there's a better way to do it.

Some reviewers might find it useful to look at all the changes at once, so here they are.

Comment on attachment 8418339 [details] [diff] [review] Part 11 - An initial mochitest (for bug 982141) that exercises the framework Review of attachment 8418339 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/test/test_bug982141.html @@ +49,5 @@ > + } > + return result; > + } > + > + function convertScrollFrameData(scrollFrames) { You're defining function that may be general to more APZ mochitest. I think we can introduce an apztest.js file or something similar that can host the shared test js code. This can be done when we introduce more APZ test. @@ +143,5 @@ > + var compositorTestData = utils.getCompositorAPZTestData(); > + > + // Get the sequence number of the last paint on the compositor side. > + // We do this before converting the APZ test data because the conversion > + // loses the order of the paints. Are the sequence numbers strictly increasing. If so that information isn't lost, we can look at the highest sequence number. Doing this feels not ideal. @@ +157,5 @@ > + // Reconstruct the APZC tree structure in the last paint. > + var apzcTree = buildApzcTree(compositorTestData.paints[lastCompositorPaintSeqNo]); > + > + // The apzc tree for this page should consist of a single root APZC > + // and a child APZC for the scrollable <div>. I'm not a fan of our tests making strong assumptions about the tree/APZC structures but I don't have a better suggestion. @@ +166,5 @@ > + > + // We should have content-side data for the same paint. > + SimpleTest.ok(lastCompositorPaintSeqNo in contentTestData.paints, > + "expected a content paint with sequence number" + lastCompositorPaintSeqNo); > + var correspondingContentPaint = contentTestData.paints[lastCompositorPaintSeqNo]; Very nice! The test doesn't assume that the main thread/compositor are on the same sequenceNo/transaction.

(In reply to Benoit Girard (:BenWa) from comment #83) > @@ +143,5 @@ > > + var compositorTestData = utils.getCompositorAPZTestData(); > > + > > + // Get the sequence number of the last paint on the compositor side. > > + // We do this before converting the APZ test data because the conversion > > + // loses the order of the paints. > > Are the sequence numbers strictly increasing. If so that information isn't > lost, we can look at the highest sequence number. Doing this feels not ideal. I'm not very familiar with JS, but my understanding is that to find the highest sequence number we'd need to do a max operation on the converted object's key set, which would be linear-time. Perhaps I'm mistaken?

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #76) > ::: gfx/layers/apz/test/test_bug982141.html > @@ +123,5 @@ > > + var root = makeNode(-1); > > + for (var scrollId in paint) { > > + if ("parentScrollId" in paint[scrollId]) { > > + addLink(root, scrollId, paint[scrollId]["parentScrollId"]); > > + } > > If "parentScrollId" doesn't exist in paint[scrollId], what does that mean? > Does scrollId need to be added as a child of the root? Whoops, yeah, good point! If it has a child then it will get added by virtue of appearing as the parentScrollId of the this, and this is the scenario that I had in mind for this test, but it doesn't necessarily have a child in general.

(In reply to comment #79) > > Not sure if I feel great about defining variables in conditionals. ;-) > > We do this in many places :-) Not an argument to keep doing it. (To be clear, I don't care that much, so I won't fight you over it.)

In addition to addressing review comments, I updated this patch to work around an MSVC build failure where nesting maps three levels deep caused some typenames to exceed a typename length limit. Re-requesting review to make sure my workaround is appropriate.

Re-requesting review because my new approach for stringification is different than that suggested in comment 70 (see also patches 3a and 3b).

(In reply to Botond Ballo [:botond] from comment #98) > Created attachment 8419108 [details] [diff] [review] > Part 11 - An initial mochitest (for bug 982141) that exercises the framework Here I'm re-requesting review to make sure my code for parsing the rectangle string in JS is reasonable.

I've posted updated patches, and carried r+ for most of them, except for a few where I indicated what I'd like re-reviewed, and the new ones (3a and 3b). The updated patches address all review comments unless otherwise indicated. Also, comment 78 (putting the logging behind a pref) remains to be addressed.

Had an unused variable Werror on non-debug builds, added a use of DebugOnly to work around it. (It's too bad DebugOnly doesn't mix well with 'auto'!)

(I'm trying to Try, but Try is failing me.)

(In reply to Botond Ballo [:botond] from comment #84) > > > > Are the sequence numbers strictly increasing. If so that information isn't > > lost, we can look at the highest sequence number. Doing this feels not ideal. > > I'm not very familiar with JS, but my understanding is that to find the > highest > sequence number we'd need to do a max operation on the converted object's key > set, which would be linear-time. Perhaps I'm mistaken? That's correct. Your test is already of order linear-time.

Comment on attachment 8419119 [details] [diff] [review] Part 2 - Introduce a data structure for storing data for APZ testing Review of attachment 8419119 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/testutil/APZTestData.h @@ +67,5 @@ > + // Use dummy derived structures wrapping the tyepdefs to work around a type > + // name length limit in MSVC. > + typedef std::map<std::string, std::string> ScrollFrameDataBase; > + struct ScrollFrameData : ScrollFrameDataBase {}; > + typedef std::map<ViewID, ScrollFrameData> BucketBase; Do these typedefs need to be public? Can we at least make the protected, if not private?

Comment on attachment 8419108 [details] [diff] [review] Part 11 - An initial mochitest (for bug 982141) that exercises the framework Review of attachment 8419108 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but I think you still need a build peer to rubber-stamp the moz.build changes. Tagging :ted for review for that.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #105) > Comment on attachment 8419119 [details] [diff] [review] > Part 2 - Introduce a data structure for storing data for APZ testing > > Review of attachment 8419119 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/apz/testutil/APZTestData.h > @@ +67,5 @@ > > + // Use dummy derived structures wrapping the tyepdefs to work around a type > > + // name length limit in MSVC. > > + typedef std::map<std::string, std::string> ScrollFrameDataBase; > > + struct ScrollFrameData : ScrollFrameDataBase {}; > > + typedef std::map<ViewID, ScrollFrameData> BucketBase; > > Do these typedefs need to be public? Can we at least make the protected, if > not private? Unfortunately, they need to be public. 'ScrollFrameData' et al. need to public because because, now that they are distinct types from std::map instances, we need to specialize ipc::ParamTraits for them. This is done in the Part 9 patch. The specializations look like this: template <> struct ParamTraits<mozilla::layers::APZTestData::ScrollFrameData> : ParamTraits<mozilla::layers::APZTestData::ScrollFrameDataBase> {}; Since it needs to mention 'ScrollFrameDataBase', that too needs to be public. (Making 'ParamTraits<ScrollFrameData>' a friend of APZTestData doesn't help because the privileged access only applies inside the body of the friend class, not to the base clauses.)

Comment on attachment 8419106 [details] [diff] [review] Part 10 - Expose client- and compositor-side APZ test data from nsIDOMWindowUtils Requesting DOM peer review of added WebIDL as per policy.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #73) > @@ +273,5 @@ > > + // we are logging about APZCs in the scroll id, and otherwise we could > > + // confuse APZCs from different layer trees with the same scroll id. > > + if (aLayersId == aOriginatingLayersId && apzc->GetParent()) { > > + aPaintLogger.LogTestData(metrics.GetScrollId(), "parentScrollId", > > + apzc->GetParent()->GetGuid().mScrollId); > > It occurs to me that in the case where the apzc is recycled (i.e. code flow > didn't take the |if (newApzc)| branch above) and also we go through the > |mRootApzc = apzc| branch, then it's possible for apzc->GetParent() to be > some stale pointer. I don't think this ever happens in practice but maybe we > should explicitly null out the parent pointer when setting mRootApzc to > apzc. Anyway, that's a pre-existing bug and unrelated to your patch. Filed bug 1007734 for this.

Comment on attachment 8419106 [details] [diff] [review] Part 10 - Expose client- and compositor-side APZ test data from nsIDOMWindowUtils Review of attachment 8419106 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the interface changes with the fixes below, and also with bz taking a quick look at the WebIDL, since I don't review a lot of WebIDL and want to make sure I'm not missing something. ::: dom/webidl/APZTestData.webidl @@ +5,5 @@ > + */ > + > +/* > + * This file declares data structures used to communicate data logged by > + * various components for the purpose of APZ testing (see bug 961289 and Trailing whitespace. @@ +10,5 @@ > + * gfx/layers/apz/test/APZTestData.h) to JS test code. > + */ > + > +// A single key-value pair in the data. > +dictionary Entry { there's no per-WebIDL file namespacing, so this creates a dictionary called "Entry" for the entire platform, which seems wrong. I think this should be called ScrollFrameDataEntry. @@ +24,5 @@ > +}; > + > +// All the scrollable frames associated with a given paint or repaint request. > +// The paint or repaint request is identified by a sequence number. > +dictionary Bucket { APZBucket

Comment on attachment 8419106 [details] [diff] [review] Part 10 - Expose client- and compositor-side APZ test data from nsIDOMWindowUtils The IDL looks fine, modulo bholley's comments. If you wanted to you could give the strings and numbers default values ("" and 0, presumably) and not need the extra Construct() calls on the C++ end). It's probably better to do ToJSValue(aContext, result, aOutValue) instead of manually calling ToObject(). Lastly, ToJS is ignoring the return value of ToObject. It should instead propagate that (or the return value of ToJSValue) to the caller, who needs to turn that into a failure nsresult return.

Updated Part 10 patch as per :bholley and :bz's suggestions. I chose to keep the Construct() calls. Carrying r+.

The mochitest for bug 982141 in the Part 11 patch was failing on Try even though it passed locally. The problem was that when testing locally, I was running just that mochitest alone. In this scenario, the mochitest page is the root document. When running mochitests as part of a suite, however, the mochitest page is in an iframe and is thus not the root document. The test for bug 982141 requires that the root document not be scrollable (since it tests code that gives a subframe an initial displayport if it's the primary async-scrollable frame, and a subframe is not the primary async-scrollable frame if the root frame is scrollable). When running in a iframe, however, we have no control over whether the root document is scrollable or not, and as it happens it was. To work around this, I modified the test to use window.open to open another page in a new window, and did the actual test on that page. Re-requesting review from Ehsan as this is a non-trivial change.

Comment on attachment 8420350 [details] [diff] [review] Part 11 - An initial mochitest (for bug 982141) that exercises the framework Review of attachment 8420350 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/test/mochitest.ini @@ +1,2 @@ > +[test_bug982141.html] > +skip-if = toolkit != 'gonk' Add a comment pointing to bug 991198 please. ::: gfx/layers/apz/test/test_bug982141_helper.html @@ +196,5 @@ > + Line 6<br> > + Line 7<br> > + Line 8<br> > + Line 9<br> > + Line 10<br> I'd add some more lines here, you don't want someone to change the font defaults or something and suddenly make this small enough so that it doesn't scroll! ;-)

Try pushes: https://tbpl.mozilla.org/?tree=Try&rev=e76d9238f58e (emulator, build + mochitests) https://tbpl.mozilla.org/?tree=Try&rev=323da211234c (all other platforms, build only) (I couldn't find a way to express this combination in trychooser as there is no "emulator" option under "Restrict tests to platform(s)".)

Addressed Ehsan's review comments from comment 114. Carrying r+.

Comment on attachment 8421077 [details] [diff] [review] Part 11 - An initial mochitest (for bug 982141) that exercises the framework I accidentally dropped the request for build peer review when I last updated this patch. Re-flagging.

(In reply to Botond Ballo [:botond] from comment #115) > https://tbpl.mozilla.org/?tree=Try&rev=e76d9238f58e (emulator, build + mochitests) Turns out on b2g-emulator mochitests are divided into 9 chunks for release builds and 15 for debug, and selecting mochitest-1 through mochitest-5 on trychooser only runs the first 5 out of those chunks. Pushing again with all mochitests on emulator this time: https://tbpl.mozilla.org/?tree=Try&rev=0a0bab54e87b

Comment on attachment 8421077 [details] [diff] [review] Part 11 - An initial mochitest (for bug 982141) that exercises the framework Review of attachment 8421077 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/apz/test/moz.build @@ +4,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +MOCHITEST_MANIFESTS += [ > + 'mochitest.ini', If you don't need a Makefile in this directory then I'd prefer that you just put: MOCHITEST_MANIFESTS += [ 'apz/test/mochitest.ini' ] in the two-level-up moz.build and save us having an extra moz.build.

Updated to address Ted's review comments. Carrying r+.

(In reply to Botond Ballo [:botond] from comment #118) > (In reply to Botond Ballo [:botond] from comment #115) > https://tbpl.mozilla.org/?tree=Try&rev=0a0bab54e87b Sadly, the test is still failing on TBPL with: 12:27:33 INFO - JavaScript error: http://mochi.test:8888/tests/gfx/layers/apz/test/test_bug982141_helper.html, line 14: window.opener is null 12:32:54 INFO - JavaScript error: http://mochi.test:8888/tests/SimpleTest/TestRunner.js, line 110: win.SimpleTest is undefined

Comment on attachment 8419098 [details] [diff] [review] Part 3a - Add a ToString method to MFBT Trying :froydnj for the MFBT review instead.

Comment on attachment 8419098 [details] [diff] [review] Part 3a - Add a ToString method to MFBT Review of attachment 8419098 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the small changes below. ::: mfbt/ToString.h @@ +3,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* Cross-platform lightweight thread local data wrappers. */ This is not the correct description of this header. :) @@ +15,5 @@ > +namespace mozilla { > + > +/** > + * A convenience function for converting an object to a string representation. > + * Supports any object which can be streamed to std::ostream. Nit: "to a std::ostream." @@ +17,5 @@ > +/** > + * A convenience function for converting an object to a string representation. > + * Supports any object which can be streamed to std::ostream. > + */ > +template <typename T> Nit: no space between template and <. @@ +18,5 @@ > + * A convenience function for converting an object to a string representation. > + * Supports any object which can be streamed to std::ostream. > + */ > +template <typename T> > +std::string ToString(const T& t) { Nit: opening braces go on separate lines. So does the return type.

Just realized I forgot to update this bug with the latest Part 11 patch which fixes the problem from comment 121. (In reply to Botond Ballo [:botond] from comment #121) > (In reply to Botond Ballo [:botond] from comment #118) > > https://tbpl.mozilla.org/?tree=Try&rev=0a0bab54e87b > > Sadly, the test is still failing on TBPL with: > > 12:27:33 INFO - JavaScript error: > http://mochi.test:8888/tests/gfx/layers/apz/test/test_bug982141_helper.html, > line 14: window.opener is null > 12:32:54 INFO - JavaScript error: > http://mochi.test:8888/tests/SimpleTest/TestRunner.js, line 110: > win.SimpleTest is undefined The problem here was that my helper file's name began with "test_", and apparently the test harness runs every such file as a test, even if it's not listed in mochitest.ini (I filed bug 1009229) about this. Renaming the helper to not begin with "test_" fixed the problem. Successful try push: https://tbpl.mozilla.org/?tree=Try&rev=62c3d7de79ab

Updated to address review comments.

Comment on attachment 8422554 [details] [diff] [review] Part 3a - Add a ToString method to MFBT Got r+ on IRC.

https://hg.mozilla.org/mozilla-central/rev/a5f4f4892687 https://hg.mozilla.org/mozilla-central/rev/5e07e7bcdd13 https://hg.mozilla.org/mozilla-central/rev/868026800a59 https://hg.mozilla.org/mozilla-central/rev/1418ea88f934 https://hg.mozilla.org/mozilla-central/rev/c200536ef0c0 https://hg.mozilla.org/mozilla-central/rev/f31cefaf8494 https://hg.mozilla.org/mozilla-central/rev/24d6ed0f3d7e https://hg.mozilla.org/mozilla-central/rev/0289c6b78933 https://hg.mozilla.org/mozilla-central/rev/d24910545fbe https://hg.mozilla.org/mozilla-central/rev/043b5bcd7bc1 https://hg.mozilla.org/mozilla-central/rev/e6eef0510bb1 https://hg.mozilla.org/mozilla-central/rev/1779847c002c