Closed
Bug 451441
Opened 16 years ago
Closed 16 years ago
don't synchronously redraw everytime we move the canvas
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
fennec1.0a1
People
(Reporter: blassey, Assigned: blassey)
Details
(Keywords: mobile)
Attachments
(3 files, 4 obsolete files)
921 bytes,
patch
|
pavlov
:
review+
|
Details | Diff | Splinter Review |
2.12 KB,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
980 bytes,
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | 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
Comment 1•16 years ago
|
||
create a new method on the nsIDOMWindowUtil that does a redrawElement(). Much less code, much nicer for everyone. cc'jst
Updated•16 years ago
|
Summary: don't redraw everytime we move the canvas → don't synchronously redraw everytime we move the canvas
Updated•16 years ago
|
Priority: -- → P1
Target Milestone: --- → Fennec A1
Assignee | ||
Comment 2•16 years ago
|
||
Assignee: nobody → blassey
Status: NEW → ASSIGNED
Assignee | ||
Updated•16 years ago
|
Attachment #334795 -
Flags: review?(jst)
Assignee | ||
Comment 3•16 years ago
|
||
Comment 4•16 years ago
|
||
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 5•16 years ago
|
||
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
Assignee | ||
Comment 6•16 years ago
|
||
(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.
Assignee | ||
Comment 8•16 years ago
|
||
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.
Updated•16 years ago
|
Flags: blocking-fennec1.0+
Assignee | ||
Comment 10•16 years ago
|
||
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 11•16 years ago
|
||
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+
Assignee | ||
Comment 13•16 years ago
|
||
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: 16 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 → ---
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.
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)
Attachment #337348 -
Flags: superreview+
Attachment #337348 -
Flags: review?(roc)
Attachment #337348 -
Flags: review+
Checked in
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Attachment #337270 -
Attachment is obsolete: true
Attachment #337270 -
Flags: superreview?(roc)
Attachment #337270 -
Flags: review?(roc)
Comment 21•13 years ago
|
||
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.
Description
•