Botond Ballo [:botond] Assignee
	Description • 11 years ago

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

	Rodrigo Rodriguez
	Comment 1 • 11 years ago

I would like to get in on this.

	Botond Ballo [:botond] Assignee
	Comment 2 • 11 years ago

(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).

	Rodrigo Rodriguez
	Comment 3 • 11 years ago

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.

	Rodrigo Rodriguez
	Comment 4 • 11 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 5 • 11 years ago

(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

	Rodrigo Rodriguez
	Comment 6 • 11 years ago

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.

	Rodrigo Rodriguez
	Comment 7 • 11 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 8 • 10 years ago

(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.)

	Botond Ballo [:botond] Assignee
	Comment 9 • 10 years ago

(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!

	Rodrigo Rodriguez
	Comment 10 • 10 years ago

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!

	Rodrigo Rodriguez
	Comment 11 • 10 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 10 years ago

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.

	Rodrigo Rodriguez
	Comment 13 • 10 years ago

(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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 14 • 10 years ago

(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

	Botond Ballo [:botond] Assignee
	Comment 15 • 10 years ago

(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

	Rodrigo Rodriguez
	Comment 16 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 17 • 10 years ago

(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.

	Rodrigo Rodriguez
	Comment 18 • 10 years ago

(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 );

	Botond Ballo [:botond] Assignee
	Comment 19 • 10 years ago

(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

	Rodrigo Rodriguez
	Comment 21 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 22 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 23 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 24 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 25 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 26 • 10 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 27 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 28 • 10 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 29 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 32 • 10 years ago

Posting updated patches. Still very much a WIP!

	Botond Ballo [:botond] Assignee
	Comment 40 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 41 • 10 years ago

Ended up using PLayerTransaction rather than PCompositor for this.

	Botond Ballo [:botond] Assignee
	Comment 42 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 45 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 46 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 47 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 59 • 10 years ago

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.

	Benoit Girard (:BenWa)
	Comment 60 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 61 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 62 • 10 years ago

(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.

	Benoit Girard (:BenWa)
	Comment 63 • 10 years ago

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.

	Benoit Girard (:BenWa)
	Comment 64 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 65 • 10 years ago

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

	(no longer active)
	Comment 66 • 10 years ago

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.

	(no longer active)
	Comment 67 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 68 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 69 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 70 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 71 • 10 years ago

(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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 72 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 73 • 10 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 74 • 10 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 75 • 10 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 76 • 10 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 77 • 10 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 78 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 79 • 10 years ago

(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

	Botond Ballo [:botond] Assignee
	Comment 80 • 10 years ago

(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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 81 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 82 • 10 years ago

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

	Benoit Girard (:BenWa)
	Comment 83 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 84 • 10 years ago

(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?

	Botond Ballo [:botond] Assignee
	Comment 85 • 10 years ago

(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.

	(no longer active)
	Comment 86 • 10 years ago

(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.)

	Botond Ballo [:botond] Assignee
	Comment 89 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 92 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 99 • 10 years ago

(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.

	Botond Ballo [:botond] Assignee
	Comment 100 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 101 • 10 years ago

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'!)

	Botond Ballo [:botond] Assignee
	Comment 102 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 103 • 10 years ago
try

https://tbpl.mozilla.org/?tree=Try&rev=64047c059c18

	Benoit Girard (:BenWa)
	Comment 104 • 10 years ago

(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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 105 • 10 years ago

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?

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 106 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 107 • 10 years ago

(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.)

	Botond Ballo [:botond] Assignee
	Comment 108 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 109 • 10 years ago

(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.

	Bobby Holley (:bholley)
	Comment 110 • 10 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 111 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 112 • 10 years ago

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

Carrying r+.

	Botond Ballo [:botond] Assignee
	Comment 113 • 10 years ago

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.

	(no longer active)
	Comment 114 • 10 years ago

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!  ;-)

	Botond Ballo [:botond] Assignee
	Comment 115 • 10 years ago
try

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)".)

	Botond Ballo [:botond] Assignee
	Comment 116 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 117 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 118 • 10 years ago

(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

	(not currently active) Ted Mielczarek
	Comment 119 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 120 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 121 • 10 years ago

(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

	Botond Ballo [:botond] Assignee
	Comment 122 • 10 years ago

Comment on attachment 8419098 [details] [diff] [review]
Part 3a - Add a ToString method to MFBT

Trying :froydnj for the MFBT review instead.

	Nathan Froyd [:froydnj]
	Comment 123 • 10 years ago

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.

	Botond Ballo [:botond] Assignee
	Comment 124 • 10 years ago

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

	Botond Ballo [:botond] Assignee
	Comment 125 • 10 years ago

Updated to address review comments.

	Botond Ballo [:botond] Assignee
	Comment 126 • 10 years ago

Comment on attachment 8422554 [details] [diff] [review]
Part 3a - Add a ToString method to MFBT

Got r+ on IRC.

	Botond Ballo [:botond] Assignee
	Comment 127 • 10 years ago
landing

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=1779847c002c

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 128 • 10 years ago

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