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.

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

@alloy => Migrate to WKWebView #606

Merged
merged 18 commits into from Sep 18, 2015

Conversation

Projects
None yet
3 participants
@orta
Member

orta commented Jul 10, 2015

This still isn't 100% but it's getting there, and greens.

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Sep 1, 2015

Member

OK, this is feeling like it can only really get done with some pair programming, @jorystiefel want to do some with me tomorrow?

Member

orta commented Sep 1, 2015

OK, this is feeling like it can only really get done with some pair programming, @jorystiefel want to do some with me tomorrow?

@jorystiefel

This comment has been minimized.

Show comment
Hide comment
@jorystiefel

jorystiefel Sep 1, 2015

Contributor

@orta thought you'd never ask

Contributor

jorystiefel commented Sep 1, 2015

@orta thought you'd never ask

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Sep 9, 2015

Member

one more test, and then some on-device testing needing.

Member

orta commented Sep 9, 2015

one more test, and then some on-device testing needing.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 10, 2015

Member

Groovy!

Member

alloy commented Sep 10, 2015

Groovy!

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Sep 10, 2015

Member

This should probably green now. I will do some on-device testing.

Member

orta commented Sep 10, 2015

This should probably green now. I will do some on-device testing.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 10, 2015

Member

And more rebasing

Member

alloy commented Sep 10, 2015

And more rebasing

@orta orta changed the title from [WIP] @alloy => Migrate to WKWebView to @alloy => Migrate to WKWebView Sep 15, 2015

@@ -4,30 +4,17 @@
#import "ARInternalShareValidator.h"
#import "ARAppDelegate.h"
@import TPDWeakProxy;

This comment has been minimized.

@alloy

alloy Sep 16, 2015

Member

Also remove this pod from the Podfile.

@alloy

alloy Sep 16, 2015

Member

Also remove this pod from the Podfile.

This comment has been minimized.

@orta

orta Sep 16, 2015

Member

🎉

@orta

orta Sep 16, 2015

Member

🎉

@@ -52,7 +39,7 @@ - (instancetype)initWithURL:(NSURL *)url
}
if (![urlString isEqualToString:url.absoluteString]) {
ARActionLog(@"Rewriting %@ as %@", urlString, url.absoluteString);
NSLog(@"Rewriting %@ as %@", urlString, url.absoluteString);

This comment has been minimized.

@alloy

alloy Sep 16, 2015

Member

ARActionLog isn’t working right anymore, is it?

@alloy

alloy Sep 16, 2015

Member

ARActionLog isn’t working right anymore, is it?

@@ -57,18 +57,18 @@ - (void)markRemoteNotificationsAsRead;
#pragma mark - Overrides
- (instancetype)initWithURL:(NSURL *)url;
- (instancetype)initWithURL:(NSURL *)URL;

This comment has been minimized.

@alloy

alloy Sep 16, 2015

Member

YES! I hate those lowercase url variables :D

@alloy

alloy Sep 16, 2015

Member

YES! I hate those lowercase url variables :D

@interface ARInternalMobileWebViewController (Testing)
@property (nonatomic, strong) ARInternalShareValidator *shareValidator;
- (NSURLRequest *)requestWithURL:(NSURL *)url;

This comment has been minimized.

@alloy

alloy Sep 16, 2015

Member

NoooooooOOOOooOoo, damned lowercase url variables come back to haunt me once again 😱

@alloy

alloy Sep 16, 2015

Member

NoooooooOOOOooOoo, damned lowercase url variables come back to haunt me once again 😱

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 16, 2015

Member

Maybe I missed it, but it appears that you are not stopping webview loading when the view disappears, which is what TSMiniBrowser used to do. With many of our web pages being resource heavy, it kinda makes sense to save the user’s bandwidth once they navigate away to another page.

This is also what the loaded ivar was for, so that the webview could be reloaded if it never finished loading the first time around and the user navigates back to the webview.

Member

alloy commented Sep 16, 2015

Maybe I missed it, but it appears that you are not stopping webview loading when the view disappears, which is what TSMiniBrowser used to do. With many of our web pages being resource heavy, it kinda makes sense to save the user’s bandwidth once they navigate away to another page.

This is also what the loaded ivar was for, so that the webview could be reloaded if it never finished loading the first time around and the user navigates back to the webview.

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 16, 2015

Member

Intens change, I love it, finally gone with the ugly chrome and way better progress hooks! 👏

Member

alloy commented Sep 16, 2015

Intens change, I love it, finally gone with the ugly chrome and way better progress hooks! 👏

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Sep 16, 2015

Member

TODO

  • url - URL
  • self.view.window
  • status codes
  • navigationAction.navigationType == WKNavigationTypeOther
  • self.webView evaluateJavaScript
Member

orta commented Sep 16, 2015

TODO

  • url - URL
  • self.view.window
  • status codes
  • navigationAction.navigationType == WKNavigationTypeOther
  • self.webView evaluateJavaScript
@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Sep 16, 2015

Member

I've updated the PR - I've also removed the web view admin stuff. If I really care ,I'll go in and add something like https://github.com/Naituw/WBWebViewConsole

Member

orta commented Sep 16, 2015

I've updated the PR - I've also removed the web view admin stuff. If I really care ,I'll go in and add something like https://github.com/Naituw/WBWebViewConsole

{
[super webViewDidFinishLoad:webView];
[super webView:webView decidePolicyForNavigationAction:navigationAction decisionHandler:decisionHandler];
self.initialRequest = self.initialRequest ?: navigationAction.request;

This comment has been minimized.

@alloy

alloy Sep 17, 2015

Member

The initialRequest needs to be set to nil when the webview is reloaded, just like lastRequestLoadedAt is set to nil from loadURL:. Otherwise there’s the chance that a second 500 would be considered ok and it won’t get reloaded.

@alloy

alloy Sep 17, 2015

Member

The initialRequest needs to be set to nil when the webview is reloaded, just like lastRequestLoadedAt is set to nil from loadURL:. Otherwise there’s the chance that a second 500 would be considered ok and it won’t get reloaded.

This comment has been minimized.

@orta

orta Sep 17, 2015

Member

Legit 👍

@orta

orta Sep 17, 2015

Member

Legit 👍

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Sep 17, 2015

Member

force pushed changes requested this morning

Member

orta commented Sep 17, 2015

force pushed changes requested this morning

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 17, 2015

Member

@orta There’s still a bunch of CI errors about shouldLoadNavigationAction:, what gives?

Member

alloy commented Sep 17, 2015

@orta There’s still a bunch of CI errors about shouldLoadNavigationAction:, what gives?

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Sep 17, 2015

Member

laziness, didn't run tests after my changes, will fix after swift at artsy

Member

orta commented Sep 17, 2015

laziness, didn't run tests after my changes, will fix after swift at artsy

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 17, 2015

Member

Groovy, baby!

Member

alloy commented Sep 17, 2015

Groovy, baby!

@orta

This comment has been minimized.

Show comment
Hide comment
@orta

orta Sep 17, 2015

Member

alright, all good again

Member

orta commented Sep 17, 2015

alright, all good again

@alloy

This comment has been minimized.

Show comment
Hide comment
@alloy

alloy Sep 18, 2015

Member

Sweet! 👯

Member

alloy commented Sep 18, 2015

Sweet! 👯

alloy added a commit that referenced this pull request Sep 18, 2015

@alloy alloy merged commit 921bbb9 into master Sep 18, 2015

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@alloy alloy deleted the orta-wkwebview branch Sep 18, 2015

@alloy alloy referenced this pull request Sep 28, 2015

Closed

Update to WKWebView #288

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment