Patrick Brosset <:pbro> Assignee
	Description • 10 years ago

It would be useful to have a standalone reflow actor on the devtools server.

I see at least 2 use cases:
- the webconsole css reflow logs (already uses today a reflow observer part of the webconsole actor)
- the inspector layout-view which today refreshes on mozAfterPaint (like, all the time!).

There could be more use cases later.

	Patrick Brosset <:pbro> Assignee
	Comment 1 • 10 years ago

Depends on bug 997092 which would greatly simplify the reflow<->domnode link.

	Patrick Brosset <:pbro> Assignee
	Comment 2 • 10 years ago

Not yet working, but getting there, slowly.
I'm not yet entirely sure if my plan to somehow couple dom and style changes with reflows is going to turn out ok, but it's worth investigating a little bit more.

	Patrick Brosset <:pbro> Assignee
	Comment 3 • 10 years ago

Ok, here is v1.

This patch contains a new tab-level reflow actor, which sends batched events about reflows to the layout-view (for now), which has been changed to stop using MozAfterPaint events.

The new file that contains the reflow actor contains more than just this. It has 4 listeners: reflows, stylesheet changes, stylerule changes and markup mutations.
For now, the reflowActor only cares about reflows, and I think we should keep it this way. however, the observer that starts the 4 listeners listed before can be required by other actors to do more things. 
Examples include:
- the inspector actor could use the markupmutations listener to avoid starting its own listener
- the stylesheets actor could use the stylesheet changes listener to refresh the list of stylesheets in the UI
- the styles actor could use the stylesheet and stylerule changes listeners to refresh the applied styles in the UI

Also in this patch, but not working yet, I'm making some attempts at linking markup mutations + style changes with reflows.
Reflows are caused by these events, and it seems doable to find out what dom or style change caused a reflow. And based on the changes that occur, it seems feasible to get a list of DOM nodes that were therefore reflowed.
For info, this is what bug 997092 is supposed to do, but in the meantime, I want to try and make this work in order to be able to detect which nodes have been reflowed and therefore check dynamically which nodes may have had their visibility change (for bug 911209).

	Patrick Brosset <:pbro> Assignee
	Comment 4 • 10 years ago

v2
Works better :)
The layout-view refreshes with when reflow events are received, unless the view is hidden.

Also, if the debuggee is older than the client and doesn't have the reflow actor running, the layout-view will only refresh when the panel is switched to.
I think the MozAfterPaint event we were using before only worked on local debugging anyway, so it's not a regression.

	Patrick Brosset <:pbro> Assignee
	Comment 6 • 10 years ago

Fixed the failing layout-view tests (also re-enabled them).

Paul, regarding our discussion in bug 911209, do you mind giving me feedback on this new actor?

The first block comment in layout.js explains in details what each class does and is important to understand the approach I've taken.

Worth noting too:
- the reflow listener is only added when the first consumer requests a LayoutChangesObserver instance, so in the case of this patch, it's only the first time the layout-view is displayed
- there's only one listener registered per tab
- since there may be several consumers of the LayoutChangesObserver events, the reflow listener is never stopped, until we destroy all the objects on toolbox shutdown.

	Paul Rouget [:paul]
	Comment 7 • 10 years ago

Comment on attachment 8411823 [details] [diff] [review]
bug997198-reflow-actor v3.patch

This is beautiful (and very much needed).

(The check to "moz-styleeditor-transitioning" is unfortunate)

	Patrick Brosset <:pbro> Assignee
	Comment 9 • 10 years ago

So here are the latest patch versions.
The part 2 is a quick cleanup of the layout-view tests.

Here's a try push for both patches: https://tbpl.mozilla.org/?tree=Try&rev=c1c2e8198f64

	Patrick Brosset <:pbro> Assignee
	Comment 10 • 10 years ago

Try build shows that first, the layoutview tests fail intermittently, which was to be expected knowing how much the view refresh mechanism has changed, and second, many tests leak in debug. This is probably due to the ReflowActor not releasing tabActor references. I'll get to that now.

	Patrick Brosset <:pbro> Assignee
	Comment 11 • 10 years ago

