Bug 80494 – REGRESSION (r100847): Entries are clipped out in Day One

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

REGRESSION (r100847): Entries are clipped out in Day One

<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>.

Created attachment 130555 [details]
Re-enable the JavaScript bindings for document.wiwidth and document.height

[Adam Barth]

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.

(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.

[Adam Barth]

That's true, but so are my statements in Comment #3.

[Adam Barth]

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.

[Sam Weinig]

(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.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

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.)

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.

[Adam Barth]

> 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

[Adam Barth]

> 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.

[Adam Barth]

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.

[Adam Barth]

(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.)

Fixed in http://trac.webkit.org/projects/webkit/changeset/110065

Feel free to reopen bug 72591 and propose a different approach to addressing it.

[Adam Barth]

To be clear, you landed this patch over my explicit objection even though the patch was marked review- at the time.

(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

[e-mail]	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.

[Adam Barth]

(Thanks for pointing out the date on the commit.  I was only looking at the comments on this bug.)

[Dimitri Glazkov (Google)]

(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 :)

[Adam Barth]

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.

[Alexey Proskuryakov]

> 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.