Skip to content
This repository was archived by the owner on Dec 19, 2018. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,14 @@ namespace Microsoft.AspNetCore.Razor.Runtime.TagHelpers
/// </summary>
public class TagHelperExecutionContext
Copy link
Member

Choose a reason for hiding this comment

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

I have to admit, I was expecting to see a codegen based solution - why is this better?

Copy link
Author

Choose a reason for hiding this comment

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

Codegen would be doing roughly the same thing and would only save a single list allocation. Would have been much more complicated for very little gain.

{
private readonly string _tagName;
private readonly string _uniqueId;
private readonly TagMode _tagMode;
private readonly List<ITagHelper> _tagHelpers;
private readonly Func<Task> _executeChildContentAsync;
private readonly Action<HtmlEncoder> _startTagHelperWritingScope;
private readonly Func<TagHelperContent> _endTagHelperWritingScope;
Copy link
Contributor

Choose a reason for hiding this comment

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

Too much understanding of how MVC uses the scope manager here. The start and end bits (Action<HtmlEncoder> and Func<TagHelperContent>) are passed into the Begin() method and should not be treated as invariant here. Update TagHelperScopeManager and code gen to pass these delegates into the TagHelperScopeManager constructor. Then passing them into this class' constructor obviously be fine.

Copy link
Author

Choose a reason for hiding this comment

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

So add these two bits to the Reinitialize method?

Copy link
Contributor

Choose a reason for hiding this comment

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

Has always been strange that these were passed into TagHelperScopeManager.Begin(). MVC doesn't have a reason to change the values from call to call. (Don't recall if it ever passed anything but a method name.) So, while we could pile more into Reinitialize(), better to give the values a sensible lifetime.

Bottom line, change the TagHelperScopeManager constructor or file a follow-up issue to do so.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thx

private TagHelperContent _childContent;
private string _tagName;
private string _uniqueId;
private TagMode _tagMode;
private Func<Task> _executeChildContentAsync;
private Dictionary<HtmlEncoder, TagHelperContent> _perEncoderChildContent;
private TagHelperAttributeList _htmlAttributes;
private TagHelperAttributeList _allAttributes;
Expand Down Expand Up @@ -63,26 +63,6 @@ public TagHelperExecutionContext(
Action<HtmlEncoder> startTagHelperWritingScope,
Func<TagHelperContent> endTagHelperWritingScope)
{
if (tagName == null)
{
throw new ArgumentNullException(nameof(tagName));
}

if (items == null)
{
throw new ArgumentNullException(nameof(items));
}

if (uniqueId == null)
{
throw new ArgumentNullException(nameof(uniqueId));
}

if (executeChildContentAsync == null)
{
throw new ArgumentNullException(nameof(executeChildContentAsync));
}

if (startTagHelperWritingScope == null)
{
throw new ArgumentNullException(nameof(startTagHelperWritingScope));
Expand All @@ -94,14 +74,11 @@ public TagHelperExecutionContext(
}

_tagHelpers = new List<ITagHelper>();
_executeChildContentAsync = executeChildContentAsync;

Reinitialize(tagName, tagMode, items, uniqueId, executeChildContentAsync);
Copy link
Contributor

Choose a reason for hiding this comment

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

Call this after initializing _tagHelpers and just .Clear() that field in Reinitialize().


_startTagHelperWritingScope = startTagHelperWritingScope;
_endTagHelperWritingScope = endTagHelperWritingScope;

_tagMode = tagMode;
_tagName = tagName;
Items = items;
_uniqueId = uniqueId;
}

/// <summary>
Expand All @@ -118,7 +95,7 @@ public bool ChildContentRetrieved
/// <summary>
/// Gets the collection of items used to communicate with other <see cref="ITagHelper"/>s.
/// </summary>
public IDictionary<object, object> Items { get; }
public IDictionary<object, object> Items { get; private set; }

/// <summary>
/// <see cref="ITagHelper"/>s that should be run.
Expand Down Expand Up @@ -214,6 +191,53 @@ public void AddTagHelperAttribute(string name, object value)
_allAttributes.Add(name, value);
}