Found and fixed the leak.
The reflow actor is the first in its hierarchy to use protocol.js, it doesn't have a parent actor that takes care of its lifetime.
I found that its destroy method wasn't being executed, therefore causing the leak.

After a bit of tracing, I found that the actor was being registered in ActorPool (in /toolkit/devtools/server/actors/common.js) and, once there, the only way to make sure it gets destroyed on shutdown is to define a `disconnect` method so that it ends up in the ActorPool's `_cleanups` array.

I'm still unsure how this is done and why the InspectorActor, which appears to be similar to the ReflowActor, has its destroy method executed on shutdown.

What I saw was that a lot of other actors seem to be behaving in the same way as the ReflowActor. For instance Canvas or WebGL which both have a `finalize` method that is called by the client-side on toolbox's closing.

	Patrick Brosset <:pbro> Assignee
	Comment 12 • 10 years ago

Refactored test head.js a little bit.

	Patrick Brosset <:pbro> Assignee
	Comment 13 • 10 years ago

Fixed an error thrown during toolbox closing.

	Patrick Brosset <:pbro> Assignee
	Comment 14 • 10 years ago

Added some missing descriptions to tests

	Patrick Brosset <:pbro> Assignee
	Comment 15 • 10 years ago

New try push for the last 2 patches: https://tbpl.mozilla.org/?tree=Try&rev=3adc3cb08d0e

	Patrick Brosset <:pbro> Assignee
	Comment 16 • 10 years ago

Fixed a bug when getting the same LayoutChangesObserver instance the second time (in getObserver).

	Brian Grinstead [:bgrins]
	Comment 17 • 10 years ago

Comment on attachment 8414432 [details] [diff] [review]
bug997198-reflow-actor v7.patch

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

The implementation looks good overall. It seems that the only thing that the reflow actor cares from the LayoutChangesObserver are reflow events.  I believe that a lot of these observers are really noisy, and we won't want them running if nothing is listening to them.  Of course, other actors will want to use the currently unused functionality eventually.  This makes me wonder if we should either:

a) Have an option for the LayoutChangesObserver to ignore certain types of events.  If two LayoutChangesObservers were requested on the same window but with different enabled options, then we would have to manage that by enabling new observers on the single instance.
b) Only start up the xObserver once something is listening to event "x".  This would be more transparent because the observers would get created on the fly once someone starts listening to it, but would probably require overriding at least this.on, then turning on the corresponding observer based on the even name being bound.

