Re-design Unsubscribe and getPermissionStatus mojo interfaces.

Re-design Unsubscribe and getPermissionStatus mojo interfaces.

We use the ErrorType as flag to indicate the calling of unsubscribe and
getPermissionStatus are success or error. So the |is_success| flag is redundant
and can be removed.

The ErrorType::UNKNOWN is changed into ErrorType::NONE to have a better meaning.

BUG=693366
Review-Url: https://codereview.chromium.org/2715903002
Cr-Commit-Position: refs/heads/master@{#453507}
Committed: https://chromium.googlesource.com/chromium/src/+/dc9d2cfd2402fe7c6e8e4b18a6cab2e03940bcb4

[ke.he, 2017-02-24 06:37:29]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 06:37:44]

Dry run: 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-02-24 07:01:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 07:01:56]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ke.he, 2017-02-24 07:45:03]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 07:45:10]

Dry run: 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-02-24 08:07:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 08:07:37]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ke.he, 2017-02-24 09:32:30]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-24 09:32:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ke.he, 2017-02-24 10:11:34]

ke.he@intel.com changed reviewers:
+ peter@chromium.org

[ke.he, 2017-02-24 10:11:34]

Hi, Peter, PTAL:)

By the way, I think wrapping {endpoint, options, p256dh, auth} into one new
structure will introduce one extra memory copy, see my comments in crbug/693366.
Any thoughts?

Thanks!

[commit-bot: I haz the power, 2017-02-24 10:13:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 10:13:29]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-02-24 15:55:35]

lgtm w/ suggested change

By the way, I'll be in Tokyo on Monday and Tuesday of next week, and OOO for the
remainder of the week. Sorry for delays in reviews this may cause.

On 2017/02/24 10:11:34, ke.he wrote:
> By the way, I think wrapping {endpoint, options, p256dh, auth} into one new
> structure will introduce one extra memory copy, see my comments in
crbug/693366.
> Any thoughts?

Sorry, I missed the comment. Creating a new struct will give us a
PushSubscriptionInfoPtr that we can std::move() around. The copy will be created
by the type map, but since it's a single copy for a super infrequent operation
I'm not concerned about that.

Up to you whether you think it's worth it. It's fine if you don't think it's
worth it -- I can always clean it up if I feel strongly.

https://codereview.chromium.org/2715903002/diff/1/content/common/push_messagi...
File content/common/push_messaging.mojom (right):

https://codereview.chromium.org/2715903002/diff/1/content/common/push_messagi...
content/common/push_messaging.mojom:139: // error and returns |error_type| and
|error_message|.
What about renaming PushErrorType::UNKNOWN to PushErrorType::NONE? That'll
remove the need for pretty much all clarifying comments you're adding.

It also means we can do a minor clean-up in:
  third_party/WebKit/Source/modules/push_messaging/PushError.cpp


By changing the WebPushError::ErrorTypeUnknown case to be NOTREACHED() as well.
(We need to keep the case to get compile-time verification of handling all of
them.)

[ke.he, 2017-02-27 07:35:17]

Description was changed from

==========
Re-design Unsubscribe and getPermissionStatus mojo interfaces.

We can use the ErrorType as flag to indicate the calling of unsubscribe and
getPermissionStatus are success or error. So the |is_success| flag is redundant
and can be removed.

BUG=693366
==========

to

==========
Re-design Unsubscribe and getPermissionStatus mojo interfaces.

We use the ErrorType as flag to indicate the calling of unsubscribe and
getPermissionStatus are success or error. So the |is_success| flag is redundant
and can be removed.

The ErrorType::UNKNOWN is changed into ErrorType::NONE to have a better meaning.

BUG=693366
==========

[ke.he, 2017-02-27 07:36:28]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 07:36:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ke.he, 2017-02-27 07:39:51]

ke.he@intel.com changed reviewers:
+ tsepez@chromium.org

[ke.he, 2017-02-27 07:39:52]

Hi, Peter, ErrorType::UNKNOWN is changed to NONE. PTAL.
Hi, Tom, PTAL on the security review.

