Bug 46435 – REGRESSION (r61285): some Dashboard widgets are always invisible due to HTML parser changes

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

[Andy Estes]

REGRESSION (r61285): some Dashboard widgets are always invisible due to HTML parser changes

[Andy Estes]

<rdar://problem/8175982>

[Andy Estes]

Created attachment 68631 [details]
Patch

Comment on attachment 68631 [details]
Patch

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

> WebKit/mac/WebView/WebView.mm:1375
> +    static bool isAIMAndNeedsParserQuirks = applicationIsAOLInstantMessenger() && webKitLinkedBeforeHTML5Parser;
> +    
> +    // Microsoft My Day loads scripts using self-closing script tags, markup
> +    // which is incompatible with the HTML5 parser. Enable parser quirks for
> +    // this application. See <https://bugs.webkit.org/show_bug.cgi?id=46334>.
> +    static bool isMicrosoftMyDayAndNeedsParserQuirks = applicationIsMicrosoftMyDay() && webKitLinkedBeforeHTML5Parser;

This is really a review of the last patch: the order of evaluation here is suboptimal. If webKitLinkedBeforeHTML5Parser is false, you don’t want to go off and string-compare bundle identifiers.

[Andy Estes]

(In reply to comment #3)
> (From update of attachment 68631 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68631&action=review
> 
> > WebKit/mac/WebView/WebView.mm:1375
> > +    static bool isAIMAndNeedsParserQuirks = applicationIsAOLInstantMessenger() && webKitLinkedBeforeHTML5Parser;
> > +    
> > +    // Microsoft My Day loads scripts using self-closing script tags, markup
> > +    // which is incompatible with the HTML5 parser. Enable parser quirks for
> > +    // this application. See <https://bugs.webkit.org/show_bug.cgi?id=46334>.
> > +    static bool isMicrosoftMyDayAndNeedsParserQuirks = applicationIsMicrosoftMyDay() && webKitLinkedBeforeHTML5Parser;
> 
> This is really a review of the last patch: the order of evaluation here is suboptimal. If webKitLinkedBeforeHTML5Parser is false, you don’t want to go off and string-compare bundle identifiers.

Good call. I can reorder the expression before landing this.

[Darin Adler]

Comment on attachment 68631 [details]
Patch

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

>>> WebKit/mac/WebView/WebView.mm:1375
>>> +    static bool webKitLinkedBeforeHTML5Parser = !WebKitLinkedOnOrAfter(WEBKIT_FIRST_VERSION_WITH_HTML5_PARSER);
>>> +    
>>> +    // 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 isAIMAndNeedsParserQuirks = applicationIsAOLInstantMessenger() && webKitLinkedBeforeHTML5Parser;
>>> +    
>>> +    // Microsoft My Day loads scripts using self-closing script tags, markup
>>> +    // which is incompatible with the HTML5 parser. Enable parser quirks for
>>> +    // this application. See <https://bugs.webkit.org/show_bug.cgi?id=46334>.
>>> +    static bool isMicrosoftMyDayAndNeedsParserQuirks = applicationIsMicrosoftMyDay() && webKitLinkedBeforeHTML5Parser;
>> 
>> This is really a review of the last patch: the order of evaluation here is suboptimal. If webKitLinkedBeforeHTML5Parser is false, you don’t want to go off and string-compare bundle identifiers.
> 
> Good call. I can reorder the expression before landing this.

I originally had the same comment, but when I saw this was a global (due to the use of static) and thus evaluated only once, I decided not to say anything.

> WebKit/mac/WebView/WebView.mm:1376
> +    static bool webKitLinkedBeforeHTML5Parser = !WebKitLinkedOnOrAfter(WEBKIT_FIRST_VERSION_WITH_HTML5_PARSER);
> +    
> +    // 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 isAIMAndNeedsParserQuirks = applicationIsAOLInstantMessenger() && webKitLinkedBeforeHTML5Parser;
> +    
> +    // Microsoft My Day loads scripts using self-closing script tags, markup
> +    // which is incompatible with the HTML5 parser. Enable parser quirks for
> +    // this application. See <https://bugs.webkit.org/show_bug.cgi?id=46334>.
> +    static bool isMicrosoftMyDayAndNeedsParserQuirks = applicationIsMicrosoftMyDay() && webKitLinkedBeforeHTML5Parser;
>  
>  
>  
>  
> +

Instead of using three separate globals you could consider having just one global for the result of all this logic above.

> WebKit/mac/WebView/WebView.mm:1382
> +        || (_private->page && _private->page->settings()->usesDashboardBackwardCompatibilityMode())

Side thought: This rule, that Dashboard mode means use parser quirks, could go into WebCore instead of here in WebKit.

[Andy Estes]

Committed r68228: <http://trac.webkit.org/changeset/68228>