Closed
Bug 1155493
Opened 9 years ago
Closed 9 years ago
Refactor event dispatching in Touch/SelectionCarets
Categories
(Core :: DOM: Selection, defect)
Core
DOM: Selection
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: TYLin, Assigned: mtseng)
References
(Blocks 1 open bug, )
Details
Attachments
(4 files, 16 obsolete files)
4.93 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
6.56 KB,
patch
|
Details | Diff | Splinter Review | |
6.16 KB,
patch
|
Details | Diff | Splinter Review | |
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.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
Rebased.
Attachment #8595821 -
Attachment is obsolete: true
Attachment #8595822 -
Attachment is obsolete: true
Attachment #8595823 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Rebased.
Assignee | ||
Comment 6•9 years ago
|
||
Rebased.
Assignee | ||
Updated•9 years ago
|
Attachment #8605127 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8605125 -
Flags: superreview?(bugs)
Attachment #8605125 -
Flags: review?(roc)
Assignee | ||
Updated•9 years ago
|
Attachment #8605126 -
Flags: review?(kchen)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mtseng
Status: NEW → ASSIGNED
Comment 7•9 years ago
|
||
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?
Assignee | ||
Comment 8•9 years ago
|
||
(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 9•9 years ago
|
||
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-
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
Address :kanru's comment.
Attachment #8605126 -
Attachment is obsolete: true
Attachment #8605126 -
Flags: review?(kchen)
Attachment #8605624 -
Flags: review?(kchen)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8605127 -
Attachment is obsolete: true
Attachment #8605127 -
Flags: review?(roc)
Attachment #8605625 -
Flags: review?(roc)
Reporter | ||
Comment 13•9 years ago
|
||
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
Assignee | ||
Comment 14•9 years ago
|
||
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 15•9 years ago
|
||
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+
Attachment #8605625 -
Flags: review?(roc) → review+
Comment 16•9 years ago
|
||
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+
Attachment #8605739 -
Flags: review?(roc) → review+
Assignee | ||
Comment 17•9 years ago
|
||
Addressed reviewer's comment.
Attachment #8605739 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
Addressed reviewer's comment.
Attachment #8605624 -
Attachment is obsolete: true
Assignee | ||
Comment 19•9 years ago
|
||
Attachment #8607924 -
Flags: review?(kchen)
Assignee | ||
Comment 20•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2edfbddb198b
Updated•9 years ago
|
Attachment #8607924 -
Flags: review?(kchen) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 21•9 years ago
|
||
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
Assignee | ||
Comment 23•9 years ago
|
||
Rebase to latest inbound.
Attachment #8608585 -
Attachment is obsolete: true
Assignee | ||
Comment 24•9 years ago
|
||
I rebased my patch. Please checkin again, thanks.
Flags: needinfo?(mtseng)
Keywords: checkin-needed
Comment 25•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e690405d314 https://hg.mozilla.org/integration/mozilla-inbound/rev/0d74bb9bb06f https://hg.mozilla.org/integration/mozilla-inbound/rev/ca51748d62b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/37cf24967016
Keywords: checkin-needed
Comment 26•9 years ago
|
||
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)
Comment 27•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7575b66cc5 https://hg.mozilla.org/integration/mozilla-inbound/rev/34b904a81f69 https://hg.mozilla.org/integration/mozilla-inbound/rev/e550da3fc7a6 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c323ae5636a
Assignee | ||
Comment 28•9 years ago
|
||
Ahh, right, I didn't notice it. Sorry. Let wait bug 1165184 land!
Flags: needinfo?(mtseng)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 29•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/896beb5088a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/58b7c1eaf3c8 https://hg.mozilla.org/integration/mozilla-inbound/rev/0380b1684e6b https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd7adb9f591
Keywords: checkin-needed
Comment 30•9 years ago
|
||
Backed out for Android test_browserElement_inproc_CopyPaste.html timeouts/crashes. https://treeherder.mozilla.org/logviewer.html#?job_id=10127627&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/cb33de12c0b5
Assignee | ||
Comment 32•9 years ago
|
||
Using pushPrefEnv to ensure the preferences are modified before re-run tests.
Attachment #8607924 -
Attachment is obsolete: true
Assignee | ||
Comment 33•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc5ed6f9004f
Assignee | ||
Comment 34•9 years ago
|
||
Fix try failed.
Attachment #8610331 -
Attachment is obsolete: true
Assignee | ||
Comment 35•9 years ago
|
||
Set longer timeout because android is slow.
Attachment #8610358 -
Attachment is obsolete: true
Reporter | ||
Comment 36•9 years ago
|
||
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.
Assignee | ||
Comment 37•9 years ago
|
||
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
Assignee | ||
Comment 38•9 years ago
|
||
Remove jar.mn.rej.
Attachment #8610329 -
Attachment is obsolete: true
Comment 40•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c816a4349211 https://hg.mozilla.org/integration/mozilla-inbound/rev/970957a8ad2a https://hg.mozilla.org/integration/mozilla-inbound/rev/6e532292984f https://hg.mozilla.org/integration/mozilla-inbound/rev/00805b1f18da
Keywords: checkin-needed
Comment 41•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c816a4349211 https://hg.mozilla.org/mozilla-central/rev/970957a8ad2a https://hg.mozilla.org/mozilla-central/rev/6e532292984f https://hg.mozilla.org/mozilla-central/rev/00805b1f18da
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•