#9641 closed task (fixed)
Split dojo/_base/html.js
Reported by: | Eugene Lazutkin | Owned by: | Eugene Lazutkin |
---|---|---|---|
Priority: | low | Milestone: | 1.7 |
Component: | HTML | Version: | 1.3.2 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
Split into two parts:
- node geometry (coords, marginBox, contentBox)
- the rest (attr, style, class)
By splitting it we can separate the unrelated functionality, simplify our lives (e.g., easier to track what exactly was changed), and provide a way for custom builds that do not want to include one of them.
buildUtil.baseMappings
in util/buildscripts/buildUtil.js
should be updated to reflect the change.
Attachments (3)
Change History (68)
comment:1 Changed 11 years ago by
Milestone: | tbd → future |
---|---|
Owner: | changed from sjmiles to Eugene Lazutkin |
Status: | new → assigned |
comment:2 Changed 10 years ago by
comment:3 Changed 10 years ago by
Milestone: | future → 1.7 |
---|
comment:4 Changed 10 years ago by
Currently, there are a few references to win.doc()
in dom-geometry.js. I'm attaching a patch to fix that.
Changed 10 years ago by
Attachment: | fix-dom-geometry.diff added |
---|
comment:5 Changed 10 years ago by
comment:7 Changed 10 years ago by
comment:8 Changed 10 years ago by
comment:9 follow-up: 10 Changed 10 years ago by
I checked over the changes. They look good. A few things I noticed:
- I'm getting a failure in html_rtl test (scrollingQuirksIframeVertSmall), not sure if it was there before or not. This is on FF4/win.
- you removed the
node = byId(node)
from replaceClass(), won't that break when user passes in an id? - in dojo._fixIeBiDiScrollLeft() you removed a checked for IE6, not sure why or if the code still works in that case (it seems suspicious though)
- in dojo.attr() that connect()/disconnect() calls should be replaced by calls to on(), but that's outside the scope of your changes
comment:10 Changed 10 years ago by
Replying to bill:
I checked over the changes. They look good. A few things I noticed:
- I'm getting a failure in html_rtl test (scrollingQuirksIframeVertSmall), not sure if it was there before or not. This is on FF4/win.
I saw it too on FF4/linux today, but not on Chrome. I actually tried to debug it, and the test is quite convoluted with two embedded timers --- I couldn't figure out what it supposed to do, so I have no idea if it is a bug in our scroller code, a bug in the test, or a bug in FF4.
- you removed the
node = byId(node)
from replaceClass(), won't that break when user passes in an id?
That's my omission of the recent change. Somehow I missed it when I was merging. Will fix.
- in dojo._fixIeBiDiScrollLeft() you removed a checked for IE6, not sure why or if the code still works in that case (it seems suspicious though)
That was not my intention. Let me re-check and put the missing piece back.
- in dojo.attr() that connect()/disconnect() calls should be replaced by calls to on(), but that's outside the scope of your changes
That was intentional. My first goal was to make the split work, and there are some things I can do to improve the code:
- Refactor module return values (right now
dojo
is returned). - Use
has()
instead of sniffing if possible. In any case thesniff
module duplicates all its variable ashas
checks, so we should usehas
. - Update other core modules to reference new modules directly, so we can build code automatically with minimal dependencies.
Changed 10 years ago by
Attachment: | dom-class.diff added |
---|
Patch to convert dom-class functions to local, with explicit export, and short names.
comment:11 Changed 10 years ago by
comment:12 Changed 10 years ago by
Changed 10 years ago by
Attachment: | dom-class2.diff added |
---|
Patch to convert dom-class functions to override-able module functions with short names. Added wrappers in dojo/_base/html.js.
comment:13 Changed 10 years ago by
comment:14 Changed 10 years ago by
comment:15 Changed 10 years ago by
comment:16 Changed 10 years ago by
comment:17 Changed 10 years ago by
comment:19 Changed 10 years ago by
comment:20 Changed 10 years ago by
comment:30 Changed 10 years ago by
comment:31 follow-up: 34 Changed 10 years ago by
(In [25721]) Fix more breakage from [25704]. In 1.6 dojo.marginBox() etc. as a setter doesn't return a value. After [25704] it returns the node. This breaks dijit's layoutChildren() method which mixes in the return value to the widget (in the private resize() method).
It would perhaps be better if dojo.marginBox() etc. worked according to it's spec, which says: Returns an object in the expected format of box (regardless if box is passed).
Refs #9641 !strict.
comment:32 Changed 10 years ago by
comment:33 Changed 10 years ago by
comment:34 Changed 10 years ago by
Replying to bill:
It would perhaps be better if dojo.marginBox() etc. worked according to it's spec, which says: Returns an object in the expected format of box (regardless if box is passed).
The only sure proof way to do that is to call dojo.getMarginBox()
right after dojo.setMarginBox()
. I suspect that it'll be very slow, because the latter triggers re-rendering, and the former will wait for it to finish. In many cases you don't want to wait, so it'll add an unnecessary delay.
The same goes for dojo.contentBox()
.
I suggest to change the doc rather than the other way around.
comment:35 Changed 10 years ago by
comment:36 Changed 10 years ago by
comment:37 Changed 10 years ago by
comment:38 follow-up: 39 Changed 10 years ago by
comment:39 Changed 10 years ago by
Replying to elazutkin:
It looks like robot missed [25901]: dom: all DOM modules are made "baseless", and html.js was corrected to provide the same API as before, expect all direct users of dom modules to be broken, !strict, refs #9641.
in ie, this definition of has
(http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/dom-attr.js?rev=25901#L176) shadows this definition of has
(http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/dom-attr.js?rev=25901#L2) which causes this line to fail http://bugs.dojotoolkit.org/browser/dojo/dojo/trunk/dom-attr.js?rev=25901#L152 since its calling the has
on L176.
comment:40 Changed 10 years ago by
a small case to demo what i'm describing in the above comment:
(function (has) { has('abc'); var attr = { has : function has() { console.log('inner'); } }; })(function () { console.log('outer'); });
comment:41 follow-up: 43 Changed 10 years ago by
Not only will that definition of has mask the other, this module is using named function expressions which cause leaks in IE (http://kangax.github.com/nfe/). We either need to null the leaked references, use function declarations and then assign those functions to the object, or use plain function expressions with no names (I vote for using function declarations).
comment:42 Changed 10 years ago by
comment:43 Changed 10 years ago by
Replying to BryanForbes:
Not only will that definition of has mask the other, this module is using named function expressions which cause leaks in IE (http://kangax.github.com/nfe/). We either need to null the leaked references, use function declarations and then assign those functions to the object, or use plain function expressions with no names (I vote for using function declarations).
Could you provide more information?
var f = function g(){};
AFAIK, memory leaks on IE when both names are used (f
and g
) anywhere. In this case g
is never used.
Another thing is in order to leak a memory in any way you have to create an opportunity for it to be reclaimed. Irreclaimable memory with no external references is called a memory leak. In the case of modules in order to create a leak we have to either load the same module repeatedly losing references to old instances, or unload them in any other way. AFAIK, there are no provisions for that => all modules and their functions are in memory since the time they were loaded irrespective of how many references they have.
What did I miss?
comment:44 follow-up: 45 Changed 10 years ago by
From my reading of the article, this won't cause any memory leaks on IE, although it might be different if you had different function definitions depending on the browser, like:
marginBox: dojo.isIE ? function ieMarginBox(){ ... } : function otherMarginBox(){ ... };
But the code doesn't have that, AFAICT.
However, adding the function names does bloat the size of the files. FWIW, I updated my spreadsheet, and the HTML code (after shrinksafe) is currently running at 20.9K, compared to 14.5K in 1.6. Of course that's due to much more than the unnecessary function names.
comment:45 Changed 10 years ago by
Replying to bill:
However, adding the function names does bloat the size of the files.
My hope was that gzip will take care of extra names.
The reason for naming functions was to help with debugging this stuff. Apparently it helps to generate correct stack traces, and aids in manual debugging as well by providing a name on a function object. Not on IE though.
comment:46 Changed 10 years ago by
Sure, I understand the debugging value, although I think the loader/builder might have an option to insert function names?
Anyway, here's the size w/function names after gzip:
mac:dojo bill$ wc dom*gz 4 31 885 dom-attr.js.gz 1 32 884 dom-class.js.gz 7 46 1394 dom-construct.js.gz 3 29 880 dom-form.js.gz 8 58 2331 dom-geometry.js.gz 4 22 846 dom-prop.js.gz 9 50 1351 dom-style.js.gz 0 22 772 dom.js.gz 4 27 775 domReady.js.gz 40 317 10118 total
And without function names:
mac:dojo bill$ wc dom*gz 1 29 866 dom-attr.js.gz 2 27 860 dom-class.js.gz 7 46 1391 dom-construct.js.gz 3 27 867 dom-form.js.gz 9 73 2285 dom-geometry.js.gz 2 32 836 dom-prop.js.gz 7 35 1344 dom-style.js.gz 0 23 772 dom.js.gz 4 27 775 domReady.js.gz 35 319 9996 total
I guess the difference is pretty minor.
comment:47 Changed 10 years ago by
A named function expression has the following pattern (as Eugene pointed out):
var f = function foo(){};
I don't want to rewrite the whole article here, but here's a good explanation of what should happen:
var f = function foo(){ return typeof foo; // "foo" is available in this inner scope }; // `foo` is never visible "outside" typeof foo; // "undefined" f(); // "function"
And what happens in IE:
var f = function foo(){ return typeof foo; // "foo" is available in this inner scope }; // `foo` is visible "outside" typeof foo; // "function" f(); // "function"
Not only does the identifier "leak" into the enclosing scope, but two distinct functions are actually created here:
var f = function foo(){}; f === foo; // false in IE
The two functions also reference each other by closure which is a circular reference and we all know how well IE6 and IE7 deal with circular references through a closure (they don't). Explicitly declaring var foo
and nulling it solves the problem and allows IE to reclaim the memory:
var f = (function(){ var f = function foo(){}; var foo = null; return f; })();
This doesn't happen just when doing an if statement or using a ?:, this happens whenever you use a named function expression in IE and don't clean up after it. This also happens when you load a module once (no repeated loading needed). And I'd like to point out that yes, all modules and their functions are in memory, but if IE is creating extra functions that have no way to be referenced and can't be collected until a page refresh, we are leaking in single page apps.
comment:48 Changed 9 years ago by
See also #13352 as an alternate way to get those symbols for debugging. Only for built code though, so maybe not so useful.
comment:50 Changed 9 years ago by
check in 26158 breaks preview.php in util/docscripts. Please have a check whether we need to fix a regression or fire another ticket to fix preview.php.
comment:62 Changed 9 years ago by
Milestone: | 1.8 → 1.7 |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
Type: | enhancement → task |
Possible names suggested during the RFC process:
Nobody proposed to split into more than two files.