Closed Bug 989198 Opened 10 years ago Closed 10 years ago

[Stingray] Dispatch KeyboardEvent across BrowserElements

Categories

(Core :: DOM: Events, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: gyeh, Assigned: gyeh)

References

()

Details

(Keywords: dev-doc-needed, Whiteboard: [ft:conndevices])

Attachments

(17 files, 93 obsolete files)

8.33 KB, patch
gyeh
: review+
Details | Diff | Splinter Review
31.76 KB, patch
gyeh
: review+
Details | Diff | Splinter Review
30.51 KB, patch
gyeh
: review+
Details | Diff | Splinter Review
7.79 KB, patch
gyeh
: review+
Details | Diff | Splinter Review
19.20 KB, patch
gyeh
: review+
Details | Diff | Splinter Review
14.75 KB, patch
gyeh
: review+
Details | Diff | Splinter Review
986 bytes, patch
gyeh
: review+
Details | Diff | Splinter Review
977 bytes, patch
gyeh
: review+
Details | Diff | Splinter Review
2.57 KB, patch
gyeh
: review+
Details | Diff | Splinter Review
8.58 KB, patch
Details | Diff | Splinter Review
29.76 KB, patch
Details | Diff | Splinter Review
29.81 KB, patch
Details | Diff | Splinter Review
7.64 KB, patch
Details | Diff | Splinter Review
20.59 KB, patch
Details | Diff | Splinter Review
14.71 KB, patch
Details | Diff | Splinter Review
986 bytes, patch
Details | Diff | Splinter Review
1.33 KB, patch
Details | Diff | Splinter Review
After we have DOM Level 3 Events for the hardware buttons, such as 'Power', 'VolumeUp' and 'VolumeDown', we'd like to dispatch these events across browser elements.

In this way, the installed applications are allowed to override these hardware buttons to provide better user experiences.

Kanru has proposed a proposal on dev-webapi:
https://groups.google.com/forum/#!topic/mozilla.dev.webapi/xPGXAHEiVmg
Blocks: 980768
any plan for this?

we have a application need hw-button interactions.
Deat Steven , 
 How about this issue ? Could you provide a plan ?
Flags: needinfo?(styang)
Flags: needinfo?(styang)
Assignee: nobody → gyeh
Attached patch WIP patch (obsolete) — Splinter Review
smaug, could you take a look at this? Just wanna to make sure that I'm on the right track. :)
Attachment #8431486 - Flags: feedback?(bugs)
Attached patch WIP patch (obsolete) — Splinter Review
Attachment #8431486 - Attachment is obsolete: true
Attachment #8431486 - Flags: feedback?(bugs)
Attachment #8431493 - Flags: feedback?(bugs)
Comment on attachment 8431493 [details] [diff] [review]
WIP patch

Looks like that KeyEventDetail and BeforeKeyEventDetail can cause forgetting to support new feature easily. Copying this should be implemented in widget/TextEvents.h or widget/shared/WidgetEventsImpl.cpp. And also if it's possible, this should be checked by automated tests.

Actually, you don't support KeyboardEvent.code value, though.

