80494 2012-03-06 23:16:07 -0800 REGRESSION (r100847): Entries are clipped out in Day One 2012-03-09 12:03:48 -0800 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar, Regression P1 Normal --- 1 mitz mitz abarth annevk ap darin dglazkov Ms2ger ojan sam webkit.review.bot oldest_to_newest 572573 0 mitz 2012-03-06 23:16:07 -0800 REGRESSION (r100847): Entries are clipped out in Day One 572580 1 mitz 2012-03-06 23:22:24 -0800 <rdar://problem/10923294> With TOT WebKit, journal entires in Day One are clipped vertically. The app uses embedded web content to render its UI, and that content relies on document.width and document.height, which were made inaccessible in <http://trac.webkit.org/r100847>. 572587 2 130555 mitz 2012-03-06 23:32:59 -0800 Created attachment 130555 Re-enable the JavaScript bindings for document.wiwidth and document.height 572596 3 abarth 2012-03-06 23:43:14 -0800 I'm not sure we should make these APIs visible to the web. They're not part of any spec and they're not implemented in any other rendering engine. It doesn't seem right for Mac app compatibility to drive changes to the web platform. 572606 4 mitz 2012-03-06 23:57:03 -0800 (In reply to comment #3) > I'm not sure we should make these APIs visible to the web. These properties have been accessible from JavaScript in major web browsers for over 5 years now. 572613 5 abarth 2012-03-07 00:00:46 -0800 That's true, but so are my statements in Comment #3. 572617 6 abarth 2012-03-07 00:04:37 -0800 Another way of explaining my point of view is that this bug is specific to Mac app compatibility. As such, it seems appropriate for the solution to also be specific to Mac apps. If there's a reason to expose these APIs to the web at large, that's another matter, but you haven't given such a reason. 572899 7 sam 2012-03-07 08:29:08 -0800 (In reply to comment #6) > Another way of explaining my point of view is that this bug is specific to Mac app compatibility. As such, it seems appropriate for the solution to also be specific to Mac apps. > > If there's a reason to expose these APIs to the web at large, that's another matter, but you haven't given such a reason. As we currently don't have a way to easily enable a JS binding for a particular mac app, it seems to me the need to fix a regression out weighs the theoretical pollution of the web platform. 572949 8 Ms2ger 2012-03-07 09:17:35 -0800 This is not a case of "theoretical pollution", and phrasing it as such is disingenuous. This is yet another case where WebKit makes the web worse for no reason at all; issues with non-web embeddings should be fixed with those embeddings, as has been done in a number of cases already. (Canvas comes to mind, in particular.) 572960 9 mitz 2012-03-07 09:34:27 -0800 Disingenuity is having this discussion as if WebKit had not been shipping with this behavior for over half a decade, it was changed very recently in TOT without compelling justification, causing a regression, and attachment 130555 is restoring the status quo. 572961 10 abarth 2012-03-07 09:37:22 -0800 > As we currently don't have a way to easily enable a JS binding for a particular mac app, it seems to me the need to fix a regression out weighs the theoretical pollution of the web platform. We actually do have an easy mechanism for doing that. One simply needs to add a script similar to http://trac.webkit.org/browser/trunk/Source/WebKit/mac/Misc/MailQuirksUserScript.js 572966 11 abarth 2012-03-07 09:39:08 -0800 > it was changed very recently in TOT without compelling justification The justification is the same as in comment #6, namely that this API isn't part of any standard and isn't implemented by other rendering engines. Exposing this API to the web is bad for the web because it reduces interoperability. 572970 12 130555 abarth 2012-03-07 09:43:22 -0800 Comment on attachment 130555 Re-enable the JavaScript bindings for document.wiwidth and document.height I'm not saying we shouldn't fix the regression---we should---I'm saying we should do so in a way that doesn't expose this API to the web platform. We might later discover that there's a reason to expose this API to the web platform, but currently it appears that doing so has only costs and not benefits. 572975 13 abarth 2012-03-07 09:48:46 -0800 (To be clear, the reason I changed Sam's r+ to an r- is because half of the reason he gave for marking it r+ was factually incorrect.) 572977 14 mitz 2012-03-07 09:52:31 -0800 Fixed in http://trac.webkit.org/projects/webkit/changeset/110065 Feel free to reopen bug 72591 and propose a different approach to addressing it. 573006 15 abarth 2012-03-07 10:39:02 -0800 To be clear, you landed this patch over my explicit objection even though the patch was marked review- at the time. 573050 16 mitz 2012-03-07 11:28:43 -0800 (In reply to comment #15) > To be clear, you landed this patch over my explicit objection even though the patch was marked review- at the time. What makes you say this? At <https://bugs.webkit.org/show_activity.cgi?id=80494> I see a line that says abarth@webkit.org 2012-03-07 09:43:22 PST Attachment #130555 Flag review+ review- and at <http://trac.webkit.org/changeset/110065> I see Timestamp: 03/07/2012 09:38:56 My understanding is that the clocks on these two systems are within less than one minute of each other. 573058 17 abarth 2012-03-07 11:34:55 -0800 (Thanks for pointing out the date on the commit. I was only looking at the comments on this bug.) 573856 18 dglazkov 2012-03-08 08:40:20 -0800 (In reply to comment #17) > (Thanks for pointing out the date on the commit. I was only looking at the comments on this bug.) Still seems a bit of an uncool situation. If I remember correctly, I've been in Dan's place a couple of years back and I immediately rolled out the patch and apologized profusely. Perhaps I am just too nice of a guy :) 574243 19 abarth 2012-03-08 14:46:16 -0800 I'm not going to press the issue. However, I'm not sure this bug is a model for how we'd like the project to operate. 575052 20 ap 2012-03-09 12:03:41 -0800 > However, I'm not sure this bug is a model for how we'd like the project to operate. I think (and what I did in some cases before) is that in controversial cases, it's desirable to give all parties some time to comment even when there is already r+ on a patch. This patch was in attachment overnight, which seems almost enough, but obviously wasn't enough to avoid complications. Monitoring bugs realtime for quick erroneous r+/commit is not something I'd like reviewers to be pressed into. However, I don't necessarily agree that undoing the commit is always the right action upon review override that happened during commit. In this case specifically, Dan returned us to a state that we've been in for years, and where we don't have a regression in a shipping 3rd party application. That's clearly a better state for calm and thoughtful discussions on the merits of API removal. 130555 2012-03-06 23:32:59 -0800 2012-03-07 09:43:22 -0800 Re-enable the JavaScript bindings for document.wiwidth and document.height bug-80494-20120306233257.patch text/plain 3245 mitz SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMDAyMCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE1IEBACisyMDEyLTAzLTA2ICBEYW4gQmVy bnN0ZWluICA8bWl0ekBhcHBsZS5jb20+CisKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzEwOTIz Mjk0PiBSRUdSRVNTSU9OIChyMTAwODQ3KTogRW50cmllcyBhcmUgY2xpcHBlZCBvdXQgaW4gRGF5 IE9uZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODA0 OTQKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUZXN0 OiBmYXN0L2RvbS9IVE1MRG9jdW1lbnQvd2lkdGgtYW5kLWhlaWdodC5odG1sCisKKyAgICAgICAg KiBodG1sL0hUTUxEb2N1bWVudC5pZGw6IFJldmVydGVkIHIxMDA4NDcgYnkgcmUtZW5hYmxpbmcg dGhlIHdpZHRoIGFuZCBoZWlnaHQgcHJvcGVydGllcworICAgICAgICBpbiB0aGUgSmF2YVNjcmlw dCBiaW5kaW5ncy4KKwogMjAxMi0wMy0wNiAgRGFuYSBKYW5zZW5zICA8ZGFuYWtqQGNocm9taXVt Lm9yZz4KIAogICAgICAgICBbY2hyb21pdW1dIEFkZCBjbGlwcGluZyB0byBzY2lzc29yIHJlY3Qg dG8gQ0NPY2NsdXNpb25UcmFja2VyCkluZGV4OiBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxEb2N1 bWVudC5pZGwKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MRG9jdW1lbnQu aWRsCShyZXZpc2lvbiAxMDk5MjcpCisrKyBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxEb2N1bWVu dC5pZGwJKHdvcmtpbmcgY29weSkKQEAgLTQ1LDEwICs0NSw4IEBAIG1vZHVsZSBodG1sIHsKICAg ICAgICAgdm9pZCBjYXB0dXJlRXZlbnRzKCk7CiAgICAgICAgIHZvaWQgcmVsZWFzZUV2ZW50cygp OwogCi0jaWYgZGVmaW5lZChMQU5HVUFHRV9PQkpFQ1RJVkVfQykgJiYgTEFOR1VBR0VfT0JKRUNU SVZFX0MKICAgICAgICAgcmVhZG9ubHkgYXR0cmlidXRlIGxvbmcgd2lkdGg7CiAgICAgICAgIHJl YWRvbmx5IGF0dHJpYnV0ZSBsb25nIGhlaWdodDsKLSNlbmRpZgogICAgICAgICAgICAgICAgICBh dHRyaWJ1dGUgW1RyZWF0TnVsbEFzPU51bGxTdHJpbmddIERPTVN0cmluZyBkaXI7CiAgICAgICAg ICAgICAgICAgIGF0dHJpYnV0ZSBbVHJlYXROdWxsQXM9TnVsbFN0cmluZ10gRE9NU3RyaW5nIGRl c2lnbk1vZGU7CiAgICAgICAgIHJlYWRvbmx5IGF0dHJpYnV0ZSBET01TdHJpbmcgY29tcGF0TW9k ZTsKSW5kZXg6IExheW91dFRlc3RzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBMYXlvdXRUZXN0 cy9DaGFuZ2VMb2cJKHJldmlzaW9uIDExMDAyMykKKysrIExheW91dFRlc3RzL0NoYW5nZUxvZwko d29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDEzIEBACisyMDEyLTAzLTA2ICBEYW4gQmVybnN0ZWlu ICA8bWl0ekBhcHBsZS5jb20+CisKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzEwOTIzMjk0PiBS RUdSRVNTSU9OIChyMTAwODQ3KTogRW50cmllcyBhcmUgY2xpcHBlZCBvdXQgaW4gRGF5IE9uZQor ICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODA0OTQKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIGZhc3QvZG9t L0hUTUxEb2N1bWVudC93aWR0aC1hbmQtaGVpZ2h0LWV4cGVjdGVkLnR4dDogQWRkZWQuCisgICAg ICAgICogZmFzdC9kb20vSFRNTERvY3VtZW50L3dpZHRoLWFuZC1oZWlnaHQuaHRtbDogQWRkZWQu CisKIDIwMTItMDMtMDYgIEhheWF0byBJdG8gIDxoYXlhdG9AY2hyb21pdW0ub3JnPgogCiAgICAg ICAgIFtTaGFkb3cgRE9NXSBFeHRlbmQgYSAncGF0aCcgbm90YXRpb24gaW4gdXRpbGl0eSBmdW5j dGlvbiBzbyB0aGF0IHdlIGNhbiBzcGVjaWZ5IGFyYml0cmFyeSBTaGFkb3dSb290LgpJbmRleDog TGF5b3V0VGVzdHMvZmFzdC9kb20vSFRNTERvY3VtZW50L3dpZHRoLWFuZC1oZWlnaHQtZXhwZWN0 ZWQudHh0Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvZG9tL0hUTUxEb2N1bWVudC93 aWR0aC1hbmQtaGVpZ2h0LWV4cGVjdGVkLnR4dAkocmV2aXNpb24gMCkKKysrIExheW91dFRlc3Rz L2Zhc3QvZG9tL0hUTUxEb2N1bWVudC93aWR0aC1hbmQtaGVpZ2h0LWV4cGVjdGVkLnR4dAkocmV2 aXNpb24gMCkKQEAgLTAsMCArMSBAQAorUEFTUy4gVGhlIGRvY3VtZW50LndpZHRoIGFuZCBkb2N1 bWVudC5oZWlnaHQgcHJvcGVydGllcyBhcmUgYWNjZXNzaWJsZSBmcm9tIEphdmFTY3JpcHQuCklu ZGV4OiBMYXlvdXRUZXN0cy9mYXN0L2RvbS9IVE1MRG9jdW1lbnQvd2lkdGgtYW5kLWhlaWdodC5o dG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIExheW91dFRlc3RzL2Zhc3QvZG9tL0hUTUxEb2N1bWVudC93aWR0 aC1hbmQtaGVpZ2h0Lmh0bWwJKHJldmlzaW9uIDApCisrKyBMYXlvdXRUZXN0cy9mYXN0L2RvbS9I VE1MRG9jdW1lbnQvd2lkdGgtYW5kLWhlaWdodC5odG1sCShyZXZpc2lvbiAwKQpAQCAtMCwwICsx LDkgQEAKKzxzY3JpcHQ+CisgICAgaWYgKHdpbmRvdy5sYXlvdXRUZXN0Q29udHJvbGxlcikKKyAg ICAgICAgbGF5b3V0VGVzdENvbnRyb2xsZXIuZHVtcEFzVGV4dCgpOworCisgICAgaWYgKCd3aWR0 aCcgaW4gZG9jdW1lbnQgJiYgJ2hlaWdodCcgaW4gZG9jdW1lbnQpCisgICAgICAgIGRvY3VtZW50 LndyaXRlKCJQQVNTLiBUaGUgZG9jdW1lbnQud2lkdGggYW5kIGRvY3VtZW50LmhlaWdodCBwcm9w ZXJ0aWVzIGFyZSBhY2Nlc3NpYmxlIGZyb20gSmF2YVNjcmlwdC4iKTsKKyAgICBlbHNlCisgICAg ICAgIGRvY3VtZW50LndyaXRlKCJGQUlMLiBkb2N1bWVudC53aWR0aCBvciBkb2N1bWVudC5oZWln aHQgYXJlIG5vdCBhY2Nlc3NpYmxlIGZyb20gSmF2YVNjcmlwdC4iKTsKKzwvc2NyaXB0Pgo=