878935 - Reflow-on-zoom should blur text before it repaints for the last time to indicate to the user it's not complete

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Readability, defect)

Description

[Scott Johnson (:jwir3)]

Opera mobile blurs text until it paints after a reflow-on-zoom event has happened, indicating to the user that the reflow event has not happened yet. This makes it perceptually more clear to the user that their reflow request has not finished. We have decided to try this approach, to see if it makes our reflow-on-zoom implementation a bit less perceptually disconcerting when it snaps to a new location.

NOTE: Opera mobile seems to perform a paint right before doing the reflow, which seems like a waste. We should try to avoid doing this.

Comment 1

[Scott Johnson (:jwir3)]

Some notes from conversations I've had on this issue:

Chris Lord indicated to me that the most efficient way of blurring would be to render to a frame-buffer-object, then render with a box-blur shader.

Code that hooks into this would likely be:
Use GLContextProvider::CreateOffscreen() to create a new context, then bind an FBO object using gl->fBindFramebuffer(LOCAL_GL_FRAMEBUFFER, gl->GetOffscreenFBO()); Render the layer tree to the FBO object. Add a filter texture. Render the FBO to the screen.

The only issue is where in the graphics pipeline we should put this.

Blassey, on the other hand, suggested delaying painting until after the reflow-on-zoom event has happened. We already blur when we zoom in, so we could just force painting to not happen until after the reflow has completed, at which time we'd turn painting back on.

Comment 2

[Scott Johnson (:jwir3)]

I've been researching what we might be able to do to solve this issue.

If we were to go about this by holding off on painting until we're done with reflow-on-zoom, then I was thinking that I might be able to implement it as follows:

1) Add a new event type, "Browser:ReflowZoomToRect" to JavaPanZoomController.java and the browser.js code that calls nsDocumentViewer::ChangeMaxLineBoxWidth.
2) Add a new method to GLController.java called "pauseCompositor", that essentially does the opposite of GLController::resumeCompositor. This would need to be synchronous, just like resumeCompositor is. It would call another added method, GeckoAppShell::schedulePauseComposition(), and then call GeckoAppShell::sendEventToGecko(GeckoEvent.createCompositorPauseEvent()). Ideally, this would pause the compositor on the Java side after the composition of the zoom animation (blurring) was done, but before we rendered the final version of the zoom (high fidelity, non-blurred).
4) Inside of nsDocumentViewer, we add a callback handler for reflow, so that when reflow is finished, the pres shell notifies the document viewer for which the reflow was triggered. We're probably going to need to keep track of each individual reflow-on-zoom sub-event that we trigger, since an nsDocumentViewer can trigger reflows for all its children. We'd then notify the app shell that it's ok to resume composition.

I'm not sure about the last bit - whether we're going to be able to communicate from the document viewer to the app shell in a reasonable way. I've never worked with the nsAppShell class before, so I'm not sure whether it's difficult to acquire a reference to it. 

I think that if we were to choose the other route - i.e. use a more active "blurring" shader - then we'd almost surely have to connect it in a similar way. 

