Opened 10 years ago

Closed 8 years ago

Last modified 7 years ago

#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)

fix-dom-geometry.diff (1.5 KB) - added by Bryan Forbes 8 years ago.
dom-class.diff (3.6 KB) - added by Eugene Lazutkin 8 years ago.
Patch to convert dom-class functions to local, with explicit export, and short names.
dom-class2.diff (16.5 KB) - added by Bryan Forbes 8 years ago.
Patch to convert dom-class functions to override-able module functions with short names. Added wrappers in dojo/_base/html.js.

Download all attachments as: .zip

Change History (68)

comment:1 Changed 10 years ago by Eugene Lazutkin

Milestone: tbdfuture
Owner: changed from sjmiles to Eugene Lazutkin
Status: newassigned

Possible names suggested during the RFC process:

Author1st file2nd file
Bill Keesegeometry.jshtml.js
Bill Keesegeometry.jsstyle.js
James Burkebox.jshtml.js
Mike Wilcoxdom.jsstyle.js

Nobody proposed to split into more than two files.

comment:2 Changed 8 years ago by Eugene Lazutkin

(In [25225]) base: split html.js into several manageable files, !strict, refs #9641.

comment:3 Changed 8 years ago by Eugene Lazutkin

Milestone: future1.7

comment:4 Changed 8 years ago by Bryan Forbes

Currently, there are a few references to win.doc() in dom-geometry.js. I'm attaching a patch to fix that.

Changed 8 years ago by Bryan Forbes

Attachment: fix-dom-geometry.diff added

comment:5 Changed 8 years ago by Rawld Gill

(In [25239]) fixed dependency vector and other typos (that caused failure in IE); refs #9641

comment:6 Changed 8 years ago by Eugene Lazutkin

Note: Rawld took care of Bryan's patch in [25239].

comment:7 Changed 8 years ago by Eugene Lazutkin

(In [25242]) html: added a reference to a sniff package for consistency; need to move it later to has, !strict, refs #9641.

comment:8 Changed 8 years ago by bill

(In [25243]) spacing fixes, missing semicolons, removed unused variables, fixed a few comments, refs #9641 !strict

comment:9 Changed 8 years ago by 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.
  • 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 in reply to:  9 Changed 8 years ago by Eugene Lazutkin

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 the sniff module duplicates all its variable as has checks, so we should use has.
  • Update other core modules to reference new modules directly, so we can build code automatically with minimal dependencies.

Changed 8 years ago by Eugene Lazutkin

Attachment: dom-class.diff added

Patch to convert dom-class functions to local, with explicit export, and short names.

comment:11 Changed 8 years ago by Eugene Lazutkin

(In [25250]) html: added the mising byId() conversion to replaceClass(), minor reformatting, !strict, refs #9641.

comment:12 Changed 8 years ago by Eugene Lazutkin

(In [25251]) html: added missing improvement to _fixIeBiDiScrollLeft(), !strict, refs #9641.

Changed 8 years ago by Bryan Forbes

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 8 years ago by bill

(In [25253]) dojo._getBorderBox() was removed in html.js refactor. Use dojo.position() instead.

Also, replace unnecessary use of private dojo._getMarginSize() with dojo.position().

Refs #9641, #11622, #13107 !strict.

comment:14 Changed 8 years ago by bill

(In [25254]) Replace unnecessary use of private dojo._getMarginSize() with dojo.position(). Refs #9641, #11622, #13107 !strict.

comment:15 Changed 8 years ago by bill

(In [25271]) "global" is defined on dojo object, not as part of hash which [will be] returned by dojo/_base/window. refs #9641 !strict.

comment:16 Changed 8 years ago by bill

(In [25272]) nevermind, I can synchronize "global" on dojo object with a "global" property in the hash which [will be] returned by dojo/_base/window. refs #9641 !strict.

comment:17 Changed 8 years ago by bill

(In [25276]) forEach() is in array, not lang, refs #9641 !strict. (I did notice the comment to stop using forEach altogether, but I'll leave that to Eugene.)

comment:18 Changed 8 years ago by Eugene Lazutkin

(In [25279]) html: eliminated forEach(), !strict, refs #9641.

comment:19 Changed 8 years ago by bill

(In [25621]) Fix usages of removed functions dojo._getBorderBox() and dojo._setOpacity(), refs #9641 !strict.

comment:20 Changed 8 years ago by Eugene Lazutkin

(In [25704]) html: moved all dom-* into dom/, added classList-based xxxClass functions, added more public API to dom/geometry, !strict, refs #9641.

comment:21 Changed 8 years ago by Eugene Lazutkin

#13104 is closed by the previous commit.

comment:22 Changed 8 years ago by Eugene Lazutkin

#13105 is closed by the previous commit.

comment:23 Changed 8 years ago by Eugene Lazutkin

#13106 is closed by the previous commit.

comment:24 Changed 8 years ago by Eugene Lazutkin

#13107 is closed by the previous commit.

comment:25 Changed 8 years ago by Eugene Lazutkin

#9747 is closed by the previous commit.

comment:26 Changed 8 years ago by Eugene Lazutkin

#11465 is closed by the previous commit.

comment:27 Changed 8 years ago by bill

(In [25716]) fix breakage from [25704], refs #9641

comment:28 Changed 8 years ago by bill

(In [25717]) fix breakage from [25704], refs #9641, #11465