Thank both of you!

[commit-bot: I haz the power, 2017-02-27 08:59:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 08:59:20]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-02-27 09:32:51]

still lgtm

Renumbering WebPushError is safe, it's not used for UMA or anything.

https://codereview.chromium.org/2715903002/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_manager.cc (right):

https://codereview.chromium.org/2715903002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_manager.cc:646:
callback.Run(blink::WebPushError::ErrorTypeNone /* success */,
nit: you could drop the /* success */ now since the name implies it.

https://codereview.chromium.org/2715903002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_manager.cc:658:
callback.Run(blink::WebPushError::ErrorTypeAbort /* error */, false,
dito w/  /* error */, although the "false" after could use it.

https://codereview.chromium.org/2715903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushError.cpp (right):

https://codereview.chromium.org/2715903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushError.cpp:24: return
DOMException::create(UnknownError);
nit: just stick webError.message in there too, even though we should never reach
that code.

[Tom Sepez, 2017-02-27 17:12:23]

lgtm

[ke.he, 2017-02-28 03:05:06]

The CQ bit was checked by ke.he@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 03:05:32]

Dry run: 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-02-28 04:52:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 04:52:28]

Dry run: This issue passed the CQ dry run.

[ke.he, 2017-02-28 05:00:50]

Thanks:)

https://codereview.chromium.org/2715903002/diff/20001/content/browser/push_me...
File content/browser/push_messaging/push_messaging_manager.cc (right):

https://codereview.chromium.org/2715903002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_manager.cc:646:
callback.Run(blink::WebPushError::ErrorTypeNone /* success */,
On 2017/02/27 09:32:50, Peter Beverloo wrote:
> nit: you could drop the /* success */ now since the name implies it.

Done.

https://codereview.chromium.org/2715903002/diff/20001/content/browser/push_me...
content/browser/push_messaging/push_messaging_manager.cc:658:
callback.Run(blink::WebPushError::ErrorTypeAbort /* error */, false,
On 2017/02/27 09:32:50, Peter Beverloo wrote:
> dito w/  /* error */, although the "false" after could use it.

Done.

https://codereview.chromium.org/2715903002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/push_messaging/PushError.cpp (right):

https://codereview.chromium.org/2715903002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/push_messaging/PushError.cpp:24: return
DOMException::create(UnknownError);
On 2017/02/27 09:32:50, Peter Beverloo wrote:
> nit: just stick webError.message in there too, even though we should never
reach
> that code.

Done.

[ke.he, 2017-02-28 05:00:58]

The CQ bit was checked by ke.he@intel.com

[ke.he, 2017-02-28 05:00:58]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org,
tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2715903002/#ps40001
(title: "Re-design Unsubscribe and getPermissionStatus mojo interfaces.")

[commit-bot: I haz the power, 2017-02-28 05:02:21]

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-02-28 05:07:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488258058049360,
"parent_rev": "a92b519592dabba45a0e7f0bd2b52f9e33a7ae75", "commit_rev":
"dc9d2cfd2402fe7c6e8e4b18a6cab2e03940bcb4"}

[commit-bot: I haz the power, 2017-02-28 05:08:24]

Description was changed from

==========
Re-design Unsubscribe and getPermissionStatus mojo interfaces.

We use the ErrorType as flag to indicate the calling of unsubscribe and
getPermissionStatus are success or error. So the |is_success| flag is redundant
and can be removed.

The ErrorType::UNKNOWN is changed into ErrorType::NONE to have a better meaning.

BUG=693366
==========

to

==========
Re-design Unsubscribe and getPermissionStatus mojo interfaces.

We use the ErrorType as flag to indicate the calling of unsubscribe and
getPermissionStatus are success or error. So the |is_success| flag is redundant
and can be removed.

The ErrorType::UNKNOWN is changed into ErrorType::NONE to have a better meaning.

BUG=693366

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

[commit-bot: I haz the power, 2017-02-28 05:08:26]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/dc9d2cfd2402fe7c6e8e4b18a6ca...