Closed Bug 1265424 Opened 4 years ago Closed 4 years ago

Fix telemetry measurements for CONTENT_RESPONSE_DURATION

Categories

(Core :: Panning and Zooming, defect)

48 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(2 files)

In bug 1261373 I added a CONTENT_RESPONSE_DURATION to measure how long the main-thread takes to send all the necessary notifications to APZ. There was a bug in the code though, it artificially sends 300ms values for wheel events in some cases. There is a more detailed description of why in bug 1261373 comment 9.
Ugh. I was going through a somewhat circuitous route to fix this since I thought the code would end up cleaner overall, but in the end there was a problem with that approach and the only way to fix it required introducing an extra flag which I didn't really like. So I'm going with a different approach that involved less code churn. The old approach is stashed at [1].

[1] https://github.com/staktrace/gecko-dev/commits/7ebfc5510eb3cbc462e016cbfcf64f3112271729
Attachment #8742457 - Flags: review?(botond) → review+
Comment on attachment 8742457 [details]
MozReview Request: Bug 1265424 - Record if the target was confirmed via a timeout or not. r?botond

https://reviewboard.mozilla.org/r/47205/#review43821

::: gfx/layers/apz/src/InputBlockState.h:47
(Diff revision 1)
>    explicit InputBlockState(const RefPtr<AsyncPanZoomController>& aTargetApzc,
>                             bool aTargetConfirmed);
>    virtual ~InputBlockState()
>    {}
>  
> -  virtual bool SetConfirmedTargetApzc(const RefPtr<AsyncPanZoomController>& aTargetApzc);
> +  virtual bool SetConfirmedTargetApzc(const RefPtr<AsyncPanZoomController>& aTargetApzc, bool aFromTimeout);

Can we just make this parameter have type TargetConfirmationState? I think call sites would read better that way, than with true/false arguments.
Comment on attachment 8742458 [details]
MozReview Request: Bug 1265424 - Ensure that HasReceivedAllContentNotifications doesn't start returning true if the target APZC was confirmed by timeout. r?botond

https://reviewboard.mozilla.org/r/47207/#review43843
Attachment #8742458 - Flags: review?(botond) → review+
(In reply to Botond Ballo [:botond] from comment #4)
> Can we just make this parameter have type TargetConfirmationState? I think
> call sites would read better that way, than with true/false arguments.

Sure, but I'm going to update the implementation to assert that only eConfirmed or eTimedOut can be passed in. The other two are internal states and it doesn't make sense for callers to be passing those in.
https://hg.mozilla.org/mozilla-central/rev/aa93e1e7cc13
https://hg.mozilla.org/mozilla-central/rev/4a137817e1f5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.