Skip to content
This repository has been archived by the owner on Jul 18, 2018. It is now read-only.

Commit

Permalink
Reland "Flatten ModuleTreeLinker"
Browse files Browse the repository at this point in the history
This CL relands https://chromium-review.googlesource.com/c/chromium/src/+/583552
with a fix for test failure in release builds:
Move FindFirstParseError() call out of DCHECK().

Original CL description:

1. This CL applies the spec change by
whatwg/html#2971.
Since [1], the code has already been behaving like the spec PR 2971, and this CL
makes the code align with PR 2971 more completely, by removing AncestorList
and adding spec comments.

2. This CL also flattens the code structure of ModuleTreeLinker, which is
enabled by the spec PR 2971, to make the code simpler to make further
optimizations easy.
That is, instead of creating a ModuleTreeLinker for each module script
(i.e. for each "fetch the descendants" call) in a module graph, this CL
creates a single ModuleTreeLinker that corresponds to a top-level module graph
script.

Most of fetching-related classes/methods, including
- ModuleTreeLinker::DependencyModuleClient
- ModuleTreeLinker::FetchDescendants()
- ModuleTreeLinker::NotifyOneDescendantFinished()
are merged into the single method NotifyModuleLoadFinished() that implements
the main body of "fetch the descendants" and
"internal module script graph fetching procedure".

This also removes ModuleTreeReachedUrlSet and instead uses HashSet<KURL>
directly, as we no longer have to share it across multiple ModuleTreeLinkers.
Modulator::FetchTreeInternal() is removed as we no longer create
ModuleTreeLinkers for subtree fetching via Modulator.

3. This CL applies a part of
whatwg/html#2991, particularly introduces
FindFirstParseError() that corresponds to "find the first parse error" and
use it to find the error to be reported, instead of propagating errors
based on Step 6.1 and 6.2 of "fetch the descendants".

(*) In some subtle cases, this might cause behavior changes in error reporting,
but these changes shouldn't be significant, because anyway the spec before
PR 2991 (and thus the previous implementation) behaves nondeterministically
in some similarly subtle cases.

This CL is an intermediate step to apply spec PRs 2971 and 2991.
This CL refactors largely ModuleTreeLinker with keeping the existing behavior
mostly (except for (*)), and subsequent CLs will apply the behavior changes
and remaining code structure changes introduced by PR 2991.

Bug: 763597
Change-Id: I4136db0db4bc8a0cdc7f5c23b18787b405d8c98f
Reviewed-on: https://chromium-review.googlesource.com/674748
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503126}
  • Loading branch information
hiroshige-g authored and Commit Bot committed Sep 20, 2017
1 parent 07ea99d commit b2ca65c
Show file tree
Hide file tree
Showing 14 changed files with 387 additions and 571 deletions.
22 changes: 0 additions & 22 deletions third_party/WebKit/Source/core/dom/AncestorList.h

This file was deleted.

