Closed Bug 1059014 Opened 10 years ago Closed 9 years ago

Remove reflow on zoom code

Categories

(Core :: Layout, defect, P5)

Product:
Core
Component:
Layout
Platform:
All
Android
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox45 --- fixed
fennec + ---

People

(Reporter: kbrosnan, Assigned: n.nethercote, Mentored)

References

Details

Attachments

(2 files, 2 obsolete files)

(part 1) - Remove support for max line box width
16.49 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
(part 2) - Remove support for reflow on zoom event pending
7.03 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
bug 1049063 sets a precedent that we don't want to pursue making reflow on zoom function on Android. I believe that we are the only consumers of this code so it could be removed. Maybe this would be mentorable?
Summary: Removed reflow on zoom code → Remove reflow on zoom code
Brad, who would be a good mentor for this?
tracking-fennec: ? → +
Flags: needinfo?(blassey.bugs)
probably dbaron. Dbaron, are you willing?
Flags: needinfo?(blassey.bugs) → needinfo?(dbaron)
For the layout side, sure. Although I'd like to think a little more about whether I think the layout code should be removed.
Flags: needinfo?(dbaron)
Mentor: dbaron
filter on [mass-p5]
Priority: -- → P5
dbaron: I'm interested in attempting this. Can you point to somewhere in the code where I should start looking? Thank you.
Flags: needinfo?(dbaron)
Stuff in layout that we can get rid of is probably: everything related to nsIPresShell::SetMaxLineBoxWidth and MaxLineBoxWidth everything related to nsIPresShell::mReflowOnZoomPending https://hg.mozilla.org/mozilla-central/rev/2f4e0a86294d layout/base/tests/test_maxLineBoxWidth.html Stuff in layout that we maybe could get rid of but might as well keep, I think: https://hg.mozilla.org/mozilla-central/rev/3d5a97333964 I might have missed something since I went through it somewhat quickly, and I didn't look at any of the code on the Android side.
Flags: needinfo?(dbaron)
This basically reverts bug 780258.
Attachment #8683473 - Flags: review?(dbaron)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
This basically reverts part 2 of bug 836565.
Attachment #8683474 - Flags: review?(dbaron)
This reverts much of part 2 of bug 878935, except for the Android browser.js changes which have already been reverted elsewhere.
Attachment #8683477 - Flags: review?(dbaron)
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #6) > Stuff in layout that we can get rid of is probably: > everything related to nsIPresShell::SetMaxLineBoxWidth and MaxLineBoxWidth > everything related to nsIPresShell::mReflowOnZoomPending > https://hg.mozilla.org/mozilla-central/rev/2f4e0a86294d > layout/base/tests/test_maxLineBoxWidth.html The three patches I've uploaded remove all this stuff. I admit to only having the vaguest idea of what all this code does. I basically just reverted the relevant patches, as mentioned in the patch log messages.
Comment on attachment 8683477 [details] [diff] [review] (part 3) - Remove support for pausing and resuming painting Glad to see this bit going away; this was always rather scary.
Attachment #8683477 - Flags: review?(dbaron) → review+
10 files changed, 11 insertions(+), 421 deletions(-)
https://hg.mozilla.org/mozilla-central/rev/fbd2e8ad5795 https://hg.mozilla.org/mozilla-central/rev/4648b9e7d65a https://hg.mozilla.org/mozilla-central/rev/3ad33d09825a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Could we revive part 3? We used it to optimize app launch time in bug 1212321
Flags: needinfo?(n.nethercote)
I guess so. You're welcome to revert that patch -- although I'm a little curious to know what you're using it for.
Flags: needinfo?(n.nethercote)
Ok, will do. That gives us a small boost when launching gaia apps, see https://bugzilla.mozilla.org/show_bug.cgi?id=1212321#c8
Pausing painting on what, for what time period, to suppress what repainting that would otherwise happen?
I keep getting this error message after this bug landed. TypeError: docShell.contentViewer.pausePainting is not a function Fabrice are you going to backout?
Flags: needinfo?(fabrice)
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #18) > Pausing painting on what, for what time period, to suppress what repainting > that would otherwise happen? Vivien, you are the one on the hook there.
Flags: needinfo?(fabrice) → needinfo?(21)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d0bf692472248d6dedd790e17a223e855114e17 Backout 3ad33d09825a (bug 1059014 part 3) because the removed code is less dead than it first appears.
I backed out part 3 due to the errors Evelyn mentioned in comment 19. This will also make Fabrice happy :)
Flags: needinfo?(21)
See Also: → 1227424
OK, I guess the issue was more with bug 1212321 that is changing how apps are painting at launch. Can we please: * backout part 3 here again * backout the patch in bug 1212321 * check that Gij16 is now OK and unhide it Thanks a lot !
Keywords: checkin-needed
Depends on: 1226284
I talked with vivien, he will reland the part 3 as part of bug 1212321 to make things clearer.
(In reply to David Baron [:dbaron] ⌚UTC-8 from comment #18) > Pausing painting on what, for what time period, to suppress what repainting > that would otherwise happen? pausePainting/resumePainting will be used to stop the painting of an application that launch an other application. For example, when the Gaia homescreen launch an application, its refresh will be paused, while the other application is opened (with a transition made by the system app on top of it). There are 2 things that affect the app launch time with our current homescreen: One is an opacity animation on the app icon that has been touched on the homescreen app. The animation cost an expensive restyle/reflow/repaint first, and then it continues to composite once the app has moved out of view, creating small pauses in main thread of the system app. A solution for this one would be to *just* remove the guilty css rule that trigger this transition but... - I have already did that twice and people keep adding such an animation (that nobody really see fwiw). - With third-party homescreen it will be easy for app author to do such things, slowing down other app launches. So that's why I'm trying to enforce that directly at the Gecko level. The other is the fact that touch events ends up triggering mouse events, which ends up triggering small reflows (I forgot exactly where there were - IIRC gdb told me something like http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#677).
(In reply to vnicolas from comment #28) > The other is the fact that touch events ends up triggering mouse events, > which ends up triggering small reflows (I forgot exactly where there were - > IIRC gdb told me something like > http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager. > cpp#677). Not sure if this bit is still a factor; I know that in the distant past we used to dispatch mousemove events for every touchmove, but we no longer do that. Now we only dispatch a mousemove/mousedown/mouseup/click when the user taps, and don't send mouse events otherwise.
(In reply to Julien Wajsberg [:julienw] from comment #27) > I talked with vivien, he will reland the part 3 as part of bug 1212321 to > make things clearer. Will this happen soon? Comment 19 is bad, and I don't want the reinstatement of the code to be waiting on other patches unnecessarily. I'm happy to re-backout if that helps.
Nicholas, we also backed out the patch in bug 1212321 that produced this error message :) Vivien should land this in a right order now.
(In reply to Julien Wajsberg [:julienw] from comment #31) > Nicholas, we also backed out the patch in bug 1212321 that produced this > error message :) Vivien should land this in a right order now. Is this going to happen soon? It's been three weeks now and we just started a new dev cycle, so the re-backout will have to be backported to Aurora :( Should I just do the re-backout myself?
Flags: needinfo?(felash)
No I think it's better and clearer if we don't do anything more here. Vivien will do the necessary changes for bug 1212321 in that bug, which makes things clearer for everybody. I'll see with him about checking it in.
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #33) > No I think it's better and clearer if we don't do anything more here. > > Vivien will do the necessary changes for bug 1212321 in that bug, which > makes things clearer for everybody. I'll see with him about checking it in. It's been a month and the patch will make it to Beta soon. Vivien's chance has been missed. I'm going to back it out myself.
Nicholas, I don't think it's necessary to back it out? Why do you think it's necessary? I was suggesting to back it out in bug 1212321 simply because it makes sense, but there is no urgency here. Or I am missing something ?
> Or I am missing something ? Part 3 broke stuff; see comment 19. Fabrice also envisions further uses for it; see comment 15.
https://hg.mozilla.org/integration/mozilla-inbound/rev/55f1fc505b88998fb91ca144d97a7fef2a14ba86 Re-backout c216ff19d690 (bug 1059014 part 3) because the removed code is less dead than it first appears.
Comment on attachment 8710280 [details] [diff] [review] Re-backout c216ff19d690 ( part 3) because the removed code is less dead than it first appears Approval Request Comment [Feature/regressing bug #]: this bug. Part 3 landed and broke stuff so I backed it out. And then the backout was backed out in bug 1212321 (which subsequently bounced), and then made it to Aurora. Confusing, I know, but the conclusion is that I've redone the backout on mozilla-inbound and need to backport it to Aurora. [User impact if declined]: errors as seen in comment 19. [Describe test coverage new/current, TreeHerder]: N/A. [Risks and why]: none. [String/UUID change made/needed]: none.
Attachment #8710280 - Flags: review+
Attachment #8710280 - Flags: approval-mozilla-aurora?
Nicholas, the issue in comment 19 was actually only caused by the patch in bug 1212321 that was backed out. We haven't seen any new occurrence of this error since then. That's why I'm saying this could be backed out simply in that bug.
(In reply to Julien Wajsberg [:julienw] from comment #40) > Nicholas, the issue in comment 19 was actually only caused by the patch in > bug 1212321 that was backed out. We haven't seen any new occurrence of this > error since then. Ah, I didn't realize that. Sorry for the confusion. I just talked with Fabrice on IRC and he said he will reland the second patch in bug 1212321. So my reinstating of pausePainting/resumePainting is ok, but I can clear the Aurora backport request.
Attachment #8683477 - Attachment is obsolete: true
Comment on attachment 8710280 [details] [diff] [review] Re-backout c216ff19d690 ( part 3) because the removed code is less dead than it first appears Aurora backporting isn't needed after all. Apologies for any confusion.
Attachment #8710280 - Attachment is obsolete: true
Attachment #8710280 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: