Bug 80494 - REGRESSION (r100847): Entries are clipped out in Day One
Summary: REGRESSION (r100847): Entries are clipped out in Day One
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: mitz
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-03-06 23:16 PST by mitz
Modified: 2012-03-09 12:03 PST (History)
9 users (show)

See Also:


Attachments
Re-enable the JavaScript bindings for document.wiwidth and document.height (3.17 KB, patch)
2012-03-06 23:32 PST, mitz
abarth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2012-03-06 23:16:07 PST
REGRESSION (r100847): Entries are clipped out in Day One
Comment 1 mitz 2012-03-06 23:22:24 PST
<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>.
Comment 2 mitz 2012-03-06 23:32:59 PST
Created attachment 130555 [details]
Re-enable the JavaScript bindings for document.wiwidth and document.height
Comment 3 Adam Barth 2012-03-06 23:43:14 PST
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.
Comment 4 mitz 2012-03-06 23:57:03 PST
(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.
Comment 5 Adam Barth 2012-03-07 00:00:46 PST
That's true, but so are my statements in Comment #3.
Comment 6 Adam Barth 2012-03-07 00:04:37 PST
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.
Comment 7 Sam Weinig 2012-03-07 08:29:08 PST
(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.
Comment 8 Ms2ger (he/him; ⌚ UTC+1/+2) 2012-03-07 09:17:35 PST
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.)
Comment 9 mitz 2012-03-07 09:34:27 PST
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 [details] is restoring the status quo.
Comment 10 Adam Barth 2012-03-07 09:37:22 PST
> 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
Comment 11 Adam Barth 2012-03-07 09:39:08 PST
> 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.
Comment 12 Adam Barth 2012-03-07 09:43:22 PST
Comment on attachment 130555 [details]
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.
Comment 13 Adam Barth 2012-03-07 09:48:46 PST
(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.)
Comment 14 mitz 2012-03-07 09:52:31 PST
Fixed in http://trac.webkit.org/projects/webkit/changeset/110065

Feel free to reopen bug 72591 and propose a different approach to addressing it.
Comment 15 Adam Barth 2012-03-07 10:39:02 PST
To be clear, you landed this patch over my explicit objection even though the patch was marked review- at the time.
Comment 16 mitz 2012-03-07 11:28:43 PST
(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 [details] 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.
Comment 17 Adam Barth 2012-03-07 11:34:55 PST
(Thanks for pointing out the date on the commit.  I was only looking at the comments on this bug.)
Comment 18 Dimitri Glazkov (Google) 2012-03-08 08:40:20 PST
(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 :)
Comment 19 Adam Barth 2012-03-08 14:46:16 PST
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.
Comment 20 Alexey Proskuryakov 2012-03-09 12:03:41 PST
> 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.