Target refactor (#373)

Author: kblok

lib/PuppeteerSharp.Tests/TargetTests/TargetTests.cs

@@ -19,9 +19,9 @@ public void BrowserTargetsShouldReturnAllOfTheTargets()
         {
             // The pages will be the testing page and the original newtab page
             var targets = Browser.Targets();
-            Assert.Contains(targets, target => target.Type == "page"
+            Assert.Contains(targets, target => target.Type == TargetType.Page
                 && target.Url == TestConstants.AboutBlank);
-            Assert.Contains(targets, target => target.Type == "browser");
+            Assert.Contains(targets, target => target.Type == TargetType.Browser);
         }
 
         [Fact]
@@ -38,7 +38,7 @@ public async Task BrowserPagesShouldReturnAllOfThePages()
         public void ShouldContainBrowserTarget()
         {
             var targets = Browser.Targets();
-            var browserTarget = targets.FirstOrDefault(target => target.Type == "browser");
+            var browserTarget = targets.FirstOrDefault(target => target.Type == TargetType.Browser);
             Assert.NotNull(browserTarget);
         }
 
@@ -102,7 +102,7 @@ void TargetCreatedEventHandler(object sender, TargetChangedArgs e)
             await Page.GoToAsync(TestConstants.ServerUrl + "/serviceworkers/empty/sw.html");
 
             var createdTarget = await createdTargetTaskCompletion.Task;
-            Assert.Equal("service_worker", createdTarget.Type);
+            Assert.Equal(TargetType.ServiceWorker, createdTarget.Type);
             Assert.Equal(TestConstants.ServerUrl + "/serviceworkers/empty/sw.js", createdTarget.Url);
 
             var targetDestroyedTaskCompletion = new TaskCompletionSource<Target>();
@@ -144,7 +144,7 @@ void ChangedTargetEventHandler(object sender, TargetChangedArgs e)
         public async Task ShouldNotReportUninitializedPages()
         {
             var targetChanged = false;
-            EventHandler<TargetChangedArgs> listener = (sender, e) => targetChanged = true;
+            void listener(object sender, TargetChangedArgs e) => targetChanged = true;
             Browser.TargetChanged += listener;
             var targetCompletionTask = new TaskCompletionSource<Target>();
             void TargetCreatedEventHandler(object sender, TargetChangedArgs e)

lib/PuppeteerSharp/Browser.cs

@@ -55,11 +55,13 @@ public Browser(Connection connection, IBrowserOptions options, Process process,
             Connection.MessageReceived += Connect_MessageReceived;
 
             _closeCallBack = closeCallBack;
+            _logger = Connection.LoggerFactory.CreateLogger<Browser>();
         }
 
         #region Private members
         private readonly Dictionary<string, Target> _targets;
         private readonly Func<Task> _closeCallBack;
+        private readonly ILogger<Browser> _logger;
         #endregion
 
         #region Properties
@@ -138,7 +140,7 @@ public async Task<Page> NewPageAsync()
         {
             string targetId = (await Connection.SendAsync("Target.createTarget", new Dictionary<string, object>
             {
-                {"url", "about:blank"}
+                ["url"] = "about:blank"
             })).targetId.ToString();
 
             var target = _targets[targetId];
@@ -272,7 +274,7 @@ private async Task DestroyTargetAsync(TargetDestroyedResponse e)
             }
             if (await target.InitializedTask)
             {
-                TargetDestroyed?.Invoke(this, new TargetChangedArgs()
+                TargetDestroyed?.Invoke(this, new TargetChangedArgs
                 {
                     Target = target
                 });
@@ -281,12 +283,21 @@ private async Task DestroyTargetAsync(TargetDestroyedResponse e)
 
         private async Task CreateTargetAsync(TargetCreatedResponse e)
         {
-            var target = new Target(this, e.TargetInfo);
+            var target = new Target(
+                e.TargetInfo,
+                () => Connection.CreateSessionAsync(e.TargetInfo.TargetId),
+                this);
+
+            if (_targets.ContainsKey(e.TargetInfo.TargetId))
+            {
+                _logger.LogError("Target should not exist before targetCreated");
+            }
+
             _targets[e.TargetInfo.TargetId] = target;
 
             if (await target.InitializedTask)
             {
-                TargetCreated?.Invoke(this, new TargetChangedArgs()
+                TargetCreated?.Invoke(this, new TargetChangedArgs
                 {
                     Target = target
                 });

lib/PuppeteerSharp/Helpers/FlexibleStringEnumConverter.cs

@@ -0,0 +1,28 @@
+using System;
+using Newtonsoft.Json;
+using Newtonsoft.Json.Converters;
+
+namespace PuppeteerSharp.Helpers
+{
+    internal class FlexibleStringEnumConverter : StringEnumConverter
+    {
+        private Enum _fallbackValue;
+
+        public FlexibleStringEnumConverter(Enum fallbackValue)
+        {
+            _fallbackValue = fallbackValue;
+        }
+
+        public override object ReadJson(JsonReader reader, Type objectType, object existingValue, JsonSerializer serializer)
+        {
+            try
+            {
+                return base.ReadJson(reader, objectType, existingValue, serializer);
+            }
+            catch
+            {
+                return _fallbackValue;
+            }
+        }
+    }
+}

lib/PuppeteerSharp/Target.cs

@@ -1,5 +1,7 @@
-using System.Diagnostics;
+using System;
+using System.Diagnostics;
 using System.Threading.Tasks;
+using PuppeteerSharp.Helpers;
 
 namespace PuppeteerSharp
 {
@@ -10,25 +12,30 @@ namespace PuppeteerSharp
     public class Target
     {
         #region Private members
-        private Browser _browser;
+        private readonly Browser _browser;
         private TargetInfo _targetInfo;
+        private string _targetId;
+        private Func<Task<CDPSession>> _sessionFactory;
         private Task<Page> _pageTask;
         #endregion
 
         internal bool IsInitialized;
 
-        internal Target(Browser browser, TargetInfo targetInfo)
+        internal Target(TargetInfo targetInfo, Func<Task<CDPSession>> sessionFactory, Browser browser)
         {
-            _browser = browser;
             _targetInfo = targetInfo;
+            _targetId = targetInfo.TargetId;
+            _sessionFactory = sessionFactory;
+            _browser = browser;
+            _pageTask = null;
 
             InitilizedTaskWrapper = new TaskCompletionSource<bool>();
             CloseTaskWrapper = new TaskCompletionSource<bool>();
-            IsInitialized = _targetInfo.Type != "page" || _targetInfo.Url != string.Empty;
+            IsInitialized = _targetInfo.Type != TargetType.Page || _targetInfo.Url != string.Empty;
 
-            if (IsInitialized)
-            {
-                InitilizedTaskWrapper.SetResult(true);
+            if (IsInitialized)
+            {
+                InitilizedTaskWrapper.SetResult(true);
             }
         }
 
@@ -42,7 +49,8 @@ internal Target(Browser browser, TargetInfo targetInfo)
         /// Gets the type. It will be <see cref="TargetInfo.Type"/> if it's "page" or "service_worker". Otherwise it will be "other"
         /// </summary>
         /// <value>The type.</value>
-        public string Type => _targetInfo.Type == "page" || _targetInfo.Type == "service_worker" || _targetInfo.Type == "browser" ? _targetInfo.Type : "other";
+        public TargetType Type => _targetInfo.Type;
+
         /// <summary>
         /// Gets the target identifier.
         /// </summary>
@@ -60,22 +68,26 @@ internal Target(Browser browser, TargetInfo targetInfo)
         /// <returns>a task that returns a new <see cref="Page"/></returns>
         public async Task<Page> PageAsync()
         {
-            if (_targetInfo.Type == "page" && _pageTask == null)
-            {
-                _pageTask = await _browser.Connection.CreateSessionAsync(_targetInfo.TargetId)
-                    .ContinueWith(clientTask
-                    => Page.CreateAsync(clientTask.Result, this, _browser.IgnoreHTTPSErrors, _browser.AppMode, _browser.ScreenshotTaskQueue));
+            if (_targetInfo.Type == TargetType.Page && _pageTask == null)
+            {
+                _pageTask = CreatePageAsync();
             }
 
             return await (_pageTask ?? Task.FromResult<Page>(null));
         }
 
+        private async Task<Page> CreatePageAsync()
+        {
+            var session = await _sessionFactory();
+            return await Page.CreateAsync(session, this, _browser.IgnoreHTTPSErrors, _browser.AppMode, _browser.ScreenshotTaskQueue);
+        }
+
         internal void TargetInfoChanged(TargetInfo targetInfo)
         {
             var previousUrl = _targetInfo.Url;
             _targetInfo = targetInfo;
 
-            if (!IsInitialized && (_targetInfo.Type != "page" || _targetInfo.Url != string.Empty))
+            if (!IsInitialized && (_targetInfo.Type != TargetType.Page || _targetInfo.Url != string.Empty))
             {
                 IsInitialized = true;
                 InitilizedTaskWrapper.SetResult(true);

lib/PuppeteerSharp/TargetInfo.cs

@@ -16,7 +16,7 @@ internal TargetInfo()
         /// </summary>
         /// <value>The type.</value>
         [JsonProperty("type")]
-        public string Type { get; internal set; }
+        public TargetType Type { get; internal set; }
         /// <summary>
         /// Gets or sets the URL.
         /// </summary>
@@ -28,6 +28,6 @@ internal TargetInfo()
         /// </summary>
         /// <value>The target identifier.</value>
         [JsonProperty("targetId")]
-        public string TargetId { get; internal set; }        
+        public string TargetId { get; internal set; }
     }
-}
+}

lib/PuppeteerSharp/TargetType.cs

@@ -0,0 +1,34 @@
+using System.Runtime.Serialization;
+using Newtonsoft.Json;
+using Newtonsoft.Json.Converters;
+using PuppeteerSharp.Helpers;
+
+namespace PuppeteerSharp
+{
+    /// <summary>
+    /// Target type.
+    /// </summary>
+    [JsonConverter(typeof(FlexibleStringEnumConverter), Other)]
+    public enum TargetType
+    {
+        /// <summary>
+        /// The other.
+        /// </summary>
+        Other,
+        /// <summary>
+        /// Target type page.
+        /// </summary>
+        [EnumMember(Value = "page")]
+        Page,
+        /// <summary>
+        /// Target type service worker.
+        /// </summary>
+        [EnumMember(Value = "service_worker")]
+        ServiceWorker,
+        /// <summary>
+        /// Target type browser.
+        /// </summary>
+        [EnumMember(Value = "browser")]
+        Browser
+    }
+}