Changed FileSystemPointProvider::IsAcccessAllowed() to take a single FileSystemURL instead of a tri…

Changed FileSystemPointProvider::IsAcccessAllowed() to take a single FileSystemURL instead of a triplet.

BUG=142267
Contributed by calvinlo@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=153463

[nhiroki (google), 2012-08-24 07:20:03]

lgtm. Please wait for reviewing by kinuko.

http://codereview.chromium.org/10873055/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10873055/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_handler_util.cc:572: if
(!file_system_url.is_valid()) {
nit: can you remove brace?

http://codereview.chromium.org/10873055/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_handler_util.cc:584: if
(!external_provider->IsAccessAllowed(file_system_url)) {
ditto.

[kinuko, 2012-08-24 07:54:47]

I believe the base code is old.  Have you run gclient sync after you checked out
master?  Can you try updating your master branch and rebase your change on top
of it?

[calvinlo, 2012-08-24 07:55:08]

Is lint available for Chromium project?

http://codereview.chromium.org/10873055/diff/1/chrome/browser/chromeos/extens...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10873055/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_handler_util.cc:572: if
(!file_system_url.is_valid()) {
On 2012/08/24 07:20:03, nhiroki (google) wrote:
> nit: can you remove brace?

Done.

http://codereview.chromium.org/10873055/diff/1/chrome/browser/chromeos/extens...
chrome/browser/chromeos/extensions/file_handler_util.cc:584: if
(!external_provider->IsAccessAllowed(file_system_url)) {
On 2012/08/24 07:20:03, nhiroki (google) wrote:
> ditto.
Done. Sorry, this will take me some time to get used to as you have to add these
for Java according to the Java google3 style. Do we have linting for Chromium to
catch such things?

[kinuko, 2012-08-24 08:02:16]

We have one in depot_tools/cpplint.py.  You can also customize your
pre-upload/presubmit hooks.
http://www.chromium.org/developers/how-tos/depottools/presubmit-scripts

[calvinlo, 2012-08-24 10:12:45]

Rebased, please take another look.

[kinuko, 2012-08-24 10:17:40]

looking close

http://codereview.chromium.org/10873055/diff/6002/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10873055/diff/6002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:90: bool
CrosMountPointProvider::IsAccessAllowed(const fileapi::FileSystemURL& ur) {
typo: ur -> url

http://codereview.chromium.org/10873055/diff/6002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:108: url.path());
url.path() -> url.virtual_path()

[calvinlo, 2012-08-25 13:18:10]

Sorry for typos, will look more closely at diff before review next time.

http://codereview.chromium.org/10873055/diff/6002/webkit/chromeos/fileapi/cro...
File webkit/chromeos/fileapi/cros_mount_point_provider.cc (right):

http://codereview.chromium.org/10873055/diff/6002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:90: bool
CrosMountPointProvider::IsAccessAllowed(const fileapi::FileSystemURL& ur) {
On 2012/08/24 10:17:44, kinuko wrote:
> typo: ur -> url

Done.

http://codereview.chromium.org/10873055/diff/6002/webkit/chromeos/fileapi/cro...
webkit/chromeos/fileapi/cros_mount_point_provider.cc:108: url.path());
On 2012/08/24 10:17:44, kinuko wrote:
> url.path() -> url.virtual_path()

Fixed. Sorry I should have looked at the diff more carefully after updating
this.

[kinuko, 2012-08-25 14:57:15]

lgtm, let's run try jobs!

[commit-bot: I haz the power, 2012-08-27 06:21:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/10873055/8014

[commit-bot: I haz the power, 2012-08-27 06:21:12]

Presubmit check for 10873055-8014 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/chromeos/extensions

Presubmit checks took 1.4s to calculate.

[calvinlo, 2012-08-27 06:30:54]

Hi Hashimoto san,

I have one small change which touches the file
chrome/browser/chromeos/extensions/file_handler_util.cc. Can you please help to
review this? it is only a one line change.

[hashimoto, 2012-08-27 06:43:22]

lgtm with a nit.

http://codereview.chromium.org/10873055/diff/8014/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_handler_util.cc (right):

http://codereview.chromium.org/10873055/diff/8014/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_handler_util.cc:579: if
(!external_provider->IsAccessAllowed(url))
nit: Combine this 'if' with the above one using '||'.

[calvinlo, 2012-08-27 11:24:11]

There is currently a problem with git svn now for me (non committer) so I cannot
upload the last change with regards to hashimoto's comment. I think Kinuko is
trying to make that change on her local machine now and committing on my behalf.

On 2012/08/27 06:43:22, hashimoto wrote:
> lgtm with a nit.
> 
>
http://codereview.chromium.org/10873055/diff/8014/chrome/browser/chromeos/ext...
> File chrome/browser/chromeos/extensions/file_handler_util.cc (right):
> 
>
http://codereview.chromium.org/10873055/diff/8014/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/file_handler_util.cc:579: if
> (!external_provider->IsAccessAllowed(url))
> nit: Combine this 'if' with the above one using '||'.

[kinuko, 2012-08-27 11:57:39]

Submitted on behalf of calvin:
https://src.chromium.org/viewvc/chrome?view=rev&revision=153463