/// <summary>
/// Clears the <see cref="TagHelperExecutionContext"/> and updates its state with the provided values.
/// </summary>
/// <param name="tagName">The tag name to use.</param>
/// <param name="tagMode">The <see cref="TagMode"/> to use.</param>
/// <param name="items">The <see cref="IDictionary{Object, Object}"/> to use.</param>
/// <param name="uniqueId">The unique id to use.</param>
/// <param name="executeChildContentAsync">The <see cref="Func{Task}"/> to use.</param>
public void Reinitialize(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest removing overlapping parameters from the constructors, requiring a Reinitialize() call for all uses. Alternatively call Reinitialize() from the constructors duplicating this code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thx

string tagName,
TagMode tagMode,
IDictionary<object, object> items,
string uniqueId,
Func<Task> executeChildContentAsync)
{
if (tagName == null)
{
throw new ArgumentNullException(nameof(tagName));
}

if (items == null)
{
throw new ArgumentNullException(nameof(items));
}

if (uniqueId == null)
{
throw new ArgumentNullException(nameof(uniqueId));
}

if (executeChildContentAsync == null)
{
throw new ArgumentNullException(nameof(executeChildContentAsync));
}

_tagName = tagName;
_tagMode = tagMode;
Items = items;
_uniqueId = uniqueId;
_executeChildContentAsync = executeChildContentAsync;
_tagHelpers.Clear();
_perEncoderChildContent?.Clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 like reusing this dictionary.

_htmlAttributes = null;
_allAttributes = null;
_childContent = null;
Copy link
Member

Choose a reason for hiding this comment

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

The next part will be to see what we can do about these things

}

