[iOS] Make page load asynchronous for fake web state. [chromium/src : main]

0 views

Skip to first unread message

Sylvain Defresne (Gerrit)

unread,

8:16 AM (5 hours ago) 8:16 AM

to Nohemi Fernandez, Mike Dougherty, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-rev...@chromium.org, ios-r...@chromium.org

Attention needed from Mike Dougherty and Nohemi Fernandez

Sylvain Defresne voted and added 2 comments

Votes added by Sylvain Defresne

Code-Review

2 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Sylvain Defresne . resolved

lgtm once the comment has been addressed

Though I think @mich...@chromium.org should also take a look. Can you wait for their +1 too?

File ios/web/public/test/fakes/fake_web_state.mm

Line 260, Patchset 5 (Latest): OnPageLoaded(web::PageLoadCompletionStatus::SUCCESS);

Sylvain Defresne . unresolved

This block capture `this` which is unsafe.

Since `WebState` has a method `GetWeakPtr()`, we should use it here to make this safe in case the test destroy the `FakeWebState` after calling `LoadData(...)` and before the block executes.

So something like this:

```
  base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
      FROM_HERE,
      base::BindOnce(
          &FakeWebState::OnPageLoaded,
          weak_ptr_factory_.GetWeakPtr(),
          web::PageLoadCompletionStatus::SUCCESS));
```

Open in Gerrit

Related details

Attention is currently required from:

Mike Dougherty
Nohemi Fernandez

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

Nohemi Fernandez (Gerrit)

unread,

11:04 AM (2 hours ago) 11:04 AM

to Mike Dougherty, Sylvain Defresne, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, ios-revie...@chromium.org, marq+...@chromium.org, ios-rev...@chromium.org, ios-r...@chromium.org

Attention needed from Mike Dougherty and Sylvain Defresne

Nohemi Fernandez added 1 comment

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Sylvain Defresne . unresolved

lgtm once the comment has been addressed
Though I think @mich...@chromium.org should also take a look. Can you wait for their +1 too?

Nohemi Fernandez

Thanks Sylvain. After additional conversation on the underlying patch, I may be re-writing it in such a way where I would no longer need the asynchronicity in tests. I'm happy to keep the current code if you think there would be general value otherwise I can revert to avoid the additional complexity.

@mich...@chromium.org let me know if you think there would still be value in exposing this asynchronous OnPageLoaded call in FakeWebState more generally. Besides the unittest included in this patch I didn't find any other major changes needed in the code to support it, though now developers would need to include a `TaskEnvironment` in their unittest if they need FakeWebState.