Closed Bug 1155493 Opened 8 years ago Closed 8 years ago

Refactor event dispatching in Touch/SelectionCarets

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox40 --- affected
firefox41 --- fixed

People

(Reporter: TYLin, Assigned: mtseng)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(4 files, 16 obsolete files)

Part 3: Dispatch event when carets are updated, pressed, released, tap, longpressonemptycontent, hidden v2.
4.93 KB, patch
roc
: review+
Details | Diff | Splinter Review
Part 1: Add CaretStateChangedEvent and corresponding utility function v4. (carry sr+: smaug, r+: roc)
6.56 KB, patch
Details | Diff | Splinter Review
Part 4 Add test for mozbrowsercaretstatechanged v4. (carry r+: kanru)
6.16 KB, patch
Details | Diff | Splinter Review
Part 2: Event hook for mozbrowser element v7. (carry r+: kanru)
14.88 KB, patch
Details | Diff | Splinter Review 
Bug 1110039 refactors TouchCaret and SelectionCarets about the caret related parts. We will refactor the event dispatching mechanism in this bug.
Rebased.
Attachment #8595821 - Attachment is obsolete: true
Attachment #8595822 - Attachment is obsolete: true
Attachment #8595823 - Attachment is obsolete: true
Attachment #8605127 - Flags: review?(roc)
Attachment #8605125 - Flags: superreview?(bugs)
Attachment #8605125 - Flags: review?(roc)
Attachment #8605126 - Flags: review?(kchen)
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Comment on attachment 8605126 [details] [diff] [review]
Part 2: Event hook for mozbrowser element.

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

