[webkit-reviews] review granted: [Bug 46435] REGRESSION (r61285): some Dashboard widgets are always invisible due to HTML parser changes : [Attachment 68631] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 46435: REGRESSION (r61285): some Dashboard widgets are always invisible due
to HTML parser changes
https://bugs.webkit.org/show_bug.cgi?id=46435

Attachment 68631: Patch
https://bugs.webkit.org/attachment.cgi?id=68631&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.