Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
757963002:
[Blink-in-JS] 'Import' function to load sub-modules and other resources for private scripts (Part-3) (Closed)
|
Created:
6 years ago by vivekg_samsung Modified:
6 years ago CC:
arv+blink, blink-reviews, blink-reviews-bindings_chromium.org, dglazkov+blink, jbroman, yosin_UTC9 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
Description[Blink-in-JS] 'Import' function to load sub-modules and other resources for private scripts (Part-3)
This CL introduces 'Import' function to load sub-modules for the private scripts.
This helps to break down larger script modules in manageable script chunks.
Part 1: https://codereview.chromium.org/755883004
Part 2: https://codereview.chromium.org/759553002
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186384
Patch Set 1 #
Total comments: 5
Patch Set 2 : More concrete example with CSS resources! #Patch Set 3 : Patch for landing! #
Total comments: 6
Patch Set 4 : Modified to return CSS value as text #Patch Set 5 : Implementing privateScriptController.import #
Messages
Total messages: 34 (6 generated)
vivekg@chromium.org changed reviewers: + vivekg@chromium.org
@haraken, sorry for the delay. Here is the WIP patch for the 'include' function. Please take a look and let me know your comments.
https://codereview.chromium.org/757963002/diff/1/Source/bindings/core/v8/Priv... File Source/bindings/core/v8/PrivateScriptRunner.cpp (right): https://codereview.chromium.org/757963002/diff/1/Source/bindings/core/v8/Priv... Source/bindings/core/v8/PrivateScriptRunner.cpp:78: compileAndRunPrivateScript(args.GetIsolate(), scriptFileName, resourceData.utf8().data(), resourceData.length()); Calling the compileAndRunPrivateScript enables the authors to call 'include' in cascaded order. e.g. DocumentXMLTreeViewer.js ---includes---> A.js ---includes---> B.js ... https://codereview.chromium.org/757963002/diff/1/Source/core/xml/DocumentXMLT... File Source/core/xml/DocumentXMLTreeViewer.js (right): https://codereview.chromium.org/757963002/diff/1/Source/core/xml/DocumentXMLT... Source/core/xml/DocumentXMLTreeViewer.js:7: include("A.js"); Legitimate include file which is present in the pak file. https://codereview.chromium.org/757963002/diff/1/Source/core/xml/DocumentXMLT... Source/core/xml/DocumentXMLTreeViewer.js:8: include("B.js"); This is an example of invalid include 'B.js'. In this case, we would throw an error and crash the renderer. https://codereview.chromium.org/757963002/diff/1/Source/core/xml/DocumentXMLT... Source/core/xml/DocumentXMLTreeViewer.js:12: aJSFunction("Calling aJSFunction"); Example of calling the function defined in the A.js from DocumentXMLTreeViewer.js https://codereview.chromium.org/757963002/diff/1/Source/core/xml/DocumentXMLT... Source/core/xml/DocumentXMLTreeViewer.js:12: aJSFunction("Calling aJSFunction"); Example of calling the function defined in the A.js from DocumentXMLTreeViewer.js
vivekg@chromium.org changed reviewers: + jbroman@chromium.org
vivekg@chromium.org changed reviewers: + yosin@chromium.org
The approach looks great to me :)
Can you remove A.js and B.js from the CL?
On 2014/11/25 at 09:27:42, haraken wrote: > Can you remove A.js and B.js from the CL? Yes I am modifying the code and will upload a new patch soon.
On 2014/11/25 at 09:27:42, haraken wrote: > Can you remove A.js and B.js from the CL? Done, PTAL. Thanks!
vivekg@chromium.org changed reviewers: + jochen@chromium.org - jbroman@chromium.org, yosin@chromium.org
I will also add some Testing code from PrivateScriptTest.js to verify the importing of js files.
https://codereview.chromium.org/757963002/diff/40001/Source/bindings/core/v8/... File Source/bindings/core/v8/PrivateScriptRunner.cpp (right): https://codereview.chromium.org/757963002/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/PrivateScriptRunner.cpp:55: if (isUndefinedOrNull(functionImportObject)) RELEASE_ASSERT(functionImportObject.IsEmpty() || functionImportObject->IsFunction()); if (functionImportObject.IsEmpty()) global->Set(...); https://codereview.chromium.org/757963002/diff/40001/Source/core/xml/Document... File Source/core/xml/DocumentXMLTreeViewer.js (right): https://codereview.chromium.org/757963002/diff/40001/Source/core/xml/Document... Source/core/xml/DocumentXMLTreeViewer.js:7: Import("DocumentXMLTreeViewer.css"); Import => import ? https://codereview.chromium.org/757963002/diff/40001/Source/core/xml/Document... Source/core/xml/DocumentXMLTreeViewer.js:7: Import("DocumentXMLTreeViewer.css"); Sorry for another idea: Would it be possible to allow a developer to write this as follows? var cssDocumentXMLTreeViewer = import("DocumentXMLTreeViewer.css"); We don't want to pollute window objects with cssXXX or YYY implicitly.
jochen@chromium.org changed reviewers: + adamk@chromium.org
+adamk fyi would it make sense to have try to have something that is closer to ES6 module loaders or similar?
On 2014/11/25 at 09:45:04, jochen wrote: > +adamk fyi > > would it make sense to have try to have something that is closer to ES6 module loaders or similar? I was referring to http://wiki.ecmascript.org/doku.php?id=harmony:modules for hints about implementing the 'import'. Also 'import' is a reserved keyword, hence renamed the function to 'Import' to reflect the same.
https://codereview.chromium.org/757963002/diff/40001/Source/bindings/core/v8/... File Source/bindings/core/v8/PrivateScriptRunner.cpp (right): https://codereview.chromium.org/757963002/diff/40001/Source/bindings/core/v8/... Source/bindings/core/v8/PrivateScriptRunner.cpp:55: if (isUndefinedOrNull(functionImportObject)) On 2014/11/25 at 09:41:41, haraken wrote: > RELEASE_ASSERT(functionImportObject.IsEmpty() || functionImportObject->IsFunction()); > if (functionImportObject.IsEmpty()) > global->Set(...); If I change to RELEASE_ASSERT(functionImportObject.IsEmpty() || functionImportObject->IsFunction()); the code is asserting immediately for the first time for functionImportObject.IsEmpty() being false. Does IsEmpty() return "True" in such case as I am seeing the result otherwise? https://codereview.chromium.org/757963002/diff/40001/Source/core/xml/Document... File Source/core/xml/DocumentXMLTreeViewer.js (right): https://codereview.chromium.org/757963002/diff/40001/Source/core/xml/Document... Source/core/xml/DocumentXMLTreeViewer.js:7: Import("DocumentXMLTreeViewer.css"); On 2014/11/25 at 09:41:41, haraken wrote: > Import => import ? I tried 'import' but this is a reserved keyword for ECMA 6.x. Hence ended up making 'Import' to look analogous.
https://codereview.chromium.org/757963002/diff/40001/Source/core/xml/Document... File Source/core/xml/DocumentXMLTreeViewer.js (right): https://codereview.chromium.org/757963002/diff/40001/Source/core/xml/Document... Source/core/xml/DocumentXMLTreeViewer.js:7: Import("DocumentXMLTreeViewer.css"); On 2014/11/25 at 09:41:41, haraken wrote: > Sorry for another idea: Would it be possible to allow a developer to write this as follows? > > var cssDocumentXMLTreeViewer = import("DocumentXMLTreeViewer.css"); > > We don't want to pollute window objects with cssXXX or YYY implicitly. Ya sure, let me try to get it this way which looks more cleaner. Thanks!
Uploaded a new patch, ptal!
Looks good, but let's wait for adamk's comment. Also let's consider moving 'Import', 'installClass', 'throwException' etc to PrivateScriptRunner's methods in a follow-up CL in order not to pollute window's variables.
On 2014/11/25 at 12:20:28, haraken wrote: > Looks good, but let's wait for adamk's comment. > > Also let's consider moving 'Import', 'installClass', 'throwException' etc to PrivateScriptRunner's methods in a follow-up CL in order not to pollute window's variables. ya sure. One question: How about having "$import" instead of "Import"?
On 2014/11/25 12:34:48, vivekg_ wrote: > On 2014/11/25 at 12:20:28, haraken wrote: > > Looks good, but let's wait for adamk's comment. > > > > Also let's consider moving 'Import', 'installClass', 'throwException' etc to > PrivateScriptRunner's methods in a follow-up CL in order not to pollute window's > variables. > > ya sure. One question: How about having "$import" instead of "Import"? I guess "PrivateScriptRunner.import" would be less controversial but I don't have a strong opinion here.
On 2014/11/25 at 12:37:14, haraken wrote: > On 2014/11/25 12:34:48, vivekg_ wrote: > > On 2014/11/25 at 12:20:28, haraken wrote: > > > Looks good, but let's wait for adamk's comment. > > > > > > Also let's consider moving 'Import', 'installClass', 'throwException' etc to > > PrivateScriptRunner's methods in a follow-up CL in order not to pollute window's > > variables. > > > > ya sure. One question: How about having "$import" instead of "Import"? > > I guess "PrivateScriptRunner.import" would be less controversial but I don't have a strong opinion here. import keyword is reserved and when used, we get this error: "SyntaxError: Unexpected reserved word"
On 2014/11/25 12:38:11, vivekg_ wrote: > On 2014/11/25 at 12:37:14, haraken wrote: > > On 2014/11/25 12:34:48, vivekg_ wrote: > > > On 2014/11/25 at 12:20:28, haraken wrote: > > > > Looks good, but let's wait for adamk's comment. > > > > > > > > Also let's consider moving 'Import', 'installClass', 'throwException' etc > to > > > PrivateScriptRunner's methods in a follow-up CL in order not to pollute > window's > > > variables. > > > > > > ya sure. One question: How about having "$import" instead of "Import"? > > > > I guess "PrivateScriptRunner.import" would be less controversial but I don't > have a strong opinion here. > > import keyword is reserved and when used, we get this error: "SyntaxError: > Unexpected reserved word" Even if it is prefixed with "PrivateScriptRunner."? For example, 'for' is a reserved keyword but window.for is a valid property access.
On 2014/11/25 at 12:45:17, haraken wrote: > On 2014/11/25 12:38:11, vivekg_ wrote: > > On 2014/11/25 at 12:37:14, haraken wrote: > > > On 2014/11/25 12:34:48, vivekg_ wrote: > > > > On 2014/11/25 at 12:20:28, haraken wrote: > > > > > Looks good, but let's wait for adamk's comment. > > > > > > > > > > Also let's consider moving 'Import', 'installClass', 'throwException' etc > > to > > > > PrivateScriptRunner's methods in a follow-up CL in order not to pollute > > window's > > > > variables. > > > > > > > > ya sure. One question: How about having "$import" instead of "Import"? > > > > > > I guess "PrivateScriptRunner.import" would be less controversial but I don't > > have a strong opinion here. > > > > import keyword is reserved and when used, we get this error: "SyntaxError: > > Unexpected reserved word" > > Even if it is prefixed with "PrivateScriptRunner."? For example, 'for' is a reserved keyword but window.for is a valid property access. My bad, yes you are right! It works perfectly.
On 2014/11/25 at 12:47:16, vivekg_ wrote: > On 2014/11/25 at 12:45:17, haraken wrote: > > On 2014/11/25 12:38:11, vivekg_ wrote: > > > On 2014/11/25 at 12:37:14, haraken wrote: > > > > On 2014/11/25 12:34:48, vivekg_ wrote: > > > > > On 2014/11/25 at 12:20:28, haraken wrote: > > > > > > Looks good, but let's wait for adamk's comment. > > > > > > > > > > > > Also let's consider moving 'Import', 'installClass', 'throwException' etc > > > to > > > > > PrivateScriptRunner's methods in a follow-up CL in order not to pollute > > > window's > > > > > variables. > > > > > > > > > > ya sure. One question: How about having "$import" instead of "Import"? > > > > > > > > I guess "PrivateScriptRunner.import" would be less controversial but I don't > > > have a strong opinion here. > > > > > > import keyword is reserved and when used, we get this error: "SyntaxError: > > > Unexpected reserved word" > > > > Even if it is prefixed with "PrivateScriptRunner."? For example, 'for' is a reserved keyword but window.for is a valid property access. > > My bad, yes you are right! It works perfectly. So in that case, what do you suggest? Should I move these methods to the PrivateScriptRunner first and then introduce this API on top of it?
On 2014/11/25 12:48:04, vivekg_ wrote: > On 2014/11/25 at 12:47:16, vivekg_ wrote: > > On 2014/11/25 at 12:45:17, haraken wrote: > > > On 2014/11/25 12:38:11, vivekg_ wrote: > > > > On 2014/11/25 at 12:37:14, haraken wrote: > > > > > On 2014/11/25 12:34:48, vivekg_ wrote: > > > > > > On 2014/11/25 at 12:20:28, haraken wrote: > > > > > > > Looks good, but let's wait for adamk's comment. > > > > > > > > > > > > > > Also let's consider moving 'Import', 'installClass', > 'throwException' etc > > > > to > > > > > > PrivateScriptRunner's methods in a follow-up CL in order not to > pollute > > > > window's > > > > > > variables. > > > > > > > > > > > > ya sure. One question: How about having "$import" instead of "Import"? > > > > > > > > > > I guess "PrivateScriptRunner.import" would be less controversial but I > don't > > > > have a strong opinion here. > > > > > > > > import keyword is reserved and when used, we get this error: "SyntaxError: > > > > Unexpected reserved word" > > > > > > Even if it is prefixed with "PrivateScriptRunner."? For example, 'for' is a > reserved keyword but window.for is a valid property access. > > > > My bad, yes you are right! It works perfectly. > > So in that case, what do you suggest? Should I move these methods to the > PrivateScriptRunner first and then introduce this API on top of it? That sounds better, but it would be even better to wait for a reply from adamk about the 'import' statement.
On 2014/11/25 09:45:04, jochen (slow) wrote: > +adamk fyi > > would it make sense to have try to have something that is closer to ES6 module > loaders or similar? Well, module loaders are in a bit of limbo right now. What's not in limbo is the "import" statement itself, but ES6 will not define what to do with the string that names the module (this will be defined outside of ECMAScript, currently in draft at https://whatwg.github.io/loader/). So if you need to start loading scripts now, I wouldn't worry too much about ES6 modules. Not sure what haraken would like me to address specifically, but my high-level comments on this patch are 1) It looks like you're not making use of the ability to load scripts via this mechanism (you're only loading CSS); any reason to add that until it's used? The CSS-loading stuff seems like a much simpler concept. 2) Rather than inventing your own semantics for "Import", I'd consider looking at existing module systems, such as that used by Node.js. This is related to (1), in that I'd expect the patch introducing "Import" to show a canonical way of creating modules. The biggest thing missing from the system you've created here is it's just evaling the loaded script, rather than treating it as a separate module. Compare to Node.js (http://nodejs.org/api/modules.html), where modules explicitly list their exports.
On 2014/11/25 at 18:29:33, adamk wrote: > On 2014/11/25 09:45:04, jochen (slow) wrote: > > +adamk fyi > > > > would it make sense to have try to have something that is closer to ES6 module > > loaders or similar? > > Well, module loaders are in a bit of limbo right now. What's not in limbo is the "import" statement itself, but ES6 will not define what to do with the string that names the module (this will be defined outside of ECMAScript, currently in draft at https://whatwg.github.io/loader/). So if you need to start loading scripts now, I wouldn't worry too much about ES6 modules. > > Not sure what haraken would like me to address specifically, but my high-level comments on this patch are > > 1) It looks like you're not making use of the ability to load scripts via this mechanism (you're only loading CSS); any reason to add that until it's used? The CSS-loading stuff seems like a much simpler concept. > > 2) Rather than inventing your own semantics for "Import", I'd consider looking at existing module systems, such as that used by Node.js. This is related to (1), in that I'd expect the patch introducing "Import" to show a canonical way of creating modules. The biggest thing missing from the system you've created here is it's just evaling the loaded script, rather than treating it as a separate module. Compare to Node.js (http://nodejs.org/api/modules.html), where modules explicitly list their exports. Thank you @adamk for the references and the detailed explanation. @haraken, please suggest the way forward on this.
On 2014/11/26 09:24:21, vivekg_ wrote: > On 2014/11/25 at 18:29:33, adamk wrote: > > On 2014/11/25 09:45:04, jochen (slow) wrote: > > > +adamk fyi > > > > > > would it make sense to have try to have something that is closer to ES6 > module > > > loaders or similar? > > > > Well, module loaders are in a bit of limbo right now. What's not in limbo is > the "import" statement itself, but ES6 will not define what to do with the > string that names the module (this will be defined outside of ECMAScript, > currently in draft at https://whatwg.github.io/loader/). So if you need to start > loading scripts now, I wouldn't worry too much about ES6 modules. > > > > Not sure what haraken would like me to address specifically, but my high-level > comments on this patch are > > > > 1) It looks like you're not making use of the ability to load scripts via this > mechanism (you're only loading CSS); any reason to add that until it's used? The > CSS-loading stuff seems like a much simpler concept. > > > > 2) Rather than inventing your own semantics for "Import", I'd consider looking > at existing module systems, such as that used by Node.js. This is related to > (1), in that I'd expect the patch introducing "Import" to show a canonical way > of creating modules. The biggest thing missing from the system you've created > here is it's just evaling the loaded script, rather than treating it as a > separate module. Compare to Node.js (http://nodejs.org/api/modules.html), where > modules explicitly list their exports. > > Thank you @adamk for the references and the detailed explanation. > > @haraken, please suggest the way forward on this. As Adamk suggested, let's land the css part first. We can handle the js part later :)
On 2014/11/26 at 10:56:33, haraken wrote: > > As Adamk suggested, let's land the css part first. We can handle the js part later :) Done, PTAL. Thanks!
LGTM
The CQ bit was checked by vivekg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/757963002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://src.chromium.org/viewvc/blink?view=rev&revision=186384 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
