Updates jQuery framework to 1.8.3 and removes buffered jQuery. #872

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Owner

publickeating commented Dec 11, 2012

Buffered jQuery theoretically improves performance by grouping all DOM manipulations through jQuery into a queue that can be flushed at once, but this doesn't seem necessary with the Run Loop and the way browsers update the DOM.

After benchmarking a number of View adds, updates, moves and removals on 1.4.5 (Core Query), 1.9.1 (jQuery 1.6.2), master (jQuery 1.8.3 buffered) and master (jQuery 1.8.3 un-buffered), I could not find a significant performance difference between the versions, but master with jQuery 1.8.3 un-buffered was usually slightly faster in the majority of tests, but not outside a margin of error.

Therefore, it seems that the buffer code is not helping anything or else I'm not testing a scenario where it makes a difference yet.

I'm submitting this as a pull request, until I can do more benchmarking to be sure of the change and to hopefully get some feedback.

@publickeating publickeating Updates jQuery framework to 1.8.3 and removes buffered jQuery.
Buffered jQuery theoretically improves performance by grouping all DOM manipulations through jQuery into a queue that can be flushed at once, but this needs to be benchmarked to be properly understood.
26c7d01
Member

nicolasbadia commented Dec 11, 2012

I've just try your pull request and having a small issue. If I set a layout like this:

layout: { right: 0, width: 10 }

When the view is render, the left, top, ... property are also set. This seem due to the change to the styles method in render_context.js

About jQuery 1.8.3 and SC, I'm using it since a few weeks with no issue.

Owner

publickeating commented Dec 11, 2012

I'm not sure I understand, because what you describe seems to be the normal behaviour. The default layout is always { left: 0, right: 0, top: 0, bottom: 0 }, so if you apply a portion of the layout, it merges it into the default.

Examples:

layout: { top: 10} -> style="left: 0px; right: 0px; top: 10px; bottom: 0px;"
layout: { left: 10, top: 10} -> style="left: 10px; right: 0px; top: 10px; bottom: 0px;"
layout: { left: 10, top: 10, width: 100 } -> style="left: 10px; top: 10px; bottom: 0px; width: 100px" // replaces right with width
layout: { right: 10, top: 10, width: 100 } -> style="right: 10px; top: 10px; bottom: 0px; width: 100px" // replaces left with width

Tyler Keating
tyler@sproutcore.com

On 2012-12-11, at 2:33 AM, Nicolas BADIA notifications@github.com wrote:

I've just try your pull request and having a small issue. If I set a layout like this:

layout: { right: 0, width: 10 }
When the view is render, the left, top, ... property are also set. This seem due to the change to the styles method in render_context.js

About jQuery 1.8.3 and SC, I'm using it since a few weeks with no issue.


Reply to this email directly or view it on GitHub.

Owner

publickeating commented Dec 11, 2012

Sorry Nicolas,

I DID misunderstand. I see what you mean, I'll fix it right away.

Thanks for testing this out.

Tyler Keating
tyler@sproutcore.com

On 2012-12-11, at 2:33 AM, Nicolas BADIA notifications@github.com wrote:

I've just try your pull request and having a small issue. If I set a layout like this:

layout: { right: 0, width: 10 }
When the view is render, the left, top, ... property are also set. This seem due to the change to the styles method in render_context.js

About jQuery 1.8.3 and SC, I'm using it since a few weeks with no issue.


Reply to this email directly or view it on GitHub.

publickeating added some commits Dec 11, 2012

@publickeating publickeating Fixes styles application after changes and updates tests to match cha…
…nges with jQuery styles.
0007363
@publickeating publickeating Gets rid of deprecated code and warning on IE. 6791436
@publickeating publickeating Patches jQuery to prevent security exceptions in IE.
This does NOT patch jQuery to make all calls to .append(), .insert(), etc. pass security validation in IE.  Instead it uses toStaticHTML (as recommended by Microsoft) to strip 3 innerHTML strings used within jQuery for testing.
03c3368
@publickeating publickeating Changes the patch of jQuery to use execUnsafeLocalFunction, which doe…
…sn't alter the test HTML used by jQuery and therefore should not effect the outcome of the jQuery tests while still removing the annoying IE warnings.
d31159a
Member

nicolasbadia commented Dec 13, 2012

Still an issue if the layout is binded. For exemple:

layoutBinding: SC.Binding.from('myPath').transform(function(val) {
  return { top: 0, right: 0, width: val?400:200 };
})

Edit:
Same problem if you set a bottomToolbar to a SC.WorkspaceView. The _scws_tile function set a layout like this :

 left: 0, right: 0, bottom: 0, height: toolbarSize

But the bottomToolbar is displayed over the topToolbar.

Member

nicolasbadia commented Dec 18, 2012

I'd just try your last commit and this issue seem to be fix.
Meanwhile, the checkboxes inside an SC.SourceListView are now weirdly rendered. This can be reproduce in the Showcase, if you set a contentCheckboxKey to SC.ListItemView:

Showcase.sourceListViews = SC.ScrollView.design({
  contentView: Showcase.ViewsListView.design({
    content: [
      Showcase.ViewsItemContent.create({
        title: 'Regular',
        example: "SC.SourceListView.extend({  classNames: ['my-source-list-view'],  layout: { bottom: 20, left: 20, right: 20, top: 20 },  contentBinding: SC.Binding.oneWay('Showcase.sourceListTree'),  exampleView: SC.ListItemView.extend({  hasContentIcon: true,  contentIconKey: 'icon', contentCheckboxKey: 'name',  contentUnreadCountKey: 'count',  contentValueKey: 'name'  }),  groupExampleView: SC.ListItemView.extend({  contentValueKey: 'groupName'  })  })"
      })
    ],
    exampleHeight: 575
  })
});

Edit :
If it can save you some time, it looks like there is a problem with this:

.addClass(theme.classNames)

and this:

.addClass(this.get('theme').classNames)
Owner

publickeating commented Jan 3, 2013

Thanks @nicolasbadia I've fixed the problem with checkboxes (and radio buttons) and updated the Showcase app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment