Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2138183002:
[ios] Added cr.isIOS function to use in WebUI pages. (Closed)
|
Created:
4 years, 5 months ago by Eugene But (OOO till 7-30) Modified:
4 years, 5 months ago Reviewers:
dpapad CC:
chromium-reviews, oshima+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Added cr.isIOS function to use in WebUI pages.
BUG=566163
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/80b0ad017d10c202838700f768f7ec3ed439564c
Cr-Commit-Position: refs/heads/master@{#405180}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed review comment #
Total comments: 4
Patch Set 3 : Account for iPod hardware #Patch Set 4 : Added missing comma, s/navigator.userAgent/navigator.platform #Messages
Total messages: 21 (8 generated)
Description was changed from ========== [ios] Added cr.isIOS function to use in WebUI pages. BUG=566163 ========== to ========== [ios] Added cr.isIOS function to use in WebUI pages. BUG=566163 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
eugenebut@chromium.org changed reviewers: + dpapad@chromium.org
https://codereview.chromium.org/2138183002/diff/1/ui/webui/resources/js/cr.js File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/2138183002/diff/1/ui/webui/resources/js/cr.js... ui/webui/resources/js/cr.js:482: return navigator.platform === 'iPhone' || navigator.platform === 'iPad'; Can we make these comparisons a bit less fragile as follows? return /(iPhone|iPad)/.test(navigator.platform); This will match even if the text is not exactly the same (for example if there is whitespace after 'iPhone').
https://codereview.chromium.org/2138183002/diff/1/ui/webui/resources/js/cr.js File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/2138183002/diff/1/ui/webui/resources/js/cr.js... ui/webui/resources/js/cr.js:482: return navigator.platform === 'iPhone' || navigator.platform === 'iPad'; On 2016/07/11 20:45:33, dpapad wrote: > Can we make these comparisons a bit less fragile as follows? > > return /(iPhone|iPad)/.test(navigator.platform); > > This will match even if the text is not exactly the same (for example if there > is whitespace after 'iPhone'). Done.
lgtm
https://codereview.chromium.org/2138183002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/2138183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:482: return /(iPhone|iPad)/.test(navigator.platform); why do you need capturing parens? /iPhone|iPad/ should be fine
https://codereview.chromium.org/2138183002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/2138183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:482: return /(iPhone|iPad)/.test(navigator.platform); On 2016/07/11 21:49:05, Dan Beam wrote: > why do you need capturing parens? > > /iPhone|iPad/ should be fine also, do we run on iPod? mine had that in it ;) https://codereview.chromium.org/2103883005/
Thanks everyone! https://codereview.chromium.org/2138183002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/2138183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:482: return /(iPhone|iPad)/.test(navigator.platform); On 2016/07/11 21:50:15, Dan Beam wrote: > On 2016/07/11 21:49:05, Dan Beam wrote: > > why do you need capturing parens? > > > > /iPhone|iPad/ should be fine > > also, do we run on iPod? mine had that in it ;) > https://codereview.chromium.org/2103883005/ Good catch. https://codereview.chromium.org/2138183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:482: return /(iPhone|iPad)/.test(navigator.platform); On 2016/07/11 21:49:05, Dan Beam wrote: > why do you need capturing parens? > > /iPhone|iPad/ should be fine Done.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2138183002/#ps40001 (title: "Account for iPod hardware")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: closure_compilation on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2138183002/#ps60001 (title: "Added missing comma, s/navigator.userAgent/navigator.platform")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [ios] Added cr.isIOS function to use in WebUI pages. BUG=566163 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== [ios] Added cr.isIOS function to use in WebUI pages. BUG=566163 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/80b0ad017d10c202838700f768f7ec3ed439564c Cr-Commit-Position: refs/heads/master@{#405180} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/80b0ad017d10c202838700f768f7ec3ed439564c Cr-Commit-Position: refs/heads/master@{#405180} |
