Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix for 2348 and 2344 #2349

Merged
merged 1 commit into from
Feb 13, 2016
Merged

Fix for 2348 and 2344 #2349

merged 1 commit into from
Feb 13, 2016

Conversation

nazar-pc
Copy link
Contributor

Fix for getters/setters for property become inaccessible when propery set on element before it is ready
Did not succeed in creating test case this time:(
Fixes #2348

nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Aug 22, 2015
@nazar-pc
New Polymer with more patches.
b793c8e 
Patches included:
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2258
* Polymer/polymer#2266
* Polymer/polymer#2295
* Polymer/polymer#2322
* Polymer/polymer#2349
@nazar-pc nazar-pc changed the title Fix for 2348 Fix for 2348 and 2344 Aug 23, 2015
@nazar-pc
Copy link
Contributor Author

Indeed it fixes #2344 as well...

@nazar-pc nazar-pc mentioned this pull request Aug 25, 2015
Closed
nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Aug 26, 2015
@nazar-pc
Git Polymer with more patches.
43e6b99 
Patches included:
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2258
* Polymer/polymer#2266
* Polymer/polymer#2295
* Polymer/polymer#2322
* Polymer/polymer#2349
* Polymer/polymer#2368
nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Aug 29, 2015
@nazar-pc
Polymer with patches rebased against stable.
a483c99 
Patches included:
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2258
* Polymer/polymer#2266
* Polymer/polymer#2295
* Polymer/polymer#2322
* Polymer/polymer#2349
* Polymer/polymer#2368
nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Sep 5, 2015
@nazar-pc
Polymer with patches rebased against git master.
932bd93 
Patches included:
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2258
* Polymer/polymer#2266
* Polymer/polymer#2295
* Polymer/polymer#2322
* Polymer/polymer#2349
* Polymer/polymer#2368
* Polymer/polymer#2419
@tjsavage
Copy link
Contributor

added @sjmiles to take a look on this as he was active on the corresponding issue

nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Oct 8, 2015
@nazar-pc
New upstream Polymer version 1.1.5.
ed370ad 
Patches applied on top of stable version:
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2295
* Polymer/polymer#2322
* Polymer/polymer#2349
* Polymer/polymer#2419
nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Nov 1, 2015
@nazar-pc
New upstream Polymer version 1.2.1 with even more patches on top.
dc1ad4e 
List of patches applied on top of stable version (one previous patch upstreamed, the rest are still here):
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2295
* Polymer/polymer#2349
* Polymer/polymer#2419
* Polymer/polymer#2642
* Polymer/polymer#2663
* Polymer/polymer#2664
nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Nov 6, 2015
@nazar-pc
New git version of Polymer 1.2.1 with even more patches on top.
384f1ad 
List of patches applied on top of git version (2 previous patches upstreamed, the rest are still here + 2 new patches added):
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2295
* Polymer/polymer#2349
* Polymer/polymer#2419
* Polymer/polymer#2642
* Polymer/polymer#2689
* Polymer/polymer#2692
* Polymer/polymer#2694
nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Nov 13, 2015
@nazar-pc
New version of Polymer 1.2.2 with patches on top.
12e9331 
List of patches applied on top of git version (no new patches upstreamed since last time:():
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2295
* Polymer/polymer#2349
* Polymer/polymer#2419
* Polymer/polymer#2642
* Polymer/polymer#2689
* https://github.com/Polymer/polymer/tree/fix-2692 branch supersedes Polymer/polymer#2692
* Polymer/polymer#2694
nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Dec 18, 2015
@nazar-pc
New version of Polymer 1.2.3 + git with patches on top.
0458303 
List of patches applied on top of git version (4 previous patches upstreamed and one fixed by alternative patch, yay!):
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2349
* Polymer/polymer#2419
* Polymer/polymer#3215
sorvell
sorvell reviewed Jan 28, 2016
View reviewed changes
src/standard/configure.html
@@ -121,6 +121,9 @@
value = value.call(this, this._config);
}
config[i] = value;
} else if (this.hasOwnProperty(i)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this[i] !== undefined should be faster than hasOwnProperty.

@sorvell
Copy link
Contributor

sorvell commented Jan 28, 2016

Sorry for the time lag on this. The main concern with this change is incurring a performance penalty on elements that do not have early-set properties. Would you be willing to do some performance testing on this? Thank.

@nazar-pc
Copy link
Contributor Author

Modified as you suggested.
About performance: created 10 elements with 300 String properties there, 10 instances of each element on page (30k properties in total, all passing through newly added code).

Without patch readystatechange took 45.56ms in Chromium, with patch 79.45ms. This is 1.74 times slower, but since this step is executed only once for each element and takes so small amount of time even for 300 properties this is an affordable change.

Code I've used for testing: http://pastebin.com/ZdMnrZqN

sorvell
sorvell reviewed Jan 29, 2016
View reviewed changes
src/standard/configure.html
@@ -121,6 +121,9 @@
value = value.call(this, this._config);
}
config[i] = value;
} else if (this[i] !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the check if and the else if should be switched here. If there's an existing value, we should take that and not the default value.

@sorvell
Copy link
Contributor

sorvell commented Jan 29, 2016

We plan to do some perf testing to see what overhead this adds to elements without early-property sets.

@nazar-pc
Copy link
Contributor Author

Modified as suggested

nazar-pc added a commit to nazar-pc/CleverStyle-Framework that referenced this pull request Feb 1, 2016
@nazar-pc
New version of Polymer 1.2.4 + git with patches on top.
816d8db 
List of patches applied on top of git version (1 previous patch upstreamed, 4 more added):
* Polymer/polymer#2205
* Polymer/polymer#2247
* Polymer/polymer#2349
* Polymer/polymer#2419
* Polymer/polymer#3222
* Polymer/polymer#3344
* Polymer/polymer#3279
* Polymer/polymer#3276
@sorvell sorvell added the p0 label Feb 3, 2016
@sorvell sorvell assigned sorvell and unassigned sjmiles Feb 3, 2016
@sorvell
Copy link
Contributor

sorvell commented Feb 4, 2016

The performance of this change looks ok to me. Can you please add a test? Thanks.

@sorvell sorvell removed the p0 label Feb 4, 2016
@nazar-pc
Fix for getters/setters for property become inaccessible when propert…
ecd9b09 
…y set on element before it is ready

Test added
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Feb 5, 2016

@sorvell, I've managed to write test for this change, also rebased against master

@kevinpschaaf kevinpschaaf mentioned this pull request Feb 13, 2016
Merged
@kevinpschaaf kevinpschaaf merged commit ecd9b09 into Polymer:master Feb 13, 2016
@nazar-pc nazar-pc deleted the fix-for-2348 branch February 25, 2016 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sorvell sorvell

Labels
cla: yes p0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@nazar-pc @tjsavage @sorvell @sjmiles @kevinpschaaf @googlebot