Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
553263003:
Remove util.platform from Files app. (Closed)
|
Created:
6 years, 3 months ago by mtomasz Modified:
6 years, 3 months ago Reviewers:
hirono CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yoshiki+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRemove util.platform from Files app.
This namespace was used for transition between old apps v1 and v2, which is
completed. There is no need to keep this redundant layer.
TEST=Manually tested Files app, Gallery and a Video player.
BUG=412653
Committed: https://crrev.com/a9ba631052f6309f669bb06906e1872998e220f6
Cr-Commit-Position: refs/heads/master@{#294583}
Patch Set 1 #Patch Set 2 : Cleaned up. #
Total comments: 7
Patch Set 3 : Fixed. #
Total comments: 8
Patch Set 4 : Fixed. #
Messages
Total messages: 18 (2 generated)
mtomasz@chromium.org changed reviewers: + hirono@chromium.org
@hirono: PTAL. Thanks!
https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_manager.js:231: chrome.storage.local.get([this.startupPrefName_], function(values) { We can pass a single key as a string. https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/gallery/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/gallery/... ui/file_manager/gallery/js/slide_mode.js:131: chrome.storage.local.get([SlideMode.OVERWRITE_KEY], function(values) { We can pass a single key as a string. https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/gallery/... ui/file_manager/gallery/js/slide_mode.js:135: (value === undefined || value) ? true : false; It looks the value can be 'false'. Could you double check? https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/gallery/... ui/file_manager/gallery/js/slide_mode.js:733: chrome.storage.local.get([SlideMode.OVERWRITE_BUBBLE_KEY], We can pass a single key as a string.
https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_manager.js:231: chrome.storage.local.get([this.startupPrefName_], function(values) { On 2014/09/10 13:03:39, hirono wrote: > We can pass a single key as a string. Done. https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/gallery/... File ui/file_manager/gallery/js/slide_mode.js (right): https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/gallery/... ui/file_manager/gallery/js/slide_mode.js:135: (value === undefined || value) ? true : false; On 2014/09/10 13:03:40, hirono wrote: > It looks the value can be 'false'. Could you double check? Done. https://codereview.chromium.org/553263003/diff/20001/ui/file_manager/gallery/... ui/file_manager/gallery/js/slide_mode.js:733: chrome.storage.local.get([SlideMode.OVERWRITE_BUBBLE_KEY], On 2014/09/10 13:03:40, hirono wrote: > We can pass a single key as a string. Done.
https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:661: chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { chrome.storage.local.get?
https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:661: chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { On 2014/09/11 12:40:37, hirono wrote: > chrome.storage.local.get? Done.
https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:661: chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { On 2014/09/12 08:35:09, mtomasz wrote: > On 2014/09/11 12:40:37, hirono wrote: > > chrome.storage.local.get? > > Done. |json| is object. Maybe we don't need to parse it.
https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:661: chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { On 2014/09/12 08:50:21, hirono wrote: > On 2014/09/12 08:35:09, mtomasz wrote: > > On 2014/09/11 12:40:37, hirono wrote: > > > chrome.storage.local.get? > > > > Done. > > |json| is object. Maybe we don't need to parse it. Hm. It should be string. We stringify the object in #680.
https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:661: chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { On 2014/09/12 08:52:23, mtomasz wrote: > On 2014/09/12 08:50:21, hirono wrote: > > On 2014/09/12 08:35:09, mtomasz wrote: > > > On 2014/09/11 12:40:37, hirono wrote: > > > > chrome.storage.local.get? > > > > > > Done. > > > > |json| is object. Maybe we don't need to parse it. > > Hm. It should be string. We stringify the object in #680. If we specified a single key, it returns the value of key? or return the object having a single value?
https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:661: chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { On 2014/09/12 08:55:05, hirono wrote: > On 2014/09/12 08:52:23, mtomasz wrote: > > On 2014/09/12 08:50:21, hirono wrote: > > > On 2014/09/12 08:35:09, mtomasz wrote: > > > > On 2014/09/11 12:40:37, hirono wrote: > > > > > chrome.storage.local.get? > > > > > > > > Done. > > > > > > |json| is object. Maybe we don't need to parse it. > > > > Hm. It should be string. We stringify the object in #680. > > If we specified a single key, it returns the value of key? or return the object > having a single value? It always returns a map. So here 'values' would be {util.AppCache.Key: JSON.stringify(map)); PTAL at the recent patch. # chrome.storage.local.set({AppCache: 'JSON'}); # chrome.storage.local.get(['AppCache'], function(v) { console.log(v); }); Object {AppCache: "JSON"} # chrome.storage.local.get('AppCache', function(v) { console.log(v); }); Object {AppCache: "JSON"}
https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:661: chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { On 2014/09/12 09:06:43, mtomasz wrote: > On 2014/09/12 08:55:05, hirono wrote: > > On 2014/09/12 08:52:23, mtomasz wrote: > > > On 2014/09/12 08:50:21, hirono wrote: > > > > On 2014/09/12 08:35:09, mtomasz wrote: > > > > > On 2014/09/11 12:40:37, hirono wrote: > > > > > > chrome.storage.local.get? > > > > > > > > > > Done. > > > > > > > > |json| is object. Maybe we don't need to parse it. > > > > > > Hm. It should be string. We stringify the object in #680. > > > > If we specified a single key, it returns the value of key? or return the > object > > having a single value? > > It always returns a map. So here 'values' would be {util.AppCache.Key: > JSON.stringify(map)); PTAL at the recent patch. > > # chrome.storage.local.set({AppCache: 'JSON'}); > # chrome.storage.local.get(['AppCache'], function(v) { console.log(v); }); > Object {AppCache: "JSON"} > # chrome.storage.local.get('AppCache', function(v) { console.log(v); }); > Object {AppCache: "JSON"} Sorry for less description. So JSON.parse(json) at #664 returns an error, because json is a map.
https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/common/js/util.js (right): https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/common/js/util.js:661: chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { On 2014/09/12 09:10:33, hirono wrote: > On 2014/09/12 09:06:43, mtomasz wrote: > > On 2014/09/12 08:55:05, hirono wrote: > > > On 2014/09/12 08:52:23, mtomasz wrote: > > > > On 2014/09/12 08:50:21, hirono wrote: > > > > > On 2014/09/12 08:35:09, mtomasz wrote: > > > > > > On 2014/09/11 12:40:37, hirono wrote: > > > > > > > chrome.storage.local.get? > > > > > > > > > > > > Done. > > > > > > > > > > |json| is object. Maybe we don't need to parse it. > > > > > > > > Hm. It should be string. We stringify the object in #680. > > > > > > If we specified a single key, it returns the value of key? or return the > > object > > > having a single value? > > > > It always returns a map. So here 'values' would be {util.AppCache.Key: > > JSON.stringify(map)); PTAL at the recent patch. > > > > # chrome.storage.local.set({AppCache: 'JSON'}); > > # chrome.storage.local.get(['AppCache'], function(v) { console.log(v); }); > > Object {AppCache: "JSON"} > > # chrome.storage.local.get('AppCache', function(v) { console.log(v); }); > > Object {AppCache: "JSON"} > > Sorry for less description. > So JSON.parse(json) at #664 returns an error, because json is a map. I think you're looking at old patchset. In the newest one we do. JSON.parse(values[util.AppCache.KEY]. Or I'm missing something.
On 2014/09/12 09:11:48, mtomasz wrote: > https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... > File ui/file_manager/file_manager/common/js/util.js (right): > > https://codereview.chromium.org/553263003/diff/40001/ui/file_manager/file_man... > ui/file_manager/file_manager/common/js/util.js:661: > chrome.storage.local.getPreference(util.AppCache.KEY, function(json) { > On 2014/09/12 09:10:33, hirono wrote: > > On 2014/09/12 09:06:43, mtomasz wrote: > > > On 2014/09/12 08:55:05, hirono wrote: > > > > On 2014/09/12 08:52:23, mtomasz wrote: > > > > > On 2014/09/12 08:50:21, hirono wrote: > > > > > > On 2014/09/12 08:35:09, mtomasz wrote: > > > > > > > On 2014/09/11 12:40:37, hirono wrote: > > > > > > > > chrome.storage.local.get? > > > > > > > > > > > > > > Done. > > > > > > > > > > > > |json| is object. Maybe we don't need to parse it. > > > > > > > > > > Hm. It should be string. We stringify the object in #680. > > > > > > > > If we specified a single key, it returns the value of key? or return the > > > object > > > > having a single value? > > > > > > It always returns a map. So here 'values' would be {util.AppCache.Key: > > > JSON.stringify(map)); PTAL at the recent patch. > > > > > > # chrome.storage.local.set({AppCache: 'JSON'}); > > > # chrome.storage.local.get(['AppCache'], function(v) { console.log(v); }); > > > Object {AppCache: "JSON"} > > > # chrome.storage.local.get('AppCache', function(v) { console.log(v); }); > > > Object {AppCache: "JSON"} > > > > Sorry for less description. > > So JSON.parse(json) at #664 returns an error, because json is a map. > > I think you're looking at old patchset. In the newest one we do. > JSON.parse(values[util.AppCache.KEY]. Or I'm missing something. Oops, yes. I saw the old one. lgtm!
Thanks!
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/553263003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as 239b44346ae935a60fe83361ad78f31902060f58
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a9ba631052f6309f669bb06906e1872998e220f6 Cr-Commit-Position: refs/heads/master@{#294583} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
