Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 10067021: Postpone setting up file handler's file permissions if handler is running lazy background page. (Closed)

Created:
6 years, 3 months ago by tbarzic
Modified:
3 weeks, 2 days ago
CC:
chromium-reviews, nkostylev+watch_chromium.org, achuith+watch_chromium.org, mihaip+watch_chromium.org, Aaron Boodman, rginda+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org
Visibility:
Public.

Description

Postpone setting up file handler's file permissions if handler is running lazy background page. We have to wait until handler's extension host loads before we can setup file access permissions with ChilsProcessSecurityPolicy. We can't get the extensions host process id before that. BUG=chromium-os:29475 TEST=*FileBrowser* Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=132458

Patch Set 1 #

Patch Set 2 : cleaned up a bit and working #

Patch Set 3 : some more cleanup #

Patch Set 4 : . #

Total comments: 9

Patch Set 5 : nits #

Total comments: 4

Patch Set 6 : use lazy background task queue directly #

Patch Set 7 : some nits #

Patch Set 8 : test #

Patch Set 9 : . #

Patch Set 10 : cleanup #

Patch Set 11 : . #

Patch Set 12 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+396 lines, -43 lines) Patch
M chrome/browser/chromeos/extensions/external_filesystem_apitest.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +57 lines, -1 line 0 comments Download
M chrome/browser/chromeos/extensions/file_browser_private_api.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_browser_private_api.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +18 lines, -4 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_handler_util.h View 1 2 3 4 5 3 chunks +24 lines, -5 lines 0 comments Download
M chrome/browser/chromeos/extensions/file_handler_util.cc View 1 2 3 4 5 6 7 chunks +82 lines, -25 lines 0 comments Download
M chrome/browser/chromeos/gdata/gdata_util.h View 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/gdata/gdata_util.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +16 lines, -8 lines 0 comments Download
A chrome/test/data/extensions/api_test/filesystem_handler_lazy_background/background.js View 1 2 3 4 5 6 7 8 9 1 chunk +79 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/filesystem_handler_lazy_background/manifest.json View 1 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/filesystem_handler_lazy_background/tab.html View 1 2 3 4 1 chunk +7 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/filesystem_handler_lazy_background/tab.js View 1 2 3 4 1 chunk +64 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
tbarzic
6 years, 3 months ago (2012-04-13 02:28:24 UTC) #1
tbarzic
http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/extensions/external_filesystem_apitest.cc File chrome/browser/chromeos/extensions/external_filesystem_apitest.cc (right): http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/extensions/external_filesystem_apitest.cc#newcode122 chrome/browser/chromeos/extensions/external_filesystem_apitest.cc:122: void SetUpCommandLine(CommandLine* command_line) { I messed up indent here.. ...
6 years, 3 months ago (2012-04-13 02:58:30 UTC) #2
dgozman
I suspect that filesystem_handler_lazy_background is identical to filesystem_handler (except for manifest change)? http://codereview.chromium.org/10067021/diff/6001/chrome/browser/chromeos/extensions/file_handler_util.cc File chrome/browser/chromeos/extensions/file_handler_util.cc ...
6 years, 3 months ago (2012-04-13 12:41:04 UTC) #3
tbarzic
Test is not identical to filesystem_handler one (I made some modifications to make it work ...
6 years, 3 months ago (2012-04-13 16:07:58 UTC) #4
dgozman
This looks good to me, but wait for Vlad or Matt to review (I'm not ...
6 years, 3 months ago (2012-04-13 17:49:36 UTC) #5
Matt Perry
http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/extensions/file_browser_private_api.cc File chrome/browser/chromeos/extensions/file_browser_private_api.cc (right): http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/extensions/file_browser_private_api.cc#newcode579 chrome/browser/chromeos/extensions/file_browser_private_api.cc:579: class ExecuteTasksFileBrowserFunction::Executor: public FileTaskExecutor { space before last : ...
6 years, 3 months ago (2012-04-13 18:45:16 UTC) #6
Matt Perry
http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/extensions/file_handler_util.cc File chrome/browser/chromeos/extensions/file_handler_util.cc (right): http://codereview.chromium.org/10067021/diff/13001/chrome/browser/chromeos/extensions/file_handler_util.cc#newcode512 chrome/browser/chromeos/extensions/file_handler_util.cc:512: RegisterNotificationObservers(); On 2012/04/13 18:45:17, Matt Perry wrote: > Now ...
6 years, 3 months ago (2012-04-13 20:29:07 UTC) #7
tonibarzic
I should be able to do cleanup without it (but it would probably make things ...
6 years, 3 months ago (2012-04-13 20:34:11 UTC) #8
tbarzic
Matt PTAL, this should work both with and without your patch :)
6 years, 3 months ago (2012-04-13 21:22:27 UTC) #9
Matt Perry
Cool, much cleaner. Thanks for bearing with me. LGTM
6 years, 3 months ago (2012-04-13 21:25:09 UTC) #10
tbarzic
Matt, sorry for bugging you, but can you take another look at changes in external_filesystem_apitest ...
6 years, 3 months ago (2012-04-13 22:50:04 UTC) #11
Matt Perry
lgtm
6 years, 3 months ago (2012-04-14 00:07:08 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tbarzic@chromium.org/10067021/11003
6 years, 3 months ago (2012-04-16 06:15:06 UTC) #13
commit-bot: I haz the power
6 years, 3 months ago (2012-04-16 09:36:43 UTC) #14
Try job failure for 10067021-11003 (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

Powered by Google App Engine
This is Rietveld 408576698