46435 2010-09-23 17:25:15 -0700 REGRESSION (r61285): some Dashboard widgets are always invisible due to HTML parser changes 2010-09-23 19:28:34 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED InRadar, Regression P2 Normal --- 1 aestes aestes oldest_to_newest 284135 0 aestes 2010-09-23 17:25:15 -0700 REGRESSION (r61285): some Dashboard widgets are always invisible due to HTML parser changes 284136 1 aestes 2010-09-23 17:26:15 -0700 <rdar://problem/8175982> 284142 2 68631 aestes 2010-09-23 17:33:32 -0700 Created attachment 68631 Patch 284147 3 68631 mitz 2010-09-23 17:42:49 -0700 Comment on attachment 68631 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. 284153 4 aestes 2010-09-23 17:47:26 -0700 (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. 284160 5 68631 darin 2010-09-23 18:05:11 -0700 Comment on attachment 68631 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. 284192 6 aestes 2010-09-23 19:28:34 -0700 Committed r68228: <http://trac.webkit.org/changeset/68228> 68631 2010-09-23 17:33:32 -0700 2010-09-23 18:05:11 -0700 Patch bug-46435-20100923173331.patch text/plain 5167 aestes SW5kZXg6IFdlYktpdC9tYWMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdC9tYWMvQ2hh bmdlTG9nCShyZXZpc2lvbiA2ODIyMCkKKysrIFdlYktpdC9tYWMvQ2hhbmdlTG9nCSh3b3JraW5n IGNvcHkpCkBAIC0xLDMgKzEsMjIgQEAKKzIwMTAtMDktMjMgIEFuZHkgRXN0ZXMgIDxhZXN0ZXNA YXBwbGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFJFR1JFU1NJT04gKHI2MTI4NSk6IHNvbWUgRGFzaGJvYXJkIHdpZGdldHMgYXJlIGFsd2F5 cyBpbnZpc2libGUgZHVlIHRvCisgICAgICAgIEhUTUwgcGFyc2VyIGNoYW5nZXMuCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00NjQzNQorCisgICAgICAg IEVuYWJsZSBwcmUtSFRNTDUgcGFyc2VyIHF1aXJrcyBpZiBEYXNoYm9hcmQgaXMgaW4gYmFja3dh cmQgY29tcGF0aWJpbGl0eQorICAgICAgICBtb2RlLgorCisgICAgICAgICogV2ViVmlldy9XZWJW aWV3Lm1tOgorICAgICAgICAoLVtXZWJWaWV3IF9uZWVkc1ByZUhUTUw1UGFyc2VyUXVpcmtzXSk6 IFJlbmFtZWQgZnJvbQorICAgICAgICBzaG91bGRVc2VQcmVIVE1MNVBhcnNlclF1aXJrcygpLiBS ZXR1cm4gdHJ1ZSBpZgorICAgICAgICBXZWJDb3JlOjpTZXR0aW5nczo6dXNlc0Rhc2hib2FyZENv bXBhdGliaWxpdHlNb2RlKCkgaXMgdHJ1ZS4KKyAgICAgICAgKC1bV2ViVmlldyBfcHJlZmVyZW5j ZXNDaGFuZ2VkTm90aWZpY2F0aW9uOl0pOgorICAgICAgICAoLVtXZWJWaWV3IF9zZXREYXNoYm9h cmRCZWhhdmlvcjp0bzpdKTogRW5hYmxlIHByZS1IVE1MNSBwYXJzZXIgcXVpcmtzCisgICAgICAg IGlmIERhc2hib2FyZCBiZWhhdmlvciBpcyBzZXQgdG8gYmFja3dhcmQgY29tcGF0aWJpbGl0eSBt b2RlLgorCiAyMDEwLTA5LTIzICBNYXR0aGV3IERlbGFuZXkgIDxtZGVsYW5leUBhcHBsZS5jb20+ CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgU2ltb24gRnJhc2VyLgpJbmRleDogV2ViS2l0L21hYy9X ZWJWaWV3L1dlYlZpZXcubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gV2ViS2l0L21hYy9XZWJWaWV3L1dlYlZp ZXcubW0JKHJldmlzaW9uIDY4MTc1KQorKysgV2ViS2l0L21hYy9XZWJWaWV3L1dlYlZpZXcubW0J KHdvcmtpbmcgY29weSkKQEAgLTYwOSwyNiArNjA5LDYgQEAgc3RhdGljIGJvb2wgY29yZVZpZGVv SGFzNzIyODgzNkZpeCgpCiAgICAgcmV0dXJuIHRydWU7CiB9CiAKLXN0YXRpYyBib29sIHNob3Vs ZFVzZVByZUhUTUw1UGFyc2VyUXVpcmtzKFdlYlByZWZlcmVuY2VzKiBwcmVmZXJlbmNlcykKLXsK LSAgICBzdGF0aWMgYm9vbCB3ZWJLaXRMaW5rZWRCZWZvcmVIVE1MNVBhcnNlciA9ICFXZWJLaXRM aW5rZWRPbk9yQWZ0ZXIoV0VCS0lUX0ZJUlNUX1ZFUlNJT05fV0lUSF9IVE1MNV9QQVJTRVIpOwot ICAgIAotICAgIC8vIEFJTSBjbGllbnRzIGxpbmtlZCBhZ2FpbnN0IHZlcnNpb25zIG9mIFdlYktp dCBwcmlvciB0byB0aGUgaW50cm9kdWN0aW9uCi0gICAgLy8gb2YgdGhlIEhUTUw1IHBhcnNlciBj b250YWluIG1hcmt1cCBpbmNvbXBhdGlibGUgd2l0aCB0aGUgbmV3IHBhcnNlci4KLSAgICAvLyBF bmFibGUgcGFyc2VyIHF1aXJrcyB0byByZW1haW4gY29tcGF0aWJsZSB3aXRoIHRoZXNlIGNsaWVu dHMuIFNlZQotICAgIC8vIDxodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9 NDYxMzQ+LgotICAgIHN0YXRpYyBib29sIGlzQUlNQW5kTmVlZHNQYXJzZXJRdWlya3MgPSBhcHBs aWNhdGlvbklzQU9MSW5zdGFudE1lc3NlbmdlcigpICYmIHdlYktpdExpbmtlZEJlZm9yZUhUTUw1 UGFyc2VyOwotICAgIAotICAgIC8vIE1pY3Jvc29mdCBNeSBEYXkgbG9hZHMgc2NyaXB0cyB1c2lu ZyBzZWxmLWNsb3Npbmcgc2NyaXB0IHRhZ3MsIG1hcmt1cAotICAgIC8vIHdoaWNoIGlzIGluY29t cGF0aWJsZSB3aXRoIHRoZSBIVE1MNSBwYXJzZXIuIEVuYWJsZSBwYXJzZXIgcXVpcmtzIGZvcgot ICAgIC8vIHRoaXMgYXBwbGljYXRpb24uIFNlZSA8aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTQ2MzM0Pi4KLSAgICBzdGF0aWMgYm9vbCBpc01pY3Jvc29mdE15RGF5QW5k TmVlZHNQYXJzZXJRdWlya3MgPSBhcHBsaWNhdGlvbklzTWljcm9zb2Z0TXlEYXkoKSAmJiB3ZWJL aXRMaW5rZWRCZWZvcmVIVE1MNVBhcnNlcjsKLSAgICAKLSAgICByZXR1cm4gaXNBSU1BbmROZWVk c1BhcnNlclF1aXJrcwotICAgICAgICB8fCBpc01pY3Jvc29mdE15RGF5QW5kTmVlZHNQYXJzZXJR dWlya3MKLSAgICAgICAgfHwgW3ByZWZlcmVuY2VzIHVzZVByZUhUTUw1UGFyc2VyUXVpcmtzXTsK LX0KLQogc3RhdGljIGJvb2wgc2hvdWxkRW5hYmxlTG9hZERlZmVycmluZygpCiB7CiAgICAgcmV0 dXJuICFhcHBsaWNhdGlvbklzQWRvYmVJbnN0YWxsZXIoKTsKQEAgLTEzNzgsNiArMTM1OCwzMiBA QCAtIChCT09MKV9uZWVkc0ZyYW1lTG9hZERlbGVnYXRlUmV0YWluUXVpCiAgICAgcmV0dXJuIG5l ZWRzUXVpcms7CiB9CiAKKworLSAoQk9PTClfbmVlZHNQcmVIVE1MNVBhcnNlclF1aXJrcworewor ICAgIHN0YXRpYyBib29sIHdlYktpdExpbmtlZEJlZm9yZUhUTUw1UGFyc2VyID0gIVdlYktpdExp bmtlZE9uT3JBZnRlcihXRUJLSVRfRklSU1RfVkVSU0lPTl9XSVRIX0hUTUw1X1BBUlNFUik7Cisg ICAgCisgICAgLy8gQUlNIGNsaWVudHMgbGlua2VkIGFnYWluc3QgdmVyc2lvbnMgb2YgV2ViS2l0 IHByaW9yIHRvIHRoZSBpbnRyb2R1Y3Rpb24KKyAgICAvLyBvZiB0aGUgSFRNTDUgcGFyc2VyIGNv bnRhaW4gbWFya3VwIGluY29tcGF0aWJsZSB3aXRoIHRoZSBuZXcgcGFyc2VyLgorICAgIC8vIEVu YWJsZSBwYXJzZXIgcXVpcmtzIHRvIHJlbWFpbiBjb21wYXRpYmxlIHdpdGggdGhlc2UgY2xpZW50 cy4gU2VlCisgICAgLy8gPGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD00 NjEzND4uCisgICAgc3RhdGljIGJvb2wgaXNBSU1BbmROZWVkc1BhcnNlclF1aXJrcyA9IGFwcGxp Y2F0aW9uSXNBT0xJbnN0YW50TWVzc2VuZ2VyKCkgJiYgd2ViS2l0TGlua2VkQmVmb3JlSFRNTDVQ YXJzZXI7CisgICAgCisgICAgLy8gTWljcm9zb2Z0IE15IERheSBsb2FkcyBzY3JpcHRzIHVzaW5n IHNlbGYtY2xvc2luZyBzY3JpcHQgdGFncywgbWFya3VwCisgICAgLy8gd2hpY2ggaXMgaW5jb21w YXRpYmxlIHdpdGggdGhlIEhUTUw1IHBhcnNlci4gRW5hYmxlIHBhcnNlciBxdWlya3MgZm9yCisg ICAgLy8gdGhpcyBhcHBsaWNhdGlvbi4gU2VlIDxodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93 X2J1Zy5jZ2k/aWQ9NDYzMzQ+LgorICAgIHN0YXRpYyBib29sIGlzTWljcm9zb2Z0TXlEYXlBbmRO ZWVkc1BhcnNlclF1aXJrcyA9IGFwcGxpY2F0aW9uSXNNaWNyb3NvZnRNeURheSgpICYmIHdlYktp dExpbmtlZEJlZm9yZUhUTUw1UGFyc2VyOworCisgICAgcmV0dXJuIGlzQUlNQW5kTmVlZHNQYXJz ZXJRdWlya3MKKyAgICAgICAgfHwgaXNNaWNyb3NvZnRNeURheUFuZE5lZWRzUGFyc2VyUXVpcmtz CisjaWYgRU5BQkxFKERBU0hCT0FSRF9TVVBQT1JUKQorICAgICAgICAvLyBQcmUtSFRNTDUgcGFy c2VyIHF1aXJrcyBhcmUgcmVxdWlyZWQgdG8gcmVtYWluIGNvbXBhdGlibGUgd2l0aCBtYW55Cisg ICAgICAgIC8vIERhc2hib2FyZCB3aWRnZXRzLiBTZWUgPHJkYXI6Ly9wcm9ibGVtLzgxNzU5ODI+ LgorICAgICAgICB8fCAoX3ByaXZhdGUtPnBhZ2UgJiYgX3ByaXZhdGUtPnBhZ2UtPnNldHRpbmdz KCktPnVzZXNEYXNoYm9hcmRCYWNrd2FyZENvbXBhdGliaWxpdHlNb2RlKCkpCisjZW5kaWYKKyAg ICAgICAgfHwgW1tzZWxmIHByZWZlcmVuY2VzXSB1c2VQcmVIVE1MNVBhcnNlclF1aXJrc107Cit9 CisKIC0gKHZvaWQpX3ByZWZlcmVuY2VzQ2hhbmdlZE5vdGlmaWNhdGlvbjooTlNOb3RpZmljYXRp b24gKilub3RpZmljYXRpb24KIHsKICAgICBXZWJQcmVmZXJlbmNlcyAqcHJlZmVyZW5jZXMgPSAo V2ViUHJlZmVyZW5jZXMgKilbbm90aWZpY2F0aW9uIG9iamVjdF07CkBAIC0xNDc0LDcgKzE0ODAs NyBAQCAtICh2b2lkKV9wcmVmZXJlbmNlc0NoYW5nZWROb3RpZmljYXRpb246CiAjZW5kaWYKICAg ICBzZXR0aW5ncy0+c2V0TWVtb3J5SW5mb0VuYWJsZWQoW3ByZWZlcmVuY2VzIG1lbW9yeUluZm9F bmFibGVkXSk7CiAgICAgc2V0dGluZ3MtPnNldEh5cGVybGlua0F1ZGl0aW5nRW5hYmxlZChbcHJl ZmVyZW5jZXMgaHlwZXJsaW5rQXVkaXRpbmdFbmFibGVkXSk7Ci0gICAgc2V0dGluZ3MtPnNldFVz ZVByZUhUTUw1UGFyc2VyUXVpcmtzKHNob3VsZFVzZVByZUhUTUw1UGFyc2VyUXVpcmtzKHByZWZl cmVuY2VzKSk7CisgICAgc2V0dGluZ3MtPnNldFVzZVByZUhUTUw1UGFyc2VyUXVpcmtzKFtzZWxm IF9uZWVkc1ByZUhUTUw1UGFyc2VyUXVpcmtzXSk7CiAKICAgICAvLyBBcHBsaWNhdGlvbiBDYWNo ZSBQcmVmZXJlbmNlcyBhcmUgc3RvcmVkIG9uIHRoZSBnbG9iYWwgY2FjaGUgc3RvcmFnZSBtYW5h Z2VyLCBub3QgaW4gU2V0dGluZ3MuCiAgICAgW1dlYkFwcGxpY2F0aW9uQ2FjaGUgc2V0RGVmYXVs dE9yaWdpblF1b3RhOltwcmVmZXJlbmNlcyBhcHBsaWNhdGlvbkNhY2hlRGVmYXVsdE9yaWdpblF1 b3RhXV07CkBAIC0yMDA0LDYgKzIwMTAsMTEgQEAgLSAodm9pZClfc2V0RGFzaGJvYXJkQmVoYXZp b3I6KFdlYkRhc2hibwogICAgICAgICAgICAgYnJlYWs7CiAgICAgICAgIH0KICAgICB9CisKKyAg ICAvLyBQcmUtSFRNTDUgcGFyc2VyIHF1aXJrcyBzaG91bGQgYmUgZW5hYmxlZCBpZiBEYXNoYm9h cmQgaXMgaW4gYmFja3dhcmQKKyAgICAvLyBjb21wYXRpYmlsaXR5IG1vZGUuIFNlZSA8cmRhcjov L3Byb2JsZW0vODE3NTk4Mj4uCisgICAgaWYgKF9wcml2YXRlLT5wYWdlKQorICAgICAgICBfcHJp dmF0ZS0+cGFnZS0+c2V0dGluZ3MoKS0+c2V0VXNlUHJlSFRNTDVQYXJzZXJRdWlya3MoW3NlbGYg X25lZWRzUHJlSFRNTDVQYXJzZXJRdWlya3NdKTsKIH0KIAogLSAoQk9PTClfZGFzaGJvYXJkQmVo YXZpb3I6KFdlYkRhc2hib2FyZEJlaGF2aW9yKWJlaGF2aW9yCg==