Fix #13494: Fallback Object.defineProperties to jQuery.extend for Android<4 #1182

Closed
wants to merge 6 commits into
from

Projects

None yet

3 participants

@gibson042
Member

cc @rwldrn

Net +4 bytes min+gzip. Each commit gets a bit smaller, but also diverges more from current master. The module's all yours, so treat this as POC.

Owner

The support.js changes are just borrowing from the bit-bank here? Let's land this as two separate commits to make that clear.

Member

Not entirely; they happened over the progression of changes (refactoring to add a new support test, then finding it unnecessary).

Member

I've poked around a bit and everything is sound. I disagree with the renaming of locker=>key... The result of the method is that either a "locker" object is created in the cache for the owner object and the "key" to "unlock" is returned or just the latter.

Member

Rebased to remove support.js clutter and correct the undefined expando glitch.

Could you go into more detail on locker vs. key? It seems to me like the primary purpose of that method is to return the cache key, and creating the object is incidental. In fact, I foresee a potential future performance optimization to only do so in set (remove already has code to that effect, currently just dead weight).

Member

edit

No, the cache creation was always the primary operation of that method. When the storage was an array, the key was being derived in each method that accessed the cache by just getting the index (which was the key). That changed when optimized implementation needed to associate an identity with each object that it interacted with; now I needed a mechanism to both create the cache and/or get the key, without too much boilerplate. Combining the operations into an aggregate operation made sense. From a real world perspective, think of it as:

  1. I go to the bank to access my safe deposit box.
  2. If I don't yet have one, a bank manager sets me up with a new one
  3. Based on my identity as the owner of the safe deposit box, the bank manager gives me the key
    ... (return)
  4. Now I can access the contents of my safe deposit box.

"safe" seemed less obvious then it's synonym "locker" to describe the mechanism that the method served. "key" is only part of the story.

Member

You should remove that condition, it's completely unnecessary

Owner

Is this ready?

I have to say the "locker" term had me puzzled at first read-through, I read it as "that which locks" versus "a secure repository for something". I know the term "key" tends to be overused but then again it's comfy and warm like a little kitty.

Member

Sure it's fine to land

@gibson042 gibson042 added a commit that closed this pull request Feb 27, 2013
@gibson042 @dmethvin gibson042 + dmethvin Fix #13494: Fallback defineProperties to jQuery.extend. Close gh-1182.
Android<4 (ancient WebKit) doesn't have full ES5 support.
85f1985
@gibson042 gibson042 closed this in 85f1985 Feb 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment