|
|
Created:
3 years, 11 months ago by kouhei (in TOK) Modified:
3 years, 11 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ES6 modules] Implement https://html.spec.whatwg.org/#resolve-a-module-specifier
BUG=594639
Review-Url: https://codereview.chromium.org/2640723002
Cr-Commit-Position: refs/heads/master@{#444684}
Committed: https://chromium.googlesource.com/chromium/src/+/605bc2f64983eba6b1f3d7a6b6e27ae72c9356c2
Patch Set 1 #Patch Set 2 : set upstream #Patch Set 3 : include #
Total comments: 4
Patch Set 4 : absoluteURL #
Total comments: 6
Patch Set 5 : yhirano review2 #
Messages
Total messages: 32 (21 generated)
kouhei@chromium.org changed reviewers: + domenic@chromium.org, dominicc@chromium.org, yhirano@chromium.org
The CQ bit was checked by kouhei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kouhei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL
Spec implementation and tests lgtm; I'll let others comment on the KURL usage (e.g. is it idiomatic to use KURL() to represent failure, and whether isValid() is an accurate representation of "could parse without failure"). https://codereview.chromium.org/2640723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModulatorTest.cpp (right): https://codereview.chromium.org/2640723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModulatorTest.cpp:32: EXPECT_TRUE( Do you want to test the serialized form for more than just the pears.mjs example, maybe?
https://codereview.chromium.org/2640723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Modulator.cpp (right): https://codereview.chromium.org/2640723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Modulator.cpp:38: KURL relativeURL(baseURL, moduleRequest); Should this be named as |absoluteURL|? According to https://html.spec.whatwg.org/#resolve-a-module-specifier , it "will return either an absolute URL or failure".
PTAL https://codereview.chromium.org/2640723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Modulator.cpp (right): https://codereview.chromium.org/2640723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Modulator.cpp:38: KURL relativeURL(baseURL, moduleRequest); On 2017/01/19 02:05:44, yhirano wrote: > Should this be named as |absoluteURL|? According to > https://html.spec.whatwg.org/#resolve-a-module-specifier , it "will return > either an absolute URL or failure". Done. https://codereview.chromium.org/2640723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModulatorTest.cpp (right): https://codereview.chromium.org/2640723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModulatorTest.cpp:32: EXPECT_TRUE( On 2017/01/18 19:04:48, domenic wrote: > Do you want to test the serialized form for more than just the pears.mjs > example, maybe? I think we should defer those to the KURLTest.
The CQ bit was checked by kouhei@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2640723002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModulatorTest.cpp (right): https://codereview.chromium.org/2640723002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModulatorTest.cpp:26: EXPECT_STREQ("http://example.com/pears.mjs", You can compare strings, e.g, EXPECT_EQ("http://example.com/pears.mjs", resolved.elidedString()); https://codereview.chromium.org/2640723002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModulatorTest.cpp:27: resolved.elidedString().utf8().data()); Should we use getString() instead? https://codereview.chromium.org/2640723002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModulatorTest.cpp:67: Modulator::resolveModuleSpecifier("pumpkins.js", KURL()).isValid()); Can we have a case for having a valid base url while NOT having "/", "./", "../" prefixes?
The CQ bit was checked by kouhei@chromium.org to run a CQ dry run
https://codereview.chromium.org/2640723002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ModulatorTest.cpp (right): https://codereview.chromium.org/2640723002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModulatorTest.cpp:26: EXPECT_STREQ("http://example.com/pears.mjs", On 2017/01/19 03:53:53, yhirano wrote: > You can compare strings, e.g, EXPECT_EQ("http://example.com/pears.mjs", > resolved.elidedString()); Done. https://codereview.chromium.org/2640723002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModulatorTest.cpp:27: resolved.elidedString().utf8().data()); On 2017/01/19 03:53:53, yhirano wrote: > Should we use getString() instead? Done. https://codereview.chromium.org/2640723002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ModulatorTest.cpp:67: Modulator::resolveModuleSpecifier("pumpkins.js", KURL()).isValid()); On 2017/01/19 03:53:53, yhirano wrote: > Can we have a case for having a valid base url while NOT having "/", "./", "../" > prefixes? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm
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 kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from domenic@chromium.org Link to the patchset: https://codereview.chromium.org/2640723002/#ps80001 (title: "yhirano review2")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484815608366590, "parent_rev": "e13f57fbda95f7260cdfb7c7610fcdeeb36e1991", "commit_rev": "605bc2f64983eba6b1f3d7a6b6e27ae72c9356c2"}
Message was sent while issue was closed.
Description was changed from ========== [ES6 modules] Implement https://html.spec.whatwg.org/#resolve-a-module-specifier BUG=594639 ========== to ========== [ES6 modules] Implement https://html.spec.whatwg.org/#resolve-a-module-specifier BUG=594639 Review-Url: https://codereview.chromium.org/2640723002 Cr-Commit-Position: refs/heads/master@{#444684} Committed: https://chromium.googlesource.com/chromium/src/+/605bc2f64983eba6b1f3d7a6b6e2... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/605bc2f64983eba6b1f3d7a6b6e2... |