Issue 29754564: Noissue - Updated requirement for $webrtc filters (Closed)
|
Created:
April 17, 2018, 3:06 p.m. by Sebastian Noack Modified:
April 17, 2018, 4:45 p.m. Reviewers:
kzar CC:
juliandoucette Visibility:
Public. |
DescriptionNoissue - Updated requirement for $webrtc filters
Patch Set 1 #
Total comments: 4
MessagesTotal messages: 5
* The Web Extension migration (3.0 release), brought $webrtc filters to Firefox as a side effect. * Also it is <extension> <version> for <browser> (not <extension> for <browser> <version>, this would mean that its for that version of that browser). * Also I didn't care to mention lack of support on Safari anymore. It's no longer a relevant platform, and inconsistent with other new filters that don't mention lack of support for Safari either.
Otherwise LGTM https://codereview.adblockplus.org/29754564/diff/29754565/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29754564/diff/29754565/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — connections opened via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> instances</a> to ICE servers (Adblock Plus 1.13.3 for Chrome and Opera, 3.0 for Firefox, or higher required)}}</li> Nit: I think the last comma could be removed "...Firefox or higher required"
https://codereview.adblockplus.org/29754564/diff/29754565/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29754564/diff/29754565/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — connections opened via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> instances</a> to ICE servers (Adblock Plus 1.13.3 for Chrome and Opera, 3.0 for Firefox, or higher required)}}</li> On 2018/04/17 15:15:28, kzar wrote: > Nit: I think the last comma could be removed "...Firefox or higher required" Wouldn't it read like "exactly 1.13.3 for Chrome/Opera" || "3.0 or higher for Firefox" then (or could at least be misunderstood like that)?
https://codereview.adblockplus.org/29754564/diff/29754565/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29754564/diff/29754565/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — connections opened via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> instances</a> to ICE servers (Adblock Plus 1.13.3 for Chrome and Opera, 3.0 for Firefox, or higher required)}}</li> On 2018/04/17 15:18:49, Sebastian Noack wrote: > On 2018/04/17 15:15:28, kzar wrote: > > Nit: I think the last comma could be removed "...Firefox or higher required" > > Wouldn't it read like "exactly 1.13.3 for Chrome/Opera" || "3.0 or higher for > Firefox" then (or could at least be misunderstood like that)? Well if this was code I would have added the comma, but I feel like here it reads better without it. But I don't feel too strongly about this either way.
https://codereview.adblockplus.org/29754564/diff/29754565/pages/filters.html File pages/filters.html (right): https://codereview.adblockplus.org/29754564/diff/29754565/pages/filters.html#... pages/filters.html:155: <li>{{webrtc <code><fix>webrtc</fix></code> — connections opened via <a href="https://developer.mozilla.org/docs/Web/API/RTCPeerConnection"><code><fix>RTCPeerConnection</fix></code> instances</a> to ICE servers (Adblock Plus 1.13.3 for Chrome and Opera, 3.0 for Firefox, or higher required)}}</li> On 2018/04/17 15:24:06, kzar wrote: > On 2018/04/17 15:18:49, Sebastian Noack wrote: > > On 2018/04/17 15:15:28, kzar wrote: > > > Nit: I think the last comma could be removed "...Firefox or higher required" > > > > Wouldn't it read like "exactly 1.13.3 for Chrome/Opera" || "3.0 or higher for > > Firefox" then (or could at least be misunderstood like that)? > > Well if this was code I would have added the comma, but I feel like here it > reads better without it. But I don't feel too strongly about this either way. Well, code is parsed by computers, language is parsed by humans. While human language has less strict rules, there can still be ambiguity, which in this case is eliminated by this comma. FWIW, I asked another native English speaker, with writing experience, who also agreed to the comma here. Anyway, since you don't feel strong, I will push the change, now. 
|
||||||||||||||||||||||||||||||||||||||
