Closed Bug 451441 Opened 12 years ago Closed 12 years ago

don't synchronously redraw everytime we move the canvas

Categories

(Firefox for Android Graveyard :: General, defect, P1)

x86
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
fennec1.0a1

People

(Reporter: blassey, Assigned: blassey)

Details

(Keywords: mobile)

Attachments

(3 files, 4 obsolete files)

Attached patch WIP component patch (obsolete) — Splinter Review
this is suboptimal: 

480           window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
481                 .getInterface(Components.interfaces.nsIDOMWindowUtils)
482                 .redraw();


http://mxr.mozilla.org/mobile-browser/source/chrome/content/deckbrowser.xml#480
create a new method on the nsIDOMWindowUtil that does a redrawElement().  Much less code, much nicer for everyone.  cc'jst
Summary: don't redraw everytime we move the canvas → don't synchronously redraw everytime we move the canvas
Priority: -- → P1
Target Milestone: --- → Fennec A1
Attached patch adds method to nsIDOMWindowUtils (obsolete) — Splinter Review
Assignee: nobody → blassey
Status: NEW → ASSIGNED
Attachment #334795 - Flags: review?(jst)
Comment on attachment 334795 [details] [diff] [review]
adds method to nsIDOMWindowUtils

driv-by nits:

How about renaming ProcessUpdates -> RedrawElement()

add a better comment in the idl that at least defines what aDOMEl can be.

no space before the !:

+    if ( !frame) 

how about a comment above:
+  gdk_window_process_updates(gdkWindow, true);


Do you wanna return NS_OK on non-gtk2 platforms?  how about NS_ERROR_NOT_IMPLEMENTED
Comment on attachment 334795 [details] [diff] [review]
adds method to nsIDOMWindowUtils

a) i don't think we want to do it this way
b) gdk code should live in widget, not content
(In reply to comment #5)
> (From update of attachment 334795 [details] [diff] [review])
> a) i don't think we want to do it this way

Do you have a suggestion other than the timer approach you suggested on irc?

> b) gdk code should live in widget, not content

We already make a gdk call from this file.  But, if you think is appropriate I can add a method to nsWidget that nsDOMWindowUtils can call

rev the UUID on nsIDOMWindowUtils.

This doesn't need to be GTK-only. And picking a window to invalidate the way you do could be quite wrong; if there's a widget on top of the frame's nearest containing window, this will basically do nothing. Also, there could be delayed invalidation that's queued as Gecko events, and you're not flushing that, so this might not do what you want.

What I'd try is, get the presshell's view manager (GetViewManager()), then call
  viewManager->BeginUpdateViewBatch();
  viewManager->EndUpdateViewBatch(VM_REFRESH_IMMEDIATE);
That will play nice with all our infrastructure. It will actually flush all pending invalidates to the whole chrome window, but unlike DOMWindowUtils.redraw, it won't force-invalidate anything, so you should be OK performance-wise.
Attached patch updated based on roc's comments (obsolete) — Splinter Review
This is a bit slower than the previous patch, and the drawing doesn't seem to be as smooth.  I think it may be because we recurse through all the child windows "manually," rather than letting gdk do it by passing TRUE to gdk_window_process_updates.

Perhaps we can combine the two patches; use the gtk2 specific code for gtk2 and the gecko code for other platforms.
This doesn't need an element parameter, you can get the presshell associated with the window and use that.

Not sure why it would be slower. I don't think the overhead here of going through different paths will be significant, we should spend all the time painting. Are there updates outside the canvas area that are being processed?

The GTK2 code you had could miss updating some invalidated areas --- ones where we have a pending Gecko Invalidate event --- so I really don't think you should use it.
Flags: blocking-fennec1.0+
Attachment #334751 - Attachment is obsolete: true
Attachment #334795 - Attachment is obsolete: true
Attachment #335018 - Attachment is obsolete: true
Attachment #335613 - Flags: review?(roc)
Attachment #334795 - Flags: review?(jst)
Comment on attachment 334798 [details] [diff] [review]
calls processUpdates() instead of redraw()

