Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
658723003:
Blink implementation of PushManager#hasPermission() (Closed)
|
Created:
6 years, 2 months ago by Miguel Garcia Modified:
6 years, 1 month ago CC:
blink-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionBlink implementation of PushManager#hasPermission()
Extra Layout tests for these new methods will depend on a change in content_shell, so they will follow in a future patch
BUG=401424
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184638
Patch Set 1 #Patch Set 2 : Upgrade enum values so it can be passed directly over IPC #Patch Set 3 : Rename enum to a more understandable name #
Total comments: 28
Patch Set 4 : #
Total comments: 53
Patch Set 5 : #
Total comments: 9
Patch Set 6 : Upgrade the push-messaging-api-surface-actual layout test to include hasPermission #
Total comments: 13
Patch Set 7 : #Patch Set 8 : #
Total comments: 2
Patch Set 9 : #Patch Set 10 : Rebase Layout test #Messages
Total messages: 37 (9 generated)
miguelg@chromium.org changed reviewers: + mvanouwerkerk@chromium.org
Ready for initial review The associated Chrome CL is in https://codereview.chromium.org/661463002 If you feel like trying it you can patch both Cls and go to https://chromiumexperiments.appspot.com/static/push_example.html
On 2014/10/15 12:56:12, Miguel Garcia wrote: > Ready for initial review > > The associated Chrome CL is in https://codereview.chromium.org/661463002 > > > If you feel like trying it you can patch both Cls and go to > https://chromiumexperiments.appspot.com/static/push_example.html Friendly ping on this review
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
drive-by comments https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... File Source/modules/push_messaging/PushManager.cpp (right): https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:28: // FIXME: These calls should be available from workers which will not have a Document object available. I would move that FIXME in the two places where the assumption is made. The file might evolve in a way that will make this comment outdated. Also, I don't think it's common to have general comment like "Implementation of the Push Api." https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:63: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(AbortError, "Document is detached from window.")); I think rejecting with "InvalidStateError" would be more appropriate. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:67: WebServiceWorkerProvider* serviceWorkerProvider = NavigatorServiceWorker::serviceWorker(document->domWindow()->navigator())->provider(); The way to access ServiceWorkerContainer is confusing: you can get the NavigatorServiceWorker from a Document or a Navigator but you must pass a Navigator to get the ServiceWorkerContainer. I wonder if it would make sense to expose the private serviceWorker() as a public method. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:69: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(AbortError, "No Service Worker installed for this document.")); ditto: InvalidStateError is probably more appropriate. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... File Source/modules/push_messaging/PushManager.idl (right): https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.idl:11: [CallWith=ScriptState] Promise permissionStatus(); Shouldn't that be called hasPermission()? https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.cpp (right): https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:6: nit: no new line between "config.h" and the header inclusion. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:31: DEFINE_STATIC_LOCAL(const String, defaultPermission, ("default")); I think "default" is an unfortunate name :( https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:48: m_resolver->resolve(permissionString(type)); Actually, do you really need permissionString() to be a different method? It seems that you only need it here. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:53: m_resolver->reject(DOMException::create(SecurityError, "Could not check permission")); I would not use SecurityError because this isn't a real security error. What about "UnknownError"? Feel free to have a look at the list of error names and pick one that sounds appropriate: http://heycam.github.io/webidl/#dfn-error-names-table https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:26: public: coding style: - there is a useless empty line before "public:" - "public:" should not be indented. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:30: // Called if the embedder is able to check the status of the push permission. nit: I would put those comments in WebPushPermissionCallback and just point that those methods are implementing the interface. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:37: static const WTF::String& permissionString(PushPermissionStatus); Could you move that in an anonymous namespace in the cpp file? https://codereview.chromium.org/658723003/diff/40001/public/platform/WebPushP... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/40001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:16: PushPermissionGranted = 0, nit: do you need to set to 0? I'm not sure if there is a Blink convention regarding that but I would tend to say that not setting the initial value is a better practice. https://codereview.chromium.org/658723003/diff/40001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:21: PushPermissionTypeLast = PushPermissionDefault I think it would be fine to simply use PushPermissionDefault in the IPC checks. It is not mandatory to create a TypeLast entry for the IPC checks, mostly because that value has to be inclusive and having a TypeLast entry that is equal to the previous entry is odd. https://codereview.chromium.org/658723003/diff/40001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:26: } nit: I think the coding style would require something like: virtual ~WebPushPermissionCallback() { }
Thanks for the review Mounir Friendly ping Michael, I'd like to start adding OWNERS ASAP if possible https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... File Source/modules/push_messaging/PushManager.cpp (right): https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:28: // FIXME: These calls should be available from workers which will not have a Document object available. On 2014/10/17 09:22:01, Mounir Lamouri wrote: > I would move that FIXME in the two places where the assumption is made. The file > might evolve in a way that will make this comment outdated. > > Also, I don't think it's common to have general comment like "Implementation of > the Push Api." Done. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:63: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(AbortError, "Document is detached from window.")); On 2014/10/17 09:22:00, Mounir Lamouri wrote: > I think rejecting with "InvalidStateError" would be more appropriate. Done. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:69: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(AbortError, "No Service Worker installed for this document.")); On 2014/10/17 09:22:01, Mounir Lamouri wrote: > ditto: InvalidStateError is probably more appropriate. Done. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... File Source/modules/push_messaging/PushManager.idl (right): https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.idl:11: [CallWith=ScriptState] Promise permissionStatus(); On 2014/10/17 09:22:01, Mounir Lamouri wrote: > Shouldn't that be called hasPermission()? Indeed, I must have confused things with the result type when reading the spec. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.cpp (right): https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:6: On 2014/10/17 09:22:01, Mounir Lamouri wrote: > nit: no new line between "config.h" and the header inclusion. Done. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:31: DEFINE_STATIC_LOCAL(const String, defaultPermission, ("default")); On 2014/10/17 09:22:01, Mounir Lamouri wrote: > I think "default" is an unfortunate name :( not much I can do here talk to Michael, he is a spec editor :) On a more serious note, we discussed using "ask" or "prompt" but it is not clear that the default behaviour is always to prompt. The default could be 3 push messages for free for example. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:48: m_resolver->resolve(permissionString(type)); On 2014/10/17 09:22:01, Mounir Lamouri wrote: > Actually, do you really need permissionString() to be a different method? It > seems that you only need it here. It seems nicer but this is just a matter of tase (plus it could be tested separately) I can inline it if you insist. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:53: m_resolver->reject(DOMException::create(SecurityError, "Could not check permission")); On 2014/10/17 09:22:01, Mounir Lamouri wrote: > I would not use SecurityError because this isn't a real security error. What > about "UnknownError"? Feel free to have a look at the list of error names and > pick one that sounds appropriate: > http://heycam.github.io/webidl/#dfn-error-names-table OperationError ? https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:26: public: On 2014/10/17 09:22:01, Mounir Lamouri wrote: > coding style: > - there is a useless empty line before "public:" > - "public:" should not be indented. Done. https://codereview.chromium.org/658723003/diff/40001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:30: // Called if the embedder is able to check the status of the push permission. On 2014/10/17 09:22:01, Mounir Lamouri wrote: > nit: I would put those comments in WebPushPermissionCallback and just point that > those methods are implementing the interface. Done. https://codereview.chromium.org/658723003/diff/40001/public/platform/WebPushP... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/40001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:16: PushPermissionGranted = 0, On 2014/10/17 09:22:01, Mounir Lamouri wrote: > nit: do you need to set to 0? I'm not sure if there is a Blink convention > regarding that but I would tend to say that not setting the initial value is a > better practice. I've seen this in many other enums (like notifications) https://codereview.chromium.org/658723003/diff/40001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:21: PushPermissionTypeLast = PushPermissionDefault On 2014/10/17 09:22:02, Mounir Lamouri wrote: > I think it would be fine to simply use PushPermissionDefault in the IPC checks. > It is not mandatory to create a TypeLast entry for the IPC checks, mostly > because that value has to be inclusive and having a TypeLast entry that is equal > to the previous entry is odd. I'll ask the security reviewer once I send it out. For now I rather leave it. https://codereview.chromium.org/658723003/diff/40001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:26: } On 2014/10/17 09:22:01, Mounir Lamouri wrote: > nit: I think the coding style would require something like: > virtual ~WebPushPermissionCallback() { } Done.
https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... File Source/modules/push_messaging/PushManager.cpp (right): https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:26: nit: delete this extra empty line https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:33: // FIXME: This calls should be available from workers which will not have a Document object available. crbug.com/389194 https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:33: // FIXME: This calls should be available from workers which will not have a Document object available. nit: s/This/These/ https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:55: // FIXME: This calls should be available from workers which will not have a Document object available. nit: same as above https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:68: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "No Service Worker installed for this document.")); The spec says to not provide any arguments when rejecting. If you feel InvalidStateError is better (I agree) then please open a spec issue to propose the API change, and a bug for us to track the divergence from the specced API. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:72: client->permissionStatus(new PushPermissionCallback(resolver), serviceWorkerProvider); It's not a simple getter is it? Then please name the method as a verb e.g. getPermissionStatus https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... File Source/modules/push_messaging/PushManager.idl (right): https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.idl:11: [CallWith=ScriptState] Promise hasPermission(); Why don't we have an idl definition for enum PushPermissionStatus? https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.cpp (right): https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:26: /* static */ const String& PushPermissionCallback::permissionString(PushPermissionStatus type) We don't tend to comment on static in cpp files in Blink, but if you really like it then please put it on the line above using the "// static" form. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:52: m_resolver->reject(DOMException::create(OperationError, "Could not check permission")); Again, this diverges from the spec so please open a spec issue and an implementation bug to track this. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:14: class String; Do you need to declare this? Noone else seems to. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:26: PushPermissionCallback(PassRefPtr<ScriptPromiseResolver>); explicit https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:35: static const WTF::String& permissionString(PushPermissionStatus); No need for WTF:: because we're using :-) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/658723003/diff/60001/public/platform/WebPushP... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/60001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:13: public: All of this indent -4. Just run "git cl format" https://codereview.chromium.org/658723003/diff/60001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:16: PushPermissionGranted = 0, Enum members must start with the full enum name.
peter@chromium.org changed reviewers: + peter@chromium.org
I would like to see some layout tests as well, to verify our Web Exposed behavior. It's fine if you commit to doing this in a follow-up CL (given that the lack of an API implementation would currently leave the Promise hanging -- which is not great to start with), but file a bug assigned to yourself to track this. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... File Source/modules/push_messaging/PushManager.cpp (right): https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:68: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "No Service Worker installed for this document.")); On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > The spec says to not provide any arguments when rejecting. If you feel > InvalidStateError is better (I agree) then please open a spec issue to propose > the API change, and a bug for us to track the divergence from the specced API. We don't care at all about whether there's a Service Worker or not, so this entire check should be removed. Permissions for push are granted per origin, not per service worker. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.cpp (right): https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:12: nit: excess newline. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:16: PushPermissionCallback::PushPermissionCallback(PassRefPtr<ScriptPromiseResolver> script_resolver) : nit: The colon needs to be on the next line. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:24: nit: excess newline. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:26: /* static */ const String& PushPermissionCallback::permissionString(PushPermissionStatus type) s/type/status/. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:45: void PushPermissionCallback::onSuccess(PushPermissionStatus type) s/type/status/. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:52: m_resolver->reject(DOMException::create(OperationError, "Could not check permission")); On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > Again, this diverges from the spec so please open a spec issue and an > implementation bug to track this. I don't think having an OperationError saying "Could not check permission" makes a lot of sense. What would I do with that information? Is it only here for consistency reasons? (With what?) https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:14: class String; On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > Do you need to declare this? Noone else seems to. I'd argue that IWYU is a good thing. Neither of the previously included files sounds like it's in any way related to a WTF::String. https://codereview.chromium.org/658723003/diff/60001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.h:22: // passed to the callback. nit: no need for the linebreak. https://codereview.chromium.org/658723003/diff/60001/public/platform/WebPushC... File public/platform/WebPushClient.h (right): https://codereview.chromium.org/658723003/diff/60001/public/platform/WebPushC... public/platform/WebPushClient.h:10: #include "public/platform/WebPushPermissionCallback.h" You should forward declare this instead. https://codereview.chromium.org/658723003/diff/60001/public/platform/WebPushP... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/60001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:12: class WebPushPermissionCallback { Instead of re-inventing the callback API, can you move the permission enumeration to its own file and replace this entire class using a WebCallbacks<PushPermissionStatus, void> (which might need another specialization)? https://codereview.chromium.org/658723003/diff/60001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:33: } // namespace blink nit: new line after this one.
https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... File Source/modules/push_messaging/PushManager.cpp (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushManager.cpp:26: On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > nit: delete this extra empty line Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushManager.cpp:33: // FIXME: This calls should be available from workers which will not have a Document object available. On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > nit: s/This/These/ Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushManager.cpp:55: // FIXME: This calls should be available from workers which will not have a Document object available. On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > nit: same as above Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushManager.cpp:68: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "No Service Worker installed for this document.")); mm I don't know, we use the service worker stuff later on to take the origin. You cannot register if you don't have a SW anyway so we might just as well bail out early. I will file the bugs once we clarify this. On 2014/10/20 12:23:49, Peter Beverloo wrote: > On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > > The spec says to not provide any arguments when rejecting. If you feel > > InvalidStateError is better (I agree) then please open a spec issue to propose > > the API change, and a bug for us to track the divergence from the specced API. > > We don't care at all about whether there's a Service Worker or not, so this > entire check should be removed. Permissions for push are granted per origin, not > per service worker. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushManager.cpp:72: client->permissionStatus(new PushPermissionCallback(resolver), serviceWorkerProvider); On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > It's not a simple getter is it? Then please name the method as a verb e.g. > getPermissionStatus Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... File Source/modules/push_messaging/PushManager.idl (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushManager.idl:11: [CallWith=ScriptState] Promise hasPermission(); On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > Why don't we have an idl definition for enum PushPermissionStatus? It does not seem needed really it is never used as an argument just as a return type. What changes exactly if define it in idl? https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... File Source/modules/push_messaging/PushPermissionCallback.cpp (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:12: On 2014/10/20 12:23:49, Peter Beverloo wrote: > nit: excess newline. Acknowledged. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:16: PushPermissionCallback::PushPermissionCallback(PassRefPtr<ScriptPromiseResolver> script_resolver) : On 2014/10/20 12:23:49, Peter Beverloo wrote: > nit: The colon needs to be on the next line. Acknowledged. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:24: On 2014/10/20 12:23:49, Peter Beverloo wrote: > nit: excess newline. Acknowledged. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:26: /* static */ const String& PushPermissionCallback::permissionString(PushPermissionStatus type) On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > We don't tend to comment on static in cpp files in Blink, but if you really like > it then please put it on the line above using the "// static" form. Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:26: /* static */ const String& PushPermissionCallback::permissionString(PushPermissionStatus type) On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > We don't tend to comment on static in cpp files in Blink, but if you really like > it then please put it on the line above using the "// static" form. Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:45: void PushPermissionCallback::onSuccess(PushPermissionStatus type) On 2014/10/20 12:23:49, Peter Beverloo wrote: > s/type/status/. Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:52: m_resolver->reject(DOMException::create(OperationError, "Could not check permission")); I will just reject here for now then. On 2014/10/20 12:23:49, Peter Beverloo wrote: > On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > > Again, this diverges from the spec so please open a spec issue and an > > implementation bug to track this. > > I don't think having an OperationError saying "Could not check permission" makes > a lot of sense. What would I do with that information? Is it only here for > consistency reasons? (With what?) https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:52: m_resolver->reject(DOMException::create(OperationError, "Could not check permission")); On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > Again, this diverges from the spec so please open a spec issue and an > implementation bug to track this. Acknowledged. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... File Source/modules/push_messaging/PushPermissionCallback.h (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.h:14: class String; On 2014/10/20 12:23:49, Peter Beverloo wrote: > On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > > Do you need to declare this? Noone else seems to. > > I'd argue that IWYU is a good thing. Neither of the previously included files > sounds like it's in any way related to a WTF::String. Agreed, I think it is a bad idea to not include things just because they are probably included somewhere else. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.h:22: // passed to the callback. On 2014/10/20 12:23:49, Peter Beverloo wrote: > nit: no need for the linebreak. Sigh https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.h:26: PushPermissionCallback(PassRefPtr<ScriptPromiseResolver>); On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > explicit Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.h:35: static const WTF::String& permissionString(PushPermissionStatus); On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > No need for WTF:: because we're using :-) > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Really? Is this in some guideline I'v missed? Otherwise I prefer to leave it. When I read this file I want to know what String class exactly is the one I am using without having to WTFString.h to see if there is a using statement there or not. Especially since I am not #including that file here, just forward declaring it. https://chromiumcodereview.appspot.com/658723003/diff/60001/public/platform/W... File public/platform/WebPushClient.h (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/public/platform/W... public/platform/WebPushClient.h:10: #include "public/platform/WebPushPermissionCallback.h" On 2014/10/20 12:23:49, Peter Beverloo wrote: > You should forward declare this instead. Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/public/platform/W... File public/platform/WebPushPermissionCallback.h (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/public/platform/W... public/platform/WebPushPermissionCallback.h:12: class WebPushPermissionCallback { On 2014/10/20 12:23:49, Peter Beverloo wrote: > Instead of re-inventing the callback API, can you move the permission > enumeration to its own file and replace this entire class using a > WebCallbacks<PushPermissionStatus, void> (which might need another > specialization)? We discussed this at length with the main reviewers before writing the code and it was decided to it this way. I really don't see the point in that WebCallbacks business. It adds unnecesary boiler plate code for something that is just a simple conversation from callback to promise. ScreenOrientation, BatteryStatus and others have gone the same way. In fact we will probably replace the way register is done as well. https://chromiumcodereview.appspot.com/658723003/diff/60001/public/platform/W... public/platform/WebPushPermissionCallback.h:13: public: On 2014/10/20 11:25:43, Michael van Ouwerkerk wrote: > All of this indent -4. Just run "git cl format" Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/public/platform/W... public/platform/WebPushPermissionCallback.h:13: public: On 2014/10/20 11:25:43, Michael van Ouwerkerk wrote: > All of this indent -4. Just run "git cl format" Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/public/platform/W... public/platform/WebPushPermissionCallback.h:16: PushPermissionGranted = 0, On 2014/10/20 11:25:43, Michael van Ouwerkerk wrote: > Enum members must start with the full enum name. Done. https://chromiumcodereview.appspot.com/658723003/diff/60001/public/platform/W... public/platform/WebPushPermissionCallback.h:33: } // namespace blink On 2014/10/20 12:23:49, Peter Beverloo wrote: > nit: new line after this one. Done.
https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... File Source/modules/push_messaging/PushManager.cpp (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushManager.cpp:68: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "No Service Worker installed for this document.")); On 2014/10/22 12:58:43, Miguel Garcia wrote: > mm I don't know, we use the service worker stuff later on to take the origin. > You cannot register if you don't have a SW anyway so we might just as well bail > out early. > > I will file the bugs once we clarify this. > > > > > > On 2014/10/20 12:23:49, Peter Beverloo wrote: > > On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > > > The spec says to not provide any arguments when rejecting. If you feel > > > InvalidStateError is better (I agree) then please open a spec issue to > propose > > > the API change, and a bug for us to track the divergence from the specced > API. > > > > We don't care at all about whether there's a Service Worker or not, so this > > entire check should be removed. Permissions for push are granted per origin, > not > > per service worker. > You could take the origin from the RenderFrameHost using GetLastCommittedURL() so no need for a service worker strictly speaking. I agree with Peter here, this method does not require a service worker. If there isn't one now, there may still be one later. https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... File Source/modules/push_messaging/PushPermissionCallback.h (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.h:35: static const WTF::String& permissionString(PushPermissionStatus); On 2014/10/22 12:58:44, Miguel Garcia wrote: > On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > > No need for WTF:: because we're using :-) > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Really? Is this in some guideline I'v missed? Otherwise I prefer to leave it. > When I read this file I want to know what String class exactly is the one I am > using without having to WTFString.h to see if there is a using statement there > or not. Especially since I am not #including that file here, just forward > declaring it. I linked to the using statement above, that's one guideline. Precedent is another. This code lives in /modules/ ... WTF::String is the default type there and the namespace is always omitted. It seems to be the pattern in Blink generally, excepting some string conversion routines.
Oh and about the layout tests. I will adding them once the blink patch lands and rolls into chromium. On 22 Oct 2014 14:20, <mvanouwerkerk@chromium.org> wrote: > > https://chromiumcodereview.appspot.com/658723003/diff/ > 60001/Source/modules/push_messaging/PushManager.cpp > File Source/modules/push_messaging/PushManager.cpp (right): > > https://chromiumcodereview.appspot.com/658723003/diff/ > 60001/Source/modules/push_messaging/PushManager.cpp#newcode68 > Source/modules/push_messaging/PushManager.cpp:68: return > ScriptPromise::rejectWithDOMException(scriptState, > DOMException::create(InvalidStateError, "No Service Worker installed for > this document.")); > On 2014/10/22 12:58:43, Miguel Garcia wrote: > >> mm I don't know, we use the service worker stuff later on to take the >> > origin. > >> You cannot register if you don't have a SW anyway so we might just as >> > well bail > >> out early. >> > > I will file the bugs once we clarify this. >> > > > > > > On 2014/10/20 12:23:49, Peter Beverloo wrote: >> > On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: >> > > The spec says to not provide any arguments when rejecting. If you >> > feel > >> > > InvalidStateError is better (I agree) then please open a spec >> > issue to > >> propose >> > > the API change, and a bug for us to track the divergence from the >> > specced > >> API. >> > >> > We don't care at all about whether there's a Service Worker or not, >> > so this > >> > entire check should be removed. Permissions for push are granted per >> > origin, > >> not >> > per service worker. >> > > > You could take the origin from the RenderFrameHost using > GetLastCommittedURL() so no need for a service worker strictly speaking. > I agree with Peter here, this method does not require a service worker. > If there isn't one now, there may still be one later. > > https://chromiumcodereview.appspot.com/658723003/diff/ > 60001/Source/modules/push_messaging/PushPermissionCallback.h > File Source/modules/push_messaging/PushPermissionCallback.h (right): > > https://chromiumcodereview.appspot.com/658723003/diff/ > 60001/Source/modules/push_messaging/PushPermissionCallback.h#newcode35 > Source/modules/push_messaging/PushPermissionCallback.h:35: static const > WTF::String& permissionString(PushPermissionStatus); > On 2014/10/22 12:58:44, Miguel Garcia wrote: > >> On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: >> > No need for WTF:: because we're using :-) >> > >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/third_party/WebKit/Source/wtf/text/WTFString.h&sq= > package:chromium&type=cs&l=669&rcl=1413737618 > > Really? Is this in some guideline I'v missed? Otherwise I prefer to >> > leave it. > >> When I read this file I want to know what String class exactly is the >> > one I am > >> using without having to WTFString.h to see if there is a using >> > statement there > >> or not. Especially since I am not #including that file here, just >> > forward > >> declaring it. >> > > I linked to the using statement above, that's one guideline. Precedent > is another. This code lives in /modules/ ... WTF::String is the default > type there and the namespace is always omitted. It seems to be the > pattern in Blink generally, excepting some string conversion routines. > > https://chromiumcodereview.appspot.com/658723003/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... File Source/modules/push_messaging/PushManager.cpp (right): https://chromiumcodereview.appspot.com/658723003/diff/60001/Source/modules/pu... Source/modules/push_messaging/PushManager.cpp:68: return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(InvalidStateError, "No Service Worker installed for this document.")); On 2014/10/22 13:20:01, Michael van Ouwerkerk wrote: > On 2014/10/22 12:58:43, Miguel Garcia wrote: > > mm I don't know, we use the service worker stuff later on to take the origin. > > You cannot register if you don't have a SW anyway so we might just as well > bail > > out early. > > > > I will file the bugs once we clarify this. > > > > > > > > > > > > On 2014/10/20 12:23:49, Peter Beverloo wrote: > > > On 2014/10/20 11:25:42, Michael van Ouwerkerk wrote: > > > > The spec says to not provide any arguments when rejecting. If you feel > > > > InvalidStateError is better (I agree) then please open a spec issue to > > propose > > > > the API change, and a bug for us to track the divergence from the specced > > API. > > > > > > We don't care at all about whether there's a Service Worker or not, so this > > > entire check should be removed. Permissions for push are granted per origin, > > not > > > per service worker. > > > > You could take the origin from the RenderFrameHost using GetLastCommittedURL() > so no need for a service worker strictly speaking. I agree with Peter here, this > method does not require a service worker. If there isn't one now, there may > still be one later. Ok on further discussion... in future, when called from a service worker context, there will not be a RenderFrameHost. It would also be an alarming error state to not have a service worker, as the PushRegistrationManager will be owned by a ServiceWorkerRegistration. So there is an interesting error to throw here.
> > > >> mm I don't know, we use the service worker stuff later on to take the > >> > > origin. > > > >> You cannot register if you don't have a SW anyway so we might just as > >> > > well bail > > > >> out early. > >> > > > > I will file the bugs once we clarify this. As agreed offline we do need the SW it seems. I am therefore leaving it as is and just rejecting with no arguments while the spec bug gets resolved > > > > https://code.google.com/p/chromium/codesearch#chromium/ > > src/third_party/WebKit/Source/wtf/text/WTFString.h&sq= > > package:chromium&type=cs&l=669&rcl=1413737618 > > > > is another. This code lives in /modules/ ... WTF::String is the default > > type there and the namespace is always omitted. It seems to be the > > pattern in Blink generally, excepting some string conversion routines. > > > > https://chromiumcodereview.appspot.com/658723003/ > > So given that String is forward declared I cannot just remove it. I would need to to include wtf/String.h instead and since you did not feel super strongly about it (as disucssed offline) I am leaving it for now
Adam Can you review the changes in public/platform? Thanks
miguelg@chromium.org changed reviewers: + abarth@chromium.org
lgtm % comments Could you clarify in the patch' description that layout tests will depend on a content_shell implementation, and that they will follow in a future patch? https://chromiumcodereview.appspot.com/658723003/diff/80001/Source/modules/pu... File Source/modules/push_messaging/PushManager.cpp (right): https://chromiumcodereview.appspot.com/658723003/diff/80001/Source/modules/pu... Source/modules/push_messaging/PushManager.cpp:67: WebServiceWorkerProvider* serviceWorkerProvider = NavigatorServiceWorker::serviceWorker(document->domWindow()->navigator())->provider(); Please add a note here explicitly saying that the currently implemented specification does not require a Service Worker to be present for the hasPermission() call to work, but that it will be a requirement once we move to the new specification. https://chromiumcodereview.appspot.com/658723003/diff/80001/Source/modules/pu... File Source/modules/push_messaging/PushPermissionCallback.cpp (right): https://chromiumcodereview.appspot.com/658723003/diff/80001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:14: PushPermissionCallback::PushPermissionCallback(PassRefPtr<ScriptPromiseResolver> script_resolver) s/script_resolver/scriptResolver/. Or just "resolver". https://chromiumcodereview.appspot.com/658723003/diff/80001/Source/modules/pu... Source/modules/push_messaging/PushPermissionCallback.cpp:24: const String& PushPermissionCallback::permissionString(WebPushPermissionStatus status) nit: move this to the last definition in the file (order of functions in the header and implementation should match). https://chromiumcodereview.appspot.com/658723003/diff/80001/public/platform/W... File public/platform/WebPushPermissionCallback.h (right): https://chromiumcodereview.appspot.com/658723003/diff/80001/public/platform/W... public/platform/WebPushPermissionCallback.h:14: class WebPushPermissionCallback { You can replace this entire class by including public/platform/WebCallbacks.h and doing: typedef WebCallbacks<WebPushPermissionStatus, void> WebPushPermissionCallback; (You may have to add a specialization of WebCallbacks<Type, void>.) https://chromiumcodereview.appspot.com/658723003/diff/80001/public/platform/W... File public/platform/WebPushPermissionStatus.h (right): https://chromiumcodereview.appspot.com/658723003/diff/80001/public/platform/W... public/platform/WebPushPermissionStatus.h:19: } // namespace blink nit: empty line after this.
https://codereview.chromium.org/658723003/diff/80001/Source/modules/push_mess... File Source/modules/push_messaging/PushManager.cpp (right): https://codereview.chromium.org/658723003/diff/80001/Source/modules/push_mess... Source/modules/push_messaging/PushManager.cpp:67: WebServiceWorkerProvider* serviceWorkerProvider = NavigatorServiceWorker::serviceWorker(document->domWindow()->navigator())->provider(); On 2014/10/23 11:33:37, Peter Beverloo wrote: > Please add a note here explicitly saying that the currently implemented > specification does not require a Service Worker to be present for the > hasPermission() call to work, but that it will be a requirement once we move to > the new specification. Done. https://codereview.chromium.org/658723003/diff/80001/Source/modules/push_mess... File Source/modules/push_messaging/PushPermissionCallback.cpp (right): https://codereview.chromium.org/658723003/diff/80001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:14: PushPermissionCallback::PushPermissionCallback(PassRefPtr<ScriptPromiseResolver> script_resolver) On 2014/10/23 11:33:37, Peter Beverloo wrote: > s/script_resolver/scriptResolver/. Or just "resolver". Done. https://codereview.chromium.org/658723003/diff/80001/Source/modules/push_mess... Source/modules/push_messaging/PushPermissionCallback.cpp:24: const String& PushPermissionCallback::permissionString(WebPushPermissionStatus status) On 2014/10/23 11:33:37, Peter Beverloo wrote: > nit: move this to the last definition in the file (order of functions in the > header and implementation should match). Done. https://codereview.chromium.org/658723003/diff/80001/public/platform/WebPushP... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/80001/public/platform/WebPushP... public/platform/WebPushPermissionCallback.h:14: class WebPushPermissionCallback { On 2014/10/23 11:33:37, Peter Beverloo wrote: > You can replace this entire class by including public/platform/WebCallbacks.h > and doing: > > typedef WebCallbacks<WebPushPermissionStatus, void> WebPushPermissionCallback; > > (You may have to add a specialization of WebCallbacks<Type, void>.) As discussed offline, the template expands to onSuccess(S*) which for an S of type enum seems like an overkill to pass a pointer. I am therefore leaving it as is.
lgtm with nits https://codereview.chromium.org/658723003/diff/100001/Source/modules/push_mes... File Source/modules/push_messaging/PushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/100001/Source/modules/push_mes... Source/modules/push_messaging/PushPermissionCallback.h:30: virtual void onSuccess(WebPushPermissionStatus) override; Nit: omit virtual https://codereview.chromium.org/658723003/diff/100001/Source/modules/push_mes... Source/modules/push_messaging/PushPermissionCallback.h:33: virtual void onError() override; Nit: omit virtual https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... File public/platform/WebPushPermissionStatus.h (right): https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionStatus.h:16: PushPermissionTypeLast = WebPushPermissionStatusDefault Shouldn't this be WebPushPermissionTypeLast ?
miguelg@chromium.org changed reviewers: + mkwst@chromium.org - abarth@chromium.org
+mike for public/platform owners
https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionCallback.h:14: class WebPushPermissionCallback { I don't think you need a whole class for this. What can't you do with `WebCallbacks<WebPushPermissionStatus>`? https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... File public/platform/WebPushPermissionStatus.h (right): https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionStatus.h:16: PushPermissionTypeLast = WebPushPermissionStatusDefault On 2014/10/24 10:36:37, Michael van Ouwerkerk wrote: > Shouldn't this be WebPushPermissionTypeLast ? Indeed.
https://codereview.chromium.org/658723003/diff/100001/Source/modules/push_mes... File Source/modules/push_messaging/PushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/100001/Source/modules/push_mes... Source/modules/push_messaging/PushPermissionCallback.h:30: virtual void onSuccess(WebPushPermissionStatus) override; On 2014/10/24 10:36:37, Michael van Ouwerkerk wrote: > Nit: omit virtual Done. https://codereview.chromium.org/658723003/diff/100001/Source/modules/push_mes... Source/modules/push_messaging/PushPermissionCallback.h:33: virtual void onError() override; On 2014/10/24 10:36:37, Michael van Ouwerkerk wrote: > Nit: omit virtual Done. https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionCallback.h:14: class WebPushPermissionCallback { On 2014/10/27 15:32:11, Mike West wrote: > I don't think you need a whole class for this. What can't you do with > `WebCallbacks<WebPushPermissionStatus>`? Peter had the same comment, in the end we agreed that this was easier since WebCallbacks expands the onSuccess method to onSuccess(S*) which for an S of type enum seems like an overkill to pass a pointer. https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... File public/platform/WebPushPermissionStatus.h (right): https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionStatus.h:16: PushPermissionTypeLast = WebPushPermissionStatusDefault On 2014/10/27 15:32:11, Mike West wrote: > On 2014/10/24 10:36:37, Michael van Ouwerkerk wrote: > > Shouldn't this be WebPushPermissionTypeLast ? > > Indeed. Done. https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionStatus.h:16: PushPermissionTypeLast = WebPushPermissionStatusDefault On 2014/10/24 10:36:37, Michael van Ouwerkerk wrote: > Shouldn't this be WebPushPermissionTypeLast ? Acknowledged.
https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionCallback.h:14: class WebPushPermissionCallback { On 2014/10/28 11:07:00, Miguel Garcia wrote: > On 2014/10/27 15:32:11, Mike West wrote: > > I don't think you need a whole class for this. What can't you do with > > `WebCallbacks<WebPushPermissionStatus>`? > > Peter had the same comment, in the end we agreed that this was easier since > WebCallbacks expands the onSuccess method to onSuccess(S*) which for an S of > type enum seems like an overkill to pass a pointer. Easier is relative. Introducing another callback type with a very similar signature that doesn't quite use the existing framework is a bit confusing. Perhaps we could add a `WebCallbacks<enum, enum>` variant? You're probably not the only callback with that use case; I know some of the CredentialManager code could use the same. On that note... https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionCallback.h:22: virtual void onError() = 0; Are you sure you're not going to need to pass any error state down from the embedder when the status can't be checked? Perhaps it's worth putting a failure-types enum together as well.
Peer, Mike, Ok, I've tried a few things to see if we could find a compromise I don't see how I can create a WebCallbacks<enum, enum> variant, I would need to define it using template<typename S, typename T> (since I cannot forward declare enums) and that is already taken by the one that returns a pointer. I have tried to change the existing template to onSuccess(S) instead of onSuccess(S*) and then change all the typedefs from #typedef WebCallbacks<Foo... to #typedef WebCallbacks<Foo* (since there are not that many https://code.google.com/p/chromium/codesearch#search/&q=typedef%5CsWebCallbac...) This however also does not work since there are existing overrides that assume the returned type is a pointer (like third_party/WebKit/Source/bindings/core/v8/CallbackPromiseAdapter.h but there are several others) In the end I have given up and changed my override to use a pointer even if it is a basic type what I am passing. There seems to be some precedent in https://code.google.com/p/chromium/codesearch#chromium/src/content/child/serv... Let me know what you think, I still think this is a less clear solution but I don't feel strongly about it and rather defer to you two who have way more experience than I do in blink. On 2014/10/28 11:23:36, Mike West wrote: > https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... > File public/platform/WebPushPermissionCallback.h (right): > > https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... > public/platform/WebPushPermissionCallback.h:14: class WebPushPermissionCallback > { > On 2014/10/28 11:07:00, Miguel Garcia wrote: > > On 2014/10/27 15:32:11, Mike West wrote: > > > I don't think you need a whole class for this. What can't you do with > > > `WebCallbacks<WebPushPermissionStatus>`? > > > > Peter had the same comment, in the end we agreed that this was easier since > > WebCallbacks expands the onSuccess method to onSuccess(S*) which for an S of > > type enum seems like an overkill to pass a pointer. > > Easier is relative. Introducing another callback type with a very similar > signature that doesn't quite use the existing framework is a bit confusing. > Perhaps we could add a `WebCallbacks<enum, enum>` variant? You're probably not > the only callback with that use case; I know some of the CredentialManager code > could use the same. > > On that note... > > https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... > public/platform/WebPushPermissionCallback.h:22: virtual void onError() = 0; > Are you sure you're not going to need to pass any error state down from the > embedder when the status can't be checked? Perhaps it's worth putting a > failure-types enum together as well.
https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... File public/platform/WebPushPermissionCallback.h (right): https://codereview.chromium.org/658723003/diff/100001/public/platform/WebPush... public/platform/WebPushPermissionCallback.h:22: virtual void onError() = 0; On 2014/10/28 11:23:35, Mike West wrote: > Are you sure you're not going to need to pass any error state down from the > embedder when the status can't be checked? Perhaps it's worth putting a > failure-types enum together as well. So right now the spec does not really pass any arguments with the kind of error you got. I think with the new typedef I just built we should be able to upgrade it easily if I need a new enum.
Thanks for taking another pass. public/ LGTM % nit. https://codereview.chromium.org/658723003/diff/140001/public/platform/WebPush... File public/platform/WebPushPermissionStatus.h (right): https://codereview.chromium.org/658723003/diff/140001/public/platform/WebPush... public/platform/WebPushPermissionStatus.h:16: WebPushPermissionTypeLast = WebPushPermissionStatusDefault Nit: s/WebPushPermissionTypeLast/WebPushPermissionStatusLast/
https://codereview.chromium.org/658723003/diff/140001/public/platform/WebPush... File public/platform/WebPushPermissionStatus.h (right): https://codereview.chromium.org/658723003/diff/140001/public/platform/WebPush... public/platform/WebPushPermissionStatus.h:16: WebPushPermissionTypeLast = WebPushPermissionStatusDefault On 2014/10/29 12:43:02, Mike West wrote: > Nit: s/WebPushPermissionTypeLast/WebPushPermissionStatusLast/ Done.
The CQ bit was checked by miguelg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/658723003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
LayoutTests/push_messaging/push-messaging-api-surface-expected.txt:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
LayoutTests/push_messaging/push-messaging-api-surface-expected.txt
Hunk #1 FAILED at 6.
1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/push_messaging/push-messaging-api-surface-expected.txt.rej
Patch: LayoutTests/push_messaging/push-messaging-api-surface-expected.txt
Index: LayoutTests/push_messaging/push-messaging-api-surface-expected.txt
diff --git a/LayoutTests/push_messaging/push-messaging-api-surface-expected.txt
b/LayoutTests/push_messaging/push-messaging-api-surface-expected.txt
index
33d754f2b26be515c4963a3b88a7779d762ecbe6..d1ec752527086df4f60addd004dab390b7fdcf3d
100644
--- a/LayoutTests/push_messaging/push-messaging-api-surface-expected.txt
+++ b/LayoutTests/push_messaging/push-messaging-api-surface-expected.txt
@@ -6,6 +6,8 @@ On success, you will see a series of "PASS" messages, followed
by "TEST COMPLETE
PASS !!navigator.push is true
PASS !!navigator.push.register is true
PASS navigator.push.register("senderId").constructor is Promise
+PASS !!navigator.push.hasPermission is true
+PASS navigator.push.hasPermission().constructor is Promise
PASS successfullyParsed is true
TEST COMPLETE
The CQ bit was unchecked by commit-bot@chromium.org
The CQ bit was checked by miguelg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/658723003/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as 184638 |
