[iOS] Add a maximum delay for loading page load content in Reading Mode. [chromium/src : main]

0 views

Skip to first unread message

Nohemi Fernandez (Gerrit)

unread,

9:47 AM (3 hours ago) 9:47 AM

to Olivier Robin, Chromium LUCI CQ, Chromium Metrics Reviews, AyeAye, chromium...@chromium.org, asvitkine...@chromium.org, ios-rev...@chromium.org, ios-revie...@chromium.org, ios-r...@chromium.org, marq+...@chromium.org

Attention needed from Olivier Robin

Nohemi Fernandez added 4 comments

Patchset-level comments

File-level comment, Patchset 7:

Olivier Robin . resolved

I am not sure what is the purpose of the CL.
Do you expect to still see the same freeze, and so want to collect data if that happens?

Nohemi Fernandez

The purpose of the CL is to ensure that we do not have a scenario where Reading Mode hangs (fails to complete and continues to apply a blur overlay) by introducing a timeout in case the page load in Reading Mode web state does not complete.

I think this is important even if we have fixed one of the issues that may cause this hang (b/454302739) because we do not control the asynchronous operation between web state's `LoadData` and the eventual call to `PageLoaded` and there still may be scenarios that are not covered that we will discover at a later time.

We also are collecting data to understand the latency expectations of `PageLoaded` because this is not something we had done previously.

I've updated the CL description to include this information.

File ios/chrome/browser/reader_mode/model/reader_mode_content_tab_helper.mm

Line 150, Patchset 7: web::PageLoadCompletionStatus load_completion_status) {

Olivier Robin . unresolved

should we check load_completion_status in the method?
Also, if timer timed out, should we avoid calling the delegate?

Nohemi Fernandez

Unfortunately, `PageLoaded` is also called during WebState activation/deactivation which does not follow the same code path as `LoadContent`. For example during testing I would see `PageLoaded` called following an update via `WebStateListDidChange` notification. This is why I've added the comment below to specify that we cannot have the expectation that `load_data_timer_` is running when entering this method.

Line 153, Patchset 7: // `ReaderModeBrowserAgent` will bypass the call to `LoadContent`.

Olivier Robin . resolved

I understand why we need to call stop, but I don't understand this comment.

Nohemi Fernandez

I've added additional details above.

File ios/chrome/browser/reader_mode/model/reader_mode_metrics_helper.mm

Line 167, Patchset 7: data_load_timer_ = std::make_unique<base::ElapsedTimer>();

Olivier Robin . unresolved

so you have 2 timer testing for the same thing? Can you have only one?

Nohemi Fernandez

I don't understand what two timers you are referring to. There is a timer that is checking that the distillation is completed which produces the HTML that we will display to the user and a timer that checks that page load has completed.

Open in Gerrit

Related details

Attention is currently required from:

Olivier Robin

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages