Closed Bug 805746 Opened 12 years ago Closed 12 years ago

[BrowserAPI] mozbrowserscroll events not always fired at endpoint of scroll

Categories

(Firefox OS Graveyard :: General, defect, P1)

Product:
Firefox OS Graveyard
Component:
General
Platform:
x86_64
Linux
Type:
defect

Tracking

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Status:
RESOLVED FIXED
Milestone:
B2G C3 (12dec-1jan)
Project Flags:
blocking-basecamp +
Tracking Flags:
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: dhylands, Assigned: bechen)

References

Details

(Keywords: otoro, unagi)

Attachments

(1 file, 11 obsolete files)

Custom domevent v5
33.29 KB, patch
bechen
: review+
Details | Diff | Splinter Review 
I saw this a couple times today, but I don't know how to reproduce it.

Symptom: The URL/Search bar (normally at the top of the Browser window) sometimes disappears. When this happens, it's impossible to browse to any other pages. You can use the Back button to move around but can't get to the list of recent pages, or enter a new URL.
Figured it out at least on Unagi 10/31 build.

## Environment :
Unagi phone, build 20121031073004
2012-10-31 10:23:10
  
1) go to people.mozilla.com/~nhirata/html_tp/formsninput.html
2) click in a text field
3) click on a white space to dismiss the keyboard

Not sure if it's the same thing that Dave saw, but it causes blurriness in the text and the URL bar to disappear.  Noming because it causes brokeness.
blocking-basecamp: --- → ?
Summary: URL Bar sometimes disappears in Browser → URL Bar can disappears in Browser
Bleh on proper grammar.
Summary: URL Bar can disappears in Browser → URL Bar can disappear from the Browser
I see the blurry text as described above, but the URL bar seems to stick around (sometimes it scrolls up if you scroll towards the bottom of the page), but it seems to come back.
Hrm. Corrections to steps:
  
1) go to people.mozilla.com/~nhirata/html_tp/
2) select formsninput.html
3) zoom in
4) click in a text area field (very last field)
5) click on a white space to dismiss the keyboard
see duplicate for another test page.
Probably a dupe of bug #799402 or maybe bug #799401.  Naoki, can you dupe to the appropriate bug?
blocking-basecamp: ? → +
This sounds like your bug Ben
Component: General → Gaia::Browser
I can't reproduce this with either of the pages mentioned above, but I have experienced difficulties getting the address bar to re-appear before.

I'm guessing that what might be happening here is that there are certain circumstances under which mozbrowserscroll events are not fired on the mozbrowseriframe with a high enough resolution to fall within the threshold required to trigger the "show address bar" logic.

Scroll events are obviously not fired for every single pixel of scroll, so what are the heuristics which determine how many scroll events to send and at what intervals? Is there some kind of timeout or cut-off which results in scroll events not getting fired under some conditions - perhaps if CPU usage is high?

If the problem is that the events are not getting fired by the platform I'm not sure if this can be fixed on the Gaia side without a UX design change, but I will try to investigate further to make sure there isn't a bug in the browser app.
We /should/ fire a scroll event for the endpoint of a scroll.  If we're not doing that, I'd call it a bug.
Roc, do you know re comment 10?
Flags: needinfo?(roc)
Roc, do you know re comment 10?
smaug tells me that scroll events might not even be sufficient for what we want; we may need to listen to scrollareachanged as well.
Right. There is no particular throttling of onscroll events. However, when you're panning via OMTC and AsyncPanZoomController you may not be doing actual scrolling as Gecko defines it.
Flags: needinfo?(roc)
So what's the right thing to do here?