> +    if (!keyboardEvent) {
> +      NS_WARNING("Not a keyboard event");

You can use NS_WARN_IF() macro.

> +    keyboardEvent->InitBasicModifiers(detail2.mCtrlKey, detail2.mAltKey, detail2.mShiftKey, detail2.mMetaKey);

Hmm, this doesn't support other modifier state, e.g., CapsLock, NumLock, etc...
Thanks, Masayuki.

I'll update the patch next week.
Attachment #8431493 - Attachment is obsolete: true
Attachment #8431493 - Flags: feedback?(bugs)
Attachment #8435712 - Flags: review?(masayuki)
Attachment #8435712 - Flags: review?(bugs)
Attachment #8435713 - Flags: review?(masayuki)
Attachment #8435713 - Flags: review?(bugs)
Attachment #8435714 - Flags: review?(masayuki)
Attachment #8435714 - Flags: review?(bugs)
Attachment #8435715 - Flags: review?(masayuki)
Attachment #8435715 - Flags: review?(bugs)
Hi Smaug and Masayuki,

I've implemented the proposal in https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent#Dispatch_KeyboardEvent_across_BrowserElements. Please take some time on these patches and let me know if you have any suggestions. Thanks.

Btw, I also push these patches to my repo: https://github.com/ginayeh/gecko-dev/tree/bug989198-keyevent
Comment on attachment 8435712 [details] [diff] [review]
[v1] Part 1: Define dictionary 'BeforeKeyEventDetail' and 'KeyEventDetail'

>diff --git a/dom/webidl/BrowserElementDictionaries.webidl b/dom/webidl/BrowserElementDictionaries.webidl
>index a259c7d..6c73db4 100644
>--- a/dom/webidl/BrowserElementDictionaries.webidl
>+++ b/dom/webidl/BrowserElementDictionaries.webidl
>@@ -18,12 +18,33 @@ dictionary AsyncScrollEventDetail {
> 
> dictionary OpenWindowEventDetail {
>   DOMString url = "";
>   DOMString name = "";
>   DOMString features = "";
>   Node? frameElement = null;
> };
> 
>+dictionary BeforeKeyEventDetail {
>+  unsigned long charCode = 0;
>+  unsigned long keyCode = 0;
>+
>+  boolean       altKey = false;
>+  boolean       ctrlKey = false;
>+  boolean       shiftKey = false;
>+  boolean       metaKey = false;
>+
>+  unsigned long location = 0;
>+  boolean       repeat = false;
>+  boolean       isComposing = false;
>+
>+  DOMString     key = "";
>+  DOMString     code = "";
>+};
>+
>+dictionary KeyEventDetail : BeforeKeyEventDetail {
>+  boolean embeddedCancelled = false;
>+};
>+

Hmm, it might be better to write comment in dom/webidl/KeyboardEvent.webidl, e.g., "Don't forget to update BeforeKeyEventDetail in BrowserElementDictionaries.webidl".

>diff --git a/widget/TextEvents.h b/widget/TextEvents.h
>index 0c9311d..6d88c9a 100644
>--- a/widget/TextEvents.h
>+++ b/widget/TextEvents.h
>@@ -5,16 +5,17 @@
> 
> #ifndef mozilla_TextEvents_h__
> #define mozilla_TextEvents_h__
> 
> #include <stdint.h>
> 
> #include "mozilla/Assertions.h"
> #include "mozilla/BasicEvents.h"
>+#include "mozilla/dom/BrowserElementDictionariesBinding.h"
> #include "mozilla/EventForwards.h" // for KeyNameIndex, temporarily
> #include "mozilla/TextRange.h"
> #include "nsCOMPtr.h"

I like better you to add it under TextRange.h since it's different directory.

>+  void AssignKeyEventData(mozilla::dom::BeforeKeyEventDetail& aDetail)
>+  {
>+    aDetail.mCharCode = charCode;
>+    aDetail.mKeyCode = keyCode;
>+
>+    aDetail.mAltKey = IsAlt();
>+    aDetail.mCtrlKey = IsControl();
>+    aDetail.mShiftKey = IsShift();
>+    aDetail.mMetaKey = IsMeta();
>+
>+    aDetail.mLocation = location;
>+    aDetail.mRepeat = mIsRepeat;
>+    aDetail.mIsComposing = mIsComposing;
>+
>+    nsAutoString key, code;
>+    GetDOMKeyName(key);
>+    aDetail.mKey = key;
>+    GetDOMCodeName(code);
>+    aDetail.mCode = code;
>+  }
>+
>+  void AssignKeyEventData(mozilla::dom::KeyEventDetail& aDetail)
>+  {
>+    AssignKeyEventData((mozilla::dom::BeforeKeyEventDetail&)aDetail);
>+    aDetail.mEmbeddedCancelled = mFlags.mDefaultPrevented;
>+  }

I don't like these method names because AssignKeyEventData() is copied from argument. However, these methods copy to the argument.

Perhaps, CopyTo() is better?

And this patch doesn't support other modifier state with .getModifierState(). E.g., .getModifierState("NumLock"). Is it okay? If not, you should add them.
FYI: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.getModifierState
Attachment #8435712 - Flags: review?(masayuki) → review+
Comment on attachment 8435712 [details] [diff] [review]
[v1] Part 1: Define dictionary 'BeforeKeyEventDetail' and 'KeyEventDetail'

Oops, and you can omit "mozilla::" in TextEvents.h and staring with "dom::" is enough.

> +    nsAutoString key, code;
> +    GetDOMKeyName(key);
> +    aDetail.mKey = key;
> +    GetDOMCodeName(code);
> +    aDetail.mCode = code;

And why don't you write this as:

GetDOMKeyName(aDetail.mKey); ?
Comment on attachment 8435713 [details] [diff] [review]
[v1] Part 2: Implement HandleKeyEvent

>diff --git a/layout/base/nsIPresShell.h b/layout/base/nsIPresShell.h
>@@ -194,16 +196,22 @@ public:
>    * controls incorrectly store their data in the frames rather than the
>    * content model and printing calls |EndObservingDocument| multiple
>    * times to make form controls behave nicely when printed.
>    */
>   virtual void Destroy() = 0;
> 
>   bool IsDestroying() { return mIsDestroying; }
> 
>+  virtual void HandleKeyEvent(nsINode* aTarget,
>+                              mozilla::WidgetEvent* aEvent,

Why don't you use WidgetKeyboardEvent here?

>+                              nsEventStatus* aStatus = nullptr,
>+                              mozilla::EventDispatchingCallback* aEventCB = nullptr,
>+                              bool aIsBefore = false) = 0;

I don't like bool argument like this especially for API design since it's too hard deveopers to understand what means "true" at reading around a caller. Additionally, you don't call this method from other classes with "true". So, I think that we can remove aIsBefore from this interface method.

Then, current PresShell::HandleKeyEvent() should be HandleKeyEventInternal() which is not a virtual method. Then, we can reduce the count of calls of virtual HandleKeyEvent().

And you need to update the IID of nsIPresShell because you add new method(s).

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>index b2f940a..f9fc8ff 100644
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -7577,16 +7581,145 @@ nsIPresShell::DispatchGotOrLostPointerCaptureEvent(bool aIsGotCapture,
>+bool
>+PresShell::DispatchKeyEvent(nsINode* aNode,
>+                            WidgetEvent* aEvent,
>+                            nsEventStatus* aStatus,
>+                            mozilla::EventDispatchingCallback* aEventCB,
>+                            bool aIsBefore)

Sounds like this name is misleadable. I think that this name should accept NS_KEY_PRESS too.

>+  nsCOMPtr<nsIDocument> document = aNode->OwnerDoc();
>+  NS_ENSURE_TRUE(document, false);

nit: We shouldn't use NS_ENSURE_*() for new code. Please write this like:

if (NS_WARN_IF(!document)) {
  return false;
}

>+  nsCOMPtr<nsIPresShell> presShell = document->GetShell();
>+  NS_ENSURE_TRUE(presShell, false);

Same.

>+  nsRefPtr<nsPresContext> presContext = presShell->GetPresContext();
>+  NS_ENSURE_TRUE(presContext, false);

Same.

>+  nsCOMPtr<nsIDOMDocument> domDoc = do_QueryInterface(document);
>+  NS_ENSURE_TRUE(domDoc, false);

Same.

>+  nsCOMPtr<nsIDOMEvent> domEvent;
>+  nsresult rv = domDoc->CreateEvent(NS_LITERAL_STRING("customevent"),
>+                                    getter_AddRefs(domEvent));

Same.

>+  nsCOMPtr<nsIGlobalObject> sgo = document->GetScopeObject();
>+  NS_ENSURE_TRUE(sgo, false);

Same.

>+  AutoJSContext cx;
>+  JS::Rooted<JSObject*> global(cx, sgo->GetGlobalJSObject());
>+  JSAutoCompartment ac(cx, global);
>+
>+  // Wrap event data
>+  nsAutoString eventName;
>+  JS::Rooted<JS::Value> val(cx);
>+  if (aIsBefore) {
>+    BeforeKeyEventDetail detail;
>+    aEvent->AsKeyboardEvent()->AssignKeyEventData(detail);
>+    NS_ENSURE_TRUE(detail.ToObject(cx, &val), false);

Same.

>+
>+    eventName = (aEvent->message == NS_KEY_DOWN) ?
>+                NS_LITERAL_STRING("mozbrowserbeforekeydown") :
>+                NS_LITERAL_STRING("mozbrowserbeforekeyup");
>+  } else {
>+    KeyEventDetail detail;
>+    aEvent->AsKeyboardEvent()->AssignKeyEventData(detail);
>+    NS_ENSURE_TRUE(detail.ToObject(cx, &val), false);
>+
>+    eventName = (aEvent->message == NS_KEY_DOWN) ?
>+                NS_LITERAL_STRING("mozbrowserkeydown") :
>+                NS_LITERAL_STRING("mozbrowserkeyup");
>+  }
>+
>+  ErrorResult res;
>+  nsCOMPtr<nsIDOMCustomEvent> domCustomEvent = do_QueryInterface(domEvent);
>+  NS_ENSURE_TRUE(domCustomEvent, false);
>+  CustomEvent* customEvent = static_cast<CustomEvent*>(domCustomEvent.get());
>+  customEvent->InitCustomEvent(cx, eventName, true, true, val, res);
>+  ENSURE_SUCCESS(res, false);
>+
>+  domCustomEvent->SetTrusted(true);

How about to set this only when the event is trusted? Escalating trusted level is risky even though current code is safe.

>+  WidgetEvent* event = domEvent->GetInternalNSEvent();
>+  event->mFlags.mWantReplyFromContentProcess = true;
>+
>+  // Dispatch event to iframe.
>+  nsCOMPtr<EventTarget> et = do_QueryInterface(document->GetWindow());
>+  if (NS_FAILED(EventDispatcher::Dispatch(et, mPresContext,
>+                                          event, domEvent, aStatus))) {
>+    return false;
>+  }
>+
>+  return true;

return NS_SUCCEEDED(EventDispatcher::Dispatch())

>+}
>+
>+void
>+PresShell::HandleKeyEvent(nsINode* aTarget,
>+                          WidgetEvent* aEvent,
>+                          nsEventStatus* aStatus,
>+                          EventDispatchingCallback* aEventCB,
>+                          bool aIsBefore)

How about this to handle NS_KEY_PRESS too? When coming event is neither keydown or keyup, this method just dispatch a DOM key event. Then, the caller of PresShell::HandleEventInternal() can be simpler since it's enough to check the struct type.

>+{
>+  WidgetKeyboardEvent* keyboardEvent = aEvent->AsKeyboardEvent();
>+  NS_ENSURE_TRUE_VOID(keyboardEvent);

Use NS_WARN_IF().

>+
>+  // Build up the mozbrowser chain.
>+  nsCOMPtr<nsINode> node = aTarget;
>+  nsTArray<nsCOMPtr<nsINode> > chain;

I don't know the enough number of this array for typical cases, though. You should use nsAutoTArray for preventing memory fragmentation.

>+  while (node) {
>+    chain.AppendElement(node);
>+    nsPIDOMWindow* win = node->OwnerDoc()->GetWindow();
>+    node = win ? win->GetFrameElementInternal() : nullptr;
>+  }
>+
>+  /**
>+   * Dispatch event from the innermost element for
>+   *   'mozbrowserbeforekeydown' and 'mozbrowserbeforekeyup'.
>+   *
>+   * Dispatch event from the outermost element for
>+   *   'mozbrowserkeydown' and 'mozbrowserkeyup'.
>+   */
>+  size_t length = chain.Length();
>+  for (int i = 0, j = length - 1; i < length; i++, j--) {
>+    node = aIsBefore ? chain[j] : chain[i];
>+
>+    bool shouldDispatchOrigEvent = false;
>+    if (node->NodeName().Find("IFRAME")) {
>+      // Fallback: dispatch original key event to event target.
>+      shouldDispatchOrigEvent = !DispatchKeyEvent(node, aEvent, aStatus,
>+                                                  aEventCB, aIsBefore);
>+     } else if (aIsBefore && !j) {

Strange indent (extra whitespace).

>+      // Dispatch 'keydown' or 'keyup' to event target.
>+      shouldDispatchOrigEvent = true;
>+    }

I wonder if the nodes in the array will be moved to other point or just removed, does this still need to continue to dispatch? This must be checked by smaug.

>+
>+    if (shouldDispatchOrigEvent) {
>+      EventDispatcher::Dispatch(static_cast<nsISupports*>(aTarget), mPresContext,
>+                                aEvent, nullptr, aStatus, aEventCB);
>+      break;
>+    }
>+  }
>+}
Attachment #8435713 - Flags: review?(masayuki)
Comment on attachment 8435714 [details] [diff] [review]
[v1] Part 3: Invoke HandleKeyEvent

>diff --git a/dom/events/EventStateManager.cpp b/dom/events/EventStateManager.cpp
>index 212a2d8..7981192 100644
>--- a/dom/events/EventStateManager.cpp
>+++ b/dom/events/EventStateManager.cpp
>@@ -1176,16 +1176,25 @@ CrossProcessSafeEvent(const WidgetEvent& aEvent)
> }
> 
> bool
> EventStateManager::HandleCrossProcessEvent(WidgetEvent* aEvent,
>                                            nsEventStatus *aStatus) {
>   if (*aStatus == nsEventStatus_eConsumeNoDefault ||
>       aEvent->mFlags.mNoCrossProcessBoundaryForwarding ||
>       !CrossProcessSafeEvent(*aEvent)) {
>+    // Dispatch 'mozbrowserkeydown'/'mozbrowserkeyup' for in-process case.
>+    nsIFrame* frame = GetEventTarget();
>+    nsIContent* target = frame ? frame->GetContent() : nullptr;
>+    nsIPresShell* presShell = mPresContext->PresShell();
>+    if (target && presShell &&
>+        !IsRemoteTarget(target) &&
>+        !aEvent->mFlags.mDefaultPrevented) {
>+      presShell->HandleKeyEvent(target, aEvent, aStatus);
>+    }
>     return false;
>   }

I don't understand the reason why you don't check if the aEvent is a key event.

>diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp
>index 05d61af..fbacc34 100644
>--- a/dom/ipc/TabParent.cpp
>+++ b/dom/ipc/TabParent.cpp
>@@ -1269,29 +1270,36 @@ TabParent::GetChildProcessOffset()
> }
> 
> bool
> TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
> {
>   NS_ENSURE_TRUE(mFrameElement, true);
> 
>   WidgetKeyboardEvent localEvent(event);
>-  // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>-  // being infinitely redispatched and forwarded to the child again.
>-  localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
> 
>   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>   // to be able to dispatch it to the <browser> element as the target element.
>   nsIDocument* doc = mFrameElement->OwnerDoc();
>   nsIPresShell* presShell = doc->GetShell();
>   NS_ENSURE_TRUE(presShell, true);
>   nsPresContext* presContext = presShell->GetPresContext();
>   NS_ENSURE_TRUE(presContext, true);
> 
>-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+  if (event.message == NS_KEY_DOWN ||
>+      event.message == NS_KEY_UP) {

If this can be changed to |event.eventStructType == NS_KEY_EVENT|, HandleKeyEvent or this method should set mNoCrossProcessBoundaryForwarding true. But I don't understand the reason why we don't need to set it only for keydown and keyup events.

>+    // Dispatch 'mozbrowserkeydown'/'mozbrowserkeyup' for out-of-process case.
>+    presShell->HandleKeyEvent(mFrameElement, &localEvent);
>+  } else {
>+    // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>+    // being infinitely redispatched and forwarded to the child again.
>+    localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
>+
>+    EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+  }
>   return true;
> }

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>index f9fc8ff..97c570d 100644
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -7533,16 +7533,21 @@ PresShell::HandleEventInternal(WidgetEvent* aEvent, nsEventStatus* aStatus)
>               eventCBPtr = nullptr;
>             }
>           }
>           if (eventTarget) {
>             if (aEvent->eventStructType == NS_COMPOSITION_EVENT ||
>                 aEvent->eventStructType == NS_TEXT_EVENT) {
>               IMEStateManager::DispatchCompositionEvent(eventTarget,
>                 mPresContext, aEvent, aStatus, eventCBPtr);
>+            } else if (aEvent->message == NS_KEY_DOWN ||
>+                       aEvent->message == NS_KEY_UP) {
>+              // Dispatch 'mozbrowserbeforekeydown'/'mozbrowserbeforekeyup'
>+              // and 'keydown'/'keyup'.
>+              HandleKeyEvent(eventTarget, aEvent, aStatus, eventCBPtr, true);

As I said, this should check eventStructType. And HandleKeyEvent() should accept keypress event.

Anyway, I should check new patch.
Attachment #8435714 - Flags: review?(masayuki)
Attachment #8435715 - Flags: review?(masayuki) → review+
Thanks for your prompt review, Masayuki. :)

Let me update the patches (part 2 and part 3) and then send review request to you later.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #13)
> 
> I don't like these method names because AssignKeyEventData() is copied from
> argument. However, these methods copy to the argument.
> 
> Perhaps, CopyTo() is better?

Yep, it makes sense to me.

> And this patch doesn't support other modifier state with
> .getModifierState(). E.g., .getModifierState("NumLock"). Is it okay? If not,
> you should add them.
> FYI:
> https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.
> getModifierState

It should be fine without these modifier states but it would be better to have them with the keyboard event. I'm going to add them into my next patch.

Again, thanks for your suggestions.
Comment on attachment 8435712 [details] [diff] [review]
[v1] Part 1: Define dictionary 'BeforeKeyEventDetail' and 'KeyEventDetail'

I agree with masayuki, and could take a look at the updated patch.
Attachment #8435712 - Flags: review?(bugs) → review-
Comment on attachment 8435715 [details] [diff] [review]
[v1] Part 4: Adapt shell.js to new events

>+    // FIXME: Hardware key events should be dispatched to applications directly.
>+    // So, we shouldn't do stopImmediatePropagation and preventDefault
>+    // afterwards. Besides, no need to send mozChromeEvent for hardware keys.
>+    // Please see bug 989198 for further details.
>+
>     // If we didn't return, then the key event represents a hardware key
>     // and we need to prevent it from propagating to Gaia
>-    evt.stopImmediatePropagation();
>-    evt.preventDefault(); // Prevent keypress events (when #501496 is fixed).
>+    // evt.stopImmediatePropagation();
>+    // evt.preventDefault(); // Prevent keypress events (when #501496 is fixed).
I don't understand this.
Why we have the stuff commented out, and the documentation that it is the correct behavior.
Attachment #8435715 - Flags: review?(bugs) → review-
Comment on attachment 8435713 [details] [diff] [review]
[v1] Part 2: Implement HandleKeyEvent

>+bool
>+PresShell::DispatchKeyEvent(nsINode* aNode,
>+                            WidgetEvent* aEvent,
>+                            nsEventStatus* aStatus,
>+                            mozilla::EventDispatchingCallback* aEventCB,
>+                            bool aIsBefore)
The method name is odd.
It may or may not dispatch an event, and the return value indicates something...

>+{
>+  MOZ_ASSERT(aNode && aEvent);
>+  MOZ_ASSERT(aEvent->message == NS_KEY_DOWN || aEvent->message == NS_KEY_UP);
>+
>+  nsCOMPtr<nsIDocument> document = aNode->OwnerDoc();
>+  NS_ENSURE_TRUE(document, false);
OwnerDoc() is guaranteed to return non-null


Btw, I don't mind use of NS_ENSURE_* macros. They tend to keep the code easier to read than
if (NS_WARN_IF(!foo)) {
  return false;
}


>+PresShell::HandleKeyEvent(nsINode* aTarget,
>+                          WidgetEvent* aEvent,
>+                          nsEventStatus* aStatus,
>+                          EventDispatchingCallback* aEventCB,
>+                          bool aIsBefore)
>+{
>+  WidgetKeyboardEvent* keyboardEvent = aEvent->AsKeyboardEvent();
>+  NS_ENSURE_TRUE_VOID(keyboardEvent);
>+
>+  // Build up the mozbrowser chain.
>+  nsCOMPtr<nsINode> node = aTarget;
>+  nsTArray<nsCOMPtr<nsINode> > chain;
>+  while (node) {
>+    chain.AppendElement(node);
>+    nsPIDOMWindow* win = node->OwnerDoc()->GetWindow();
>+    node = win ? win->GetFrameElementInternal() : nullptr;
>+  }
What chain is this?
If we have nested iframes, this will include then all, not only mozbrowsers.
Why would we want that?

>+    if (node->NodeName().Find("IFRAME")) {
looks like you want actually nsIContent here and do
content->IsHTML(nsGkAtoms::iframe)
Attachment #8435713 - Flags: review?(bugs) → review-
Comment on attachment 8435714 [details] [diff] [review]
[v1] Part 3: Invoke HandleKeyEvent

> EventStateManager::HandleCrossProcessEvent(WidgetEvent* aEvent,
>                                            nsEventStatus *aStatus) {
>   if (*aStatus == nsEventStatus_eConsumeNoDefault ||
>       aEvent->mFlags.mNoCrossProcessBoundaryForwarding ||
>       !CrossProcessSafeEvent(*aEvent)) {
>+    // Dispatch 'mozbrowserkeydown'/'mozbrowserkeyup' for in-process case.
>+    nsIFrame* frame = GetEventTarget();
>+    nsIContent* target = frame ? frame->GetContent() : nullptr;
>+    nsIPresShell* presShell = mPresContext->PresShell();
>+    if (target && presShell &&
>+        !IsRemoteTarget(target) &&
>+        !aEvent->mFlags.mDefaultPrevented) {
>+      presShell->HandleKeyEvent(target, aEvent, aStatus);
>+    }

This setup looks odd. In a method which is for cross-process event dispatching, we dispatchin
non-cross-process event in case the target isn't for cross-process case.
And we may end up checking event defaultPrevented twice: 
*aStatus == nsEventStatus_eConsumeNoDefault and !aEvent->mFlags.mDefaultPrevented
Attachment #8435714 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #20)
> Comment on attachment 8435715 [details] [diff] [review]
> [v1] Part 4: Adapt shell.js to new events
> 
> >+    // FIXME: Hardware key events should be dispatched to applications directly.
> >+    // So, we shouldn't do stopImmediatePropagation and preventDefault
> >+    // afterwards. Besides, no need to send mozChromeEvent for hardware keys.
> >+    // Please see bug 989198 for further details.
> >+
> >     // If we didn't return, then the key event represents a hardware key
> >     // and we need to prevent it from propagating to Gaia
> >-    evt.stopImmediatePropagation();
> >-    evt.preventDefault(); // Prevent keypress events (when #501496 is fixed).
> >+    // evt.stopImmediatePropagation();
> >+    // evt.preventDefault(); // Prevent keypress events (when #501496 is fixed).
> I don't understand this.
> Why we have the stuff commented out, and the documentation that it is the
> correct behavior.

Oops! This shouldn't be part of the patch. I will update the patch again. Thanks.
Smaug, thanks for your feedbacks. I'm outside the office these two weeks, but I'll update the patches according your comments once I have time or go back to the office.
feature-b2g: --- → 2.1
Blocks: 1026354
No longer blocks: 980768
Whiteboard: [FT:Stream3]
feature-b2g: 2.1 → ---
No longer blocks: Stream3_2.1
feature-b2g: --- → 2.1
Hi Masayuki,

Per our discussion via IRC, we agreed to add new event message in widget/BasicEvents.h and define new events in dom/events/EventNameList.h

I'd like to confirm the usage of macro EVENT and NON_IDL_EVENT in EventNameList.h

* Can I use NON_IDL_EVENT for these events? Should I add these events to GlobalEventHandlers (http://dxr.mozilla.org/mozilla-central/source/dom/webidl/EventHandler.webidl#28)?

Or,
* Should I use EVENT for these events and add them to both GlobalEventHandlers and nsIInlineEventHandlers (http://dxr.mozilla.org/mozilla-central/source/dom/interfaces/core/nsIInlineEventHandlers.idl#11)?

Can you give me some suggestions? Thanks.
Flags: needinfo?(masayuki)
> Can I use NON_IDL_EVENT for these events?

I think so.

> Should I add these events to GlobalEventHandlers (http://dxr.mozilla.org/mozilla-central/source/dom/webidl/EventHandler.webidl#28)?

I don't think so because mozbrowser* events shouldn't be available on event handler attribute.

Sorry for the delay to reply.
Flags: needinfo?(masayuki)
I see. Thanks, Masayuki. :)
Blocks: Stream3_2.1
The implementation is greatly changed, and I'd like to get some feedbacks before moving on the other parts.
Attachment #8435712 - Attachment is obsolete: true
Attachment #8435713 - Attachment is obsolete: true
Attachment #8435714 - Attachment is obsolete: true
Attachment #8435715 - Attachment is obsolete: true
Here's a brief overview of my patch:
- Create "BrowserElementKeyboardEvent.webidl" inherited from KeyboardEvent.
- Implemente class BrowserElementKeyboardEvent.
- Add a data member in WidgetKeyboardEvent.
- Define new event messages.
Besides, I got a few questions and I'd like to hear from you.
- I'm not sure whether the name of webidl is clear nor not. Any suggestion?
- Regarding event names,
  - should I use "mozbrowser" as prefix?
  - which is better, "mozbrowserkeydown" or "mozbrowserafterkeydown"?
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
Comment on attachment 8457890 [details] [diff] [review]
Patch 1(v2): Create BrowserElementKeyboardEvent

Just want to make sure I'm on the right way. Please quickly go through the patch when you have time, and let me know if you have any concerns.
Attachment #8457890 - Flags: feedback?(masayuki)
Attachment #8457890 - Flags: feedback?(bugs)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #30)
> Besides, I got a few questions and I'd like to hear from you.
> - I'm not sure whether the name of webidl is clear nor not. Any suggestion?
BrowserElementKeyboardEvent is quite ok, since CrossProcessKeyboardEvent wouldn't
really be quite right.

I don't understand
readonly attribute boolean? embeddedCancelled;
Why boolean? and not just boolean.
And embeddedCancelled sure needs some documentation.

> - Regarding event names,
>   - should I use "mozbrowser" as prefix?
>   - which is better, "mozbrowserkeydown" or "mozbrowserafterkeydown"?
I don't have preference. Maybe the latter
Flags: needinfo?(bugs)
Comment on attachment 8457890 [details] [diff] [review]
Patch 1(v2): Create BrowserElementKeyboardEvent

>+rowserElementKeyboardEvent::BrowserElementKeyboardEvent(EventTarget* aOwner,
>+                                                         nsPresContext* aPresContext,
>+                                                         WidgetKeyboardEvent* aEvent)
>+  : KeyboardEvent(aOwner, aPresContext,
>+                  aEvent ? aEvent : new WidgetKeyboardEvent(false, 0, nullptr))
>+{
>+  MOZ_ASSERT(mEvent->eventStructType == NS_KEY_EVENT,
>+             "event type mismatch NS_KEY_EVENT");
>+  MOZ_ASSERT(mEvent->message == NS_KEY_BEFORE_DOWN ||
>+             mEvent->message == NS_KEY_BEFORE_UP ||
>+             mEvent->message == NS_KEY_AFTER_DOWN ||
>+             mEvent->message == NS_KEY_AFTER_UP,
>+             "event message mismatch");
This assert is wrong. Nothing guarantees those messages, for example when
aEvent is null, mEvent->message will be 0


>+    nsAutoString typeString;
>+    switch (mEvent->message) {
>+      case NS_KEY_BEFORE_DOWN:
>+        typeString.AssignLiteral("mozbrowserbeforekeydown");
>+        break;
>+      case NS_KEY_BEFORE_UP:
>+        typeString.AssignLiteral("mozbrowserbeforekeyup");
>+        break;
>+      case NS_KEY_AFTER_DOWN:
>+        typeString.AssignLiteral("mozbrowserkeydown");
>+        break;
>+      case NS_KEY_AFTER_UP:
>+        typeString.AssignLiteral("mozbrowserkeyup");
>+        break;
>+      default:
>+        break;
>+    }
>+    mEvent->typeString = typeString;
>+    nsCOMPtr<nsIAtom> atom = do_GetAtom(NS_LITERAL_STRING("on") + typeString);
>+    mEvent->userType = atom;
This all shouldn't be needed.
typeString should be updated automatically when event.type is used, 
and usertype when Event::SetEventType is called.



>+    if (mEvent->message == NS_KEY_AFTER_DOWN ||
>+        mEvent->message == NS_KEY_AFTER_UP) {
>+      mEmbeddedCancelled.SetValue(aEvent->AsKeyboardEvent()->mFlags.mDefaultPrevented);
>+    }
oh, are we reusing the event struct? that won't work. Or maybe I'm missing something here.

(but in general looks ok)
Attachment #8457890 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8457890 [details] [diff] [review]
Patch 1(v2): Create BrowserElementKeyboardEvent

Oops, I should've pointed that you should export a patch with "-U 8 -P". "-U 4" isn't enough for reviewers.

>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h
>@@ -772,8 +772,12 @@ GK_ATOM(onmouseout, "onmouseout")
> GK_ATOM(onmouseover, "onmouseover")
> GK_ATOM(onMozMouseHittest, "onMozMouseHittest")
> GK_ATOM(onmouseup, "onmouseup")
> GK_ATOM(onMozAfterPaint, "onMozAfterPaint")
>+GK_ATOM(onmozbrowserbeforekeydown, "onmozbrowserbeforekeydown")
>+GK_ATOM(onmozbrowserbeforekeyup, "onmozbrowserbeforekeyup")
>+GK_ATOM(onmozbrowserkeydown, "onmozbrowserkeydown")
>+GK_ATOM(onmozbrowserkeyup, "onmozbrowserkeyup")
> GK_ATOM(onmozfullscreenchange, "onmozfullscreenchange")
> GK_ATOM(onmozfullscreenerror, "onmozfullscreenerror")
> GK_ATOM(onmozpointerlockchange, "onmozpointerlockchange")
> GK_ATOM(onmozpointerlockerror, "onmozpointerlockerror")

>diff --git a/dom/events/BrowserElementKeyboardEvent.cpp b/dom/events/BrowserElementKeyboardEvent.cpp
>+BrowserElementKeyboardEvent::BrowserElementKeyboardEvent(EventTarget* aOwner,
>+                                                         nsPresContext* aPresContext,
>+                                                         WidgetKeyboardEvent* aEvent)
>+  : KeyboardEvent(aOwner, aPresContext,
>+                  aEvent ? aEvent : new WidgetKeyboardEvent(false, 0, nullptr))
>+{
>+  MOZ_ASSERT(mEvent->eventStructType == NS_KEY_EVENT,
>+             "event type mismatch NS_KEY_EVENT");
>+  MOZ_ASSERT(mEvent->message == NS_KEY_BEFORE_DOWN ||
>+             mEvent->message == NS_KEY_BEFORE_UP ||
>+             mEvent->message == NS_KEY_AFTER_DOWN ||
>+             mEvent->message == NS_KEY_AFTER_UP,
>+             "event message mismatch");

MOZ_ASSERT() works only on debug build. So, in the cases which you don't assume would run on release build. And as smuag said, the message could be 0 if untrusted event is created with not proper value.

>+  if (aEvent) {

I think that you should check |!aEvent| here. And handle and return the case first. Then, following block can be outdented.

>+    mEventIsInternal = false;

This should've been initialized with Event's constructor (actually, Event::ConstructorInit).  So, you don't need to do this. Although, I don't understand why other Event classes do this and set time. We should remove the redundant code from every event class in another bug.

>+    switch (mEvent->message) {
>+      case NS_KEY_BEFORE_DOWN:
>+        typeString.AssignLiteral("mozbrowserbeforekeydown");
>+        break;
>+      case NS_KEY_BEFORE_UP:
>+        typeString.AssignLiteral("mozbrowserbeforekeyup");
>+        break;
>+      case NS_KEY_AFTER_DOWN:
>+        typeString.AssignLiteral("mozbrowserkeydown");
>+        break;
>+      case NS_KEY_AFTER_UP:
>+        typeString.AssignLiteral("mozbrowserkeyup");
>+        break;
>+      default:
>+        break;
>+    }
>+    mEvent->typeString = typeString;
>+    nsCOMPtr<nsIAtom> atom = do_GetAtom(NS_LITERAL_STRING("on") + typeString);

Maybe, userType is better name?

>+    mEvent->userType = atom;

This must be wrong because in the "default" case.

I think that above 3 lines and following if block should be performed only when |!typeString.IsEmpty()|.

>+
>+    if (mEvent->message == NS_KEY_AFTER_DOWN ||
>+        mEvent->message == NS_KEY_AFTER_UP) {
>+      mEmbeddedCancelled.SetValue(aEvent->AsKeyboardEvent()->mFlags.mDefaultPrevented);
>+    }
>+  } else {
>+    mEventIsInternal = true;
>+    mEvent->time = PR_Now();

So, I don't think that these lines necessary.

>+BrowserElementKeyboardEvent::Constructor(EventTarget* aOwner,
>+                                         const nsAString& aType,
>+                                         const BrowserElementKeyboardEventInit& aParam)

This line is over 80 characters. You should write this as:

>+BrowserElementKeyboardEvent::Constructor(
>+                               EventTarget* aOwner,
>+                               const nsAString& aType,
>+                               const BrowserElementKeyboardEventInit& aParam)

However, do we really need this and following method? In my understanding, these events shouldn't be created with event constructor in JS.

If you can build without Constructor in .webidl and these methods, please remove them. Otherwise, they should stay.

>+  nsRefPtr<BrowserElementKeyboardEvent> newEvent =
>+    new BrowserElementKeyboardEvent(aOwner, nullptr, nullptr);
>+  bool trusted = newEvent->Init(aOwner);
>+  newEvent->InitKeyEvent(aType, aParam.mBubbles, aParam.mCancelable,
>+                         aParam.mView, aParam.mCtrlKey, aParam.mAltKey,
>+                         aParam.mShiftKey, aParam.mMetaKey,
>+                         aParam.mKeyCode, aParam.mCharCode);
>+  newEvent->SetTrusted(trusted);

You don't initialize mEmbeddedCancelled...

And also, some values of KeyboardEventInit are not initialized here. E.g., .key and .code. Please check them if these methods should stay.

>+// static
>+already_AddRefed<BrowserElementKeyboardEvent>
>+BrowserElementKeyboardEvent::Constructor(const GlobalObject& aGlobal,
>+                                         const nsAString& aType,
>+                                         const BrowserElementKeyboardEventInit& aParam,
>+                                         ErrorResult& aRv)

Use same style above.

>+uint32_t
>+BrowserElementKeyboardEvent::CharCode()
>+{
>+  return 0;
>+}

This isn't correct if this event is created with constructor. You shouldn't override the attributes defined in KeyboardEvent.

>+
>+uint32_t
>+BrowserElementKeyboardEvent::KeyCode()
>+{
>+  return mEvent->AsKeyboardEvent()->keyCode;
>+}
>+
>+uint32_t
>+BrowserElementKeyboardEvent::Which()
>+{
>+  return KeyCode();
>+}

So, you should remove them.

>+nsresult
>+NS_NewDOMBrowserElementKeyboardEvent(nsIDOMEvent** aInstancePtrResult,
>+                                     EventTarget* aOwner,
>+                                     nsPresContext* aPresContext,
>+                                     WidgetKeyboardEvent *aEvent)
>+{
>+  BrowserElementKeyboardEvent *it =

Move * to the end of the type.

>diff --git a/dom/events/BrowserElementKeyboardEvent.h b/dom/events/BrowserElementKeyboardEvent.h
>+  virtual uint32_t CharCode() MOZ_OVERRIDE;
>+  virtual uint32_t KeyCode() MOZ_OVERRIDE;
>+  virtual uint32_t Which() MOZ_OVERRIDE;

Remove them.

>diff --git a/dom/events/EventNameList.h b/dom/events/EventNameList.h
>index 83702a6..7265637 100644
>--- a/dom/events/EventNameList.h
>+++ b/dom/events/EventNameList.h
>@@ -623,18 +623,18 @@ NON_IDL_EVENT(DOMFocusIn,
> NON_IDL_EVENT(DOMFocusOut,
>               NS_UI_FOCUSOUT,
>               EventNameType_HTMLXUL,
>               NS_UI_EVENT)
>-                                  
>+

You shouldn't do this here because this line isn't related to this bug.

> NON_IDL_EVENT(MozMousePixelScroll,
>               NS_MOUSE_PIXEL_SCROLL,
>               EventNameType_HTMLXUL,
>               NS_MOUSE_SCROLL_EVENT)
>-                                                
>+

Same.

>+NON_IDL_EVENT(mozbrowserbeforekeydown,
>+              NS_KEY_BEFORE_DOWN,
>+              EventNameType_All,
>+              NS_KEY_EVENT)
>+NON_IDL_EVENT(mozbrowserkeydown,
>+              NS_KEY_AFTER_DOWN,
>+              EventNameType_All,
>+              NS_KEY_EVENT)
>+NON_IDL_EVENT(mozbrowserbeforekeyup,
>+              NS_KEY_BEFORE_UP,
>+              EventNameType_All,
>+              NS_KEY_EVENT)
>+NON_IDL_EVENT(mozbrowserkeyup,
>+              NS_KEY_AFTER_UP,
>+              EventNameType_All,
>+              NS_KEY_EVENT)

Isn't EventNameType_All allows to write onmozbrowserbeforekeydown in HTML document? If so, is it really necessary?

>diff --git a/dom/webidl/BrowserElementKeyboardEvent.webidl b/dom/webidl/BrowserElementKeyboardEvent.webidl
>+[Constructor(DOMString typeArg, optional BrowserElementKeyboardEventInit keyboardEventInitDict)]

I guess that we don't need this line and BrowserElementKeyboardEventInit. Although, I'm not 100% sure.

>diff --git a/widget/BasicEvents.h b/widget/BasicEvents.h
> // Key is pressed within a window
>-#define NS_KEY_PRESS                    (NS_WINDOW_START + 31)
>-// Key is released within a window
>-#define NS_KEY_UP                       (NS_WINDOW_START + 32)
>+#define NS_KEY_BEFORE_DOWN              (NS_WINDOW_START + 31)
>+#define NS_KEY_DOWN                     (NS_WINDOW_START + 32)
>+#define NS_KEY_AFTER_DOWN               (NS_WINDOW_START + 33)
>+
> // Key is pressed within a window
>-#define NS_KEY_DOWN                     (NS_WINDOW_START + 33)
>+#define NS_KEY_PRESS                    (NS_WINDOW_START + 34)
>+
>+// Key is released within a window
>+#define NS_KEY_BEFORE_UP                (NS_WINDOW_START + 35)
>+#define NS_KEY_UP                       (NS_WINDOW_START + 36)
>+#define NS_KEY_AFTER_UP                 (NS_WINDOW_START + 37)

I think that you shouldn't change existing messages. Yes, nobody should be broken by this change. However, we don't need to take such risk in this bug. Anyway, we should redefine them as typed enum... (bug 895274)

You never called NS_NewDOMBrowserElementKeyboardEvent(). They should be called from EventDispatcher::CreateEvent(). However, as I mentioned above, if this shouldn't be called from CreateEvent(), you should not define and implement it. (EventDispatcher::CreateEvent() is implementation of |document.createEvent("foo")| of JS.
Attachment #8457890 - Flags: feedback?(masayuki) → feedback-
(In reply to Olli Pettay [:smaug] from comment #32)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #30)
> > Besides, I got a few questions and I'd like to hear from you.
> > - I'm not sure whether the name of webidl is clear nor not. Any suggestion?
> BrowserElementKeyboardEvent is quite ok, since CrossProcessKeyboardEvent
> wouldn't
> really be quite right.
> 
> I don't understand
> readonly attribute boolean? embeddedCancelled;
> Why boolean? and not just boolean.
> And embeddedCancelled sure needs some documentation.

For "mozbrowserbefore*" events, all attributes are the same with original keyboard event, that is to say, no embeddedCancelled attribute is included.

However, for "mozbrowserafter*" events, an attribute named embeddedCancelled is added. Please refer to https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent to see more explanation for the attribute. I'll also add some simple comments here. Thanks for your suggestion.
(In reply to Olli Pettay [:smaug] from comment #33)
> Comment on attachment 8457890 [details] [diff] [review]
> Patch 1(v2): Create BrowserElementKeyboardEvent
> 
> >+rowserElementKeyboardEvent::BrowserElementKeyboardEvent(EventTarget* aOwner,
> >+                                                         nsPresContext* aPresContext,
> >+                                                         WidgetKeyboardEvent* aEvent)
> >+  : KeyboardEvent(aOwner, aPresContext,
> >+                  aEvent ? aEvent : new WidgetKeyboardEvent(false, 0, nullptr))
> >+{
> >+  MOZ_ASSERT(mEvent->eventStructType == NS_KEY_EVENT,
> >+             "event type mismatch NS_KEY_EVENT");
> >+  MOZ_ASSERT(mEvent->message == NS_KEY_BEFORE_DOWN ||
> >+             mEvent->message == NS_KEY_BEFORE_UP ||
> >+             mEvent->message == NS_KEY_AFTER_DOWN ||
> >+             mEvent->message == NS_KEY_AFTER_UP,
> >+             "event message mismatch");
> This assert is wrong. Nothing guarantees those messages, for example when
> aEvent is null, mEvent->message will be 0

I see.

> >+    mEvent->typeString = typeString;
> >+    nsCOMPtr<nsIAtom> atom = do_GetAtom(NS_LITERAL_STRING("on") + typeString);
> >+    mEvent->userType = atom;
> This all shouldn't be needed.
> typeString should be updated automatically when event.type is used, 
> and usertype when Event::SetEventType is called.

Thanks for pointing it out.

> >+    if (mEvent->message == NS_KEY_AFTER_DOWN ||
> >+        mEvent->message == NS_KEY_AFTER_UP) {
> >+      mEmbeddedCancelled.SetValue(aEvent->AsKeyboardEvent()->mFlags.mDefaultPrevented);
> >+    }
> oh, are we reusing the event struct? that won't work. Or maybe I'm missing
> something here.
> 
> (but in general looks ok)

Yes. the event struct of "NS_KEY_EVENT" is reused here since only one data member is added.

Why does it not work? Could you give me more details?
Target Milestone: --- → 2.1 S1 (1aug)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #35)
> However, for "mozbrowserafter*" events, an attribute named embeddedCancelled
> is added. Please refer to
> https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent to see more
> explanation for the attribute. I'll also add some simple comments here.
> Thanks for your suggestion.

But why you need nullability '?' with the attribute?
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #34)
> Oops, I should've pointed that you should export a patch with "-U 8 -P". "-U
> 4" isn't enough for reviewers.

Got it! I'll do it in later patches. ;)

> MOZ_ASSERT() works only on debug build. So, in the cases which you don't
> assume would run on release build. And as smuag said, the message could be 0
> if untrusted event is created with not proper value.
> 
> >+  if (aEvent) {
> 
> I think that you should check |!aEvent| here. And handle and return the case
> first. Then, following block can be outdented.
> 

Agree.

> >+    mEventIsInternal = false;
> 
> This should've been initialized with Event's constructor (actually,
> Event::ConstructorInit).  So, you don't need to do this. Although, I don't
> understand why other Event classes do this and set time. We should remove
> the redundant code from every event class in another bug.
> 

I'll remove this in my patch and file a follow-up for removing it from the other event classes.

> >+BrowserElementKeyboardEvent::Constructor(EventTarget* aOwner,
> >+                                         const nsAString& aType,
> >+                                         const BrowserElementKeyboardEventInit& aParam)
> 
> This line is over 80 characters. You should write this as:
> 
> >+BrowserElementKeyboardEvent::Constructor(
> >+                               EventTarget* aOwner,
> >+                               const nsAString& aType,
> >+                               const BrowserElementKeyboardEventInit& aParam)
> 

Okay!

> However, do we really need this and following method? In my understanding,
> these events shouldn't be created with event constructor in JS.
> 
> If you can build without Constructor in .webidl and these methods, please
> remove them. Otherwise, they should stay.

Hmm... These events should not be created in JS. I remove the Constructor in webidl and remove these functions. Also, removing "NS_NewDOMBrowserElementKeyboardEvent" in nsIDOMEvent.idl.

> >+  nsRefPtr<BrowserElementKeyboardEvent> newEvent =
> >+    new BrowserElementKeyboardEvent(aOwner, nullptr, nullptr);
> >+  bool trusted = newEvent->Init(aOwner);
> >+  newEvent->InitKeyEvent(aType, aParam.mBubbles, aParam.mCancelable,
> >+                         aParam.mView, aParam.mCtrlKey, aParam.mAltKey,
> >+                         aParam.mShiftKey, aParam.mMetaKey,
> >+                         aParam.mKeyCode, aParam.mCharCode);
> >+  newEvent->SetTrusted(trusted);
> You don't initialize mEmbeddedCancelled...

Oops! Well, this part is going to be removed.

> And also, some values of KeyboardEventInit are not initialized here. E.g.,
> .key and .code. Please check them if these methods should stay.

Although I'm going to remove this part, I don't quite understand the above comments. Can you give me further details? and here's my understanding:

When |event.key| is invoked, I expect KeyboardEvent::GetKey(nsAString& aKeyName) is triggered and then WidgetKeyboarEvent::GetDOMKeyName(nsAString& aKeyName) is executed to set |aKeyName| to corresponding string based on |mKeyNameIndex|.

What else should be initialized?

> >+BrowserElementKeyboardEvent::CharCode()
> >+{
> >+  return 0;
> >+}
> 
> This isn't correct if this event is created with constructor. You shouldn't
> override the attributes defined in KeyboardEvent.
> 

I see.

> >+BrowserElementKeyboardEvent::KeyCode()
> >+{
> >+  return mEvent->AsKeyboardEvent()->keyCode;
> >+}
> >+
> >+uint32_t
> >+BrowserElementKeyboardEvent::Which()
> >+{
> >+  return KeyCode();
> >+}
> 
> So, you should remove them.

In KeyboardEvent::KeyCode(), we'll check event message and only NS_KEY_UP / NS_KEY_PRESS / NS_KEY_DOWN are supported, so I'd like to override the function. Should I copy the implementation and add new event messages manually or modify the implementation of KeyboardEvent? I will remove these functions if we choose the latter.

> >+nsresult
> >+NS_NewDOMBrowserElementKeyboardEvent(nsIDOMEvent** aInstancePtrResult,
> >+                                     EventTarget* aOwner,
> >+                                     nsPresContext* aPresContext,
> >+                                     WidgetKeyboardEvent *aEvent)
> >+{
> >+  BrowserElementKeyboardEvent *it =
> 
> Move * to the end of the type.

Okay!

> >diff --git a/dom/events/EventNameList.h b/dom/events/EventNameList.h
> >index 83702a6..7265637 100644
> >--- a/dom/events/EventNameList.h
> >+++ b/dom/events/EventNameList.h
> >@@ -623,18 +623,18 @@ NON_IDL_EVENT(DOMFocusIn,
> > NON_IDL_EVENT(DOMFocusOut,
> >               NS_UI_FOCUSOUT,
> >               EventNameType_HTMLXUL,
> >               NS_UI_EVENT)
> >-                                  
> >+
> 
> You shouldn't do this here because this line isn't related to this bug.

I see.

> 
> >+NON_IDL_EVENT(mozbrowserbeforekeydown,
> >+              NS_KEY_BEFORE_DOWN,
> >+              EventNameType_All,
> >+              NS_KEY_EVENT)
> >+NON_IDL_EVENT(mozbrowserkeydown,
> >+              NS_KEY_AFTER_DOWN,
> >+              EventNameType_All,
> >+              NS_KEY_EVENT)
> >+NON_IDL_EVENT(mozbrowserbeforekeyup,
> >+              NS_KEY_BEFORE_UP,
> >+              EventNameType_All,
> >+              NS_KEY_EVENT)
> >+NON_IDL_EVENT(mozbrowserkeyup,
> >+              NS_KEY_AFTER_UP,
> >+              EventNameType_All,
> >+              NS_KEY_EVENT)
> 
> Isn't EventNameType_All allows to write onmozbrowserbeforekeydown in HTML
> document? If so, is it really necessary?

How about EventNameType_HTMLXUL?

> >diff --git a/dom/webidl/BrowserElementKeyboardEvent.webidl b/dom/webidl/BrowserElementKeyboardEvent.webidl
> >+[Constructor(DOMString typeArg, optional BrowserElementKeyboardEventInit keyboardEventInitDict)]
> 
> I guess that we don't need this line and BrowserElementKeyboardEventInit.
> Although, I'm not 100% sure.

yep, I think you're right. ;)

> >diff --git a/widget/BasicEvents.h b/widget/BasicEvents.h
> > // Key is pressed within a window
> >-#define NS_KEY_PRESS                    (NS_WINDOW_START + 31)
> >-// Key is released within a window
> >-#define NS_KEY_UP                       (NS_WINDOW_START + 32)
> >+#define NS_KEY_BEFORE_DOWN              (NS_WINDOW_START + 31)
> >+#define NS_KEY_DOWN                     (NS_WINDOW_START + 32)
> >+#define NS_KEY_AFTER_DOWN               (NS_WINDOW_START + 33)
> >+
> > // Key is pressed within a window
> >-#define NS_KEY_DOWN                     (NS_WINDOW_START + 33)
> >+#define NS_KEY_PRESS                    (NS_WINDOW_START + 34)
> >+
> >+// Key is released within a window
> >+#define NS_KEY_BEFORE_UP                (NS_WINDOW_START + 35)
> >+#define NS_KEY_UP                       (NS_WINDOW_START + 36)
> >+#define NS_KEY_AFTER_UP                 (NS_WINDOW_START + 37)
> 
> I think that you shouldn't change existing messages. Yes, nobody should be
> broken by this change. However, we don't need to take such risk in this bug.
> Anyway, we should redefine them as typed enum... (bug 895274)
> 

Okay! I'll keep the existing messages and add new event messages for new events.

> You never called NS_NewDOMBrowserElementKeyboardEvent(). They should be
> called from EventDispatcher::CreateEvent(). However, as I mentioned above,
> if this shouldn't be called from CreateEvent(), you should not define and
> implement it. (EventDispatcher::CreateEvent() is implementation of
> |document.createEvent("foo")| of JS.

Actually, NS_NewDOMBrowserElementKeyboardEvent is invoked in EventDispatcher::CreateEvent(), and I put it in latter patches. However, I didn't notice EventDispatcher::CreateEvent) is the implementation of |document.createEvent()| of JS....  Currently, there's no use case that we want to create this event from JS. Should we prevent user creating it from JS? Anyway to do so?
Flags: needinfo?(masayuki)
(In reply to Olli Pettay [:smaug] from comment #37)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #35)
> > However, for "mozbrowserafter*" events, an attribute named embeddedCancelled
> > is added. Please refer to
> > https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent to see more
> > explanation for the attribute. I'll also add some simple comments here.
> > Thanks for your suggestion.
> 
> But why you need nullability '?' with the attribute?

Are you saying that use KeyboardEvent as the interface of "mozbrowserbeforekey*" events? If so, we don't need to make the attribute nullable, but KeyboardEvent should support events with message of NS_KEY_BEFORE_DOWN and NS_KEY_BEFORE_UP.

I'd like to make BrowserElementKeyboardEvent as the interface of all "mozbrowser*key*" events. That is to say, 
the implementation of these events will be kept in the same class. Another benefit I can think is that it's easier to add new functions for all "mozbrowser*key*" events. (Just add them to interface BrowserElementKeyboardEvent and keep the implementation in one class.) However, we don't have this use case for now, maybe we can do it as necessary.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #39)
> Are you saying that use KeyboardEvent as the interface of
> "mozbrowserbeforekey*" events?
No. I'm just talking about the question mark in 
in the type of BrowserElementKeyboardEvent's embeddedCancelled attribute.
Why do we need to support values
event.embeddedCancelled == true
event.embeddedCancelled == false
event.embeddedCancelled == null
why not just the first two ones?
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #38)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #34)
> > And also, some values of KeyboardEventInit are not initialized here. E.g.,
> > .key and .code. Please check them if these methods should stay.
> 
> Although I'm going to remove this part, I don't quite understand the above
> comments. Can you give me further details? and here's my understanding:
> 
> When |event.key| is invoked, I expect KeyboardEvent::GetKey(nsAString&
> aKeyName) is triggered and then WidgetKeyboarEvent::GetDOMKeyName(nsAString&
> aKeyName) is executed to set |aKeyName| to corresponding string based on
> |mKeyNameIndex|.
> 
> What else should be initialized?

Well, you should NOT do that yourself. Instead, you should use the Constructor of dom::KeyboardEvent. Then, you should do additional initialization only for the new members.

> > >+BrowserElementKeyboardEvent::KeyCode()
> > >+{
> > >+  return mEvent->AsKeyboardEvent()->keyCode;
> > >+}
> > >+
> > >+uint32_t
> > >+BrowserElementKeyboardEvent::Which()
> > >+{
> > >+  return KeyCode();
> > >+}
> > 
> > So, you should remove them.
> 
> In KeyboardEvent::KeyCode(), we'll check event message and only NS_KEY_UP /
> NS_KEY_PRESS / NS_KEY_DOWN are supported, so I'd like to override the
> function. Should I copy the implementation and add new event messages
> manually or modify the implementation of KeyboardEvent? I will remove these
> functions if we choose the latter.

I don't think that you need to override them because current behavior is not invalid at least for trusted events. They just sanitize strange member values.

On the other hand, if an event instance is created by constructor (new KeyboardEvent("keydown", { foo: bar... })), all initialized values are used without the check.

So, if you need to unlock the check, the event dispatchers must dispatch wrong events.

> > >+NON_IDL_EVENT(mozbrowserbeforekeydown,
> > >+              NS_KEY_BEFORE_DOWN,
> > >+              EventNameType_All,
> > >+              NS_KEY_EVENT)
> > >+NON_IDL_EVENT(mozbrowserkeydown,
> > >+              NS_KEY_AFTER_DOWN,
> > >+              EventNameType_All,
> > >+              NS_KEY_EVENT)
> > >+NON_IDL_EVENT(mozbrowserbeforekeyup,
> > >+              NS_KEY_BEFORE_UP,
> > >+              EventNameType_All,
> > >+              NS_KEY_EVENT)
> > >+NON_IDL_EVENT(mozbrowserkeyup,
> > >+              NS_KEY_AFTER_UP,
> > >+              EventNameType_All,
> > >+              NS_KEY_EVENT)
> > 
> > Isn't EventNameType_All allows to write onmozbrowserbeforekeydown in HTML
> > document? If so, is it really necessary?
> 
> How about EventNameType_HTMLXUL?

If so, it sounds like <input onmozbrowserbeforekeydown="doSometing();"> works. I don't think that it should work...

> > You never called NS_NewDOMBrowserElementKeyboardEvent(). They should be
> > called from EventDispatcher::CreateEvent(). However, as I mentioned above,
> > if this shouldn't be called from CreateEvent(), you should not define and
> > implement it. (EventDispatcher::CreateEvent() is implementation of
> > |document.createEvent("foo")| of JS.
> 
> Actually, NS_NewDOMBrowserElementKeyboardEvent is invoked in
> EventDispatcher::CreateEvent(), and I put it in latter patches. However, I
> didn't notice EventDispatcher::CreateEvent) is the implementation of
> |document.createEvent()| of JS....  Currently, there's no use case that we
> want to create this event from JS. Should we prevent user creating it from
> JS? Anyway to do so?

If you don't allow to create untrusted events of the new key with:

var event = document.createEvent("BrowserElementKeyboardEvent");

Then, you don't need to change the bottom half of the method.

But for creating a DOM event from Widget*Event, you need to change the top half of the method. At this time, NS_NewDOMBrowserElementKeyboardEvent() is necessary. So, you shouldn't remove NS_NewDOMBrowserElementKeyboardEvent() from next patch. Sorry, I took mistake about this. NS_NewDOMBrowserElementKeyboardEvent() is necessary even if you don't allow to create the instance from JS.
BTW, we should write a guide to implement new DOM event in MDN or mozilla wiki...
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #42)
> BTW, we should write a guide to implement new DOM event in MDN or mozilla
> wiki...

That would be wonderful ;)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #42)
> BTW, we should write a guide to implement new DOM event in MDN or mozilla
> wiki...
Yes. Although usually event codegen is enough.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #41)
> I don't think that you need to override them because current behavior is not
> invalid at least for trusted events. They just sanitize strange member
> values.
> 
> On the other hand, if an event instance is created by constructor (new
> KeyboardEvent("keydown", { foo: bar... })), all initialized values are used
> without the check.
> 
> So, if you need to unlock the check, the event dispatchers must dispatch
> wrong events.

Ok. I'm going to add new event messages in KeyboardEvent.

> If so, it sounds like <input onmozbrowserbeforekeydown="doSometing();">
> works. I don't think that it should work...

Actually, only mozbrowser element will receive these events.

I've checked enum in EventNameType nsContentUtils.h.  Does EventNameType_HTMLBodyOrFramesetOnly make sense to you?

> If you don't allow to create untrusted events of the new key with:
> 
> var event = document.createEvent("BrowserElementKeyboardEvent");
> 
> Then, you don't need to change the bottom half of the method.
> 
> But for creating a DOM event from Widget*Event, you need to change the top
> half of the method. At this time, NS_NewDOMBrowserElementKeyboardEvent() is
> necessary. So, you shouldn't remove NS_NewDOMBrowserElementKeyboardEvent()
> from next patch. Sorry, I took mistake about this.
> NS_NewDOMBrowserElementKeyboardEvent() is necessary even if you don't allow
> to create the instance from JS.

Thanks for you detailed information.
Update WIP patch.
Attachment #8457890 - Attachment is obsolete: true
Attachment #8460139 - Attachment is obsolete: true
It has been modified based on the feedbacks.

- Rename "mozbrowserkeydown" to "mozbrowserafterkeydown".
- No override existing event messages.
- Use EventNameType::EventNameType_HTMLBodyOrFramesetOnly.
- Add utils function like IsKeyDownEvent() and IsKeyUpEvent().
Attachment #8460144 - Attachment is obsolete: true
Attach WIP patches to see the whole picture more easily.

Feel free to let me know if you have any concerns regarding to current implementation.
Comment on attachment 8462462 [details] [diff] [review]
Patch 1(v3): Create BrowserElementKeyboardEvent.webidl

Thanks for your valuable feedbacks. The patch has been updated, and please help to review it again.
Attachment #8462462 - Flags: review?(masayuki)
Attachment #8462462 - Flags: review?(bugs)
Comment on attachment 8462462 [details] [diff] [review]
Patch 1(v3): Create BrowserElementKeyboardEvent.webidl

I don't understand why mEmbeddedCancelled is added to WidgetKeyboardEvent.
WidgetKeyboardEvent maps to KeyboardEvent in EventDispatcher::CreateEvent, but
you want BrowserElementKeyboardEvent to be created.
So, please don't add such inconsistency to EventDispatcher::CreateEvent.
No other event interface has such.

You may want to add a new event struct which inherits WidgetKeyboardEvent.
Attachment #8462462 - Flags: review?(bugs) → review-
Comment on attachment 8462462 [details] [diff] [review]
Patch 1(v3): Create BrowserElementKeyboardEvent.webidl

>+nsresult
>+NS_NewDOMBrowserElementKeyboardEvent(nsIDOMEvent** aInstancePtrResult,
>+                                     EventTarget* aOwner,
>+                                     nsPresContext* aPresContext,
>+                                     WidgetKeyboardEvent *aEvent)

nit: WidgetKeyboardEvent* aEvent (the position of *)

>+NON_IDL_EVENT(mozbrowserbeforekeydown,
>+              NS_KEY_BEFORE_DOWN,
>+              EventNameType_HTMLBodyOrFramesetOnly,
>+              NS_KEY_EVENT)
>+NON_IDL_EVENT(mozbrowserafterkeydown,
>+              NS_KEY_AFTER_DOWN,
>+              EventNameType_HTMLBodyOrFramesetOnly,
>+              NS_KEY_EVENT)
>+NON_IDL_EVENT(mozbrowserbeforekeyup,
>+              NS_KEY_BEFORE_UP,
>+              EventNameType_HTMLBodyOrFramesetOnly,
>+              NS_KEY_EVENT)
>+NON_IDL_EVENT(mozbrowserafterkeyup,
>+              NS_KEY_AFTER_UP,
>+              EventNameType_HTMLBodyOrFramesetOnly,
>+              NS_KEY_EVENT)

Although, I'm not familiar with this. Smaug, specifying EventNameType_HTMLBodyOrFramesetOnly is okay?

>@@ -281,32 +281,40 @@ KeyboardEvent::KeyCode()
>   if (mInitializedByCtor) {
>     return mEvent->AsKeyboardEvent()->keyCode;
>   }
> 
>   switch (mEvent->message) {
>   case NS_KEY_UP:
>   case NS_KEY_PRESS:
>   case NS_KEY_DOWN:
>+  case NS_KEY_BEFORE_DOWN:
>+  case NS_KEY_BEFORE_UP:
>+  case NS_KEY_AFTER_DOWN:
>+  case NS_KEY_AFTER_UP:
>     return mEvent->AsKeyboardEvent()->keyCode;
>   }
>   return 0;
> }

This must be same as |if (!mEvent->message) {|. If keyboard event is created with unexpected event name, the message is 0. Otherwise, NS_KEY_*.

>+  bool IsKeyDownEvent() {
>+    return (message == NS_KEY_DOWN ||
>+            message == NS_KEY_BEFORE_DOWN ||
>+            message == NS_KEY_AFTER_DOWN);
>+  }
>+
>+  bool IsKeyUpEvent() {
>+     return (message == NS_KEY_UP ||
>+             message == NS_KEY_BEFORE_UP ||
>+             message == NS_KEY_AFTER_UP);
>   }

I don't see the users of this. So, you should remove them.

> (In reply to Olli Pettay [:smaug] from comment #51)
>> Comment on attachment 8462462 [details] [diff] [review]
>> Patch 1(v3): Create BrowserElementKeyboardEvent.webidl
>> 
>> I don't understand why mEmbeddedCancelled is added to WidgetKeyboardEvent.
>> WidgetKeyboardEvent maps to KeyboardEvent in EventDispatcher::CreateEvent,
>> but
>> you want BrowserElementKeyboardEvent to be created.
>> So, please don't add such inconsistency to EventDispatcher::CreateEvent.
>> No other event interface has such.
>> 
>> You may want to add a new event struct which inherits WidgetKeyboardEvent.
> 

Hmm, indeed. For EventDispatcher::CreateEvent(), it should have another event struct type.

Although, I don't like but there is an approach which only changes eventStructType value. This approach defined NS_SVGZOOM_EVENT and NS_SMIL_TIME_EVENT.

Smaug, how do you think about this? I think that they also should have independent event class which just inherits their necessary classes. If you agree with this idea, I'll file a bug.

(And also could you check if specifying EventNameType_HTMLBodyOrFramesetOnly is okay?)
Attachment #8462462 - Flags: review?(masayuki) → feedback?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #52)
> >@@ -281,32 +281,40 @@ KeyboardEvent::KeyCode()
> >   switch (mEvent->message) {
> >   case NS_KEY_UP:
> >   case NS_KEY_PRESS:
> >   case NS_KEY_DOWN:
> >+  case NS_KEY_BEFORE_DOWN:
> >+  case NS_KEY_BEFORE_UP:
> >+  case NS_KEY_AFTER_DOWN:
> >+  case NS_KEY_AFTER_UP:
> >     return mEvent->AsKeyboardEvent()->keyCode;
> >   }
> >   return 0;
> > }
> 
> This must be same as |if (!mEvent->message) {|. If keyboard event is created
> with unexpected event name, the message is 0. Otherwise, NS_KEY_*.

I see. Thanks.

> >+  bool IsKeyDownEvent() {
> >+    return (message == NS_KEY_DOWN ||
> >+            message == NS_KEY_BEFORE_DOWN ||
> >+            message == NS_KEY_AFTER_DOWN);
> >+  }
> >+
> >+  bool IsKeyUpEvent() {
> >+     return (message == NS_KEY_UP ||
> >+             message == NS_KEY_BEFORE_UP ||
> >+             message == NS_KEY_AFTER_UP);
> >   }
> 
> I don't see the users of this. So, you should remove them.

These functions are used in Patch 2. Let me move them to patch 2.

> > (In reply to Olli Pettay [:smaug] from comment #51)
> >> Comment on attachment 8462462 [details] [diff] [review]
> >> Patch 1(v3): Create BrowserElementKeyboardEvent.webidl
> >> 
> >> I don't understand why mEmbeddedCancelled is added to WidgetKeyboardEvent.
> >> WidgetKeyboardEvent maps to KeyboardEvent in EventDispatcher::CreateEvent,
> >> but
> >> you want BrowserElementKeyboardEvent to be created.
> >> So, please don't add such inconsistency to EventDispatcher::CreateEvent.
> >> No other event interface has such.
> >> 
> >> You may want to add a new event struct which inherits WidgetKeyboardEvent.
> > 
> 
> Hmm, indeed. For EventDispatcher::CreateEvent(), it should have another
> event struct type.
> 
> Although, I don't like but there is an approach which only changes
> eventStructType value. This approach defined NS_SVGZOOM_EVENT and
> NS_SMIL_TIME_EVENT.
> 
> Smaug, how do you think about this? I think that they also should have
> independent event class which just inherits their necessary classes. If you
> agree with this idea, I'll file a bug.
> 
> (And also could you check if specifying EventNameType_HTMLBodyOrFramesetOnly
> is okay?)

Okay! I'm going to add a new event struct for BrowserElementKeyboardEvent, so that we can keep consistency in EventDispatcher::CreateEvent().

As Masayuki said in the previous comment, may I change event struct from NS_KEY_EVENT to the new one in ctor of BrowserElementKeyboardEvent? If this is inappropriate, I can create a new class which inherits WidgetKeyboardEvent. Please let me know about it. Thanks.
> Although, I'm not familiar with this. Smaug, specifying
> EventNameType_HTMLBodyOrFramesetOnly is okay?
Looks wrong to me. I would expect these events to be not available for event handlers.
EventNameType_None perhaps.

> Although, I don't like but there is an approach which only changes eventStructType value. This approach defined NS_SVGZOOM_EVENT and > NS_SMIL_TIME_EVENT.
> 
> Smaug, how do you think about this? I think that they also should have independent event class which just inherits their necessary
> classes. If you agree with this idea, I'll file a bug.
SVGZOOM and SMIL_TIME are indeed very hackish, and should not change the struct type. It is just too error prone.
Attachment #8462462 - Flags: feedback?(bugs)
(In reply to Olli Pettay [:smaug] from comment #54)
> Looks wrong to me. I would expect these events to be not available for event
> handlers.
> EventNameType_None perhaps.

I see.

> > Although, I don't like but there is an approach which only changes eventStructType value. This approach defined NS_SVGZOOM_EVENT and > NS_SMIL_TIME_EVENT.
> > 
> > Smaug, how do you think about this? I think that they also should have independent event class which just inherits their necessary
> > classes. If you agree with this idea, I'll file a bug.
> SVGZOOM and SMIL_TIME are indeed very hackish, and should not change the
> struct type. It is just too error prone.

Okay. I will create a new class which inherites WidgetKeyboardEvent and add mEmbeddedCancelled in it. Thanks.
(In reply to Olli Pettay [:smaug] from comment #54)
> > Smaug, how do you think about this? I think that they also should have independent event class which just inherits their necessary
> > classes. If you agree with this idea, I'll file a bug.
> SVGZOOM and SMIL_TIME are indeed very hackish, and should not change the
> struct type. It is just too error prone.

I filed bug 1045978.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #38)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #34)
> > >+    mEventIsInternal = false;
> > 
> > This should've been initialized with Event's constructor (actually,
> > Event::ConstructorInit).  So, you don't need to do this. Although, I don't
> > understand why other Event classes do this and set time. We should remove
> > the redundant code from every event class in another bug.
> > 
> 
> I'll remove this in my patch and file a follow-up for removing it from the
> other event classes.

Oops, this is my mistake. You need to initialize mEventIsInternal in your new DOM event class because mEventIsInternal is always initialized true in super classes because their constructors receive dummy event class instance which is created by the most descendent class.

But I still think that you need to set time in your class.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #57)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #38)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #34)
> > > >+    mEventIsInternal = false;
> > > 
> > > This should've been initialized with Event's constructor (actually,
> > > Event::ConstructorInit).  So, you don't need to do this. Although, I don't
> > > understand why other Event classes do this and set time. We should remove
> > > the redundant code from every event class in another bug.
> > > 
> > 
> > I'll remove this in my patch and file a follow-up for removing it from the
> > other event classes.
> 
> Oops, this is my mistake. You need to initialize mEventIsInternal in your
> new DOM event class because mEventIsInternal is always initialized true in
> super classes because their constructors receive dummy event class instance
> which is created by the most descendent class.
> 
> But I still think that you need to set time in your class.

I also found some comments in Event.cpp describing how/why to set mEventIsInternal:
http://dxr.mozilla.org/mozilla-central/source/dom/events/Event.cpp#80
The document is very helpful. Thanks. :D
* Add new event message: NS_BEFORE_AFTER_KEY_EVENT
* Create class WidgetBeforeAfterKeyboardEvent inherited from WidgetKeyboardEvent
* Add new event message: NS_BEFORE_AFTER_KEY_EVENT
* Create class WidgetBeforeAfterKeyboardEvent inherited from WidgetKeyboardEvent


Smaug and Masayuki, could you take some time to review it? Thanks.
Attachment #8466083 - Attachment is obsolete: true
Attachment #8466122 - Flags: review?(masayuki)
Attachment #8466122 - Flags: review?(bugs)
* Rename event to BeforeAfterKeyboardEvent
* Set event name type to EventNameType_None
Attachment #8462462 - Attachment is obsolete: true
Attachment #8462464 - Attachment is obsolete: true
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #52)
> >@@ -281,32 +281,40 @@ KeyboardEvent::KeyCode()
> >   if (mInitializedByCtor) {
> >     return mEvent->AsKeyboardEvent()->keyCode;
> >   }
> > 
> >   switch (mEvent->message) {
> >   case NS_KEY_UP:
> >   case NS_KEY_PRESS:
> >   case NS_KEY_DOWN:
> >+  case NS_KEY_BEFORE_DOWN:
> >+  case NS_KEY_BEFORE_UP:
> >+  case NS_KEY_AFTER_DOWN:
> >+  case NS_KEY_AFTER_UP:
> >     return mEvent->AsKeyboardEvent()->keyCode;
> >   }
> >   return 0;
> > }
> 
> This must be same as |if (!mEvent->message) {|. If keyboard event is created
> with unexpected event name, the message is 0. Otherwise, NS_KEY_*.
> 

I just realized that I don't quite understand above comment. Here's my understanding, if a keyboard evnet is created with unexpected event name, the message is 0 and we'll return 0 when |keyboardEvent.keyCode| is called. After adding messages NS_KEY_[BEFORE|AFTER]_[DOWN|UP] in the switch statement here, the behaviour shouldn't be changed, right? Did I miss anything?
* Rename event to BeforeAfterKeyboardEvent
* Set event name type to EventNameType_None
* Support new event struct type

I separate the Widget*Event part from the original patch. This patch implements DOM event BeforeAfterKeyboardEvent.
Attachment #8466126 - Attachment is obsolete: true
Attachment #8466131 - Flags: review?(masayuki)
Attachment #8466131 - Flags: review?(bugs)
Comment on attachment 8466122 [details] [diff] [review]
Patch 1(v1): Implementation of WidgetBeforeAfterKeyboardEvent

>+NS_EVENT_CLASS(Widget, BeforeAfterKeyboardEvent)

I wonder is this dispatched from widget? If not so, the prefix should be "Internal".

>+  void AssignBeforeAfterKeyEventData(
>+                                   const WidgetBeforeAfterKeyboardEvent& aEvent,
>+                                   bool aCopyTargets)

nit: I think following style is better:

>  void AssignBeforeAfterKeyEventData(
>         const WidgetBeforeAfterKeyboardEvent& aEvent,
>         bool aCopyTargets)

(1 level indent from the start of the method name.)

>+  {
>+    AssignKeyEventData(aEvent, aCopyTargets);
>+
>+    mEmbeddedCancelled = aEvent.mEmbeddedCancelled;
>+  }
>+
>+  void AssignBeforeAfterKeyEventData(const WidgetKeyboardEvent& aEvent,
>+                                     bool aCopyTargets)
>+  {
>+    AssignKeyEventData(aEvent, aCopyTargets);
>+    mFlags = aEvent.mFlags;
>+
>+    // Override eventStructType and set mEmbeddedCancelled to default value.
>+    eventStructType = NS_BEFORE_AFTER_KEY_EVENT;

I don't understand why this is necessary? Isn't it enough MOZ_ASSERT()?

Otherwise, LGTM.
Attachment #8466122 - Flags: review?(masayuki) → review+
Comment on attachment 8466131 [details] [diff] [review]
Patch 2(v4): Implementation of BeforeAfterKeyboardEvent

>diff --git a/dom/events/BeforeAfterKeyboardEvent.cpp b/dom/events/BeforeAfterKeyboardEvent.cpp
>+++ b/dom/events/BeforeAfterKeyboardEvent.cpp
>+BeforeAfterKeyboardEvent::BeforeAfterKeyboardEvent(
>+                                         EventTarget* aOwner,
>+                                         nsPresContext* aPresContext,
>+                                         WidgetBeforeAfterKeyboardEvent* aEvent)
>+  : KeyboardEvent(aOwner, aPresContext,
>+                  aEvent ? aEvent : new WidgetBeforeAfterKeyboardEvent(false, 0, nullptr))
>+{
>+  if (!aEvent) {
>+    return;
>+  }

Hmm, you do this, but..

>+
>+  MOZ_ASSERT(mEvent->eventStructType == NS_BEFORE_AFTER_KEY_EVENT,
>+             "event type mismatch NS_BEFORE_AFTER_EVENT");
>+  MOZ_ASSERT(mEvent->message == NS_KEY_BEFORE_DOWN ||
>+             mEvent->message == NS_KEY_BEFORE_UP ||
>+             mEvent->message == NS_KEY_AFTER_DOWN ||
>+             mEvent->message == NS_KEY_AFTER_UP,
>+             "event message mismatch");
>+
>+  if (aEvent) {

Why do you need this check and the else block?

>+    mEventIsInternal = false;
>+
>+    if (mEvent->message == NS_KEY_AFTER_DOWN ||
>+        mEvent->message == NS_KEY_AFTER_UP) {
>+      bool value = aEvent->mFlags.mDefaultPrevented;
>+      mEmbeddedCancelled.SetValue(value);

How about:

>       mEmbeddedCancelled.SetValue(
>         aEvent->mFlags.mDefaultPrevented);

if the line is over 80 characters without the wrap.

>diff --git a/dom/events/BeforeAfterKeyboardEvent.h b/dom/events/BeforeAfterKeyboardEvent.h
>new file mode 100644
>index 0000000..a7354c3
>--- /dev/null
>+++ b/dom/events/BeforeAfterKeyboardEvent.h
>@@ -0,0 +1,42 @@
>+/* 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/.
>+ *
>+ * Portions Copyright 2013 Microsoft Open Technologies, Inc. */
>+
>+#ifndef mozilla_dom_BeforeAfterKeyboardEvent_h_
>+#define mozilla_dom_BeforeAfterKeyboardEvent_h_
>+
>+#include "mozilla/dom/KeyboardEvent.h"
>+#include "mozilla/dom/BeforeAfterKeyboardEventBinding.h"
>+
>+class nsPresContext;
>+
>+namespace mozilla {
>+namespace dom {
>+
>+class BeforeAfterKeyboardEvent : public KeyboardEvent
>+{
>+public:
>+  Nullable<bool> GetEmbeddedCancelled() const
>+  {
>+    return mEmbeddedCancelled;
>+  }
>+
>+private:
>+  Nullable<bool> mEmbeddedCancelled;
>+};

Hmm, then, why do we need to add mEmbeddedCancelled to the WidgetBeforeAfterKeyboardEvent (or InternalBeforeAfterKeyboardEvent?)

I guess that you don't need to add mEmbeddedCancelled to the new event class but you still need to create it for creating the this new DOM event from EventDispatcher::CreateEvent().

>-[builtinclass, uuid(02d54f52-a1f5-4ad2-b560-36f14012935e)]
>+[builtinclass, uuid(82303e4c-fcfe-4cc9-bbae-08f108271d07)]
> interface nsIDOMEvent : nsISupports
> {
>   // PhaseType
>   /**
>    * The event isn't being dispatched.
>    */
>   const unsigned short      NONE                           = 0;
>   /**

You must not do this because you don't change the interface. Just adding a new factory method.

And I cannot review the EventStateManager part because you don't include the dispatcher of the event. So, I cannot judge adding new messages to each switch statement.

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #64)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #52)
> > >@@ -281,32 +281,40 @@ KeyboardEvent::KeyCode()
> > >   if (mInitializedByCtor) {
> > >     return mEvent->AsKeyboardEvent()->keyCode;
> > >   }
> > > 
> > >   switch (mEvent->message) {
> > >   case NS_KEY_UP:
> > >   case NS_KEY_PRESS:
> > >   case NS_KEY_DOWN:
> > >+  case NS_KEY_BEFORE_DOWN:
> > >+  case NS_KEY_BEFORE_UP:
> > >+  case NS_KEY_AFTER_DOWN:
> > >+  case NS_KEY_AFTER_UP:
> > >     return mEvent->AsKeyboardEvent()->keyCode;
> > >   }
> > >   return 0;
> > > }
> > 
> > This must be same as |if (!mEvent->message) {|. If keyboard event is created
> > with unexpected event name, the message is 0. Otherwise, NS_KEY_*.
> > 
> 
> I just realized that I don't quite understand above comment. Here's my
> understanding, if a keyboard evnet is created with unexpected event name,
> the message is 0 and we'll return 0 when |keyboardEvent.keyCode| is called.
> After adding messages NS_KEY_[BEFORE|AFTER]_[DOWN|UP] in the switch
> statement here, the behaviour shouldn't be changed, right? Did I miss
> anything?

You're right. But I don't think that it causes actual problem. And the check is ignored if web applications use event constructor such as |var event = new KeyboardEvent("foo", { keyCode: 128, charCode: 256 })|.
Attachment #8466131 - Flags: review?(masayuki) → review-
Comment on attachment 8466131 [details] [diff] [review]
Patch 2(v4): Implementation of BeforeAfterKeyboardEvent

I would have given similar comments as masayuki.
I'll review the updated patch.
Attachment #8466131 - Flags: review?(bugs)
Comment on attachment 8466122 [details] [diff] [review]
Patch 1(v1): Implementation of WidgetBeforeAfterKeyboardEvent

>+  void AssignBeforeAfterKeyEventData(const WidgetKeyboardEvent& aEvent,
>+                                     bool aCopyTargets)
>+  {
>+    AssignKeyEventData(aEvent, aCopyTargets);
>+    mFlags = aEvent.mFlags;
>+
>+    // Override eventStructType and set mEmbeddedCancelled to default value.
>+    eventStructType = NS_BEFORE_AFTER_KEY_EVENT;
Ah, so we basically copy keyboardevent to beforeafterkeyevent.
Should be fine, but need to see where this is actually used.


>+  bool IsKeyDownEvent() {
{ should be on its own line.
same also with some other methods
Attachment #8466122 - Flags: review?(bugs) → review+
FYI: I refactored nsEventStructType. See bug 1046101. Now, its values are generatable with EventClassList.h and something related types, variables and methods are renamed. See part.1 and part.33 of the landed patches.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e08b99faf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0cfff4ee625
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #66)
> >+NS_EVENT_CLASS(Widget, BeforeAfterKeyboardEvent)
> 
> I wonder is this dispatched from widget? If not so, the prefix should be
> "Internal".

Looks good to me. We should use "Internal" in this case.

Note that mEmbeddedCancelled is removed from the class according to review comments of patch 2.

> >+  void AssignBeforeAfterKeyEventData(const WidgetKeyboardEvent& aEvent,
> >+                                     bool aCopyTargets)
> >+  {
> >+    AssignKeyEventData(aEvent, aCopyTargets);
> >+    mFlags = aEvent.mFlags;
> >+
> >+    // Override eventStructType and set mEmbeddedCancelled to default value.
> >+    eventStructType = NS_BEFORE_AFTER_KEY_EVENT;
> 
> I don't understand why this is necessary? Isn't it enough MOZ_ASSERT()?

Event struct type is overriden to NS_KEY_EVENT in function AssignKeyEventData(). That's why I reset the value here.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #70)
> FYI: I refactored nsEventStructType. See bug 1046101. Now, its values are
> generatable with EventClassList.h and something related types, variables and
> methods are renamed. See part.1 and part.33 of the landed patches.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e08b99faf8
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c0cfff4ee625

Thank you for your kind reminder. I'll rebase my patch later.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #71)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #66)
> > >+  void AssignBeforeAfterKeyEventData(const WidgetKeyboardEvent& aEvent,
> > >+                                     bool aCopyTargets)
> > >+  {
> > >+    AssignKeyEventData(aEvent, aCopyTargets);
> > >+    mFlags = aEvent.mFlags;
> > >+
> > >+    // Override eventStructType and set mEmbeddedCancelled to default value.
> > >+    eventStructType = NS_BEFORE_AFTER_KEY_EVENT;
> > 
> > I don't understand why this is necessary? Isn't it enough MOZ_ASSERT()?
> 
> Event struct type is overriden to NS_KEY_EVENT in function
> AssignKeyEventData(). That's why I reset the value here.

Ah, I see. But the reason why you need it is now removed. So, you don't need to do it ;-)
https://hg.mozilla.org/integration/mozilla-inbound/diff/b6e08b99faf8/widget/BasicEvents.h
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #73)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #71)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #66)
> > > >+  void AssignBeforeAfterKeyEventData(const WidgetKeyboardEvent& aEvent,
> > > >+                                     bool aCopyTargets)
> > > >+  {
> > > >+    AssignKeyEventData(aEvent, aCopyTargets);
> > > >+    mFlags = aEvent.mFlags;
> > > >+
> > > >+    // Override eventStructType and set mEmbeddedCancelled to default value.
> > > >+    eventStructType = NS_BEFORE_AFTER_KEY_EVENT;
> > > 
> > > I don't understand why this is necessary? Isn't it enough MOZ_ASSERT()?
> > 
> > Event struct type is overriden to NS_KEY_EVENT in function
> > AssignKeyEventData(). That's why I reset the value here.
> 
> Ah, I see. But the reason why you need it is now removed. So, you don't need
> to do it ;-)
> https://hg.mozilla.org/integration/mozilla-inbound/diff/b6e08b99faf8/widget/
> BasicEvents.h

Cool! I think it's time to rebase. ;)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #67)
> Comment on attachment 8466131 [details] [diff] [review]
> Patch 2(v4): Implementation of BeforeAfterKeyboardEvent
> >+class BeforeAfterKeyboardEvent : public KeyboardEvent
> >+{
> >+public:
> >+  Nullable<bool> GetEmbeddedCancelled() const
> >+  {
> >+    return mEmbeddedCancelled;
> >+  }
> >+
> >+private:
> >+  Nullable<bool> mEmbeddedCancelled;
> >+};
> 
> Hmm, then, why do we need to add mEmbeddedCancelled to the
> WidgetBeforeAfterKeyboardEvent (or InternalBeforeAfterKeyboardEvent?)
> 
> I guess that you don't need to add mEmbeddedCancelled to the new event class
> but you still need to create it for creating the this new DOM event from
> EventDispatcher::CreateEvent().

I've removed mEmbeddedCancelled from InternalBeforeAfterKeyboardEvent. However, I think we should keep mEmbeddedCancelled in BeforeAfterKeyboardEvent because the value should be cached and be returned/used in the getter function.

> >-[builtinclass, uuid(02d54f52-a1f5-4ad2-b560-36f14012935e)]
> >+[builtinclass, uuid(82303e4c-fcfe-4cc9-bbae-08f108271d07)]
> > interface nsIDOMEvent : nsISupports
> > {
> You must not do this because you don't change the interface. Just adding a
> new factory method.
> 
> And I cannot review the EventStateManager part because you don't include the
> dispatcher of the event. So, I cannot judge adding new messages to each
> switch statement.

I see. I'll move this part to the next patch.

> 
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #64)
> > (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #52)
> > > >@@ -281,32 +281,40 @@ KeyboardEvent::KeyCode()
> > > >   if (mInitializedByCtor) {
> > > >     return mEvent->AsKeyboardEvent()->keyCode;
> > > >   }
> > > > 
> > > >   switch (mEvent->message) {
> > > >   case NS_KEY_UP:
> > > >   case NS_KEY_PRESS:
> > > >   case NS_KEY_DOWN:
> > > >+  case NS_KEY_BEFORE_DOWN:
> > > >+  case NS_KEY_BEFORE_UP:
> > > >+  case NS_KEY_AFTER_DOWN:
> > > >+  case NS_KEY_AFTER_UP:
> > > >     return mEvent->AsKeyboardEvent()->keyCode;
> > > >   }
> > > >   return 0;
> > > > }
> > > 
> > > This must be same as |if (!mEvent->message) {|. If keyboard event is created
> > > with unexpected event name, the message is 0. Otherwise, NS_KEY_*.
> > > 
> > 
> > I just realized that I don't quite understand above comment. Here's my
> > understanding, if a keyboard evnet is created with unexpected event name,
> > the message is 0 and we'll return 0 when |keyboardEvent.keyCode| is called.
> > After adding messages NS_KEY_[BEFORE|AFTER]_[DOWN|UP] in the switch
> > statement here, the behaviour shouldn't be changed, right? Did I miss
> > anything?
> 
> You're right. But I don't think that it causes actual problem. And the check
> is ignored if web applications use event constructor such as |var event =
> new KeyboardEvent("foo", { keyCode: 128, charCode: 256 })|.

Thanks for your clarification. :)
QA Whiteboard: [2.1-feature-qa+]
Patch with nits addressed. Note that mEmbeddedCancelled is removed from WidgetBeforeAfterKeyboardEvent.
Attachment #8466122 - Attachment is obsolete: true
Review comments are addressed in this patch. Also, I've rebased it to the latest m-c.
Attachment #8466131 - Attachment is obsolete: true
Some changes are moved to patch 3. It makes more sense to review them with other changes in patch 3.
Attachment #8467695 - Attachment is obsolete: true
Attachment #8467709 - Flags: review?(masayuki)
Attachment #8467709 - Flags: review?(bugs)
This patch haven't been polished yet, but it might be easier for reviewers to see how it works.
Sorry! Wrong attachment. Please review the new one. Thanks.
Attachment #8467709 - Attachment is obsolete: true
Attachment #8467709 - Flags: review?(masayuki)
Attachment #8467709 - Flags: review?(bugs)
Attachment #8467717 - Flags: review?(masayuki)
Attachment #8467717 - Flags: review?(bugs)
Comment on attachment 8467711 [details] [diff] [review]
Patch 3(WIP): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

>--- a/content/base/src/nsGkAtomList.h
>+++ b/content/base/src/nsGkAtomList.h
>@@ -772,16 +772,20 @@ GK_ATOM(onmousedown, "onmousedown")
> GK_ATOM(onmouseenter, "onmouseenter")
> GK_ATOM(onmouseleave, "onmouseleave")
> GK_ATOM(onmousemove, "onmousemove")
> GK_ATOM(onmouseout, "onmouseout")
> GK_ATOM(onmouseover, "onmouseover")
> GK_ATOM(onMozMouseHittest, "onMozMouseHittest")
> GK_ATOM(onmouseup, "onmouseup")
> GK_ATOM(onMozAfterPaint, "onMozAfterPaint")
>+GK_ATOM(onmozbrowserafterkeydown, "onmozbrowserafterkeydown")
>+GK_ATOM(onmozbrowserafterkeyup, "onmozbrowserafterkeyup")
>+GK_ATOM(onmozbrowserbeforekeydown, "onmozbrowserbeforekeydown")
>+GK_ATOM(onmozbrowserbeforekeyup, "onmozbrowserbeforekeyup")

You define attributes of the event handlers...

>diff --git a/dom/events/EventNameList.h b/dom/events/EventNameList.h
>index 607ce7b..c0d99f1 100644
>--- a/dom/events/EventNameList.h
>+++ b/dom/events/EventNameList.h
>@@ -232,16 +232,32 @@ EVENT(keydown,
> EVENT(keypress,
>       NS_KEY_PRESS,
>       EventNameType_HTMLXUL,
>       eKeyboardEventClass)
> EVENT(keyup,
>       NS_KEY_UP,
>       EventNameType_HTMLXUL,
>       eKeyboardEventClass)
>+NON_IDL_EVENT(mozbrowserbeforekeydown,
>+              NS_KEY_BEFORE_DOWN,
>+              EventNameType_None,
>+              eBeforeAfterKeyboardEventClass)
>+NON_IDL_EVENT(mozbrowserafterkeydown,
>+              NS_KEY_AFTER_DOWN,
>+              EventNameType_None,
>+              eBeforeAfterKeyboardEventClass)
>+NON_IDL_EVENT(mozbrowserbeforekeyup,
>+              NS_KEY_BEFORE_UP,
>+              EventNameType_None,
>+              eBeforeAfterKeyboardEventClass)
>+NON_IDL_EVENT(mozbrowserafterkeyup,
>+              NS_KEY_AFTER_UP,
>+              EventNameType_None,
>+              eBeforeAfterKeyboardEventClass)

But you agree with they shouldn't be available with event handler attributes. So, the atom definition should be removed.

> bool
> TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
> {
>   NS_ENSURE_TRUE(mFrameElement, true);
> 
>-  WidgetKeyboardEvent localEvent(event);
>-  // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>-  // being infinitely redispatched and forwarded to the child again.
>-  localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
>-
>   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>   // to be able to dispatch it to the <browser> element as the target element.
>   nsIDocument* doc = mFrameElement->OwnerDoc();
>   nsIPresShell* presShell = doc->GetShell();
>   NS_ENSURE_TRUE(presShell, true);
>   nsPresContext* presContext = presShell->GetPresContext();
>   NS_ENSURE_TRUE(presContext, true);
> 
>-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+  WidgetKeyboardEvent localEvent(event);
>+  if (event.message == NS_KEY_DOWN ||
>+      event.message == NS_KEY_UP) {
>+    // Dispatch 'mozbrowserkeydown'/'mozbrowserkeyup' for out-of-process case.
>+    localEvent.message = (event.message == NS_KEY_DOWN) ?
>+                         NS_KEY_AFTER_DOWN : NS_KEY_AFTER_UP;
>+    presShell->DispatchKeyboardEvent(mFrameElement, &localEvent);
>+  } else {
>+    // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>+    // being infinitely redispatched and forwarded to the child again.
>+    localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
>+
>+    EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+  }
>   return true;
> }
> 
> /**
>  * Try to answer query event using cached text.
>  *
>  * For NS_QUERY_SELECTED_TEXT, fail if the cache doesn't contain the whole
>  *  selected range. (This shouldn't happen because PuppetWidget should have

>+  virtual void DispatchKeyboardEvent(
>+                 nsINode* aTarget,
>+                 mozilla::WidgetKeyboardEvent* aEvent,
>+                 nsEventStatus* aStatus = nullptr,
>+                 mozilla::EventDispatchingCallback* aEventCB = nullptr) = 0;

nit: Please explain this method with comment.

>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>index 1259d88..aa029fc 100644
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -7782,16 +7782,22 @@ PresShell::HandleEventInternal(WidgetEvent* aEvent, nsEventStatus* aStatus)
>               eventCBPtr = nullptr;
>             }
>           }
>           if (eventTarget) {
>             if (aEvent->mClass == eCompositionEventClass ||
>                 aEvent->mClass == eTextEventClass) {
>               IMEStateManager::DispatchCompositionEvent(eventTarget,
>                 mPresContext, aEvent, aStatus, eventCBPtr);
>+            } else if (aEvent->message == NS_KEY_DOWN ||
>+                       aEvent->message == NS_KEY_UP) {
>+              // Dispatch 'mozbrowserbeforekeydown'/'mozbrowserbeforekeyup'
>+              // and 'keydown'/'keyup'.
>+              DispatchKeyboardEvent(eventTarget, aEvent->AsKeyboardEvent(),
>+                                    aStatus, eventCBPtr);

I feel strange this usage. I think that NS_KEY_PRESS should be dispatched with it because other developers must guess it's called for dispatching every key event.

And also I wonder before/after keypress aren't really necessary?

> void
>+PresShell::DispatchBeforeAfterKeyboardEventInternal(

I think that "Internal" isn't necessary because there is no DispatchBeforeAfterKeyboardEvent().

>+                                    nsINode* aNode,
>+                                    InternalBeforeAfterKeyboardEvent* aEvent,
>+                                    nsEventStatus* aStatus,
>+                                    mozilla::EventDispatchingCallback* aEventCB)
>+{
>+  MOZ_ASSERT(aNode && aEvent);
>+  MOZ_ASSERT(aEvent->message == NS_KEY_DOWN || aEvent->message == NS_KEY_UP);

I guess that this check is wrong because following code checks if the event is NS_KEY_BEFORE_*.

>+  nsCOMPtr<EventTarget> et = do_QueryInterface(document->GetWindow());
>+  if (NS_WARN_IF(!et)) {
>+    return;
>+  }

nit: et isn't good name. Please use eventTarget.

>+  nsCOMPtr<nsIDOMEvent> domEvent;
>+  aEvent->mFlags.mWantReplyFromContentProcess = true;
>+  EventDispatcher::CreateEvent(et, mPresContext,
>+                               static_cast<WidgetEvent*>(aEvent), EmptyString(),
>+                               getter_AddRefs(domEvent));
>+  EventDispatcher::Dispatch(et, mPresContext, aEvent, domEvent, aStatus);

Why do you need to create DOM event manually here?

>+  // When 'mozbrowserbeforekeydown' or 'mozbrowserbeforekeyup' is prevented,
>+  // dispatch 'mozbrowserkeydown'/'mozbrowserkeyup' event immediately.
>+  if (aEvent->mFlags.mDefaultPrevented &&
>+      (aEvent->message == NS_KEY_BEFORE_DOWN ||
>+       aEvent->message == NS_KEY_BEFORE_UP)) {
>+    WidgetKeyboardEvent newEvent(aEvent->mFlags.mIsTrusted,
>+                                 aEvent->message, aEvent->widget);

This should be InternalBeforeAfterKeyboardEvent? And newEvent isn't good name. afterKeyEvent?

>+    newEvent.AssignKeyEventData(*aEvent, false);
>+    newEvent.message = (aEvent->message == NS_KEY_BEFORE_DOWN) ?
>+                       NS_KEY_AFTER_DOWN : NS_KEY_AFTER_UP;
>+    DispatchKeyboardEvent(aNode, &newEvent, aStatus, aEventCB);
>+  }

Hmm, might be the aNode already removed from the tree?

>+void
>+PresShell::DispatchKeyboardEvent(nsINode* aTarget,
>+                                 WidgetKeyboardEvent* aEvent,
>+                                 nsEventStatus* aStatus,
>+                                 EventDispatchingCallback* aEventCB)
>+{
>+  MOZ_ASSERT(aTarget && aEvent);
>+  MOZ_ASSERT(aEvent->message == NS_KEY_DOWN ||
>+             aEvent->message == NS_KEY_UP ||
>+             aEvent->message == NS_KEY_AFTER_DOWN ||
>+             aEvent->message == NS_KEY_AFTER_UP);

If the event is not one of them, i.e., NS_KEY_PRESS, just dispatch it here and return immediately.

>+  // Build up the mozbrowser chain.
>+  nsCOMPtr<nsINode> node = aTarget;
>+  nsTArray<nsCOMPtr<nsINode> > chain;

Why don't you use nsCOMArra<nsINode>?

>+  while (node) {
>+    chain.AppendElement(node);
>+    nsPIDOMWindow* win = node->OwnerDoc()->GetWindow();
>+    node = win ? win->GetFrameElementInternal() : nullptr;
>+  }
>+
>+  /**
>+   * Dispatch event from the innermost element for
>+   *   'mozbrowserbeforekeydown' and 'mozbrowserbeforekeyup'.
>+   *
>+   * Dispatch event from the outermost element for
>+   *   'mozbrowserafterkeydown' and 'mozbrowserafterkeyup'.
>+   */
>+  size_t length = chain.Length();
>+  bool isBefore = (aEvent->message == NS_KEY_DOWN ||
>+                   aEvent->message == NS_KEY_UP);
>+  uint32_t eventMessage = aEvent->message;
>+  for (int i = 0, j = length - 1; i < length; i++, j--) {
>+    node = isBefore ? chain[j] : chain[i];
>+    if (node->NodeName().Find("IFRAME") == -1) {
>+      if (isBefore) {
>+        // Going to dispatch 'keydown'/'keyup'.
>+        aEvent->mFlags.mWantReplyFromContentProcess = true;
>+        EventDispatcher::Dispatch(static_cast<nsISupports*>(aTarget), mPresContext,
>+                                  aEvent, nullptr, aStatus, aEventCB);

So, similarly above, this event dispatch may cause removing aTarget from the tree... And I guess that the keyboard event is dispatched two or more times but WidgetEvent::mFlags is never reset. I'm not sure if it's correct.

>+        uint32_t message = (aEvent->message == NS_KEY_DOWN) ? NS_KEY_AFTER_DOWN : NS_KEY_AFTER_UP;
>+
>+        WidgetKeyboardEvent newEvent(true, message, aEvent->widget);

This should be InternalBeforeAfterKeyboardEvent? And should be afterKeyEvent?

>+        newEvent.AssignKeyEventData(*aEvent, false);
>+        DispatchKeyboardEvent(aTarget, newEvent.AsKeyboardEvent(), aStatus, aEventCB);

You don't need to call AsKeyboardEvent() here. And this may cause removing aTarget from the tree too.

>+        break;
>+      } else {
>+        continue;
>+      }

nit:

if (!isBefore) {
  continue;
}

Then, the if block can be outdented.

>+    // Override message if needed and then dispatch the corresponding event.
>+    uint32_t message = aEvent->message;
>+    if (isBefore) {
>+      message = (eventMessage == NS_KEY_DOWN) ? NS_KEY_BEFORE_DOWN : NS_KEY_BEFORE_UP;
>+    }
>+    InternalBeforeAfterKeyboardEvent newEvent(true, message, aEvent->widget);

beforeKeyEvent is better name?

>+    newEvent.AssignBeforeAfterKeyEventData(*aEvent, false);
>+    newEvent.mFlags.mDefaultPrevented = aEvent->mFlags.mDefaultPrevented;
>+    DispatchBeforeAfterKeyboardEventInternal(node, &newEvent,
>+                                             aStatus, aEventCB);

Then, the node might be removed from the tree.

And there are a lot of mapping event messages. So, how about to create two methods to WidgetKeyboardEvent such as GetAfterKeyMessage() const and GetBeforeKeyMessage() which compute with its message value.
Comment on attachment 8467717 [details] [diff] [review]
Patch 2(v5): Implementation of BeforeAfterKeyboardEvent

>diff --git a/dom/events/BeforeAfterKeyboardEvent.cpp b/dom/events/BeforeAfterKeyboardEvent.cpp
>new file mode 100644
>index 0000000..1f68fb7
>--- /dev/null
>+++ b/dom/events/BeforeAfterKeyboardEvent.cpp
>@@ -0,0 +1,62 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* 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/.
>+ *
>+ * Portions Copyright 2013 Microsoft Open Technologies, Inc. */
>+
>+#include "mozilla/dom/BeforeAfterKeyboardEvent.h"
>+#include "mozilla/dom/KeyboardEvent.h"
>+#include "mozilla/TextEvents.h"
>+#include "prtime.h"
>+
>+namespace mozilla {
>+namespace dom {
>+
>+BeforeAfterKeyboardEvent::BeforeAfterKeyboardEvent(
>+                                         EventTarget* aOwner,
>+                                         nsPresContext* aPresContext,
>+                                         InternalBeforeAfterKeyboardEvent* aEvent)
>+  : KeyboardEvent(aOwner, aPresContext,
>+                  aEvent ? aEvent : new InternalBeforeAfterKeyboardEvent(false, 0, nullptr))

This line is over 80 characters. Like this?

  : KeyboardEvent(aOwner, aPresContext,
                  aEvent ? aEvent :
                           new InternalBeforeAfterKeyboardEvent(false, 0,
                                                                nullptr))

Anyway, ugly, though...

>+{
>+  if (!aEvent) {
>+    mEventIsInternal = true;
>+    mEvent->time = PR_Now();
>+    return;
>+  }

If this is never created from JS, this can be just MOZ_ASSERT().


Then, looks like you never use the key event message utils which you implemented in part 1 except IsAfterKeyEvent(). I think that you should remove the utils from part 1 and implement is in part 2 or later when you need them.
Attachment #8467717 - Flags: review?(masayuki) → review+
Comment on attachment 8467717 [details] [diff] [review]
Patch 2(v5): Implementation of BeforeAfterKeyboardEvent


>+  if (mEvent->AsBeforeAfterKeyboardEvent()->IsAfterKeyEvent()) {
>+    mEmbeddedCancelled.SetValue(aEvent->mFlags.mDefaultPrevented);
>+  }
I don't understand this, and looks wrong.
One can always just check event.defaultPrevented.
Also, it is possible that preventDefault() is called after this DOM Event is created, and in that
case mEmbeddedCancelled wouldn't be updated.

New Event interfaces should have constructors, for the sake of consistency, and
BeforeAfterKeyboardEvent doesn't have one. In fact, nothing in the patch uses BeforeAfterKeyboardEventInit.
Attachment #8467717 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #84)
> New Event interfaces should have constructors, for the sake of consistency,

If it's impossible to create untrusted events of the new events from JS, the event handlers don't need to check if coming event is trusted. Isn't it better?
But chrome js might want to dispatch these events.


Also, I forgot from the review, don't we want to expose the new event interface only in some
contexts. So Pref="" or CheckPermission?

And * Portions Copyright 2013 Microsoft Open Technologies, Inc. */ looks wrong.
You probably copied the header from PointerEvents files.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #83)
> Then, looks like you never use the key event message utils which you
> implemented in part 1 except IsAfterKeyEvent(). I think that you should
> remove the utils from part 1 and implement is in part 2 or later when you
> need them.

Okay! I'll re-organize the patches.

(In reply to Olli Pettay [:smaug] from comment #84)
> Comment on attachment 8467717 [details] [diff] [review]
> Patch 2(v5): Implementation of BeforeAfterKeyboardEvent
> >+  if (mEvent->AsBeforeAfterKeyboardEvent()->IsAfterKeyEvent()) {
> >+    mEmbeddedCancelled.SetValue(aEvent->mFlags.mDefaultPrevented);
> >+  }
> I don't understand this, and looks wrong.
> One can always just check event.defaultPrevented.
> Also, it is possible that preventDefault() is called after this DOM Event is
> created, and in that
> case mEmbeddedCancelled wouldn't be updated.

You are right! It's kind of tricky to use event.defaultPrevented in this way.

> New Event interfaces should have constructors, for the sake of consistency,
> and
> BeforeAfterKeyboardEvent doesn't have one. In fact, nothing in the patch
> uses BeforeAfterKeyboardEventInit.

I Agree. I think BeforeAfterKeyboardEventInit should be used to create new event rather than using EventDispatcher::CreateEvent().
(In reply to Olli Pettay [:smaug] from comment #84)
> New Event interfaces should have constructors, for the sake of consistency,
> and
> BeforeAfterKeyboardEvent doesn't have one. In fact, nothing in the patch
> uses BeforeAfterKeyboardEventInit.

(In reply to Olli Pettay [:smaug] from comment #86)
> But chrome js might want to dispatch these events.

So, I assume that we should have a Constructor in BeforeAfterKeyboardEvent to keep consistency with other events.

> Also, I forgot from the review, don't we want to expose the new event
> interface only in some
> contexts. So Pref="" or CheckPermission?

I'm not quite sure. The new event interface should only be dispatched to a iframe which sets the mozbrowser attribute. I.e. <iframe mozbrowser>. Should we use extended attributes for WebIDL like Pref or CheckPermissions in this case?

> And * Portions Copyright 2013 Microsoft Open Technologies, Inc. */ looks
> wrong.
> You probably copied the header from PointerEvents files.

Oops! Thanks for pointing it out.
Move util functions to other patches.
Attachment #8467700 - Attachment is obsolete: true
Add constructor to event interface.
Attachment #8467717 - Attachment is obsolete: true
Attachment #8468376 - Flags: review?(masayuki)
Attachment #8468376 - Flags: review?(bugs)
Comment on attachment 8468376 [details] [diff] [review]
Patch 2(v6): Implementation of BeforeAfterKeyboardEvent

Review of attachment 8468376 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/BeforeAfterKeyboardEvent.cpp
@@ +23,5 @@
> +  if (!aEvent) {
> +    mEventIsInternal = true;
> +    mEvent->time = PR_Now();
> +    return;
> +  }

I'm not quite sure whether this part should be removed or not if chrome js might dispatch these events.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #82)
> Comment on attachment 8467711 [details] [diff] [review]
> Patch 3(WIP): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent
> 
> >--- a/content/base/src/nsGkAtomList.h
> >+++ b/content/base/src/nsGkAtomList.h
> >@@ -772,16 +772,20 @@ GK_ATOM(onmousedown, "onmousedown")
> > GK_ATOM(onmouseenter, "onmouseenter")
> > GK_ATOM(onmouseleave, "onmouseleave")
> > GK_ATOM(onmousemove, "onmousemove")
> > GK_ATOM(onmouseout, "onmouseout")
> > GK_ATOM(onmouseover, "onmouseover")
> > GK_ATOM(onMozMouseHittest, "onMozMouseHittest")
> > GK_ATOM(onmouseup, "onmouseup")
> > GK_ATOM(onMozAfterPaint, "onMozAfterPaint")
> >+GK_ATOM(onmozbrowserafterkeydown, "onmozbrowserafterkeydown")
> >+GK_ATOM(onmozbrowserafterkeyup, "onmozbrowserafterkeyup")
> >+GK_ATOM(onmozbrowserbeforekeydown, "onmozbrowserbeforekeydown")
> >+GK_ATOM(onmozbrowserbeforekeyup, "onmozbrowserbeforekeyup")
> 
> You define attributes of the event handlers...
> 
> >diff --git a/dom/events/EventNameList.h b/dom/events/EventNameList.h
> >index 607ce7b..c0d99f1 100644
> >--- a/dom/events/EventNameList.h
> >+++ b/dom/events/EventNameList.h
> >@@ -232,16 +232,32 @@ EVENT(keydown,
> > EVENT(keypress,
> >       NS_KEY_PRESS,
> >       EventNameType_HTMLXUL,
> >       eKeyboardEventClass)
> > EVENT(keyup,
> >       NS_KEY_UP,
> >       EventNameType_HTMLXUL,
> >       eKeyboardEventClass)
> >+NON_IDL_EVENT(mozbrowserbeforekeydown,
> >+              NS_KEY_BEFORE_DOWN,
> >+              EventNameType_None,
> >+              eBeforeAfterKeyboardEventClass)
> >+NON_IDL_EVENT(mozbrowserafterkeydown,
> >+              NS_KEY_AFTER_DOWN,
> >+              EventNameType_None,
> >+              eBeforeAfterKeyboardEventClass)
> >+NON_IDL_EVENT(mozbrowserbeforekeyup,
> >+              NS_KEY_BEFORE_UP,
> >+              EventNameType_None,
> >+              eBeforeAfterKeyboardEventClass)
> >+NON_IDL_EVENT(mozbrowserafterkeyup,
> >+              NS_KEY_AFTER_UP,
> >+              EventNameType_None,
> >+              eBeforeAfterKeyboardEventClass)
> 
> But you agree with they shouldn't be available with event handler
> attributes. So, the atom definition should be removed.

I tried to remove the atom definition but build failed:

In file included from ../../../../../../B2G-flame/gecko/content/base/src/nsContentUtils.cpp:632:0:
../../../dist/include/mozilla/EventNameList.h: In static member function 'static bool nsContentUtils::InitializeEventTable()':
../../../dist/include/mozilla/EventNameList.h:240:3: error: 'onmozbrowserbeforekeydown' is not a member of 'nsGkAtoms'
In file included from ../../../../../../B2G-flame/gecko/content/base/src/nsContentUtils.cpp:632:0:
../../../dist/include/mozilla/EventNameList.h:244:3: error: 'onmozbrowserafterkeydown' is not a member of 'nsGkAtoms'
In file included from ../../../../../../B2G-flame/gecko/content/base/src/nsContentUtils.cpp:632:0:
../../../dist/include/mozilla/EventNameList.h:248:3: error: 'onmozbrowserbeforekeyup' is not a member of 'nsGkAtoms'
In file included from ../../../../../../B2G-flame/gecko/content/base/src/nsContentUtils.cpp:632:0:
../../../dist/include/mozilla/EventNameList.h:252:3: error: 'onmozbrowserafterkeyup' is not a member of 'nsGkAtoms'


Should I keep the atom definition? Any other suggestion?
Note that feedbacks in comment 82 haven't been fully addressed. (Thanks, Masayuki.) I'm still working on it and will send review request once finished.
Attachment #8467711 - Attachment is obsolete: true
Yes you need those atoms for the event list.
QA Whiteboard: [2.1-feature-qa+] → [2.1-feature-qa?]
Comment on attachment 8468376 [details] [diff] [review]
Patch 2(v6): Implementation of BeforeAfterKeyboardEvent

>+BeforeAfterKeyboardEvent::Constructor(
>+                                     EventTarget* aOwner,
>+                                     const nsAString& aType,
>+                                     const BeforeAfterKeyboardEventInit& aParam)
>+{
>+  nsRefPtr<BeforeAfterKeyboardEvent> event =
>+    new BeforeAfterKeyboardEvent(aOwner, nullptr, nullptr);
>+  bool trusted = event->Init(aOwner);
>+  event->InitKeyEvent(aType, aParam.mBubbles, aParam.mCancelable,
>+                      aParam.mView, aParam.mCtrlKey, aParam.mAltKey,
>+                      aParam.mShiftKey, aParam.mMetaKey,
>+                      aParam.mKeyCode, aParam.mCharCode);
>+  event->SetTrusted(trusted);
>+  if (aType.EqualsLiteral("mozbrowserafterkeydown") ||
>+      aType.EqualsLiteral("mozbrowserafterkeyup")) {
>+    event->mEmbeddedCancelled = aParam.mEmbeddedCancelled;
>+  }
Drop the aType.Equals checks.
If the caller wants to initialize mEmbeddedCancelled, then it should be initialized.
Whether or not the type is something special.
And since the default is null, event->mEmbeddedCancelled will be initialized by default to null.


>+
>+  InternalBeforeAfterKeyboardEvent* internalEvent =
>+    event->mEvent->AsBeforeAfterKeyboardEvent();
>+  internalEvent->location = aParam.mLocation;
>+  internalEvent->mIsRepeat = aParam.mRepeat;
>+  internalEvent->mIsComposing = aParam.mIsComposing;
>+  internalEvent->mKeyNameIndex = KEY_NAME_INDEX_USE_STRING;
>+  internalEvent->mKeyValue = aParam.mKey;
>+  internalEvent->mCodeNameIndex = CODE_NAME_INDEX_USE_STRING;
>+  internalEvent->mCodeValue = aParam.mCode;


I don't see you initializing detail or which and such.
Please check what KeyboardEvent::Constructor does.
(and feel free to create some helper method which both BeforeAfterKeyboardEvent and KeyboardEvent could use -
 if that makes code simpler)

Would like to still see a new patch.
Attachment #8468376 - Flags: review?(bugs) → review-
Gina, sprint 1 was passed. Can you update the target milestone? Thanks.
Flags: needinfo?(gyeh)
We'll move this bug into sprint 3.
Flags: needinfo?(gyeh)
Target Milestone: 2.1 S1 (1aug) → 2.1 S3 (29aug)
- Add a helper function in WidgetKeyboardEvent.
- Remove type checking for setting mEmbeddedCancelled.
- Make data members in KeyboardEvent protected.
Attachment #8468376 - Attachment is obsolete: true
Attachment #8468376 - Flags: review?(masayuki)
Comment on attachment 8469009 [details] [diff] [review]
Patch 2(v7): Implementation of BeforeAfterKeyboardEvent

Patch 2 has been updated. Please help to review. Thanks.
Attachment #8469009 - Flags: review?(masayuki)
Attachment #8469009 - Flags: review?(bugs)
Comment on attachment 8469009 [details] [diff] [review]
Patch 2(v7): Implementation of BeforeAfterKeyboardEvent

>diff --git a/dom/events/BeforeAfterKeyboardEvent.cpp b/dom/events/BeforeAfterKeyboardEvent.cpp
>--- /dev/null
>+++ b/dom/events/BeforeAfterKeyboardEvent.cpp
>@@ -0,0 +1,102 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* 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/. */
       ^
Looks odd. Why there is extra * below "h" of "This" and below its right space.

>+BeforeAfterKeyboardEvent::BeforeAfterKeyboardEvent(
>+                                       EventTarget* aOwner,
>+                                       nsPresContext* aPresContext,
>+                                       InternalBeforeAfterKeyboardEvent* aEvent)
>+  : KeyboardEvent(aOwner, aPresContext,
>+                  aEvent ? aEvent :
>+                           new InternalBeforeAfterKeyboardEvent(false, 0,
>+                                                                nullptr))
>+{
>+  if (!aEvent) {
>+    mEventIsInternal = true;
>+    mEvent->time = PR_Now();
>+    return;
>+  }
>+
>+  MOZ_ASSERT(mEvent->mClass == eBeforeAfterKeyboardEvent,
>+             "event type mismatch eBeforeAfterKeyboardEvent");
>+  MOZ_ASSERT(mEvent->message == NS_KEY_BEFORE_DOWN ||
>+             mEvent->message == NS_KEY_BEFORE_UP ||
>+             mEvent->message == NS_KEY_AFTER_DOWN ||
>+             mEvent->message == NS_KEY_AFTER_UP,
>+             "event message mismatch");
>+
>+  mEventIsInternal = false;

I think that this is redundant.

>+// static
>+already_AddRefed<BeforeAfterKeyboardEvent>
>+BeforeAfterKeyboardEvent::Constructor(
>+                                     EventTarget* aOwner,
>+                                     const nsAString& aType,
>+                                     const BeforeAfterKeyboardEventInit& aParam)

nit: Outdent to below 'n' of 'Constructor'.

>+{
>+  nsRefPtr<BeforeAfterKeyboardEvent> event =
>+    new BeforeAfterKeyboardEvent(aOwner, nullptr, nullptr);
>+
>+  bool trusted = event->Init(aOwner);
>+  event->InitKeyEvent(aType, aParam.mBubbles, aParam.mCancelable,
>+                      aParam.mView, aParam.mCtrlKey, aParam.mAltKey,
>+                      aParam.mShiftKey, aParam.mMetaKey,
>+                      aParam.mKeyCode, aParam.mCharCode);
>+  event->SetTrusted(trusted);
>+
>+  // Members inherited from UIEvent.
>+  event->mDetail = aParam.mDetail;
>+  // Members inherited from KeyboardEvent.
>+  event->mInitializedByCtor = true;
>+  event->mInitializedWhichValue = aParam.mWhich;

Hmm, why don't you create a common constructor method in KeyboardEvent like:

static void ConstructorInternal(WidgetKeyboardEvent* aEvent,
                                EventTarget* aOwner,
                                const nsAString& aType,
                                const KeyboardEventInit& aParam);

instead of initializing all of them by yourself?

>+  // Members of BeforeAfterKeyboardEvent.
>+  event->mEmbeddedCancelled = aParam.mEmbeddedCancelled;

Right. According to the event constructor's spec, any attributes should be initializable with any values even if its type value is any value.

>+  InternalBeforeAfterKeyboardEvent* internalEvent =
>+    event->mEvent->AsBeforeAfterKeyboardEvent();
>+  internalEvent->CopyKeyEventData(aParam);

I guess that if you call KeyboardEvent::Constructor(), you don't need to create and call this method.

>+// static
>+already_AddRefed<BeforeAfterKeyboardEvent>
>+BeforeAfterKeyboardEvent::Constructor(
>+                                     const GlobalObject& aGlobal,
>+                                     const nsAString& aType,
>+                                     const BeforeAfterKeyboardEventInit& aParam,
>+                                     ErrorResult& aRv)

nit: Outdent to below 'n' of 'Constructor'.

>diff --git a/dom/events/BeforeAfterKeyboardEvent.h b/dom/events/BeforeAfterKeyboardEvent.h
>--- /dev/null
>+++ b/dom/events/BeforeAfterKeyboardEvent.h
>@@ -0,0 +1,51 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* 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/. */

Same, there are odd extra '*'.

>diff --git a/dom/events/KeyboardEvent.cpp b/dom/events/KeyboardEvent.cpp
>--- a/dom/events/KeyboardEvent.cpp
>+++ b/dom/events/KeyboardEvent.cpp
>@@ -12,17 +12,17 @@ namespace mozilla {
> namespace dom {
> 
> KeyboardEvent::KeyboardEvent(EventTarget* aOwner,
>                              nsPresContext* aPresContext,
>                              WidgetKeyboardEvent* aEvent)
>   : UIEvent(aOwner, aPresContext,
>             aEvent ? aEvent : new WidgetKeyboardEvent(false, 0, nullptr))
>   , mInitializedByCtor(false)
>-  , mInitialzedWhichValue(0)
>+  , mInitializedWhichValue(0)

Wow, thanks for this fix!

>diff --git a/dom/events/KeyboardEvent.h b/dom/events/KeyboardEvent.h
>--- a/dom/events/KeyboardEvent.h
>+++ b/dom/events/KeyboardEvent.h
>@@ -69,21 +69,21 @@ public:
>     aRv = InitKeyEvent(aType, aCanBubble, aCancelable, aView,
>                        aCtrlKey, aAltKey, aShiftKey,aMetaKey,
>                        aKeyCode, aCharCode);
>   }
> 
> protected:
>   ~KeyboardEvent() {}
> 
>-private:
>   // True, if the instance is created with Constructor().
>   bool mInitializedByCtor;
>+

By creating ConstructorInternal(), this change isn't necessary.

>diff --git a/widget/TextEvents.h b/widget/TextEvents.h
>index 40c832a..fd43184 100644
>--- a/widget/TextEvents.h
>+++ b/widget/TextEvents.h

Changes in this file too.
Attachment #8469009 - Flags: review?(masayuki) → review-
Comment on attachment 8469009 [details] [diff] [review]
Patch 2(v7): Implementation of BeforeAfterKeyboardEvent

I guess it is better to review the probably last version of the patch, since
masayuki already reviewed this.
Attachment #8469009 - Flags: review?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #100)
> Comment on attachment 8469009 [details] [diff] [review]
> Patch 2(v7): Implementation of BeforeAfterKeyboardEvent
> 
> >diff --git a/dom/events/BeforeAfterKeyboardEvent.cpp b/dom/events/BeforeAfterKeyboardEvent.cpp
> >--- /dev/null
> >+++ b/dom/events/BeforeAfterKeyboardEvent.cpp
> >@@ -0,0 +1,102 @@
> >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* 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/. */
>        ^
> Looks odd. Why there is extra * below "h" of "This" and below its right
> space.

Sorry. Seems like I broke the format somehow. Will fix it in the next patch.

> 
> >+BeforeAfterKeyboardEvent::BeforeAfterKeyboardEvent(
> >+                                       EventTarget* aOwner,
> >+                                       nsPresContext* aPresContext,
> >+                                       InternalBeforeAfterKeyboardEvent* aEvent)
> >+  : KeyboardEvent(aOwner, aPresContext,
> >+                  aEvent ? aEvent :
> >+                           new InternalBeforeAfterKeyboardEvent(false, 0,
> >+                                                                nullptr))
> >+{
> >+  if (!aEvent) {
> >+    mEventIsInternal = true;
> >+    mEvent->time = PR_Now();
> >+    return;
> >+  }
> >+
> >+  MOZ_ASSERT(mEvent->mClass == eBeforeAfterKeyboardEvent,
> >+             "event type mismatch eBeforeAfterKeyboardEvent");
> >+  MOZ_ASSERT(mEvent->message == NS_KEY_BEFORE_DOWN ||
> >+             mEvent->message == NS_KEY_BEFORE_UP ||
> >+             mEvent->message == NS_KEY_AFTER_DOWN ||
> >+             mEvent->message == NS_KEY_AFTER_UP,
> >+             "event message mismatch");
> >+
> >+  mEventIsInternal = false;
> 
> I think that this is redundant.

Shall we keep it as the reason for the same reason in comment 57 and comment 58?

> >+{
> >+  nsRefPtr<BeforeAfterKeyboardEvent> event =
> >+    new BeforeAfterKeyboardEvent(aOwner, nullptr, nullptr);
> >+
> >+  bool trusted = event->Init(aOwner);
> >+  event->InitKeyEvent(aType, aParam.mBubbles, aParam.mCancelable,
> >+                      aParam.mView, aParam.mCtrlKey, aParam.mAltKey,
> >+                      aParam.mShiftKey, aParam.mMetaKey,
> >+                      aParam.mKeyCode, aParam.mCharCode);
> >+  event->SetTrusted(trusted);
> >+
> >+  // Members inherited from UIEvent.
> >+  event->mDetail = aParam.mDetail;
> >+  // Members inherited from KeyboardEvent.
> >+  event->mInitializedByCtor = true;
> >+  event->mInitializedWhichValue = aParam.mWhich;
> 
> Hmm, why don't you create a common constructor method in KeyboardEvent like:
> 
> static void ConstructorInternal(WidgetKeyboardEvent* aEvent,
>                                 EventTarget* aOwner,
>                                 const nsAString& aType,
>                                 const KeyboardEventInit& aParam);
> 
> instead of initializing all of them by yourself?

Good suggestion. I change the function signature as following and rename it to InitWithKeyboardEventInit.

static void InitWithKeyboardEventInit(KeyboardEvent* aEvent,
                                      EventTarget* aOwner,
                                      const nsAString& aType,
                                      const KeyboardEventInit& aParam,
                                      ErrorResult& aRv);

So, we can initialize all members of both dom event and widget event in one function and reuse it in BeforeAfterKeyboardEvent. Does it make sense to you?
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #102)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #100)
> > >+  mEventIsInternal = false;
> > 
> > I think that this is redundant.
> 
> Shall we keep it as the reason for the same reason in comment 57 and comment
> 58?

The comment in Event.cpp is in the !aEvent case. I don't think we need to set mEventIsInternal to false again.

> > >+  nsRefPtr<BeforeAfterKeyboardEvent> event =
> > >+    new BeforeAfterKeyboardEvent(aOwner, nullptr, nullptr);
> > >+
> > >+  bool trusted = event->Init(aOwner);
> > >+  event->InitKeyEvent(aType, aParam.mBubbles, aParam.mCancelable,
> > >+                      aParam.mView, aParam.mCtrlKey, aParam.mAltKey,
> > >+                      aParam.mShiftKey, aParam.mMetaKey,
> > >+                      aParam.mKeyCode, aParam.mCharCode);
> > >+  event->SetTrusted(trusted);
> > >+
> > >+  // Members inherited from UIEvent.
> > >+  event->mDetail = aParam.mDetail;
> > >+  // Members inherited from KeyboardEvent.
> > >+  event->mInitializedByCtor = true;
> > >+  event->mInitializedWhichValue = aParam.mWhich;
> > 
> > Hmm, why don't you create a common constructor method in KeyboardEvent like:
> > 
> > static void ConstructorInternal(WidgetKeyboardEvent* aEvent,
> >                                 EventTarget* aOwner,
> >                                 const nsAString& aType,
> >                                 const KeyboardEventInit& aParam);
> > 
> > instead of initializing all of them by yourself?
> 
> Good suggestion. I change the function signature as following and rename it
> to InitWithKeyboardEventInit.
> 
> static void InitWithKeyboardEventInit(KeyboardEvent* aEvent,
>                                       EventTarget* aOwner,
>                                       const nsAString& aType,
>                                       const KeyboardEventInit& aParam,
>                                       ErrorResult& aRv);
> 
> So, we can initialize all members of both dom event and widget event in one
> function and reuse it in BeforeAfterKeyboardEvent. Does it make sense to you?

Sure. SGTM.
Feedbacks for v7 has been addressed.
Attachment #8469009 - Attachment is obsolete: true
Attachment #8469837 - Flags: review?(masayuki)
Attachment #8469837 - Flags: review?(bugs)
Something just came into my mind. I think we can just make the function protected, so that we don't have to pass a KeyboardEvent* as argument.
Attachment #8469837 - Attachment is obsolete: true
Attachment #8469837 - Flags: review?(masayuki)
Attachment #8469837 - Flags: review?(bugs)
Attachment #8469839 - Flags: review?(masayuki)
Attachment #8469839 - Flags: review?(bugs)
Comment on attachment 8469839 [details] [diff] [review]
Patch 2(v8): Implementation of BeforeAfterKeyboardEvent

Review of attachment 8469839 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/events/BeforeAfterKeyboardEvent.h
@@ +8,5 @@
> +
> +#include "mozilla/dom/KeyboardEvent.h"
> +#include "mozilla/dom/BeforeAfterKeyboardEventBinding.h"
> +
> +class nsPresContext;

This line is redundant. I'll remove it in the final patch

::: dom/webidl/BeforeAfterKeyboardEvent.webidl
@@ +3,5 @@
> + * 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/.
> + */
> +
> +interface WindowProxy;

This is redundant, too.
I found the changes in KeyboardEvent::CharCode() are missing. Allow me to re-attach my patch. I also remove the redundant lines mentioned in comment 106.
Attachment #8469839 - Attachment is obsolete: true
Attachment #8469839 - Flags: review?(masayuki)
Attachment #8469839 - Flags: review?(bugs)
Attachment #8469853 - Flags: review?(masayuki)
Attachment #8469853 - Flags: review?(bugs)
I know this has been mentioned quite a few times, but I'm not sure if we want to adopt similar mechanism for NS_KEY_PRESS. Any idea?
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #108)
> I know this has been mentioned quite a few times, but I'm not sure if we
> want to adopt similar mechanism for NS_KEY_PRESS. Any idea?

I guess that it's necessary. keypress event should be fired only when pressing a key causes inputting text. However, Gecko dispatches keypress event for all keys except modifier keys (this is not conforming to D3E spec). But I'm not sure if it really necessary. If you don't implement them, at least, embedder cannot handle something immediately after keypress.

And regret to say, I cannot review the patch tomorrow because I don't have much time. I'm sorry.
Flags: needinfo?(masayuki)
Masayuki, thanks for your prompt response. :)
Fix nits.
Attachment #8469853 - Attachment is obsolete: true
Attachment #8469853 - Flags: review?(masayuki)
Attachment #8469853 - Flags: review?(bugs)
Attachment #8469948 - Flags: review?(masayuki)
Attachment #8469948 - Flags: review?(bugs)
- Add nsIPresShell::DispatchKeyboardEvent() for dispatching BeforeAfterKeyboardEvent and KeyboardEvent (if necessary) according to widget event message:
  * NS_KEY_PRESS: dispatch 'keypress' event to event target
  * NS_KEY_BEFORE_DOWN: dispatch 'mozbrowserbeforekeydown' event to all ancestors (which must be <iframe mozbrowser mozapp>) of event target from the outermost element, and then dispatch 'keydown' event to event target
  * NS_KEY_AFTER_DOWN: dispatch 'mozbrowserafterkeydown' event to all ancestors (which must be <iframe mozbrowser mozapp>) of event target from the innermost element
  * NS_KEY_BEFORE_UP: dispatch 'mozbrowserbeforekeyup' event to all ancestors (which must be <iframe mozbrowser mozapp>) of event target from the outermost element, and then dispatch 'keyup' event to event target
  * NS_KEY_AFTER_UP: dispatch 'mozbrowserafterkeyup' event to all ancestors (which must be <iframe mozbrowser mozapp>) of event target from the innermost element

- Add nsPresShell::DispatchBeforeAfterKeyboardEvent() for dispatching a BeforeAfterKeyboardEvent to an iframe with mozbrowser attribute and mozapp attribute.

- Define new event in event list

- Add util functions in WidgetKeyboardEvent
Attachment #8468389 - Attachment is obsolete: true
Attachment #8469966 - Flags: review?(masayuki)
Attachment #8469966 - Flags: review?(bugs)
Comment on attachment 8469948 [details] [diff] [review]
Patch 2(v8): Implementation of BeforeAfterKeyboardEvent

>+BeforeAfterKeyboardEvent::BeforeAfterKeyboardEvent(
>+                                       EventTarget* aOwner,
>+                                       nsPresContext* aPresContext,
>+                                       InternalBeforeAfterKeyboardEvent* aEvent)
>+  : KeyboardEvent(aOwner, aPresContext,
>+                  aEvent ? aEvent :
>+                           new InternalBeforeAfterKeyboardEvent(false, 0,
>+                                                                nullptr))
>+{
>+  MOZ_ASSERT(mEvent->mClass == eBeforeAfterKeyboardEvent,
>+             "event type mismatch eBeforeAfterKeyboardEvent");
>+  MOZ_ASSERT(aEvent &&
>+             (mEvent->IsBeforeKeyEvent() || mEvent->IsAfterKeyEvent()),
>+             "event message mismatch");
This assertion is wrong. If internal event was created, it is not before or after key event.
I assume you want aEvent, not mEvent in the assertion

>   switch (mEvent->message) {
>-    case NS_KEY_UP:
>-    case NS_KEY_DOWN:
>-      return KeyCode();
>-    case NS_KEY_PRESS:
>-      //Special case for 4xp bug 62878.  Try to make value of which
>-      //more closely mirror the values that 4.x gave for RETURN and BACKSPACE
>-      {
>-        uint32_t keyCode = mEvent->AsKeyboardEvent()->keyCode;
>-        if (keyCode == NS_VK_RETURN || keyCode == NS_VK_BACK) {
>-          return keyCode;
>-        }
>-        return CharCode();
>+  case NS_KEY_BEFORE_DOWN:
>+  case NS_KEY_DOWN:
>+  case NS_KEY_AFTER_DOWN:
>+  case NS_KEY_BEFORE_UP:
>+  case NS_KEY_UP:
>+  case NS_KEY_AFTER_UP:
>+    return KeyCode();
>+  case NS_KEY_PRESS:
>+    //Special case for 4xp bug 62878.  Try to make value of which
>+    //more closely mirror the values that 4.x gave for RETURN and BACKSPACE
>+    {
>+      uint32_t keyCode = mEvent->AsKeyboardEvent()->keyCode;
>+      if (keyCode == NS_VK_RETURN || keyCode == NS_VK_BACK) {
>+        return keyCode;
>       }
>+      return CharCode();
>+    }
>   }
Why you unintend code 'case' clauses here? Per coding style they should be intended.
>+++ b/dom/webidl/BeforeAfterKeyboardEvent.webidl
>@@ -0,0 +1,22 @@
>+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* 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/.
>+ */
>+
>+[Constructor(DOMString typeArg, optional BeforeAfterKeyboardEventInit eventInitDict)]
We need a Pref or CheckPermission or some such here so that BeforeAfterKeyboardEvent interface isn't exposed to all the web pages.


Those fixed, r+
Attachment #8469948 - Flags: review?(bugs) → review+
(and some test would be nice)
Flags: needinfo?(bugs)
And we need to add the new interface to test_interfaces.html
Comment on attachment 8469966 [details] [diff] [review]
Patch 3(v1): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

>+bool TabParent::SendRealKeyEvent(WidgetKeyboardEvent& aEvent)
> {
>   if (mIsDestroyed) {
>     return false;
>   }
>-  MaybeForwardEventToRenderFrame(event, nullptr);
>-  if (!MapEventCoordinatesForChildProcess(&event)) {
>+  MaybeForwardEventToRenderFrame(aEvent, nullptr);
>+  if (!MapEventCoordinatesForChildProcess(&aEvent)) {
>     return false;
>   }
> 
>-
>   MaybeNativeKeyBinding bindings;
>   bindings = void_t();
>-  if (event.message == NS_KEY_PRESS) {
>+  if (aEvent.message == NS_KEY_PRESS) {
>     nsCOMPtr<nsIWidget> widget = GetWidget();
> 
>     AutoInfallibleTArray<mozilla::CommandInt, 4> singleLine;
>     AutoInfallibleTArray<mozilla::CommandInt, 4> multiLine;
>     AutoInfallibleTArray<mozilla::CommandInt, 4> richText;
> 
>     widget->ExecuteNativeKeyBinding(nsIWidget::NativeKeyBindingsForSingleLineEditor,
>-                                    event, DoCommandCallback, &singleLine);
>+                                    aEvent, DoCommandCallback, &singleLine);
>     widget->ExecuteNativeKeyBinding(nsIWidget::NativeKeyBindingsForMultiLineEditor,
>-                                    event, DoCommandCallback, &multiLine);
>+                                    aEvent, DoCommandCallback, &multiLine);
>     widget->ExecuteNativeKeyBinding(nsIWidget::NativeKeyBindingsForRichTextEditor,
>-                                    event, DoCommandCallback, &richText);
>+                                    aEvent, DoCommandCallback, &richText);
> 
>     if (!singleLine.IsEmpty() || !multiLine.IsEmpty() || !richText.IsEmpty()) {
>       bindings = NativeKeyBinding(singleLine, multiLine, richText);
>     }
>   }
> 
>-  return PBrowserParent::SendRealKeyEvent(event, bindings);
>+  aEvent.ConvertToKeyboardEvent();
>+  return PBrowserParent::SendRealKeyEvent(aEvent, bindings);
I don't understand why we want to change the event type here.

>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent)
> {
>   NS_ENSURE_TRUE(mFrameElement, true);
> 
>-  WidgetKeyboardEvent localEvent(event);
>+  WidgetKeyboardEvent localEvent(aEvent);
>   // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>   // being infinitely redispatched and forwarded to the child again.
>   localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
> 
>   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>   // to be able to dispatch it to the <browser> element as the target element.
>   nsIDocument* doc = mFrameElement->OwnerDoc();
>   nsIPresShell* presShell = doc->GetShell();
>-  NS_ENSURE_TRUE(presShell, true);
>-  nsPresContext* presContext = presShell->GetPresContext();
>-  NS_ENSURE_TRUE(presContext, true);
>+  if (!presShell) {
>+    return true;
>+  }
> 
>-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+  localEvent.ConvertToAfterKeyboardEvent();
>+  presShell->DispatchKeyboardEvent(mFrameElement, &localEvent);
This certainly breaks Bug 862519

>@@ -7782,16 +7784,21 @@ PresShell::HandleEventInternal(WidgetEvent* aEvent, nsEventStatus* aStatus)
>               eventCBPtr = nullptr;
>             }
>           }
>           if (eventTarget) {
>             if (aEvent->mClass == eCompositionEventClass ||
>                 aEvent->mClass == eTextEventClass) {
>               IMEStateManager::DispatchCompositionEvent(eventTarget,
>                 mPresContext, aEvent, aStatus, eventCBPtr);
>+            } else if (aEvent->mClass == eKeyboardEventClass) {
>+              WidgetKeyboardEvent* keyboardEvent = aEvent->AsKeyboardEvent();
>+              keyboardEvent->ConvertToBeforeKeyboardEvent();
Why we want this conversion? Wouldn't this break all the keydown events


>+
>+  nsRefPtr<BeforeAfterKeyboardEvent> event =
>+    BeforeAfterKeyboardEvent::Constructor(eventTarget, eventName, init);
>+  if (event) {
>+    bool dummy;
>+    eventTarget->DispatchEvent(event->InternalDOMEvent(), &dummy);
>+  }

Why don't you just dispatch using the BeforeAfterKeyboard struct.
No need to create the DOM event manually

>+  // Build up iframe chain.
>+  nsCOMPtr<nsINode> node = aTarget;
>+  nsCOMPtr<nsIContent> content;
>+  nsCOMArray<nsINode> chain;
>+  while (node) {
>+    content = do_QueryInterface(node);
>+    if (content && content->IsHTML(nsGkAtoms::iframe)) {
>+      chain.AppendElement(node);
>+    }
>+
>+    nsPIDOMWindow* win = node->OwnerDoc()->GetWindow();
>+    node = win ? win->GetFrameElementInternal() : nullptr;
>+  }
>+
>+  // Dispatch before events from the outermost element and dispatch after events
>+  // from the innermost element.
>+  size_t length = chain.Length();
>+  for (uint8_t i = 0, j = length - 1; i < length; i++, j--) {
>+    node = aEvent->IsAfterKeyEvent() ? chain[i] : chain[j];
>+
>+    InternalBeforeAfterKeyboardEvent newEvent(true, aEvent->message, aEvent->widget);
>+    newEvent.AssignBeforeAfterKeyEventData(*aEvent, false);
>+    DispatchBeforeAfterKeyboardEvent(node, &newEvent, aStatus, aEventCB,
>+                                     aEvent->mFlags.mDefaultPrevented);
>+  }
We don't want to dispatch the events to random iframes. Only some privileged which have the right permission.

>+  void ConvertToBeforeKeyboardEvent()
>+  {
...
>+      case NS_KEY_AFTER_DOWN:
>+        message = NS_KEY_BEFORE_DOWN;
This looks really odd, could you explain.


>+      case NS_KEY_AFTER_UP:
>+        message = NS_KEY_BEFORE_UP;
And this


>+  void ConvertToAfterKeyboardEvent()
>+  {
...
>+      case NS_KEY_BEFORE_DOWN:
>+        message = NS_KEY_AFTER_DOWN;
And this

>+      case NS_KEY_BEFORE_UP:
>+        message = NS_KEY_AFTER_UP;
And this



In general I don't quite understand the setup here.
Attachment #8469966 - Flags: review?(bugs) → review-
QA Whiteboard: [2.1-feature-qa?]
Whiteboard: [FT:Stream3] → [FT:Stream3][2.1-feature-qa+]
Flags: in-moztrap?(fyen)
QA Contact: fyen
(In reply to Olli Pettay [:smaug] from comment #113)
> Comment on attachment 8469948 [details] [diff] [review]
> Patch 2(v8): Implementation of BeforeAfterKeyboardEvent
> 
> >+BeforeAfterKeyboardEvent::BeforeAfterKeyboardEvent(
> >+                                       EventTarget* aOwner,
> >+                                       nsPresContext* aPresContext,
> >+                                       InternalBeforeAfterKeyboardEvent* aEvent)
> >+  : KeyboardEvent(aOwner, aPresContext,
> >+                  aEvent ? aEvent :
> >+                           new InternalBeforeAfterKeyboardEvent(false, 0,
> >+                                                                nullptr))
> >+{
> >+  MOZ_ASSERT(mEvent->mClass == eBeforeAfterKeyboardEvent,
> >+             "event type mismatch eBeforeAfterKeyboardEvent");
> >+  MOZ_ASSERT(aEvent &&
> >+             (mEvent->IsBeforeKeyEvent() || mEvent->IsAfterKeyEvent()),
> >+             "event message mismatch");
> This assertion is wrong. If internal event was created, it is not before or
> after key event.
> I assume you want aEvent, not mEvent in the assertion

Yep! This should be aEvent. Thanks.

> >+++ b/dom/webidl/BeforeAfterKeyboardEvent.webidl
> >@@ -0,0 +1,22 @@
> >+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* 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/.
> >+ */
> >+
> >+[Constructor(DOMString typeArg, optional BeforeAfterKeyboardEventInit eventInitDict)]
> We need a Pref or CheckPermission or some such here so that
> BeforeAfterKeyboardEvent interface isn't exposed to all the web pages.
> 

How about CheckPermissions="browser"?

[Constructor(DOMString typeArg,
 optional BeforeAfterKeyboardEventInit eventInitDict),
 CheckPermissions="browser"]
interface BeforeAfterKeyboardEvent : KeyboardEvent
......
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #117)
> > >+++ b/dom/webidl/BeforeAfterKeyboardEvent.webidl
> > >@@ -0,0 +1,22 @@
> > >+/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > >+/* 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/.
> > >+ */
> > >+
> > >+[Constructor(DOMString typeArg, optional BeforeAfterKeyboardEventInit eventInitDict)]
> > We need a Pref or CheckPermission or some such here so that
> > BeforeAfterKeyboardEvent interface isn't exposed to all the web pages.
> > 
> 
> How about CheckPermissions="browser"?
> 
> [Constructor(DOMString typeArg,
>  optional BeforeAfterKeyboardEventInit eventInitDict),
>  CheckPermissions="browser"]
> interface BeforeAfterKeyboardEvent : KeyboardEvent
> ......

After discussing with Kanru, we'd like to use "embed-apps" rather than "browser" since the event is only dispatched to a iframe which has both mozBrowser attribute (permission="browser") and mozApp attribute (permission="embed-apps"). If an iframe wants to get "embed-apps" permission, it must get "browser" permission[1]. So, we choose "embed-apps" permission here.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Using_the_Browser_API
(In reply to Olli Pettay [:smaug] from comment #116)
> >+
> >+  nsRefPtr<BeforeAfterKeyboardEvent> event =
> >+    BeforeAfterKeyboardEvent::Constructor(eventTarget, eventName, init);
> >+  if (event) {
> >+    bool dummy;
> >+    eventTarget->DispatchEvent(event->InternalDOMEvent(), &dummy);
> >+  }
> 
> Why don't you just dispatch using the BeforeAfterKeyboard struct.
> No need to create the DOM event manually

Hmm. Thanks for your suggestion.

> >+  // Build up iframe chain.
> >+  nsCOMPtr<nsINode> node = aTarget;
> >+  nsCOMPtr<nsIContent> content;
> >+  nsCOMArray<nsINode> chain;
> >+  while (node) {
> >+    content = do_QueryInterface(node);
> >+    if (content && content->IsHTML(nsGkAtoms::iframe)) {
> >+      chain.AppendElement(node);
> >+    }
> >+
> >+    nsPIDOMWindow* win = node->OwnerDoc()->GetWindow();
> >+    node = win ? win->GetFrameElementInternal() : nullptr;
> >+  }
> >+
> >+  // Dispatch before events from the outermost element and dispatch after events
> >+  // from the innermost element.
> >+  size_t length = chain.Length();
> >+  for (uint8_t i = 0, j = length - 1; i < length; i++, j--) {
> >+    node = aEvent->IsAfterKeyEvent() ? chain[i] : chain[j];
> >+
> >+    InternalBeforeAfterKeyboardEvent newEvent(true, aEvent->message, aEvent->widget);
> >+    newEvent.AssignBeforeAfterKeyEventData(*aEvent, false);
> >+    DispatchBeforeAfterKeyboardEvent(node, &newEvent, aStatus, aEventCB,
> >+                                     aEvent->mFlags.mDefaultPrevented);
> >+  }
> We don't want to dispatch the events to random iframes. Only some privileged
> which have the right permission.

In function DispatchBeforeAfterKeyboardEvent(), permission of the iframe is checked.

  // BeforeAfterKeyboardEvent should be only dispatched a iframe which has
  // mozBrowser attribute and mozApp attribute.
  nsCOMPtr<nsIDocShell> docShell = document->GetDocShell();
  if (NS_WARN_IF(!docShell) || !docShell->GetIsBrowserOrApp()) {
    return;
  }

As described in comment 118, the check is going to be

  if (NS_WARN_IF(!docShell) || !docShell->GetIsApp()) {
    return;
  }

One more thing, we're going to add new permission "embed-widgets" in bug 1005818, so I'll file a follow-up for it. 



> In general I don't quite understand the setup here.

As I described in comment 112, basically, I use event message to indicate what kind of keyboard event we're dispatching, and I set event message every time before calling PresShell::DispatchKeyboardEvent().

Here's an example. When a key is pressed, a WidgetKeyboardEvent is dispatched and handled in PresShell::HandleEventInternal(). Then, I set event message to NS_KEY_BEFORE_DOWN and pass the widget event to PresShell::DispatchKeyboardEvent().

In PresShell::DispatchKeyboardEvent(), I build an iframe chain, and then invoke PresShell::DispatchBeforeAfterKeyboardEvent() to dispatch before events (because the event message is NS_KEY_BEFORE_DOWN) to all ancestors of the event target. Note that ancestors must have "embed-apps" permission.

Then, restore the event message back to NS_KEY_DOWN and dispatch "keydown" event to event target in PresShell::DispatchKeyboardEvent().

After that, set event message again to NS_KEY_AFTER_DOWN, and re-invoke PresShell::DispatchKeyboardEvent() to dispatch after events (because the event message is  NS_KEY_AFTER_DOWN) to all ancestors of the event target. Again, the ancestors must have "embed-apps" permission.

The setup might be obscure. Maybe I should add a bool parameter like |aIsBefore| for PresShell::DispatchKeyboardEvent(). Any ideas?
Sorry for the delay. But I'm still busy for other bugs. Could you modify the patches with the reviews of Smaug?
Sure ;)

I am working on Patch 3.
Attachment #8469966 - Attachment is obsolete: true
Attachment #8469966 - Flags: review?(masayuki)
I'd like to add mEembeddedCancelled back to InternalBeforeAfterKeyboardEvent since it's used in Patch 3.

Should I create a new patch for reviewing or modify Patch 1&2 and then send review request again?
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
Attachment #8473628 - Flags: review?(masayuki)
Attachment #8473628 - Flags: review?(bugs)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #124)
> I'd like to add mEembeddedCancelled back to InternalBeforeAfterKeyboardEvent
> since it's used in Patch 3.
> 
> Should I create a new patch for reviewing or modify Patch 1&2 and then send
> review request again?
whichever is easier for you, I think.
Flags: needinfo?(bugs)
Comment on attachment 8473628 [details] [diff] [review]
Patch 3(v2): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

>+PresShell::DispatchKeyboardEvent(nsINode* aTarget,
>+                                 WidgetKeyboardEvent* aEvent,
>+                                 bool aIsBefore,
>+                                 bool aEmbeddedCancelled,
>+                                 nsEventStatus* aStatus,
>+                                 EventDispatchingCallback* aEventCB)
>+{
>+  if (aEvent->message == NS_KEY_PRESS) {
>+    EventDispatcher::Dispatch(static_cast<nsISupports*>(aTarget), mPresContext,
>+                              aEvent, nullptr, aStatus, aEventCB);
>+    return;
>+  }
>+
>+  MOZ_ASSERT(aTarget && aEvent);
>+  MOZ_ASSERT(aEvent->message == NS_KEY_DOWN || aEvent->message == NS_KEY_UP);
>+
>+  // Build up a target chain. Each item in the chain will receive a before/after
>+  // event later.
>+  nsCOMArray<Element> chain;
>+  bool dispatchKeyboardEvent = false;
>+  BuildTargetChainForBeforeAfterKeyboardEvent(aTarget, aIsBefore,
>+                                              chain, dispatchKeyboardEvent);
>+
>+  // Dispatch before events from the innermost element and dispatch after events
>+  // from the outermost element.
>+  uint32_t message = (aEvent->message == NS_KEY_DOWN) ?
>+                     (aIsBefore ? NS_KEY_BEFORE_DOWN : NS_KEY_AFTER_DOWN) :
>+                     (aIsBefore ? NS_KEY_BEFORE_UP : NS_KEY_AFTER_UP);
>+  bool embeddedCancelled = aEmbeddedCancelled;
>+  nsCOMPtr<Element> frameElement;
>+  size_t length = chain.Length();
>+  for (size_t i = 0, j = length - 1; i < length; i++, j--) {
>+    frameElement = aIsBefore ? chain[j] : chain[i];
>+
>+    // Dispatch before/after event. The value of mEmbeddedCancelled indicates
>+    // whether the previous event (which could be before event, after event,
>+    // or keydown/keyup event) is preventDefault() or not.
>+    InternalBeforeAfterKeyboardEvent newEvent(aEvent->mFlags.mIsTrusted,
>+                                              message, aEvent->widget);
>+    newEvent.AssignBeforeAfterKeyEventData(*aEvent, false);
>+    newEvent.mEmbeddedCancelled = embeddedCancelled;
>+    DispatchBeforeAfterKeyboardEvent(frameElement, &newEvent, aStatus, aEventCB);
>+    embeddedCancelled = newEvent.mFlags.mDefaultPrevented;
>+
>+    // Dispatch after event if before event is preventDefault().
>+    if (embeddedCancelled && aIsBefore) {
>+      WidgetKeyboardEvent afterEvent(aEvent->mFlags.mIsTrusted,
>+                                     aEvent->message, aEvent->widget);
>+      afterEvent.AssignKeyEventData(*aEvent, false);
>+      DispatchKeyboardEvent(frameElement, &afterEvent, false, false,
>+                            aStatus, aEventCB);
>+
>+      // Default action (dispatching keydown/keyup event) is prevented. So, no
>+      // need to forward the event to child process.
>+      aEvent->mFlags.mNoCrossProcessBoundaryForwarding = true;
>+      return;
>+    }
>+  }
>+
>+  if (!dispatchKeyboardEvent) {
>+    return;
>+  }
>+
>+  // Dispatch keydown/keyup event.
>+  aEvent->mFlags.mWantReplyFromContentProcess = true;
>+  EventDispatcher::Dispatch(static_cast<nsISupports*>(aTarget), mPresContext,
>+                            aEvent, nullptr, aStatus, aEventCB);
So here we dispatch the actual key event.


>+
>+  // Dispatch after events.
>+  WidgetKeyboardEvent afterEvent(aEvent->mFlags.mIsTrusted,
>+                                 aEvent->message, aEvent->widget);
>+  afterEvent.AssignKeyEventData(*aEvent, false);
Then we here make a copy of it


>+  DispatchKeyboardEvent(aTarget, &afterEvent, false,
>+                        aEvent->mFlags.mDefaultPrevented, aStatus, aEventCB);
>+}
And dispatch it again using DispatchKeyboardEvent. Dispatching the same even twice sounds like a bug to me ;)

And by calling DispatchKeyboardEvent again means we're doing the heavy 
BuildTargetChainForBeforeAfterKeyboardEvent operation again. We should certainly reuse the chain, that
would also make the API a bit easier to understand, IMO.








>+
>+void
> PresShell::DispatchTouchEvent(WidgetEvent* aEvent,
>                               nsEventStatus* aStatus,
>                               nsPresShellEventCB* aEventCB,
>                               bool aTouchIsNew)
> {
>   // calling preventDefault on touchstart or the first touchmove for a
>   // point prevents mouse events. calling it on the touchend should
>   // prevent click dispatching.
>diff --git a/layout/base/nsPresShell.h b/layout/base/nsPresShell.h
>index 76242ce..07fd0d9 100644
>--- a/layout/base/nsPresShell.h
>+++ b/layout/base/nsPresShell.h
>@@ -367,16 +367,25 @@ public:
>   virtual void EnsureImageInVisibleList(nsIImageLoadingContent* aImage) MOZ_OVERRIDE;
> 
>   virtual void RemoveImageFromVisibleList(nsIImageLoadingContent* aImage) MOZ_OVERRIDE;
> 
>   virtual bool AssumeAllImagesVisible() MOZ_OVERRIDE;
> 
>   virtual void RecordShadowStyleChange(mozilla::dom::ShadowRoot* aShadowRoot);
> 
>+  virtual void DispatchKeyboardEvent(
>+                 nsINode* aTarget,
>+                 mozilla::WidgetKeyboardEvent* aEvent,
>+                 bool aIsBefore,
>+                 bool aEmbeddedCancelled,
>+                 nsEventStatus* aStatus = nullptr,
>+                 mozilla::EventDispatchingCallback* aEventCB = nullptr)
>+                 MOZ_OVERRIDE;
>+
>   void SetNextPaintCompressed() { mNextPaintCompressed = true; }
> 
> protected:
>   virtual ~PresShell();
> 
>   void HandlePostedReflowCallbacks(bool aInterruptible);
>   void CancelPostedReflowCallbacks();
> 
>@@ -389,16 +398,22 @@ protected:
>   nsresult DidCauseReflow();
>   friend class nsAutoCauseReflowNotifier;
> 
>   void DispatchTouchEvent(mozilla::WidgetEvent* aEvent,
>                           nsEventStatus* aStatus,
>                           nsPresShellEventCB* aEventCB,
>                           bool aTouchIsNew);
> 
>+  void DispatchBeforeAfterKeyboardEvent(
>+         mozilla::dom::Element* aElement,
>+         mozilla::InternalBeforeAfterKeyboardEvent* aEvent,
>+         nsEventStatus* aStatus,
>+         mozilla::EventDispatchingCallback* aEventCB);
>+
>   void     WillDoReflow();
> 
>   /**
>    * Callback handler for whether reflow happened.
>    *
>    * @param aInterruptible Whether or not reflow interruption is allowed.
>    * @param aWasInterrupted Whether or not the reflow was interrupted earlier.
>    *
>-- 
>1.7.9.5
>
Attachment #8473628 - Flags: review?(bugs) → review-
Comment on attachment 8473628 [details] [diff] [review]
Patch 3(v2): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

I'll check this after you create new patch.
Attachment #8473628 - Flags: review?(masayuki)
Flags: needinfo?(masayuki)
Comment on attachment 8469948 [details] [diff] [review]
Patch 2(v8): Implementation of BeforeAfterKeyboardEvent

Per comment 124 and comment 125, I'll review the new patch.
Attachment #8469948 - Flags: review?(masayuki)
Attachment #8474513 - Flags: review?(masayuki)
Attachment #8474513 - Flags: review?(bugs)
This patch has been updated based on Smaug's feedbacks.
Attachment #8469948 - Attachment is obsolete: true
Attachment #8474516 - Flags: review?(masayuki)
This patch will be merged into Patch 2 after reviewing.
Attachment #8474517 - Flags: review?(masayuki)
Attachment #8474517 - Flags: review?(bugs)
Attachment #8473628 - Attachment is obsolete: true
Attachment #8474519 - Flags: review?(masayuki)
Attachment #8474519 - Flags: review?(bugs)
Comment on attachment 8474513 [details] [diff] [review]
Patch 1a(v1): Add mEmbeddedCancelled in InternalBeforeAfterKeyboardEvent

So you don't initialize mEmbeddedCancelled normally at all.
(does this really not cause warnings on tryserver logs?)
Attachment #8474513 - Flags: review?(bugs) → review-
Comment on attachment 8474517 [details] [diff] [review]
Patch 2a(v1): Set BeforeAfterKeyboardEvent.embeddedCancelled for after event.

You can just use mEvent->mEmbeddedCancelled in the DOM event.
Why we need mEmbeddedCancelled at all in the DOM event, when the same
member variable is in the event struct.
Attachment #8474517 - Flags: review?(bugs) → review-
Comment on attachment 8474519 [details] [diff] [review]
Patch 3(v3): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent)
> {
>   NS_ENSURE_TRUE(mFrameElement, true);
> 
>-  WidgetKeyboardEvent localEvent(event);
>+  WidgetKeyboardEvent localEvent(aEvent);
>   // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>   // being infinitely redispatched and forwarded to the child again.
>   localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
> 
>   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>   // to be able to dispatch it to the <browser> element as the target element.
>   nsIDocument* doc = mFrameElement->OwnerDoc();
>   nsIPresShell* presShell = doc->GetShell();
>-  NS_ENSURE_TRUE(presShell, true);
>-  nsPresContext* presContext = presShell->GetPresContext();
>-  NS_ENSURE_TRUE(presContext, true);
>-
>-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+  if (presShell) {
>+    if (localEvent.message == NS_KEY_PRESS) {
>+      nsPresContext* presContext = presShell->GetPresContext();
>+      EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+    } else {
>+      nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
>+      presShell->DispatchAfterKeyboardEvent(node, localEvent,
>+                                            aEvent.mFlags.mDefaultPrevented);
>+    }
I don't understand why you want to explicitly dispatch after events there.
And you don't dispatch the expected keydown/up events at all in this case.

>+static bool
>+CheckPermissionForBeforeAfterKeyboardEvent(Element* aElement)
>+{
>+  // An element which is chrome-privileged should be able to handle before
>+  // events and after events.
>+  nsCOMPtr<nsIDocument> document(aElement->OwnerDoc());
You don't need strong reference here, well, actually you don't need the
variable at all.

>+  nsCOMPtr<nsIPrincipal> principal(document->NodePrincipal());
Just do aElement->NodePrincipal()


>+  if (principal && nsContentUtils::IsSystemPrincipal(principal)) {
NodePrincipal is guaranteed to return non-null
Attachment #8474519 - Flags: review?(bugs) → review-
|mEmbeddedCancelled| is initialized only in the private c-tor accidentally. I add the initialization for the public c-tor in v2 patch.
Attachment #8474513 - Attachment is obsolete: true
Attachment #8474513 - Flags: review?(masayuki)
Attachment #8474926 - Flags: review?(masayuki)
Attachment #8474926 - Flags: review?(bugs)
According to feedbacks from Smaug in comment 134, I'd like to merget Patch 2 and Patch 2a.

No |mEmbeddedCancelled| in DOM event anymore. Just use mEvent->mEmbeddedCancelled to get the value of |BeforeAfterKeyboardEvent.embeddedCancelled|.
Attachment #8474516 - Attachment is obsolete: true
Attachment #8474517 - Attachment is obsolete: true
Attachment #8474516 - Flags: review?(masayuki)
Attachment #8474517 - Flags: review?(masayuki)
Attachment #8474985 - Flags: review?(masayuki)
Attachment #8474985 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #135)
> Comment on attachment 8474519 [details] [diff] [review]
> Patch 3(v3): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent
> 
> >+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent)
> > {
> >   NS_ENSURE_TRUE(mFrameElement, true);
> > 
> >-  WidgetKeyboardEvent localEvent(event);
> >+  WidgetKeyboardEvent localEvent(aEvent);
> >   // Set mNoCrossProcessBoundaryForwarding to avoid this event from
> >   // being infinitely redispatched and forwarded to the child again.
> >   localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
> > 
> >   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
> >   // to be able to dispatch it to the <browser> element as the target element.
> >   nsIDocument* doc = mFrameElement->OwnerDoc();
> >   nsIPresShell* presShell = doc->GetShell();
> >-  NS_ENSURE_TRUE(presShell, true);
> >-  nsPresContext* presContext = presShell->GetPresContext();
> >-  NS_ENSURE_TRUE(presContext, true);
> >-
> >-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
> >+  if (presShell) {
> >+    if (localEvent.message == NS_KEY_PRESS) {
> >+      nsPresContext* presContext = presShell->GetPresContext();
> >+      EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
> >+    } else {
> >+      nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
> >+      presShell->DispatchAfterKeyboardEvent(node, localEvent,
> >+                                            aEvent.mFlags.mDefaultPrevented);
> >+    }
> I don't understand why you want to explicitly dispatch after events there.
> And you don't dispatch the expected keydown/up events at all in this case.
> 

When a real key event is generated, before events are dispatched to it's ancestors in nsPresShell.cpp, and the actual key event is then dispatched to the event target. After that, we dispatch after events to it's ancestors again in reverse order. Two cases should be considered: in-process and out-of-process. For in-process case, after events are dispathed in nsPresShell.cpp, same as before events. And, for out-of-process case, rather than dispatching in nsPresShell.cpp, I dispatch after events when the parent process receies reply from the child process. Smaug, does it make sense to you?
Flags: needinfo?(bugs)
Attachment #8474519 - Attachment is obsolete: true
Attachment #8474519 - Flags: review?(masayuki)
Attachment #8475070 - Flags: review?(masayuki)
Attachment #8475070 - Flags: review?(bugs)
Attachment #8468372 - Attachment description: Patch 1: Implementation of WidgetBeforeAfterKeyboardEvent, r=smaug,masayuki → Patch 1: Implementation of WidgetBeforeAfterKeyboardEvent,(carry r+: smaug, masayuki)
Attachment #8468372 - Flags: review+
Attachment #8468372 - Attachment description: Patch 1: Implementation of WidgetBeforeAfterKeyboardEvent,(carry r+: smaug, masayuki) → Patch 1: Implementation of WidgetBeforeAfterKeyboardEvent (carry r+: smaug, masayuki)
Comment on attachment 8474926 [details] [diff] [review]
Patch 1a(v2): Add mEmbeddedCancelled in InternalBeforeAfterKeyboardEvent

>   void AssignBeforeAfterKeyEventData(
>          const WidgetKeyboardEvent& aEvent,
>          bool aCopyTargets)
>   {
>     AssignKeyEventData(aEvent, aCopyTargets);
>   }

Why don't you initialize it in here for consistency?
Attachment #8474926 - Flags: review?(masayuki) → review+
Comment on attachment 8474985 [details] [diff] [review]
Patch 2(v9): Implementation of BeforeAfterKeyboardEvent

>+// static
>+already_AddRefed<BeforeAfterKeyboardEvent>
>+BeforeAfterKeyboardEvent::Constructor(
>+                            EventTarget* aOwner,
>+                            const nsAString& aType,
>+                            const BeforeAfterKeyboardEventInit& aParam)
>+{
>+  nsRefPtr<BeforeAfterKeyboardEvent> event =
>+    new BeforeAfterKeyboardEvent(aOwner, nullptr, nullptr);
>+  ErrorResult rv;
>+  event->InitWithKeyboardEventInit(aOwner, aType, aParam, rv);
>+  NS_WARN_IF(rv.Failed());
>+
>+  if (!aParam.mEmbeddedCancelled.IsNull()) {
>+    event->mEvent->AsBeforeAfterKeyboardEvent()->mEmbeddedCancelled =
>+      aParam.mEmbeddedCancelled.Value();
>+  }

I know mEmbeddedCancelled is false if aParam.mEmbeddedCancelled is null.

How ever, for clearer to other developers, I like:

InternalBeforeAfterKeyboardEvent* internalEvent =
  event->mEvent->AsBeforeAfterKeyboardEvent();
internalEvent->mEmbeddedCancelled =
  !aParam.mEmbeddedCancelled.IsNull() && aParam.mEmbeddedCancelled.Value();

>+  bool IsBeforeKeyEvent()
>+  {
>+    return (message == NS_KEY_BEFORE_DOWN || message == NS_KEY_BEFORE_UP);
>+  }

Although, this is used only in debug build, i.e., you can wrap this with |#ifdef DEBUG|. But it's okay to me.
Attachment #8474985 - Flags: review?(masayuki) → review+
Feedbacks from Masayuki has been addressed. Please review the updated one. Thanks.
Attachment #8474926 - Attachment is obsolete: true
Attachment #8474926 - Flags: review?(bugs)
Attachment #8475630 - Flags: review?(bugs)
Attachment #8475630 - Attachment description: Patch 1a(v3): Add mEmbeddedCancelled in InternalBeforeAfterKeyboardEvent → Patch 1a(v3): Add mEmbeddedCancelled in InternalBeforeAfterKeyboardEvent, r=masayuki
Attachment #8468372 - Attachment description: Patch 1: Implementation of WidgetBeforeAfterKeyboardEvent (carry r+: smaug, masayuki) → Patch 1: Implementation of WidgetBeforeAfterKeyboardEvent, r=smaug,masayuki
Feedbacks in comment 141 for Patch 2 are addressed. Please review the updated patch.
Attachment #8474985 - Attachment is obsolete: true
Attachment #8474985 - Flags: review?(bugs)
Attachment #8475632 - Flags: review?(bugs)
Comment on attachment 8475070 [details] [diff] [review]
Patch 3(v4): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

>-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+  if (presShell) {
>+    if (localEvent.message == NS_KEY_PRESS) {
>+      nsPresContext* presContext = presShell->GetPresContext();
>+      EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+    } else {
>+      nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
>+      presShell->DispatchAfterKeyboardEvent(node, localEvent,
>+                                            aEvent.mFlags.mDefaultPrevented);
>+    }
>+  }

Why don't you use early return style here? Such as:

> if (NS_WARN_IF(!presShell) {
>   return true;
> }
> 
> if (localEvent.message == NS_KEY_PRESS) {
>   ...
>   return true;
> }
> 
> ...
> return true;
>@@ -849,16 +850,23 @@ public:
>    * Dispatch event to content only (NOT full processing)
>    * @note The caller must have a strong reference to the PresShell.
>    */
>   virtual nsresult HandleDOMEventWithTarget(nsIContent* aTargetContent,
>                                                         nsIDOMEvent* aEvent,
>                                                         nsEventStatus* aStatus) = 0;
> 
>   /**
>+   * Dispatch AfterKeyboardEvent with specific target.
>+   */
>+  virtual void DispatchAfterKeyboardEvent(nsINode* aTarget,
>+                                          const mozilla::WidgetKeyboardEvent& aEvent,
>+                                          bool aEmbeddedCancelled) = 0;
>+

You're adding a method to nsIPresShell. Therefore, you need to modify uuid of NS_IPRESSHELL_IID. (You can get new uuid with |./mach uuid|)

And also you should request super review for this change to smaug.

>+static int32_t
>+DispatchBeforeKeyboardEventInternal(const nsCOMArray<Element>& aChain,
>+                                    nsPresContext* aPresContext,
>+                                    const WidgetKeyboardEvent& aEvent)
>+{
>+  uint32_t message =
>+    (aEvent.message == NS_KEY_DOWN) ? NS_KEY_BEFORE_DOWN : NS_KEY_BEFORE_UP;
>+  uint32_t length = aChain.Length();

nit: size_t.

>+  nsCOMPtr<EventTarget> eventTarget;
>+  nsCOMPtr<nsIDOMEvent> domEvent;

Move domEvent definition to above EventDispatcher::CreateEvent().

>+  // Dispatch before events from the outermost element.
>+  for (int32_t i = length - 1; i >= 0; i--) {
>+    eventTarget = do_QueryInterface(aChain[i]->OwnerDoc()->GetWindow());
>+    if (!eventTarget) {
>+      continue;
>+    }
>+
>+    InternalBeforeAfterKeyboardEvent beforeEvent(aEvent.mFlags.mIsTrusted,
>+                                                 message, aEvent.widget);
>+    beforeEvent.AssignBeforeAfterKeyEventData(aEvent, false);
>+    beforeEvent.mFlags.mWantReplyFromContentProcess = true;
>+    EventDispatcher::CreateEvent(eventTarget, aPresContext,
>+                                 static_cast<WidgetEvent*>(&beforeEvent),

Do you really need this static_cast?

>+                                 EmptyString(), getter_AddRefs(domEvent));
>+    EventDispatcher::Dispatch(eventTarget, aPresContext, &beforeEvent,
>+                              domEvent);

Dispatching event might cause destroying aEvent.widget. So, you should check nsIWidget::Destroyed() and if it's true, you shouldn't dispatch event any more.

>+
>+    if (beforeEvent.mFlags.mDefaultPrevented) {
>+      return i;
>+    }
>+  }
>+
>+  return -1;

Use constant value, see below.

>+}
>+
>+static void
>+DispatchAfterKeyboardEventInternal(const nsCOMArray<Element>& aChain,
>+                                   nsPresContext* aPresContext,
>+                                   const WidgetKeyboardEvent& aEvent,
>+                                   bool aEmbeddedCancelled,
>+                                   uint32_t aStartOffset = 0)
>+{
>+  uint32_t message =
>+    (aEvent.message == NS_KEY_DOWN) ? NS_KEY_AFTER_DOWN : NS_KEY_AFTER_UP;
>+  bool embeddedCancelled = aEmbeddedCancelled;
>+  uint32_t length = aChain.Length();

nit: size_t.

>+  if (!length) {
>+    return;
>+  }
>+
>+  nsCOMPtr<EventTarget> eventTarget;
>+  nsCOMPtr<nsIDOMEvent> domEvent;

Defining domEvent should be moved to above line of EventDispatcher::CreateEvent().

>+    InternalBeforeAfterKeyboardEvent afterEvent(aEvent.mFlags.mIsTrusted,
>+                                                message, aEvent.widget);
>+    afterEvent.AssignBeforeAfterKeyEventData(aEvent, false);
>+    afterEvent.mEmbeddedCancelled = embeddedCancelled;
>+    afterEvent.mFlags.mWantReplyFromContentProcess = true;
>+    EventDispatcher::CreateEvent(eventTarget, aPresContext,
>+                                 static_cast<WidgetEvent*>(&afterEvent),

Do you really need this static_cast?

>+                                 EmptyString(), getter_AddRefs(domEvent));
>+    EventDispatcher::Dispatch(eventTarget, aPresContext, &afterEvent,
>+                              domEvent);

Dispatching event might cause destroying aEvent.widget. So, you should check nsIWidget::Destroyed() and if it's true, you shouldn't dispatch event any more.

>+
>+    embeddedCancelled = afterEvent.mFlags.mDefaultPrevented;
>+  }
>+}
>+
>+void
>+PresShell::DispatchAfterKeyboardEvent(nsINode* aTarget,
>+                                      const WidgetKeyboardEvent& aEvent,
>+                                      bool aEmbeddedCancelled)
>+{
>+  MOZ_ASSERT(aTarget);
>+  MOZ_ASSERT(aEvent.message == NS_KEY_DOWN || aEvent.message == NS_KEY_UP);
>+
>+  // Build up a target chain. Each item in the chain will receive an after event.
>+  nsCOMArray<Element> chain;

Um, it might be better to use nsAutoTArray<nsCOMPtr<Element>, 5?> for preventing memory fragmentation because this method called a lot!

>+static void
>+HandleKeyboardEvent(nsINode* aTarget,
>+                    nsPresContext* aPresContext,
>+                    WidgetKeyboardEvent& aEvent,
>+                    bool aEmbeddedCancelled,
>+                    nsEventStatus* aStatus,
>+                    EventDispatchingCallback* aEventCB)

I'm not sure why this and Dispatch* methods should be static rather than member methods of PresShell?

>+  MOZ_ASSERT(aTarget && aPresContext);
>+  MOZ_ASSERT(aEvent.message == NS_KEY_DOWN || aEvent.message == NS_KEY_UP);

The latter MOZ_ASSERT() is really a bug if it occurs. However, it's impossible to detect it in all over the key event handling which are dispatched from widget. So, for safety, I recommend you should add return statement if the message is unknown for release build.

>+  // Build up a target chain. Each item in the chain will receive a before event.
>+  nsCOMArray<Element> chain;

So, I think this should be nsAutoTArray<nsCOMPtr<Element>, 5?> too.

>+  bool targetIsIframe;
>+  BuildTargetChainForBeforeAfterKeyboardEvent(aTarget, chain, targetIsIframe);
>+
>+  // Dispatch before events. If each item in the chain handles the before
>+  // event and doesn't prevent its default action, then the return value of
>+  // the following function should be -1, and we should go further to dispatch
>+  // the actual key event and after events. Otherwise, dispatch after events
>+  // to partial items in the chain.
>+  int32_t offset =
>+    DispatchBeforeKeyboardEventInternal(chain, aPresContext, aEvent);

I think that aPresContext could be destroyed by the event handlers. So, I think that if aPresContext->GetPresShell() returns nullptr (and/or should check PresShell::mHaveShutDown?), you shouldn't dispatch DOM events anymore.

Smaug, if I'm wrong, please correct it.

>+  if (offset != -1) {

I don't like the magic number -1 here. It's better to define a static const value above the implementation of DispatchBeforeKeyboardEventInternal(), and its name should be kNobodyConsumedBeforeKeyboardEvent or something.

>+  if (!targetIsIframe) {

Looks like you can use early return style here if you won't add something by following patch(es).

>+    // Dispatch actual key event to event target.
>+    aEvent.mFlags.mWantReplyFromContentProcess = true;
>+    EventDispatcher::Dispatch(static_cast<nsISupports*>(aTarget), aPresContext,
>+                              &aEvent, nullptr, aStatus, aEventCB);

Same, this could cause destroying aPresContext.
Attachment #8475070 - Flags: review?(masayuki) → feedback?(bugs)
I'd like not to wrap IsBeforeKeyboardEvent() and IsAfterKeyboardEvent() with |#ifdef DEBUG| since they're also used in BeforeAfterKeyboardEvent::GetEmbeddedCancelled().
Attachment #8475632 - Attachment is obsolete: true
Attachment #8475632 - Flags: review?(bugs)
Attachment #8475655 - Flags: review?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #144)
> Comment on attachment 8475070 [details] [diff] [review]
> Patch 3(v4): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent
> 
> >-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
> >+  if (presShell) {
> >+    if (localEvent.message == NS_KEY_PRESS) {
> >+      nsPresContext* presContext = presShell->GetPresContext();
> >+      EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
> >+    } else {
> >+      nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
> >+      presShell->DispatchAfterKeyboardEvent(node, localEvent,
> >+                                            aEvent.mFlags.mDefaultPrevented);
> >+    }
> >+  }
> 
> Why don't you use early return style here? Such as:
> 
> > if (NS_WARN_IF(!presShell) {
> >   return true;
> > }
> > 
> > if (localEvent.message == NS_KEY_PRESS) {
> >   ...
> >   return true;
> > }
> > 
> > ...
> > return true;

Looks good to me ;)
Going to update my patch. Thanks.

> >@@ -849,16 +850,23 @@ public:
> >   /**
> >+   * Dispatch AfterKeyboardEvent with specific target.
> >+   */
> >+  virtual void DispatchAfterKeyboardEvent(nsINode* aTarget,
> >+                                          const mozilla::WidgetKeyboardEvent& aEvent,
> >+                                          bool aEmbeddedCancelled) = 0;
> >+
> 
> You're adding a method to nsIPresShell. Therefore, you need to modify uuid
> of NS_IPRESSHELL_IID. (You can get new uuid with |./mach uuid|)
> 
> And also you should request super review for this change to smaug.

Thanks for your reminding.
 
> >+static int32_t
> >+DispatchBeforeKeyboardEventInternal(const nsCOMArray<Element>& aChain,
> >+                                    nsPresContext* aPresContext,
> >+                                    const WidgetKeyboardEvent& aEvent)
> >+{
> >+  uint32_t message =
> >+    (aEvent.message == NS_KEY_DOWN) ? NS_KEY_BEFORE_DOWN : NS_KEY_BEFORE_UP;
> >+  uint32_t length = aChain.Length();
> 
> nit: size_t.

Fixed.

> >+  nsCOMPtr<EventTarget> eventTarget;
> >+  nsCOMPtr<nsIDOMEvent> domEvent;
> 
> Move domEvent definition to above EventDispatcher::CreateEvent().

Fixed.

> >+    EventDispatcher::CreateEvent(eventTarget, aPresContext,
> >+                                 static_cast<WidgetEvent*>(&beforeEvent),
> 
> Do you really need this static_cast?

Removed.

> >+                                 EmptyString(), getter_AddRefs(domEvent));
> >+    EventDispatcher::Dispatch(eventTarget, aPresContext, &beforeEvent,
> >+                              domEvent);
> 
> Dispatching event might cause destroying aEvent.widget. So, you should check
> nsIWidget::Destroyed() and if it's true, you shouldn't dispatch event any
> more.

Okay! I'll check it in the next patch.

> >+
> >+    if (beforeEvent.mFlags.mDefaultPrevented) {
> >+      return i;
> >+    }
> >+  }
> >+
> >+  return -1;
> 
> Use constant value, see below.

The setup is changed. A bool is returned to indicate whether before events are dispatched to each item in the chain. Please see the updated patch for further details.

> >+void
> >+PresShell::DispatchAfterKeyboardEvent(nsINode* aTarget,
> >+                                      const WidgetKeyboardEvent& aEvent,
> >+                                      bool aEmbeddedCancelled)
> >+{
> >+  MOZ_ASSERT(aTarget);
> >+  MOZ_ASSERT(aEvent.message == NS_KEY_DOWN || aEvent.message == NS_KEY_UP);
> >+
> >+  // Build up a target chain. Each item in the chain will receive an after event.
> >+  nsCOMArray<Element> chain;
> 
> Um, it might be better to use nsAutoTArray<nsCOMPtr<Element>, 5?> for
> preventing memory fragmentation because this method called a lot!

Okay! Updated in the next patch.

> >+static void
> >+HandleKeyboardEvent(nsINode* aTarget,
> >+                    nsPresContext* aPresContext,
> >+                    WidgetKeyboardEvent& aEvent,
> >+                    bool aEmbeddedCancelled,
> >+                    nsEventStatus* aStatus,
> >+                    EventDispatchingCallback* aEventCB)
> 
> I'm not sure why this and Dispatch* methods should be static rather than
> member methods of PresShell?

I set them to be static since these functions are only used in nsPresShell.cpp for now. However, member functions of PresShell is fine for me.

> >+  MOZ_ASSERT(aTarget && aPresContext);
> >+  MOZ_ASSERT(aEvent.message == NS_KEY_DOWN || aEvent.message == NS_KEY_UP);
> 
> The latter MOZ_ASSERT() is really a bug if it occurs. However, it's
> impossible to detect it in all over the key event handling which are
> dispatched from widget. So, for safety, I recommend you should add return
> statement if the message is unknown for release build.

Thanks for your suggestion. Fixed.

> >+  bool targetIsIframe;
> >+  BuildTargetChainForBeforeAfterKeyboardEvent(aTarget, chain, targetIsIframe);
> >+
> >+  // Dispatch before events. If each item in the chain handles the before
> >+  // event and doesn't prevent its default action, then the return value of
> >+  // the following function should be -1, and we should go further to dispatch
> >+  // the actual key event and after events. Otherwise, dispatch after events
> >+  // to partial items in the chain.
> >+  int32_t offset =
> >+    DispatchBeforeKeyboardEventInternal(chain, aPresContext, aEvent);
> 
> I think that aPresContext could be destroyed by the event handlers. So, I
> think that if aPresContext->GetPresShell() returns nullptr (and/or should
> check PresShell::mHaveShutDown?), you shouldn't dispatch DOM events anymore.
> 
> Smaug, if I'm wrong, please correct it.
> 

I don't quite understand this part. The event handler should be invoked whenever the actual key event is dispatched, right? If so, mPresContext should be still valid at this time. Please correct me if I was wrong.

> >+  if (offset != -1) {
> 
> I don't like the magic number -1 here. It's better to define a static const
> value above the implementation of DispatchBeforeKeyboardEventInternal(), and
> its name should be kNobodyConsumedBeforeKeyboardEvent or something.

The return value of DispatchBeforeKeyboardEventInternal() has been modified. Please see next patch.

> >+  if (!targetIsIframe) {
> 
> Looks like you can use early return style here if you won't add something by
> following patch(es).

Good suggestions. Thanks.
Attachment #8475831 - Flags: superreview?(bugs)
Attachment #8475831 - Flags: review?(masayuki)
Attachment #8475070 - Flags: review?(bugs)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #144) 
> I think that aPresContext could be destroyed by the event handlers. So, I
> think that if aPresContext->GetPresShell() returns nullptr (and/or should
> check PresShell::mHaveShutDown?), you shouldn't dispatch DOM events anymore.
> 
> Smaug, if I'm wrong, please correct it.


Yes, any event listener may kill prescontext/presshell. The objects itself should be kept alive by
some smart point on stack, but methods like GetPresShell may start to return null.
Flags: needinfo?(bugs)
Attachment #8475070 - Flags: feedback?(bugs)
Attachment #8475831 - Flags: superreview?(bugs) → review?(bugs)
Attachment #8475070 - Attachment is obsolete: true
Remove unnecessary event message check in ctor of BeforeAfterKeyboardEvent.
Attachment #8475655 - Attachment is obsolete: true
Attachment #8475655 - Flags: review?(bugs)
Attachment #8478115 - Flags: review?(bugs)
Attached patch Patch 4(WIP): Modify test cases. (obsolete) — Splinter Review
Two cases are modified:
1) dom/tests/mochitest/general/test_interfaces.html
2) dom/events/test/test_all_synthetic_events.html

I pushed all of my patches (including Patch 4) to try server (https://tbpl.mozilla.org/?tree=Try&rev=33c3bbae8659). Some mochitests are failed, and I'm not sure if I miss anything in test_interfaces.html.
Can you point me in the right direction? Thanks.

Besides, should there be any other test cases need to be modified, please let me know. :)
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
Comment on attachment 8475831 [details] [diff] [review]
Patch 3(v5): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

> bool
>-TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent)
> {

>-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+  if (presShell) {
>+    if (localEvent.message == NS_KEY_PRESS) {
>+      nsPresContext* presContext = presShell->GetPresContext();
>+      EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+    } else {
>+      nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
>+      presShell->DispatchAfterKeyboardEvent(node, localEvent,
>+                                            aEvent.mFlags.mDefaultPrevented);
>+    }
>+  }
>   return true;

nit: you can write this as:

if (!presShell) {
  return true;
}

if (localEvent.message == NS_KEY_PRESS) {
  nsPresContext* presContext = presShell->GetPresContext();
  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
} else {
  nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
  presShell->DispatchAfterKeyboardEvent(node, localEvent,
                                        aEvent.mFlags.mDefaultPrevented);
}
return true;

>+//a0a4b515-0b91-4f13-a060-4bfb3500dc00

I think that this comment isn't useful.

> #define NS_IPRESSHELL_IID \
>-		{ 0x42e9a352, 0x76f3, 0x4ba3, \
>-		  { 0x94, 0x0b, 0x78, 0x9e, 0x58, 0x38, 0x73, 0x4f } }
>+    { 0xa0a4b515, 0x0b91, 0x4f13, \
>+      { 0xa0, 0x60, 0x4b, 0xfb, 0x35, 0x00, 0xdc, 0x00 } }

Shoudl be outdented 2 spaces?

>+bool
>+PresShell::DispatchBeforeKeyboardEventInternal(const nsTArray<nsCOMPtr<Element> >& aChain,
>+                                               const WidgetKeyboardEvent& aEvent,
>+                                               size_t& aChainIndex)
>+{

>+    InternalBeforeAfterKeyboardEvent beforeEvent(aEvent.mFlags.mIsTrusted,
>+                                                 message, aEvent.widget);
>+    beforeEvent.AssignBeforeAfterKeyEventData(aEvent, false);
>+    beforeEvent.mFlags.mWantReplyFromContentProcess = true;
>+
>+    nsCOMPtr<nsIDOMEvent> domEvent;
>+    EventDispatcher::CreateEvent(eventTarget, mPresContext, &beforeEvent,
>+                                 EmptyString(), getter_AddRefs(domEvent));
>+    EventDispatcher::Dispatch(eventTarget, mPresContext, &beforeEvent,
>+                              domEvent);

After here, mPresContext may be nullptr. And mHaveShutDown may be true. If so, you shouldn't continue to dispatch events because the document is already unloaded.

>+void
>+PresShell::DispatchAfterKeyboardEventInternal(const nsTArray<nsCOMPtr<Element> >& aChain,
>+                                              const WidgetKeyboardEvent& aEvent,
>+                                              bool aEmbeddedCancelled,
>+                                              size_t aStartOffset)
>+{

>+    InternalBeforeAfterKeyboardEvent afterEvent(aEvent.mFlags.mIsTrusted,
>+                                                message, aEvent.widget);
>+    afterEvent.AssignBeforeAfterKeyEventData(aEvent, false);
>+    afterEvent.mEmbeddedCancelled = embeddedCancelled;
>+    afterEvent.mFlags.mWantReplyFromContentProcess = true;
>+
>+    nsCOMPtr<nsIDOMEvent> domEvent;
>+    EventDispatcher::CreateEvent(eventTarget, mPresContext, &afterEvent,
>+                                 EmptyString(), getter_AddRefs(domEvent));
>+    EventDispatcher::Dispatch(eventTarget, mPresContext, &afterEvent,
>+                              domEvent);

Same.

>+void
>+PresShell::DispatchAfterKeyboardEvent(nsINode* aTarget,
>+                                      const WidgetKeyboardEvent& aEvent,
>+                                      bool aEmbeddedCancelled)
>+{
>+  MOZ_ASSERT(aTarget);
>+
>+  if (aEvent.message != NS_KEY_DOWN && aEvent.message != NS_KEY_UP) {
>+    NS_WARNING("event message mistatch");

nit:

if (NS_WARN_IF(aEvent.message != NS_KEY_DOWN &&
               aEvent.message != NS_KEY_UP)) {

>+void
>+PresShell::HandleKeyboardEvent(nsINode* aTarget,
>+                               WidgetKeyboardEvent& aEvent,
>+                               bool aEmbeddedCancelled,
>+                               nsEventStatus* aStatus,
>+                               EventDispatchingCallback* aEventCB)
>+{

>+  size_t chainIndex;
>+  if (!DispatchBeforeKeyboardEventInternal(chain, aEvent, chainIndex)) {

Same, Check mPresContext and mHaveShutDown before calling DispatchAfterKeyboardEventInternal().

>+    // Dispatch after events to partial items.
>+    DispatchAfterKeyboardEventInternal(chain, aEvent,
>+                                       aEvent.mFlags.mDefaultPrevented, chainIndex);
>+
>+    // No need to forward the event to child process.
>+    aEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
>+    return;
>+  }

Same, Check mPresContext and mHaveShutDown before calling EventDispatcher::Dispatch().

>+  // Dispatch actual key event to event target.
>+  aEvent.mFlags.mWantReplyFromContentProcess = true;
>+  EventDispatcher::Dispatch(aTarget, mPresContext,
>+                            &aEvent, nullptr, aStatus, aEventCB);

And also here.

>+  // Dispatch after events to all items in the chain.
>+  DispatchAfterKeyboardEventInternal(chain, aEvent,
>+                                     aEvent.mFlags.mDefaultPrevented);
Attachment #8475831 - Flags: review?(masayuki) → review-
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #150)
> Created attachment 8478120 [details] [diff] [review]
> Patch 4(WIP): Modify test cases.
> 
> Two cases are modified:
> 1) dom/tests/mochitest/general/test_interfaces.html
> 2) dom/events/test/test_all_synthetic_events.html
> 
> I pushed all of my patches (including Patch 4) to try server
> (https://tbpl.mozilla.org/?tree=Try&rev=33c3bbae8659). Some mochitests are
> failed, and I'm not sure if I miss anything in test_interfaces.html.
> Can you point me in the right direction? Thanks.
> 
> Besides, should there be any other test cases need to be modified, please
> let me know. :)

It's confirmation if you want to expose the new BeforeAfterKeyboardEvent interface to web apps. Although, I'm not sure how do we do in this case. Let's wait Smaug's comment.
Flags: needinfo?(masayuki)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #151)
> Comment on attachment 8475831 [details] [diff] [review]
> Patch 3(v5): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent
> 
> > bool
> >-TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
> >+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent)
> > {
> 
> >-  EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
> >+  if (presShell) {
> >+    if (localEvent.message == NS_KEY_PRESS) {
> >+      nsPresContext* presContext = presShell->GetPresContext();
> >+      EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
> >+    } else {
> >+      nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
> >+      presShell->DispatchAfterKeyboardEvent(node, localEvent,
> >+                                            aEvent.mFlags.mDefaultPrevented);
> >+    }
> >+  }
> >   return true;
> 
> nit: you can write this as:
> 
> if (!presShell) {
>   return true;
> }
> 
> if (localEvent.message == NS_KEY_PRESS) {
>   nsPresContext* presContext = presShell->GetPresContext();
>   EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
> } else {
>   nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
>   presShell->DispatchAfterKeyboardEvent(node, localEvent,
>                                         aEvent.mFlags.mDefaultPrevented);
> }
> return true;

Oops! I forgot to add these changes into the updated patch. I'll make sure they are all included in the next version.
Sorry...  :(

> 
> >+//a0a4b515-0b91-4f13-a060-4bfb3500dc00
> 
> I think that this comment isn't useful.

Okay. Removed.

> > #define NS_IPRESSHELL_IID \
> >-		{ 0x42e9a352, 0x76f3, 0x4ba3, \
> >-		  { 0x94, 0x0b, 0x78, 0x9e, 0x58, 0x38, 0x73, 0x4f } }
> >+    { 0xa0a4b515, 0x0b91, 0x4f13, \
> >+      { 0xa0, 0x60, 0x4b, 0xfb, 0x35, 0x00, 0xdc, 0x00 } }
> 
> Shoudl be outdented 2 spaces?

I think so, but I'm not sure why we outdented 4 spaces before the latest commit.  :|
Well, let me use 2 spaces here. 

> >+bool
> >+PresShell::DispatchBeforeKeyboardEventInternal(const nsTArray<nsCOMPtr<Element> >& aChain,
> >+                                               const WidgetKeyboardEvent& aEvent,
> >+                                               size_t& aChainIndex)
> >+{
> 
> >+    InternalBeforeAfterKeyboardEvent beforeEvent(aEvent.mFlags.mIsTrusted,
> >+                                                 message, aEvent.widget);
> >+    beforeEvent.AssignBeforeAfterKeyEventData(aEvent, false);
> >+    beforeEvent.mFlags.mWantReplyFromContentProcess = true;
> >+
> >+    nsCOMPtr<nsIDOMEvent> domEvent;
> >+    EventDispatcher::CreateEvent(eventTarget, mPresContext, &beforeEvent,
> >+                                 EmptyString(), getter_AddRefs(domEvent));
> >+    EventDispatcher::Dispatch(eventTarget, mPresContext, &beforeEvent,
> >+                              domEvent);
> 
> After here, mPresContext may be nullptr. And mHaveShutDown may be true. If
> so, you shouldn't continue to dispatch events because the document is
> already unloaded.

Got it! Thanks~
Attachment #8475630 - Flags: review?(bugs) → review+
Flags: needinfo?(bugs)
Comment on attachment 8475831 [details] [diff] [review]
Patch 3(v5): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

it is better if I look at this after the comments from masayuki have been addressed.

(Sorry, this all takes a bit time and several iterations, but this stuff
is complicated and changing some of the core key event handling.)
Attachment #8475831 - Flags: review?(bugs)
Comment on attachment 8478115 [details] [diff] [review]
Patch 2(v10): Implementation of BeforeAfterKeyboardEvent, r=masayuki

>+  InternalBeforeAfterKeyboardEvent* internalEvent =
>+    event->mEvent->AsBeforeAfterKeyboardEvent();
>+  internalEvent->mEmbeddedCancelled =
>+    !aParam.mEmbeddedCancelled.IsNull() && aParam.mEmbeddedCancelled.Value();
This looks odd.
Why do we have nullable value in webidl interface, but we don't actually store the possible null value.
Could you perhaps make
InternalBeforeAfterKeyboardEvent::mEmbeddedCancelled nullable ...

>+BeforeAfterKeyboardEvent::GetEmbeddedCancelled() const
>+{
>+  Nullable<bool> embeddedCancelled;
>+  InternalBeforeAfterKeyboardEvent* internalEvent =
>+    mEvent->AsBeforeAfterKeyboardEvent();
>+  if (internalEvent->IsAfterKeyEvent()) {
>+    embeddedCancelled.SetValue(internalEvent->mEmbeddedCancelled);
>+  }
>+  return embeddedCancelled;
...and then just return the value here.
>+  if (internalEvent->IsAfterKeyEvent()) {
>+    embeddedCancelled.SetValue(internalEvent->mEmbeddedCancelled);
>+  }
is definitely wrong. The return value shouldn't depend on the event type.


Those fixed, r+
Attachment #8478115 - Flags: review?(bugs) → review+
Confirmed with EM/EPM, and this can be landed before FL.
Carry r+. (Both Patch 1 and Patch 1a has been reviewed by Smaug and Masayuki, and I'd like to merge them into one patch.)

According to the feedbacks from Smaug in comment 155, |mEmbeddedCancelled| is now a nullable variable in InternalBeforeAfterKeyboardEvent. Some minor changes are made in widget/nsGUIEventIPC.h. Please feel free to let me know if you have any concerns for those changes.
Attachment #8468372 - Attachment is obsolete: true
Attachment #8475630 - Attachment is obsolete: true
Attachment #8478948 - Flags: review+
Carry r+.

Some minor changes are done in class BeforeAfterKeyboardEvent since we'd like to make InternalBeforeAfterKeyboardEvent.mEmbeddedCancelled nullable. Also, helper functions like IsBeforeKeyboardEvent and IsAfterKeyboardEvent are removed because it turns out no one uses them.
Attachment #8478115 - Attachment is obsolete: true
Attachment #8478952 - Flags: review+
Feedbacks are addressed.

|mPresContext| and |mHaveShutdown| are always checked before dispatching an event.
Attachment #8475831 - Attachment is obsolete: true
Attachment #8478954 - Flags: review?(masayuki)
Attachment #8478954 - Flags: review?(bugs)
Attached patch Patch 4(v1): Modify test cases. (obsolete) — Splinter Review
Two test cases are updated.

Note that I've pushed these patches to try server (Please refer to the link in comment 150) and got warnings like:

2163 INFO TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/general/test_interfaces.html | If this is failing: DANGER, are you sure you want to expose the new interface BeforeAfterKeyboardEvent to all webpages as a property on the window (XBL: false)? Do not make a change to this file without a review from a DOM peer for that specific change!!! (or a JS peer for changes to ecmaGlobals) 2164 INFO dumping last 100 message(s)

How can I get rid of the warning/failure of the test case?
Attachment #8478120 - Attachment is obsolete: true
Attachment #8478956 - Flags: review?(masayuki)
Attachment #8478956 - Flags: review?(bugs)
Hi Fabrice,

We got a new proposal for keyboard event. Please refer to https://wiki.mozilla.org/WebAPI/BrowserAPI/KeyboardEvent for further details.

In short, each key event (excluding keypress event for now) is handled in the following order:
1) Dispatch a "mozbrowserbeforekeydown"/"mozbrowserbeforekeyup" to all ancestors of the event target

2) Dispatch "keydown"/"keyup" event to the event target. 

3) Dispatch "mozbrowserafterkeydown"/"mozbrowserafterkeyup" to all ancestors.

As a result, for most cases, shell.js will receive these new events rather than original keydown/keyup event. So, I add some event listeners in shell.js and make it send chrome event as usual.

Note that @dwi2 is working on Bug 1014418, making Gaia handle these new events and getting rid of mozChromeEvent for hardware keys. After Bug 1014418 is landed, I think we can remove these mozChromeEvents for hardware keys in shell.js. (I'll file a follow-up for this). Any suggestions?

Please let me know if you have any questions/feedbacks. Thanks.
Attachment #8478966 - Flags: review?(fabrice)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #160)
> 2163 INFO TEST-UNEXPECTED-FAIL |
> /tests/dom/tests/mochitest/general/test_interfaces.html | If this is
> failing: DANGER, are you sure you want to expose the new interface
> BeforeAfterKeyboardEvent to all webpages as a property on the window (XBL:
> false)? Do not make a change to this file without a review from a DOM peer
> for that specific change!!! (or a JS peer for changes to ecmaGlobals) 2164
> INFO dumping last 100 message(s)
> 
> How can I get rid of the warning/failure of the test case?
You must not expose the new interface to the web content...
but you have CheckPermissions="embed-apps system"

Add the interface to test_interfaces.html and expose it only in case of certain permission?
Comment on attachment 8478954 [details] [diff] [review]
Patch 3(v6): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

>+static bool
>+CheckPermissionForBeforeAfterKeyboardEvent(Element* aElement)

I forgot to say. I cannot review this method because I'm not familiar with the code.

>+static void
>+BuildTargetChainForBeforeAfterKeyboardEvent(nsINode* aTarget,
>+                                            nsTArray<nsCOMPtr<Element> >& aChain,
>+                                            bool& aTargetIsIframe)

And also this. This must be reviewed by smaug, though.

>+bool
>+PresShell::DispatchBeforeKeyboardEventInternal(const nsTArray<nsCOMPtr<Element> >& aChain,
>+                                               const WidgetKeyboardEvent& aEvent,
>+                                               size_t& aChainIndex)
>+{
>+  if (!mPresContext || mHaveShutDown) {
>+    return true;
>+  }

You return true here...

>+  uint32_t message =
>+    (aEvent.message == NS_KEY_DOWN) ? NS_KEY_BEFORE_DOWN : NS_KEY_BEFORE_UP;
>+  nsCOMPtr<EventTarget> eventTarget;
>+  // Dispatch before events from the outermost element.
>+  for (int32_t i = length - 1; i >= 0; i--) {
>+    eventTarget = do_QueryInterface(aChain[i]->OwnerDoc()->GetWindow());
>+    if (!eventTarget || !mPresContext || mHaveShutDown ||
>+        !aEvent.widget || aEvent.widget->Destroyed()) {
>+      return false;
>+    }

But you return false here... I guess that here should return true.

BTW, I think that you should create a method like PresShell::CanDispatchEvent(WidgetGUIEvent* aEvent). You should check mPresContext, mHaveShutDown and widget. Additionally, please test nsContentUtils::IsSafeToRunScript(). This is not necessary for you, but such name method should check this too.

So, only the event target should be tested outside of the method.

>+    EventDispatcher::Dispatch(eventTarget, mPresContext, &beforeEvent,
>+                              domEvent);
>+
>+    if (beforeEvent.mFlags.mDefaultPrevented) {
>+      return false;
>+    }

Oh, but you return false here. I don't like this. In widget, e.g., nsWindow::DispatchEvent() returns true if the event is consumed. So, could you return true if the event is consumed or dispatching event causes destroying the PresShell or widget.

>+void
>+PresShell::HandleKeyboardEvent(nsINode* aTarget,
>+                               WidgetKeyboardEvent& aEvent,
>+                               bool aEmbeddedCancelled,
>+                               nsEventStatus* aStatus,
>+                               EventDispatchingCallback* aEventCB)
>+  // Dispatch actual key event to event target.
>+  aEvent.mFlags.mWantReplyFromContentProcess = true;
>+  EventDispatcher::Dispatch(aTarget, mPresContext,
>+                            &aEvent, nullptr, aStatus, aEventCB);
>+
>+  // Dispatch after events to all items in the chain.
>+  DispatchAfterKeyboardEventInternal(chain, aEvent,
>+                                     aEvent.mFlags.mDefaultPrevented);
>+}

Between them, you should check if an event can be dispatched. I know DispatchAfterKeyboardEventInternal() will check it before dispatching event. However, somebody may insert something here. Then, it might be danger.
Attachment #8478954 - Flags: review?(masayuki) → review-
Comment on attachment 8478956 [details] [diff] [review]
Patch 4(v1): Modify test cases.

r=me, only for test_all_synthetic_events.html.
Attachment #8478956 - Flags: review?(masayuki) → review+
(In reply to Olli Pettay [:smaug] from comment #162)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #160)
> > 2163 INFO TEST-UNEXPECTED-FAIL |
> > /tests/dom/tests/mochitest/general/test_interfaces.html | If this is
> > failing: DANGER, are you sure you want to expose the new interface
> > BeforeAfterKeyboardEvent to all webpages as a property on the window (XBL:
> > false)? Do not make a change to this file without a review from a DOM peer
> > for that specific change!!! (or a JS peer for changes to ecmaGlobals) 2164
> > INFO dumping last 100 message(s)
> > 
> > How can I get rid of the warning/failure of the test case?
> You must not expose the new interface to the web content...
> but you have CheckPermissions="embed-apps system"
> 
> Add the interface to test_interfaces.html and expose it only in case of
> certain permission?

I've declared permission in test_interfaces.html, but still get the warning. (Please see patch 4 for further details. The patch is pretty short.) Did I miss anything?

 {name: "BeforeAfterKeyboardEvent", permission: "embed-apps system"},
Flags: needinfo?(bugs)
- Helper function is created (CanDispatchEvent)
- New function signature for DispatchBeforeKeyboardEventInternal is introduced.
Attachment #8478954 - Attachment is obsolete: true
Attachment #8478954 - Flags: review?(bugs)
Attachment #8479774 - Flags: review?(masayuki)
Attachment #8479774 - Flags: review?(bugs)
I got one question. After dispatching "keydown"/"keyup" to event target, I found the widget is destroyed, i.e. WidgetGUIEvent.widget is nullptr. In this case, no events should be dispatched afterwards. However, we need to dispatch after events though. What should I do?

Sorry for bothering you all again. I know it's not easy to follow all discussion details on Bugzilla :(

@Masayuki, any suggestions?

@Smaug, could you give me some feedbacks regard to the above question and also comment 138 (the reason why I create nsIPresShell::DispatchAfterKeyboardEvent and dispatch after event in TabParent.cpp)  and comment 160 (mochitest failure)?

Thanks.
Flags: needinfo?(masayuki)
Attachment #8478956 - Flags: review?(bugs) → review+
Flags: needinfo?(bugs)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #165)
> I've declared permission in test_interfaces.html, but still get the warning.
> (Please see patch 4 for further details. The patch is pretty short.) Did I
> miss anything?
> 
>  {name: "BeforeAfterKeyboardEvent", permission: "embed-apps system"},

On which platform do you see the error?
And I guess you need to just debug why you get that failure in the test.
Check which case is failing there.

(I see the failures where on b2g, so at least we're not leaking the interface name to desktop which is good.)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #167)
> I got one question. After dispatching "keydown"/"keyup" to event target, I
> found the widget is destroyed, i.e. WidgetGUIEvent.widget is nullptr. 
So not destroyed, but .widget is set to null.
Couldn't you just take a strong ref to it before the event is dispatched and use that
in the after* events
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #138)

> When a real key event is generated, before events are dispatched to it's
> ancestors in nsPresShell.cpp, and the actual key event is then dispatched to
> the event target. After that, we dispatch after events to it's ancestors
> again in reverse order. Two cases should be considered: in-process and
> out-of-process. For in-process case, after events are dispathed in
> nsPresShell.cpp, same as before events. And, for out-of-process case, rather
> than dispatching in nsPresShell.cpp, I dispatch after events when the parent
> process receies reply from the child process. Smaug, does it make sense to
> you?

Well, TabParent::RecvReplyKeyEvent is called only in special cases when mWantReplyFromContentProcess is true, and
in normal cases we shouldn't use that.

But looks like you set mWantReplyFromContentProcess in many cases. I don't yet understand the reason.
Reading the patch...
Or feel free to explain the reasons for mWantReplyFromContentProcess
Comment on attachment 8479774 [details] [diff] [review]
Patch 3(v7): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

In other words, mWantReplyFromContentProcess should be set to true only
in parent process, as its name says, but looks like it is used for something else in this patch, and I don't understand for what.
Doesn't the patch set mWantReplyFromContentProcess always to true, in
all the processes.

We don't want to dispatch any extra events if some pref isn't set, so
that we don't get the events in desktop FF, nor in Mobile FF.
And please cache the value of the pref.
Use AddBoolVarCache for that.
We also should return early from BuildTargetChainForBeforeAfterKeyboardEvent
if the pref isn't set.

>+    InternalBeforeAfterKeyboardEvent beforeEvent(aEvent.mFlags.mIsTrusted,
>+                                                 message, aEvent.widget);
>+    beforeEvent.AssignBeforeAfterKeyEventData(aEvent, false);
>+    beforeEvent.mFlags.mWantReplyFromContentProcess = true;
>+
>+    nsCOMPtr<nsIDOMEvent> domEvent;
>+    EventDispatcher::CreateEvent(eventTarget, mPresContext, &beforeEvent,
>+                                 EmptyString(), getter_AddRefs(domEvent));
Why do you need to create the DOM event?
Just dispatch 'beforeEvent'. DOM event will be created automatically if needed.

>+    InternalBeforeAfterKeyboardEvent afterEvent(aEvent.mFlags.mIsTrusted,
>+                                                message, aEvent.widget);
>+    afterEvent.AssignBeforeAfterKeyEventData(aEvent, false);
>+    afterEvent.mEmbeddedCancelled.SetValue(embeddedCancelled);
>+    afterEvent.mFlags.mWantReplyFromContentProcess = true;
>+
>+    nsCOMPtr<nsIDOMEvent> domEvent;
>+    EventDispatcher::CreateEvent(eventTarget, mPresContext, &afterEvent,
>+                                 EmptyString(), getter_AddRefs(domEvent));
ditto
Attachment #8479774 - Flags: review?(bugs) → review-
(Sorry, if the review comments are a bit picky, but this is very complicated stuff, and 
I certainly don't want to cause any regressions, including performance regressions, in desktop Firefox, or e10s Firefox. Also, need to figure out how to make this setup easier to understand.)
(In reply to Olli Pettay [:smaug] from comment #170)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #167)
> > I got one question. After dispatching "keydown"/"keyup" to event target, I
> > found the widget is destroyed, i.e. WidgetGUIEvent.widget is nullptr. 
> So not destroyed, but .widget is set to null.
> Couldn't you just take a strong ref to it before the event is dispatched and
> use that
> in the after* events

Thanks for your suggestion. :)

I'll use NS_ADDREF and NS_RELEASE to take a strong reference. Please correct me if I was wrong.
WidgetGUIEvent::widget is nsCOMPtr<nsIWidget>. So, it's always strong reference. Why does it become nullptr at dispatching the event??
Flags: needinfo?(masayuki)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #167)
> I got one question. After dispatching "keydown"/"keyup" to event target, I
> found the widget is destroyed, i.e. WidgetGUIEvent.widget is nullptr. In
> this case, no events should be dispatched afterwards. However, we need to
> dispatch after events though. What should I do?

I think you shouldn't dispatch afterkey events in such case because the document should be going unload.

> Sorry for bothering you all again. I know it's not easy to follow all
> discussion details on Bugzilla :(

No. Discussing in bugzilla is very useful for other developers especially when somebody needs to check why the original developer does so in the future. Then, they can get the reason from bugzilla. If it's in IRC, ML or private emails, it's hard they to find the discussion.
Comment on attachment 8479774 [details] [diff] [review]
Patch 3(v7): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent

I'll check this next patch which fixes the points smaug suggested.
Attachment #8479774 - Flags: review?(masayuki)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #176)
> WidgetGUIEvent::widget is nsCOMPtr<nsIWidget>. So, it's always strong
> reference. Why does it become nullptr at dispatching the event??

Actually, it happens only in OOP case. When event target is a remote iframe, we dispatch before events in parent process first, and then dispatch a key event in child process. After that, a local key event is created in parent process (TabParent::RecvReplyKeyEvent()) and dispatched then. In my patch, I changed the setup a little bit. Instead of dispatching the local key event, I invoke nsPresShell::DispatchAfterKeyboardEvent() to dispatch after events, however, WidgetKeyboardEvent.widget has becomed nullptr in TabParent at the time.

Should I add a static variable in nsIPresShell (like gKeyDownTarget) to take a strong reference for the above case?
Flags: needinfo?(masayuki)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #179)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #176)
> > WidgetGUIEvent::widget is nsCOMPtr<nsIWidget>. So, it's always strong
> > reference. Why does it become nullptr at dispatching the event??
> 
> Actually, it happens only in OOP case. When event target is a remote iframe,
> we dispatch before events in parent process first, and then dispatch a key
> event in child process. After that, a local key event is created in parent
> process (TabParent::RecvReplyKeyEvent()) and dispatched then. In my patch, I
> changed the setup a little bit. Instead of dispatching the local key event,
> I invoke nsPresShell::DispatchAfterKeyboardEvent() to dispatch after events,
> however, WidgetKeyboardEvent.widget has becomed nullptr in TabParent at the
> time.
> 
> Should I add a static variable in nsIPresShell (like gKeyDownTarget) to take
> a strong reference for the above case?

How about setting localEvent.widget to TabParent::GetWidget()?
(In reply to Olli Pettay [:smaug] from comment #173)
> Comment on attachment 8479774 [details] [diff] [review]
> Patch 3(v7): Dispatch KeyboardEvent and BeforeAfterKeyboardEvent
> 
> In other words, mWantReplyFromContentProcess should be set to true only
> in parent process, as its name says, but looks like it is used for something
> else in this patch, and I don't understand for what.
> Doesn't the patch set mWantReplyFromContentProcess always to true, in
> all the processes.
> 
> We don't want to dispatch any extra events if some pref isn't set, so
> that we don't get the events in desktop FF, nor in Mobile FF.
> And please cache the value of the pref.
> Use AddBoolVarCache for that.
> We also should return early from BuildTargetChainForBeforeAfterKeyboardEvent
> if the pref isn't set.

Thanks for your explanation. I'm totally agreed with you. mWantReplyFromContentProcess should be only set to true in parent process. And, it should be set to true only for key event, which is unnecessary for before events and after events. I'll remove them in the next patch. 

For those platforms which the pref is disabled, does the key events propogate between content and chrome?

> >+    InternalBeforeAfterKeyboardEvent beforeEvent(aEvent.mFlags.mIsTrusted,
> >+                                                 message, aEvent.widget);
> >+    beforeEvent.AssignBeforeAfterKeyEventData(aEvent, false);
> >+    beforeEvent.mFlags.mWantReplyFromContentProcess = true;
> >+
> >+    nsCOMPtr<nsIDOMEvent> domEvent;
> >+    EventDispatcher::CreateEvent(eventTarget, mPresContext, &beforeEvent,
> >+                                 EmptyString(), getter_AddRefs(domEvent));
> Why do you need to create the DOM event?
> Just dispatch 'beforeEvent'. DOM event will be created automatically if
> needed.

Okay! Thanks for your input.
> For those platforms which the pref is disabled, does the key events
> propogate between content and chrome?
Events don't propagate cross processes, by default.
mWantReplyFromContentProcess should still work as it works now (it is used for e10s)
Attachment #8478966 - Flags: review?(fabrice) → review+
Attachment #8479774 - Attachment is obsolete: true
Attachment #8481119 - Flags: review?(masayuki)
Attachment #8481119 - Flags: review?(bugs)
Attachment #8478956 - Attachment is obsolete: true
Attachment #8481120 - Flags: review+
Carry sr+ from smaug and r+ from masayuki.
Attachment #8478952 - Attachment is obsolete: true
Attachment #8481159 - Flags: superreview+
Attachment #8481159 - Flags: review+
Flags: needinfo?(masayuki)
Comment on attachment 8481159 [details] [diff] [review]
Patch 2: Implementation of BeforeAfterKeyboardEvent, sr=smaug, r=masayuki

Smaug, I've merged patch 4 (mochitests) into patch 2. Both of them have been r+'d, should I send a superreview request to you?

Since preference for enabling before/after events is used in patch 3, I'd like to update BeforeAfterKeyboardEvent.webidl and test_interfaces.html. Please feel free to let me know if something is wrong. Thanks.

BeforeAfterKeyboardEvent.webidl
+[Constructor(DOMString typeArg,
+ optional BeforeAfterKeyboardEventInit eventInitDict),
+ CheckPermissions="embed-apps system",
+ Pref="dom.beforeAfterKeyboardEvent.enabled"]

test_interfaces.html
+    {name: "BeforeAfterKeyboardEvent", b2g: true, permission: "embed-apps system", pref: "dom.beforeAfterKeyboardEvent.enabled"},
Flags: needinfo?(bugs)
Attachment #8481120 - Attachment is obsolete: true
Attachment #8481122 - Attachment description: Patch 5: Listen BeforeAfterKeyboardEvent and dispatch chrome event for hardward keys, r=fabrice → Patch 4: Listen BeforeAfterKeyboardEvent and dispatch chrome event for hardward keys, r=fabrice
Add initialization for |targetIsIframe|.
Attachment #8481119 - Attachment is obsolete: true
Attachment #8481119 - Flags: review?(masayuki)
Attachment #8481119 - Flags: review?(bugs)
Attachment #8481201 - Flags: review?(masayuki)
Attachment #8481201 - Flags: review?(bugs)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #187) 
> Since preference for enabling before/after events is used in patch 3, I'd
> like to update BeforeAfterKeyboardEvent.webidl and test_interfaces.html.
> Please feel free to let me know if something is wrong. Thanks.
Looks fine
Flags: needinfo?(bugs)
Comment on attachment 8481201 [details] [diff] [review]
Patch 3(v8): Dispatch BeforeAfterKeyboardEvent on b2g

>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent)
> {
>   NS_ENSURE_TRUE(mFrameElement, true);
> 
>-  WidgetKeyboardEvent localEvent(event);
>+  WidgetKeyboardEvent localEvent(aEvent);
>+  localEvent.widget = GetWidget();
>   // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>   // being infinitely redispatched and forwarded to the child again.
>   localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
> 
>   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>   // to be able to dispatch it to the <browser> element as the target element.
>   nsIDocument* doc = mFrameElement->OwnerDoc();
>   nsIPresShell* presShell = doc->GetShell();
>-  NS_ENSURE_TRUE(presShell, true);
>-  nsPresContext* presContext = presShell->GetPresContext();
>-  NS_ENSURE_TRUE(presContext, true);
>+  if (NS_WARN_IF(!presShell)) {
>+    return true;
>+  }
>+
>+  if (localEvent.message == NS_KEY_PRESS ||
>+      !PresShell::BeforeAfterKeyboardEventEnabled()) {
This looks wrong. The old code doesn't have 
localEvent.message == NS_KEY_PRESS check.
So we don't want to change the behavior for KEY_DOWN/UP even if PresShell::BeforeAfterKeyboardEventEnabled() is true.


>+  nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
>+  presShell->DispatchAfterKeyboardEvent(node, localEvent,
>+                                        aEvent.mFlags.mDefaultPrevented);
I still don't understand this. Why only After*
https://bugzilla.mozilla.org/show_bug.cgi?id=989198#c138 didn't explain that.
What happens to before events?
>+    InternalBeforeAfterKeyboardEvent afterEvent(aEvent.mFlags.mIsTrusted,
>+                                                message, aEvent.widget);
>+    afterEvent.AssignBeforeAfterKeyEventData(aEvent, false);
>+    afterEvent.mEmbeddedCancelled.SetValue(embeddedCancelled);
>+    EventDispatcher::Dispatch(eventTarget, mPresContext, &afterEvent, nullptr);
You shouldn't need null here

>+PresShell::HandleKeyboardEvent(nsINode* aTarget,
>+                               WidgetKeyboardEvent& aEvent,
>+                               bool aEmbeddedCancelled,
>+                               nsEventStatus* aStatus,
>+                               EventDispatchingCallback* aEventCB)
>+{
>+  if (aEvent.message == NS_KEY_PRESS) {
>+    EventDispatcher::Dispatch(static_cast<nsISupports*>(aTarget), mPresContext,
>+                              &aEvent, nullptr, aStatus, aEventCB);
>+    return;
>+  }
Couldn't you check there if beforeafter events are disabled and then just call ::dispatch immediately


Almost there I think.
Attachment #8481201 - Flags: review?(bugs) → review-
Thanks for your prompt response. :)

(In reply to Olli Pettay [:smaug] from comment #190)
> Comment on attachment 8481201 [details] [diff] [review]
> Patch 3(v8): Dispatch BeforeAfterKeyboardEvent on b2g
> 
> >+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent)
> > {
> >   NS_ENSURE_TRUE(mFrameElement, true);
> > 
> >-  WidgetKeyboardEvent localEvent(event);
> >+  WidgetKeyboardEvent localEvent(aEvent);
> >+  localEvent.widget = GetWidget();
> >   // Set mNoCrossProcessBoundaryForwarding to avoid this event from
> >   // being infinitely redispatched and forwarded to the child again.
> >   localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
> > 
> >   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
> >   // to be able to dispatch it to the <browser> element as the target element.
> >   nsIDocument* doc = mFrameElement->OwnerDoc();
> >   nsIPresShell* presShell = doc->GetShell();
> >-  NS_ENSURE_TRUE(presShell, true);
> >-  nsPresContext* presContext = presShell->GetPresContext();
> >-  NS_ENSURE_TRUE(presContext, true);
> >+  if (NS_WARN_IF(!presShell)) {
> >+    return true;
> >+  }
> >+
> >+  if (localEvent.message == NS_KEY_PRESS ||
> >+      !PresShell::BeforeAfterKeyboardEventEnabled()) {
> This looks wrong. The old code doesn't have 
> localEvent.message == NS_KEY_PRESS check.
> So we don't want to change the behavior for KEY_DOWN/UP even if
> PresShell::BeforeAfterKeyboardEventEnabled() is true.
> 
> 

It is exactly what I expect. For NS_KEY_DOWN/NS_KEY_UP, only after events should be dispatched here since we've dispatched before events in nsPresShell::HandleKeyEvent in parent process.

I listed few key steps and hope it helps to explain my idea.
For OOP case,
1) <chrome> a widget event is handled in nsPresShell::HandleEvent()
2) <chrome> before events is dispatched to ancestors
3) <chrome> TabParent::SendRealKeyEvent() with the widget event
4) <content> TabChild::RecvRealKeyEvent() with the widget event
5) <content> the widget event is handled in nsPresShell::HandleEvent()
6) <content> key event is dispatched to event target
7) <content> TabChild::SendReplyKeyEvent()
8) <chrome> TabParent::RecvRealKeyEvent()
9) <chrome> after events is dispatched to ancestors

For In-process case,
1) <chrome> a widget event is handled in nsPresShell::HandleEvent()
2) <chrome> before events is dispatched to ancestors
3) <chrome> key event is dispatched to event target
4) <chrome> after events is dispatched to ancestors

> >+  nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
> >+  presShell->DispatchAfterKeyboardEvent(node, localEvent,
> >+                                        aEvent.mFlags.mDefaultPrevented);
> I still don't understand this. Why only After*
> https://bugzilla.mozilla.org/show_bug.cgi?id=989198#c138 didn't explain that.
> What happens to before events?

Please see my above example.

> >+    InternalBeforeAfterKeyboardEvent afterEvent(aEvent.mFlags.mIsTrusted,
> >+                                                message, aEvent.widget);
> >+    afterEvent.AssignBeforeAfterKeyEventData(aEvent, false);
> >+    afterEvent.mEmbeddedCancelled.SetValue(embeddedCancelled);
> >+    EventDispatcher::Dispatch(eventTarget, mPresContext, &afterEvent, nullptr);
> You shouldn't need null here

Okay!

> >+PresShell::HandleKeyboardEvent(nsINode* aTarget,
> >+                               WidgetKeyboardEvent& aEvent,
> >+                               bool aEmbeddedCancelled,
> >+                               nsEventStatus* aStatus,
> >+                               EventDispatchingCallback* aEventCB)
> >+{
> >+  if (aEvent.message == NS_KEY_PRESS) {
> >+    EventDispatcher::Dispatch(static_cast<nsISupports*>(aTarget), mPresContext,
> >+                              &aEvent, nullptr, aStatus, aEventCB);
> >+    return;
> >+  }
> Couldn't you check there if beforeafter events are disabled and then just
> call ::dispatch immediately
> 

Good point! Let me update my next patch.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #191)
> > 
> > 
> 
> It is exactly what I expect. For NS_KEY_DOWN/NS_KEY_UP, only after events
> should be dispatched here since we've dispatched before events in
> nsPresShell::HandleKeyEvent in parent process.
I'm not really talking about the events you're interested in, but the existing behavior, which we must not regress even if the
new pref is enabled.


 
> I listed few key steps and hope it helps to explain my idea.
> For OOP case,
> 1) <chrome> a widget event is handled in nsPresShell::HandleEvent()
> 2) <chrome> before events is dispatched to ancestors
> 3) <chrome> TabParent::SendRealKeyEvent() with the widget event
> 4) <content> TabChild::RecvRealKeyEvent() with the widget event  
Note parent->child communication is asynchronous.

> 5) <content> the widget event is handled in nsPresShell::HandleEvent()
> 6) <content> key event is dispatched to event target
> 7) <content> TabChild::SendReplyKeyEvent()
> 8) <chrome> TabParent::RecvRealKeyEvent()
> 9) <chrome> after events is dispatched to ancestors
So why we need TabParent to dispatch any after events? doesn't the presshell in the parent process do that?



> For In-process case,
> 1) <chrome> a widget event is handled in nsPresShell::HandleEvent()
> 2) <chrome> before events is dispatched to ancestors
> 3) <chrome> key event is dispatched to event target
> 4) <chrome> after events is dispatched to ancestors
> 

This is simpler, and looks fine.
(In reply to Olli Pettay [:smaug] from comment #192)
> I'm not really talking about the events you're interested in, but the
> existing behavior, which we must not regress even if the
> new pref is enabled.

I think I understand your point of view but I'd like to confirm more deatils.

When the pref is disabled, the behaviour should be consistent with current behaviour, and we have no doubt about it.
1) a key event is dispatched in the parent process.
2) a key event is dispatched in the child process.
3) a key evnet is dispatched in the parent process.

However, when the pref is enabled, the behaviour is changed a little bit. Two possible ways for this case.

First, key event is only dispatched in the child process.
1) before events are dispatched in the parent process.
2) a key event is dispatched in the child process.
3) after events are dispatched in the parent process.

Second, in order to keep it consistent with the behaviour when the pref is disabled, key events are still dispatched in the parent process and new before/after events are also fired.
1) a key event is dispatched in the parent process.
1a) before events are dispatched in the parent process.
2) a key event is dispatched in the child process.
3) a key event is dispatched in the parent process.
3a) after events are dispatched in the parent process.

What I had done in patches is to implement the first proposal. But, my understanding is that you prefer the latter one since we don't want to regress the existing behaviour, right?

I think it's fine for me to follow the second proposal.

Well, modifications are required for b2g.js to adapt the new proposal.

> > I listed few key steps and hope it helps to explain my idea.
> > For OOP case,
> > 1) <chrome> a widget event is handled in nsPresShell::HandleEvent()
> > 2) <chrome> before events is dispatched to ancestors
> > 3) <chrome> TabParent::SendRealKeyEvent() with the widget event
> > 4) <content> TabChild::RecvRealKeyEvent() with the widget event  
> Note parent->child communication is asynchronous.
Yep.
> > 5) <content> the widget event is handled in nsPresShell::HandleEvent()
> > 6) <content> key event is dispatched to event target
> > 7) <content> TabChild::SendReplyKeyEvent()
> > 8) <chrome> TabParent::RecvRealKeyEvent()
> > 9) <chrome> after events is dispatched to ancestors
> So why we need TabParent to dispatch any after events? doesn't the presshell
> in the parent process do that?

Nope. After dispatching |before events| (step 2), the presshell in the parent process has been done with key event, and we go further to async IPC (step 3&4) to create key event in the child process (step 5&6). When the child process has done with the key event, async IPC again (step 7&8) and go back to parent process. |After events| are dispatched at the moment (step 9).

Note that, rather than dispatching |after events| BEFORE the child process handles the key event, we dispatch |after events| AFTER the child process handles the key event.

Besides, in TabParent::RecvReplyKeyEvent(), the local event is dispatched by EventDispatcher::Dispatcher rather than PresShell. So, the presshell doesn't handle the local key event and that's why I invoke DispatchAfterKeyboardEvent() manually to dispatch |after events| here.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #193)
> (In reply to Olli Pettay [:smaug] from comment #192)
> > I'm not really talking about the events you're interested in, but the
> > existing behavior, which we must not regress even if the
> > new pref is enabled.
> 
> I think I understand your point of view but I'd like to confirm more deatils.
> 
> When the pref is disabled, the behaviour should be consistent with current
> behaviour, and we have no doubt about it.
> 1) a key event is dispatched in the parent process.
> 2) a key event is dispatched in the child process.
> 3) a key evnet is dispatched in the parent process.
Right.

> 
> However, when the pref is enabled, the behaviour is changed a little bit.
> Two possible ways for this case.
> 
> First, key event is only dispatched in the child process.
> 1) before events are dispatched in the parent process.
> 2) a key event is dispatched in the child process.
> 3) after events are dispatched in the parent process.

When would a key event be dispatched to child process only?


> 
> Second, in order to keep it consistent with the behaviour when the pref is
> disabled, key events are still dispatched in the parent process and new
> before/after events are also fired.
When the pref is disabled, we don't fire before/after events. Bu I assume you mean enabled here.

> 1) a key event is dispatched in the parent process.
> 1a) before events are dispatched in the parent process.
Why this order? Shouldn't we dispatch a before event before key event

> 2) a key event is dispatched in the child process.
> 3) a key event is dispatched in the parent process.
> 3a) after events are dispatched in the parent process.
> 
> What I had done in patches is to implement the first proposal. But, my
> understanding is that you prefer the latter one since we don't want to
> regress the existing behaviour, right?
Right. We should just add some new events and not change anything in the existing key event handling.


> > > I listed few key steps and hope it helps to explain my idea.
> > > For OOP case,
> > > 1) <chrome> a widget event is handled in nsPresShell::HandleEvent()
> > > 2) <chrome> before events is dispatched to ancestors
> > > 3) <chrome> TabParent::SendRealKeyEvent() with the widget event
> > > 4) <content> TabChild::RecvRealKeyEvent() with the widget event  
> > Note parent->child communication is asynchronous.
> Yep.
> > > 5) <content> the widget event is handled in nsPresShell::HandleEvent()
> > > 6) <content> key event is dispatched to event target
> > > 7) <content> TabChild::SendReplyKeyEvent()
> > > 8) <chrome> TabParent::RecvRealKeyEvent()
> > > 9) <chrome> after events is dispatched to ancestors
> > So why we need TabParent to dispatch any after events? doesn't the presshell
> > in the parent process do that?
> 
> Nope. After dispatching |before events| (step 2), the presshell in the
> parent process has been done with key event, and we go further to async IPC
> (step 3&4) to create key event in the child process (step 5&6).
So in the patch what prevents presshell in the parent process to dispatch after events?
I mean, what guarantees we don't get after events twice?


> Besides, in TabParent::RecvReplyKeyEvent(), the local event is dispatched by
> EventDispatcher::Dispatcher rather than PresShell. So, the presshell doesn't
> handle the local key event and that's why I invoke
> DispatchAfterKeyboardEvent() manually to dispatch |after events| here.
...ah, that.

Ok, we need tests for this all.
(In reply to Olli Pettay [:smaug] from comment #194)
> > However, when the pref is enabled, the behaviour is changed a little bit.
> > Two possible ways for this case.
> > 
> > First, key event is only dispatched in the child process.
> > 1) before events are dispatched in the parent process.
> > 2) a key event is dispatched in the child process.
> > 3) after events are dispatched in the parent process.
> 
> When would a key event be dispatched to child process only?

I think the answer is no. A key event should be always grouped with a set of before/after events. (The number of before/after events depends on the DOM.

> > Second, in order to keep it consistent with the behaviour when the pref is
> > disabled, key events are still dispatched in the parent process and new
> > before/after events are also fired.
> When the pref is disabled, we don't fire before/after events. Bu I assume
> you mean enabled here.

Yep! Sorry for the confusion.

> > 1) a key event is dispatched in the parent process.
> > 1a) before events are dispatched in the parent process.
> Why this order? Shouldn't we dispatch a before event before key event

Okay! I think we should exchange the order of 1 and 1a.

> > 2) a key event is dispatched in the child process.
> > 3) a key event is dispatched in the parent process.
> > 3a) after events are dispatched in the parent process.
> > 
> > What I had done in patches is to implement the first proposal. But, my
> > understanding is that you prefer the latter one since we don't want to
> > regress the existing behaviour, right?
> Right. We should just add some new events and not change anything in the
> existing key event handling.

Ok! So, let's focus on proposal 2. I'll update patch to follow new proposal.

> > > > I listed few key steps and hope it helps to explain my idea.
> > > > For OOP case,
> > > > 1) <chrome> a widget event is handled in nsPresShell::HandleEvent()
> > > > 2) <chrome> before events is dispatched to ancestors
> > > > 3) <chrome> TabParent::SendRealKeyEvent() with the widget event
> > > > 4) <content> TabChild::RecvRealKeyEvent() with the widget event  
> > > Note parent->child communication is asynchronous.
> > Yep.
> > > > 5) <content> the widget event is handled in nsPresShell::HandleEvent()
> > > > 6) <content> key event is dispatched to event target
> > > > 7) <content> TabChild::SendReplyKeyEvent()
> > > > 8) <chrome> TabParent::RecvRealKeyEvent()
> > > > 9) <chrome> after events is dispatched to ancestors
> > > So why we need TabParent to dispatch any after events? doesn't the presshell
> > > in the parent process do that?
> > 
> > Nope. After dispatching |before events| (step 2), the presshell in the
> > parent process has been done with key event, and we go further to async IPC
> > (step 3&4) to create key event in the child process (step 5&6).
> So in the patch what prevents presshell in the parent process to dispatch
> after events?
> I mean, what guarantees we don't get after events twice?

