Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
181233007:
kiosk: Fix network check skipped regression. (Closed)
|
Created:
6 years, 9 months ago by xiyuan Modified:
6 years, 9 months ago CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org, miket_OOO, zel Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionkiosk: Fix network check skipped regression.
Fix the the regression by providing a more reasonable default value
for platform apps' default offline_enabled value: Apps default to
being offline enabled unless webview permission is requested.
BUG=349200, 350129
TEST=ExtensionManifestOfflineEnabledTest.* and KioskTest.*LaunchAppNetworkDown
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255988
Patch Set 1 #
Total comments: 4
Patch Set 2 : offline_enabled default value from webview permission #
Total comments: 13
Patch Set 3 : for document comments #Patch Set 4 : rebase #Patch Set 5 : fix win compile #
Messages
Total messages: 24 (0 generated)
https://codereview.chromium.org/181233007/diff/1/chrome/browser/chromeos/app_... File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right): https://codereview.chromium.org/181233007/diff/1/chrome/browser/chromeos/app_... chrome/browser/chromeos/app_mode/startup_app_launcher.cc:156: // true in its manifest. Is this a good idea? It's giving a lot of subtlety to the offline_enabled key. Would something more explicit like a new and separate "requires_network_check" key be less surprising?
lgtm https://codereview.chromium.org/181233007/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/kiosk_browsertest.cc (right): https://codereview.chromium.org/181233007/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/kiosk_browsertest.cc:512: ->OnConfigureNetwork(); nit: indent
https://codereview.chromium.org/181233007/diff/1/chrome/browser/chromeos/app_... File chrome/browser/chromeos/app_mode/startup_app_launcher.cc (right): https://codereview.chromium.org/181233007/diff/1/chrome/browser/chromeos/app_... chrome/browser/chromeos/app_mode/startup_app_launcher.cc:156: // true in its manifest. On 2014/03/05 18:32:22, miket wrote: > Is this a good idea? It's giving a lot of subtlety to the offline_enabled key. > Would something more explicit like a new and separate "requires_network_check" > key be less surprising? Agree that having a new key would be more straightforward. But think we still needs this CL to fix M34 out there for not breaking the existing behavior.
CL is changed to use a different approach, using a more reasonable default offline_enabled value for platform apps. Please take another look. Thanks. https://codereview.chromium.org/181233007/diff/1/chrome/browser/chromeos/logi... File chrome/browser/chromeos/login/kiosk_browsertest.cc (right): https://codereview.chromium.org/181233007/diff/1/chrome/browser/chromeos/logi... chrome/browser/chromeos/login/kiosk_browsertest.cc:512: ->OnConfigureNetwork(); On 2014/03/05 18:36:34, Tim Song wrote: > nit: indent Done.
I thikn you may want to update the CL description and maybe add another BUG link? Anyway, thanks, this is great, tentative lgtm pending documentation comments. Resolve as many as you see fit, but it's a little bit subtle so I'd like to make it decently polished. I've also pinged Meggin to have a proper look at it since she will have better high level comments, but I think you shouldn't wait for that to submit. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html (right): https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:8: you should probably preface this somewhere with it being Chrome 35 onwards. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:10: Default values are provided for apps that do not speicify <code>"offline_enabled"</code> specify, though I think that you don't need this first sentence at all, it reads better without it. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:19: network connectivie check will be performed when launching an app in ChromeOS connectivity https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:20: kiosk mode. A network connectivity check will be performed when <code>"offline_enabled"</code> I'd leave out the "Chrome OS" part, but, up to you. I'd definitely link to http://developer.chrome.com/apps/manifest/kiosk_enabled though. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:21: is <code>false</code> and the app launching is on hold until the device obtains This might give the false impression that you actually need to set this value. I'd just say "A network connectivity check will be performed when apps are not offline enabled, and app launching put on hold..." https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:23: </p> Meggin could you go over this when you have time? The existing content isn't very good. https://codereview.chromium.org/181233007/diff/40001/extensions/common/manife... File extensions/common/manifest_handlers/offline_enabled_info.cc (right): https://codereview.chromium.org/181233007/diff/40001/extensions/common/manife... extensions/common/manifest_handlers/offline_enabled_info.cc:43: // A platform apps is offline enabled unless it requests the webview apps -> app
CL description updated to what the change does and added a bug to track the default offline_enabled value change. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html (right): https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:8: On 2014/03/06 22:39:15, kalman wrote: > you should probably preface this somewhere with it being Chrome 35 onwards. Done. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:10: Default values are provided for apps that do not speicify <code>"offline_enabled"</code> On 2014/03/06 22:39:15, kalman wrote: > specify, though I think that you don't need this first sentence at all, it reads > better without it. Removed. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:19: network connectivie check will be performed when launching an app in ChromeOS On 2014/03/06 22:39:15, kalman wrote: > connectivity Done. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:20: kiosk mode. A network connectivity check will be performed when <code>"offline_enabled"</code> On 2014/03/06 22:39:15, kalman wrote: > I'd leave out the "Chrome OS" part, but, up to you. > > I'd definitely link to http://developer.chrome.com/apps/manifest/kiosk_enabled > though. We need to document this somewhere. I'll keep it here for now. Link added. https://codereview.chromium.org/181233007/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/manifest/offline_enabled.html:21: is <code>false</code> and the app launching is on hold until the device obtains On 2014/03/06 22:39:15, kalman wrote: > This might give the false impression that you actually need to set this value. > I'd just say "A network connectivity check will be performed when apps are not > offline enabled, and app launching put on hold..." Done. https://codereview.chromium.org/181233007/diff/40001/extensions/common/manife... File extensions/common/manifest_handlers/offline_enabled_info.cc (right): https://codereview.chromium.org/181233007/diff/40001/extensions/common/manife... extensions/common/manifest_handlers/offline_enabled_info.cc:43: // A platform apps is offline enabled unless it requests the webview On 2014/03/06 22:39:15, kalman wrote: > apps -> app Done.
The CQ bit was checked by xiyuan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xiyuan@chromium.org/181233007/100001
The CQ bit was checked by xiyuan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xiyuan@chromium.org/181233007/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by xiyuan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xiyuan@chromium.org/181233007/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by xiyuan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xiyuan@chromium.org/181233007/110001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xiyuan@chromium.org/181233007/110001
Message was sent while issue was closed.
Change committed as 255988
Message was sent while issue was closed.
lgtm... I also reviewed the manifest doc http://developer.chrome.com/apps/manifest/kiosk_enabled#kiosk_enabled. Do we explain what ChromeOS kiosk mode is (somewhere)?
Message was sent while issue was closed.
On 2014/03/10 20:30:57, mkearney1 wrote: > lgtm... > > I also reviewed the manifest doc > http://developer.chrome.com/apps/manifest/kiosk_enabled#kiosk_enabled. > > Do we explain what ChromeOS kiosk mode is (somewhere)? I don't recall we have such doc in the app document. I think Zel/Vidya put up the documents on supports center: e.g. https://support.google.com/chromebook/answer/3134673?hl=en
Message was sent while issue was closed.
On 2014/03/10 20:38:18, xiyuan wrote: > On 2014/03/10 20:30:57, mkearney1 wrote: > > lgtm... > > > > I also reviewed the manifest doc > > http://developer.chrome.com/apps/manifest/kiosk_enabled#kiosk_enabled. > > > > Do we explain what ChromeOS kiosk mode is (somewhere)? > > I don't recall we have such doc in the app document. I think Zel/Vidya put up > the documents on supports center: > e.g. > https://support.google.com/chromebook/answer/3134673?hl=en Great, thanks. Just wanted to make sure there's something describing it. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
