Bug 352093 (widget-removal)

Remove child widgets from content area

RESOLVED FIXED in mozilla1.9.2

Status

()

defect
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Blocks 2 bugs)

Trunk
mozilla1.9.2
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 8 obsolete attachments)

13.38 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.37 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.98 KB, patch
masayuki
: review+
Details | Diff | Splinter Review
1.97 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
2.49 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
884 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.15 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
8.12 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.88 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
26.47 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.08 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
867 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
6.22 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
3.36 KB, patch
surkov
: review+
Details | Diff | Splinter Review
52.82 KB, patch
jaas
: review+
dbaron
: review+
Details | Diff | Splinter Review
6.92 KB, patch
Details | Diff | Splinter Review
We can simplify a lot of code and probably get a significant performance win by removing usage of native widgets everywhere except for the toplevel window and plugins.

This will make view removal much easier so it would be best if it happened before view removal.

Work that this depends on includes bug 339548 (get plugin widgets up to the top of their widget hierarchy) and bug 130078 (integrate view manager hierarchies so there's one view manager hierarchy per toplevel window).
Did you forget about bug 316489? :)
*** Bug 316489 has been marked as a duplicate of this bug. ***
Alias: widget-removal
Blocks: 317991
Blocks: 356955
Blocks: 358936
Blocks: 327646
Blocks: 293656
Could bug 369599 be solved by this ?
(In reply to comment #0)
> We can simplify a lot of code and probably get a significant performance win by
> removing usage of native widgets everywhere except for the toplevel window and
> plugins.

By "native" are you referring to OS-native?

I hope not.
This bug isn't about user interface widgets (buttons, checkboxes, etc).  It's about native windows being used as drawing areas.
(In reply to comment #6)
> This bug isn't about user interface widgets (buttons, checkboxes, etc).  It's
> about native windows being used as drawing areas.
> 

Uhhh. OK.

Nevermind. :)
Blocks: 443766
Blocks: 457913
Blocks: 469007
I have this implemented. I'm going to file supporting bugs for various pieces of preliminary work that can land on their own. A great deal of the work will end up in bug 339548. This bug will contain two large patches: a patch to remove widgets for IFRAMEs, and a patch to remove widgets for scrollframes.
No longer depends on: 130078, compositor
Summary: Remove child widgets → Remove child widgets from content area
PresShell::DispatchSynthMouseMove needs to be able to dispatch an nsGUIEvent to a particular document. That breaks if that document has no widget, since the widget alone is no longer enough information to target the right document. So this patch adds a view parameter to nsViewManager::DispatchEvent. This avoids unnecessary GetViewFor calls in DispatchEvent too.
Attachment #386159 - Flags: review?(bzbarsky)
Comment on attachment 386159 [details] [diff] [review]
Part 1: Add view parameter to nsViewManager::DispatchEvent

>+++ b/view/src/nsView.cpp
>-    vm->DispatchEvent(aEvent, &result);
>+    view->GetViewManager()->DispatchEvent(aEvent, view, &result);

Why the change from "vm" to "view->GetViewManager()" here?

Apart from that, looks good.
Attachment #386159 - Flags: review?(bzbarsky) → review+
Probably a merge issue. I'll fix it before checking in. Thanks.
Quite a few places in our code call nsIViewManager::GetWidget to get the widget at the root of the document, and will be surprised when that returns null. So let's create GetRootWidget, which gets the root widget for the document if there is one, otherwise returns the root widget for the nearest enclosing document that has one.
nsAccessible::GetRelationByType needs to add NODE_CHILD_OF relations for document roots even when the document root has no widget.

nsDocAccessible's constructor will only set mWnd if the document has a widget. I think that's OK. I'm changing this here because nsIViewManager::GetWidget is going to go away.
Attachment #386178 - Flags: review? → review?(masayuki)
In some mochitests we set the zoom factor of a content iframe, so the iframe has a different appunits-per-dev-pixel value from its parent. When every document has a widget that's OK because we use the nearest widget to convert to screen coords, and that widget is always in the same document. But if there's no widget at the root, nsFrame::GetScreenRect goes wrong because it implicitly assumes that that value is the same all the way up the frame tree.
Attachment #386180 - Flags: review?(dbaron)
nsXULPopupManager saves mouse coordinates from an event and uses them later for another event. It assumes that "client coordinates" are interpreted relative to the same origin (the viewport top-left) as the top-left of the nearest widget for the document's root frame, which is no longer going to be true. So store these coordinates relative to the GetRootWidget().
Attachment #386182 - Flags: review?(enndeakin)
GetViewForRendering fails to return the toplevel document's root view, instead it returns the current document's root view. withinViewOffset is already correctly set to be relative to the toplevel document's root view. This is an unrelated drive-by bug fix. (Since only nsContentEventHandler uses eTopLevelWindowCoordinates, this could only have affected some IMEs in IFRAMEs.)
Attachment #386189 - Flags: review?(bzbarsky)
AdjustContextMenuKeyEvent sets the widget and coordinates of a mouse event to control where the context menu pops up. Currently it uses the document's widget. Instead, let's use the widget for the document at the top of the view hierarchy.
Attachment #386191 - Flags: review?(dbaron)
-- nsContentSink just needs any old widget. Use GetRootWidget.
-- nsEventStateManager has some debug-only code that's unused even in there.
-- nsGlobalChromeWindow::SetCursor needs to find a widget to set the cursor on. Use GetRootWidget.
-- I have no idea what nsWidgetUtils::UpdateFromEvent is for, but aWidget and aViewManager are always null, so remove them. mWidget is only used as the widget to construct a scroll event, and we dispatch to the correct view automatically (thanks to patch 1) so I think using GetRootWidget here is fine.
-- nsObjectFrame has some gnarly code for Windows to figure out which widget to return when a windowless plugin asks for one. I'm just hacking that code here to return the nearest document widget instead of the current document widget, but the good news is that the work in this bug will mean we can actually remove this code completely! This code chooses between returning the "nearest widget" and the "nearest document widget", and after this bug is fixed they will always be the same!
Attachment #386192 - Flags: review?(Olli.Pettay)
Attachment #386189 - Flags: review?(bzbarsky) → review+
-- nsIView::GetNearestWidget can just return null if it was unable to find a view with a widget in its ancestor chain. Calling GetWidget won't find anything; if the root view had a widget, we'd have found it already.
-- nsViewManager::DispatchEvent can just use GetRootWidget instead of GetWidget since it just wants to find a widget to check for transparency.
Attachment #386193 - Flags: review?(bzbarsky)
This is a bit more interesting. Fortunately docshell is pretty easy; we just need to avoid checking that its parent widget is non-null. Also, get pixel bounds from the content viewer instead of checking the widget of its view manager.

nsDocumentViewer's mWindow is now allowed to be null. We store explicit bounds in the docviewer since the widget might not be around. CreateDeviceContext takes a view parameter from which it can find a widget, instead of getting the widget directly. If someone calls SetBounds and there's no widget, we call nsIViewManager::SetWindowDimensions directly instead of having the widget do an NS_SIZE event. FindContainerView gets more complicated ... we find the container view by locating the frame for the document-embedding content and getting the view from it.
Attachment #386194 - Flags: review?(bzbarsky)
Attachment #386177 - Flags: review?(surkov.alexander)
At this point we've removed all uses of nsIViewManager::GetWidget, so get rid of it since anyone trying to use it is almost certainly broken.
Attachment #386195 - Flags: review?(bzbarsky)
Everything else has been building up to this. This is the patch that actually makes content IFRAMEs windowless.
Comment on attachment 386178 [details] [diff] [review]
Part 4: IME code should use GetRootWidget

I don't test these patches actually, but looks ok for me.
Attachment #386178 - Flags: review?(masayuki) → review+
This is the other hard bit. The main thing here is that nsIWidget::Scroll changes dramatically; it now takes a rectangle to be moved, and a list of child widgets to be moved, resized and clipped to given regions. So there are Windows, GTK2 and Mac implementations of the new API.

The GTK2 implementation is roughly described here:
http://weblogs.mozillazine.org/roc/archives/2009/06/stupid_x_tricks.html

The Windows implementation is quite different. On Windows we use SW_SCROLLCHILDREN if all children are moving. We shrink the clip regions of all changing children before we move them, then enlarge them afterward if necessary, so that bits of windows don't stick out further than they should.

The Mac implementation is easy and mostly just removes code that's no longer needed.

nsScrollPortView changes a fair bit. We make ViewPositionDidChange return a list of configuration changes (nsGfxScrollFrame asks the RootPresContext for the desired plugin widget changes). Those changes are passed on down to nsIWidget::Scroll(). We had Windows-only code that finds the largest rectangle in the scrollable area which should actually be scrolled (very useful when there is a fixed-pos header or footer bar which we don't really want to scroll); this is now enabled on all platforms since we have cross-platform ability to scroll an arbitrary rectangle. One subtle thing is that nsScrollPortView is now responsible for invalidating the area that has been newly scrolled into view, so there's a bit of code added ("// Compute the area that's being exposed ...")
Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

Guess I'll ask dbaron for review on the view/layout parts, Josh for Mac, Karl to GTK2, and Jim Mathies for Windows.
Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

Josh for the Mac changes...
Attachment #386202 - Flags: review?(joshmoz)
Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

Karl for GTK2
Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

Jim for Windows
Attachment #386202 - Flags: review?(jmathies)
I've published all the patches leading from mozilla-central up to and including this bug here:
http://hg.mozilla.org/users/rocallahan_mozilla.com/compositor-patches/
It must be worth to create try server build and test it with Windows screen
readers since they rely on HWNDs and if we are going to get rid most of HWNDs
then it might affect on screen readers. Marco, could you please perform testing
of these patches?

James, any thoughts how it can affect on NVDA?
(In reply to comment #13)
> Created an attachment (id=386177) [details]
> Part 3: fix accessibility to not require a widget per document
> 
> nsAccessible::GetRelationByType needs to add NODE_CHILD_OF relations for
> document roots even when the document root has no widget.

It might be not needed any more because now we provide node_child_of relation because accessible tree is changed by Windows, i.e. when we had widget with HWND then Windows creates accessible that is not linked with Gecko a11y tree correctly. Therefore we use this relation to "repair" the accessible tree.

James, could you comment please?
I published builds here:
https://build.mozilla.org/tryserver-builds/rocallahan@mozilla.com-try-f9bf8fa4178/
These aren't quite the latest patches but the only things I've fixed have been repainting bugs. MarcoZ apparently tried them and they worked OK.
I tested a build recently with both JAWS and Window-Eyes, as well as NVDA on Windows, and they all seem to be using the non-hwnd-dependent NAVRELATION_EMBEDS relation as documented here: http://www.mozilla.org/access/windows/at-apis#relation_embeds. I didn't notice any breackage with current versions of JAWS 10, WE 7.1 and NVDA.
Marco, are tests from test_relations.xul passed? There we have a test for node_child_of relation for windows.
roc, IMEs almost work fine on your tryserver build (I confirmed on Win32).

However, IMEs (both IMM32 and TSF) don't work fine only on the rich format editor of GMail. The composition string is committed immediately. Looks like that the editor of designMode=true doesn't work fine. But contentediable works fine for me.
roc:

I checked the log of nsIMM32Handler. By the log, query content events are failed on the designMode editors. Would you check the related code of the event handling?
(In reply to comment #36)
> Marco, are tests from test_relations.xul passed?

Yes. The builds pass all tests.

(In reply to comment #38)
> I checked the log of nsIMM32Handler. By the log, query content events are
> failed on the designMode editors. Would you check the related code of the event
> handling?

My guess is that we're dispatching the query content events at the top level document (since it's the only document with a widget), and nsContentEventHandler is only looking at that document. It needs to look at the document that has focus. Perhaps the right thing to do is to forward query content events to the focused element the same way we do for IME and key events, here?
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6099

It would be great if you could make your own build. Just apply the patches from http://hg.mozilla.org/users/rocallahan_mozilla.com/compositor-patches on top of changeset 7a8502b70fdf from mozilla-central.
(In reply to comment #39)
> (In reply to comment #36)
> > Marco, are tests from test_relations.xul passed?
> 
> Yes. The builds pass all tests.

So we still have HWND for window of iframe, but you got rid windows for scrollable areas, for example, do I understand right?
No. IFRAMEs do not have HWNDs. See "Part 14".
(In reply to comment #41)
> No. IFRAMEs do not have HWNDs. See "Part 14".

Does nsIWidget have HWND? If so do you have any idea why this works (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/test_relations.xul#66)
(In reply to comment #42)
> Does nsIWidget have HWND? If so do you have any idea why this works
> (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/test_relations.xul#66)

Yes, on Windows every nsIWidget has an HWND. But with my work in this bug, content IFRAMEs have no associated nsIWidget.

That test works because of my "Part 3" patch:
-          if (scrollFrame || view->GetWidget()) {
+          if (scrollFrame || view->GetWidget() || !frame->GetParent()) {
We provide a NODE_CHILD_OF relation for every IFRAME's nsDocAccessible, because nsDocAccessible::GetFrame() returns the document's root frame, and the root frame's GetParent() is null.
Depends on: 501618
Attachment #386192 - Flags: superreview+
Attachment #386192 - Flags: review?(Olli.Pettay)
Attachment #386192 - Flags: review+
(In reply to comment #43)
> (In reply to comment #42)
> > Does nsIWidget have HWND? If so do you have any idea why this works
> > (http://mxr.mozilla.org/mozilla-central/source/accessible/tests/mochitest/test_relations.xul#66)
> 
> Yes, on Windows every nsIWidget has an HWND. But with my work in this bug,
> content IFRAMEs have no associated nsIWidget.
> 
> That test works because of my "Part 3" patch:

Ok, I see. Node child of relation don't make sense for iframes any more I
think.

If I have few windows open (for example, Firefox and DOM Inspector) then do
they have own HWND's? If so then I think we should expose node_child_of
relation for them only.
(In reply to comment #44)
> Ok, I see. Node child of relation don't make sense for iframes any more I
> think.

OK. If it's OK with you, then I would prefer to not change that behaviour in this bug, just preserve it using my existing patch. If you want to remove those relations then you can do that in a separate bug.
(In reply to comment #45)
> (In reply to comment #44)
> > Ok, I see. Node child of relation don't make sense for iframes any more I
> > think.
> 
> OK. If it's OK with you, then I would prefer to not change that behaviour in
> this bug, just preserve it using my existing patch.

Could you upload new patch so that Marco could try?
Nothing has changed related to accessiblity since the builds I mentioned in comment #34.
This completely breaks NVDA for documents containing frames; the document renders as blank. I haven't quite worked out why this is the case yet...
(In reply to comment #44)
> Node child of relation don't make sense for iframes any more I
> think.
I can confirm that this isn't necessary any more in theory, as retrieving the parent of the sub-document now correctly returns its frame. However, NVDA currently depends on this relation and will break if it is removed. I'd be willing to consider changing this, but I think this is a separate issue.
(In reply to comment #49)
> (In reply to comment #44)
> > Node child of relation don't make sense for iframes any more I
> > think.
> I can confirm that this isn't necessary any more in theory, as retrieving the
> parent of the sub-document now correctly returns its frame. However, NVDA
> currently depends on this relation and will break if it is removed. I'd be
> willing to consider changing this, but I think this is a separate issue.

Ok, thanks, Jamie.

Robert please ignore that my comment then.
Okay. Some debugging reveals that AccessibleObjectFromEvent() always returns the accessible for the root document when requesting an accessible in a sub-document. For example, if a link in a sub-document gets focus, it fires an event specifying the hwnd of the document and the unique ID of the link. However, when AccessibleObjectFromEvent() is called with those parameters, it returns the accessible for the root document instead of the accessible for the link. This causes a great deal of nastiness.

Note that this only occurs for sub-documents. Documents without frames/iframes are fine, as might be expected.
(In reply to comment #39)
> My guess is that we're dispatching the query content events at the top level
> document (since it's the only document with a widget), and
> nsContentEventHandler is only looking at that document. It needs to look at the
> document that has focus. Perhaps the right thing to do is to forward query
> content events to the focused element the same way we do for IME and key
> events, here?
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6099

No, you need to change here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5914

> 5914   // key and IME events must be targeted at the presshell for the focused frame
> 5915   if (!sDontRetargetEvents &&
> 5916       (NS_IS_KEY_EVENT(aEvent) || NS_IS_IME_EVENT(aEvent) ||
> +           NS_IS_QUERY_CONTENT_EVENT(aEvent) || NS_IS_SELECTION_EVENT(aEvent) ||
> 5917        NS_IS_CONTEXT_MENU_KEY(aEvent))) {
> 5918     nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> 5919     if (!fm)
> 5920       return NS_ERROR_FAILURE;

This works fine. However, this means that we cannot use the content events in deactivated window. But it's ok for now if you don't have better fix.
(In reply to comment #51)
> Some debugging reveals that AccessibleObjectFromEvent() always returns
> the accessible for the root document when requesting an accessible in a
> sub-document.
Further details:
AccessibleObjectFromEvent() sends a WM_GETOBJECT message, which returns the root accessible for the window. Before this patch, the root accessible was the accessible for the sub-document, but now, it is the accessible for the top document (since there is only one window/hwnd).
AccessibleObjectFromEvent() then calls get_accChild() on this accessible, passing it the child ID received in the win event. This child ID is the unique ID from Gecko.
On the Gecko side, this calls NSDocAccessibleWrap::get_accChild(), which searches a cache for an accessible with the specified unique ID.
Unfortunately, with this patch, this gets called on the accessible for the top document. However, the unique ID supplied is for an accessible in a sub-document, so it will not be found in the top document's cache.
Fail!
Robert could you do one more try build including my patch?
Posted image screenshot (obsolete) —
On Mac, the position of IM's candidate window is wrong with Kotoeri.
(In reply to comment #57)
> Created an attachment (id=386534) [details]
> screenshot
> 
> On Mac, the position of IM's candidate window is wrong with Kotoeri.

Isn't it fixed by the code of comment 52?
Attachment #386179 - Flags: review?(enndeakin) → review+
Comment on attachment 386182 [details] [diff] [review]
Part 7: fix nsXULPopupManager's saved mouse coordinates

               // no widget, so just use the client point if available
+              // XXXroc when does this happen?

For popupshowing and other popup events, which use nsMouseEvent. 

>+              // XXX this doesn't handle IFRAMEs in transforms
>+              nsPoint offsetToRoot =
>+                presShell->FrameManager()->GetRootFrame()->GetOffsetTo(rootFrame);

I got a bit lost here because the term 'root' is used to refer to two different things. It's confusing that we're trying to getting the offset from GetRootFrame() to rootFrame. Can you use better names for each?
Comment on attachment 386534 [details]
screenshot

(In reply to comment #58)
> > On Mac, the position of IM's candidate window is wrong with Kotoeri.
> 
> Isn't it fixed by the code of comment 52?

Right. No problem with comment 52.
Attachment #386534 - Attachment is obsolete: true
(In reply to comment #60)
> Right. No problem with comment 52.

Great, thank you for your testing.
New try builds here:
https://build.mozilla.org/tryserver-builds/rocallahan@mozilla.com-try-be5734263a2f/
All pending review comments in this bug and bug 339548 have been addressed. I've added Alexander Surkov's patch. Latest patches uploaded here:
http://hg.mozilla.org/users/rocallahan_mozilla.com/compositor-patches/
I found a problem, even with the new try build.

Steps to reproduce:
1. Use Windows XP.
2. Go to Control Panel -> Date, Time, Language, and Regional Options -> Regional and Language Options -> Languages.
3. Enable Install files for East Asian languages in Supplemental language support, and click Apply.
4. Click Details in Text services and input languages (in the same tab as 2.).
5. Click Add in Installed services.
6. Select Japanese as Input language and select Microsoft Natural Input 2002 ver.8.1 as Keyboard layout/IME.
7. Click OK, OK, OK
8. Start Firefox.
9. Change intl.enable_tsf_support to true and restart Firefox.
10. Open data:text/html,<iframe src="data:text/html,<input>"></iframe>
11. Move focus to the <input>.
12. Choose JP (Japanese) as language in the Lnaguage Bar. (See http://support.microsoft.com/kb/306993 if you don't know)
13. Choose Hiragana as Input Mode in the Language Bar.
14. Press "a" key. Underlined "あ" will appear.
15. Press Tab key.

Actual result:
The focus moves, but "あ" is still underlined.

Expected result:
"あ" should be committed, i.e. not underlined.
Or, focus should not move at this time.
I'm not sure which behavior is correct.
TSF is not enabled by default yet due to bugs. I would prefer to not block on TSF issues.
This new try build crashes when I log into GMail (after login, when the Inbox should appear). This is with JAWS 10 running.
(In reply to comment #65)
> This new try build crashes when I log into GMail (after login, when the Inbox
> should appear). This is with JAWS 10 running.

It might be my bad, I'll debug.
(In reply to comment #66)
> (In reply to comment #65)
> > This new try build crashes when I log into GMail (after login, when the Inbox
> > should appear). This is with JAWS 10 running.
> 
> It might be my bad, I'll debug.

I don't get crash when my patch is applied only. Robert is there a easy way to apply all your patches and compile own build?
Yes. I'm not sure of the best way to do it, but an easy way would be something like this:

hg clone https://hg.mozilla.org/mozilla-central
hg clone http://hg.mozilla.org/users/rocallahan_mozilla.com/compositor-patches
cd mozilla-central
hg qinit
hg update -C -r 1c6ec6110a0f
cp ../compositor-patches/* .hg/patches
mkdir layout/base/tests/chrome
hg qpush -a

That mkdir is to work around a Mercurial bug that's affect the chromify-tests patch.

You could do this in an existing mozilla-central checkout if you're not already using MQ in it.

1c6ec6110a0f is the changeset that my patches currently apply cleanly to. That's trunk as of a couple of days ago.
(In reply to comment #64)
> TSF is not enabled by default yet due to bugs. I would prefer to not block on
> TSF issues.

Yes. And the issue will be gone by the next patch of bug 480708.
QA Contact: general
Attachment #386176 - Flags: review?(bzbarsky) → review+
Comment on attachment 386176 [details] [diff] [review]
Part 2: Create nsIViewManager::GetRootWidget

r=bzbarsky
Attachment #386195 - Flags: review?(bzbarsky) → review+
Comment on attachment 386195 [details] [diff] [review]
Part 13: remove nsIViewManager::GetWidget

r=bzbarsky, assuming the plan is to add callers of this new GetWidget.
Attachment #386193 - Flags: review?(bzbarsky) → review+
Comment on attachment 386193 [details] [diff] [review]
Part 11: remove nsViewManager calls to nsViewManager::GetWidget

r=bzbarsky, though the nsViewManager.cpp change here looks like it should be in an earlier patch on this bug or something.
Attachment #386196 - Flags: review?(bzbarsky) → review+
Comment on attachment 386194 [details] [diff] [review]
Part 12: Teach nsDocumentViewer and nsDocShell to deal with not having a widget

>+++ b/layout/base/nsDocumentViewer.cpp
> DocumentViewerImpl::InitInternal(nsIWidget* aParentWidget,
>   if (aDoCreation) {
>+    nsresult rv = CreateDeviceContext(containerView);

This looks like it adds an extra call to CreateDeviceContext that we didn't use to have before when coming from SetPageMode.  Is that desirable?  I guess it's ok at the very least; CreateDeviceContext will just create a new one and be done with it...

Also, the documentation for InitInternal in the class decl needs fixing, looks like.

> DocumentViewerImpl::FindContainerView()

I really wish we had a better way to do what you need here in the !mParentWidget && mContainer case...

Maybe we could just improve the relevant APIs to stop shoehorning everything into nsIBaseWindow and just pass in the parent view directly...  We can worry about this later.  Please do file a followup bug to clean this up, though?

>+    nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mContainer));

Might as well query into an nsIDocShellTreeItem here.  You need one later on anyway, and you never need an nsIDocShell for mContainer.

>+DocumentViewerImpl::CreateDeviceContext(nsIView* aContainerView)
>   if (doc) {
>-    NS_ASSERTION(!aWidget, "Shouldn't have a widget here");

Should still be able to assert !aContainerView, no?

>+++ b/widget/public/nsIBaseWindow.idl
>+	It is possible to pass null for both parentNativeWindow and parentWidget,
>+	but only docshells support this.

Yeah, I was going to make nsWebBrowser do that too.... ;)

r=bzbarsky with those nits picked.
Attachment #386194 - Flags: review?(bzbarsky) → review+
Yes to all those comments.
It doesn't crash and read gmail.com content for me.
Attachment #386465 - Attachment is obsolete: true
Attachment #386865 - Flags: superreview?(roc)
Attachment #386865 - Flags: review?(marco.zehe)
Attachment #386865 - Flags: review?(bolterbugz)
Comment on attachment 386865 [details] [diff] [review]
part 3.2 fix accessibility (should be applied with part 3, version 2)

sorry for the spam, this version works incorrectly, I tested wrong build.
Attachment #386865 - Attachment is obsolete: true
Attachment #386865 - Flags: superreview?(roc)
Attachment #386865 - Flags: review?(marco.zehe)
Attachment #386865 - Flags: review?(bolterbugz)
On Mac OS X 10.5.7, crashes several times a day.
Though I don't yet find reliable steps to reproduce.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000048
0x02860fe1 in nsCOMPtr<nsIWidget>::get (this=0x48) at nsCOMPtr.h:777
777	          return reinterpret_cast<T*>(mRawPtr);
(gdb) bt
#0  0x02860fe1 in nsCOMPtr<nsIWidget>::get (this=0x48) at nsCOMPtr.h:777
#1  0x02860ff7 in nsCOMPtr<nsIWidget>::operator nsIWidget* (this=0x48) at nsCOMPtr.h:790
#2  0x02b8c5d2 in nsPluginInstanceOwner::FixUpPluginWindow (this=0x0, inPaintState=1) at /builds/mozilla-central/mozilla/layout/generic/nsObjectFrame.cpp:4972
#3  0x02b8c9fa in nsObjectFrame::DidSetWidgetGeometry (this=0x249dc17c) at /builds/mozilla-central/mozilla/layout/generic/nsObjectFrame.cpp:1189
#4  0x02b0e60f in PluginDidSetGeometryEnumerator (aEntry=0x1e7e1380, userArg=0x0) at /builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:2214
#5  0x02b14763 in nsTHashtable<nsPtrHashKey<nsObjectFrame> >::s_EnumStub (table=0x5e60a40, entry=0x1e7e1380, number=0, arg=0xbfffd858) at nsTHashtable.h:420
#6  0x035b1c03 in PL_DHashTableEnumerate (table=0x5e60a40, etor=0x2b14746 <nsTHashtable<nsPtrHashKey<nsObjectFrame> >::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*)>, arg=0xbfffd858) at pldhash.c:754
#7  0x02b13c0d in nsTHashtable<nsPtrHashKey<nsObjectFrame> >::EnumerateEntries (this=0x5e60a40, enumFunc=0x2b0e5f0 <PluginDidSetGeometryEnumerator>, userArg=0x0) at nsTHashtable.h:241
#8  0x02b0eb34 in nsRootPresContext::DidApplyPluginGeometryUpdates (this=0x5e60800) at /builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:2221
#9  0x02b134ac in nsRootPresContext::UpdatePluginGeometry (this=0x5e60800, aChangedRoot=0x5e623e0) at /builds/mozilla-central/mozilla/layout/base/nsPresContext.cpp:2207
#10 0x02acfc80 in nsCSSFrameConstructor::ProcessRestyledFrames (this=0x1e7e3d10, aChangeList=@0xbfffd978) at /builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp:7797
#11 0x02ad01c6 in nsCSSFrameConstructor::ProcessOneRestyle (this=0x1e7e3d10, aContent=0x1e9ae9e0, aRestyleHint=0, aChangeHint=nsChangeHint_RepaintFrame) at /builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp:11617
#12 0x02ad1862 in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x1e7e3d10) at /builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp:11718
#13 0x02b21c0f in PresShell::FlushPendingNotifications (this=0x5e68800, aType=Flush_Style) at /builds/mozilla-central/mozilla/layout/base/nsPresShell.cpp:4833
#14 0x02abfed8 in nsCSSFrameConstructor::RestyleEvent::Run (this=0x22944570) at /builds/mozilla-central/mozilla/layout/base/nsCSSFrameConstructor.cpp:11802
#15 0x035f9a07 in nsThread::ProcessNextEvent (this=0x501d80, mayWait=0, result=0xbfffe17c) at /builds/mozilla-central/mozilla/xpcom/threads/nsThread.cpp:527
#16 0x035b8cc7 in NS_ProcessPendingEvents_P (thread=0x501d80, timeout=20) at nsThreadUtils.cpp:180
#17 0x0355c0fb in nsBaseAppShell::NativeEventCallback (this=0x533c20) at /builds/mozilla-central/mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:121
#18 0x03520bee in nsAppShell::ProcessGeckoEvents (aInfo=0x533c20) at /builds/mozilla-central/mozilla/widget/src/cocoa/nsAppShell.mm:413
#19 0x92b1c595 in CFRunLoopRunSpecific ()
#20 0x92b1cc78 in CFRunLoopRunInMode ()
#21 0x96a6028c in RunCurrentEventLoopInMode ()
#22 0x96a5ffde in ReceiveNextEventCommon ()
#23 0x96a5ff19 in BlockUntilNextEventMatchingListInMode ()
#24 0x939fed0d in _DPSNextEvent ()
#25 0x939fe5c0 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#26 0x939f75fb in -[NSApplication run] ()
#27 0x0351f02d in nsAppShell::Run (this=0x533c20) at /builds/mozilla-central/mozilla/widget/src/cocoa/nsAppShell.mm:766
#28 0x03352c9a in nsAppStartup::Run (this=0x552d80) at /builds/mozilla-central/mozilla/toolkit/components/startup/src/nsAppStartup.cpp:193
#29 0x02852d36 in XRE_main (argc=1, argv=0xbffff78c, aAppData=0x50ed70) at /builds/mozilla-central/mozilla/toolkit/xre/nsAppRunner.cpp:3386
#30 0x00002489 in main (argc=1, argv=0xbffff78c) at /builds/mozilla-central/mozilla/browser/app/nsBrowserApp.cpp:156
Steps to reproduce for previous comment:
1. Go to http://b.hatena.ne.jp/entry/www.nicovideo.jp/watch/sm101534
2. Go to another page in the same tab.
3. Click Back button and wait for few seconds.
4. Click Forward button and wait for few seconds.
5. Repeat 3. and 4. several times.
Attachment #386202 - Flags: review?(joshmoz) → review+
Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

I only looked at the mac code, looks good to me. Nice to see nsChildView::Scroll cleaned up.
(In reply to comment #63)
> I found a problem, even with the new try build.
> 
> Steps to reproduce:
> 1. Use Windows XP.

The same problem can be reproduced on Windows 7 RC + Microsoft IME if TSF is enabled.
(In reply to comment #77)
> On Mac OS X 10.5.7, crashes several times a day.

> #2  0x02b8c5d2 in nsPluginInstanceOwner::FixUpPluginWindow (this=0x0,
> inPaintState=1) at
> /builds/mozilla-central/mozilla/layout/generic/nsObjectFrame.cpp:4972
> #3  0x02b8c9fa in nsObjectFrame::DidSetWidgetGeometry (this=0x249dc17c) at
> /builds/mozilla-central/mozilla/layout/generic/nsObjectFrame.cpp:1189

I guess Part 10.5 of bug 339548 triggers this crash.
Ok, it makes gmail.com to work with JAWS (look for accessible through all cached documents). Though I really don't understand deeply all related code. So I haven't idea why previous version of the patch don't work (where I search through the document and its subdocuments caches). Also we have some a11y related code in Windows.cpp and I'm not sure if we should change something there. I'll put the link on try build a bit later.
This latest try server build no longer crashes with JAWS 10 running and logging into Gmail.

But NVDA is still completely broken in GMail. The virtual buffer is blank, there is no content at all. On pages without iframes everything's fine. But with this, we're completely breaking NVDA in GMail and other places.
Comment on attachment 386202 [details] [diff] [review]
Part 15: make scrollframes windowless

+     * be performed simultaneously with the scrolling, as far as possible,658

Accidental paste, it seems.

   // Note that child widgets may be scrolled by the native widget scrolling,
   // so don't update their positions

Does this comment need updating?

+     * Scroll this widget and (as simultaneously as possible) modify
+     * child widgets.

"Scroll a rectangle in this widget"?
"modify the specified child widgets"?

+    GdkDrawable* drawable = GDK_DRAWABLE(mDrawingarea->inner_window);
+    GdkGC* gc = gdk_gc_new(drawable);
+    gdk_draw_drawable(drawable, gc, drawable,
+                      aSource.x, aSource.y,
+                      aSource.x + aDelta.x, aSource.y + aDelta.y,
+                      aSource.width, aSource.height);
+    g_object_unref(gc);

Don't you want graphics_exposures set here?
Xlib GCs default graphics_exposures to True but GdkGCs default to False.

Have you considered gdk_window_move_region here?
The main advantage would be that it ensures that expose events not yet
processed get translated appropriately.
It also does the necessary adjustment of the update area.
It invalidates the portion of the source not in the destination
(which we do anyway) but that doesn't look like it'll be a problem.

+    nsIntRect affectedArea;
+    affectedArea.UnionRect(aSource, aSource + aDelta);

This is not used.

+        if (configuration->mClipRegion.IsEmpty() ||
+            configuration->mBounds != w->mBounds) {
+            w->Show(PR_FALSE);

Setting an empty clip region would seem less invasive then effecting an unmap.
I wouldn't know for sure without trying, but it might also mean that the code
to set the configurations could be shared with nsWindow::ConfigureChildren.
(In reply to comment #85)
>    // Note that child widgets may be scrolled by the native widget scrolling,
>    // so don't update their positions
> 
> Does this comment need updating?

I just removed it.

> +     * Scroll this widget and (as simultaneously as possible) modify
> +     * child widgets.
> 
> "Scroll a rectangle in this widget"?
> "modify the specified child widgets"?

OK

> Setting an empty clip region would seem less invasive then effecting an unmap.
> I wouldn't know for sure without trying, but it might also mean that the code
> to set the configurations could be shared with nsWindow::ConfigureChildren.

Using gdk_window_move_region and setting an empty clip region to hide the widgets causes major plugin flickering when scrolling. I'll try to figure out why.
(In reply to comment #86)
> Using gdk_window_move_region and setting an empty clip region to hide the
> widgets causes major plugin flickering when scrolling. I'll try to figure out
> why.

It appears that setting the clip region to empty causes a gray area to be eagerly painted where the plugin was. Using gdk_window_move_region is probably going to be OK.
I hope this is clearer
Attachment #386182 - Attachment is obsolete: true
Attachment #388193 - Flags: review?(enndeakin)
Attachment #386182 - Flags: review?(enndeakin)
Karl's comments addressed.
Attachment #386202 - Attachment is obsolete: true
Attachment #388196 - Flags: review?(mozbugz)
Attachment #386202 - Flags: review?(mozbugz)
Attachment #386202 - Flags: review?(jmathies)
Attachment #386202 - Flags: review?(dbaron)
Attachment #388193 - Flags: review?(enndeakin) → review+
These try-server builds exhibit the same problem with NVDA as the previous ones: The virtual buffer still stays blank. JAWS continues to work fine.
Comment on attachment 388196 [details] [diff] [review]
Part 15 v2: make scrollframes windowless

The win32 scrolling code looks good. Running this for the weekend didn't turn anything up.

It'd be great if we could work out any TSF issues as we really wanted to get that enabled by default for better win7 touch input support.
Attachment #388196 - Flags: review?(jmathies) → review+
The new tryserver build does not crash with Comment #78.
Thanks for fixing it.
(In reply to comment #91)
> These try-server builds exhibit the same problem with NVDA as the previous
> ones: The virtual buffer still stays blank. JAWS continues to work fine.

Right, I used the same patch from Alexander as last time. We need a new patch from Alexander to fix NVDA. (Unless we're triggering an NVDA issue now?)
(In reply to comment #91)
> These try-server builds exhibit the same problem with NVDA as the previous
> ones: The virtual buffer still stays blank.
Unfortunately, it seems there is another problem. :( IAccessible2::get_windowHandle is failing on sub-documents, returning 0 instead of the actual window handle. NVDA needs this to render buffers correctly.
(In reply to comment #95)
> IAccessible2::get_windowHandle is failing on sub-documents, returning 0 instead
> of the actual window handle.

One of the major goals of the work here is stop sub-documents from having actual native windows. So instead of 0, should we be returning the HWND of the nearest enclosing document that actually has a native window?
(In reply to comment #96)
> (In reply to comment #95)
> > IAccessible2::get_windowHandle is failing on sub-documents, returning 0 instead
> > of the actual window handle.
> 
> One of the major goals of the work here is stop sub-documents from having
> actual native windows. So instead of 0, should we be returning the HWND of the
> nearest enclosing document that actually has a native window?

That should be right I think.
Assuming that's what you want, I have a fix and I'll make try-server builds later today.
(In reply to comment #96)
> One of the major goals of the work here is stop sub-documents from having
> actual native windows.
Understood.
> So instead of 0, should we be returning the HWND of the
> nearest enclosing document that actually has a native window?
Yes please. :) As I understand it, get_windowHandle will then return the same hwnd for the top document and sub-documents.
The Windows builds haven't appeared as of 1:15 AM PDT. Did the build fail?
I can confirm that this try-server build works with both JAWS and NVDA in GMail.
Also confirmed with this try-server build that Window-Eyes, SuperNova and System Access still work. All's good on supporting assistive technologies on Windows!
(In reply to comment #104)
> Also confirmed with this try-server build that Window-Eyes, SuperNova and
> System Access still work. All's good on supporting assistive technologies on
> Windows!
Sorry, I can confirm that NVDA now shows the content of frames and iframes. However, there is yet another issue with this try build and NVDA:
NVDA no longer will pick up new document loads in frames and iframes. E.g. http://xahlee.org/js/frame/0.html

Clicking  'b' or 'c' links in the first (left) frame load a new document in that frame, yet NVDA does not reflect this in its virtualBuffer. This works with a standard nightly of Firefox, but not this try build. 
I have also got Marco to confirm this.
Jamie and I will try and provide some more info on this tomorrow as we investigate why NVDA is failing in this situation.
(In reply to comment #105)
> NVDA no longer will pick up new document loads in frames and iframes.
This problem is caused by a check in our code relating to a work around for bug 420845. Because a reorder event is not fired on sub-frames when a new document is loaded into them, we have to use the state change on the new document. Normally, we ignore events for nodes which don't exist in our buffer (which is the case for this new document). However, due to bug 420845, we have a special exception for a state change on a node which has a different window handle to the root document. This check relies on an assumption (different window handle for sub-documents) which is changed by these patches, hence the breakage.

I think I have a solution on our side. However, bug 420845 probably should still be fixed at some point.
James, if we'll fix the bug 420845 then will current NVDA version work? It's worth not to force NVDA users to upgrade NVDA once they upgraded firefox.
(In reply to comment #107)
> James, if we'll fix the bug 420845 then will current NVDA version work?
Yes. Note that I've worked around the issue in our code now anyway, but that obviously won't affect people running earlier releases of NVDA.
Attachment #388196 - Flags: review?(mozbugz) → review+
Comment on attachment 388196 [details] [diff] [review]
Part 15 v2: make scrollframes windowless

r+=me on the GTK changes.

try-cc17fe5d2d61 seemed to scroll plugins nicely on my system with and without kompmgr running.

We'll need this soon as guffaw scrolling has been removed from git GTK+ with the landing of client-side windows (probably because child windows with real X windows are expected to be rare):
http://git.gnome.org/cgit/gtk+/commit/?id=eabac453e652d5aa2e535d957057f9c84803eea9
Robert, could you publish new try build updated to trunk because bug 420845 was fixed?
Updated builds here:
https://build.mozilla.org/tryserver-builds/rocallahan@mozilla.com-try-3021cd75023e/
These are updated to today's trunk and contain the fix for bug 420845. They also contain the latest patches for bug 339548 and bug 505184.
With that build I noticed that scrolling in the awesomebar dropdown doesn't work correctly on Mac. The newly-visible parts are drawn correctly but the rest of the content isn't moved.
This bug also exists in the build from comment 34.
This latest try server build is fine with both JAWS and NVDA in both pages and framesets. The example Mick and Jamie mentioned also works fine with the same build of NVDA that a few days ago would not pick up the changes.

From my end this is good!
Robert, please upload new a11y patch with your additional HWND fix from comment #98.
Attachment #387602 - Flags: superreview?(roc)
Attachment #387602 - Flags: review?(bolterbugz)
Attachment #387602 - Flags: review?(bolterbugz) → review+
Comment on attachment 387602 [details] [diff] [review]
part 3.2 fix accessibility (should be applied with part 3, version 3) 

r=me thanks (I didn't try the patch out). Some minor things:

> {
>   *aXPAccessible = nsnull;
>-  if (!mWeakShell)
>-    return; // This document has been shut down

>+  if (IsDefunct())
>     return;

Note we don't currently check mWeakShell in IsDefunct but we do check mDocument which should be sufficient? (I don't have time to check this right now)

> 
>-STDMETHODIMP nsDocAccessibleWrap::get_accChild( 
>-      /* [in] */ VARIANT varChild,
>-      /* [retval][out] */ IDispatch __RPC_FAR *__RPC_FAR *ppdispChild)
>+STDMETHODIMP
>+nsDocAccessibleWrap::get_accChild(VARIANT aVarChild,
>+                                  IDispatch __RPC_FAR *__RPC_FAR *aPPDispChild)

nit: I think the name ppdispChild is more recognizable for COM stuff, and I'm not sure about capitalizing the hungarian notation. My preference would be to leave the name as is here.

>+
>+struct nsSearchAccessibleInCacheArg
>+{
>+  nsCOMPtr<nsIAccessNode> mAccessNode;
>+  void *mUniqueID;
>+};
>+
>+static PLDHashOperator
>+SearchAccessibleInCache(const void* aKey, nsIAccessNode* aAccessNode,
>+                        void* aUserArg)
>+{
>+  nsCOMPtr<nsIAccessibleDocument> docAccessible(do_QueryInterface(aAccessNode));
>+  NS_ASSERTION(docAccessible,
>+               "No doc accessible for the object in doc accessible cache!");

When can this happen? If never, fine :)  If there are known cases we should invalidate the cache (and possibly bail out).


>+
>+  // nsDocAccessibleWrap
>+
>+  /**
>+   * Search the accessible by the given child ID though cached documents.

How about: "Find the document accessible that contains the given child ID." ?

>+  static void GetXPAccessibleForChildID(const VARIANT& aVarChild,

The pldhash stuff looks right but I've not used it before.
Attachment #386177 - Attachment is obsolete: true
Attachment #389544 - Flags: review?(surkov.alexander)
Attachment #386177 - Flags: review?(surkov.alexander)
Attachment #387602 - Flags: superreview?(roc) → superreview+
Need to implement ::Scroll on nsCocoaWindow, forwarding to mPopupContentView. This fixes the awesomebar scrolling bug that Markus mentioned.
Attachment #388196 - Attachment is obsolete: true
Attachment #389557 - Flags: review?(joshmoz)
Attachment #388196 - Flags: review?(dbaron)
Comment on attachment 389557 [details] [diff] [review]
Part 15 v3: fix Mac scrolling issue

Still need dbaron review on the layout/view changes
Comment on attachment 389544 [details] [diff] [review]
Part 3 v2: Accessibility fixes

r=me
Attachment #389544 - Flags: review?(surkov.alexander) → review+
(In reply to comment #115)
> (From update of attachment 387602 [details] [diff] [review])
> r=me thanks (I didn't try the patch out). Some minor things:
> 
> > {
> >   *aXPAccessible = nsnull;
> >-  if (!mWeakShell)
> >-    return; // This document has been shut down
> 
> >+  if (IsDefunct())
> >     return;

> Note we don't currently check mWeakShell in IsDefunct but we do check mDocument
> which should be sufficient? (I don't have time to check this right now)

nsDocAccessible::IsDefunct() calls IsDefunct from base class, so we check mWeakShell.

> nit: I think the name ppdispChild is more recognizable for COM stuff, and I'm
> not sure about capitalizing the hungarian notation. My preference would be to
> leave the name as is here.
 
ok

> >+  nsCOMPtr<nsIAccessibleDocument> docAccessible(do_QueryInterface(aAccessNode));
> >+  NS_ASSERTION(docAccessible,
> >+               "No doc accessible for the object in doc accessible cache!");
> 
> When can this happen? If never, fine :)  If there are known cases we should
> invalidate the cache (and possibly bail out).

never I think

> >+   * Search the accessible by the given child ID though cached documents.
> 
> How about: "Find the document accessible that contains the given child ID." ?

It's not correct, I changed on "Find an accessible by the given child ID in cached documents."
Comment on attachment 389557 [details] [diff] [review]
Part 15 v3: fix Mac scrolling issue

>diff --git a/widget/src/cocoa/nsCocoaWindow.mm b/widget/src/cocoa/nsCocoaWindow.mm
>--- a/widget/src/cocoa/nsCocoaWindow.mm
>+++ b/widget/src/cocoa/nsCocoaWindow.mm
>@@ -817,21 +817,30 @@ NS_IMETHODIMP nsCocoaWindow::Show(PRBool
>   return NS_OK;
> 
>   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
> }
> 
> nsresult
> nsCocoaWindow::ConfigureChildren(const nsTArray<Configuration>& aConfigurations)
> {
>-  for (PRUint32 i = 0; i < aConfigurations.Length(); ++i) {
>-    nsChildView::ApplyConfiguration(this, aConfigurations[i], PR_TRUE);
>+  if (mPopupContentView) {
>+    mPopupContentView->ConfigureChildren(aConfigurations);
>   }
>   return NS_OK;
>-} 
>+}

I don't fully understand this change, can you explain?
(In reply to comment #121)
> I don't fully understand this change, can you explain?

nsCocoaWindow can't have direct plugin children, but it can have a mPopupContentView and we have to delegate scroll and ConfigureChildren calls to it.
Attachment #386180 - Flags: review?(dbaron) → review+
Comment on attachment 386180 [details] [diff] [review]
Part 6: fix GetScreenRect to handle appunits-per-dev-pixel changes across document boundaries where there's no widget

r=dbaron.

I suppose you'll have to rewrite nsLayoutUtils::GetCrossDocParentFrame when you do view removal.

We probably want to have some automated tests for this (the things that use this function, in cases where the inner document is zoomed... which I presume is what can cause this case), though.
Comment on attachment 386191 [details] [diff] [review]
Part 9: fix context menu event coordinates

>+  mPresContext->RootPresContext()->PresShell()->GetViewManager()->aEvent
>+    GetRootWidget(getter_AddRefs(aEvent->widget));

r=dbaron asuming the "aEvent" at the end of the first line above is extraneous, and that you've actually tested this patch without that.

(Do we have tests that cover this?)
Attachment #386191 - Flags: review?(dbaron) → review+
(In reply to comment #124)
> We probably want to have some automated tests for this (the things that use
> this function, in cases where the inner document is zoomed... which I presume
> is what can cause this case), though.

Some tests trigger this, actually, that's how I found it. Right now at 6am I don't remember which ones

(In reply to comment #125)
> r=dbaron asuming the "aEvent" at the end of the first line above is extraneous,
> and that you've actually tested this patch without that.
> 
> (Do we have tests that cover this?)

Yes, we do.
Attachment #389557 - Flags: review?(joshmoz) → review+
Comment on attachment 389557 [details] [diff] [review]
Part 15 v3: fix Mac scrolling issue

Please document in nsIScrollPositionListener.h that aConfigurations is
an array to be appended to by the implementation.  (Is there a good
reason it's a pointer rather than a reference, both here and in your
patch named "plugin-shaping"?)

Is there a useful way to share the code in nsTreeBodyFrame?

Why is passing an empty configurations array OK in nsTreeBodyFrame?  Is
it because things in trees never claim to be opaque?  Could you document
the reason?

In nsScrollPortView::Scroll... is the second parameter in the call to
AdjustChildWidgets still right in general?  The case you removed
(!scrollWidget) had a different one that looked a little more general to
me than the one in the case you're keeping, but I'm really not sure.

>+      nsIntRect destScroll = (biggestRect + nearestWidgetOffset).ToInsidePixels(aP2A);

Why is this ToInsidePixels rather than ToNearestPixels?  Couldn't this 
result in repainting fringes around things that you're blitting?


I didn't look too closely at the widget changes; I assume the other
reviewers looked at them better than I could.

r=dbaron
Attachment #389557 - Flags: review?(dbaron) → review+
(In reply to comment #127)
> (From update of attachment 389557 [details] [diff] [review])
> Please document in nsIScrollPositionListener.h that aConfigurations is
> an array to be appended to by the implementation.  (Is there a good
> reason it's a pointer rather than a reference, both here and in your
> patch named "plugin-shaping"?)

No, other than I prefer pointers for out or in-out parameters because it's more obvious in the callers that the parameter can be modified.

> Is there a useful way to share the code in nsTreeBodyFrame?

Yes, I'll do that.

> Why is passing an empty configurations array OK in nsTreeBodyFrame?  Is
> it because things in trees never claim to be opaque?  Could you document
> the reason?

It's because a plugin would never have a tree widget as its parent widget. I'll document that.

> In nsScrollPortView::Scroll... is the second parameter in the call to
> AdjustChildWidgets still right in general?  The case you removed
> (!scrollWidget) had a different one that looked a little more general to
> me than the one in the case you're keeping, but I'm really not sure.

We're passing the same value: the offset result from GetNearestWidget. That's what we want.

> >+      nsIntRect destScroll = (biggestRect + nearestWidgetOffset).ToInsidePixels(aP2A);
> 
> Why is this ToInsidePixels rather than ToNearestPixels?  Couldn't this 
> result in repainting fringes around things that you're blitting?

Yes it could. Generally scrolls should be pixel-aligned, hopefully. But when they're not, I don't want to scroll something that could include anti-aliased fragments of something that's not moving.
> > Is there a useful way to share the code in nsTreeBodyFrame?
> 
> Yes, I'll do that.

Actually there isn't a great way to do that, since the horizontal and vertical scrolling cases require different code; unifying them would require more complex code involving regions. So I'll pass on that.
I landed everything. Things are mostly good except we seem to have a Tdhtml regression on Linux. I filed bug 505665 for that instead of backing out because a) backing all this out will be extremely painful b) it would put us at schedule risk c) Tdhtml is unreliable d) only Linux affected and e) we can now get rid of mozdrawingarea on GTK which will probably win back.
Depends on: 505743
I've walked through the entire dependency tree and marked bugs as fixed where possible. Remaining are the following:
bug 215055 - fixed.
  bug 470358 - unable to reproduce as stated (Linux? The testcase to me appears to be the same as bug 215055, which is fixed.)
    bug 444238 - unable to reproduce: original source doesn't exist anymore.
  bug 333994 - unable to reproduce: testcase depends on original source which doesn't exist anymore.
bug 242917 - unable to reproduce (mac?)
bug 266582 - unable to reproduce (linux xft only) + original source gone.
bug 303036 - unable to reproduce (no printer)
bug 337801 + entire tree - old performance tracker dependency mess (bz kept bug 118933 open a year ago; might be really wfm now?) + some other bugs not expected to be fixed
bug 361354 + tree - other bugs not expected to be fixed
bug 443766 - hard to reproduce; possibly not fixed?
bug 449734 - not expected to be fixed
bug 469007 - unable to reproduce (mac)
Depends on: 506304
Depends on: compositor
Could this patch be causing Bug 507222 -  Thinkpad Trackpoint middle button mouse scroll does not scroll window up and down ?  Bustage happened in the nightly after this was landed, but this patch is way to complex for me to understand.

Mike Kaply might know more about the details how the Trackpoint driver works, since he said he talked to the developers about getting rid of some Mozilla hacks in  Bug 253115 .
Bug 507222 seems to have been caused bug 506615 (backed out now).
Sorry, scratch that. Anyway, I'll deal with this in bug 507222.
Depends on: 509329
Depends on: 511883
Blocks: 244371
No longer blocks: 244371
Depends on: 513684
Depends on: 514026
Depends on: 513254
Depends on: 517772
No longer depends on: 517772
Depends on: 526055
Depends on: 559076
Depends on: 481737
Blocks: 263160
Landed before 1.9.2a1 (based on bug 507222).
Target Milestone: --- → mozilla1.9.2
You need to log in before you can comment on or make changes to this bug.