|bool targetIsIframe| is used for this purpose.

It is set to true in BuildTargetChainForBeforeAfterKeyboardEvent() if the event target is an iframe. In this case, we dispatch before events and key event, but no after event. After events is dispatched when the child process handles the key event and send reply back to parent process, which is TabParent::RecvReplyKeyEvent().

> Ok, we need tests for this all.

Can we file a follow-up for test cases? I would like to land these patches with pref disabled first without breaking all existing test cases. Then, Gaia developers can enable the preference for testing and start working on those blockers. Does it make sense to you?
This is so complicated that I think we need some tests even if the pref is disabled.
One can always enable a pref for tests.

Writing tests for this shouldn't be that difficult or time consuming.
And writing tests tend to reveal some bugs ;)
This patches follow the proposal two in comment 193.
Attachment #8481201 - Attachment is obsolete: true
Attachment #8481201 - Flags: review?(masayuki)
Attachment #8482634 - Flags: review?(masayuki)
Attachment #8482634 - Flags: review?(bugs)
QA Whiteboard: [2.1-feature-slip+]
Kevin - Is this still planned to happen in 2.1? Just want to confirm if QA still needs to have resources allocated here to support testing in the 2.1 timeframe, given that this has missed the FL merge date.
Flags: needinfo?(khu)
Comment on attachment 8482634 [details] [diff] [review]
Patch 3(v9): Dispatch BeforeAfterKeyboardEvent on b2g

