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

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

Created:
7 years, 1 month ago by tbarzic
Modified:
10 months, 3 weeks 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
7 years, 1 month 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.. ...
7 years, 1 month 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 ...
7 years, 1 month 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 ...
7 years, 1 month 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 ...
7 years, 1 month 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 : ...
7 years, 1 month 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 ...
7 years, 1 month 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 ...
7 years, 1 month ago (2012-04-13 20:34:11 UTC) #8
tbarzic
Matt PTAL, this should work both with and without your patch :)
7 years, 1 month ago (2012-04-13 21:22:27 UTC) #9
Matt Perry
Cool, much cleaner. Thanks for bearing with me. LGTM
7 years, 1 month 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 ...
7 years, 1 month ago (2012-04-13 22:50:04 UTC) #11
Matt Perry
lgtm
7 years, 1 month 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
7 years, 1 month ago (2012-04-16 06:15:06 UTC) #13
commit-bot: I haz the power
7 years, 1 month 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