I guess instead of getting general-purpose scroll notifications, perhaps we want to get a notification when scrollTop becomes < x.  Would that be feasible on the platform side of things?
The fix for this bug has been called out as possibly having greater than normal risk to non-B2G platforms. Moving into the C2 milestone. We should try to resolve as soon as possible, to prevent the need for branching B2G from mozilla-beta earlier than planned.
tracking-firefox18: --- → ?
Target Milestone: --- → B2G C2 (20nov-10dec)
Assignee: nobody → ben
This bug has been called out as likely having risk to non-B2G platforms. Given that, marking as P1, and moving into the C2 milestone. We should prioritize this landing to mozilla-beta as soon as possible, to prevent late-breaking regressions to other platforms.
Priority: -- → P1
(In reply to Justin Lebar [:jlebar] from comment #15)
> So what's the right thing to do here?
> 
> I guess instead of getting general-purpose scroll notifications, perhaps we
> want to get a notification when scrollTop becomes < x.  Would that be
> feasible on the platform side of things?

Maybe whatever logic triggers showing of the URL bar also needs to be able to be triggered by AsyncPanZoomController panning?
> Maybe whatever logic triggers showing of the URL bar also needs to be able to be triggered by 
> AsyncPanZoomController panning?

AIUI (and I don't understand it well) the panning is controlled by dom/browser-element/BrowserElementScrolling.js, which ends up setting scroll{Top,Left}.  It strikes me as strange that that shouldn't get us the events we want.
I think it's true that setting scrollLeft (and having it actually change --- it's clamped based on the size of the scrollport and the scrolled content, and it's also rounded so may fail to register very small changes) will eventually result in the dispatch of a scroll event.
It sounds like this needs a platform fix and may not require any Gaia changes. Please comment if otherwise.
Assignee: ben → nobody
Blocks: browser-api
Component: Gaia::Browser → General
Summary: URL Bar can disappear from the Browser → [BrowserAPI] mozbrowserscroll events not always fired at endpoint of scroll
> [BrowserAPI] mozbrowserscroll events not always fired at endpoint of scroll

Could you please verify at the API level that these events are not being fired for the endpoint of scrolls?  Based on comment 20, that would be a surprising result.
(In reply to Justin Lebar [:jlebar] from comment #23)
> Could you please verify at the API level that these events are not being
> fired for the endpoint of scrolls?  Based on comment 20, that would be a
> surprising result.

I have no idea how to do that, I was just triaging the 70+ bugs filed under Gaia::Browser and trying re-categorise the ones which don't seem to be fixable in Gaia because they're more general platform bugs. I based the summary on your previous comment but please don't hesitate to change it if it now seems inaccurate.
> I have no idea how to do that,

I was thinking that you'd register a mozbrowserscroll listener which gets and dump()s the frame's scrollx/scrolly.  Then reproduce this bug and see if, when the bug reproduces, we in fact don't get the scrolly <= Z (for whatever Z) that you use to trigger showing the address bar.

> I based the summary on your previous comment but please don't hesitate to change it if it 
> now seems inaccurate.

The summary seems like our best guess; I was just asking you to verify that guess.
I've spent some time debugging this. I think there may be several contributing factors and I'm still not ruling out there being a bug in the browser app here, I sometimes see some inconsistent and odd behaviour.

The one thing I have been able to consistently reproduce is that scroll events only seem to get fired for kinetic scrolling. That is to say that if you drag the page and keep your finger on the screen, no scroll events are fired. If you swipe and lift your finger to trigger the kinetic scroll, then the scroll events do get fired.

If your finger is still touching the screen when you reach the end of the scroll, no scroll event is fired within the required threshold and the address bar does not show/hide.
Urgh. I take it back, I can't even consistently reproduce that part. I just killed the browser app and started it again and now it doesn't happen.

If the console log I'm looking at is to believed, the platform can sometimes get into a state where it only fires scrolling events during kinetic scrolling. Sometimes when the rendering engine is particularly busy no scroll events get fired at all. And sometimes scroll events do get fired, but the browser app doesn't react to them.
cc cjones, who actually understands our scrolling code.

You might have some luck looking into BrowserElementScrolling.js.  For example, we do a mozRequestAnimationFrame() call, which sounds suspiciously related to what you're describing.  (The animation will get throttled if the platform is busy.)
AIUI, BrowserElementScrolling.js is what's being used here, not async pan/zoom, because we explicitly disable async pan/zoom for browser content.  But that would be worth checking.
I think it's the other way around Justin, async pan/zoom is currently only used in the browser app (though cjones would also like to turn it on for some, but not all, bookmarks opened in the homescreen wrapper in bug 815886).
Ben, we discussed this bug during triage and you seem to be doing most of the current investigation so we thought giving it to you was best.  Please re-assign when your work is finished and/or you've found a better owner :)
Assignee: nobody → ben
There's no guarantee that apzc will ever cause a scrollTo() for loaded content even if the content is scrollable.

When apzc samples a content transform, it knows exactly which CSS pixel is at the top of the content view.  (Or at least, it could compute that if it wanted.)  However, this happens on the compositor thread.

We could try to use that to fire some notification on the main thread, but
 - I'm concerned about perf
 - if the browser app tries to update UI using that value, it'll be permanently lagging by a few frames
>  - I'm concerned about perf

APZC knows when the pan/zoom ends, right?  Can we just ensure that we get the event at the endpoint?  That would mitigate the worries about lag, too.
We do, but content can scrollTo() throughout the pan/zoom series at times that are unpredictable to the browser app.  And the last scrollTo() seen by the browser won't necessarily correspond to the top-left CSS pixel at the end of the series.

I think we're getting close to something implementable though.
Does someone want to take this bug? I'm not sure what further action I can take.
(In reply to Ben Francis [:benfrancis] from comment #35)
> Does someone want to take this bug? I'm not sure what further action I can
> take.

Even in the absence of an owner, it looks like you're not the right person.  Dear release drivers: I'm not the right person, either.  We need someone who understands async pan/zoom.
Assignee: ben → nobody
Let's start by turning this problem around.

Ben, in an ideal world, what data do you want apzc to send the browser?
Flags: needinfo?(ben)
Who could be the best owner for this case?
Assign to Benjamin Chen during the Taipei triage today.
Assignee: nobody → bechen
(In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
> Ben, in an ideal world, what data do you want apzc to send the browser?

A single event containing:
* top - distance between top of content and top of iframe
* bottom- distance between bottom of content and bottom of iframe
* Guaranteed to always fire at the end point of the scroll

I *think* that would solve this bug, and allow me to solve bug 797633
Flags: needinfo?(ben)
apzc always knows the most recent top and bottom, but "end point of the scroll" isn't well defined.

I think we should be able to set things up so that there's a "side channel" of information the browser can use in preference to the mozbrowser scroll notifications.
My test case:
1) go to people.mozilla.com/~nhirata/html_tp/formsninput.html
2) scroll it smoothly/slowly or quickly

I found that when I scroll it slowly, there is no mozbrowserscroll event.
But if I scroll it quickly, there is always mozbrowserscroll event send to browser.js.

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#948
It blocks the  mozbrowserscroll event.

AsyncPanZoomController::RequestContentRepaint() 
*mGeckoContentController->RequestContentRepaint(mFrameMetrics);
TabChild::RecvUpdateFrame
*DispatchMessageManagerMessage(NS_LITERAL_STRING("Viewport:Change"), data);
*ScrollWindowTo(window, aFrameMetrics.mScrollOffset);

The idea is that when we are in the end of scroll, the Velocity is always be 0.

======
If we need more information to send to browser, 
we may copy the code from BrowserElementScrolling
addMessageListener("Viewport:Change", this._recvViewportChange.bind(this));
...

Add it into BrowserElementChild.js

I'll try to implement it on my next patch.
Based on attachment 689059 [details] [diff] [review], 
BrowserElementChild.js will get the Viewport:Change and pass a value to browser.js

The value(bottom_distance) is calculated by _cssPageRect and _cssCompositedRect.
_cssPageRect.height: the total content height
_cssCompositedRect.y: the coordination of content map to device's top
_cssCompositedRect.height: the length of content that map to device's height


The value bottom_distance is the distance between content and device bottom.
It will be positive or negative.
positive value: not reach the content bottom
negative value: reach content bottom but also scroll too over!? 
(It is easily to reproduce the negative value if you scroll it quickly, seems another bug..)

Please correct me if I was wrong or misunderstand something.
Attachment #689142 - Flags: feedback?(jones.chris.g)
Attachment #689142 - Flags: feedback?(ben)
This sounds good.

Chris, what did you mean by a "side channel" of information?
Attachment #689142 - Flags: feedback?(ben) → feedback+
Comment on attachment 689142 [details] [diff] [review]
Pass the distance between content bottom and device to browser.js.

Hi Benjamin,

Unfortunately, this isn't the best approach for resolving this bug :(.  The problems here are
 - this patch will cause unnecessary repainting; we're not doing well at this currently (bug 799401)
 - this will cause a "round trip" through web content for the browser UI to get the notification

What we want to do is have AsyncPanZoomController dispatch a notification directly to the embedder in the parent process.

We should decide along with Ben what those notifications should be.
Attachment #689142 - Flags: feedback?(jones.chris.g) → feedback-
Benjamin, it looks like the information you're providing is OK with Ben.

So we can fix up this approach by having apzc fire the data directly to the embedder when we change states from panning or zooming to idle, and without sending a message to the remote content (so as not to force unnecessary repaints).
Blocks: 797633
Now my plan is : 
fire a dom custom event at
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#369

==
Detail implementation will be like 
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementParent.cpp#79 #134
DispatchOpenWindowEvent and OpenWindowOOP

In AsyncPanZoomController, I can use the AsyncPanZoomController::mGeckoContentController cast to RemoteContentController
then use RemoteContentController::mRenderFrame to get the TabParent* ...
==

If I do the implementation above, there is still no mozbrowserscroll event at touch end? Is that OK for browser.js? or just named the event to mozbrowserscroll?
Attached patch Custom domevent v1 (obsolete) — — Splinter Review 
The patch implements comment 47:
Create a custum dom event send to browser.js.

But the patch doesn't add a new idl such as nsIOpenwindowEventDetail.idl, it can only pass a single value right now.

nsCOMPtr<nsIWritableVariant> detailVariant = new nsVariant();
nsresult rv = detailVariant->SetAsFloat(y_bottom_distance);

I think the info is not sufficient to browser.js if we only pass a value at touchend without current "top y coordinate".
Attached patch Custom domevent v2 (obsolete) — — Splinter Review 
In this patch, we add a new idl nsICustomScrollEventDetail.idl to pass information to browser.js.
And fire a dom event "mozbrowsercustomscroll" at touch end.
The detail information:
evt.detail.top : the top coordinate
evt.detail.bottomdis: the distance between content and device bottom 

The difference between this patch and attachment 690302 [details] [diff] [review] is the value we send.
attachment 690302 [details] [diff] [review]: bottomdis
This patch: top and bottomdis
Attachment #690752 - Flags: feedback?(jones.chris.g)
Attachment #690752 - Flags: feedback?(ben)
I don't understand why we need both mozbrowserscroll and mozbrowsercustomscroll and where does the "bottomdis" name come from?
Comment on attachment 690752 [details] [diff] [review]
Custom domevent v2

Thanks Benjamin, this is looking better :).  Some things to fix for a final patch
 - naming nit: s/customscroll/asyncscroll/
 - have the event contain a rect |x, y, width, height|
 - make sure to carefully document what units the values are and what they're relative to
 - you'll want jlebar's feedback on the file naming style

I'm not a DOM peer so I need help reviewing the event and DOM code in a final patch.  I'm happy to review the apzc parts though.
Attachment #690752 - Flags: feedback?(justin.lebar+bug)
Attachment #690752 - Flags: feedback?(jones.chris.g)
Attachment #690752 - Flags: feedback+
Comment on attachment 690752 [details] [diff] [review]
Custom domevent v2

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+  if(mGeckoContentController) {
>+    gfx::Rect cssCompositedRect =
>+      AsyncPanZoomController::CalculateCompositedRectInCssPixels(mFrameMetrics);
>+    mGeckoContentController->SendCustomScrollDOMEvent(mFrameMetrics.mScrollOffset.y,
>+      mFrameMetrics.mScrollableRect.height - (mFrameMetrics.mScrollOffset.y + cssCompositedRect.height));
>+  }

Also, if we reach this code and our mState is PANNING, we'll transition into the FLING state.  We should fire this event at the end of flings animations too, and probably pinch-zooms.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #52)
> Comment on attachment 690752 [details] [diff] [review]
> Custom domevent v2
> 
> >diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp
> 
> >+  if(mGeckoContentController) {
> >+    gfx::Rect cssCompositedRect =
> >+      AsyncPanZoomController::CalculateCompositedRectInCssPixels(mFrameMetrics);
> >+    mGeckoContentController->SendCustomScrollDOMEvent(mFrameMetrics.mScrollOffset.y,
> >+      mFrameMetrics.mScrollableRect.height - (mFrameMetrics.mScrollOffset.y + cssCompositedRect.height));
> >+  }
> 
> Also, if we reach this code and our mState is PANNING, we'll transition into
> the FLING state.  We should fire this event at the end of flings animations
> too, and probably pinch-zooms.

We'll also need to ensure that when content scrolls itself, we fire this event.  We could consider firing it on every repaint request we make to content.  That should be OK performance wise.
(In reply to Ben Francis [:benfrancis] from comment #50)
> I don't understand why we need both mozbrowserscroll and
> mozbrowsercustomscroll

The browser needs to know which events are generated by content and which are generated by apzc.  If it's getting events from apzc, it should just completely ignore the content events (for updating the URL bar, at least).
> > Also, if we reach this code and our mState is PANNING, we'll transition into
> > the FLING state.  We should fire this event at the end of flings animations
> > too, and probably pinch-zooms.
Yeah, we can also fire the event when |mState| transition from |ANIMATING_ZOOM| |FLING| into |NOTHING|.

> 
> We'll also need to ensure that when content scrolls itself, we fire this
> event.  We could consider firing it on every repaint request we make to
> content.  That should be OK performance wise.

If we fire the event on every repaint request, do we need a flag to distinguish the event comes from user or content? :cyu describe a scenario at bug 797633 comment 18
(In reply to Benjamin Chen from comment #55)
> > We'll also need to ensure that when content scrolls itself, we fire this
> > event.  We could consider firing it on every repaint request we make to
> > content.  That should be OK performance wise.
> 
> If we fire the event on every repaint request, do we need a flag to
> distinguish the event comes from user or content? :cyu describe a scenario
> at bug 797633 comment 18

The event we fire from apzc asyncscroll, right?  Content will only fire scroll and we can ignore that in the browser app UI.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #56)
> The event we fire from apzc asyncscroll, right?

Correction: The event we fire from apzc is asyncscroll, right?
(In reply to Chris Jones [:cjones] [:warhammer] from comment #56)
> (In reply to Benjamin Chen from comment #55)
> > > We'll also need to ensure that when content scrolls itself, we fire this
> > > event.  We could consider firing it on every repaint request we make to
> > > content.  That should be OK performance wise.
> > 
> > If we fire the event on every repaint request, do we need a flag to
> > distinguish the event comes from user or content? :cyu describe a scenario
> > at bug 797633 comment 18
> 
> The event we fire from apzc asyncscroll, right?  Content will only fire
> scroll and we can ignore that in the browser app UI.
Yes, assume that apzc fire asyncscroll event directly and content's scroll only fire the scroll event, how do we ensure that "when content scrolls itself, we fire this event" in comment 53?

For example: focus on a input form, the keyboard will be shown and the content will scroll itself.
apzc knows when content scrolls itself because it will get a layers update and we'll recomposite with the new metrics.

What we can try, which is simpler and probably what we actually want to do in the long run, is in this code

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#1062

save the |scrollOffset| on each call to that method (which happens during composition).  Then if |scrollOffset| changes more than X pixels since the last composition and we haven't fired a notification for more than Y milliseconds, fire the asyncscroll event that you've made here.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #59)
> save the |scrollOffset| on each call to that method (which happens during
> composition).  Then if |scrollOffset| changes more than X pixels since the
> last composition and we haven't fired a notification for more than Y
> milliseconds, fire the asyncscroll event that you've made here.

I may be wrong but that sounds like the kind of heuristic which could cause the event not get fired at the end point of the scroll in all cases, which is what this bug was about in the first place. I'm not saying the event should fire for every pixel because that would be daft, but is there any way to guarantee the condition described in the bug summary?
Yes, we can ensure the asyncscroll event always fires within X ms after it changes while being throttled.
Comment on attachment 690752 [details] [diff] [review]
Custom domevent v2

I'm reasonably happy here from a DOM perspective.

>> I don't understand why we need both mozbrowserscroll and
>> mozbrowsercustomscroll
>
> The browser needs to know which events are generated by content and which are
> generated by apzc.  If it's getting events from apzc, it should just
> completely ignore the content events (for updating the URL bar, at least).

I agree with Ben that it's better from an embedder's standpoint if we have just one scroll event.

Are there circumstances under which the browser should listen to APZC for scrolling?

If not, can we just turn off the BrowserElementParent scroll events when APZC is sending its own scroll events?  Then we can make the two events identical and indistinguishable, right?

>diff --git a/dom/browser-element/nsICustomScrollEventDetail.idl b/dom/browser-element/nsICustomScrollEventDetail.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/browser-element/nsICustomScrollEventDetail.idl

>@@ -0,0 +1,16 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+
>+/**
>+ * When we send a mozbrowsercustomscroll event (an instance of CustomEvent), we
>+ * use an instance of this interface as the event's detail.
>+ */
>+[scriptable, uuid(d10947d1-e4e5-4139-9210-1aa78330b118)]
>+interface nsICustomScrollEventDetail : nsISupports
>+{
>+  readonly attribute float top;
>+  readonly attribute float bottomdis;
>+};

The current BrowserElementParent scroll event sends top and left.

Instead of sending bottomdis, it makes more sense to me if we sent

  scrollTop, scrollLeft, contentHeight, scrollHeight, contentWidth, and
  scrollWidth,

since these properties are all existing concepts in the DOM.  We can change the
regular BrowserElementParent scroll event to match this.

You could compute bottomdis as scrollHeight - (top + contentHeight).

>diff --git a/dom/browser-element/nsCustomScrollEventDetail.h b/dom/browser-element/nsCustomScrollEventDetail.h

>+/**
>+ * When we send a mozbrowsercustomscroll event (an instance of CustomEvent), we
>+ * use an instance of this class as the event's detail.
>+ */
>+class nsCustomScrollEventDetail : public nsICustomScrollEventDetail

Since this class does not contain any pointer members, it definitely does not
need to be cycle-collected.  Regular NS_IMPL_ISUPPORTS1 will do.

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
>--- a/layout/ipc/RenderFrameParent.cpp
>+++ b/layout/ipc/RenderFrameParent.cpp

>@@ -552,16 +556,58 @@ public:
>     if (mRenderFrame) {
>       TabParent* browser = static_cast<TabParent*>(mRenderFrame->Manager());
>       browser->HandleLongTap(aPoint);
>     }
>   }
> 
>   void ClearRenderFrame() { mRenderFrame = nullptr; }
> 
>+  virtual bool SendCustomScrollDOMEvent(const float yTop, const float y_bottom_distance) MOZ_OVERRIDE

This probably should live in BrowserElementParent.cpp.  The function there
should take a TabParent* and then the rest of your params (or it can take a
FrameMetrics object, if you like).

We already have code in there that you can probably share with.
Attachment #690752 - Flags: feedback?(justin.lebar+bug) → feedback+
(In reply to Justin Lebar [:jlebar] from comment #62)
> Comment on attachment 690752 [details] [diff] [review]
> Custom domevent v2
> 
> I'm reasonably happy here from a DOM perspective.
> 
> >> I don't understand why we need both mozbrowserscroll and
> >> mozbrowsercustomscroll
> >
> > The browser needs to know which events are generated by content and which are
> > generated by apzc.  If it's getting events from apzc, it should just
> > completely ignore the content events (for updating the URL bar, at least).
> 
> I agree with Ben that it's better from an embedder's standpoint if we have
> just one scroll event.
> 
> Are there circumstances under which the browser should listen to APZC for
> scrolling?

The scroll event sent by content will not say what content top-left pixel is being displayed on screen, in general.  In fact that's quite common.
>> Are there circumstances under which the browser should listen to APZC for
>> scrolling?

Sorry, I missed a "not".  Are there circumstances under which we should forward both APZC and regular content scroll events to the embedder?  Or, for a given frame, can we forward only one type of event?

> The scroll event sent by content will not say what content top-left pixel is being 
> displayed on screen, in general.

While this is true, I don't understand what it has to to do with the question, which means I'm probably missing something.  BEP adds the top-left pixel to the content scroll event before sending it to the embedder.
Attached patch Custom domevent v3 (obsolete) — — Splinter Review 
This patch combine the attachment 689142 [details] [diff] [review] and attachment 690752 [details] [diff] [review].

1. fire asyncscroll when FLING transition to NOTHING
2. fire asyncscroll when ANIMATING_ZOOM transition to NOTHING
* because the function |SampleContentTransformForFrame| is not invoked at main thread, so I need to new a class: SendAsyncScrollEvent : public nsRunnable

3. modify BrowserElementChild to sync with asyncscroll

4.
> Since this class does not contain any pointer members, it definitely does not
> need to be cycle-collected.  Regular NS_IMPL_ISUPPORTS1 will do.
I had tried this but failed 
getting error like: JavaScript Error: "Error: Permission denied for  to create wrapper for object of class UnnamedClass" 

==
Apply this patch, we can ensure that the event(scroll/asyncscroll) will been fired at every "scroll end point" including content scroll itself.
But there still some rounding or inaccuracy problem.
Sometimes browser will receive both scroll and asyncscroll events for the same "scroll action" but the value is not the same.
asyncscroll:197.28334045410156
scroll: 196
In my test environment,(scroll fast/smooth, pinch, double tap) 
the asyncscroll event which is fired at FLING->NOTHING
and ANIMATING_ZOOM->NOTHING seems a duplication of scroll event.
And if browser handles these 2 events as the same logic, maybe I should named the mozbrowserasyncscroll into mozbrowserscroll and remove the event at FLING->NOTHING and ANIMATING_ZOOM->NOTHING. (attachment 689142 [details] [diff] [review] + attachment 690752 [details] [diff] [review])

> The scroll event sent by content will not say what content top-left pixel 
> is being displayed on screen, in general.  In fact that's quite common.
Is there any website or circumstance that I can reproduce the scroll event without top-left pixel ?
> I had tried this but failed 
> getting error like: JavaScript Error: "Error: Permission denied for  to create wrapper 
> for object of class UnnamedClass" 

What code did you use, exactly?

Also want to make sure cjones didn't miss outstanding questions in comment 64.
(In reply to Justin Lebar [:jlebar] from comment #64)
> >> Are there circumstances under which the browser should listen to APZC for
> >> scrolling?
> 
> Sorry, I missed a "not".  Are there circumstances under which we should
> forward both APZC and regular content scroll events to the embedder?  Or,
> for a given frame, can we forward only one type of event?
> 

The real content scroll offset can be interesting, for example if the embedder wants to take a screenshot.  For the URL bar here, no, there's no reason to ever use the real scroll offset.  We should always use asyncscroll when it's available.

> > The scroll event sent by content will not say what content top-left pixel is being 
> > displayed on screen, in general.
> 
> While this is true, I don't understand what it has to to do with the
> question, which means I'm probably missing something.  BEP adds the top-left
> pixel to the content scroll event before sending it to the embedder.

Note, *displayed on screen* is the key.  BEP has no idea what content pixel was last drawn in the top-left of its frame because that's updated asynchronously by the compositor thread.  bechen's patch is sending that info to BEP.

For non-asyncpanzoom frames, what BEP reports is correct.
cjones, can we send just one scroll event, depending whether we have APZ enabled?  Or do we need to send two?

From an API perspective, sending two separate events seems to expose a deep implementation detail of Gecko, and I'd like to avoid that if we can.  I'd rather send one event that has some degree of truthiness than send two events and ask embedders to figure out what the heck we're doing.

Is that OK?
Well, the content scroll offset and the top-left pixel being displayed in the frame on screen are fundamentally not the same thing.

If you want to invent a new concept for that that's not called "scroll event", then sure.  We'll just ignore content scrolls when there's an apzc around.
> Well, the content scroll offset and the top-left pixel being displayed in the frame on screen are 
> fundamentally not the same thing.

But are they not close in general?

The embedder doesn't have a ton of ways to discover when we're fudging the truth.  I'm happy if an embedder can discover that e.g. screenshots sometimes don't quite match the indicated scroll height under some circumstances, for example.
(In reply to Justin Lebar [:jlebar] from comment #67)
> > I had tried this but failed 
> > getting error like: JavaScript Error: "Error: Permission denied for  to create wrapper 
> > for object of class UnnamedClass" 
> 
> What code did you use, exactly?
nsAsyncScrollEventDetail.cpp:
NS_IMPL_ISUPPORTS1(nsAsyncScrollEventDetail,nsIAsyncScrollEventDetail)
DOMCI_DATA(AsyncScrollEventDetail, nsAsyncScrollEventDetail)

Getting error when browser.js is trying to get the value(evt.detail.top) from asyncscroll event. Seems that I miss S_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(AsyncScrollEventDetail).

In the latest patch:
NS_IMPL_ADDREF(nsAsyncScrollEventDetail)
NS_IMPL_RELEASE(nsAsyncScrollEventDetail)
NS_INTERFACE_MAP_BEGIN(nsAsyncScrollEventDetail)
  NS_INTERFACE_MAP_ENTRY(nsISupports)
  NS_INTERFACE_MAP_ENTRY(nsIAsyncScrollEventDetail)
  NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(AsyncScrollEventDetail)
NS_INTERFACE_MAP_END
DOMCI_DATA(AsyncScrollEventDetail, nsAsyncScrollEventDetail)
Attached patch Custom domevent v4 (obsolete) — — Splinter Review 
1. remove the modification in v3 about scroll event (in BrowserElementChild.js)
2. fire asyncscroll event at:
2.1 AsyncPanZoomController::OnTouchEnd
2.2 FLING -> NOTHING
2.3 ANIMATING_ZOOM -> NOTHING
2.4 AsyncPanZoomController::RequestContentRepaint()
2.5 AsyncPanZoomController::SampleContentTransformForFrame
2.6 After 300ms since last throttling event

For 2.5, we add 2 members (mLastAsyncScrollTime, mLastAsyncScrollOffset)in APZC. And the throttling heuristic is 100ms and different scroll offset.
For 2.6, we Also add a 300ms timer to make sure the "last asyncscroll" event is always been fired.

4. move the code that creating/sending domevent into BrowserElementParent::DispatchAsyncScrollEvent
Attachment #689059 - Attachment is obsolete: true
Attachment #690302 - Attachment is obsolete: true
Attachment #692904 - Flags: review?(justin.lebar+bug)
Attachment #692904 - Flags: review?(jones.chris.g)
Please target this for landing early this week - this bug represents non-zero risk and is one of a small handful of platform C2 bugs that missed the milestone.
Target Milestone: B2G C2 (20nov-10dec) → B2G C3 (12dec-1jan)
Just to calibrate your expectations, Alex: I'm not finished with my review, but I'm probably going to want to take another look at this.  The most likely case is that I'll r+ the new version of the patch tomorrow, with changes.  So that targets us for landing Benjamin's final patch on m-i by Wednesday.  Then we have to wait for an m-i --> m-c merge, and then we have to wait for Ryan VM to notice this bug and uplift it.

So realistically, we're looking at Thursday or Friday before a fix here appears on branches.
Comment on attachment 692904 [details] [diff] [review]
Custom domevent v4

Note: I didn't review the DOM parts of this patch.

jlebar, there's a usage of nsITimer in AsyncPanZoomController that I'm
not sure is kosher.  Please take a look.

One high-level issue with this patch is that we'll fire duplicate
asyncscroll events in many cases.  Benjamin, did you measure pan/zoom
performance with and without this patch?  Is it different?  (Note, you
would need to measure with the browser app listening for the
asyncscroll event.)

Please use gfx::Rect and gfx::Size as much as possible in the C++
code, instead of raw floats.

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+void
>+AsyncPanZoomController::TimeExceededFireAsyncScroll(nsITimer* aTimer, void* aClosure)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  AsyncPanZoomController* self = static_cast<AsyncPanZoomController*>(aClosure);

You need to hold a strong ref to the AsyncPanZoomController here.

>+  // throttling the mozbrowserasyncscroll event
>+  // 100ms and different scrolloffset
>+  // 300ms force to fire an asyncscroll

Please make the 100/300ms values symbolic constants.

>+  TimeDuration delta = aSampleTime - mLastAsyncScrollTime;
>+  if (delta.ToMilliseconds() > 100 && (mLastAsyncScrollOffset != scrollOffset)) {
>+    SendAsyncScrollEvent();
>+    mLastAsyncScrollTime = aSampleTime;
>+    mLastAsyncScrollOffset = scrollOffset;

You update to update mLastAsyncScrollOffset before calling
SendAsyncScrollEvent().

>+  }
>+  else {

>+    // set to main thread
>+    nsCOMPtr<nsIThread> mainThread;
>+    NS_GetMainThread(getter_AddRefs(mainThread));
>+    mTimer->SetTarget(mainThread);
>+    mTimer->InitWithFuncCallback(TimeExceededFireAsyncScroll,
>+                                 this,
>+                                 300,
>+                                 nsITimer::TYPE_ONE_SHOT);

We need to verify this usage of the nsITimer.

jlebar, this code always runs on the compositor thread, so the
initialization of this event can race with it running on the main
thread.  I suspect this isn't legal.

>+void AsyncPanZoomController::SendAsyncScrollEvent() {
>+  if(mGeckoContentController) {
>+    gfx::Rect cssCompositedRect =
>+      AsyncPanZoomController::CalculateCompositedRectInCssPixels(mFrameMetrics);
>+    gfx::Rect rect(mFrameMetrics.mScrollOffset.x, mFrameMetrics.mScrollOffset.y,
>+                   cssCompositedRect.width, cssCompositedRect.height);
>+    mGeckoContentController->SendAsyncScrollDOMEvent(
>+      rect,
>+      mFrameMetrics.mScrollableRect.width,
>+      mFrameMetrics.mScrollableRect.height);

Issues here
 - we need to hold |mMonitor| when reading mFrameMetrics

 - we want to send |mLastAsyncScrollOffset| to the browser-element,
   since is the offset that was most recently drawn to screen

I would recommend restructuring this code like

  void AsyncPanZoomController::SendAsyncScrollEvent() {
    if (!mGeckoContentController) {
       return;
    }
    gfx::Rect contentRect;
    gfx::Size scrollableSize;
    {
      MonitorAutoEnter lock(mMonitor);
      contentRect =
        AsyncPanZoomController::CalculateCompositedRectInCssPixels(mFrameMetrics);
      contentRect.MoveTo(mLastAsyncScrollOffset);
    }

    mGeckoContentController->SendAsyncScrollDOMEvent(contentRect, scrollableSize);
  }

>diff --git a/gfx/layers/ipc/GeckoContentController.h b/gfx/layers/ipc/GeckoContentController.h

>+  /**
>+   * Requests sending a asyncscroll domevent to embedder. |screenRect| is in CSS pixels,
>+   * relative to the current cssPage.
>+   */
>+  virtual bool SendAsyncScrollDOMEvent(const gfx::Rect &screenRect,
>+    const float contentTotalWidth, const float contentTotalHeight) = 0;

Style nits:

  virtual bool SendAsyncScrollDOMEvent(const gfx::Rect& aScreenRect,
                                       const gfx::Size& aTotalContentSize) = 0;

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp

>+class SendAsyncScrollEventRunnable : public nsRunnable {

You don't need this code.  Please see below.

> class RemoteContentController : public GeckoContentController {

>+  virtual bool SendAsyncScrollDOMEvent(const gfx::Rect& rect,
>+    const float totalContentWidth, const float totalContentHeight) MOZ_OVERRIDE
>+  {

Instead of this implementation, you can instead use the much simpler

  if (MessageLoop::current() != mUILoop) {
    mUILoop->PostTask(
      FROM_HERE,
      NewRunnableMethod(this, &RemoteContentController::SendAsyncScrollDOMEvent, rect, size);
    return;
  }
  if (mRenderFrame) {
    TabParent* browser = static_cast<TabParent*>(mRenderFrame->Manager());
    BrowserElementParent::DispatchAsyncScrollEvent(browser, rect, size);
  }

This lets us remove the |SendAsyncScrollEventRunnable| class above.
Attachment #692904 - Flags: review?(jones.chris.g)
See comment 77.
Flags: needinfo?(justin.lebar+bug)
Comment on attachment 692904 [details] [diff] [review]
Custom domevent v4

> Please make the 100/300ms values symbolic constants.

Or even better yet, make them prefs.  You may want to use
Preferences::AddUintVarCache and stick the value into static memory instead of
reading the pref every time.

>diff --git a/dom/browser-element/nsIAsyncScrollEventDetail.idl b/dom/browser-element/nsIAsyncScrollEventDetail.idl

>+/**
>+ * When we send a mozbrowsercustomscroll event (an instance of CustomEvent), we
>+ * use an instance of this interface as the event's detail.
>+ */

Looking through this patch it doesn't seem likely we can easily use one event
for the async scrolls and the existing scrolls.  That's disappointing, but I'm
happy to back off that request.

In the meantime, this should be "mozbrowserasyncscroll" above, right?

>diff --git a/dom/browser-element/BrowserElementParent.h b/dom/browser-element/BrowserElementParent.h
>--- a/dom/browser-element/BrowserElementParent.h
>+++ b/dom/browser-element/BrowserElementParent.h

>@@ -81,13 +81,30 @@ public:
>+  /**
>+   * We fire a mozbrowserasyncscroll CustomEvent on the element.
>+   * This event's detail is an instance of nsIAsyncScrollEventDetail.

s/We fire/Fire/.  ("We fire an event" isn't what you want because it doesn't
say that the event is fired /when you call this method/.  If you change it to
simply "Fire an event", it's now imperative, which elucidates the "when" part.)

s/the element/the given TabParent's frame element/

>+   * @param left, top, width, height: the rect map to device screen in CSS pixel.

As cjones said, please use const gfxRect& instead of the raw floats.

Also, I don't know what "the rect map to device screen" is.  Do you mean that
[left, height, right, width] describes the portion of the page which is
currently visible, and that these values have units of CSS pixels?

>+   * @param totalContentWidth, totalContentHeight: the content total width/height

I'm not sure "total" is the right word here.  A total implies that you're
adding things together.  Did you have to add two or more things in order to get
totalContentHeight?  If not, maybe this should just be "contentHeight".

>+   * if (top + height > totalContentHeight) : the content is over scrolled.

s/over scrolled/over-scrolled/.  Also, instead of phrasing it this way, it
would be more clear to me if we phrased it as

  top + height may be larger than totalContentHeight.  This indicates that the
  content is over-scrolled, which occurs when the page "rubber-bands" after
  being scrolled all the way to the bottom.

  Similarly, left + width may be greater than totalContentWidth, and both left
  and height may be negative.

At least, that would be clearer to me if it correctly describes what's going
on.  I'm not sure if it does.  :)

>+   */
>+  static bool
>+  DispatchAsyncScrollEvent(mozilla::dom::TabParent* aOpenerTabParent,

s/aOpenerTabParent/aTabParent/.  This TabParent is not an opener of anything.

Nit: Since this code is inside namespace mozilla, I don't think you need to
quality the class name with mozilla::.

>diff --git a/dom/browser-element/BrowserElementParent.cpp b/dom/browser-element/BrowserElementParent.cpp
>--- a/dom/browser-element/BrowserElementParent.cpp
>+++ b/dom/browser-element/BrowserElementParent.cpp

>+bool
>+BrowserElementParent::DispatchAsyncScrollEvent(mozilla::dom::TabParent* aOpenerTabParent,
>+                                               const float left, const float top,
>+                                               const float width, const float height,
>+                                               const float totalContentWidth,
>+                                               const float totalContentHeight)
>+ [...]

Much of this code is very similar to another method in BrowserElementParent.
Can you combine these two methods?

>diff --git a/dom/browser-element/nsAsyncScrollEventDetail.h b/dom/browser-element/nsAsyncScrollEventDetail.h
>+/**
>+ * When we send a mozbrowserasyncscroll event (an instance of CustomEvent), we
>+ * use an instance of this class as the event's detail.
>+ */
>+class nsAsyncScrollEventDetail : public nsIAsyncScrollEventDetail

Thanks for writing this comment!

>diff --git a/dom/browser-element/nsAsyncScrollEventDetail.cpp b/dom/browser-element/nsAsyncScrollEventDetail.cpp
>+//NS_IMPL_ISUPPORTS1(nsAsyncScrollEventDetail,nsIAsyncScrollEventDetail)

Please remove this.

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp
>--- a/gfx/layers/ipc/AsyncPanZoomController.cpp
>+++ b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+void
>+AsyncPanZoomController::TimeExceededFireAsyncScroll(nsITimer* aTimer, void* aClosure)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  AsyncPanZoomController* self = static_cast<AsyncPanZoomController*>(aClosure);
>+  if (self) {
>+    self->SendAsyncScrollEvent();
>+  }

You don't need this |if (self)| check here, since you only ever pass |this| in
to the timer, and |this| is never null.

>+    if (!mTimer) {
>+      mTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
>+    }
>+    // set to main thread
>+    nsCOMPtr<nsIThread> mainThread;
>+    NS_GetMainThread(getter_AddRefs(mainThread));
>+    mTimer->SetTarget(mainThread);
>+    mTimer->InitWithFuncCallback(TimeExceededFireAsyncScroll,
>+                                 this,
>+                                 300,
>+                                 nsITimer::TYPE_ONE_SHOT);

It's an error to call SetTarget() after you've called Init.  (It doesn't crash,
but the SetTarget() call is ignored.)  You should probably move the SetTarget
call into the |if (!mTimer)| branch.

> jlebar, this code always runs on the compositor thread, so the
> initialization of this event can race with it running on the main
> thread.  I suspect this isn't legal.

nsTimerImpl does not appear to be  threadsafe.  (Its refcounting is threadsafe,
but that's all.)  InitWithFuncCallback clears mCallback, and that could race
with code which invokes mCallback.  So yes, I think we need a mutex here, or
something.

We also need to cancel the timer in ~AsyncPanZoomController, otherwise we have
a use-after-free.  Is the AsyncPanZoomController released on the main thread?
Otherwise this becomes even more complicated, since the timer's firing can race
with the destructor.

If AsyncPanZoomController is always released on the main thread, let's add an
is-main-thread assertion to ~AsyncPanZoomController.

If not, I think we probably have to keep the object alive until the timer
fires.

>@@ -1317,10 +1359,22 @@ void AsyncPanZoomController::SetZoomAndR
>+void AsyncPanZoomController::SendAsyncScrollEvent() {
>+  if(mGeckoContentController) {

Nit: if (...) {
Attachment #692904 - Flags: review?(justin.lebar+bug) → feedback+
Flags: needinfo?(justin.lebar+bug)
Attached patch Custom domevent v5 (obsolete) — — Splinter Review 

        Attachment #694310 -
        Flags: review?(jones.chris.g)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Custom domevent v5 (obsolete)
        — 
          — Splinter Review
      

    


  
1. Use PostDelayedTask to replace the timer in v4
* this implementation seems no more racing condition

2. Use ReentrantMonitor to replace Monitor
*  We want to get the lock in SendAsyncScrollEvent(),use ReentrantMonitor since there are multiple ways to call the SendAsyncScrollEvent().

        Attachment #689142 -
        Attachment is obsolete: true

        Attachment #690752 -
        Attachment is obsolete: true

        Attachment #692245 -
        Attachment is obsolete: true

        Attachment #692904 -
        Attachment is obsolete: true

        Attachment #694310 -
        Attachment is obsolete: true

        Attachment #690752 -
        Flags: feedback?(ben)

        Attachment #694310 -
        Flags: review?(jones.chris.g)

        Attachment #694318 -
        Flags: review?(justin.lebar+bug)

        Attachment #694318 -
        Flags: review?(jones.chris.g)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Custom domevent v5 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #694318 -
        Attachment is obsolete: true

        Attachment #694318 -
        Flags: review?(justin.lebar+bug)

        Attachment #694318 -
        Flags: review?(jones.chris.g)

    
  

        Attachment #694320 -
        Flags: review?(justin.lebar+bug)

        Attachment #694320 -
        Flags: review?(jones.chris.g)

    
    

    
  
Comment on attachment 694320 [details] [diff] [review]
Custom domevent v5

>diff --git a/dom/browser-element/nsIAsyncScrollEventDetail.idl b/dom/browser-element/nsIAsyncScrollEventDetail.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/browser-element/nsIAsyncScrollEventDetail.idl

>@@ -0,0 +1,20 @@
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+
>+/**
>+ * When we send a mozbrowserasyncscroll event (an instance of CustomEvent), we
>+ * use an instance of this interface as the event's detail.
>+ */
>+[scriptable, uuid(d0c13577-31e6-4701-b9b7-3535bbe19fe6)]
>+interface nsIAsyncScrollEventDetail : nsISupports
>+{
>+  readonly attribute float top;
>+  readonly attribute float left;
>+  readonly attribute float width;
>+  readonly attribute float height;
>+  readonly attribute float scrollWidth;
>+  readonly attribute float scrollHeight;
>+};

Although we have a good comment on
BrowserElementParent::DispatchAsyncScrollEvent, we also need to describe here
what these parameters mean (and include the caveats about rubber-banding that
we have in the DispatchAsyncScrollEvent comment).

>diff --git a/dom/browser-element/BrowserElementParent.h b/dom/browser-element/BrowserElementParent.h
>--- a/dom/browser-element/BrowserElementParent.h
>+++ b/dom/browser-element/BrowserElementParent.h

>@@ -81,13 +86,33 @@ public:
>    * @param aURI the URI the new window should load.  May be null.
>    */
>   static bool
>   OpenWindowInProcess(nsIDOMWindow* aOpenerWindow,
>                       nsIURI* aURI,
>                       const nsAString& aName,
>                       const nsACString& aFeatures,
>                       nsIDOMWindow** aReturnWindow);
>+
>+  /**
>+   * Fire a mozbrowserasyncscroll CustomEvent on the given TabParent's frame element.
>+   * This event's detail is an instance of nsIAsyncScrollEventDetail.
>+   *
>+   * @param contentRect: The portion of the page which is currently visible on
>+   *                     screen in CSS pixel.

s/CSS pixel/CSS pixels/.

English nerd nit: s/on screen/on-screen/ or s/on screen/onscreen/ or s/on
screen/on the screen/.  (I looked it up and "on screen" is falling out of
use.)

>+   * @param contentSize: The content width/height.

Nit: Please indicate what the units are here.

>+   * contentRect.top + contentRect.height may be larger than contentSize.height.
>+   * This indicates that the content is over-scrolled, which occurs when the
>+   * page "rubber-bands" after being scrolled all the way to the bottom.
>+   * Similarly, left + width may be greater than contentSize.width, and both left
>+   * and top may be negative.

Just to check: This comment is actually correct?  I wrote it as a guess as to
what these values meant, and I don't want to r+ my own incorrect comment.  :)

>+  static bool
>+  DispatchAsyncScrollEvent(dom::TabParent* aTabParent,
>+                           const gfx::Rect& contentRect,
>+                           const gfx::Size& contentSize);
> };

>diff --git a/dom/browser-element/BrowserElementParent.cpp b/dom/browser-element/BrowserElementParent.cpp
>--- a/dom/browser-element/BrowserElementParent.cpp
>+++ b/dom/browser-element/BrowserElementParent.cpp

>+static bool

Nit: Because this is in a (much maligned) anonymous namespace, static is
redundant here.  (I only mention this because if we say "static" here and not
on the other functions in the anonymous namespace, then readers will want to
figure out why, and that's distracting.)

The style guide says to prefer static over anon namespaces, but given that
we're in an anonymous namespace, I don't think a redundant static is an
improvement.

>+DispatchCustomDOMEvent(Element* aFrameElement, const nsString& eventname,
>+                       nsISupports *aValue)

s/eventname/aEventName/
s/aValue/aDetailValue/

Also, when you take an nsString as a function arg, it should usually be |const
nsAString&|.  (The "A" stands for "abstract".)  There are some exceptions
(particularly in code which deals with IPDL), but this is not one of them, so
please use nsAString here.

>+{
>+  nsIPresShell *shell = aFrameElement->OwnerDoc()->GetShell();
>+  nsRefPtr<nsPresContext> presContext;
>+  if (shell) {
>+    presContext = shell->GetPresContext();
>+  }
>+
>+  nsCOMPtr<nsIDOMEvent> domEvent;
>+  nsEventDispatcher::CreateEvent(presContext, nullptr,
>+                                 NS_LITERAL_STRING("customevent"),
>+                                 getter_AddRefs(domEvent));
>+  NS_ENSURE_TRUE(domEvent, false);
>+
>+
>+  nsCOMPtr<nsIWritableVariant> detailVariant = new nsVariant();

Nit: Remove one of the two blank lines above.

>+  nsresult rv = detailVariant->SetAsISupports(aValue);
>+  NS_ENSURE_SUCCESS(rv, false);
>+  nsCOMPtr<nsIDOMCustomEvent> customEvent = do_QueryInterface(domEvent);
>+  NS_ENSURE_TRUE(customEvent, false);
>+  customEvent->InitCustomEvent(eventname,
>+                               /* bubbles = */ true,
>+                               /* cancelable = */ false,
>+                               detailVariant);
>+  customEvent->SetTrusted(true);
>+  // Dispatch the event.
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  rv = nsEventDispatcher::DispatchDOMEvent(aFrameElement, nullptr,
>+                                           domEvent, presContext, &status);
>+  return true;

The old code would return false if DispatchDOMEvent returned an error.  Unless
doing so creates some problem for your patch, we should do the same here, with

    return NS_SUCCEEDED(rv);

>@@ -85,59 +120,32 @@ DispatchOpenWindowEvent(Element* aOpener
>+  DispatchCustomDOMEvent(aOpenerFrameElement, NS_LITERAL_STRING("mozbrowseropenwindow"),
>+                         detail);

Please don't ignore the return value of DispatchCustomDOMEvent.

>@@ -225,9 +233,24 @@ BrowserElementParent::OpenWindowInProces
>+bool
>+BrowserElementParent::DispatchAsyncScrollEvent(TabParent* aTabParent,
>+                                               const gfx::Rect& contentRect,
>+                                               const gfx::Size& contentSize)

In Gecko, we use the "a" prefix for function arguments.

s/contentRect/aContentRect/
s/contentSize/aContentSize/

>+{
>+  nsIDOMElement* element = aTabParent->GetOwnerElement();
>+  nsCOMPtr<Element> aFrameElement = do_QueryInterface(element);

I'm not positive aFrameElement can't be null, so adding

    NS_ENSURE_TRUE(aFrameElement, false);

would make feel safer.  Or if you prefer you could add that check into
DispatchCustomDOMEvent.

Also, please s/aFrameElement/frameElement/, because aFrameElement is not a
function argument.

>+  // Create the event's detail object.
>+  nsRefPtr<nsAsyncScrollEventDetail> detail =
>+    new nsAsyncScrollEventDetail(contentRect.x, contentRect.y,
>+                                 contentRect.width, contentRect.height,
>+                                 contentSize.width, contentSize.height);
>+  return DispatchCustomDOMEvent(aFrameElement, NS_LITERAL_STRING("mozbrowserasyncscroll"), detail);

Please wrap lines at 80 chars.

>diff --git a/dom/browser-element/nsAsyncScrollEventDetail.h b/dom/browser-element/nsAsyncScrollEventDetail.h
>new file mode 100644
>--- /dev/null
>+++ b/dom/browser-element/nsAsyncScrollEventDetail.h

>+/**
>+ * When we send a mozbrowserasyncscroll event (an instance of CustomEvent), we
>+ * use an instance of this class as the event's detail.
>+ */
>+class nsAsyncScrollEventDetail : public nsIAsyncScrollEventDetail
>+{
>+public:
>+  nsAsyncScrollEventDetail(const float left, const float top, const float width, const float height,
>+    const float contentWidth, const float contentHeigh)

Nit: Please wrap lines at 80 characters.  (layout/ipc seems not to do this, but
that makes it the exception in our code.)

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.h b/gfx/layers/ipc/AsyncPanZoomController.h
>--- a/gfx/layers/ipc/AsyncPanZoomController.h
>+++ b/gfx/layers/ipc/AsyncPanZoomController.h

>@@ -432,16 +438,23 @@ protected:
>+  /**
>+   * Timeout function for asyncscroll event. Because we throttle asyncscroll
>+   * events in some conditions, this function can ensure that the last
>+   * asyncscoll event will be fired in after a fixed duration.
>+   */
>+  void TimeoutFireAsyncScroll();

Nit: A slightly better name might be "FireAsyncScrollOnTimeout" or
"OnAsyncScrollTimeout()".

s/can ensure/ensures/
s/in after/after/
s/fixed duration/period of time/

>@@ -524,16 +537,23 @@ private:
> 
>   // How long it took in the past to paint after a series of previous requests.
>   nsTArray<TimeDuration> mPreviousPaintDurations;
> 
>   // When the last paint request started. Used to determine the duration of
>   // previous paints.
>   TimeStamp mPreviousPaintStartTime;
> 
>+  // The last time and offset we fire the asyncscroll event
>+  // when compositor has sampled the content transform for this frame

This comment is confusing because it's trying to describe three things, and
sentences like this are very hard to write in English (many native speakers
can't do it right!).

Can you split this up into three separate comments?

>+  TimeStamp mLastAsyncScrollTime;
>+  gfx::Point mLastAsyncScrollOffset;
>+  gfx::Point mLatestAsyncScrollOffset;

mLastAsyncScrollOffset and mLatestAsyncScrollOffset are confusing because they
look so similar.

I might suggest mLastAsyncScrollOffset and mCurrentAsyncScrollOffset, but
perhaps cjones has a better idea.

>+  CancelableTask* mAsyncScrollTimeoutTask;

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp
>--- a/gfx/layers/ipc/AsyncPanZoomController.cpp
>+++ b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+/**
>+ * Time duration that we throttle asyncscroll event in compositor thread.
>+ */
>+static const uint32_t ASYNCSCROLL_THROTTLE_DEFAULT_TIME = 100;
>+
>+/**
>+ * Amount of time after we throttle an asyncscroll event.
>+ * Ensure that the last asyncscroll event is fired.
>+ */
>+static const uint32_t ASYNCSCROLL_THROTTLE_DEFAULT_TIMEOUT = 300;

I don't know how cjones feels about this, but personally I think defining
constants for these defaults is not useful, since you only use the constants
once; I'd just hardcode the 100 and 300 into the Preferences::AddUintVarCache
call.

>+/* static */ uint32_t sAsyncScrollThrottleTime;
>+/* static */ uint32_t sAsyncScrollTimeout;
>+
> AsyncPanZoomController::AsyncPanZoomController(GeckoContentController* aGeckoContentController,
>                                                GestureBehavior aGestures)
>   :  mGeckoContentController(aGeckoContentController),
>      mTouchListenerTimeoutTask(nullptr),
>      mX(this),
>      mY(this),
>      mAllowZoom(true),
>      mMinZoom(MIN_ZOOM),
>      mMaxZoom(MAX_ZOOM),
>      mMonitor("AsyncPanZoomController"),
>      mLastSampleTime(TimeStamp::Now()),
>      mState(NOTHING),
>+     mLastAsyncScrollTime(TimeStamp::Now()),
>+     mLastAsyncScrollOffset(0, 0),
>+     mLatestAsyncScrollOffset(0, 0),
>+     mAsyncScrollTimeoutTask(nullptr),
>      mDPI(72),
>      mWaitingForContentToPaint(false),
>      mDisableNextTouchBatch(false),
>      mHandlingTouchQueue(false)
> {
>   if (aGestures == USE_GESTURE_DETECTOR) {
>     mGestureEventListener = new GestureEventListener(this);
>   }

>@@ -104,16 +123,22 @@ AsyncPanZoomController::AsyncPanZoomCont
>   SetDPI(mDPI);
> 
>   if (!gComputedTimingFunction) {
>     gComputedTimingFunction = new ComputedTimingFunction();
>     gComputedTimingFunction->Init(
>       nsTimingFunction(NS_STYLE_TRANSITION_TIMING_FUNCTION_EASE));
>     ClearOnShutdown(&gComputedTimingFunction);
>   }
>+  Preferences::AddUintVarCache(&sAsyncScrollThrottleTime,
>+                               "apzc.asyncscroll.throttle",
>+                               ASYNCSCROLL_THROTTLE_DEFAULT_TIME);
>+  Preferences::AddUintVarCache(&sAsyncScrollTimeout,
>+                               "apzc.asyncscroll.timeout",
>+                               ASYNCSCROLL_THROTTLE_DEFAULT_TIMEOUT);

Is AsyncPanZoomController a singleton?  If not, you don't want to do this every
time an instance the object is constructed; you only want to do it once.

> }
> 
> AsyncPanZoomController::~AsyncPanZoomController() {
> 
> }
> 
> static gfx::Point
> WidgetSpaceToCompensatedViewportSpace(const gfx::Point& aPoint,

>@@ -972,16 +1002,23 @@ void AsyncPanZoomController::RequestCont
>+void
>+AsyncPanZoomController::TimeoutFireAsyncScroll()
>+{
>+  SendAsyncScrollEvent();

Do you want to ensure that mLastAsyncScrollOffset != mLatestAsyncScrollOffset
here, so we don't fire a duplicate event?

>+  mAsyncScrollTimeoutTask = nullptr;
>+}
>+

>@@ -1050,22 +1087,44 @@ bool AsyncPanZoomController::SampleConte
>-    scrollOffset = mFrameMetrics.mScrollOffset;
>+    mLatestAsyncScrollOffset = mFrameMetrics.mScrollOffset;
>+  }
>+
>+  // throttling the mozbrowserasyncscroll event
>+  // sAsyncScrollThrottle(100ms) and different scrolloffset
>+  // sAsyncScrollTimeout(300ms) force to fire an asyncscroll

This comment doesn't make sense to me.  Perhaps you mean something like:

    Fire the mozbrowserasyncscroll event immediately if it's been
    sAsyncScrollThrottle ms since the last time we fired the event and the
    current scroll offset is different than the scroll offset we sent with the
    last event.

    Otherwise, start a timer to fire the event sAsyncScrollTimeout ms from now.

>+  TimeDuration delta = aSampleTime - mLastAsyncScrollTime;
>+  if (delta.ToMilliseconds() > sAsyncScrollThrottleTime &&
>+      mLastAsyncScrollOffset != mLatestAsyncScrollOffset) {
>+    mLastAsyncScrollTime = aSampleTime;
>+    mLastAsyncScrollOffset = mLatestAsyncScrollOffset;
>+    SendAsyncScrollEvent();
>+  }
>+  else {
>+    if (mAsyncScrollTimeoutTask) {
>+      mAsyncScrollTimeoutTask->Cancel();
>+      mAsyncScrollTimeoutTask = nullptr;
>+    }
>+    mAsyncScrollTimeoutTask =
>+      NewRunnableMethod(this, &AsyncPanZoomController::TimeoutFireAsyncScroll);

AIUI this will keep |this| alive until the task fires.  That might be fine; I
just want to check that e.g. SendAsyncScrollEvent won't do something bad if
it's fired after we would have destroyed the AsyncPanZoomController.

If we do fire an AsyncScroll event, do we want to cancel mAsyncScrollTimeoutTask?

>diff --git a/gfx/layers/ipc/GeckoContentController.h b/gfx/layers/ipc/GeckoContentController.h
>--- a/gfx/layers/ipc/GeckoContentController.h
>+++ b/gfx/layers/ipc/GeckoContentController.h

>@@ -39,16 +39,24 @@ public:
>   virtual void HandleSingleTap(const nsIntPoint& aPoint) = 0;
> 
>   /**
>    * Requests handling a long tap. |aPoint| is in CSS pixels, relative to the
>    * current scroll offset.
>    */
>   virtual void HandleLongTap(const nsIntPoint& aPoint) = 0;
> 
>+  /**
>+   * Requests sending a asyncscroll domevent to embedder. |contentRect| is in CSS pixels,
>+   * relative to the current cssPage. |scrollableSize| is the current content
>+   * width/height.
>+   */
>+  virtual void SendAsyncScrollDOMEvent(const gfx::Rect &contentRect,
>+                                       const gfx::Size &scrollableSize) = 0;

Comment nit: "Requests that we send /an/ asyncscroll DOM event to the embedder
of this window."

>diff --git a/layout/ipc/RenderFrameParent.cpp b/layout/ipc/RenderFrameParent.cpp
>--- a/layout/ipc/RenderFrameParent.cpp
>+++ b/layout/ipc/RenderFrameParent.cpp

>@@ -552,16 +553,32 @@ public:
>     if (mRenderFrame) {
>       TabParent* browser = static_cast<TabParent*>(mRenderFrame->Manager());
>       browser->HandleLongTap(aPoint);
>     }
>   }
> 
>   void ClearRenderFrame() { mRenderFrame = nullptr; }
> 
>+  virtual void SendAsyncScrollDOMEvent(const gfx::Rect& contentRect,
>+                                       const gfx::Size& contentSize) MOZ_OVERRIDE
>+  {
>+    if (MessageLoop::current() != mUILoop) {

Just to check: If MessageLoop::current() == mUILoop, are we on the main thread?
BrowserElementParent::DispatchAsyncScrollEvent needs to happen on the main
thread, as written.

This looks good, but there are enough changes here that I'd like to take one
more look, if you don't mind.

        Attachment #694320 -
        Flags: review?(justin.lebar+bug)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Custom domevent v5 (obsolete)
        — 
          — Splinter Review
      

    


  
>>+* contentRect.top + contentRect.height may be larger than contentSize.height.
>>+   * This indicates that the content is over-scrolled, which occurs when the
>>+   * page "rubber-bands" after being scrolled all the way to the bottom.
>>+   * Similarly, left + width may be greater than contentSize.width, and both left
>>+   * and top may be negative.

> Just to check: This comment is actually correct?  I wrote it as a guess as to
> what these values meant, and I don't want to r+ my own incorrect comment.  :)
The comment should be correct but I don't understand what happened if the content is over-scrolled
and the term "rubber-bands" for current page.

> Is AsyncPanZoomController a singleton?  If not, you don't want to do this >every
> time an instance the object is constructed; you only want to do it once.
I'm not sure AsyncPanZoomController is a singleton or not. 
But it seems there's only one AsyncPanZoomController for each TabParent.

>>+  TimeDuration delta = aSampleTime - mLastAsyncScrollTime;
>>+  if (delta.ToMilliseconds() > sAsyncScrollThrottleTime &&
>>+      mLastAsyncScrollOffset != mLatestAsyncScrollOffset) {
>>+    mLastAsyncScrollTime = aSampleTime;
>>+    mLastAsyncScrollOffset = mLatestAsyncScrollOffset;
>>+    SendAsyncScrollEvent();
>>+  }
>>+  else {
>>+    if (mAsyncScrollTimeoutTask) {
>>+      mAsyncScrollTimeoutTask->Cancel();
>>+      mAsyncScrollTimeoutTask = nullptr;
>>+    }
>>+    mAsyncScrollTimeoutTask =
>>+      NewRunnableMethod(this, >>&AsyncPanZoomController::TimeoutFireAsyncScroll);

> AIUI this will keep |this| alive until the task fires.  That might be fine; I
> just want to check that e.g. SendAsyncScrollEvent won't do something bad if
> it's fired after we would have destroyed the AsyncPanZoomController.
NewRunnableMethod will add the obj refcount, so is it possible that we fired an event after 
destroyed the AsyncPanZoomController?

> If we do fire an AsyncScroll event, do we want to cancel mAsyncScrollTimeoutTask?
Yes, we need to cancel mAsyncScrollTimeoutTask. So I made some modification here.

>>+    if (MessageLoop::current() != mUILoop) {
> Just to check: If MessageLoop::current() == mUILoop, are we on the main thread?
The code will run on the main thread.

        Attachment #694320 -
        Attachment is obsolete: true

        Attachment #694320 -
        Flags: review?(jones.chris.g)

        Attachment #694731 -
        Flags: review?(justin.lebar+bug)

        Attachment #694731 -
        Flags: review?(jones.chris.g)

    
    

    
  
> The comment should be correct but I don't understand what happened if the content is 
> over-scrolled and the term "rubber-bands" for current page.

Sorry, I don't understand what it is that you don't understand here.

> I'm not sure AsyncPanZoomController is a singleton or not. 
> But it seems there's only one AsyncPanZoomController for each TabParent.

If each TabParent gets its own AsyncPanZoomController, then it's not a singleton, because you can have multiple TabParent instances in one process.

And indeed, TabParent is the manager() of APZC, so it sounds like APZC is not a singleton.

> NewRunnableMethod will add the obj refcount, so is it possible that we fired an event 
> after destroyed the AsyncPanZoomController?

No, but what I was worried about is if we fired an event after we /would have destroyed/ the APZC, if it weren't for the additional addref from NewRunnableMethod.

That is, the APZC gets torn down and would normally get delete'd, and then the timer fires and tries to use the APZC.  We've had problems with this sort of thing in mozbrowser.  cjones can probably say whether the current code is safe.

    
    

    
  
(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #85)
> > The comment should be correct but I don't understand what happened if the content is 
> > over-scrolled and the term "rubber-bands" for current page.
> 
> Sorry, I don't understand what it is that you don't understand here.

The content page may be over-scrolled, and there is white/background color band append the screen edge. The term "rubber-bands" describes the bands, right?
And what I don't know is that how the gecko layout engine to handle the over-scrolled page. Hence I don't know the term "rubber-bands" is applicable here.

    
    

    
  
Comment on attachment 694731 [details] [diff] [review]
Custom domevent v5

Sorry for the review lag.

(Not reviewing dom/ stuff again.)

>diff --git a/gfx/layers/ipc/AsyncPanZoomController.cpp b/gfx/layers/ipc/AsyncPanZoomController.cpp

>+  nsresult rv = Preferences::GetDefaultUint("apzc.asyncscroll.throttle",
>+                                             &sAsyncScrollThrottleTime);
>+  if(NS_FAILED(rv)) {
>+    Preferences::AddUintVarCache(&sAsyncScrollThrottleTime,
>+                                 "apzc.asyncscroll.throttle", 100);
>+  }
>+
>+  rv = Preferences::GetDefaultUint("apzc.asyncscroll.timeout",
>+                                   &sAsyncScrollTimeout);
>+  if(NS_FAILED(rv)) {
>+    Preferences::AddUintVarCache(&sAsyncScrollTimeout,
>+                                 "apzc.asyncscroll.timeout", 300);

This is safe because APZC is always created on the main thread.  That
may not be true in the future.  Please add a
MOZ_ASSERT(NS_IsMainThread()).

Please remove the Add*Cache() calls because they'll introduce data
races.

>@@ -135,17 +155,17 @@ WidgetSpaceToCompensatedViewportSpace(co
> 
> nsEventStatus
> AsyncPanZoomController::ReceiveInputEvent(const nsInputEvent& aEvent,
>                                           nsInputEvent* aOutEvent)
> {
>   gfxFloat currentResolution;
>   gfx::Point currentScrollOffset, lastScrollOffset;
>   {
>-    MonitorAutoLock monitor(mMonitor);
>+    ReentrantMonitorAutoEnter monitor(mMonitor);

I got to here and then got pretty scared.  Reentrant monitors usually
indicate bad design and can cause bad performance problems.  Why do
we need this?

        Attachment #694731 -
        Flags: review?(jones.chris.g)

    
    

    
  
(In reply to Chris Jones [:cjones] [:warhammer] from comment #87)
> >@@ -135,17 +155,17 @@ WidgetSpaceToCompensatedViewportSpace(co
> > 
> > nsEventStatus
> > AsyncPanZoomController::ReceiveInputEvent(const nsInputEvent& aEvent,
> >                                           nsInputEvent* aOutEvent)
> > {
> >   gfxFloat currentResolution;
> >   gfx::Point currentScrollOffset, lastScrollOffset;
> >   {
> >-    MonitorAutoLock monitor(mMonitor);
> >+    ReentrantMonitorAutoEnter monitor(mMonitor);
> 
> I got to here and then got pretty scared.  Reentrant monitors usually
> indicate bad design and can cause bad performance problems.  Why do
> we need this?
For comment 77, we want to get the mMonitor in 
AsyncPanZoomController::SendAsyncScrollEvent().
But there are many entry points for SendAsyncScrollEvent(). Please see comment 74.
Some of them had already got the mMonitor before calling SendAsyncScrollEvent(), that will cause a deadlock if SendAsyncScrollEvent() lock the monitor again. So I use reentrant monitor to prevent the deadlock.

If we don't use reentrant monitor, 
I should remove the monitor in SendAsyncScrollEvent() and then get the monitor before calling SendAsyncScrollEvent() if needed. Also write the comment "*** The monitor must be held while calling this."

    
    

    
  
> Please remove the Add*Cache() calls because they'll introduce data
> races.

See above: GetDefaultUint is the wrong function to call.

I'm personally not at all worried about data races on static pref variables, but if you are, we should use GetUint.  And we to avoid races we should GetUint only once (or store the prefs as members of the class instead of static variables and GetUint in each constructor).

> The content page may be over-scrolled, and there is white/background color band append 
> the screen edge. The term "rubber-bands" describes the bands, right?

Ah, no.  "Rubber-banding" refers to the way that we over-scroll and then, when the user stops scrolling, we snap back, like an elastic band.

> And what I don't know is that how the gecko layout engine to handle the over-scrolled 
> page. Hence I don't know the term "rubber-bands" is applicable here.

I see.  I don't know that either.  :)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Custom domevent v5 (obsolete)
        — 
          — Splinter Review
      

    


  
1. Revert reentrant monitor and also refine the SendAsyncScrollEvent().
2. Add two members mAsyncScrollThrottleTime, mAsyncScrollTimeout, default values are 100/300ms if getting preference failed at constructor.

        Attachment #694731 -
        Attachment is obsolete: true

        Attachment #694731 -
        Flags: review?(justin.lebar+bug)

        Attachment #695407 -
        Flags: review?(justin.lebar+bug)

        Attachment #695407 -
        Flags: review?(jones.chris.g)

        Attachment #695407 -
        Flags: review?(jones.chris.g) → review+

    
    

    
  


  
re-base version.

dom/tests/mochitest/general/test_interfaces.html:
add "AsyncScrollEventDetail"

try server:
https://tbpl.mozilla.org/?tree=Try&rev=f1bda724c733

        Attachment #695407 -
        Attachment is obsolete: true

        Attachment #695407 -
        Flags: review?(justin.lebar+bug)

        Attachment #695698 -
        Flags: review+

    
    

    
  
Is it possible to write a test for AsyncScrollEvent?

    
    

    
  
(In reply to Justin Lebar [:jlebar] (away 12/21-1/2) from comment #92)
> Is it possible to write a test for AsyncScrollEvent?

Yes, I will file another bug to do this after this patch landed.

    
    

    
  
Note, it's not possible to write one that works on desktop currently, since this event requires apzc which requires omtc.

    
  
Keywords: checkin-needed

    
    

    
  
This stuff should be #ifdef MOZ_B2G'ed.

    
    

    
  
Or in some other way be hidden from normal content.

    
    

    
  
What stuff is exposed to normal content atm?  The new detail object?  Can you please file a follow-up bug?  We may need to change something for the window-open event as well.

    
    

    
  
Yes, the detail interface.
test_interfaces.html was added to prevent accidental global scope polluting. Apparently it isn't as effective as I was hoping.

    
    

    
  
We have some _CHROME_ONLY stuff in nsDOMClassInfo. Perhaps that could work in this case.

    
    

    
  
> test_interfaces.html was added to prevent accidental global scope polluting. Apparently 
> it isn't as effective as I was hoping.

Well, it's not always clear what are the implications of sticking something in there, even to people like me who should know better.  :)

> We have some _CHROME_ONLY stuff in nsDOMClassInfo. Perhaps that could work in this case.

It's not chrome-only -- the B2G browser is content, like all B2G apps.  That's the whole point of having a browser API to begin with.

mozbrowser was used for a brief time on desktop -- I'm not sure we should ifdef B2G this.  Can we expose it only if the mozbrowser pref is true?  I guess that would require restarting the browser after we flip the pref, which wouldn't be helpful for tests...

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/305236f284cf
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: