Skip to content
This repository has been archived by the owner on Dec 29, 2020. It is now read-only.

[APTT-27] WKWebView / UIWebView - Send HTTP/S network request status information to backend. #41

Merged
merged 3 commits into from
May 14, 2018

Conversation

vito1188
Copy link
Contributor

@vito1188 vito1188 commented May 9, 2018

No description provided.

@vito1188 vito1188 requested review from SwinX and donnie-jp May 9, 2018 14:20
@codecov-io
Copy link

codecov-io commented May 9, 2018

Codecov Report

Merging #41 into master will decrease coverage by 8.36%.
The diff coverage is 37.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   88.59%   80.23%   -8.37%     
==========================================
  Files          25       25              
  Lines        1280     1285       +5     
==========================================
- Hits         1134     1031     -103     
- Misses        146      254     +108
Impacted Files Coverage Δ
...eTracking/Private/_RPTClassManipulator+UIWebView.m 85.56% <0%> (-2.19%) ⬇️
...eTracking/Private/_RPTClassManipulator+WKWebView.m 76.78% <0%> (-1.4%) ⬇️
RPerformanceTracking/Private/_RPTTracker.m 100% <100%> (ø) ⬆️
...ng/Private/_RPTClassManipulator+NSURLSessionTask.m 100% <100%> (ø) ⬆️
RPerformanceTracking/Private/_RPTLocation.m 3.33% <0%> (-93.34%) ⬇️
RPerformanceTracking/Private/_RPTEventWriter.m 61.11% <0%> (-26.39%) ⬇️
...erformanceTracking/Private/_RPTMainThreadWatcher.m 78.26% <0%> (-21.74%) ⬇️
RPerformanceTracking/Private/_RPTTrackingManager.m 77.14% <0%> (-20%) ⬇️
RPerformanceTracking/Private/_RPTConfiguration.m 90.69% <0%> (-2.33%) ⬇️
RPerformanceTracking/Private/_RPTNSURLProtocol.m 80.48% <0%> (+4.87%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7685bc8...54dd085. Read the comment docs.

if (statusCode != 0)
{
measurement.statusCode = statusCode;
}
Copy link
Contributor

@donnie-jp donnie-jp May 10, 2018

Choose a reason for hiding this comment

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

As discussed, instead of this workaround I think it would be better to replace this method with an update:statusCode method that is called from the decidePolicyForNavigationResponse swizzle when there is a valid trackingId associated object.

@vito1188 vito1188 changed the title [APTT-27] Avoid resetting the status code of the measurement to 0. [APTT-27] WKWebView / UIWebView - Send HTTP/S network request status information to backend. May 10, 2018
@@ -148,7 +148,7 @@ + (void)_swizzleUIWebViewDelegate:(id<UIWebViewDelegate>)delegate
uint_fast64_t trackingIdentifier = [objc_getAssociatedObject(webView, _RPT_UIWEBVIEW_TRACKINGIDENTIFIER) unsignedLongLongValue];
if (trackingIdentifier)
{
[_RPTTrackingManager.sharedInstance.tracker end:trackingIdentifier statusCode:httpResponse.statusCode];
[_RPTTrackingManager.sharedInstance.tracker updateStatusCode:httpResponse.statusCode trackingIdentifier:trackingIdentifier];
Copy link
Contributor

Choose a reason for hiding this comment

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

We only update the statusCode for didFinishLoad.
I think we should also do it for didFailLoad if we can get access to the httpurlresponse.
Then that common logic can be extracted into a helper method.

.travis.yml Outdated
@@ -4,6 +4,8 @@ xcode_scheme: Tests
osx_image: xcode9.1

before_install:
- gem uninstall --force -x -i /Users/travis/.rvm/gems/ruby-2.4.2@global xcodeproj
- gem install -q -N -f --no-update-sources xcodeproj:1.5.7
- gem update fastlane --no-ri --no-rdoc --no-document
Copy link
Contributor

@donnie-jp donnie-jp May 11, 2018

Choose a reason for hiding this comment

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

For a workaround like this please add a FIXME comment that references the responsible ticket CocoaPods/Xcodeproj#572 so that we remember to remove the workaround.

Copy link
Contributor

@donnie-jp donnie-jp left a comment

Choose a reason for hiding this comment

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

Inline comments

.travis.yml Outdated
@@ -4,6 +4,8 @@ xcode_scheme: Tests
osx_image: xcode9.1

before_install:
- gem uninstall --force -x -i /Users/travis/.rvm/gems/ruby-2.4.2@global xcodeproj
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious: why do we provide a full to the gem? Wouldn't be enough to just say gem uninstall xcodeproj cause it seems that we're using default ruby version everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, without the full path it failed and the travis log suggested using that path. On jenkins I didn't specify the full path.

@@ -148,7 +148,7 @@ + (void)_swizzleUIWebViewDelegate:(id<UIWebViewDelegate>)delegate
uint_fast64_t trackingIdentifier = [objc_getAssociatedObject(webView, _RPT_UIWEBVIEW_TRACKINGIDENTIFIER) unsignedLongLongValue];
if (trackingIdentifier)
{
[_RPTTrackingManager.sharedInstance.tracker end:trackingIdentifier statusCode:httpResponse.statusCode];
[_RPTTrackingManager.sharedInstance.tracker updateStatusCode:httpResponse.statusCode trackingIdentifier:trackingIdentifier];
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a helper for updating status code+static void updateStatusCodeForWebView(NSInteger statusCode, WKWebView *webView) a bit below. Why not to use it here?

@vito1188 vito1188 merged commit 174bd34 into rakutentech:master May 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants