Extensions: Clarify ManifestHandler interface expectations.

Extensions: Clarify ManifestHandler interface expectations.

ManifestHandler::Parse is called from within Extension::Create and may or may
not be called on a thread which supports IO. On the other hand,
ManifestHandler::ValidateExtension is called through
file_util::ValidateExtension and hence is always called on a thread supporting
IO. There are assumptions in the code relying on this as well. This CL documents
the same in the ManifestHandler header.

BUG=None.

Review-Url: https://codereview.chromium.org/2846553003
Cr-Commit-Position: refs/heads/master@{#467526}
Committed: https://chromium.googlesource.com/chromium/src/+/f47da77f6fe0df56a7cf213230a6da1ef105617f

[karandeepb, 2017-04-26 21:31:14]

The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 21:32:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[karandeepb, 2017-04-26 21:33:23]

Description was changed from

==========
Extensions: Clarify ManifestHandler interface expectations.
==========

to

==========
Extensions: Clarify ManifestHandler interface expectations.

ManifestHandler::Parse is called from within Extension::Create and may or may
not be called on a thread which supports IO. On the other hand,
ManifestHandler::ValidateExtension is called through
file_util::ValidateExtension and hence is always called on a thread supporting
IO. There are assumptions in the code relying on this as well. This CL documents
the same in the ManifestHandler header.

BUG=None.
==========

[karandeepb, 2017-04-26 21:36:23]

karandeepb@chromium.org changed reviewers:
+ lazyboy@chromium.org

[karandeepb, 2017-04-26 21:36:24]

PTAL Istiaque. This wasn't evident to me from the header. See
StorageSchemaManifestHandler::Validate for an example of Validate relying on IO.

[lazyboy, 2017-04-26 22:06:54]

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handler.cc (right):

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
extensions/common/manifest_handler.cc:94:
base::ThreadRestrictions::AssertIOAllowed();
Can we put the IO comment for this function too in the header?

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handler.h (right):

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
extensions/common/manifest_handler.h:32: // This should not do any IO.
nit:
// This does not require any IO operations.

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
extensions/common/manifest_handler.h:37: // warning messages to |warnings|. It
is guaranteed that this is called on a
nit: It is not guaranteed, but the caller should guarantee that.
// This may require IO operations.
should be enough to give the hint.

[karandeepb, 2017-04-26 22:20:00]

PTAL.

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handler.cc (right):

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
extensions/common/manifest_handler.cc:94:
base::ThreadRestrictions::AssertIOAllowed();
On 2017/04/26 22:06:53, lazyboy wrote:
> Can we put the IO comment for this function too in the header?

Done.

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handler.h (right):

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
extensions/common/manifest_handler.h:32: // This should not do any IO.
On 2017/04/26 22:06:53, lazyboy wrote:
> nit:
> // This does not require any IO operations.

Done. I was directing comments to the "implementors" of the interface.

https://codereview.chromium.org/2846553003/diff/1/extensions/common/manifest_...
extensions/common/manifest_handler.h:37: // warning messages to |warnings|. It
is guaranteed that this is called on a
On 2017/04/26 22:06:53, lazyboy wrote:
> nit: It is not guaranteed, but the caller should guarantee that.
> // This may require IO operations.
> should be enough to give the hint.

Done.

[lazyboy, 2017-04-26 22:53:03]

lgtm

https://codereview.chromium.org/2846553003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler.h (right):

https://codereview.chromium.org/2846553003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler.h:32: // This does not perform any IO
operations.
Thinking about it, this doesn't seem that helpful, b/c: 1) by default we assume
we're not doing IO if not explicitly specified. 2) we're not ensuring the fact
in code (opposed to ValidateExtension, where we assert it).
You can remove this but feel free to keep it if you feel necessary.

[karandeepb, 2017-04-26 22:57:38]

https://codereview.chromium.org/2846553003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler.h (right):

https://codereview.chromium.org/2846553003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler.h:32: // This does not perform any IO
operations.
On 2017/04/26 22:53:03, lazyboy wrote:
> Thinking about it, this doesn't seem that helpful, b/c: 1) by default we
assume
> we're not doing IO if not explicitly specified
We were doing IO for Validate as in the example I pointed out.

. 2) we're not ensuring the fact
> in code (opposed to ValidateExtension, where we assert it).
I think this is helpful for those implementing a ManifestHandler. That's why I
wanted it to be a guidance -> "This should not perform any IO operations".

[karandeepb, 2017-04-26 23:31:33]

The CQ bit was checked by karandeepb@chromium.org

[commit-bot: I haz the power, 2017-04-26 23:33:25]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 00:29:12]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493249493349540,
"parent_rev": "b9eec19bd79f6e7018f5dfdda37e54a37c4c0dd1", "commit_rev":
"f47da77f6fe0df56a7cf213230a6da1ef105617f"}

[commit-bot: I haz the power, 2017-04-27 00:30:23]

Description was changed from

==========
Extensions: Clarify ManifestHandler interface expectations.

ManifestHandler::Parse is called from within Extension::Create and may or may
not be called on a thread which supports IO. On the other hand,
ManifestHandler::ValidateExtension is called through
file_util::ValidateExtension and hence is always called on a thread supporting
IO. There are assumptions in the code relying on this as well. This CL documents
the same in the ManifestHandler header.

BUG=None.
==========

to

==========
Extensions: Clarify ManifestHandler interface expectations.

ManifestHandler::Parse is called from within Extension::Create and may or may
not be called on a thread which supports IO. On the other hand,
ManifestHandler::ValidateExtension is called through
file_util::ValidateExtension and hence is always called on a thread supporting
IO. There are assumptions in the code relying on this as well. This CL documents
the same in the ManifestHandler header.

BUG=None.

Review-Url: https://codereview.chromium.org/2846553003
Cr-Commit-Position: refs/heads/master@{#467526}
Committed:
https://chromium.googlesource.com/chromium/src/+/f47da77f6fe0df56a7cf213230a6...
==========

[commit-bot: I haz the power, 2017-04-27 00:30:24]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/f47da77f6fe0df56a7cf213230a6...