Chris, what do you think about this? Does it seem reasonable from your point of view, or am I making this more complicated than it should be?

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Scott Johnson (:jwir3) from comment #2)
> I've been researching what we might be able to do to solve this issue.
> 
> If we were to go about this by holding off on painting until we're done with
> reflow-on-zoom, then I was thinking that I might be able to implement it as
> follows:
> 
> 1) Add a new event type, "Browser:ReflowZoomToRect" to
> JavaPanZoomController.java and the browser.js code that calls
> nsDocumentViewer::ChangeMaxLineBoxWidth.
> 2) Add a new method to GLController.java called "pauseCompositor", that
> essentially does the opposite of GLController::resumeCompositor. This would
> need to be synchronous, just like resumeCompositor is. It would call another
> added method, GeckoAppShell::schedulePauseComposition(), and then call
> GeckoAppShell::sendEventToGecko(GeckoEvent.createCompositorPauseEvent()).
> Ideally, this would pause the compositor on the Java side after the
> composition of the zoom animation (blurring) was done, but before we
> rendered the final version of the zoom (high fidelity, non-blurred).
> 4) Inside of nsDocumentViewer, we add a callback handler for reflow, so that
> when reflow is finished, the pres shell notifies the document viewer for
> which the reflow was triggered. We're probably going to need to keep track
> of each individual reflow-on-zoom sub-event that we trigger, since an
> nsDocumentViewer can trigger reflows for all its children. We'd then notify
> the app shell that it's ok to resume composition.
> 
> I'm not sure about the last bit - whether we're going to be able to
> communicate from the document viewer to the app shell in a reasonable way.
> I've never worked with the nsAppShell class before, so I'm not sure whether
> it's difficult to acquire a reference to it. 
> 
> I think that if we were to choose the other route - i.e. use a more active
> "blurring" shader - then we'd almost surely have to connect it in a similar
> way. 
> 
> Chris, what do you think about this? Does it seem reasonable from your point
> of view, or am I making this more complicated than it should be?

Sorry it's taken me so long to get to this...

I think what you're suggesting (at a high level) is something we should do anyway to reduce unnecessary work, but we should be clear that this isn't us performing any effect here - the blurring you get from zooming in isn't aesthetically pleasing in the same way as a blur shader and I don't think it'd convey the same message to the user. But that's a little besides the point I suppose.

I'm not sure you'd want to pause/resume the compositor while this happens, as that would halt all drawing and make the browser seem as if it had crashed. Instead, you just want to stop drawing Gecko-side. I'm not sure what exactly in layout you'd change to do this, I guess something in the refresh driver? (that isn't code I'm familiar with, sorry)

I don't think there needs to be any compositor interaction to do what you want to do - on the other hand, if you wanted to apply an effect while the reflow was happening, that would be a different story.


I think we already cancel all draws when our resolution has changed incidentally (see progressiveUpdateCallback in GeckoLayerClient.java), so some of what you want to happen may already be happening. I guess we need is a way to atomically set resolution, display-port and reflow-on-zoom (so some kind of pause-reflow or pause-refresh capability in layout might be a way to go about this). I think someone from the layout team would be better advising here.

Comment 4

[Scott Johnson (:jwir3)]

Attachment: b878935-progressiveupdatecallback (WIP)

Here's a first version of this, which does work, but I'm wondering about a couple of things: 

1) Whether this is the right approach (i.e. implementing this in the Pan/Zoom controller, rather than in the refresh driver). My rationale for implementing this here is that if I were to implement the display "pausing" in the refresh driver, then, at some point, I'm going to need a method of communicating from Java->Gecko (i.e. another JNI method). On the other hand, if I simply delay the painting until reflow is finished within progressiveUpdateCallback(), then it solves the problem without the added message-passing required, and I get the bonus of it drawing the low-precision tiles (the blurring effect) while the positioning is ongoing.

2) I'm not sure if I need to do anything special in the APZC - I've implemented the same methods, but I don't know if the synchronization is going to need to change, or if perhaps these methods even apply to the APZC.

Just looking for some idea of whether this is the right general solution. Thanks, Chris.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 805599 [details] [diff] [review]
b878935-progressiveupdatecallback (WIP)

Chris is on leave for a couple of months. However I took a look at the patch and I'm not sure this is the best way. It seems like a lot of overhead and binding to the PZC which is conceptually unrelated to this. Also it doesn't actually prevent gecko from painting; it just prevents the tiles from getting uploaded to the compositor.

I agree with what Chris said in comment 3, what you want to do is in browser.js, instead of seting reflowOnZoomStage to 'zoom', you want to tell Gecko to stop painting somehow, and then when calling performReflowOnZoom you tell Gecko to start painting again. I believe the nsRefreshDriver::Freeze() and ::Thaw() functions would be appropriate for this but you'd need to expose them somehow to be callable from browser.js. That should be the extent of what you need to do here, you shouldn't need to touch java code at all, I think.

