Refactor parts of NSURLConnection #2464

ms-jihua · 2017-04-07T19:52:41Z

- Implement [NSURLConnection sendAsynchronousRequest:]
    - Re-implement [NSURLConnection sendSynchronousRequest:] on top of it, as stated in docs
- Implement a not-yet fully-working setDelegateQueue: (callbacks are not yet called on it)
- Remove NSURLConnectionState, replace with a lighter-weight, more flexible inner class
- Changed the NSURLConnection functional tests to no longer run on the main run loop.
  This no longer works, which is correct (doesn't work on reference platform either)
    - Instead, run on another thread's run loop, and an operation queue.
    - Also added tests for sendSynchronousRequest:
- Change parts of NSURLProtocol, NSRunLoop to use stricter typing
    - Change instances of NSRunLoopMode to actually say NSRunLoopMode instead of NSString*

Fixes #2298

- Implement [NSURLConnection sendAsynchronousRequest:] - Re-implement [NSURLConnection sendSynchronousRequest:] on top of it, as stated in docs - Implement a not-yet fully-working setDelegateQueue: (callbacks are not yet called on it) - Remove NSURLConnectionState, replace with a lighter-weight, more flexible inner class - Changed the NSURLConnection functional tests to no longer run on the main run loop. This no longer works, which is correct (doesn't work on reference platform either) - Instead, run on another thread's run loop, and an operation queue. - Also added tests for sendSynchronousRequest: - Change parts of NSURLProtocol, NSRunLoop to use stricter typing - Change instances of NSRunLoopMode to actually say NSRunLoopMode instead of NSString* Fixes microsoft#2298

ms-jihua · 2017-04-07T19:54:55Z

tests/functionaltests/Tests/NSURLConnection.mm

+@end
+
+// Test to verify a request can be successfully made and a valid response and data is received.
+static void _testRequestWithURL(NSURLConnectionTestHelper* connectionTestHelper) {


I considered doing parameterized tests, but since TAEF only parameterizes on Wex Strings, it felt like I was trying too hard to make it fit, so I opted for this instead.

msft-Jeyaram · 2017-04-07T20:07:19Z

Frameworks/Foundation/NSURLConnection.mm

+    if (ret) {
+        ret->_operationQueue = queue;
+        ret->_completionHandler = handler;
+    }


nit: should really be in an initializer then called by the +method

msft-Jeyaram · 2017-04-07T20:08:04Z

Frameworks/Foundation/NSURLConnection.mm


-    [connection cancel];
+    std::vector<std::pair<StrongId<NSRunLoop>, StrongId<NSString>>> _scheduledRunLoops; // NSRunLoop, NSRunLoopMode
+    // TODO #: Cannot currently schedule on more than one runloop


#: [](start = 12, length = 2)

Task ID

i know, i knowww - i dont like filing these tasks until the PR is actually out because they often lack context without the PR,

msft-Jeyaram · 2017-04-07T20:08:44Z

Frameworks/Foundation/NSURLConnection.mm

+ (NSData*)sendSynchronousRequest:(NSURLRequest*)request
+                returningResponse:(NSURLResponse* _Nullable*)response
+                            error:(NSError* _Nullable*)error {
+    if (request) {


if (request) { [](start = 3, length = 15)

nit: early return :p

I disagree 😛

msft-Jeyaram · 2017-04-07T20:10:55Z

Frameworks/Foundation/NSURLConnection.mm

-    [_protocol release];
-    [_delegate release];
-    [_response release];
+    [_originalRequest release];


[_originalRequest release]; [](start = 1, length = 30)

good candidate for StrongId

it's a property, which makes it a less good candidate for StrongId

DHowett-MSFT · 2017-04-07T20:01:53Z

Frameworks/Foundation/NSURLConnection.mm

-+ (NSData*)sendSynchronousRequest:(id)request returningResponse:(NSURLResponse**)responsep error:(NSError**)errorp {
-    NSURLConnectionState* state = [[[NSURLConnectionState alloc] init] autorelease];
-    NSURLConnection* connection = [[self alloc] initWithRequest:request delegate:state startImmediately:FALSE];
+ (_NSURLConnection_DataDelegate*)delegateWithQueue:(NSOperationQueue*)queue handler:(void (^)(NSURLResponse*, NSData*, NSError*))handler {


nit: prefer all factory functions X to go through an appropriate initializer initWithX:

DHowett-MSFT · 2017-04-07T20:02:54Z

Frameworks/Foundation/NSURLConnection.mm

-    [connection scheduleInRunLoop:[NSRunLoop currentRunLoop] forMode:mode];
-    [connection start];
+- (void)connectionDidFinishLoading:(NSURLConnection*)connection {
+    [_operationQueue addOperation:[NSBlockOperation blockOperationWithBlock:^void() {


Do we know whether the queue is fire-and-forget or fire-and-wait?
I assumed NSNotificationCenter+OperationQueue was fire-and-forget, but it actually waits for the notification block to complete.

it's fire-and-forget

DHowett-MSFT · 2017-04-07T20:03:24Z

Frameworks/Foundation/NSURLConnection.mm

+// Actual NSURLConnection implementation
+@implementation NSURLConnection {
+@private
+    StrongId<NSObject> _delegate;


This delegate should be protocol-qualified; StrongId<NSObject<NSURLConnectionDelegate>>

DHowett-MSFT · 2017-04-07T20:04:15Z

Frameworks/Foundation/NSURLConnection.mm

+ @Status Interoperable
+*/
+ (BOOL)canHandleRequest:(id)request {
+    return ([NSURLProtocol _URLProtocolClassForRequest:request] != nil) ? YES : NO;


This also requires a -[protocolClass canInitWithRequest:]

DHowett-MSFT · 2017-04-07T20:05:34Z

Frameworks/Foundation/NSURLConnection.mm

+                            *error = innerError ? [[innerError retain] autorelease] : nil;
+                        }
+
+                        data = innerData ? [[innerData retain] autorelease] : nil;


It would be safer to only retain data in here and only autorelease it outside the block. Otherwise, this +1/-1 pair could resolve when the Operation Queue loops around again and drains its pool.

good point.

DHowett-MSFT · 2017-04-07T20:11:30Z

Frameworks/Foundation/NSURLConnection.mm

    _storagePolicy = policy;

    if ([_delegate respondsToSelector:@selector(connection:willCacheResponse:)]) {
-        [_delegate connection:self willCacheResponse:response];
+        [_delegate connection:self willCacheResponse:static_cast<NSCachedURLResponse*>(response)];


whoa, this is scary. I realize it is what was already happening, but those two things are very much not the same thing

yuuuppp, lemme leave a TODO here to fix up later.

DHowett-MSFT · 2017-04-07T20:12:14Z

Frameworks/Foundation/NSURLConnection.mm


+    NSURLProtocol* protocol = _protocolForRequest(request, self);
+    if (!protocol) {
+        TraceError(TAG, L"[NSURLConnection start] could not create a NSURLProtocol for its NSURLRequest.");


not start anymore!

DHowett-MSFT · 2017-04-07T20:12:46Z

Frameworks/Foundation/NSURLConnection.mm

+        TraceError(TAG, L"[NSURLConnection start] could not create a NSURLProtocol for its NSURLRequest.");
+        return;
+    }
+    _protocol = protocol;


do we need to update _request like setRequest was doing?

DHowett-MSFT · 2017-04-07T20:13:40Z

Frameworks/Foundation/NSURLProtocol.mm

-        _request.attach([request mutableCopy]);
-        _cachedResponse = cachedResponse;
-        _client = client;
+        _request = [request mutableCopy];


hmm.. why move away from StrongId?

they were properties, and one was working incorrectly and not taking a copy. seemed like a good idea to just pivot to autosynthesis

DHowett-MSFT · 2017-04-07T20:14:12Z

Frameworks/Foundation/NSURLProtocol_WinHTTP.mm

@@ -251,7 +251,7 @@ - (void)startLoading {

        NSError* error = nil;
        try {
-            auto httpRequestMessage = _requestMessageForNS(_request);
+            auto httpRequestMessage = _requestMessageForNS(self.request);


these getter calls seem unnecessary; NSURLProtocol for HTTP is a leaf class and cannot be subclassed.

NSURLProtocol_WinHTTP doesn't have access to the superclass member now that it's synthesized

If that's the case, we should at least lift these out to locals then. Otherwise, each hit is a [[_request retain] autorelease]

(the compiler can't perform common subexpression elimination as it can't prove that self.request is idempotent)

aballway · 2017-04-07T19:59:39Z

Frameworks/Foundation/NSURLConnection.mm

+- (void)connection:(NSURLConnection*)connection didReceiveResponse:(NSURLResponse*)response {
+    // We get some of these for each redirect. Only capture the final one.
+    _response = response;
+    if (!_data.get() || [_data length] > 0) {


nit: just use !_data for the operator bool()

didnt know that worked, til

aballway · 2017-04-07T20:00:28Z

Frameworks/Foundation/NSURLConnection.mm


-    [connection cancel];
+    std::vector<std::pair<StrongId<NSRunLoop>, StrongId<NSString>>> _scheduledRunLoops; // NSRunLoop, NSRunLoopMode


nit: use NSRunLoopMode

can't, NSRunLoopMode is unfortunately typedef'd as NSString* and not NSString :(

huh.. AutoId should be able to remove_pointer that properly. I wanted for AutoId<SNString> and AutoId<NSString*> to be functionally equivalent, so it strips or adds the pointer as necessary.

oh. i didn't actually try it i just figured it wouldn't work. ;;;;

it works. i'm silly.

aballway · 2017-04-07T20:03:38Z

Frameworks/Foundation/NSURLConnection.mm

+                                queue:queue
+                    completionHandler:^void(NSURLResponse* innerResponse, NSData* innerData, NSError* innerError) {
+                        if (response) {
+                            *response = innerResponse ? [[innerResponse retain] autorelease] : nil;


How does autorelease work within an async block? If that block is executed within an autoreleasepool, will these be released then?

pretty much; oops

aballway · 2017-04-07T20:06:14Z

Frameworks/Foundation/NSURLConnection.mm

-    [_protocol startLoading];
+/**
+ @Status Caveat
+ @Notes  TODO #: Cannot currently schedule on more than one runloop


nit: add issue number here

aballway · 2017-04-07T20:06:44Z

Frameworks/Foundation/NSURLConnection.mm

+                        format:@"A connection cannot be scheduled with both an operation queue and a run loop."];
+        }
+
+        if (_scheduledRunLoops.size() >= 1) {


should this just be > 1?

? no, as is, it only allows something in if the vector is empty, which is what i want.

aballway · 2017-04-07T20:07:33Z

Frameworks/Foundation/NSURLConnection.mm

+
+/**
+ @Status Caveat
+ @Notes  TODO #: Callbacks do not currently occur on delegate queue


nit: same as above

aballway · 2017-04-07T20:10:31Z

Frameworks/Foundation/NSURLConnection.mm

-    _scheduled = YES;
+- (void)unscheduleFromRunLoop:(NSRunLoop*)aRunLoop forMode:(NSRunLoopMode)mode {
+    @synchronized(self) {
+        for (auto it = _scheduledRunLoops.begin(); it != _scheduledRunLoops.end(); ++it) {


nit: shouldn't modify a collection while iterating over it, even if it's breaking out of it at the end. Would prefer std::find() with an erase if != end()

aballway · 2017-04-07T20:13:26Z

Frameworks/Foundation/NSURLProtocol.mm

        NSInteger count = [classes count];

        while (--count >= 0) {
-            id check = [classes objectAtIndex:count];
+            Class check = [classes objectAtIndex:count];


nit: update this to be for in loop

@aballway This is iterating in reverse. Cheaper than a reverseObjectEnumerator.

⭕️ that they are

aballway · 2017-04-07T20:14:04Z

Frameworks/Foundation/NSURLProtocol.mm

-        _request.attach([request mutableCopy]);
-        _cachedResponse = cachedResponse;
-        _client = client;
+        _request = [request mutableCopy];


won't these overretain?

_foo = doesn't call the setter, so everything is +1 as it should be

aballway · 2017-04-07T20:15:32Z

Frameworks/Foundation/NSURLConnection.mm

+ (NSData*)sendSynchronousRequest:(NSURLRequest*)request
+                returningResponse:(NSURLResponse* _Nullable*)response
+                            error:(NSError* _Nullable*)error {
+    if (request) {


I disagree 😛

msft-Jeyaram · 2017-04-07T20:28:46Z

// Copyright (c) 2016 Microsoft Corporation. All rights reserved.

nit: remove

Refers to: Frameworks/include/NSURLProtocolInternal.h:3 in 120e3ec. [](commit_id = 120e3ec, deletion_comment = False)

msft-Jeyaram · 2017-04-07T20:29:21Z

tests/functionaltests/Tests/NSURLConnection.mm

 - (instancetype)init {
    if (self = [super init]) {
-        self->_condition = [[NSCondition alloc] init];


[NSCondition alloc] init]; [](start = 28, length = 26)

:)

msft-Jeyaram · 2017-04-07T20:31:18Z

tests/functionaltests/Tests/NSURLConnection.mm

+
+- (instancetype)init {
+    if (self = [super init]) {
+        self->_thread = [[NSThread alloc] initWithTarget:self selector:@selector(spinRunLoop) object:nil];


self->_thread = [[NSThread alloc] initWithTarget:self selector:@selector(spinRunLoop) object:nil]; [](start = 8, length = 98)

+1 on the init and +1 on the retain?

this is an arc-enabled file

rajsesh · 2017-04-07T20:32:45Z

include/Foundation/NSURLConnection.h

 + (BOOL)canHandleRequest:(NSURLRequest*)request;
 @property (readonly, copy) NSURLRequest* originalRequest;
-@property (readonly, copy) NSURLRequest* currentRequest;
+@property (readonly, copy) NSURLRequest* currentRequest STUB_PROPERTY;


STUB_PROPERTY [](start = 56, length = 13)

while we are here, can we implement this? this should be easy...

msft-Jeyaram

🚶

DHowett-MSFT · 2017-04-10T19:09:41Z

Frameworks/Foundation/NSURLConnection.mm

@@ -54,27 +55,32 @@ @implementation _NSURLConnection_DataDelegate {

    StrongId<NSURLResponse> _response;
    StrongId<NSMutableData> _data;
+
+    bool _started;


Wait, this doesn't even seem like it gets used in the body of this data delegate... but _started gets used in NSURLConnection. Did some wires get crossed?

DHowett-MSFT · 2017-04-10T19:10:25Z

Frameworks/Foundation/NSURLConnection.mm

+        return NO;
+    }
+
+    return [protocolClass canInitWithRequest:request];


you can keep this folded, actually. [nil canInitWithRequest:request] returns NO

(no need for the if check that the compiler is going to emit anyway)

DHowett-MSFT · 2017-04-10T19:12:35Z

Frameworks/Foundation/NSURLConnection.mm

-                _scheduledRunLoops.erase(it);
-                return;
-            }
+        auto it = std::find(_scheduledRunLoops.begin(), _scheduledRunLoops.end());


this appears to be finding "the run loop"? It doesn't care about its arguments. If that's the case, why not just _scheduledRunLoops.clear()?

ugh this is what i get for rushing on friday...

DHowett-MSFT · 2017-04-10T19:14:08Z

tests/unittests/Foundation/NSURLTests.m

+    NSURLConnection* connection = [self _createConnectionWithRequest:URLString];
+    [connection setDelegateQueue:_operationQueue];
+    // [connection setDelegateQueue:[NSOperationQueue new]];


commented code

oops, ty for catching

DHowett-MSFT · 2017-04-10T19:14:31Z

tests/unittests/Foundation/NSURLTests.m

nit: prefer not cluttering the URL tests with URL Download tests

oops, I was testing something on the reference platform and apparently forgot to delete this?

DHowett-MSFT · 2017-04-10T19:14:45Z

tests/unittests/Foundation/NSURLTests.m

+        [connectionTestHelper.condition waitUntilDate:[NSDate dateWithTimeIntervalSinceNow:3]];
+    }
+    ASSERT_EQ_MSG(2, [connectionTestHelper.delegateCallOrder count], "FAILED: We should have received two delegates call by now!");


nit: delegate calls

DHowett-MSFT · 2017-04-10T19:14:53Z

tests/unittests/Foundation/NSURLTests.m

+
+    // Make sure there was no error.
+    ASSERT_TRUE_MSG((connectionTestHelper.error == nil), "FAILED: Connection returned error %@!", connectionTestHelper.error);


nit: assert_eq?

DHowett-MSFT · 2017-04-10T19:16:04Z

tests/unittests/Foundation/NSURLTests.m

+
+// Test to verify a request can be successfully made but no data was received and a valid response error code was received.
+static void _testRequestWithURL_Failure(NSURLConnectionTestHelper* connectionTestHelper) {


this appears unused

DHowett-MSFT · 2017-04-10T19:20:19Z

Frameworks/Foundation/NSURLConnection.mm

+    @synchronized(self) {
+        auto it = std::find(_scheduledRunLoops.begin(), _scheduledRunLoops.end());
+        if (it != _scheduledRunLoops.end()) {
+            _scheduledRunLoops.erase(it);


also, shouldn't this call something on the runloop to unschedule ourselves?

Nope. https://developer.apple.com/reference/foundation/nsurlconnection/1409722-unschedulefromrunloop?language=objc

"Use this method to unschedule the connection from an undesired run loop and mode before starting the connection.
You cannot reschedule a connection after it has started."

DHowett-MSFT · 2017-04-10T20:43:13Z

Frameworks/Foundation/NSURLConnection.mm

@@ -110,18 +109,15 @@ @implementation NSURLConnection {
    StrongId<NSURLResponse> _response;
    NSURLCacheStoragePolicy _storagePolicy;
    StrongId<NSURLProtocol> _protocol;
+
+    bool _started;


(is this superior to property autosynthesis? it looks like there's already a property for this ivar, and the ivar will be automatically generated just like it was before this change)

? There's no property for this ivar.

DHowett-MSFT · 2017-04-10T20:43:30Z

Frameworks/Foundation/NSURLConnection.mm

-        auto it = std::find(_scheduledRunLoops.begin(), _scheduledRunLoops.end());
+        auto it = std::find_if(_scheduledRunLoops.begin(),
+                               _scheduledRunLoops.end(),
+                               [aRunLoop](std::pair<StrongId<NSRunLoop>, StrongId<NSRunLoopMode>>& pair) {


should this ref be const?

DHowett-MSFT · 2017-04-10T20:43:48Z

Frameworks/Foundation/NSURLConnection.mm

+        auto it = std::find_if(_scheduledRunLoops.begin(),
+                               _scheduledRunLoops.end(),
+                               [aRunLoop](std::pair<StrongId<NSRunLoop>, StrongId<NSRunLoopMode>>& pair) {
+                                   return aRunLoop == pair.first.get();


nit: AutoId should have an operator== for T (but I wouldn't be surprised if it didn't)

ms-jihua · 2017-04-11T22:32:13Z

ping!

ms-jihua · 2017-04-12T18:03:20Z

ping! last call @msft-Jeyaram @aballway @rajsesh-msft

aballway

Whoops thought I approved this already 🇱🇦

msft-Jeyaram · 2017-04-12T18:17:28Z

Frameworks/Foundation/NSURLConnection.mm

-@implementation NSURLConnection
+// Private helper function that returns a NSURLProtocol object capable of handling a NSURLRequest
+// Returns nil if one cannot be created
+static NSURLProtocol* _protocolForRequest(NSURLRequest* request, id<NSURLProtocolClient> client) {


_protocolForRequest [](start = 22, length = 19)

nit: __ for static function used within the file

Hmm, originally i only proposed this convention for within CoreText/Graphics, given that we had a high incidence of both file-private and framework-private functions. Did we begin to use this more broadly?

msft-Jeyaram · 2017-04-12T18:31:06Z

Frameworks/Foundation/NSURLConnection.mm

+                    }
+
+                    if (error) {
+                        *error = innerError ? [innerError retain] : nil;


innerError ? [innerError retain] : nil; [](start = 33, length = 39)

does this matter, if the innerError is nil, retain would return nil?

mm, i guess that's probably fine, yeah

msft-Jeyaram · 2017-04-12T18:35:43Z

// Copyright (c) 2016 Microsoft Corporation. All rights reserved.

still needs update

In reply to: 292643560 [](ancestors = 292643560)

Refers to: Frameworks/include/NSURLProtocolInternal.h:3 in 120e3ec. [](commit_id = 120e3ec, deletion_comment = False)

msft-Jeyaram · 2017-04-12T18:38:07Z

tests/functionaltests/Tests/NSURLConnection.mm

 - (instancetype)init {
    if (self = [super init]) {
-        self->_condition = [[NSCondition alloc] init];
+        self->_condition = [NSCondition new];


self->_condition [](start = 8, length = 16)

just use _condition, I almost confused myself thinking this was +2.

is _condition really more clear than self->_condition?

It would be, given self.* isn't a syntax access of the instance variable directly. It would be confusing to use self-> which is a direct instance access (by passing the property)

In reply to: 111228615 [](ancestors = 111228615)

it wouldn't be a good choice to use self->, unless you have a specific reason to.

In reply to: 111229286 [](ancestors = 111229286,111228615)

I generally default to using self-> in inits, to guard against [super init] doing a replacement. That's not really possible here since the superclass is NSObject, so I'll change it, but I disagree that there needs to be a specific reason to use self->, especially since inheritance interactions aren't always this clear...

In reply to: 111229643 [](ancestors = 111229643,111229286,111228615)

oops im just wrong

In reply to: 111231093 [](ancestors = 111231093,111229643,111229286,111228615)

msft-Jeyaram

⏲

msft-Jeyaram · 2017-04-12T18:47:24Z

tests/functionaltests/Tests/NSURLConnection.mm

+@implementation NSURLConnectionTestHelper_OperationQueue
+- (instancetype)init {
+    if (self = [super init]) {
+        self->_operationQueue = [NSOperationQueue new];


self-> [](start = 7, length = 7)

here too

msftclas added the cla-not-required label Apr 7, 2017

ms-jihua commented Apr 7, 2017

View reviewed changes

msft-Jeyaram reviewed Apr 7, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 7, 2017

View reviewed changes

aballway reviewed Apr 7, 2017

View reviewed changes

msft-Jeyaram reviewed Apr 7, 2017

View reviewed changes

rajsesh reviewed Apr 7, 2017

View reviewed changes

msft-Jeyaram suggested changes Apr 7, 2017

View reviewed changes

cr feedback

50a0f16

DHowett-MSFT suggested changes Apr 10, 2017

View reviewed changes

DHowett-MSFT reviewed Apr 10, 2017

View reviewed changes

oops

c11f9e5

DHowett-MSFT approved these changes Apr 10, 2017

View reviewed changes

ms-jihua added 2 commits April 10, 2017 17:04

cr feedback

a0b00c7

remove NSURLConnectionState from vcxproj also

ddf244d

aballway approved these changes Apr 12, 2017

View reviewed changes

msft-Jeyaram reviewed Apr 12, 2017

View reviewed changes

msft-Jeyaram suggested changes Apr 12, 2017

View reviewed changes

msft-Jeyaram reviewed Apr 12, 2017

View reviewed changes

cr feedback

de07fe6

msft-Jeyaram approved these changes Apr 13, 2017

View reviewed changes

ms-jihua merged commit 4c6f2ed into microsoft:develop Apr 13, 2017


		[connection cancel];
		std::vector<std::pair<StrongId<NSRunLoop>, StrongId<NSString>>> _scheduledRunLoops; // NSRunLoop, NSRunLoopMode


		// Make sure there was no error.
		ASSERT_TRUE_MSG((connectionTestHelper.error == nil), "FAILED: Connection returned error %@!", connectionTestHelper.error);


		// Test to verify a request can be successfully made but no data was received and a valid response error code was received.
		static void _testRequestWithURL_Failure(NSURLConnectionTestHelper* connectionTestHelper) {

Refactor parts of NSURLConnection #2464

Refactor parts of NSURLConnection #2464

Conversation

ms-jihua commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-Jeyaram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-jihua Apr 7, 2017 •

edited

Loading

DHowett-MSFT Apr 7, 2017 •

edited

Loading

ms-jihua Apr 10, 2017 •

edited

Loading

ms-jihua Apr 10, 2017 •

edited

Loading