Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 6668950364487680: Issue 1593 - Extend workaround for Chrome bug breaking Flash videos to Chrome >=38 (Closed)

Created:
Nov. 22, 2014, 12:19 p.m. by Sebastian Noack
Modified:
Nov. 25, 2014, 5:04 p.m.
Reviewers:
kzar, Wladimir Palant
Visibility:
Public.

Patch Set 1 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -3 lines) Patch
M chrome/ext/background.js View 1 chunk +3 lines, -3 lines 2 comments Download

Messages

Total messages: 4
Sebastian Noack
Nov. 22, 2014, 12:19 p.m. (2014-11-22 12:19:44 UTC) #1
kzar
On 2014/11/22 12:19:44, Sebastian Noack wrote: LGTM
Nov. 24, 2014, 4:39 p.m. (2014-11-24 16:39:34 UTC) #2
Wladimir Palant
http://codereview.adblockplus.org/6668950364487680/diff/5741031244955648/chrome/ext/background.js File chrome/ext/background.js (right): http://codereview.adblockplus.org/6668950364487680/diff/5741031244955648/chrome/ext/background.js#newcode325 chrome/ext/background.js:325: if (requestType == "other" && parseInt(navigator.userAgent.match(/\bChrome\/(\d+)/)[1], 10) >= 38) ...
Nov. 25, 2014, 4:45 p.m. (2014-11-25 16:45:50 UTC) #3
Sebastian Noack
Nov. 25, 2014, 5:04 p.m. (2014-11-25 17:04:04 UTC) #4
http://codereview.adblockplus.org/6668950364487680/diff/5741031244955648/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/6668950364487680/diff/5741031244955648/chro...
chrome/ext/background.js:325: if (requestType == "other" &&
parseInt(navigator.userAgent.match(/\bChrome\/(\d+)/)[1], 10) >= 38)
On 2014/11/25 16:45:51, Wladimir Palant wrote:
> This will error out if navigator.userAgent.match() returns null (shouldn't
> happen too often but is possible).

Well, in that case parseInt() will return NaN, and |NaN >= 38| returns false.
Therefore this code really is the same as:

  var match = navigator.userAgent.match(/\bChrome\/(\d+)/)[1], 10);
  if (match && parseInt(match[0], 10) >= 38)

> Given that we are in the background page here, why not do
> |Services.vc.compare(require("info").applicationVersion, "38.0") >= 0|?

Because we are in the abstraction layer. That's one reason why I would like to
convert ext into CommonJS modules bundled with our other code, that we can
easier reuse those functions.

Powered by Google App Engine
This is Rietveld