::: dom/browser-element/BrowserElementChild.js
@@ +37,5 @@
>    if (isTopBrowserElement(docShell) &&
>        Services.prefs.getBoolPref("dom.mozInputMethod.enabled")) {
>      try {
>        Services.scriptloader.loadSubScript("chrome://global/content/forms.js");
> +      Services.scriptloader.loadSubScript("chrome://global/content/BrowserElementCopyPaste.js");

Do you really want to load this script only if dom.mozInputMethod.enabled is true?

::: dom/browser-element/BrowserElementParent.js
@@ +87,5 @@
>  
>    Services.obs.addObserver(this, 'ask-children-to-exit-fullscreen', /* ownsWeak = */ true);
>    Services.obs.addObserver(this, 'oop-frameloader-crashed', /* ownsWeak = */ true);
>    Services.obs.addObserver(this, 'copypaste-docommand', /* ownsWeak = */ true);
> +  Services.obs.addObserver(this, 'copypaste-do-command', /* ownsWeak = */ true);

This name is very confusing..

@@ +444,5 @@
> +    function sendDoCommandMsg(cmd) {
> +      let data = { command: cmd };
> +      self._sendAsyncMsg('copypaste-do-command', data);
> +    }
> +    Cu.exportFunction(sendDoCommandMsg, evt.detail, { defineAs: 'sendDoCommandMsg' });

What is this for?
(In reply to Kan-Ru Chen [:kanru] from comment #7)
> Comment on attachment 8605126 [details] [diff] [review]
> Part 2: Event hook for mozbrowser element.
> 
> Review of attachment 8605126 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/browser-element/BrowserElementChild.js
> @@ +37,5 @@
> >    if (isTopBrowserElement(docShell) &&
> >        Services.prefs.getBoolPref("dom.mozInputMethod.enabled")) {
> >      try {
> >        Services.scriptloader.loadSubScript("chrome://global/content/forms.js");
> > +      Services.scriptloader.loadSubScript("chrome://global/content/BrowserElementCopyPaste.js");
> 
> Do you really want to load this script only if dom.mozInputMethod.enabled is
> true?
Nope, I'll move it!
> 
> ::: dom/browser-element/BrowserElementParent.js
> @@ +87,5 @@
> >  
> >    Services.obs.addObserver(this, 'ask-children-to-exit-fullscreen', /* ownsWeak = */ true);
> >    Services.obs.addObserver(this, 'oop-frameloader-crashed', /* ownsWeak = */ true);
> >    Services.obs.addObserver(this, 'copypaste-docommand', /* ownsWeak = */ true);
> > +  Services.obs.addObserver(this, 'copypaste-do-command', /* ownsWeak = */ true);
> 
> This name is very confusing..
Hmm, I'll find a better name for it. BTW, we'll remove old code if refactored is done. So 'copypaste-docommand' will be removed.
> 
> @@ +444,5 @@
> > +    function sendDoCommandMsg(cmd) {
> > +      let data = { command: cmd };
> > +      self._sendAsyncMsg('copypaste-do-command', data);
> > +    }
> > +    Cu.exportFunction(sendDoCommandMsg, evt.detail, { defineAs: 'sendDoCommandMsg' });
> 
> What is this for?
This provides a interface that embedder(gaia in our case) can call do-command such as 'cut', 'copy', 'paste', 'selectall'.
Comment on attachment 8605125 [details] [diff] [review]
Part 1: Add CaretStateChangedEvent and corresponding utility function.

>+AccessibleCaretManager::DispatchCaretStateChangedEvent(CaretChangedReason aReason) const
>+{
Could you add a MOZ_ASSERT(nsContentUtils::IsSafeToRunScript()); to
AccessibleCaretManager::DispatchCaretStateChangedEvent


>+  Selection* sel = GetSelection();
>+  if (!sel) {
>+    return;
>+  }
>+
>+  nsIDocument* doc = mPresShell->GetDocument();
>+  MOZ_ASSERT(doc);
>+
>+  CaretStateChangedEventInit init;
>+  init.mBubbles = true;
>+
>+  // XXX: Do we need to flush layout?
>+  mPresShell->FlushPendingNotifications(Flush_Layout);
>+  const nsRange* range = sel->GetAnchorFocusRange();
sel is possibly deleted object here. Flushing layout may run scripts, and delete worlds, and
sel is just a raw pointer. Same with doc.
And does something keep the AccessibleCaretManager object itself alive during the DispatchCaretStateChangedEvent() method call? If not, accessing
any of its member variables is unsafe.
Attachment #8605125 - Flags: superreview?(bugs) → superreview-
Addressed :smaug's comment.
Attachment #8605125 - Attachment is obsolete: true
Attachment #8605125 - Flags: review?(roc)
Attachment #8605622 - Flags: superreview?(bugs)
Attachment #8605622 - Flags: review?(roc)
Address :kanru's comment.
Attachment #8605126 - Attachment is obsolete: true
Attachment #8605126 - Flags: review?(kchen)
Attachment #8605624 - Flags: review?(kchen)
Comment on attachment 8605622 [details] [diff] [review]
Part 1: Add CaretStateChangedEvent and corresponding utility function v2.

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

::: layout/base/AccessibleCaretManager.cpp
@@ +905,5 @@
> +
> +  nsRefPtr<DOMRect> domRect = new DOMRect(ToSupports(doc));
> +  nsRect rect = nsContentUtils::GetSelectionBoundingRect(sel);
> +  if (commonAncestorNode) {
> +    nsIFrame* commonAncestorFrame = commonAncestorNode->AsContent()->GetPrimaryFrame();

commonAncestorFrame might be nullptr. Reproduce steps and the callstack here. https://pastebin.mozilla.org/8833432
Addressed :TYLin's comment by adding a null check.
Attachment #8605622 - Attachment is obsolete: true
Attachment #8605622 - Flags: superreview?(bugs)
Attachment #8605622 - Flags: review?(roc)
Attachment #8605739 - Flags: superreview?(bugs)
Attachment #8605739 - Flags: review?(roc)
Comment on attachment 8605739 [details] [diff] [review]
Part 1: Add CaretStateChangedEvent and corresponding utility function v3.

>+  if (commonAncestorNode) {
Perhaps before the 'if' add
if (!commonAncestorNode->IsContent()) {
  return;
}
Attachment #8605739 - Flags: superreview?(bugs) → superreview+
Comment on attachment 8605624 [details] [diff] [review]
Part 2: Event hook for mozbrowser element v2.

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

::: dom/browser-element/BrowserElementCopyPaste.js
@@ +47,5 @@
> +    e.stopPropagation();
> +
> +    let boundingClientRect = e.boundingClientRect;
> +    let canPaste = this._isCommandEnabled("paste");
> +    let zoomFactor = content.screen.width / content.innerWidth;

What if content.innerWidth is zero?

::: dom/browser-element/BrowserElementParent.js
@@ +437,5 @@
>    },
>  
> +  _handleCaretStateChanged: function(data) {
> +    let evt = this._createEvent('caretstatechanged', data.json,
> +                                /* cancelable = */ false);

I wish the format of the event.detail is documented somewhere.
Attachment #8605624 - Flags: review?(kchen) → review+
Depends on: 1166251
Depends on: 1165184
Addressed reviewer's comment.
Attachment #8605624 - Attachment is obsolete: true
Attachment #8607924 - Flags: review?(kchen) → review+
this failed to apply:

adding 1155493 to series file
renamed 1155493 -> Bug-1155493---Part-2-Event-hook-for-mozbrowser-ele.patch
applying Bug-1155493---Part-2-Event-hook-for-mozbrowser-ele.patch
patching file dom/browser-element/BrowserElementParent.js
Hunk #1 FAILED at 82
Hunk #2 FAILED at 193
Hunk #4 succeeded at 1003 with fuzz 1 (offset 5 lines).
2 out of 4 hunks FAILED -- saving rejects to file dom/browser-element/BrowserElementParent.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug-1155493---Part-2-Event-hook-for-mozbrowser-ele.patch

can you take a look, thanks!
Flags: needinfo?(mtseng)
Keywords: checkin-needed
Rebase to latest inbound.
Attachment #8608585 - Attachment is obsolete: true
I rebased my patch. Please checkin again, thanks.
Flags: needinfo?(mtseng)
Keywords: checkin-needed
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=10041405&repo=mozilla-inbound

seems this depend on https://bugzilla.mozilla.org/show_bug.cgi?id=1165184 somehow
Flags: needinfo?(mtseng)
Ahh, right, I didn't notice it. Sorry. Let wait bug 1165184 land!
Flags: needinfo?(mtseng)
Using pushPrefEnv to ensure the preferences are modified before re-run tests.
Attachment #8607924 - Attachment is obsolete: true
Fix try failed.
Attachment #8610331 - Attachment is obsolete: true
Set longer timeout because android is slow.
Attachment #8610358 - Attachment is obsolete: true
Depends on: 1168881
Comment on attachment 8610329 [details] [diff] [review]
Part 2: Event hook for mozbrowser element v6. (carry r+: kanru)

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

dom/ipc/jar.mn.rej must be added by accident.
Although bug1168881's patch is applied. I still encounter failed on android 4.3. Please see https://treeherder.mozilla.org/#/jobs?repo=try&revision=619dd91172d0

I found that copy/paste test for browser-element runs very slow on android. I double tests in part-4 patch and hit timeout issue on android. So I request longer timeout for this test. This is new try for this change.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84547af9933b
Blocks: 1169151
Looks great.
Keywords: checkin-needed
Blocks: 1170084
You need to log in before you can comment on or make changes to this bug.