>-  NS_ENSURE_TRUE(presShell, true);
>+  if (NS_WARN_IF(!presShell)) {
>+    return true;
>+  }
No need for this change.


>   nsPresContext* presContext = presShell->GetPresContext();
>-  NS_ENSURE_TRUE(presContext, true);
>-
Why this change? Please don't remove the null check.



>   EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+
>+  if (PresShell::BeforeAfterKeyboardEventEnabled() &&
>+      localEvent.message != NS_KEY_PRESS) {
>+    nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));
mFrameElement is a node, no need for QI. (Element inherits nsINode)
>+CheckPermissionForBeforeAfterKeyboardEvent(Element* aElement)
>+{
>+  // An element which is chrome-privileged should be able to handle before
>+  // events and after events.
>+  nsCOMPtr<nsIPrincipal> principal(aElement->NodePrincipal());
You don't need nsCOMPtr here. nsIPrincipal* is fine..

>+BuildTargetChainForBeforeAfterKeyboardEvent(nsINode* aTarget,
>+                                            nsTArray<nsCOMPtr<Element> >& aChain,
>+                                            bool& aTargetIsIframe)
>+{
>+  nsCOMPtr<nsIContent> content(do_QueryInterface(aTarget));
>+  nsCOMPtr<nsPIDOMWindow> window(aTarget->OwnerDoc()->GetWindow());
>+  nsCOMPtr<Element> frameElement;
Why this needs to be nsCOMPtr?
>+
>+  // Initialize frameElement.
>+  if (content && content->IsHTML(nsGkAtoms::iframe)) {
>+    aTargetIsIframe = true;
>+    frameElement = do_QueryInterface(aTarget);
Just do frameElement = aTarget->AsElement();

>+  } else {
>+    // If event target is not an iframe, dispatch keydown/keyup event to its
>+    // window after dispatching before events to its ancestors.
>+    aTargetIsIframe = false;
>+
>+    // And skip the event target and get its parent frame.
>+    frameElement = window->GetFrameElementInternal();
Window can be in theory null here.

>+    if (frameElement) {
>+      window = frameElement->OwnerDoc()->GetWindow();
>+      frameElement = window ? window->GetFrameElementInternal() : nullptr;
And you in fact do null check here, but don't null check earlier.


But I really want to see testcases for this stuff. This is complicated.
Attachment #8482634 - Flags: review?(bugs) → review+
I was told by Jenny Liu that connected device team is targeting to land this by this week. 
Jenny / Joe, do you have comments about this? 

Thank you.
Flags: needinfo?(khu)
Flags: needinfo?(jeliu)
Flags: needinfo?(jcheng)
> But I really want to see testcases for this stuff. This is complicated.

Thanks for your effort and suggestions.

I'm working on the test cases and will send review requests once I complete them.
redirecting ni to mchen
but yes the goal is to ask for approval to land this
Flags: needinfo?(jcheng) → needinfo?(mchen)
(In reply to Kevin Hu [:khu] from comment #200)
> I was told by Jenny Liu that connected device team is targeting to land this
> by this week. 
> Jenny / Joe, do you have comments about this? 
> 

Hi Kevin,

After discussing with Gina, we decided to de-scope this feature from V2.1.
The reasons are
  1. This feature is not small but complex.
  2. This would be used by TV partner only. (as we know)

Thus we will choose to land it on master then rebase patches for V2.1 to TV partner.

Then could Kevin help to change the flags to proper values? Thanks.
Flags: needinfo?(mchen)
Flags: needinfo?(khu)
Flags: needinfo?(jeliu)
feature-b2g: 2.1 → 2.2?
Flags: needinfo?(khu)
Comment on attachment 8482634 [details] [diff] [review]
Patch 3(v9): Dispatch BeforeAfterKeyboardEvent on b2g

>+// Enable before keyboard events and after keyboard events.
>+pref("dom.beforeAfterKeyboardEvent.enabled", true);

"before_after_keyboard_event"?

I think a lot of prefs use "_" for word separators. But prefs which is used in webidls are usually not so, keep the current name. 

> bool
>-TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& aEvent)
> {
>   NS_ENSURE_TRUE(mFrameElement, true);
> 
>-  WidgetKeyboardEvent localEvent(event);
>+  WidgetKeyboardEvent localEvent(aEvent);
>+  localEvent.widget = GetWidget();
>   // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>   // being infinitely redispatched and forwarded to the child again.
>   localEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
> 
>   // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>   // to be able to dispatch it to the <browser> element as the target element.
>   nsIDocument* doc = mFrameElement->OwnerDoc();
>   nsIPresShell* presShell = doc->GetShell();
>-  NS_ENSURE_TRUE(presShell, true);
>+  if (NS_WARN_IF(!presShell)) {
>+    return true;
>+  }
>   nsPresContext* presContext = presShell->GetPresContext();
>-  NS_ENSURE_TRUE(presContext, true);
>-
>   EventDispatcher::Dispatch(mFrameElement, presContext, &localEvent);
>+
>+  if (PresShell::BeforeAfterKeyboardEventEnabled() &&
>+      localEvent.message != NS_KEY_PRESS) {
>+    nsCOMPtr<nsINode> node(do_QueryInterface(mFrameElement));

Is this mFrameElement safe here? mFrameElement isn't an nsRefPtr. So, may not it be broken by dispatching the keydown or keyup event?

>+/* static */ bool
>+PresShell::BeforeAfterKeyboardEventEnabled()
>+{
>+  static bool initialized = false;

nit: sInitialized

>+bool
>+PresShell::CanDispatchEvent(const WidgetGUIEvent* aEvent) const
>+{
>+  if (!aEvent || !aEvent->widget || aEvent->widget->Destroyed() ||
>+      !mPresContext || mHaveShutDown || !nsContentUtils::IsSafeToRunScript()) {
>+    return false;
>+  }
>+
>+  return true;

return aEvent && aEvent->widget && !aEvent->widget->Destroyed() &&
       mPresContext && !mHaveShutDown && nsContentUtils::IsSafeToRunScript();

>+}
>+
>+void
>+PresShell::HandleKeyboardEvent(nsINode* aTarget,
>+                               WidgetKeyboardEvent& aEvent,
>+                               bool aEmbeddedCancelled,
>+                               nsEventStatus* aStatus,
>+                               EventDispatchingCallback* aEventCB)
>+{
>+  if (aEvent.message == NS_KEY_PRESS ||
>+      !BeforeAfterKeyboardEventEnabled()) {
>+    EventDispatcher::Dispatch(aTarget, mPresContext,
>+                              &aEvent, nullptr, aStatus, aEventCB);
>+    return;
>+  }
>+
>+  MOZ_ASSERT(aTarget);
>+  MOZ_ASSERT(aEvent.message == NS_KEY_DOWN || aEvent.message == NS_KEY_UP);
>+
>+  // Build up a target chain. Each item in the chain will receive a before event.
>+  nsAutoTArray<nsCOMPtr<Element>, 5> chain;
>+  bool targetIsIframe = false;
>+  BuildTargetChainForBeforeAfterKeyboardEvent(aTarget, chain, targetIsIframe);
>+
>+  // Dispatch before events. If each item in the chain consumes the before
>+  // event and doesn't prevent the default action, we will go further to
>+  // dispatch the actual key event and after events in the reverse order.
>+  // Otherwise, only items which has handled the before event will receive an
>+  // after event.
>+  size_t chainIndex;
>+  bool defaultPrevented = false;
>+  DispatchBeforeKeyboardEventInternal(chain, aEvent, chainIndex,
>+                                      defaultPrevented);
>+
>+  // Dispatch after events to partial items.
>+  if (defaultPrevented) {
>+    DispatchAfterKeyboardEventInternal(chain, aEvent,
>+                                       aEvent.mFlags.mDefaultPrevented, chainIndex);
>+
>+    // No need to forward the event to child process.
>+    aEvent.mFlags.mNoCrossProcessBoundaryForwarding = true;
>+    return;
>+  }
>+
>+  // Event listeners may kill nsPresContext and nsPresShell.
>+  if (!mPresContext || mHaveShutDown) {
>+    return;
>+  }

Hmm, I'd like you to implement another CanDispatchEvent() which takes no arguments.

It checks only mPresContext, mHaveShutDown and nsContentUtils::IsSafeToRunScript().

The other should call this after checking the event and its widget.

>+
>+  // Dispatch actual key event to event target.
>+  aEvent.mFlags.mWantReplyFromContentProcess = true;
>+  EventDispatcher::Dispatch(aTarget, mPresContext,
>+                            &aEvent, nullptr, aStatus, aEventCB);
>+
>+  // Event listeners may kill nsPresContext and nsPresShell.
>+  if (targetIsIframe || !mPresContext || mHaveShutDown) {
>+    return;
>+  }

Then, you can use it here too.



If mFrameElement issue is not a problem, r=me. Otherwise, please make it safe.

FYI: I7ll be offline until 9/11. So, please ask additional review to smaug if it's necessary.
Attachment #8482634 - Flags: review?(masayuki) → review+
QA Whiteboard: [2.1-feature-slip+]
Flags: in-moztrap?(fyen)
Whiteboard: [FT:Stream3][2.1-feature-qa+] → [FT:Stream3]
nits and comments are addressed. Carry r+.
Attachment #8482634 - Attachment is obsolete: true
Attachment #8487836 - Flags: review+
Attached patch Patch 5(v1): Mochitest (obsolete) — Splinter Review
Attached patch Patch 5(v1): Mochitest (obsolete) — Splinter Review
Attachment #8487837 - Attachment is obsolete: true
Patch 1-4 are applied and pushed to try server:
https://tbpl.mozilla.org/?tree=Try&rev=22ea44000cf6

Apparently, some test cases are failed somehow. I'll take a look at them and make sure all are passed before pushing to m-i.
Comment on attachment 8487839 [details] [diff] [review]
Patch 5(v1): Mochitest

Smaug, I've added some testcases. Please help to review them. Thanks.
Attachment #8487839 - Flags: review?(bugs)
Comment on attachment 8487839 [details] [diff] [review]
Patch 5(v1): Mochitest

I don't see off-the-main thread tests here.
We really must have such tests too.
Attachment #8487839 - Flags: review?(bugs) → review-
off-the-main thread tests? Did you mean tests for OOP iframes, like <iframe mozbrowser remote>?
Yes, remote iframes, and then check how these new events work in the child process and in the parent.
The ordering is important case here.
Attached patch Patch 5(v2): Mochitest (obsolete) — Splinter Review
Add tests for remote iframes.
Attachment #8481159 - Attachment is obsolete: true
Attachment #8487839 - Attachment is obsolete: true
Attachment #8481159 - Attachment is obsolete: false
Comment on attachment 8489331 [details] [diff] [review]
Patch 5(v2): Mochitest

Review of attachment 8489331 [details] [diff] [review]:
-----------------------------------------------------------------

I found that two key events are dispatched to a remote iframe. That's why I define expectedEvents as two key events in testsForEventOrder. Should I file a bug for it?
Attachment #8489331 - Flags: review?(bugs)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #214)
> I found that two key events are dispatched to a remote iframe.
Why? With or also without your patches in this bug?
Comment on attachment 8489331 [details] [diff] [review]
Patch 5(v2): Mochitest

+
>+  var remoteIframe = document.createElement("iframe");
>+  remoteIframe.id = "remote_embedded";
>+  remoteIframe.src = "data:text/html,<html><body><div id='eventTarget'></div></body></html>";
>+  remoteIframe.setAttribute("remote", "true");
>+  SpecialPowers.wrap(remoteIframe).mozbrowser = true;
>+  remoteIframe.addEventListener("mozbrowserloadend", function onloadend() {
>+    remoteIframe.removeEventListener("mozbrowserloadend", onloadend);
>+    remoteIframe.focus();
>+    runTests();
>+    return;
>+  });
>+
>+  document.body.appendChild(remoteIframe);
>+  setupHandlers(remoteIframe);
I don't understand this setup. Where do we set the event listener in the child process?


>+function setupHandlers(win)
>+{
>+  win.addEventListener('keydown', handler);
>+  win.addEventListener('keyup', handler);
>+  win.addEventListener('mozbrowserbeforekeydown', handler);
>+  win.addEventListener('mozbrowserbeforekeyup', handler);
>+  win.addEventListener('mozbrowserafterkeydown', handler);
>+  win.addEventListener('mozbrowserafterkeyup', handler);
>+}
This function is very misleading, since 
win points occasionally to a window of object, but occasionally two an iframe element.
Attachment #8489331 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #215)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #214)
> > I found that two key events are dispatched to a remote iframe.
> Why? With or also without your patches in this bug?

Without my patches.
(In reply to Olli Pettay [:smaug] from comment #216)
> Comment on attachment 8489331 [details] [diff] [review]
> Patch 5(v2): Mochitest
> 
> +
> >+  var remoteIframe = document.createElement("iframe");
> >+  remoteIframe.id = "remote_embedded";
> >+  remoteIframe.src = "data:text/html,<html><body><div id='eventTarget'></div></body></html>";
> >+  remoteIframe.setAttribute("remote", "true");
> >+  SpecialPowers.wrap(remoteIframe).mozbrowser = true;
> >+  remoteIframe.addEventListener("mozbrowserloadend", function onloadend() {
> >+    remoteIframe.removeEventListener("mozbrowserloadend", onloadend);
> >+    remoteIframe.focus();
> >+    runTests();
> >+    return;
> >+  });
> >+
> >+  document.body.appendChild(remoteIframe);
> >+  setupHandlers(remoteIframe);
> I don't understand this setup. Where do we set the event listener in the
> child process?
> 

Just to make sure that child process does receive actual key events and no before/after events.

> >+function setupHandlers(win)
> >+{
> >+  win.addEventListener('keydown', handler);
> >+  win.addEventListener('keyup', handler);
> >+  win.addEventListener('mozbrowserbeforekeydown', handler);
> >+  win.addEventListener('mozbrowserbeforekeyup', handler);
> >+  win.addEventListener('mozbrowserafterkeydown', handler);
> >+  win.addEventListener('mozbrowserafterkeyup', handler);
> >+}
> This function is very misleading, since 
> win points occasionally to a window of object, but occasionally two an
> iframe element.

Let me rename the parameter. Thanks.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #218)
> 
> Just to make sure that child process does receive actual key events and no
> before/after events.
And we need to have listeners for all the event in the test both in child and parent processes and check the
right events are dispatched.

But why can't child process get before/after event in case the child process has right permissions and
key events are for example dispatched to some element in some iframe in the child process.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #217)
> (In reply to Olli Pettay [:smaug] from comment #215)
> > (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #214)
> > > I found that two key events are dispatched to a remote iframe.
> > Why? With or also without your patches in this bug?
> 
> Without my patches.

Well, in e10s there sure aren't two events in the child process.
(http://mozilla.pettay.fi/moztests/events/keycode-charcode-tester.html)
And couldn't see double events in the parent process either when testing it manually using
Scratchpad.
Could you debug where the extra events are coming from?
Does it happen for some key codes only, since I guess bug 862519 might have lead to that.
And if that is the case, then file a new bug and make it block bug 862519.
(In reply to Olli Pettay [:smaug] from comment #220)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #217)
> > (In reply to Olli Pettay [:smaug] from comment #215)
> > > (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #214)
> > > > I found that two key events are dispatched to a remote iframe.
> > > Why? With or also without your patches in this bug?
> > 
> > Without my patches.
> 
> Well, in e10s there sure aren't two events in the child process.
> (http://mozilla.pettay.fi/moztests/events/keycode-charcode-tester.html)
> And couldn't see double events in the parent process either when testing it
> manually using
> Scratchpad.
> Could you debug where the extra events are coming from?
> Does it happen for some key codes only, since I guess bug 862519 might have
> lead to that.
> And if that is the case, then file a new bug and make it block bug 862519.

(In reply to Olli Pettay [:smaug] from comment #219)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #218)
> > 
> > Just to make sure that child process does receive actual key events and no
> > before/after events.
> And we need to have listeners for all the event in the test both in child
> and parent processes and check the
> right events are dispatched.

Yep! I also add listeners for parent processes at the beginning of function prepareTest().

> But why can't child process get before/after event in case the child process
> has right permissions and
> key events are for example dispatched to some element in some iframe in the
> child process.

The answer is yes. Child process can get before/after events in your case.

For example,

<html>
<body>
  <iframe mozbrowser remote id="A">
    <iframe mozbrowser id="B"></iframe>
  </iframe>
</body>
</html>

Whenever iframe B is focused, the html itself and iframe A get before/after events, and iframe B gets key events.



However, it's different from my test case. My test case is like

<html>
<body>
  <iframe mozbrowser remote id="A"></iframe>
</body>
</html>

Thus, the expected results are child process gets key events and parent process gets before/after events. Does it make sense to you?
Sure, but we want to test also that case when there is the B iframe.
Sounds good. Let me add that case into my test case.
I found another interesting thing.

I create an iframe element with remote=true and assign its src to "bug989198_embedded.html", and then append to the dom tree. It gets different results no different applications.

On desktop/browser, the remote iframe is loaded successfully. However, on desktop-b2g, it failed to load the iframe and got the following error message:

NeckoParent::AllocPHttpChannelParent: FATAL error: App does not have permission: KILLING CHILD PROCESS



After taking some time to investigate the root cause, I found the appId is NO_APP_ID/0 somehow, so we failed to get validated app information, and, as a result, the webpage cann't be loaded successfully.



I think I should file a bug for fixing that, but I'm not sure how to skip some tests which are blocked by the above issue. If I add test case mentioned in comment 222 in same file, can we skip some tests by adding some magics in mochitest.ini? Should I separate them into another file and specify test metadata with "skip-if" in mochitest.ini?
Comment on attachment 8490508 [details] [diff] [review]
Patch 4a(v1): Let key events propogate to Gaia

Hi Fabrice,

I just realized something is missing in the last patch, and this part should be included to let Gaia handle key events. Please take a look at it and let me know if you have any concerns. Thanks.
Attachment #8490508 - Flags: review?(fabrice)
Comment on attachment 8490512 [details] [diff] [review]
Patch 3a(v1): Add new permission in PermissionsTable.

The patch is quite simple. Please take some time to review it.

New permission is used in patch 3, so we need to modify PermissionsTable. (I did add on my local branch for testing but forgot to put them into the patch for reviewing).
Attachment #8490512 - Flags: review?(bugs)
Comment on attachment 8490515 [details] [diff] [review]
Patch 3b(v1): Dispatch a key event only when the target is not an iframe

This patch is quite small and it fixes a bug in Patch 3.

Key events should be only dispatched to a non-iframe event target. When the event target is an iframe in parent process, it means that we'll handle the key event in child process later. And, child process is responsible to dispatch before events to ancestors (if any) and key event to the non-iframe event target.
Attachment #8490515 - Flags: review?(bugs)
Comment on attachment 8490512 [details] [diff] [review]
Patch 3a(v1): Add new permission in PermissionsTable.

I'm not familiar with the PermissionTable, so I don't quite know what
ALLOW_ACTION means there.
By default we shouldn't dispatch before/after events, even to system app.

Anyhow, I can't really review this patch.
Attachment #8490512 - Flags: review?(bugs)
Comment on attachment 8490515 [details] [diff] [review]
Patch 3b(v1): Dispatch a key event only when the target is not an iframe

Looks wrong. We should always dispatch the key event to the focused element.
The patch would make key events work differently in case
after/before events are enabled or not.
Attachment #8490515 - Flags: review?(bugs) → review-
Fabrice should review the permissions table changes.
Attachment #8490508 - Flags: review?(fabrice) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #233)
> Fabrice should review the permissions table changes.

Thanks, Kyle. ;) Let me send a requst to Fabrice.
Comment on attachment 8490512 [details] [diff] [review]
Patch 3a(v1): Add new permission in PermissionsTable.

Review of attachment 8490512 [details] [diff] [review]:
-----------------------------------------------------------------

The new permission is only available to certified app. We would like to have system app with the permission, so that system app is guaranteed to receive all before/after events no matter whether event target is in an app, browser frame, or iframe.
Attachment #8490512 - Flags: review?(fabrice)
Why does system app need to get the before/after events? Is there some use for them in phones?
I thought this all would be enabled at least for now in stingray only.

Another option is to enable the events in system app, but add a flag which tells whether there
are any listeners for these events, and only in that case dispatch the events.
That would be quite easy to implement. See EventListenerManager::AddEventListener
In other words, I'm just a bit worried about doing extra work (including IPC) when not actually needed.
(In reply to Olli Pettay [:smaug] from comment #236)
> Why does system app need to get the before/after events? Is there some use
> for them in phones?
> I thought this all would be enabled at least for now in stingray only.
> 

It's mainly for Stingray since there are many keys on the remote controller. Currently, all products share a Gaia repository and Stingray does have a system app.

We would like to let system app handle all before/after events to make sure some critical functions are always working.

One use case is lock screen. Lock screen is a plain iframe and it's appended to system app. When lock screen gets focused, users press power key and we expect system app to get before/after event and then turn off the screen. However, since lock screen is a plain iframe (without mozbrowser and mozapp), no "mozbrowser[before|after]key*" should be fired. Thus, system app cannot handle the power key properly...

Case 1. Lock Screen
  <iframe mozbrowser mozapp id='systemapp'>
    <iframe id='lockscreen'></iframe>
  </iframe>

One solution is to add new permission for system app. With the permission, we guarantee that it gets before/after events no matter if the event target is in a browser iframe.

With the permission control, we can also prevent a 3-rd party app to retrieve any before/after events when it embeds an plain iframe.

Case 2. Thired-party App Embeds a Page Which Requires Password
<iframe mozbrowser mozapp id='systemapp'>                            <-- before/after events
  <iframe mozbrowser mozapp id='thired-party-app'>                   <-- none
    <iframe id='input-password'></iframe>                            <-- key events
  </iframe>
</iframe>

> Another option is to enable the events in system app, but add a flag which
> tells whether there
> are any listeners for these events, and only in that case dispatch the
> events.
> That would be quite easy to implement. See
> EventListenerManager::AddEventListener

This solution fulfills Case 1 but no Case 2. We don't want to let a 3-rd party app to add any event listeners to retrive any key information for this case.
(In reply to Olli Pettay [:smaug] from comment #220)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #217)
> > (In reply to Olli Pettay [:smaug] from comment #215)
> > > (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #214)
> > > > I found that two key events are dispatched to a remote iframe.
> > > Why? With or also without your patches in this bug?
> > 
> > Without my patches.
> 
> Well, in e10s there sure aren't two events in the child process.
> (http://mozilla.pettay.fi/moztests/events/keycode-charcode-tester.html)
> And couldn't see double events in the parent process either when testing it
> manually using
> Scratchpad.
> Could you debug where the extra events are coming from?
> Does it happen for some key codes only, since I guess bug 862519 might have
> lead to that.
> And if that is the case, then file a new bug and make it block bug 862519.

Sorry for misleading. Some mistakes are made in my test case. No bug is found and which is good. ;)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #238)
> We would like to let system app handle all before/after events to make sure
> some critical functions are always working.
I can very much see reasons for this.
In single process Firefox the UI code can use capturing event listeners in default group
as "before" events, and system group as after. And they are both being used.
But that is very much internal to Gecko and Firefox, not exposed to the web.

So I can see reasons for exposing similar functionality in b2g.
Just need to be careful with performance.
Attachment #8490512 - Flags: review?(fabrice) → review+
Attachment #8490515 - Attachment is obsolete: true
Merge Patch 4, 4a, and 3a, and carry r+ from fabrice.
Attachment #8481122 - Attachment is obsolete: true
Attachment #8490508 - Attachment is obsolete: true
Attachment #8490512 - Attachment is obsolete: true
Attachment #8492046 - Flags: review+
Attached patch Patch 5(v3): Mochitest (obsolete) — Splinter Review
Currently, two test cases are added.

test_dom_before_after_keyboard_event.html:
  <iframe id='embedder'>
    <iframe id='embedded' src='bug989198_embedded.html'></iframe>
  </iframe>

test_dom_before_after_keyboard_event_remote.html:
  <iframe id='embedder'>
    <iframe id='remote_embedded' src='bug989198_embedded.html' remote></iframe>
  </iframe>

Befor/After events are sent to 'embedder' and key events are sent to 'embedded'. Note that, for remote case, we skip the latter test case since nested OOP is disabled for now.
Attachment #8489331 - Attachment is obsolete: true
Attachment #8492049 - Flags: review?(bugs)
Comment on attachment 8492049 [details] [diff] [review]
Patch 5(v3): Mochitest

So this is failing on try, so something needs to be fixed for sure.


>+const kUnknownEvent       = 0x000;
>+const kBeforeKeyDownEvent = 0x011;
>+const kKeyDownEvent       = 0x021;
>+const kAfterKeyDownEvent  = 0x041;
>+const kBeforeKeyUpEvent   = 0x012;
>+const kKeyUpEvent         = 0x022;
>+const kAfterKeyUpEvent    = 0x042;
>+const kParent             = 0x100;
>+const kChild              = 0x200;

Could you perhaps have flags for kKeyDown and kKeyUp and then | those to the different events.


>+function classifyEvents(test)
>+{
>+  // Categorize resultEvents into KEYDOWN group and KEYUP group.
>+  for (var i = 0; i < gCurrentTest.resultEvents.length ; i++) {
>+    var code = test.resultEvents[i];
>+    if ((code & 0xF) == 0x1) { // KEYDOWN
>+      test.classifiedEvents[0].push(code);
>+    } else if ((code & 0xF) == 0x2) { // KEYUP
>+      test.classifiedEvents[1].push(code);

and use kKeyDown/Up here then

>+var testsForEventOrder = [
>+  {
>+    description: "Testing the order of the events (OOP)",
>+    expectedEvents: [ [ kParent | kBeforeKeyDownEvent,
>+                        kParent | kKeyDownEvent,
>+                        kChild | kKeyDownEvent,
>+                        kParent | kKeyDownEvent,
>+                        kParent | kAfterKeyDownEvent ],
>+                      [ kParent | kBeforeKeyUpEvent,
>+                        kParent | kKeyUpEvent,
>+                        kChild | kKeyUpEvent,
>+                        kParent | kKeyUpEvent,
>+                        kParent | kAfterKeyUpEvent ] ],
So why does parent get two keydowns and keyups?
Comment 239 hinted it wasn't suppose to happen.


>+  synthesizeKey('a', {}, document.getElementById("remote_embedded").contentWindow);
>+  // It take some time to propagate events to a remote iframe.
>+  setTimeout(function() {
>+    classifyEvents(gCurrentTest);
>+    verifyResults(gCurrentTest);
>+    testEventOrder();
>+  }, 1000);
Would be better to use shorter timeout, and then just re-set the timer if we haven't got all the necessary events from the child process.
Attachment #8492049 - Flags: review?(bugs) → review-
> 
> >+var testsForEventOrder = [
> >+  {
> >+    description: "Testing the order of the events (OOP)",
> >+    expectedEvents: [ [ kParent | kBeforeKeyDownEvent,
> >+                        kParent | kKeyDownEvent,
> >+                        kChild | kKeyDownEvent,
> >+                        kParent | kKeyDownEvent,
> >+                        kParent | kAfterKeyDownEvent ],
> >+                      [ kParent | kBeforeKeyUpEvent,
> >+                        kParent | kKeyUpEvent,
> >+                        kChild | kKeyUpEvent,
> >+                        kParent | kKeyUpEvent,
> >+                        kParent | kAfterKeyUpEvent ] ],
> So why does parent get two keydowns and keyups?
> Comment 239 hinted it wasn't suppose to happen.
> 
Hi smaug,

Since Gina is on vacation, I will take care of this bug temporarily.
I found that parent get two keydowns and keyups is because mWantReplyFromContentProcess is set to true. In TabParent::RecvReplyKeyEvent(), the key events are dispatched again. I think this is not the expected behavior, right?
I think we cannot reuse mWantReplyFromContentProcess for dispatching after*event. We will need another new flag for this.

Please correct if I am wrong.

Thanks.
Flags: needinfo?(bugs)
Yes, that sounds right. I somehow missed that the patch sets mWantReplyFromContentProcess in PresShell,
although I complained about it in comment 171 and others.
mWantReplyFromContentProcess is really for the case like Bug 862519.
Flags: needinfo?(bugs)
Hi smaug,

Would you please review the patch3 again? I just added a new IPC message for dispatching afterkeyboard event.

Thanks.
Attachment #8487836 - Attachment is obsolete: true
Attachment #8497519 - Flags: review?(bugs)
Kershaw, could you upload an interdiff?
Flags: needinfo?(kechang)
Attached patch patch3 - interdiff (obsolete) — Splinter Review
Attachment #8498599 - Flags: review?(bugs)
Flags: needinfo?(kechang)
Blocks: 1075445
Blocks: 1075449
Blocks: 1076627
Blocks: 1076633
Comment on attachment 8497519 [details] [diff] [review]
Patch 3(v10): Dispatch BeforeAfterKeyboardEvent on b2g

>@@ -2357,16 +2357,18 @@ TabChild::RecvRealKeyEvent(const WidgetKeyboardEvent& event,
>   if (event.message == NS_KEY_DOWN) {
>     mIgnoreKeyPressEvent = status == nsEventStatus_eConsumeNoDefault;
>   }
> 
>   if (localEvent.mFlags.mWantReplyFromContentProcess) {
>     SendReplyKeyEvent(localEvent);
>   }
> 
>+  SendDispatchAfterKeyboardEvent(localEvent);
I we have before/after keyboard events disabled, we shouldn't call this.
So 
if (PresShell::BeforeAfterKeyboardEventEnabled()) {
  SendDispatchAfterKeyboardEvent(localEvent);
}
>+bool
>+TabParent::RecvDispatchAfterKeyboardEvent(const WidgetKeyboardEvent& aEvent)
>+{
>+  NS_ENSURE_TRUE(mFrameElement, true);
>+
>+  WidgetKeyboardEvent localEvent(aEvent);
>+  localEvent.widget = GetWidget();
>+
>+  nsIDocument* doc = mFrameElement->OwnerDoc();
>+  nsIPresShell* presShell = doc->GetShell();
This must be nsCOMPtr<nsIPresShell>
Otherwise the shell may die while we're still dispatching the events.
Attachment #8497519 - Flags: review?(bugs) → review+
Comment on attachment 8498599 [details] [diff] [review]
patch3 - interdiff

(gave conditional r+ to the other patch)
Attachment #8498599 - Flags: review?(bugs)
Review comments are fixed.
Attachment #8497519 - Attachment is obsolete: true
Attachment #8498599 - Attachment is obsolete: true
Attached patch Patch 5(v4): Mochitest (obsolete) — Splinter Review
Summary of changes:
1. Last review comments.
2. Some errors of last try server result are fixed.

Try server:
https://tbpl.mozilla.org/?tree=Try&rev=86e819d7b4c4
Attachment #8492049 - Attachment is obsolete: true
Attachment #8499656 - Flags: review?(bugs)
Attachment #8499656 - Flags: review?(bugs) → review+
Back! Thanks Kershaw for his help ;)
Comment on attachment 8499654 [details] [diff] [review]
Patch 3: Dispatch BeforeAfterKeyboardEvent on b2g, r=smaug

Carry r+ from Smaug.
Attachment #8499654 - Flags: review+
Rebase.
Attachment #8499654 - Attachment is obsolete: true
Attachment #8500221 - Flags: review+
Attached patch Patch 5: Mochitest, r=smaug (obsolete) — Splinter Review
Carry r+ from smaug.
Attachment #8499656 - Attachment is obsolete: true
Attachment #8500224 - Flags: review+
Update permission name to "before-after-keyboard-event".
Attachment #8481159 - Attachment is obsolete: true
Attachment #8500352 - Flags: superreview+
Attachment #8500352 - Flags: review+
Update permission name to "before-after-keyboard-event" in comment.
Attachment #8500221 - Attachment is obsolete: true
Attachment #8500353 - Flags: review+
The patch fixes the test case failure mentioned in comment 160.
Attachment #8500352 - Attachment is obsolete: true
Attachment #8500904 - Flags: review?(bugs)
Well, I'd like to set a default value for the preference and the patch is very very tiny.
Attachment #8500907 - Flags: review?(bugs)
Hi Fabrice,

It's just a one-line patch. Just notice that we got new app type which is trusted app, and, for each permission, we have to declare permission level for trusted app.
Attachment #8500908 - Flags: review?(fabrice)
Attachment #8500352 - Attachment is obsolete: false
Comment on attachment 8500904 [details] [diff] [review]
Patch 2a: Support multiple permissions in test_interfaces.html

Need a super review.
Attachment #8500904 - Flags: review?(bugs) → superreview?(bugs)
Comment on attachment 8500907 [details] [diff] [review]
Patch 3a: Disable preference by default

Why do we need this?
Don't we default to false when dealing with dom.beforeAfterKeyboardEvent.enabled?
If not, please fix that.

But we can have this too.
Attachment #8500907 - Flags: review?(bugs) → review+
Comment on attachment 8500904 [details] [diff] [review]
Patch 2a: Support multiple permissions in test_interfaces.html

>   var prefs = SpecialPowers.Services.prefs;
>   var version = SpecialPowers.Cc["@mozilla.org/xre/app-info;1"].getService(SpecialPowers.Ci.nsIXULAppInfo).version;
>   var isNightly = version.endsWith("a1");
>   var isRelease = !version.contains("a");
>   var isDesktop = !/Mobile|Tablet/.test(navigator.userAgent);
>   var isB2G = !isDesktop && !navigator.userAgent.contains("Android");
>   var hasPermission = function (aPermission) {
>-    return SpecialPowers.hasPermission(aPermission, window.document);
>+    var result = false;
>+    for (var p of aPermission) {
>+      result = result | SpecialPowers.hasPermission(p, window.document);
I'd prefer ||, not |, and could you rename aPermission to aPermissions
Attachment #8500904 - Flags: superreview?(bugs) → superreview+
Attachment #8500908 - Flags: review?(fabrice) → review+
(In reply to Olli Pettay [:smaug] from comment #267)
> Comment on attachment 8500907 [details] [diff] [review]
> Patch 3a: Disable preference by default
> 
> Why do we need this?
> Don't we default to false when dealing with
> dom.beforeAfterKeyboardEvent.enabled?
> If not, please fix that.
> 
> But we can have this too.

We did it for sBeforeAfterKeyboardEventEnabled, and I'd like to have both.
(In reply to Olli Pettay [:smaug] from comment #268)
> Comment on attachment 8500904 [details] [diff] [review]
> Patch 2a: Support multiple permissions in test_interfaces.html
> 
> >   var prefs = SpecialPowers.Services.prefs;
> >   var version = SpecialPowers.Cc["@mozilla.org/xre/app-info;1"].getService(SpecialPowers.Ci.nsIXULAppInfo).version;
> >   var isNightly = version.endsWith("a1");
> >   var isRelease = !version.contains("a");
> >   var isDesktop = !/Mobile|Tablet/.test(navigator.userAgent);
> >   var isB2G = !isDesktop && !navigator.userAgent.contains("Android");
> >   var hasPermission = function (aPermission) {
> >-    return SpecialPowers.hasPermission(aPermission, window.document);
> >+    var result = false;
> >+    for (var p of aPermission) {
> >+      result = result | SpecialPowers.hasPermission(p, window.document);
> I'd prefer ||, not |, and could you rename aPermission to aPermissions

Looks good to me. Thanks!
Merge Patch 2 and Patch 2a
Attachment #8500352 - Attachment is obsolete: true
Attachment #8500904 - Attachment is obsolete: true
Attachment #8501465 - Flags: superreview+
Attachment #8501465 - Flags: review+
Merge Patch 3 and 3a
Attachment #8500353 - Attachment is obsolete: true
Attachment #8500907 - Attachment is obsolete: true
Attachment #8501466 - Flags: review+
Merge Patch 4 and 4a
Attachment #8500222 - Attachment is obsolete: true
Attachment #8500908 - Attachment is obsolete: true
Attachment #8501467 - Flags: review+
The results of last try server link isn't shown somehow...

https://tbpl.mozilla.org/?tree=Try&rev=57e54a24730f
The results looks good to me. I propose to land Patch 1-5 first, and then add some more complicated test cases in follow-ups. Thoughts?
Flags: needinfo?(bugs)
Sound ok.
Flags: needinfo?(bugs)
While preparing to land these patches to inbound, I found a few bugs in mochitest. So, let's hold it for a while.
I've tested Patch 5a on b2g-emulator. Push to try server and see more results: https://tbpl.mozilla.org/?tree=Try&rev=f84011822239
Hi Smaug,

In patch 5, we didn't verify the value of attribute embeddedCancelled somehow. I've fixed it in this patch. Please help to review. Thanks.
Attachment #8502439 - Attachment is obsolete: true
Attachment #8503923 - Flags: review?(bugs)
Attachment #8503923 - Flags: review?(bugs) → review+
Depends on: 1082963
Depends on: 1083008
Backed out for causing bug 1083231. Please be sure to include the fixes for bug 1082963 and bug 1083008 as well when this re-lands.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a53ee57c736
Status: RESOLVED → REOPENED
Depends on: 1083231
Resolution: FIXED → ---
Target Milestone: 2.1 S3 (29aug) → ---
Comment on attachment 8506673 [details] [diff] [review]
Patch 6(v1): No need to forward KeyboardEvents in BrowserElementChildPreload.js

Hi Kanru,

Since the key events have already been dispatched in both parent and child, we don't have to forward key events back to parent in browser elements. Please help to review the patch. Thanks.
Attachment #8506673 - Flags: review?(kchen)
Comment on attachment 8506698 [details] [diff] [review]
Patch 4(v2): Dispatch both chrome event and key event for hardward keys, and add new permission.

Hi Fabrice,

In my last patch, we send mozChromeEvent for both mozbrowserbeforekey* and key* event, and it explains Tzu-Lin's observations in https://bugzilla.mozilla.org/show_bug.cgi?id=1083231#c11.

When we short press the power key,

1. dispatch 'mozbrowserbeforekeydown' event
   => sendChromeEvent, 1st sleep-button-press
2. dispatch 'mozbrowserbeforekeyup' event
   => sendchromeEvent, 1st sleep-button-release
3. dispatch 'keydown' event
   => sendChromeEvent, 2nd sleep-button-press
4. dispatch 'keyup' event
   => sendchromeEvent, 2nd sleep-button-release
5. dispatch 'keydown' event (This is a bug and should be fixed in Patch 6)
   => sendChromeEvent, 3rd sleep-button-press
6. dispatch 'keyup' event (This is a bug and should be fixed in Patch 6)
   => sendChromeEvent, 3rd sleep-button-release
7. dispatch 'mozbrowserafterkeydown' event
   => sendChromeEvent, 3rd sleep-button-press
8. dispatch 'mozbrowserafterkeyup' event
   => sendChromeEvent, 3rd sleep-button-release

With new patch, only one pair of mozChromeEvent are sent.

Please take some time on reviewing. Thanks.
Attachment #8506698 - Flags: review?(fabrice)
One more thing, when volume keys are pressed on a real device, keydown/keyup events are dispatched with keyCode of NS_VK_PAGE_DOWN/NS_VK_PAGE_UP (since volume keys are mapped to NS_VK_PAGE_DOWN/NS_VK_PAGE_UP in http://dxr.mozilla.org/mozilla-central/source/widget/gonk/GonkKeyMapping.h#52), and then the page is scrolled down/up a little bit.

Masayuki, can we change the key mapping in GonkKeyMapping? Thoughts?
Flags: needinfo?(masayuki)
Comment on attachment 8506673 [details] [diff] [review]
Patch 6(v1): No need to forward KeyboardEvents in BrowserElementChildPreload.js

Review of attachment 8506673 [details] [diff] [review]:
-----------------------------------------------------------------

Land this after Gaia is modified to use the new mechanism.
Attachment #8506673 - Flags: review?(kchen) → review+
try: -b do -p all -u all -t none
Blocks: 1084297
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #291)
> One more thing, when volume keys are pressed on a real device, keydown/keyup
> events are dispatched with keyCode of NS_VK_PAGE_DOWN/NS_VK_PAGE_UP (since
> volume keys are mapped to NS_VK_PAGE_DOWN/NS_VK_PAGE_UP in
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/GonkKeyMapping.
> h#52), and then the page is scrolled down/up a little bit.
> 
> Masayuki, can we change the key mapping in GonkKeyMapping? Thoughts?

If all key handlers will use KeyboardEvent.key for checking volume keys, you could do that. But perhaps, some applications would be broken by the change. I'm not a good person about this issue. Find a original author of the mapping. Note that key events dispatched from gonk/nsAppShell must have proper .key value.
Flags: needinfo?(masayuki)
FYI: At least for now, .key value for volume keys are stable in D3E spec. Although, they could be renamed if it would be necessary due to some issues.
https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent.key
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #295)
> (In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #291)
> > One more thing, when volume keys are pressed on a real device, keydown/keyup
> > events are dispatched with keyCode of NS_VK_PAGE_DOWN/NS_VK_PAGE_UP (since
> > volume keys are mapped to NS_VK_PAGE_DOWN/NS_VK_PAGE_UP in
> > http://dxr.mozilla.org/mozilla-central/source/widget/gonk/GonkKeyMapping.
> > h#52), and then the page is scrolled down/up a little bit.
> > 
> > Masayuki, can we change the key mapping in GonkKeyMapping? Thoughts?
> 
> If all key handlers will use KeyboardEvent.key for checking volume keys, you
> could do that. But perhaps, some applications would be broken by the change.
> I'm not a good person about this issue. Find a original author of the
> mapping. Note that key events dispatched from gonk/nsAppShell must have
> proper .key value.

Thanks for your input, and I'll ask @mwu for suggestions.

For B2G, instead of propogating the key events to Gaia, we send mozChromeEvent to System App, and only System App can handle these keys. So, I think no application would be broken by the change. :)
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #291)
> One more thing, when volume keys are pressed on a real device, keydown/keyup
> events are dispatched with keyCode of NS_VK_PAGE_DOWN/NS_VK_PAGE_UP (since
> volume keys are mapped to NS_VK_PAGE_DOWN/NS_VK_PAGE_UP in
> http://dxr.mozilla.org/mozilla-central/source/widget/gonk/GonkKeyMapping.
> h#52), and then the page is scrolled down/up a little bit.
> 

mwu, can we change the key mapping of volume keys in GonkKeyMapping.h? The .key value is quite stable in W3C spec as Masayuki said in comment 296. Thoughts?
Flags: needinfo?(mwu)
(In reply to Kan-Ru Chen [:kanru] from comment #292)
> Comment on attachment 8506673 [details] [diff] [review]
> Patch 6(v1): No need to forward KeyboardEvents in
> BrowserElementChildPreload.js
> 
> Review of attachment 8506673 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Land this after Gaia is modified to use the new mechanism.

Thanks for your prompt reply. Per discussion with Tzu-Lin, we'll land this first and get both key events and mozChromeEvent working. Then, in bug 1014418, changes are made to handle key events. After that, I'll file a follow-up to remove those mozChromeEvents sending from shell.js.
(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #298)
> mwu, can we change the key mapping of volume keys in GonkKeyMapping.h? The
> .key value is quite stable in W3C spec as Masayuki said in comment 296.
> Thoughts?

Sure.
Flags: needinfo?(mwu)
Please review the patch. Thanks.
Attachment #8509208 - Flags: review?(mwu)
Attachment #8509208 - Flags: review?(mwu) → review+
Hi Fabrice, key mapping of volume keys have been changed in Patch 7, so new keyCode for volumes keys are DOM_VK_VOLUME_UP/DOM_VK_VOLUME_DOWN. Please help to review the patch. Thanks.
Attachment #8506698 - Attachment is obsolete: true
Attachment #8506698 - Flags: review?(fabrice)
Attachment #8509337 - Flags: review?(fabrice)
Attachment #8509337 - Flags: review?(fabrice) → review+
Rebase.
Attachment #8501466 - Attachment is obsolete: true
Carry r+ from fabrice.
Attachment #8509337 - Attachment is obsolete: true
Attachment #8510080 - Flags: review+
Comment on attachment 8510078 [details] [diff] [review]
Patch 3: Dispatch BeforeAfterKeyboardEvent on b2g, r=smaug,masayuki

Review of attachment 8510078 [details] [diff] [review]:
-----------------------------------------------------------------

Carry r+ from smaug.
Attachment #8510078 - Flags: review+
Rebase and carry r+
Attachment #8501465 - Attachment is obsolete: true
Attachment #8510089 - Flags: superreview+
Attachment #8510089 - Flags: review+
Attached patch Patch 5: Mochitest, r=smaug (obsolete) — Splinter Review
Merge Patch 5 and 5a.
Attachment #8500224 - Attachment is obsolete: true
Attachment #8503923 - Attachment is obsolete: true
Attachment #8510091 - Flags: review+
Carry r+ from kchen.
Attachment #8506673 - Attachment is obsolete: true
Attachment #8510092 - Flags: review+
Carry r+ from mwu.
Attachment #8509208 - Attachment is obsolete: true
Attachment #8510093 - Flags: review+
Whiteboard: [FT:Stream3] → [ft:conndevices]
Blocks: 1091353
Attachment #8510091 - Attachment is obsolete: true
Attachment #8515724 - Flags: review+
Attachment #8510093 - Attachment is obsolete: true
Attachment #8515726 - Flags: review+
This patch is originated from bug 1082963.
Attachment #8515728 - Flags: review+
This patch is originated from bug 1083008.
Attachment #8515729 - Flags: review+
All patches are rebased. Push to try server to make sure everything is fine:

https://tbpl.mozilla.org/?tree=Try&rev=5162aea3c98d
If you found sth strange like bug 1084187, please update Gaia. It should be fixed in bug 1091353.
Blocks: 1093604
See Also: → 1094066
Blocks: 1103836
For patches for V2.1, Patch 8 and 9 are merged to Patch 2 and 5.
Blocks: 1106844
feature-b2g: 2.2? → ---
Blocks: 1120268
You need to log in before you can comment on or make changes to this bug.