Bug 45644 – Implement EOF handling in TextMode

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Implement EOF handling in TextMode

[Adam Barth]

Created attachment 67382 [details]
Patch

[Darin Adler]

Comment on attachment 67382 [details]
Patch

This patch breaks the old versions of Apple’s Mail application. There are a conditional comments here such as “if we need this setting”, but we do need it. I know it’s easier to just remove it, but we at Apple need to keep this. Maybe you can find someone from my team at Apple to work with you on this.

[Eric Seidel (no email)]

@abarth: you should just split this up so the text stuff can be landed w/o contention.

Also, I don't think the mail-specific code works anymore.  But someone would need to test.

[Eric Seidel (no email)]

Comment on attachment 67382 [details]
Patch

Oh, nm.  I see these are tied.

I guess I agree then.  We should remove this code and add it back with tests.  It's possible to test this now that we have layoutTestController.overridePreference().  We can use that in conjunction with a platform/mac-leopard or mac-tiger test to test this code if it's truly needed.

[Eric Seidel (no email)]

"if it's truly needed" -> it's possible that the new parser behavior matches old-mails needed quirk already.  Since the old code isn't tested, its impossible to know.

[Darin Adler]

(In reply to comment #3)
> But someone would need to test.

Whether this patch broke it or the HTML parser work broke it earlier, this does need to be kept working. It doesn’t need to be tracked in this bug report.

Here’s how someone can test it: On Leopard, run Apple Mail with the WebKit you wish to test. Open Mail’s Preferences. Click on Signatures. Click the "+" once. This should correctly make a user signature. Click the "+" again. If the bug is active, no signature is created and exception will occur.

[Darin Adler]

The incorrect old mail assumption was that the first child of the first child of the document is the body, not the head.

[Adam Barth]

> This patch breaks the old versions of Apple’s Mail application. There are a conditional comments here such as “if we need this setting”, but we do need it. I know it’s easier to just remove it, but we at Apple need to keep this. Maybe you can find someone from my team at Apple to work with you on this.

I think this setting is already broken.  This patch only affects the behavior of EOF in "text mode," which is if the document ends before we close a <style> or <script> tag.

Who's a good person to talk to about the requirements of Mail?  I can guess from how the old code worked, but it would be best to gather the requirements directly instead of reverse engineering them from the previous implementation.

[Adam Barth]

(In reply to comment #7)
> The incorrect old mail assumption was that the first child of the first child of the document is the body, not the head.

I see.  Presumably in the case where the head is created implicitly and there are no XML processing directives / DOCTYPEs / etc.

[Darin Adler]

(In reply to comment #9)
> (In reply to comment #7)
> > The incorrect old mail assumption was that the first child of the first child of the document is the body, not the head.
> 
> I see.  Presumably in the case where the head is created implicitly and there are no XML processing directives / DOCTYPEs / etc.

That’s right. And it’s only the older versions of Mail that have this dependency.

[Adam Barth]

Created attachment 67431 [details]
Patch for landing

[Eric Seidel (no email)]

@darin:  It seems a better hack for this would be to just hack the Obj-C DOM bindings layer.

Make a DOMHTMLDocumentCustom.mm file and have it check the bundle identifier for Mail and override firstChild to return body instead of the real first child.

Hacking this into WebCore seems like a bad idea.

[Eric Seidel (no email)]

The hack would involve hacking Node.idl to make a custom binding for firstChild in the Leopard/Tiger Obj-C case.

This is assuming of course that this is how Mail is accessing the first child of the document and assuming it's the body. :)

[Darin Adler]

It’s OK to put this hack in the bindings layer, but I’m not sure it’s better. It’s probably more complicated, not less.

[Adam Barth]

(In reply to comment #14)
> It’s OK to put this hack in the bindings layer, but I’m not sure it’s better. It’s probably more complicated, not less.

The version in the parser is https://bugs.webkit.org/show_bug.cgi?id=45693

[Adam Barth]

Comment on attachment 67431 [details]
Patch for landing

Clearing flags on attachment: 67431

Committed r67406: <http://trac.webkit.org/changeset/67406>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.