comment:29 Changed 8 years ago by bill

(In [25718]) fixing another breakage from [25704], refs #9641

comment:30 Changed 8 years ago by Eugene Lazutkin

(In [25720]) html: fixed typos related to margin box calculations (inspected the node itself, instead of its parent), !strict, refs #9641.

comment:31 Changed 8 years ago by bill

(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 8 years ago by Eugene Lazutkin

(In [25722]) html: typo fixes in class.js, better work with edge cases, !strict, refs #9641.

comment:33 Changed 8 years ago by Eugene Lazutkin

(In [25723]) html: swapped class implementations, !strict, refs #9641.

comment:34 in reply to:  31 Changed 8 years ago by Eugene Lazutkin

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 8 years ago by Eugene Lazutkin

Robot has missed [25745]: html: moved all dom/* modules to dom-*, removed classList path in dom-class, removed all public aliases in dom-geometry, !strict, refs #9641.

Effectively it reverts [25704] and subsequent fixes/refinements.

comment:36 Changed 8 years ago by Eugene Lazutkin

Reverted back previously closed tickets: #13104, #13105, #13106, #13107, #9747, #11465. The last two were marked as wontfix as per the decision made during the last meeting.

comment:37 Changed 8 years ago by bill

(In [25746]) dom/construct has been moved back to dom-construct; refs #9641 !strict. ps: not sure how to test this file.

comment:38 Changed 8 years ago by Eugene Lazutkin

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.

comment:39 in reply to:  38 Changed 8 years ago by ben hockey

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 8 years ago by ben hockey

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 Changed 8 years ago by Bryan Forbes

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 8 years ago by Eugene Lazutkin

Missed commit [25911]: dom: fixed some inline docs, renamed a few functions to avoid name clashes, thx neonstalwart!, !strict, refs #9641.

comment:43 in reply to:  41 Changed 8 years ago by Eugene Lazutkin

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 Changed 8 years ago by bill

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 in reply to:  44 Changed 8 years ago by Eugene Lazutkin

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 8 years ago by bill

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 8 years ago by Bryan Forbes

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 8 years ago by bill

See also #13352 as an alternate way to get those symbols for debugging. Only for built code though, so maybe not so useful.

comment:49 Changed 8 years ago by Eugene Lazutkin

In [26158]:

dom-geometry: converting set* functions to accept a hash of values compatible with get* functions, thx Bill and Adam!, !strict, refs #9641.

comment:50 Changed 8 years ago by zhangyp

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:51 Changed 8 years ago by zhangyp

BTW, I am using preview.php on chrome and firefox of Linux.

comment:52 Changed 8 years ago by Eugene Lazutkin

In [26161]:

dom-geometry: removed private aliases for set* functions, thx Bill for suggestion!, !strict, refs #9641.

comment:53 Changed 8 years ago by bill

In [26166]:

Revert dijit to using hash for setMarginBox() and setContentSize(), same as in 1.6. Actually pulled those chunks of code from before the baseless refactor. Refs #9641 !strict.

comment:54 Changed 8 years ago by bill

In [26174]:

Final setMarginBox() call. Refs #9641 !strict.

comment:55 Changed 8 years ago by Eugene Lazutkin

In [26179]:

html: providing function wrappers for destroy/empty functions to facilitate one-point AOP, thx Bill!, !strict, refs #9641.

comment:55 Changed 8 years ago by Eugene Lazutkin

In [26180]:

dom-attr: removed reference to dojo/kernel --- not needed anymore, thx Bill!, !strict, refs #9641.

comment:56 Changed 8 years ago by Eugene Lazutkin

In [26181]:

dom-construct: converted external references to internal ones (missed typos), thx Bill!, !strict, refs #9641.

comment:57 Changed 8 years ago by Eugene Lazutkin

In [26182]:

dom-prop: needs a dom-construct reference, but in doing so creates a loop, leaving it like that for now, !strict, refs #9641.

comment:58 Changed 8 years ago by bill

In [26185]:

Since app code can call domGeometry.empty() etc. directly, need to connect there instead of on dojo.empty(). Fixes Dialog_mouse.html test failures. Refs #9641, fixes #13625 !strict. See also [26107] which enables a single on() call to monitor both dojo.empty() and domConstruct.empty() calls.

comment:59 Changed 8 years ago by Eugene Lazutkin

In [26188]:

dom: fixed the missing rename (dojo=>dom) and removed a reference to dojo/kernel, thx Akira Sudoh!, !strict, refs #9641.

comment:60 Changed 8 years ago by bill

In [26258]:

Eugene's patch (based on Rawld's advice) to resolve circular dependency problem by using "exports" variable, refs #9641 !strict

comment:61 Changed 8 years ago by bill

This ticket can be closed, right? I think (and hope) it's done.

comment:62 Changed 8 years ago by bill

Milestone: 1.81.7
Resolution: fixed
Status: assignedclosed
Type: enhancementtask

comment:63 Changed 7 years ago by bill

In [30353]:

Stop using deprecated dojo/_base/html and dojo/ready in parser and parser tests. Also use doh.is() rather than doh.t() for equality comparison. Refs #9641 !strict.

comment:64 Changed 7 years ago by bill

In [30399]:

remove obsolete comment (dom-prop now explicitly requires dom-construct), refs #9641 !strict.

Note: See TracTickets for help on using tickets.