Bug 195597 - Remove MediaWiki site specific quirks
Summary: Remove MediaWiki site specific quirks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-11 19:37 PDT by Ryosuke Niwa
Modified: 2019-03-11 23:27 PDT (History)
9 users (show)

See Also:


Attachments
cleanup (7.01 KB, patch)
2019-03-11 20:08 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (7.58 KB, patch)
2019-03-11 22:15 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2019-03-11 19:37:33 PDT
A site specific quirk was added to workaround KHTML workaround in MediaWiki in https://bugs.webkit.org/show_bug.cgi?id=28350
Chromium removed this workaround way back in 2013: https://github.com/chromium/chromium/commit/ecf84fc9c1a51c8ede7adfd0b0cba446d9a8caa0

It's 2019 now. Six years should be more than enough buffer for us to follow the suit.
Comment 1 Ryosuke Niwa 2019-03-11 20:08:31 PDT
Created attachment 364341 [details]
cleanup
Comment 2 Simon Fraser (smfr) 2019-03-11 20:18:00 PDT
Comment on attachment 364341 [details]
cleanup

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

> Source/WebCore/css/parser/CSSParserContext.cpp:-82
> -    needsSiteSpecificQuirks = true;

Yay for removing this!
Comment 3 Ryosuke Niwa 2019-03-11 20:23:30 PDT
Waiting for EWS...
Comment 4 Daniel Bates 2019-03-11 20:31:16 PDT
Comment on attachment 364341 [details]
cleanup

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

I got it. You want a pat on the back. Good job.

> Source/WebCore/ChangeLog:8
> +        In the afternoon of the bright sunny [1] seventeenth day of August in the year two thousand and nine,

Why are you writing like this? It’s not fun or funny in my eyes, but maybe it will make someone laugh (not counting yourself) :/ Also when I read it the first thought I had was Ryosuke is trying to be sarcastic, but as I continued reading I was liked, yeah doesn’t seem sarcastic maybe he’s trying to be funny. Didn’t laugh though.

> Source/WebCore/ChangeLog:11
> +        Blink since removed this workaround on the seventh day of November in the year two thousand thirteen:

Talk about beating a dead horse......

> Source/WebCore/ChangeLog:15
> +        workaround obsolete and unnecessary for the Web compatibility.

It’s a joke. Guess I am a tough crowd because I didn’t find this funny. :/

> Source/WebCore/ChangeLog:17
> +        [1] https://www.wunderground.com/history/daily/KNUQ/date/2009-8-17?reqdb.zip=95014

I don’t get it. Is this a joke?

> Source/WebCore/css/parser/CSSParserContext.cpp:-63
> -    needsSiteSpecificQuirks = document.settings().needsSiteSpecificQuirks();

No way! Sorry, I don’t believe you. As much as I want to get rid of this, your joke and the linked Blink commit don’t support seem to support the premise that you can remove this or maybe the joke just left a very bad taste in my mouth. No, it was both.

>> Source/WebCore/css/parser/CSSParserContext.cpp:-82
>> -    needsSiteSpecificQuirks = true;
> 
> Yay for removing this!

Ok, Simon is convinced. That’s good enough.
Comment 5 Ryosuke Niwa 2019-03-11 22:15:43 PDT
Created attachment 364351 [details]
Patch for landing
Comment 6 Ryosuke Niwa 2019-03-11 22:16:00 PDT
Comment on attachment 364351 [details]
Patch for landing

Wait for EWS.
Comment 7 Daniel Bates 2019-03-11 22:42:34 PDT
Comment on attachment 364351 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:15
> +        workaround obsolete and unnecessary for the Web compatibility.

Not grammatically correct. "the Web"?
Comment 8 Daniel Bates 2019-03-11 22:44:04 PDT
Comment on attachment 364351 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:9
> +        we added a site specific quirk for the KHTML workaround in MediaWiki in the WebKit revision 47383.