Comment 6

[Scott Johnson (:jwir3)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> Comment on attachment 805599 [details] [diff] [review]
> b878935-progressiveupdatecallback (WIP)
> 
> Chris is on leave for a couple of months. However I took a look at the patch
> and I'm not sure this is the best way. It seems like a lot of overhead and
> binding to the PZC which is conceptually unrelated to this. Also it doesn't
> actually prevent gecko from painting; it just prevents the tiles from
> getting uploaded to the compositor.
> 
> I agree with what Chris said in comment 3, what you want to do is in
> browser.js, instead of seting reflowOnZoomStage to 'zoom', you want to tell
> Gecko to stop painting somehow, and then when calling performReflowOnZoom
> you tell Gecko to start painting again. I believe the
> nsRefreshDriver::Freeze() and ::Thaw() functions would be appropriate for
> this but you'd need to expose them somehow to be callable from browser.js.
> That should be the extent of what you need to do here, you shouldn't need to
> touch java code at all, I think.

Yes, so this is the original avenue I took to this, but it seems like this is overkill for something as trivial as this. Freezing the refresh driver doesn't seem like it's the best approach to this problem, since so much is now becoming tied to the refresh driver. 

Additionally, it doesn't seem like something we want to give access to JS code to do - even from chrome. Although, I could be wrong on this point.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Ok, but I still think you want some way of pausing Gecko from painting. The current approach lets Gecko paint but then throws away the tiles. This is inefficient because Gecko is painting for no reason, and also might lead to correctness issues because Gecko will assume that everything it painted was drawn to the screen, whereas some or all of it was actually thrown away. Subsequent paints by Gecko may therefore use a smaller invalid region than is actually invalid and so stuff might not get fully painted.

Comment 8

[Scott Johnson (:jwir3)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> Ok, but I still think you want some way of pausing Gecko from painting. The
> current approach lets Gecko paint but then throws away the tiles. This is
> inefficient because Gecko is painting for no reason, and also might lead to
> correctness issues because Gecko will assume that everything it painted was
> drawn to the screen, whereas some or all of it was actually thrown away.
> Subsequent paints by Gecko may therefore use a smaller invalid region than
> is actually invalid and so stuff might not get fully painted.

That's a totally reasonable point. I'll see what I can do to augment this.

Just for some clarification, the reason I didn't want to use the refresh driver was because it's being used as a global timer for animations (GIF animations are all controlled by the refresh driver, for example, as, I think are CSS animations and transitions, as well as some SVG stuff, like filter effects). So, if we turn off painting via the refresh driver, I _think_ all of this will cease to animate. I've verified the effects in the animated favicon at the top bar, although this isn't that important of a feature. However, I'm concerned about more important features (like the throbber icon, and possibly even some button animations). 

Moreover, I would expect those to be controlled by a refresh driver that's owned by the chrome presshell, but it seems that all of the drawable areas in the screen on android that are painted by the refresh driver are actually inside of a chrome presshell - that is, if I turn off the refresh driver ticks only for refresh drivers in non-chrome presentations, it continues to redraw all of the content.

Perhaps there's a happy medium here that I can come up with. Perhaps some method of hinting to the presentation that painting isn't going to be displayed for a certain period of time? It seems to me that the document viewer is the closest object to the presentation shell/context that interfaces with chrome Javascript. If I could pass a "change hint" of some kind to the document viewer that tells the presentation that frames for a given period of time aren't likely to be composited, then perhaps we could use this somehow to either draw at a lower resolution, or not draw at all, but only for content.

It could also serve as a hint that we'd have to use a full invalidation region on every cycle.

Let me see what I can come up with and we can talk tomorrow.

Thanks for the feedback, Kats!

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Scott Johnson (:jwir3) from comment #8)
> 
> Just for some clarification, the reason I didn't want to use the refresh
> driver was because it's being used as a global timer for animations (GIF
> animations are all controlled by the refresh driver, for example, as, I
> think are CSS animations and transitions, as well as some SVG stuff, like
> filter effects). So, if we turn off painting via the refresh driver, I
> _think_ all of this will cease to animate. 
I think we *want* everything to stop animating while we're doing this reflow.


> Moreover, I would expect those to be controlled by a refresh driver that's
> owned by the chrome presshell, but it seems that all of the drawable areas
> in the screen on android that are painted by the refresh driver are actually
> inside of a chrome presshell - that is, if I turn off the refresh driver
> ticks only for refresh drivers in non-chrome presentations, it continues to
> redraw all of the content.
certainly seem like we can do better here, but let's not let that block us. Follow ups are your friend.

Comment 10

[Scott Johnson (:jwir3)]

Attachment: b878935

After the discussion on here, and speaking with dbaron about this, I have implemented the version I think you were getting at, kats. If you could just take a look and make sure that this is what you had in mind, I'll send it over to dbaron for review (unless of course, you feel comfortable reviewing it yourself).

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 809249 [details] [diff] [review]
b878935

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

This is on the right track, and definitely in line with what I had in mind before. Looking at the code in browser.js now the main concern I have is that it might be possible to get in situations where you call pausePainting() but then because of early returns or other things never get around to setting _mReflozPositioned. If that happens you might end up stuck with painting paused and it doesn't get resumed until much later. It's been a while since I've looked at this code so I forget what functions get invoked in what order but it seems to me that the call to pausePainting() and the setting of _mReflozPositioned should be closer together.

Comment 12

[Scott Johnson (:jwir3)]

Attachment: b878935

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> Looking at the code in browser.js now the main concern I have is
> that it might be possible to get in situations where you call
> pausePainting() but then because of early returns or other things never get
> around to setting _mReflozPositioned. If that happens you might end up stuck
> with painting paused and it doesn't get resumed until much later. It's been
> a while since I've looked at this code so I forget what functions get
> invoked in what order but it seems to me that the call to pausePainting()
> and the setting of _mReflozPositioned should be closer together.

So, I've looked into this. I initially thought that, as you suggested, bringing pausePainting() closer to the setting of mReflozPositioned would be beneficial, but the problem is that I want it to happen before any painting is done that recomputes the higher-fidelity/zoomed-in version of the viewport (that is, I want the blurriness to remain until after the reflow is finished, otherwise there isn't any benefit to the patch).

