Bug 46435

Summary: REGRESSION (r61285): some Dashboard widgets are always invisible due to HTML parser changes
Product: WebKit Reporter: Andy Estes <aestes>
Component: DOMAssignee: Andy Estes <aestes>
Status: RESOLVED FIXED    
Severity: Normal Keywords: InRadar, Regression
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch darin: review+

Description Andy Estes 2010-09-23 17:25:15 PDT
REGRESSION (r61285): some Dashboard widgets are always invisible due to HTML parser changes
Comment 1 Andy Estes 2010-09-23 17:26:15 PDT
<rdar://problem/8175982>
Comment 2 Andy Estes 2010-09-23 17:33:32 PDT
Created attachment 68631 [details]
Patch
Comment 3 mitz 2010-09-23 17:42:49 PDT
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.
Comment 4 Andy Estes 2010-09-23 17:47:26 PDT
(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.
Comment 5 Darin Adler 2010-09-23 18:05:11 PDT
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.
Comment 6 Andy Estes 2010-09-23 19:28:34 PDT
Committed r68228: <http://trac.webkit.org/changeset/68228>