Skip to content

[webview_flutter_platform_interface] Adds WebResourceRequest to HttpResponseError #4025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

[webview_flutter_platform_interface] Adds WebResourceRequest to HttpResponseError #4025

wants to merge 9 commits into from

Conversation

HugoOlthof
Copy link
Contributor

@HugoOlthof HugoOlthof commented May 17, 2023 •
edited
Loading

As requested in #3695 this PR adds the WebResourceRequest object to HttpResponseError so it's possible for the client to know which request failed.

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@HugoOlthof HugoOlthof requested a review from bparrishMines as a code owner May 17, 2023 08:33
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@HugoOlthof HugoOlthof mentioned this pull request May 17, 2023
Closed
11 tasks
@ghost ghost force-pushed the web-resource-request branch from 56f3997 to 6c4be6a Compare May 25, 2023 14:59
bparrishMines
bparrishMines approved these changes Jun 2, 2023
View reviewed changes
Copy link
Contributor

@bparrishMines bparrishMines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a couple of nits

cc @stuartmorgan

@stuartmorgan-g stuartmorgan-g self-requested a review June 5, 2023 19:10
@stuartmorgan-g
Copy link
Contributor

@HugoOlthof Are you still planning on updating this based on the feedback above?

@HugoOlthof
Copy link
Contributor Author

@stuartmorgan I am, I was on vacation for a few weeks :) I will update the PR as soon as possible.

@ghost ghost force-pushed the web-resource-request branch 2 times, most recently from 1d50f39 to b3b50f1 Compare July 16, 2023 10:07
@stuartmorgan-g
Copy link
Contributor

@HugoOlthof Are you planning on addressing the remaining review comments here?

@ghost ghost force-pushed the web-resource-request branch from b3b50f1 to 8fa0424 Compare August 14, 2023 15:13
@stuartmorgan-g
Copy link
Contributor

@HugoOlthof I see there are commits since my last comment; was this ready for re-review?

@HugoOlthof
Copy link
Contributor Author

@stuartmorgan Yes it is :)

stuartmorgan-g
stuartmorgan-g approved these changes Sep 7, 2023
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@stuartmorgan-g
Copy link
Contributor

The stuck ci.yaml validation was a known infra issue; it should automatically resolve the next time you merge in main.

@Hixie
Copy link
Contributor

Hixie commented Sep 12, 2023

@HugoOlthof Thanks for the contribution! Will you be able to do the merge to get this landed?

@ghost ghost force-pushed the web-resource-request branch from 8fa0424 to b7e23a8 Compare September 13, 2023 10:20
@stuartmorgan-g
Copy link
Contributor

@bparrishMines Ping on this second review

@Hixie
Copy link
Contributor

Hixie commented Oct 24, 2023

What is the next step here?

@Hixie Hixie mentioned this pull request Oct 24, 2023
Closed
11 tasks
stuartmorgan-g
stuartmorgan-g requested changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In all of the discussion I lost track of the fact that this is missing tests.

The new data types should have trivial tests that they work as expected, and we should have a test that would have caught the missing export (e.g., something that imports just types.dart and then tries to construct a HttpResponseError with all of its fields.

.../webview_flutter/webview_flutter_platform_interface/lib/src/types/web_resource_response.dart
});

/// The URI that this response is associated with.
final String? uri;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I know I said we should go back to String? above, but now that I'm looking at it again in the context of not asking it to be changed to a request object: shouldn't it be Uri??

...s/webview_flutter/webview_flutter_platform_interface/lib/src/types/web_resource_request.dart
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

types.dart needs to export both of these new files, unless I'm missing something, since it exports http_response_error.dart and these are now part of that class's public interface.

@bparrishMines bparrishMines mentioned this pull request Jan 3, 2024
Merged
11 tasks
auto-submit bot pushed a commit that referenced this pull request Jan 22, 2024
@bparrishMines
[webview_platform_interface] Adds WebResourceRequest to HttpResponseE…
a94ed29 
…rror (#5790)

This is a copy of #4025 with updates from PR comments. The original didn't allow maintainers to update the code, so I create this branch.
@stuartmorgan-g
Copy link
Contributor

Obsoleted by #5790

arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@bparrishMines @arc-yong
[webview_platform_interface] Adds WebResourceRequest to HttpResponseE…
928b3d1 
…rror (flutter#5790)

This is a copy of flutter#4025 with updates from PR comments. The original didn't allow maintainers to update the code, so I create this branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g requested changes

@bparrishMines bparrishMines Awaiting requested review from bparrishMines bparrishMines is a code owner

Assignees
No one assigned
Labels
needs tests p: webview_flutter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HugoOlthof @stuartmorgan-g @Hixie @bparrishMines