Closed Bug 941138 Opened 6 years ago Closed 6 years ago

APZCTreeManager has some almost-duplicated code to calculate mApzcForInputBlock and mCachedTransformToApzcForInputBlock

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: kats, Assigned: kats)

Details

Attachments

(3 files, 3 obsolete files)

Attached patch WIP part 1 (obsolete) — Splinter Review
There's some cleanup we can do here. There might also be a possible bug.
Attached patch WIP part 3 (possibly a bug fix) (obsolete) — Splinter Review
Discovered while writing the above to WIPs
If possible I'd like to make the ReceiveInputData(const InputData&, ...) version also use GetTouchInputBlockAPZC but in order to extract ScreenPoint instances from the InputData we might need to pass in a function pointer or do some template magic or something.
No functional changes intended, just code re-arranging. This results in the nice property that GetTouchInputBlockAPZC doesn't touch mApzcForInputBlock or mCachedTransformToApzcForInputBlock
Assignee: nobody → bugmail.mozilla
Attachment #8335449 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8360670 - Flags: review?(botond)
Again, no functional changes intended.
Attachment #8335450 - Attachment is obsolete: true
Attachment #8360672 - Flags: review?(botond)
There are two codepaths that are identical in their functionality but one operates on a WidgetInputEvent and the other on an InputData. The InputData version was missing code to reset the cached transform.
Attachment #8335454 - Attachment is obsolete: true
Attachment #8360677 - Flags: review?(botond)
Attachment #8360670 - Flags: review?(botond) → review+
Comment on attachment 8360672 [details] [diff] [review]
Part 2 - Remove a redundant argument and null check

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

::: gfx/layers/composite/APZCTreeManager.cpp
@@ -370,5 @@
>      BuildOverscrollHandoffChain(apzc);
>    }
> -  if (!apzc) {
> -    return apzc.forget();
> -  }

Can we add a comment, either at the declaration of CommonAncestor or at the beginning of its body, stating that a null pointer is a valid input for it? It took me a few minutes to convince myself of this.
Attachment #8360672 - Flags: review?(botond) → review+
Attachment #8360677 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> Created attachment 8360677 [details] [diff] [review]
> Part 3 - Reset the cached apz transform correctly
> 
> There are two codepaths that are identical in their functionality but one
> operates on a WidgetInputEvent and the other on an InputData. The InputData
> version was missing code to reset the cached transform.

It seems a little strange to have a helper function (GetTouchInputBlockAPZC) for one of these codepaths but not the other. Can we either have a helper function for both, or neither?
(In reply to Botond Ballo [:botond] from comment #7)
> Can we add a comment, either at the declaration of CommonAncestor or at the
> beginning of its body, stating that a null pointer is a valid input for it?
> It took me a few minutes to convince myself of this.

There is one in the body already but I can move it a few lines up to the beginning of the body.

(In reply to Botond Ballo [:botond] from comment #8)
> It seems a little strange to have a helper function (GetTouchInputBlockAPZC)
> for one of these codepaths but not the other. Can we either have a helper
> function for both, or neither?

Yeah.. it is strange. At this point I'm too lazy to make that change but we'll be killing off one of the two codepaths eventually so maybe we can clean it up then :)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9)
> (In reply to Botond Ballo [:botond] from comment #7)
> > Can we add a comment, either at the declaration of CommonAncestor or at the
> > beginning of its body, stating that a null pointer is a valid input for it?
> > It took me a few minutes to convince myself of this.
> 
> There is one in the body already but I can move it a few lines up to the
> beginning of the body.

Oh. Not sure how I missed that... Maybe I'm unconsciously transferring the ability I've developed for ignoring ads and other non-content on a webpage to ignoring comments in code or something :) Anyways, it's fine the way it is then.
I have some half-baked theories about how reading code is fundamentally different from reading comments and so people tend to ignore comments when they're in "code reading" mode. :)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ff662b5b45f4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cb989ceae01b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/87aeba6685d4
After applying mozilla-inbound: changeset 163955:87aeba6685d4 - http://hg.mozilla.org/integration/mozilla-inbound/rev/87aeba6685d4 Flash is crashing Fx29 on Windows 8.1.  Does this make sense as this patch is specified for Firefox OS?
(In reply to Gary [:streetwolf] from comment #12)
> After applying mozilla-inbound: changeset 163955:87aeba6685d4 -
> http://hg.mozilla.org/integration/mozilla-inbound/rev/87aeba6685d4 Flash is
> crashing Fx29 on Windows 8.1.  Does this make sense as this patch is
> specified for Firefox OS?

The APZC code is used in the firefox metro interface as well, so it's not unreasonable for this code to trigger a crash there. However the specific patch you pointed out (the part 3 on this bug) touches a codepath that shouldn't be used on Firefox metro at all. Do you happen to have a backtrace of the crash?
Let me just clarify that I was using the desktop version of Fx29 not the Modern(Metro) version.  Here's a crash report I submitted:

https://crash-stats.mozilla.com/report/index/f040cb63-48c0-45da-a032-5bfba2140117
If you were not using the Metro interface then this code shouldn't have affected anything. The crash report unfortunately doesn't seem to have symbols so I can't tell what was causing it. Is the crash reproducible for you? And were you able to isolate it to that particular change (i.e. it doesn't crash without the change but does crash with it)?
I'm using the hourly inbounds which I don't think have symbols.  It is 100% reproducible for me.
Go figure.  I installed the latest inbound hourly http://hg.mozilla.org/integration/mozilla-inbound/rev/0496d73bf88e and all is working fine on the desktop.
You need to log in before you can comment on or make changes to this bug.