// Internal for testing.
internal async Task<TagHelperContent> GetChildContentAsync(bool useCachedResult, HtmlEncoder encoder)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ namespace Microsoft.AspNetCore.Razor.Runtime.TagHelpers
/// </summary>
public class TagHelperScopeManager
{
private readonly Stack<TagHelperExecutionContext> _executionScopes;
private readonly ExecutionContextPool _executionContextPool;

/// <summary>
/// Instantiates a new <see cref="TagHelperScopeManager"/>.
/// </summary>
public TagHelperScopeManager()
{
_executionScopes = new Stack<TagHelperExecutionContext>();
_executionContextPool = new ExecutionContextPool();
}

/// <summary>
Expand Down Expand Up @@ -72,20 +72,21 @@ public TagHelperExecutionContext Begin(
}

IDictionary<object, object> items;
var parentExecutionContext = _executionContextPool.Current;

// If we're not wrapped by another TagHelper, then there will not be a parentExecutionContext.
if (_executionScopes.Count > 0)
if (parentExecutionContext != null)
{
items = new CopyOnWriteDictionary<object, object>(
_executionScopes.Peek().Items,
parentExecutionContext.Items,
comparer: EqualityComparer<object>.Default);
}
else
{
items = new Dictionary<object, object>();
}

var executionContext = new TagHelperExecutionContext(
var executionContext = _executionContextPool.Rent(
tagName,
tagMode,
items,
Expand All @@ -94,8 +95,6 @@ public TagHelperExecutionContext Begin(
startTagHelperWritingScope,
endTagHelperWritingScope);

_executionScopes.Push(executionContext);

return executionContext;
}

Expand All @@ -106,7 +105,7 @@ public TagHelperExecutionContext Begin(
/// <c>null</c> otherwise.</returns>
public TagHelperExecutionContext End()
{
if (_executionScopes.Count == 0)
if (_executionContextPool.Current == null)
{
throw new InvalidOperationException(
Resources.FormatScopeManager_EndCannotBeCalledWithoutACallToBegin(
Expand All @@ -115,14 +114,61 @@ public TagHelperExecutionContext End()
nameof(TagHelperScopeManager)));
}

_executionScopes.Pop();
_executionContextPool.ReturnCurrent();

var parentExecutionContext = _executionContextPool.Current;

return parentExecutionContext;
}

private class ExecutionContextPool
{
private readonly List<TagHelperExecutionContext> _executionContexts;
private int _nextIndex;

public ExecutionContextPool()
{
_executionContexts = new List<TagHelperExecutionContext>();
}

public TagHelperExecutionContext Current => _nextIndex > 0 ? _executionContexts[_nextIndex - 1] : null;

if (_executionScopes.Count != 0)
public TagHelperExecutionContext Rent(
string tagName,
TagMode tagMode,
IDictionary<object, object> items,
string uniqueId,
Func<Task> executeChildContentAsync,
Action<HtmlEncoder> startTagHelperWritingScope,
Func<TagHelperContent> endTagHelperWritingScope)
{
return _executionScopes.Peek();
TagHelperExecutionContext tagHelperExecutionContext;

if (_nextIndex == _executionContexts.Count)
{
tagHelperExecutionContext = new TagHelperExecutionContext(
tagName,
tagMode,
items,
uniqueId,
executeChildContentAsync,
startTagHelperWritingScope,
endTagHelperWritingScope);

_executionContexts.Add(tagHelperExecutionContext);
}
else
{
tagHelperExecutionContext = _executionContexts[_nextIndex];
Copy link
Contributor

Choose a reason for hiding this comment

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

Debug.Assert(_nextIndex >= 0) to give this class some control over its own destiny. Really just double-checking TagHelperScopeManager.End() throws when it should.

Copy link
Author

Choose a reason for hiding this comment

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

Since TagHelperScopeManager.End does the throw (with a test that verifies it throws) I think it'd be a bit much to also have a Debug.Assert here

tagHelperExecutionContext.Reinitialize(tagName, tagMode, items, uniqueId, executeChildContentAsync);
}

_nextIndex++;

return tagHelperExecutionContext;
}

return null;
public void ReturnCurrent() => _nextIndex--;
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.Encodings.Web;
Expand All @@ -13,6 +14,106 @@ namespace Microsoft.AspNetCore.Razor.Runtime.TagHelpers
{
public class TagHelperExecutionContextTest
{
[Fact]
public async Task ExecutionContext_Reinitialize_UpdatesTagHelperOutputAsExpected()
{
// Arrange
var tagName = "div";
var tagMode = TagMode.StartTagOnly;
var callCount = 0;
Func<Task> executeChildContentAsync = () =>
{
callCount++;
return Task.FromResult(true);
};
Action<HtmlEncoder> startTagHelperWritingScope = _ => { };
Func<TagHelperContent> endTagHelperWritingScope = () => null;
var executionContext = new TagHelperExecutionContext(
tagName,
tagMode,
items: new Dictionary<object, object>(),
uniqueId: string.Empty,
executeChildContentAsync: executeChildContentAsync,
startTagHelperWritingScope: startTagHelperWritingScope,
endTagHelperWritingScope: endTagHelperWritingScope);
var updatedTagName = "p";
var updatedTagMode = TagMode.SelfClosing;
var updatedCallCount = 0;
Func<Task> updatedExecuteChildContentAsync = () =>
{
updatedCallCount++;
return Task.FromResult(true);
};
executionContext.AddMinimizedHtmlAttribute("something");

// Act - 1
executionContext.Reinitialize(
updatedTagName,
updatedTagMode,
items: new Dictionary<object, object>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please something in this dictionary and confirm it exists in // Assert - 1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind this one. Done in the other test.

uniqueId: string.Empty,
executeChildContentAsync: updatedExecuteChildContentAsync);
executionContext.AddMinimizedHtmlAttribute("Another attribute");

// Assert - 1
var output = executionContext.CreateTagHelperOutput();
Assert.Equal(updatedTagName, output.TagName);
Assert.Equal(updatedTagMode, output.TagMode);
var attribute = Assert.Single(output.Attributes);
Assert.Equal("Another attribute", attribute.Name);

// Act - 2
await output.GetChildContentAsync();

// Assert - 2
Assert.Equal(callCount, 0);
Assert.Equal(updatedCallCount, 1);
}

[Fact]
public void ExecutionContext_Reinitialize_UpdatesTagHelperContextAsExpected()
{
// Arrange
var tagName = "div";
var tagMode = TagMode.StartTagOnly;
var items = new Dictionary<object, object>();
var uniqueId = "some unique id";
var callCount = 0;
Func<Task> executeChildContentAsync = () =>
{
callCount++;
return Task.FromResult(true);
};
Action<HtmlEncoder> startWritingScope = _ => { };
Func<TagHelperContent> endWritingScope = () => null;
var executionContext = new TagHelperExecutionContext(
tagName,
tagMode,
items,
uniqueId,
executeChildContentAsync,
startWritingScope,
endWritingScope);
var updatedItems = new Dictionary<object, object>();
var updatedUniqueId = "another unique id";
executionContext.AddMinimizedHtmlAttribute("something");

// Act
executionContext.Reinitialize(
tagName,
tagMode,
updatedItems,
updatedUniqueId,
executeChildContentAsync);
executionContext.AddMinimizedHtmlAttribute("Another attribute");

// Assert
var context = executionContext.CreateTagHelperContext();
var attribute = Assert.Single(context.AllAttributes);
Assert.Equal(attribute.Name, "Another attribute");
Assert.Equal(updatedUniqueId, context.UniqueId);
Assert.Same(updatedItems, context.Items);
}

[Theory]
[InlineData(TagMode.SelfClosing)]
Expand Down