So, instead of doing this, I'm going to go through and try to verify for you that every call to pausePainting() is now accompanied by a corresponding call to resumePainting().

The order of events looks something like this (note that "... some time ..." indicates that the event following happens asynchronously at some point in the future):
onDoubleTap() (sets up metadata for positioning if refloz enabled and pauses painting) -> ... some time ... -> setViewport() (from the initial zoom event) -> performReflowOnZoom (schedules a change to max line box width) -> ... some time ... -> doChangeMaxLineBoxWidth() (does the platform-level max line box width change, which in turn schedules a reflow) -> zoomInAndSnapToRange() (positions the viewport and sets appropriate flags) -> ... some time ... -> setViewport() (changes position) -> Observe('after-viewport-change') (re-enables painting)

I'm going to start with arguing that doChangeMaxLineBoxWidth and zoomInAndSnapToRange call resumePainting() at all junctures before returning, and then use these to argue that the other functions accompany calls to pausePainting() with a corresponding call to resumePainting().

Inside of zoomInAndSnapToRange(), we have a potential early return where we encountered an error with the positioning. Although I don't think this is a possible path to get to, I added a call to reset the reflow-on-zoom flags and re-enable painting in this case, just in case we get here somehow.  Otherwise, we adjust our position and set the mReflozPositioned flag to true (which will, in turn , ensure that we will re-enable painting after the next viewport change.  I also modified the patch to set this flag to true prior to the sendMessageToJava() call, so we ensure that it isn't possible that the after-viewport-change callback could happen prior to this flag being set.

