Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821803002:
Introduce ScriptLoader::script_type_ (Closed)
|
Created:
3 years, 8 months ago by hiroshige Modified:
3 years, 8 months ago CC:
chromium-reviews, Yoav Weiss, blink-reviews-html_chromium.org, sof, eae+blinkwatch, loading-reviews+parser_chromium.org, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce ScriptLoader::script_type_
This CL introduces the logic for determining the script type to ScriptLoader.
script_type_ will be used by subsequent CLs.
This CL also makes Step 6 of ScriptLoader::PrepareScript()
(IsValidScriptTypeAndLanguage()) to correspond directly to the spec.
This CL doesn't change the behavior.
BUG=594639, 686281
Review-Url: https://codereview.chromium.org/2821803002
Cr-Commit-Position: refs/heads/master@{#465443}
Committed: https://chromium.googlesource.com/chromium/src/+/08697f894248cda25e67bd5c82565ffbc33f21fa
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase #Patch Set 3 : Add missing include #
Messages
Total messages: 27 (15 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make Step 6 of PrepareScript() to correspond directly to the spec BUG= ========== to ========== Introduce ScriptLoader::script_type_ This CL introduces the logic for determining the script type to ScriptLoader. script_type_ will be used by subsequent CLs. This CL also makes Step 6 of ScriptLoader::PrepareScript() (IsValidScriptTypeAndLanguage()) to correspond directly to the spec. This CL doesn't change the behavior. BUG=594639, 686281 ==========
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, kouhei@chromium.org, sigbjornf@opera.com
PTAL. I started the review for this part (while script_type_ is not yet used anywhere) because this touches the same code region (Step 6, prepare a script) as Nate's noModule effort, and making Step 6 more spec-conformant makes noModule impl also more spec-conformant.
https://codereview.chromium.org/2821803002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.cpp (right): https://codereview.chromium.org/2821803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:191: bool ScriptLoader::IsValidScriptTypeAndLanguage( rename this to DetermineScriptType and have this return ScriptType https://codereview.chromium.org/2821803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.cpp:217: bool ScriptLoader::IsScriptTypeSupported(LegacyTypeSupport support_legacy_types, DetermineScriptTypeForElement -> return ScriptTYpe https://codereview.chromium.org/2821803002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ScriptLoader.h (right): https://codereview.chromium.org/2821803002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ScriptLoader.h:173: ScriptType script_type_ = ScriptType::kClassic; Should we have "invalid" ScriptType so that we can ensure that script_type_ was determined somewhere in the pipeline?
or lgtm this as an intermediate step, given that we have https://codereview.chromium.org/2824583002/ depend on this
I also considered adding kInvalid to ScriptType. Drawback is we have to more careful about error checking when acccessing ScriptType (probably using switch()?); Yes, it's better to check ScriptLoader::script_type_ more strictly, but how about e.g. Script::GetScriptType(), which inherently cannot return kInvalid (because we only have ClassicScript and ModuleScript)? Even in such case I think we have to consider kInvalid for completeness. As for just checking script_type_ is set when accessed, we can use e.g. a separate bool is_script_type_set_. WDYT? (I start thinking of adding kInvalid as both two of us consider the sme option, and will draft a CL later)
On 2017/04/15 01:42:39, hiroshige wrote:
> I also considered adding kInvalid to ScriptType.
> Drawback is we have to more careful about error checking when acccessing
> ScriptType (probably using switch()?); Yes, it's better to check
> ScriptLoader::script_type_ more strictly, but how about e.g.
> Script::GetScriptType(), which inherently cannot return kInvalid (because we
> only have ClassicScript and ModuleScript)? Even in such case I think we have
to
> consider kInvalid for completeness.
I prefer to have strict error checking sometime in future. However as I said in
#7, I don't think strict type checking is P{0,1}. This CL is advancing the code
in the right direction, so feel free to land this as is.
> As for just checking script_type_ is set when accessed, we can use e.g. a
> separate bool is_script_type_set_.
I'm not sure if is_script_type_set_ is a good idea
> WDYT?
> (I start thinking of adding kInvalid as both two of us consider the sme
option,
> and will draft a CL later)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2820703003 Patch 140001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
On 2017/04/15 01:50:58, kouhei wrote:
> On 2017/04/15 01:42:39, hiroshige wrote:
> > I also considered adding kInvalid to ScriptType.
> > Drawback is we have to more careful about error checking when acccessing
> > ScriptType (probably using switch()?); Yes, it's better to check
> > ScriptLoader::script_type_ more strictly, but how about e.g.
> > Script::GetScriptType(), which inherently cannot return kInvalid (because we
> > only have ClassicScript and ModuleScript)? Even in such case I think we have
> to
> > consider kInvalid for completeness.
> I prefer to have strict error checking sometime in future. However as I said
in
> #7, I don't think strict type checking is P{0,1}. This CL is advancing the
code
> in the right direction, so feel free to land this as is.
>
> > As for just checking script_type_ is set when accessed, we can use e.g. a
> > separate bool is_script_type_set_.
> I'm not sure if is_script_type_set_ is a good idea
>
> > WDYT?
> > (I start thinking of adding kInvalid as both two of us consider the sme
> option,
> > and will draft a CL later)
Committing this as is. I'll revisit adding kInvalid.
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2821803002/#ps20001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2821803002/#ps40001 (title: "Add missing include")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492556594706070,
"parent_rev": "ab84818703b820d9596a9637a91f73e64096f5b8", "commit_rev":
"08697f894248cda25e67bd5c82565ffbc33f21fa"}
Message was sent while issue was closed.
Description was changed from ========== Introduce ScriptLoader::script_type_ This CL introduces the logic for determining the script type to ScriptLoader. script_type_ will be used by subsequent CLs. This CL also makes Step 6 of ScriptLoader::PrepareScript() (IsValidScriptTypeAndLanguage()) to correspond directly to the spec. This CL doesn't change the behavior. BUG=594639, 686281 ========== to ========== Introduce ScriptLoader::script_type_ This CL introduces the logic for determining the script type to ScriptLoader. script_type_ will be used by subsequent CLs. This CL also makes Step 6 of ScriptLoader::PrepareScript() (IsValidScriptTypeAndLanguage()) to correspond directly to the spec. This CL doesn't change the behavior. BUG=594639, 686281 Review-Url: https://codereview.chromium.org/2821803002 Cr-Commit-Position: refs/heads/master@{#465443} Committed: https://chromium.googlesource.com/chromium/src/+/08697f894248cda25e67bd5c8256... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/08697f894248cda25e67bd5c8256... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