::: browser/devtools/layoutview/view.js
@@ +237,5 @@
> +   */
> +  trackReflows: function() {
> +    if (!this.reflowFront) {
> +      let toolbox = this.inspector.toolbox;
> +      if (toolbox.target.form.reflowActor) {

So if this is used with an old version of the server, then reflows / MozAfterPaint will not be detected while a node is not selected?  That seems like a reasonable failure case, esp since update is still called on onNewNode

@@ +329,5 @@
>      this.sizeLabel = null;
>      this.inspector = null;
>      this.doc = null;
> +
> +    if (reflowFront) {

Needs to be `this.reflowFront`

@@ +375,5 @@
>      this.dimmed = true;
>    },
>  
>    /**
>     * Show the layout boxes. A node is selected.

Can you add comments in undim/dim header about the layout view being active/inactive in addition to a node being selected?

@@ +486,5 @@
>      toolbox.highlighterUtils.highlightNodeFront(nodeFront, options);
>    },
>  
> +  /**
> +   * Hide the box-model highlihgter on the currently selected element

/s/highlihgter/highlighter

::: toolkit/devtools/server/actors/layout.js
@@ +57,5 @@
> +
> +/**
> + * The reflow actor tracks reflows and emits events about them.
> + */
> +let ReflowActor = exports.ReflowActor = protocol.ActorClass({

Should this be exported?  Seems that it is will only be used within this file

@@ +63,5 @@
> +
> +  events: {
> +    /**
> +     * The reflows event is emitted when reflows have been detected. The event
> +     * is sent with an array of reflows that occured. Eeach item has the

/s/Eeach/Each

@@ +221,5 @@
> + * The LayoutChangesObserver class is instantiated only once per given tab
> + * and is used to track reflows and dom and style changes in that tab.
> + * The LayoutActor uses this class to send reflow events to its clients.
> + *
> + * DO NOT INSTANTIATE THIS CLASS, instead use

Rather than have a warning in the header comment not to instantiate, maybe we could keep this object non-exported and attach the getObserver() / releaseObserver() to a separate object that is exported.  This could prevent confusion when using it with other actors.

A couple of ideas:

exports.LayoutChangesObservers = { getObserver: function() {...}, releaseObserver: function() {...}}

Or 

exports.getLayoutChangesObserver = function() { ... }; exports.releaseLayoutChangesObserver = function() { ... }

@@ +277,5 @@
> +
> +  let obs = new LayoutChangesObserver(tabActor);
> +  observedWindows.set(tabActor, {
> +    observer: obs,
> +    refCounting: 1 // counting references allows to stop the observer when no

Looking at how refCounting is used, I'm left thinking that it could be removed without the functionality changing at all.  When it is the observer is instantiated, it is initialized to 1.  When the observer is removed, it is decremented (to 0), then checked to see if it is falsy.  It always will be.  So it seems like you could just do `observedWindows.set(tabActor, obs)` and make the corresponding change in releaseObserver

@@ +348,5 @@
> +   * Start the event loop, which regularly checks if there are reflows and other
> +   * style and markup changes to be sent as batched events.
> +   * Calls itself in a loop.
> +   */
> +  _eventLoop: function() {

Name this _startEventLoop to match _stopEventLoop

@@ +493,5 @@
> +  }
> +});
> +
> +/**
> + * Instantiate and start a stylesheetchanges observer on a given window's.

Sentence is cut off: "on a given window's"

@@ +508,5 @@
> +}
> +
> +StyleSheetChangesObserver.prototype = Heritage.extend(Observable.prototype, {
> +  _start: function() {
> +    this.win.document.styleSheetChangeEventsEnabled = true;

I added a log statement here for this.win.location, because I was wondering about this styleSheetChangeEventsEnabled bit here.

If I open layoutview on http://firstpage, then I see {Location → http:firstpage}.  Then if I navigate to http://secondpage, I don't see this same log.  Does this mean that the styleSheetChangeEventsEnabled bit is not being set on the second window?

@@ +534,5 @@
> +  }
> +});
> +
> +/**
> + * Instantiate and start a stylerulechanges observer on a given window's.

Sentence is cut off: "on a given window's"

@@ +543,5 @@
> + */
> +function StyleRuleChangesObserver(tabActor, callback) {
> +  Observable.call(this, tabActor, callback);
> +  this._onStyleRuleChange = this._onStyleRuleChange.bind(this);
> +  this.handler = tabActor.isRootActor ?

Can you explain the significance of this.handler here?  When will this be called with a rootActor vs not?  Will this somehow change the functionality of this observer?

	Brian Grinstead [:bgrins]
	Comment 18 • 10 years ago

Something weird I've found playing with it (even after fixing the this.reflowFront in view.destroy):

1) Open http://www.mozilla.org/en-US/
2) Open devtools and layout view
3) Refresh Page
4) Close tab

Throws this exception:
Handler function BrowserTabList.prototype.onCloseWindow's delayed body threw an exception: TypeError: can't access dead object
Stack: MarkupMutationObserver.prototype<._stop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:436:5
Observable.prototype.stop@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:193:7
Observable.prototype.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:213:5
MarkupMutationObserver.prototype<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:451:5
LayoutChangesObserver.prototype<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:309:5
LayoutChangesObserver.releaseObserver@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:302:5
exports.ReflowActor<.destroy@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:99:5
exports.ReflowActor<.disconnect@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/layout.js:94:5
AP_cleanup@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/common.js:161:7
DSC_removeActorPool/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:914:32
DSC_removeActorPool@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:914:9
BTA_detach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:749:7
BTA_exit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:665:1
BrowserTabActor.prototype.exit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:1139:3
BrowserTabList.prototype._handleActorClose@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:324:3
BrowserTabList.prototype.onCloseWindow</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:485:9
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:84:7
Line: 436, column: 4 DevToolsUtils.js:60
error occurred while processing 'detach: TypeError: this.threadActor is null
Stack: BTA_popContext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:722:5
BTA_detach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:743:5
BTA_onDetach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:776:9
DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1099:9
LDT_send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/transport.js:279:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:84:7
Line: 722, column: 4 main.js:975
onPacket threw an exception: Error: Server did not specify an actor, dropping packet: {"error":"unknownError","message":"error occurred while processing 'detach: TypeError: this.threadActor is null\nStack: BTA_popContext@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:722:5\nBTA_detach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:743:5\nBTA_onDetach@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webbrowser.js:776:9\nDSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/main.js:1099:9\nLDT_send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/transport.js:279:11\nmakeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:84:7\nLine: 722, column: 4"}
Stack: DebuggerClient.prototype.onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:685:1
resolve@resource://gre/modules/devtools/deprecated-sync-thenables.js:40:11
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:735:1
LDT_send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/transport.js:279:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:84:7
Line: 685, column: 0

	Patrick Brosset <:pbro> Assignee
	Comment 19 • 10 years ago

Thanks for the review.
See my comments inline below.

(In reply to Brian Grinstead [:bgrins] from comment #17)
> Comment on attachment 8414432 [details] [diff] [review]
> bug997198-reflow-actor v7.patch
> 
> Review of attachment 8414432 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The implementation looks good overall. It seems that the only thing that the
> reflow actor cares from the LayoutChangesObserver are reflow events.  I
> believe that a lot of these observers are really noisy, and we won't want
> them running if nothing is listening to them.  Of course, other actors will
> want to use the currently unused functionality eventually.  This makes me
> wonder if we should either:
> 
> a) Have an option for the LayoutChangesObserver to ignore certain types of
> events.  If two LayoutChangesObservers were requested on the same window but
> with different enabled options, then we would have to manage that by
> enabling new observers on the single instance.
> b) Only start up the xObserver once something is listening to event "x". 
> This would be more transparent because the observers would get created on
> the fly once someone starts listening to it, but would probably require
> overriding at least this.on, then turning on the corresponding observer
> based on the even name being bound.
I originally added the stylerule/sheet and markupmutation observers because I wanted to find a way to link a reflow with dom nodes so that I would know which dom nodes had been reflowed, and therefore would be able to only refresh the layout-view when needed (and also update nodes' visibility without having to loop over all known nodes: see bug 911209).

It turns out this isn't really possible nor really useful in the end.

It's pretty much impossible especially when stylesheet events are observed, cause if you add or remove a stylesheet, you'd have to basically loop over all selectors in these stylesheets, querySelectorAll the corresponding nodes and then check if the rules associated with these selectors can actually cause reflows.

It isn't really useful either because by batching reflow events before sending them to the UI, we avoid performance problems. Also, I filed bug 997092 to get the list of DOM nodes impacted by a reflow.

So overall, I think I should just get rid of these observers and be done with it. I've been pondering this for a while but kept them until now because I wasn't sure.

2 pretty quick benefits those would have though is: being able to refresh the rule-view and style-editor when stylesheets are added/removed (see bug 922146), and refactoring the inspector actor to use the MarkupMuationObserver.
My original idea, when naming this new file "layout.js" was to create an entity on the server-side that would be responsible for all things layout-related. So anything that, in any way, impacts the layout and causes reflows.

But I think the best solution is, as I said, to get rid of them for now because they aren't used immediately, so better not maintain code that isn't used.
Also, if we need them later, they're here in bugzilla's history. Also, the LayoutChangesObserver is done in a way that new observers are easy to add anyway, so it's not like it'll be harder to do later.

> ::: browser/devtools/layoutview/view.js
> @@ +237,5 @@
> > +   */
> > +  trackReflows: function() {
> > +    if (!this.reflowFront) {
> > +      let toolbox = this.inspector.toolbox;
> > +      if (toolbox.target.form.reflowActor) {
> 
> So if this is used with an old version of the server, then reflows /
> MozAfterPaint will not be detected while a node is not selected?  That seems
> like a reasonable failure case, esp since update is still called on onNewNode
Keep in mind that so far, the layout-view only updates when debugging locally. When remote debugging, the layout-view only updates when switching nodes or selecting another sidebar tab and coming back again to the layout-view.
That's because we use the MozAfterPaint event at browser level, from the client-side.
So my patch doesn't change anything when it comes to remote debugging older targets. It makes it better when remote debugging same code-level targets. And it makes it the same when locally debugging (because when debugging a local tab, the server code-level is of course the same as the client code-level).

> ::: toolkit/devtools/server/actors/layout.js
> @@ +57,5 @@
> > +
> > +/**
> > + * The reflow actor tracks reflows and emits events about them.
> > + */
> > +let ReflowActor = exports.ReflowActor = protocol.ActorClass({
> 
> Should this be exported?  Seems that it is will only be used within this file
You're right. I seem to remember I needed to use it from outside the module at some stage, but that's not the case now. Done.

> @@ +221,5 @@
> > + * The LayoutChangesObserver class is instantiated only once per given tab
> > + * and is used to track reflows and dom and style changes in that tab.
> > + * The LayoutActor uses this class to send reflow events to its clients.
> > + *
> > + * DO NOT INSTANTIATE THIS CLASS, instead use
> 
> Rather than have a warning in the header comment not to instantiate, maybe
> we could keep this object non-exported and attach the getObserver() /
> releaseObserver() to a separate object that is exported.  This could prevent
> confusion when using it with other actors.
> 
> A couple of ideas:
> 
> exports.LayoutChangesObservers = { getObserver: function() {...},
> releaseObserver: function() {...}}
> 
> Or 
> 
> exports.getLayoutChangesObserver = function() { ... };
> exports.releaseLayoutChangesObserver = function() { ... }
Much better indeed, thanks.
Done.

> @@ +277,5 @@
> > +
> > +  let obs = new LayoutChangesObserver(tabActor);
> > +  observedWindows.set(tabActor, {
> > +    observer: obs,
> > +    refCounting: 1 // counting references allows to stop the observer when no
> 
> Looking at how refCounting is used, I'm left thinking that it could be
> removed without the functionality changing at all.  When it is the observer
> is instantiated, it is initialized to 1.  When the observer is removed, it
> is decremented (to 0), then checked to see if it is falsy.  It always will
> be.  So it seems like you could just do `observedWindows.set(tabActor, obs)`
> and make the corresponding change in releaseObserver
Oh, good catch. If that code seemed confusing it's because I completely forgot to increment the refCounting in getObserver.
So, with this fixed, now everytime we get an observer for a tabActor we already have an observer for, we return it and increment its refCounting (which is now 2).
So when the consumer releases this observer, the refCounting goes back to 1, and therefore isn't destroyed.

With this mechanism, we're sure to have only one reflowobserver (or markup observer, stylesheet observer ... when we'll have them) per tab and one listener (the LayoutChangesObserver).

I've fixed this now.

> @@ +508,5 @@
> > +}
> > +
> > +StyleSheetChangesObserver.prototype = Heritage.extend(Observable.prototype, {
> > +  _start: function() {
> > +    this.win.document.styleSheetChangeEventsEnabled = true;
> 
> I added a log statement here for this.win.location, because I was wondering
> about this styleSheetChangeEventsEnabled bit here.
> 
> If I open layoutview on http://firstpage, then I see {Location →
> http:firstpage}.  Then if I navigate to http://secondpage, I don't see this
> same log.  Does this mean that the styleSheetChangeEventsEnabled bit is not
> being set on the second window?
Good catch. I doesn't matters as much now that I removed everything not reflow-related. Nevertheless I think you're right, this flag is per document and I don't think it persists through page navigations (for info, this was added in bug 839103).
That's why it's better to get rid of this code now and really test it when we'll need it. No use having some broken code laying around.

> 
> @@ +543,5 @@
> > + */
> > +function StyleRuleChangesObserver(tabActor, callback) {
> > +  Observable.call(this, tabActor, callback);
> > +  this._onStyleRuleChange = this._onStyleRuleChange.bind(this);
> > +  this.handler = tabActor.isRootActor ?
> 
> Can you explain the significance of this.handler here?  When will this be
> called with a rootActor vs not?  Will this somehow change the functionality
> of this observer?
I've removed this part here too, but here's the explanation:
tabActor is passed to all tab-level actors' init method, but it's implementation differs depending on the environment:

- On a firefox desktop content page: tabActor is a BrowserTabActor (extends TabActor) from which the chromeEventHandler property can use to listen to events on, even in nested iframes.
- On B2G: tabActor is a ContentActor but it overrides TabActor, and has a docshell too, so its chromeEventHandler property can also be used for the same purpose.
- When using the Browser Toolbox (to inspect firefox desktop): tabActor is the RootActor instead, in which case, the window property (chrome window) can be used to listen to events.

	Patrick Brosset <:pbro> Assignee
	Comment 20 • 10 years ago

(In reply to Brian Grinstead [:bgrins] from comment #18)
> Something weird I've found playing with it (even after fixing the
> this.reflowFront in view.destroy):
> 
> 1) Open http://www.mozilla.org/en-US/
> 2) Open devtools and layout view
> 3) Refresh Page
> 4) Close tab
> 
> Throws this exception: ...
This part of the code is now gone. A bug must have made its way in the markupMutationObserver and since I haven't tested this much, I never saw it.

	Patrick Brosset <:pbro> Assignee
	Comment 21 • 10 years ago

Addressed Brian's comments.

What's left to do is:
- fix the intermittently failing tests shown in https://tbpl.mozilla.org/?tree=Try&rev=3adc3cb08d0e
- test on B2G (both with an older and similar target code-level)

A couple of interesting nodes:
- I have tested a few CSS animations (animating the padding for instance) and they don't seem to trigger reflows. Or the reflows that are triggered aren't sent to the listener we use on the docshell. This can probably be discussed/fixed in a follow-up bug.
- Pref "layout.interruptible-reflow.enabled" was recently added and with it, we can disable interruptible reflows. Maybe that's a service this actor could provide at some stage.

	Patrick Brosset <:pbro> Assignee
	Comment 22 • 10 years ago

Fixed the intermittent test failures (the test I modified weren't waiting for node style changes to be reflected in the layout-view).
See this new try build: https://tbpl.mozilla.org/?tree=Try&rev=02111a146561

	Brian Grinstead [:bgrins]
	Comment 23 • 10 years ago

Comment on attachment 8414752 [details] [diff] [review]
bug997198-reflow-actor v8.patch

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

The only remaining thing here that I'm thinking is that we should have test(s) for the LayoutChangesObserver/ReflowActor inside of toolkit/devtools/server/tests.  I'm not sure which type of test would make the most sense here, but I think it makes sense to have a test directly for this instead of just for the layoutview.

::: browser/devtools/layoutview/test/browser.ini
@@ -4,5 @@
>  support-files =
>    head.js
>  
>  [browser_layoutview.js]
> -skip-if = true

I'm thinking we should reenable the tests in part 2, where the intermittents are fixed.

	Brian Grinstead [:bgrins]
	Comment 24 • 10 years ago

Comment on attachment 8415552 [details] [diff] [review]
bug997198 (part 2)-reflow-actor-cleanup-layout-tests v4.patch

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

Test changes look good

	Patrick Brosset <:pbro> Assignee
	Comment 26 • 10 years ago

Thanks Brian for the review.
Just re-uploading a new version of the part2 patch, with the removal of the skip-if line in there.

	Patrick Brosset <:pbro> Assignee
	Comment 27 • 10 years ago

Added the skip-if line back in browser.ini.
I'm going to work on adding actor tests now.

	Patrick Brosset <:pbro> Assignee
	Comment 28 • 10 years ago

v10.

In this version I added a unit test (xpcshell) for the LayoutChangesObserver.
The test mocks the TabActor and fakes reflow events in order to unit test the class' API and behavior.
The actual reflow logic is tested with browser mochitests in browser/devtools/layoutview.

I also tested this version on:
- Fennec
- B2G with and without the reflow actor
- Remote desktop debugging with and without the reflow actor
- Browser toolbox.

All these cases now work with v10.

When debugging a remote target that doesn't have the reflow actor (older B2G, Fennec, remote debugging Aurora), then the layout-view behaves exactly as before: it only updates when you re-select the node or switch to the layout-view panel. This was the case before my patch because we were using mozAfterPaint events that only worked in the local browser.
With my patch, this behavior remains because there is no reflow actor on the server that sends the events.

When debugging a remote target that has the reflow actor (patched B2G, patched remote firefox), then the actor sends events over the wire and the layout-view updates.
For B2G, I had to change the way I was importing EventEmitter (was XPCOMUtils.import, changed to require) as this was preventing the actor from loading correctly.

In the case of the browser toolbox, I had to do register the actor as a globalActor on top of a tabActor, otherwise, it just isn't being registered.
I found that most (if not all) actors actually do this, probably for the same reason.

	Patrick Brosset <:pbro> Assignee
	Comment 29 • 10 years ago

Try build: https://tbpl.mozilla.org/?tree=Try&rev=c91dfeefa537

	Patrick Brosset <:pbro> Assignee
	Comment 30 • 10 years ago

Filed follow-up bug 1007021 to deal with nested frames as the reflow actor doesn't observe reflows in them so far.
This is done as a follow-up bug rather than directly in this bug as it requires going through the docshell tree and listening for frames load and unload, some of which is being cleaned up in bug 977043

	Patrick Brosset <:pbro> Assignee
	Comment 31 • 10 years ago

I don't know how I missed this until now.
There was a mistake in view.js/destroy:

    if (reflowFront) {
      ...
    }

instead of

    if (this.reflowFront) {
      ...
    }

	Brian Grinstead [:bgrins]
	Comment 32 • 10 years ago

Comment on attachment 8418753 [details] [diff] [review]
bug997198-reflow-actor v11.patch

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

Looks good.  r+ with suggested test additions

::: toolkit/devtools/server/tests/unit/test_layout-reflows-observer.js
@@ +83,5 @@
> +  releaseLayoutChangesObserver(tabActor2);
> +  releaseLayoutChangesObserver(tabActor2);
> +}
> +
> +function eventsAreBatched() {

I'd like to see some checks somewhere to handles cases with reflows and stopped observers:

1) Make sure that reflows that happen while an observer is stopped do not get queued up in the same way that they do when it is running.
2) Make sure that if reflows are queued on a running observer and the observer is stopped and timer executed, then there are 0 reflows.

	Patrick Brosset <:pbro> Assignee
	Comment 33 • 10 years ago

With a couple more test cases in the xpcshell unit test of the LayoutChangesObserver.
And a new try build for good measure: https://tbpl.mozilla.org/?tree=Try&rev=5523deffd62a

	Brian Grinstead [:bgrins]
	Comment 34 • 10 years ago

Comment on attachment 8420959 [details] [diff] [review]
bug997198-reflow-actor v12.patch

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

Changes look good

	Patrick Brosset <:pbro> Assignee
	Comment 35 • 10 years ago

Fixed in fx-team:
 https://hg.mozilla.org/integration/fx-team/rev/8a9c9878d2ce
 https://hg.mozilla.org/integration/fx-team/rev/99ea54c33bcf

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/8a9c9878d2ce
https://hg.mozilla.org/mozilla-central/rev/99ea54c33bcf