Summary: | REGRESSION (r61285): some Dashboard widgets are always invisible due to HTML parser changes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andy Estes <aestes> | ||||
Component: | DOM | Assignee: | 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
Andy Estes
2010-09-23 17:25:15 PDT
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. (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 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. Committed r68228: <http://trac.webkit.org/changeset/68228> |