1 change: 0 additions & 1 deletion third_party/WebKit/Source/core/dom/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ blink_core_sources("dom") {
"AccessibleNode.h",
"AccessibleNodeList.cpp",
"AccessibleNodeList.h",
"AncestorList.h",
"AnimationWorkletProxyClient.cpp",
"AnimationWorkletProxyClient.h",
"Attr.cpp",
Expand Down
9 changes: 0 additions & 9 deletions third_party/WebKit/Source/core/dom/Modulator.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include "bindings/core/v8/ScriptModule.h"
#include "core/CoreExport.h"
#include "core/dom/AncestorList.h"
#include "platform/bindings/ScriptWrappable.h"
#include "platform/bindings/V8PerContextData.h"
#include "platform/heap/Handle.h"
Expand All @@ -24,7 +23,6 @@ class ModuleScript;
class ModuleScriptFetchRequest;
class ModuleScriptFetcher;
class ModuleScriptLoaderClient;
class ModuleTreeReachedUrlSet;
class ReferrerScriptInfo;
class ScriptModuleResolver;
class ScriptPromiseResolver;
Expand Down Expand Up @@ -91,13 +89,6 @@ class CORE_EXPORT Modulator : public GarbageCollectedFinalized<Modulator>,
virtual void FetchTree(const ModuleScriptFetchRequest&,
ModuleTreeClient*) = 0;

// https://html.spec.whatwg.org/#internal-module-script-graph-fetching-procedure
virtual void FetchTreeInternal(const ModuleScriptFetchRequest&,
const AncestorList&,
ModuleGraphLevel,
ModuleTreeReachedUrlSet*,
ModuleTreeClient*) = 0;

// Asynchronously retrieve a module script from the module map, or fetch it
// and put it in the map if it's not there already.
// https://html.spec.whatwg.org/#fetch-a-single-module-script
Expand Down
22 changes: 5 additions & 17 deletions third_party/WebKit/Source/core/dom/ModulatorImplBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,30 +56,18 @@ void ModulatorImplBase::FetchTree(const ModuleScriptFetchRequest& request,
// its argument.
DCHECK(request.GetReferrer().IsNull());

AncestorList empty_ancestor_list;
FetchTreeInternal(request, empty_ancestor_list,
ModuleGraphLevel::kTopLevelModuleFetch, nullptr, client);
// We ensure module-related code is not executed without the flag.
// https://crbug.com/715376
CHECK(RuntimeEnabledFeatures::ModuleScriptsEnabled());

tree_linker_registry_->Fetch(request, this, client);

// Step 2. When the internal module script graph fetching procedure
// asynchronously completes with result, asynchronously complete this
// algorithm with result.
// Note: We delegate to ModuleTreeLinker to notify ModuleTreeClient.
}

void ModulatorImplBase::FetchTreeInternal(
const ModuleScriptFetchRequest& request,
const AncestorList& ancestor_list,
ModuleGraphLevel level,
ModuleTreeReachedUrlSet* reached_url_set,
ModuleTreeClient* client) {
// We ensure module-related code is not executed without the flag.
// https://crbug.com/715376
CHECK(RuntimeEnabledFeatures::ModuleScriptsEnabled());

tree_linker_registry_->Fetch(request, ancestor_list, level, this,
reached_url_set, client);
}

void ModulatorImplBase::FetchDescendantsForInlineScript(
ModuleScript* module_script,
ModuleTreeClient* client) {
Expand Down
6 changes: 0 additions & 6 deletions third_party/WebKit/Source/core/dom/ModulatorImplBase.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ class ExecutionContext;
class ModuleMap;
class ModuleScriptLoaderRegistry;
class ModuleTreeLinkerRegistry;
class ModuleTreeReachedUrlSet;
class ScriptState;
class WebTaskRunner;

Expand Down Expand Up @@ -51,11 +50,6 @@ class ModulatorImplBase : public Modulator {
void FetchTree(const ModuleScriptFetchRequest&, ModuleTreeClient*) override;
void FetchDescendantsForInlineScript(ModuleScript*,
ModuleTreeClient*) override;
void FetchTreeInternal(const ModuleScriptFetchRequest&,
const AncestorList&,
ModuleGraphLevel,
ModuleTreeReachedUrlSet*,
ModuleTreeClient*) override;
void FetchSingle(const ModuleScriptFetchRequest&,
ModuleGraphLevel,
SingleModuleClient*) override;
Expand Down
1 change: 0 additions & 1 deletion third_party/WebKit/Source/core/loader/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ blink_core_sources("loader") {
"modulescript/ModuleTreeLinker.h",
"modulescript/ModuleTreeLinkerRegistry.cpp",
"modulescript/ModuleTreeLinkerRegistry.h",
"modulescript/ModuleTreeReachedUrlSet.h",
"modulescript/WorkletModuleScriptFetcher.cpp",
"modulescript/WorkletModuleScriptFetcher.h",
"private/FrameClientHintsPreferencesContext.cpp",
Expand Down
Loading

0 comments on commit b2ca65c

Please sign in to comment.