Bug 46134

Summary: REGRESSION (r61285): AIM 2.1.296: Code rendered as text in Welcome screen
Product: WebKit Reporter: Andy Estes <aestes>
Component: DOMAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, mitz
Priority: P2 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 40961    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Andy Estes 2010-09-20 16:42:32 PDT
After r61285, markup is displayed on the Mac AIM client's welcome screen when it links against a ToT WebKit.framework. This is due to a change in how WebKit's parser handles invalid markup. An app-specific workaround should be checked in.
Comment 1 Andy Estes 2010-09-20 16:43:28 PDT
<rdar://problem/8154379>
Comment 2 Andy Estes 2010-09-20 18:34:08 PDT
Created attachment 68172 [details]
Patch
Comment 3 mitz 2010-09-20 18:39:40 PDT
Comment on attachment 68172 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68172&action=review

> WebKit/mac/WebView/WebView.mm:615
> +static bool shouldUsePreHTML5ParserQuirks(WebPreferences* preferences)
> +{
> +    return ([preferences usePreHTML5ParserQuirks] || applicationIsAOLInstantMessenger());
> +}

Should this be limited to certain versions of AIM or simply versions of AIM linked against versions of WebKit that had the old parser?
Comment 4 Andy Estes 2010-09-20 18:52:15 PDT
(In reply to comment #3)
> (From update of attachment 68172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68172&action=review
> 
> > WebKit/mac/WebView/WebView.mm:615
> > +static bool shouldUsePreHTML5ParserQuirks(WebPreferences* preferences)
> > +{
> > +    return ([preferences usePreHTML5ParserQuirks] || applicationIsAOLInstantMessenger());
> > +}
> 
> Should this be limited to certain versions of AIM or simply versions of AIM linked against versions of WebKit that had the old parser?

bdash pointed out the rationale of doing this, so yes we should do this. I'm not sure whether it's better to check the version of AIM or check the version of WebKit AIM linked against; it seems the same thing can be accomplished either way.
Comment 5 mitz 2010-09-20 19:05:09 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 68172 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=68172&action=review
> > 
> > > WebKit/mac/WebView/WebView.mm:615
> > > +static bool shouldUsePreHTML5ParserQuirks(WebPreferences* preferences)
> > > +{
> > > +    return ([preferences usePreHTML5ParserQuirks] || applicationIsAOLInstantMessenger());
> > > +}
> > 
> > Should this be limited to certain versions of AIM or simply versions of AIM linked against versions of WebKit that had the old parser?
> 
> bdash pointed out the rationale of doing this, so yes we should do this. I'm not sure whether it's better to check the version of AIM or check the version of WebKit AIM linked against; it seems the same thing can be accomplished either way.

I would check the WebKit version, since AIM’s version numbering is out of my control (and doesn't make much sense). I am not even sure that this behavior should be restricted to AIM.
Comment 6 Daniel Bates 2010-09-20 21:55:05 PDT
Comment on attachment 68172 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68172&action=review

> WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Briefly glanced at this patch and noticed this OOPS line. Obviously you will need to remove this line before landing.
Comment 7 Andy Estes 2010-09-20 23:04:44 PDT
Created attachment 68189 [details]
Patch
Comment 8 Adam Roben (:aroben) 2010-09-21 06:37:53 PDT
Comment on attachment 68189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68189&action=review

> WebKit/mac/WebView/WebView.mm:621
> +static bool shouldUsePreHTML5ParserQuirks(WebPreferences* preferences)
> +{
> +    // AIM clients linked against versions of WebKit prior to the introduction
> +    // of the HTML5 parser contain markup incompatible with the new parser.
> +    // Enable parser quirks to remain compatible with these clients. See
> +    // <https://bugs.webkit.org/show_bug.cgi?id=46134>.
> +    static bool shouldUsePreHTML5ParserQuirks = [preferences usePreHTML5ParserQuirks]
> +        || (!WebKitLinkedOnOrAfter(WEBKIT_FIRST_VERSION_WITH_HTML5_PARSER) && applicationIsAOLInstantMessenger());
> +    return shouldUsePreHTML5ParserQuirks;
> +}

This doesn't seem to support a client application calling [preferences setUsePreHTML5ParserQuirks] after this function is first called, or applications that use multiple WebPreferences instances.

If you put the linked-on-or-after and is-AOL checks into their own static bool, then you can check [preferences usePreHTML5ParserQuirks] every time this function is called.
Comment 9 Andy Estes 2010-09-21 11:57:50 PDT
Created attachment 68270 [details]
Patch
Comment 10 Andy Estes 2010-09-21 11:58:22 PDT
(In reply to comment #8)
> (From update of attachment 68189 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68189&action=review
> 
> > WebKit/mac/WebView/WebView.mm:621
> > +static bool shouldUsePreHTML5ParserQuirks(WebPreferences* preferences)
> > +{
> > +    // AIM clients linked against versions of WebKit prior to the introduction
> > +    // of the HTML5 parser contain markup incompatible with the new parser.
> > +    // Enable parser quirks to remain compatible with these clients. See
> > +    // <https://bugs.webkit.org/show_bug.cgi?id=46134>.
> > +    static bool shouldUsePreHTML5ParserQuirks = [preferences usePreHTML5ParserQuirks]
> > +        || (!WebKitLinkedOnOrAfter(WEBKIT_FIRST_VERSION_WITH_HTML5_PARSER) && applicationIsAOLInstantMessenger());
> > +    return shouldUsePreHTML5ParserQuirks;
> > +}
> 
> This doesn't seem to support a client application calling [preferences setUsePreHTML5ParserQuirks] after this function is first called, or applications that use multiple WebPreferences instances.
> 
> If you put the linked-on-or-after and is-AOL checks into their own static bool, then you can check [preferences usePreHTML5ParserQuirks] every time this function is called.

Yep, you're right. Fixed.
Comment 11 Andy Estes 2010-09-21 13:44:11 PDT
Committed r67983: <http://trac.webkit.org/changeset/67983>