Closed
Bug 1046344
Opened 8 years ago
Closed 8 years ago
Unbitrot APZ glue code for Fennec
Categories
(Core Graveyard :: Widget: Android, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla34
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(4 files, 3 obsolete files)
5.33 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
28.98 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
2.65 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
30.71 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
The glue code that I wrote eons ago for getting APZ working with Fennec has fallen into a state of severe disrepair. It needs to be cleaned up and gotten working again as a prerequisite for bug 776030.
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8467830 -
Flags: review?(snorp)
Assignee | ||
Comment 5•8 years ago
|
||
This is mostly just pushing code around. Some of the function implementations I updated as well.
Attachment #8464967 -
Attachment is obsolete: true
Attachment #8467832 -
Flags: review?(snorp)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8464968 [details] [diff] [review] Part 2 (WIP) - Some touch event handling These other two patches need some more work so I'll move them to another bug.
Attachment #8464968 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8464969 -
Attachment is obsolete: true
Attachment #8467830 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8467835 -
Flags: review?(snorp)
Comment on attachment 8467832 [details] [diff] [review] Part 2 - General unbitrotting Review of attachment 8467832 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/android/APZCCallbackHandler.cpp @@ +137,5 @@ > +APZCCallbackHandler::SendAsyncScrollDOMEvent(bool aIsRoot, > + const CSSRect& aContentRect, > + const CSSSize& aScrollableSize) > +{ > + // no-op, we don't want to support this event on fennec What is this and why don't we want to support it? @@ +162,5 @@ > + if (i == 0) { > + // if we're inserting it at the head of the queue, notify Java because > + // we need to get a callback at an earlier time than the last scheduled > + // callback > + mNativePanZoomController->PostDelayedCallbackWrapper((int64_t)aDelayMs); Will this cancel/replace the existing callback? @@ +167,5 @@ > + } > +} > + > +int64_t > +APZCCallbackHandler::RunDelayedTasks() It seems like all of this task stuff should be abstracted out into some kind of DelayedTaskQueue type of thing that is separate from APZC. ::: widget/android/AndroidBridge.cpp @@ +45,5 @@ > using namespace mozilla; > using namespace mozilla::widget::android; > using namespace mozilla::gfx; > > +AndroidBridge* AndroidBridge::sBridge; Why do we need to do this?
Attachment #8467832 -
Flags: review?(snorp) → review+
Attachment #8467835 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #8) > > + // no-op, we don't want to support this event on fennec > > What is this and why don't we want to support it? It's basically the same as the scroll event. It gets fired about as often while the user is scrolling around. We want to get rid of it entirely (bug 898075) because it provides no advantage over just listening for the scroll event. We can't just yet because some B2G code uses it. I can update the comment to point to bug 898075. > @@ +162,5 @@ > > + if (i == 0) { > > + // if we're inserting it at the head of the queue, notify Java because > > + // we need to get a callback at an earlier time than the last scheduled > > + // callback > > + mNativePanZoomController->PostDelayedCallbackWrapper((int64_t)aDelayMs); > > Will this cancel/replace the existing callback? No. So if this called twice, with the second task having an earlier run-at time than the first task, there will be two callbacks posted in the java-side code, and they will both run. The first one will probably schedule yet another one (see [1]) but whichever runs last will be a no-op because it will find no tasks waiting to be run. I can add code to cancel the existing callback but it seems like a bunch of complexity for not much benefit. It doesn't even allocate more memory because the same CallbackRunnable instance is used on the java side. [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/NativePanZoomController.java?rev=36853e536d68#100 > @@ +167,5 @@ > > +int64_t > > +APZCCallbackHandler::RunDelayedTasks() > > It seems like all of this task stuff should be abstracted out into some kind > of DelayedTaskQueue type of thing that is separate from APZC. It is separate from APZC, but it requires going back and forth across the JNI barrier into java code. I might be able to abstract it out I guess but it won't really be a general-purpose thing since it's only useful for posting tasks from C++ running on the android UI thread. > ::: widget/android/AndroidBridge.cpp > > +AndroidBridge* AndroidBridge::sBridge; > > Why do we need to do this? Now that AndroidBridge no longer extends from GeckoContentController it no longer implements AddRef() and friends, so we can't keep a RefPtr to it. This restores the code to the state it was in before I landed bug 839641.
Assignee | ||
Comment 10•8 years ago
|
||
I moved the code out into GeckoAppShell. I'll squash this into part 2 on landing, just keeping it separate for easier review.
Attachment #8468000 -
Flags: review?(snorp)
Assignee | ||
Comment 11•8 years ago
|
||
try |
Try push at https://tbpl.mozilla.org/?tree=Try&rev=654e95fa1c09
Assignee | ||
Comment 12•8 years ago
|
||
greentry |
https://tbpl.mozilla.org/?tree=Try&rev=8cbc1ca188fc https://tbpl.mozilla.org/?tree=Try&rev=f6adb8f7c5b1
Comment on attachment 8468000 [details] [diff] [review] Part 4 (will be folded into part 2) - Move the RunDelayedTasks stuff out of NativePanZoomController Review of attachment 8468000 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good
Attachment #8468000 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 14•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ff86c2ca064 https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb6a9651e0a https://hg.mozilla.org/integration/mozilla-inbound/rev/fcc3d8857f00
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9ff86c2ca064 https://hg.mozilla.org/mozilla-central/rev/3cb6a9651e0a https://hg.mozilla.org/mozilla-central/rev/fcc3d8857f00
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•9 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•