papegaaij commented on code in PR #1595:
URL: https://github.com/apache/wicket/pull/1595#discussion_r3992819787
##########
wicket-core/src/main/java/org/apache/wicket/ajax/AbstractDefaultAjaxBehavior.java:
##########
@@ -89,12 +89,6 @@ protected void onBind()
final Component component = getComponent();
component.setOutputMarkupId(true);
-
Review Comment:
Because an id no longer has to be handed out in order to exist.
On master an id is a position in a `BehaviorIdList` kept in the component's
meta data, allocated in the order `getBehaviorId` is first called. That order
is whatever rendering happens to do, which is not reproducible for a stateless
page: the page is thrown away and rebuilt on the next request, while the id is
already baked into the callback url in the markup. Calling `getBehaviorId` from
`onBind` pinned the id to *add* order instead, which is reproducible. That is
what the block was for.
On this branch an id *is* the behavior's index in the component's behavior
array, so it is add order by construction and there is nothing left to force.
Doing it eagerly would now cost something, too. `getBehaviorId` compacts the
behavior array one last time and then sets `FLAG_BEHAVIOR_IDS_FIXED`, which
stops it from ever being compacted again so that the ids it hands out stay put.
Calling it at bind time would set that flag on every component carrying an ajax
behavior, so it would keep the gaps left by removed behaviors for the rest of
its life - exactly the footprint this PR is trying to get rid of. The stability
guarantee itself is unchanged; it just starts at first use rather than at bind.
I wrote the contract down on `IRequestableComponent#getBehaviorId` in 7323aca.
##########
wicket-core/src/main/java/org/apache/wicket/core/util/resource/locator/ExtensionResourceNameIterator.java:
##########
@@ -42,13 +42,14 @@ public class ExtensionResourceNameIterator implements
Iterator<String>
public ExtensionResourceNameIterator(final Iterable<String> extensions)
{
// Fail safe: hasNext() needs to return at least once with true
- if (extensions == null || !extensions.iterator().hasNext())
+ Iterator<String> extensionIterator = extensions == null ? null
: extensions.iterator();
+ if (extensionIterator == null || !extensionIterator.hasNext())
{
this.iterator = NULL_ITERABLE.iterator();
Review Comment:
It is, and this change does not move that either way.
`ExtensionResourceNameIterator` instances are per lookup -
`ResourceNameIterator` constructs one per `locate()` - so they are never shared
between threads. The only shared state is `NULL_ITERABLE`, which predates this
branch: a one-element `Arrays.asList` that is never mutated, and whose
`iterator()` hands out a fresh `ArrayItr` with its own cursor on every call.
`ArrayItr` does not implement `remove()`, so
`ExtensionResourceNameIterator.remove()` throws `UnsupportedOperationException`
on that path rather than touching the shared list.
What the change does is ask `extensions` for an iterator once instead of
twice. `hasNext()` does not advance, so reusing the iterator the check was made
on is equivalent to throwing it away and asking for a second one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