Inside of doChangeMaxLineBoxWidth, there are two possible return options: 1) we have a range as set up in onDoubleTap (we triggered a series of events leading to a reflow-on-zoom action), or 2) we don't have this range, in which case we reset the flags associated with reflow-on-zoom and re-enable painting.  If we do have a range, then we call zoomInAndSnapToRange(). In either case, resumePainting() is called.

pausePainting is called in onDoubleTap, if a) reflow-on-zoom is enabled, b) we don't already have a reflow-on-zoom point in memory (i.e. a successive call to onDoubleTap before it's had a chance to process) and c) we aren't suppressing reflow-on-zoom. This sets the flag for 'we probably need to reflow-on-zoom'. If, for some reason, we need to zoom out (early return possibility #1), then we will call resetMaxLineBoxWidth(), which clears the 'probably need reflow-on-zoom' bit, and schedules a reflow-on-zoom event via doChangeMaxLineBoxWidth(), which will re-enable painting.

Now, if we didn't early-return from onDoubleTap() or call zoomOut(), then we will call zoomToElement(). This function will, if reflow-on-zoom is enabled, either zoomOut() (early return potential #2 and #3, but which we've already established calls resumePainting()) or schedule a ZoomToRect event.

If we schedule the ZoomToRect event (normal progression), then at the end of this, we'll end up calling setViewport() (at some time in the future). If we're zooming in, but it's a pinch-zoom in (i.e. we don't want to utilize the reflow-on-zoom positioning and max line box width truncation), then we clear the bits associated with reflow-on-zoom and re-enable painting. If we do want to perform reflow-on-zoom (that is, we've determined we're a) zooming, b) reflow-on-zoom is enabled, and c) the 'probably need to perform reflow on zoom' flag is set, then we call performReflowOnZoom() at this point.

performReflowOnZoom() will schedule a reflow-on-zoom event via doChangeMaxLineBoxWidth. (i.e. no early return possibility).

So, with all of that, I'm requesting review from you, kats, for the logic in browser.js. If you think that another individual should also review it, feel free to let me know.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 811153 [details] [diff] [review]
b878935

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

I still think this code is brittle but at least you've convinced me that it's not wrong :) r=me with comments addressed.