r+ once the other piece lands
Attachment #334798 - Flags: review+
Comment on attachment 335613 [details] [diff] [review]
removes the dom element arguement

+  nsIFrame* frame = presShell->GetRootFrame();
+  if ( !frame) 
+    return NS_ERROR_UNEXPECTED;

You don't use this. Remove it.
Attachment #335613 - Flags: superreview+
Attachment #335613 - Flags: review?(roc)
Attachment #335613 - Flags: review+
pushed to mozilla-central:
author	Brad Lassey <blassey@mozilla.com>
	Wed Aug 27 17:24:03 2008 -0400 (at Wed Aug 27 17:24:03 2008 -0400)
changeset 18474	0d33aa00cf89
parent 18473	43a4c95537b8

and mobile-browser:
author	Brad Lassey <blassey@mozilla.com>
	Wed Aug 27 17:28:03 2008 -0400 (at Wed Aug 27 17:28:03 2008 -0400)
changeset 116	0b17ad3cd200
parent 115	02edca6b16db
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
This impl is bogus -- as written, it's essentially a noop.  As far as I can tell, EndUpdateViewBatch with VMREFRESH_IMMEDIATE eventually gets to ViewManager::UpdateWidget area, and from there, best-case, to widget->Invalidate(bounds, PR_FALSE);  Given PR_FALSE there, invalidate will only call gdk_window_invalidate_rect(); If it was PR_TRUE, we'd get a gdk_window_process_updates(window, FALSE); as well.

Neither of these is good enough; at the point where fennec needs this, all the invalidates have already been sent -- it just needs to tell gdk to process them.  So the gdk_window_invalidate_rect call is unnecessary.  gdk_window_process_updates -might- work, except that this is being called on the toplevel nsWindow/GdkWindow, and the FALSE is telling it to not descend into children... fennec is (currently) highly xul:deck-happy, so the a process_updates on the top level window won't actually process any necessary updates.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch let's just make the gdk call (obsolete) — Splinter Review
Let's just call gdk_process_all_updates() -- this is generally what's desired anyway, and it insulates callers from needing to know the details about when we do or don't create a native widget for some part of the document tree.  Leaving the old-code in there for non-gdk platforms since it may do something elsewhere depending on the widget impl (but whatever it does probably won't be sync, so I dunno..)
Attachment #337270 - Flags: superreview?(roc)
Attachment #337270 - Flags: review?(roc)
What should have been happening is that EndUpdateViewBatch(VMREFRESH_IMMEDIATE) calls EnableRefresh(VMREFRESH_IMMEDIATE) calls Composite() calls ForceUpdate() calls UpdateWidgetsForView() calls nsIWidget::Update() for every widget which calls gdk_window_process_updates for that widget's mDrawingarea->inner_window (with recursive=false, so every widget gets gdk_window_process_updates once).

If that's not working on trunk, I'd like to understand why, and fix whatever that problem is.
EndUpdateViewBatch(VMREFRESH_IMMEDIATE) ->
EnableRefresh(VMREFRESH_IMMEDIATE), we get that far..

I think the problem is here:
http://hg.mozilla.org/mozilla-central/file/tip/view/src/nsViewManager.cpp#l1926

mHasPendingUpdates is always FALSE here: I think it's because it's set to FALSE at the end of FlushPendingInvalidates, which is called at the end of a normal reflow update batch.  The ProcessPendingUpdates right before eventually just calls Invalidate on the widget (async), so no updates actually get processed, just an invalidate.
OK then, we just need to fix EnableRefresh so the Composite() call isn't guarded by mHasPendingUpdates, since that's just an incorrect optimization.
Attached patch how 'bout this?Splinter Review
Ok, how's this?  It only moves the mHasPendingUpdates check below the IMMEDIATE handling, to preserve earlier behaviour (avoid calling FlushPendingInvalidates unnecessarily).
Attachment #337348 - Flags: review?(roc)
Checked in
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Attachment #337270 - Attachment is obsolete: true
Attachment #337270 - Flags: superreview?(roc)
Attachment #337270 - Flags: review?(roc)
could someone verify this bug? or provide some steps to reproduce?
You need to log in before you can comment on or make changes to this bug.