Not grammatically correct. "the WebKit"?
Comment 9 Daniel Bates 2019-03-11 22:44:41 PDT
oh, this is going to be fun.
Comment 10 Daniel Bates 2019-03-11 22:51:14 PDT
Comment on attachment 364351 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:8
> +        In the afternoon of the bright sunny [1] seventeenth day of August in the year two thousand and nine,

of the? Doesn't sound correct.
Comment 11 Ryosuke Niwa 2019-03-11 23:00:08 PDT
Dan, please stop making further comments on this bug. Your behavior is at least passive aggressive if not outright offensive.
Comment 12 Daniel Bates 2019-03-11 23:07:47 PDT
(In reply to Ryosuke Niwa from comment #11)
> Dan, please stop making further comments on this bug. Your behavior is at
> least passive aggressive if not outright offensive.

I apologize if it came off this way. It wasn't my intention to do either. I just wanted to help ensure that the commit message was accurate because it doesn't correspond to the code change or explain why this code change is correct. For comparison, the Blink commit you referenced has an understandable commit message that is written in layman terminology that clearly identifies the reason for the code change. Your change goes above and beyond the Blink change and isn't very clear. I don't understand why we need to try to make jokes in the ChangeLog instead of just clearly stating the reason for the change and especially for the bits that aren't corresponding to the Blink change.
Comment 13 Ryosuke Niwa 2019-03-11 23:10:00 PDT
Committed r242780: <https://trac.webkit.org/changeset/242780>
Comment 14 Radar WebKit Bug Importer 2019-03-11 23:10:18 PDT
<rdar://problem/48798911>
Comment 15 Daniel Bates 2019-03-11 23:18:39 PDT
(In reply to Ryosuke Niwa from comment #13)
> Committed r242780: <https://trac.webkit.org/changeset/242780>

Thank you, Ryosuke, for updating the ChangeLog. It reads well and clearly describes both the reason for the quirk and the reason you feel confident about removing it. It is in my opinion 1000x better than the original.
Comment 16 Ryosuke Niwa 2019-03-11 23:19:09 PDT
(In reply to Daniel Bates from comment #12)
> (In reply to Ryosuke Niwa from comment #11)
> > Dan, please stop making further comments on this bug. Your behavior is at
> > least passive aggressive if not outright offensive.
> 
> I apologize if it came off this way. It wasn't my intention to do either. I
> just wanted to help ensure that the commit message was accurate because it
> doesn't correspond to the code change or explain why this code change is
> correct. For comparison, the Blink commit you referenced has an
> understandable commit message that is written in layman terminology that
> clearly identifies the reason for the code change. Your change goes above
> and beyond the Blink change and isn't very clear. I don't understand why we
> need to try to make jokes in the ChangeLog instead of just clearly stating
> the reason for the change and especially for the bits that aren't
> corresponding to the Blink change.

If that were your intention, please state from the get-go instead of keep making sneaky & passive aggressive comments in the future. FWIW, I received them as personal attacks.
Comment 17 Daniel Bates 2019-03-11 23:27:52 PDT
(In reply to Ryosuke Niwa from comment #16)
> (In reply to Daniel Bates from comment #12)
> > (In reply to Ryosuke Niwa from comment #11)
> > > Dan, please stop making further comments on this bug. Your behavior is at
> > > least passive aggressive if not outright offensive.
> > 
> > I apologize if it came off this way. It wasn't my intention to do either. I
> > just wanted to help ensure that the commit message was accurate because it
> > doesn't correspond to the code change or explain why this code change is
> > correct. For comparison, the Blink commit you referenced has an
> > understandable commit message that is written in layman terminology that
> > clearly identifies the reason for the code change. Your change goes above
> > and beyond the Blink change and isn't very clear. I don't understand why we
> > need to try to make jokes in the ChangeLog instead of just clearly stating
> > the reason for the change and especially for the bits that aren't
> > corresponding to the Blink change.
> 
> If that were your intention, please state from the get-go instead of keep
> making sneaky & passive aggressive comments in the future. FWIW, I received
> them as personal attacks.

I will keep this in mind moving forward. Never meant for my remarks to be taken as an attack.