::: mobile/android/chrome/content/browser.js
@@ +2769,4 @@
>    performReflowOnZoom: function(aViewport) {
> +    let zoom = this._drawZoom ? this._drawZoom : aViewport.zoom;
> +
> +    let viewportWidth = gScreenWidth / zoom;

As long as you're fixing indentation, can you fix onPinchFinish and the closing brace on _zoomInAndSnapToRange as well?

@@ +3145,5 @@
>        // because we are pinch-zooming to zoom out.
>        BrowserEventHandler.resetMaxLineBoxWidth();
>        BrowserApp.selectedTab.reflozPinchSeen = false;
>      } else if (BrowserApp.selectedTab.reflozPinchSeen &&
>                 isZooming) {

As discussed on IRC I think this reflozPinchSeen stuff is dead code and should probably be removed (in a follow-up bug is fine)

@@ +4377,5 @@
>      if (BrowserEventHandler.mReflozPref &&
>         !BrowserApp.selectedTab._mReflozPoint &&
>         !this._shouldSuppressReflowOnZoom(element)) {
> +
> +      let data = JSON.parse(aData);

It would be nice to put a block comment in here that provides a high-level overview of how reflow-on-zoom is handled (e.g. something that lists the order of events/messages between JS and Java, and maybe the important JS functions invoked).

@@ +4492,5 @@
>  
>    _zoomInAndSnapToRange: function(aRange) {
>      if (!aRange) {
>        Cu.reportError("aRange is null in zoomInAndSnapToRange. Unable to maintain position.");
> +      BrowserApp.selectedTab.clearReflowOnZoomPendingActions();

Yeah, the only call site for this function already has an "if (range)" check so for clarity maybe remove this check and just add a comment that says aRange must be non-null here.

Comment 14

[Scott Johnson (:jwir3)]

Fixed issues mentioned above, and landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5549d97d8d

Comment 15

[(no longer active)]

Shouldn't this patch change the IIDs of both nsIMarkupDocumentViewer and nsIPresShell?

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6a5549d97d8d

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> Shouldn't this patch change the IIDs of both nsIMarkupDocumentViewer and
> nsIPresShell?

Yup. Sorry I missed that during review. Also it occurs to me that in my head I was only r+'ing the browser.js part of the patch and somebody from layout should probably still r+ the rest of the patch because I'm not qualified to review it. Maybe it would be better to back this out until that happens? Or get a post-review from somebody and address any review comments + the IID thing in a follow-up patch.

Comment 18

[Scott Johnson (:jwir3)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> > Shouldn't this patch change the IIDs of both nsIMarkupDocumentViewer and
> > nsIPresShell?
> 
> Yup. Sorry I missed that during review. Also it occurs to me that in my head
> I was only r+'ing the browser.js part of the patch and somebody from layout
> should probably still r+ the rest of the patch because I'm not qualified to
> review it. Maybe it would be better to back this out until that happens? Or
> get a post-review from somebody and address any review comments + the IID
> thing in a follow-up patch.

Ugh, yeah, sorry about that. I misunderstood - I thought you were reviewing the whole patch. I'll back out for the IID issue and re-request review.

Comment 19

[Scott Johnson (:jwir3)]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1382e5fe60f

Comment 21

[Scott Johnson (:jwir3)]

Attachment: b878935

Version with IID changed for nsIMarkupDocumentViewer. Kats requested that I get another review from layout.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Two comments and a question:

 (1) you need to rev NS_IPRESSHELL_IID as wel

 (2) it seems like the names of the methods on the pres shell and on the document viewer ought to be more similar unless there's a logical reason for them to be different

 (3) it seems odd to me that you walk down the content viewer tree and then back up the pres shell tree.  That seems O(N^2) in tree depth, which isn't great.  Is there a better way to deal with that?

Comment 23

[Kartikaya Gupta (email:[e-mail])]

(In reply to Scott Johnson (:jwir3) from comment #18)
> Ugh, yeah, sorry about that. I misunderstood - I thought you were reviewing
> the whole patch.

My mistake - I should have said so. Thanks for doing the backout.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, does something guarantee that this doesn't interfere with existing users of Freeze/Thaw?  Maybe the fact that you're always operating on a foreground tab?  (It seems a little fragile, but maybe it's ok.)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Hmm.  Current use of Freeze()/Thaw() basically happens when going in/out of bfcache.  So what happens if we freeze for this stuff, then the page unloads, going into bfcache, and then the Thaw() for this new code happens?

Comment 26

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] (needinfo? me) from comment #22)
> Two comments and a question:
> 
>  (1) you need to rev NS_IPRESSHELL_IID as wel

Yep, I'll do that.

>  (2) it seems like the names of the methods on the pres shell and on the
> document viewer ought to be more similar unless there's a logical reason for
> them to be different

Well, actually, I thought this was less confusing this way, but if you disagree, then how about the following names:

nsIMarkupDocumentViewer::PausePainting()
nsIMarkupDocumentViewer::ResumePainting()
PresShell::PausePainting()
PresShell::ResumePainting

The reason I wanted "Freeze/Thaw" was to avoid confusion with the existing Suppress/Unsuppress painting methods.


>  (3) it seems odd to me that you walk down the content viewer tree and then
> back up the pres shell tree.  That seems O(N^2) in tree depth, which isn't
> great.  Is there a better way to deal with that?

Well, perhaps what I could do here is to find all of the child document viewers (i.e. the leaves of the tree) and then walk up the pres shell tree from each of these. I'm not sure that this will help a lot, though. On the other hand, is it absolutely necessary to walk down through all the children of the document viewers? I'm not sure this is useful, since we're going to be disabling painting on the uppermost presshell(s). It's possible I'm wrong about this, though.

(In reply to David Baron [:dbaron] (needinfo? me) from comment #24)
> Also, does something guarantee that this doesn't interfere with existing
> users of Freeze/Thaw?  Maybe the fact that you're always operating on a
> foreground tab?  (It seems a little fragile, but maybe it's ok.)

I doubt there is anything that ensures non-interference with these. I could set up some guards, though, that would indicate failure if the refresh driver in question is already frozen.

(In reply to Boris Zbarsky [:bz] from comment #25)
> Hmm.  Current use of Freeze()/Thaw() basically happens when going in/out of
> bfcache.  So what happens if we freeze for this stuff, then the page
> unloads, going into bfcache, and then the Thaw() for this new code happens?

I'm not sure what bfcache is. If we were to call Freeze(), then the page unloads (which I'm not sure what would trigger this... the user minimizing/backgrounding the application?), and then Thaw() is called from this new code, what will happen is that the refresh driver will thaw (since we don't have a freeze/thaw "count"), consequently resuming refresh notifications.

If this isn't the behavior we want, I could add a "freeze count" to the refresh driver - i.e. every call to Freeze() increments the counter, and every call to Thaw() decrements the counter, and mFrozen == 
freezeCount > 0.

This would alleviate (I think) the problem dbaron mentioned in comment 24, as well.

Comment 27

[Boris Zbarsky [:bzbarsky]]

> then the page unloads (which I'm not sure what would trigger this

Think pending navigation that started before whatever it is you're doing?

> I could add a "freeze count" to the refresh driver

Seems like that's what we need to do, yes.

Comment 28

[Boris Zbarsky [:bzbarsky]]

One other question.  What happens if  the presshell is removed from the tree between FreezePainting() and ThawPainting()?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One other option is to use a reference count, where 0 means thawed, and >0 means frozen.

Comment 30

[Scott Johnson (:jwir3)]

(In reply to Boris Zbarsky [:bz] from comment #28)
> One other question.  What happens if  the presshell is removed from the tree
> between FreezePainting() and ThawPainting()?

Nothing special happens to handle this right now. I can add functionality that will handle this, though.

Comment 31

[Scott Johnson (:jwir3)]

Attachment: b878935-freezecount

I've split this modification to the refresh driver (adding a "freeze count") into a separate prerequisite patch.

Comment 32

[Scott Johnson (:jwir3)]

Attachment: b878935-part2

Addressed comments as I discussed in comment 26. The only one I didn't address was the following: 
> >  (3) it seems odd to me that you walk down the content viewer tree and then
> > back up the pres shell tree.  That seems O(N^2) in tree depth, which isn't
> > great.  Is there a better way to deal with that?
> 
> Well, perhaps what I could do here is to find all of the child document
> viewers (i.e. the leaves of the tree) and then walk up the pres shell tree
> from each of these. I'm not sure that this will help a lot, though. On the
> other hand, is it absolutely necessary to walk down through all the children
> of the document viewers? I'm not sure this is useful, since we're going to
> be disabling painting on the uppermost presshell(s). It's possible I'm wrong
> about this, though.

As I didn't know how you wanted me to handle this.

Comment 33

[Scott Johnson (:jwir3)]

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=cbe718fbb7a9

Comment 34

[Aaron Train [:aaronmt]]

We removed double tap to reflow and users want it back, on bug 900564 (where we disabled it) this was a dependency. Should this track a release?

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 814996 [details] [diff] [review]
b878935-freezecount

r=dbaron, although this (or the next patch) is going to need some additions to fix the potential freeze/thaw during navigation problems with the next patch

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 815008 [details] [diff] [review]
b878935-part2

So one option for what we should do here to fix both comment 28 and comment 32 is:

 (1) remove the bits of PresShell::{Pause,Resume}Painting that step up to the parent pres shell

 (2) change the same methods so that they only call Freeze/Thaw on the refresh driver if GetPresContext()->RefreshDriver()->PresContext() == GetPresContext() (that is, if the pres context owns the refresh driver, rather than pointing to one from its parent)

 (3) document that pause/resume painting (on both pres shell and document viewer) might not work if the pres shell doesn't have its own refresh driver

I'm not sure if that will actually work, though -- it's not clear to me what can happen during a refresh tick on the parent.  Maybe roc or mattwoodrow could provide some guidance as to whether that's a way we'd reasonably want to support for suppressing painting on the child?

**If** that's not good enough, then I guess I can see two other paths, by:

adding an explicit thawed/frozen counter on the pres shell as well, and then either:

 (a) going back to walking up to the parent, but using the boolean to correctly managed the parent's counter in the presence of navigation, or

 (b) doing something more explicit to suppress painting based on that counter

(b) feels preferable to me.  But I also feel like a drop of advice from someone who knows our painting setup these days better wouldn't be a bad idea.

Comment 38

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: reflow_freeze_part2.patch

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So what happens when the set of children changes between nsDocumentViewer::PausePainting() and nsDocumentViewer::ResumePainting()?  At the very least it seems like that could lead to (a) assertions in nsRefreshDriver::Thaw (as modified by attachment 814996 [details] [diff] [review]) and (b) occasional painting where we don't want it.

Comment 40

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #39)
> So what happens when the set of children changes between
> nsDocumentViewer::PausePainting() and nsDocumentViewer::ResumePainting()? 
> At the very least it seems like that could lead to (a) assertions in
> nsRefreshDriver::Thaw (as modified by attachment 814996 [details] [diff] [review]
> [review]) 
Maybe I'm missing something, but I don't see how this would cause an assertion. Can you explain?
> and (b) occasional painting where we don't want it.
The odd (and I assume rare) unwanted paint seems pretty inconsequential

Comment 41

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #40)
> (In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #39)
> > So what happens when the set of children changes between
> > nsDocumentViewer::PausePainting() and nsDocumentViewer::ResumePainting()? 
> > At the very least it seems like that could lead to (a) assertions in
> > nsRefreshDriver::Thaw (as modified by attachment 814996 [details] [diff] [review]
> > [review]) 
> Maybe I'm missing something, but I don't see how this would cause an
> assertion. Can you explain?

(1) UI code calls PausePainting on the document viewer, which in turn calls PausePainting on every refresh driver in that subtree
(2) the document loading process adds a new iframe under the conditions that lead us to give that subframe its own refresh driver
(3) UI code calls ResumePainting on the same document viewer, which calls PausePainting on every refresh driver touched in (1) which is fine, plus the one created in (2) which will assert.

I suppose (2) is rare, though, given the current conditions in nsPresContext::Init, though those may change over time, and I feel a bit bad about setting up a situation that makes it harder to change those.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8346373 [details] [diff] [review]
reflow_freeze_part2.patch

nsDocumentViewer::PausePainting, ResumePainting, and ChangeMaxLineBoxWidth should be NS_IMETHODIMP (otherwise it won't compile on Windows, I think, since there will be a mismatch as to whether it's marked __stdcall.

r=dbaron, although I suspect we may end up needing to weaken the NS_ASSERTION in patch 1.  And thanks for finishing this up.

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3d5a97333964

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70befb4cf2f4

Comment 45

[Carsten Book [:Tomcat]]

Hi,

had to backout this change from mc (and integration trees) as part of https://tbpl.mozilla.org/?tree=Fx-Team&rev=28f65c4592b9 - seems there is a regression on fx-team causing as example https://tbpl.mozilla.org/php/getParsedLog.php?id=32271205&tree=Fx-Team and https://tbpl.mozilla.org/php/getParsedLog.php?id=32271639&tree=Fx-Team and hoping this backout fix this

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Looks like this should have had a UUID change? That would certainly lead to random hilarity.

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2f4e0a86294d