[ServiceWorker] Implement ServiceWorkerContainerHost.Register [chromium/src : master]
Han Leon (Gerrit)
ps#1 just defines ServiceWorkerContainerHost.Register prototype and some other necessary mojom/typemap, PTAL to see whether it makes sense now, if OK then I'll complete the real impl based on it.
2 comments:
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Patch Set #1, Line 21:
RegisterI'd like to impl only Register within this CL, and will pay attention to possible IPC ordering issue while investigating/implementing, if necessary will also impl other methods together. If you already have some concerns about this please let me know ;-)
File third_party/WebKit/public/platform/modules/serviceworker/web_service_worker_error.mojom:
Patch Set #1, Line 7:
enum WebServiceWorkerErrorTypeTo avoid making this CL too big, should I separate all changes(mojom,traits,typemap) for this to another CL and land it firstly? WDYT? Thanks.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
I'd like to understand the provider lifetimes issues before adding on more mojo IPCs.
However I think it'd be convenient to move WebServiceWorkerError etc into mojom. Can you do this without adding additional typemaps? i.e., make code in both content and WebKit use the genererated mojom type directly or a common C++ type? We could probably move stuff to WebKit/common/serviceworker or WebKit/common/modules/serviceworker instead of WebKit/public/platform/modules/serviceworker ("public" and "web" is supposed to go away: see https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-dev/L8D46_Yabyc)
1 comment:
Patch Set #1, Line 7:
enum WebServiceWorkerErrorType
To avoid making this CL too big, should I separate all changes(mojom,traits,typemap) for this to ano […]
Yes landing mojom changes sounds good
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
Patch Set 1:
(1 comment)
I'd like to understand the provider lifetimes issues before adding on more mojo IPCs.
However I think it'd be convenient to move WebServiceWorkerError etc into mojom. Can you do this without adding additional typemaps? i.e., make code in both content and WebKit use the genererated mojom type directly or a common C++ type? We could probably move stuff to WebKit/common/serviceworker or WebKit/common/modules/serviceworker instead of WebKit/public/platform/modules/serviceworker ("public" and "web" is supposed to go away: see https://groups.google.com/a/chromium.org/forum/#!topic/platform-architecture-dev/L8D46_Yabyc)
Yeah I understand our ultimate target is to put WebServiceWorkerError into mojom under WebKit/common/, and let all content and WebKit codes use the generated C++ mojom type instead of current WebServiceWorkerError, thus we can eliminate WebServiceWorkerError at last. However, I found that current WebServiceWorkerError usages are so huge and it's hard to replace all at once, I think we'd better use typemapping for now so that we can start our mojofication work soon based on it, and after mojofication done we can get a clearer image around all usages of those types and then apply mojom types everywhere at last. IIUC actually all typemappings should disappear at last, they're all temporary solution to make mojofication work easier, mojom types should be used directly everywhere when that final day comes ;-)
Makoto Shimazu (Gerrit)
Yeah I understand our ultimate target is to put WebServiceWorkerError into mojom under WebKit/common/, and let all content and WebKit codes use the generated C++ mojom type instead of current WebServiceWorkerError, thus we can eliminate WebServiceWorkerError at last. However, I found that current WebServiceWorkerError usages are so huge and it's hard to replace all at once, I think we'd better use typemapping for now so that we can start our mojofication work soon based on it, and after mojofication done we can get a clearer image around all usages of those types and then apply mojom types everywhere at last. IIUC actually all typemappings should disappear at last, they're all temporary solution to make mojofication work easier, mojom types should be used directly everywhere when that final day comes ;-)
I think typemapping is still good way for structs since we can have additional constraints for each params, but for enums I agree with that:)
Anyway, I think we can go forward with typemapping if changing all of usage at once seems difficult.
5 comments:
File content/browser/service_worker/service_worker_provider_host.h:
Patch Set #1, Line 411:
mojom::ServiceWorkerProviderHostImplements mojom::ServiceWorkerProviderHost.
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #1, Line 1128:
NOTREACHED();NOTIMPLEMENTED();
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Patch Set #1, Line 12:
// TODO(leonhsl): implement this interface.I think it'd be better to put this comment between L.25-26.
Patch Set #1, Line 21:
Register
I'd like to impl only Register within this CL, and will pay attention to possible IPC ordering issue […]
That seems good for me:)
File third_party/WebKit/public/platform/modules/serviceworker/web_service_worker_error.mojom:
Patch Set #1, Line 7:
enum WebServiceWorkerErrorType
Yes landing mojom changes sounds good
+1 to land it firstly.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
Will continue updating this CL from now on.
4 comments:
File content/browser/service_worker/service_worker_provider_host.h:
Patch Set #1, Line 411:
d UnregisterWorkerFetchContext(mImplements mojom::ServiceWorkerProviderHost.
Done
File content/browser/service_worker/service_worker_provider_host.cc:
NOTIMPLEMENTED();
Done
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Patch Set #1, Line 12:
// mojom::ServiceWorkerProviderHost is a br
I think it'd be better to put this comment between L.25-26.
Done
File third_party/WebKit/public/platform/modules/serviceworker/web_service_worker_error.mojom:
Patch Set #1, Line 7:
// Error codes for service wor
+1 to land it firstly.
Separated to another CL https://chromium-review.googlesource.com/c/620169 and made this CL depend on that.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Makoto Shimazu (Gerrit)
lgtm with a nit
Patch set 3:Code-Review +1
1 comment:
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Patch Set #3, Line 16:
the service worker javascript |script_url|The following might be a bit simpler:
Registers a service worker from |script_url| with |options|.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
Could we add the implementaiton along with this patch?
Regarding typemapping: I've been burned by complexity in BUILD.gn caused by typemapping and my preference is to default to no typemaps until proven needed.
Especially for simple structs like this, I'd rather the mojom just replace the C++ type when it's introduced.
When we do typemaps I'd like them to be in WebKit/common
instead of content/common or else it boxes us into adding stuff to content/renderer instead of Blink; in WebKit/common we have more freedom to put it in Blink when it fits.
Anyway we can keep the typemap for this patch since WebKit/common/serviceworker is not yet established, but it's something to avoid for future patches.
2 comments:
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #3, Line 914:
NOTIMPLEMENTED();Could we add the implementation along with this patch? Although I'm a fan of small patches, in this case it seems not so useful, it's hard for security to review a Mojo interface for example without the implementation along with it.
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Patch Set #3, Line 19:
// and null |attributes|.This can be cleaned up with something like:
"On success, |error| is kNone with |registration| and |attributes| set. Otherwise, |error| and |error_msg| describe the failure."
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
Patch Set 3:
(2 comments)
Could we add the implementaiton along with this patch?
Regarding typemapping: I've been burned by complexity in BUILD.gn caused by typemapping and my preference is to default to no typemaps until proven needed.
Especially for simple structs like this, I'd rather the mojom just replace the C++ type when it's introduced.
When we do typemaps I'd like them to be in WebKit/common
instead of content/common or else it boxes us into adding stuff to content/renderer instead of Blink; in WebKit/common we have more freedom to put it in Blink when it fits.Anyway we can keep the typemap for this patch since WebKit/common/serviceworker is not yet established, but it's something to avoid for future patches.
Got it, now I'm clear about our attitude towards typemapping ;-)
3 comments:
Patch Set #3, Line 914:
NOTIMPLEMENTED();
Could we add the implementation along with this patch? Although I'm a fan of small patches, in this […]
Yeah this CL's target is to implement at least the |Register| method completely.
Will keep updating this CL continuously, at the same time I'll probably put some inline questions to ask for your help, and once it's all OK I'll ask for an formal review.
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Patch Set #3, Line 16:
the service worker javascript |script_url|The following might be a bit simpler: […]
Ack and Thanks!
Patch Set #3, Line 19:
// and null |attributes|.
This can be cleaned up with something like: […]
Ack and Thanks!
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
ps#4 is ready for review now, PTAL, Thanks.
3 comments:
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:
Patch Set #4, Line 375:
RegisterAndExpect(remote_endpoint.host_ptr()->get(),As SWProviderHost handles Register IPC now, I think we need to move Register related tests into unit test file of SWProviderHost. But I'd like to keep them here for now and reconstruct all tests for all methods of SWProviderHost mojo interface later in unit test file of SWProviderHost.
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Patch Set #3, Line 16:
a service worker from |script_url| with |oAck and Thanks!
Done
Patch Set #3, Line 19:
Register(url.mojom.Url script_url, ServiceWorkerRegistrationOptions options)Ack and Thanks!
Done
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
I'd like to put more informative comments so I added some inline questions for discussions.
3 comments:
File content/browser/service_worker/service_worker_dispatcher_host.h:
Patch Set #4, Line 118:
bool AllowServiceWorker(const GURL& scope,How about a comment like the following?
"
This is used only by SWProviderHost to impl Register() now. Once all other related IPCs (GetRegistration, GetRegistrations, GetRegistrationReady etc.) handlers moved into SWProviderHost, this function will be also moved into SWProviderHost at last.
"
File content/child/service_worker/service_worker_provider_context.h:
// For service worker clients. Gets the mojom::ServiceWorkerProviderHost* for
// sending requests to browser-side ServiceWorkerProviderHost. May be nullptr
// if above OnNetworkProviderDestroyed() has already been called.Improve the comment like following?
"
Called only by document(renderer frame) client of SW for now. Please see comments of |provider_host_|. May be nullptr if above OnNetworkProviderDestroyed() has already been called.
"Patch Set #4, Line 136:
provider_host_Add a comment like following?
"
Only document(renderer frame) client of SW uses this to send requests to browser-side ServiceWorkerProviderHost. Both non-document(shared worker) client context and SW context itself do not need to request ServiceWorkerProviderHost to do anything, because non-document context does not expose ServiceWorkerContainer javascript object and SW context's WorkerNavigator.serviceWorker is not really wired up yet (https://crbug.com/371690), for such cases keeping |provider_host_| here is just for keeping alive the corresponding ServiceWorkerProviderHost instance in browser-side.
Note: Currently this is always bound on main thread.
"
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
This looks pretty good!
18 comments:
Patch Set #4, Line 118:
bool AllowServiceWorker(const GURL& scope,
How about a comment like the following? […]
Hmm, since all this function does is call ChromeContentBrowserClient::AllowServiceWorker(), maybe dispatcher host should just have a resource_context() getter and then provider host can call that directly?
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #4, Line 946:
// Need to run the mojo response callback anyway, although the aboveWhy needed? Does Mojo crash if we don't call the callbacK?
Patch Set #4, Line 949:
kUnknownI think we should avoid "Unknown" whenever possible since it makes debugging more difficult. Maybe add a "kBadMessage" ?
lines 943-976 is quite long just to kill a renderer on a bad message, which we expect to be an exceptional case. could we just combine all the checks into one bad message?
Something like this:
if (!IsValidRegisterMessage(script_url, options)) {
bad_message::ReceivedBadMessage(dispatcher_host_,
bad_message::SWPH_REGISTER);
std::move(callback).Run(...)
return;
}Could we also verify that this provider host is allowed to register a service worker, i.e., it's a provider for a Document?
Patch Set #4, Line 989:
"ServiceWorker", "ServiceWorkerProviderHost::RegisterServiceWorker", this,Register
Patch Set #4, Line 993:
base::Bind(&ServiceWorkerProviderHost::RegistrationComplete, AsWeakPtr(),Can this use BindOnce and std::move(callback)
What happens if the callback is never called? Does Mojo crash? We might never run the callback if this ProviderHost is destructed.
Patch Set #4, Line 1021:
context_->GetLiveRegistration(registration_id);// ServiceWorkerRegisterJob calls its completion callback, which results in this function being called, while the registration is live.
Patch Set #4, Line 1027:
AsWeakPtr(), registration, &info, &attrs);Hm, this reminds me of a cleanup I planned to do, I think this function can move to ProviderHost but let's keep it this way in this patch. I'll investigate in parallel and it should be a separate CL anyway.
File content/child/service_worker/service_worker_provider_context.h:
// For service worker clients. Gets the mojom::ServiceWorkerProviderHost* for
// sending requests to browser-side ServiceWorkerProviderHost. May be nullptr
// if above OnNetworkProviderDestroyed() has already been called.
Improve the comment like following? […]
I'd just add on to the first sentence to say it can only be called "for clients that are Documents, see comments of |provider_host_|."
Patch Set #4, Line 136:
provider_host_
Add a comment like following? […]
I'd tighten this up though:
// The |provider_host_| interface (which implements functions for navigator.serviceWorker), can only be used if |this| is a provider for a Document, since currently navigator.serviceWorker is only implemented for Document (https://crbug.com/371690). For other providers, |provider_host_| is just for keeping alive the corresponding browser-side ServiceWorkerProviderHost instance.
File content/child/service_worker/service_worker_provider_context.cc:
Patch Set #4, Line 133:
const {We should DCHECK here that this is a Document
File content/child/service_worker/web_service_worker_provider_impl.cc:
Patch Set #4, Line 75:
auto scope = (GURL)pattern;can this be GURL scope(pattern);
Patch Set #4, Line 76:
auto sw_url = (GURL)script_url;I'd prefer something like
GURL script_url(web_script_url)
Patch Set #4, Line 160:
static_cast<int32_t>(error)I think mojom enums have a special way to convert to a string, which would be nicer than the int value.
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Patch Set #4, Line 16:
// Registers a service worker from |script_url| with |options|.Also add Corresponds to navigator.serviceWorker.register().
Patch Set #4, Line 21:
mojo.common.mojom.String16? error_msg,we could just use string not string16. the legacy code used an old webkit convention
File content/common/service_worker/service_worker_types.mojom:
Patch Set #4, Line 33:
Containerlet's avoid using "Container" these days
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
1 comment:
File content/browser/bad_message.h:
Patch Set #4, Line 75:
SWDH_REGISTER_CANNOT = 51,Can these be marked obsolete?
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
19 comments:
File content/browser/bad_message.h:
Patch Set #4, Line 75:
SWDH_REGISTER_BAD_URL = 49, // obsolete; no longer used
Can these be marked obsolete?
Patch Set #4, Line 118:
bool AllowServiceWorker(const GURL& scope,
Hmm, since all this function does is call ChromeContentBrowserClient::AllowServiceWorker(), maybe di […]
In addition to resource_context_, implementation of this function also needs a utility function GetWebContents() defined locally in service_worker_dispatcher_host.cc file. So I want to avoid copying GetWebContents() codes into service_worker_provider_host.cc.
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #4, Line 946:
void ServiceWorkerProviderHost::RegistrationComplete(
Why needed? Does Mojo crash if we don't call the callbacK?
Yeah, right after the execution of this function, mojo will check whether the response callback has been run or scheduled for a later run, if no, will raise a crash like:
[147560:147560:0824/173403.703703:8726266657920:FATAL:interface_endpoint_client.cc(32)] Check failed: !is_valid. The callback passed to ServiceWorkerProviderHost::Register() was never run.
I think we should avoid "Unknown" whenever possible since it makes debugging more difficult. […]
In product env the above ReceivedBadMessage() should have killed renderer process, so actually renderer process has no chance to get these response values.
In some tests env, ReceivedBadMessage() behavior can be overridden just as following:
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_worker_dispatcher_host_unittest.cc?rcl=19a781d5c693db99f8c1dcc4e4cd7c3844796e58&l=113,
so that test codes can check whether a bad message error has been raised or not, but test codes never care about the mojo response values. So I used kUnknown here.Patch Set #4, Line 976:
ServiceWorkerVersionAttributes attrs;lines 943-976 is quite long just to kill a renderer on a bad message, which we expect to be an excep […]
Done
Patch Set #4, Line 977:
dispatcher_host_->GetRegistrationObjectInfoAndVersionAttributes(Could we also verify that this provider host is allowed to register a service worker, i.e. […]
Done
Patch Set #4, Line 989:
std::string error_message;Register
Done and Thanks!
Can this use BindOnce and std::move(callback) […]
Changed to BindOnce but must adapt it to a repeating callback because context_->RegisterServiceWorker() can't take a once callback.
If ProviderHost destroyed, its' member the mojo binding destruction will just destroy all pending callbacks without any DCHECK crash.
// ServiceWorkerRegisterJob calls its completion callback, which results in this function being call […]
Done
Hm, this reminds me of a cleanup I planned to do, I think this function can move to ProviderHost but […]
Ack
File content/child/service_worker/service_worker_provider_context.h:
const std::set<uint32_t>& used_features() const;
// For service worker clients. Creates a ServiceWorkerWorkerClienI'd just add on to the first sentence to say it can only be called "for clients that are Documents, […]
Done
Patch Set #4, Line 136:
f aboveI'd tighten this up though: […]
Done
File content/child/service_worker/service_worker_provider_context.cc:
Patch Set #4, Line 133:
attrs->active = state->active->info();
We should DCHECK here that this is a Document
can this be GURL scope(pattern);
Done
Patch Set #4, Line 76:
GURL pattern(web_pattern);I'd prefer something like […]
Done
I think mojom enums have a special way to convert to a string, which would be nicer than the int val […]
Done
File content/common/service_worker/service_worker_provider_interfaces.mojom:
Also add Corresponds to navigator.serviceWorker.register().
Done
Patch Set #4, Line 21:
// synchronous XHR in the main thread. So We need to know whether the worker iswe could just use string not string16. […]
Done
File content/common/service_worker/service_worker_types.mojom:
Patch Set #4, Line 33:
Struct fo
let's avoid using "Container" these days
Done
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
some quick comments, didn't read the whole thing again yet
4 comments:
Patch Set #4, Line 118:
bool AllowServiceWorker(const GURL& scope,
In addition to resource_context_, implementation of this function also needs a utility function GetW […]
GetWebContents() is only two lines though... I'd prefer to just copy it now to make the dependency more clear (i.e., ProviderHost only needs resource_context from DispatcherHost to make this call)
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:
Patch Set #4, Line 375:
RegisterAndExpect(remote_endpoint.host_ptr()->get(),
As SWProviderHost handles Register IPC now, I think we need to move Register related tests into unit […]
I'd prefer to have the tests move with the implementation but maybe I don't understand the concern. Is it difficult to add unit tests for ServiceWorkerProviderHost::Register() in ServiceWorkerProviderHostTest now?
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #4, Line 946:
void ServiceWorkerProviderHost::RegistrationComplete(
Yeah, right after the execution of this function, mojo will check whether the response callback has […]
I think if you use mojo::ReportBadMessage, you don't need to worry about the callback.
https://groups.google.com/a/chromium.org/d/msg/chromium-mojo/NNsogKNurlA/HdyJxDsBAQAJ
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #6, Line 915:
if (!IsValidRegisterMessage(script_url, options)) {If we can move to the mojo::BadMessage() thing, it may be better for IsValidRegisterMessage() to also return a string for the specific error so we can have more info on crash reports (I'm assuming they show up there, not sure)
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
Sorry for so late response as I was OOO on Mon&Tue.
4 comments:
Patch Set #4, Line 118:
bool AllowServiceWorker(const GURL& scope,
GetWebContents() is only two lines though... […]
Done
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:
Patch Set #4, Line 375:
RegisterAndExpect(remote_endpoint.host_ptr()->get(),
I'd prefer to have the tests move with the implementation but maybe I don't understand the concern. […]
I was thinking that we'd better keep the tests for all SWContainer related IPCs at one place, including Register,GetRegistration etc. As some tests logic may involve multiple of them. Then after all got mojofied, we can move them all at once easily.
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #4, Line 946:
void ServiceWorkerProviderHost::RegistrationComplete(
I think if you use mojo::ReportBadMessage, you don't need to worry about the callback. […]
Seems mojo::ReportBadMessage is a better choice.. But I have no idea how to adapt unit test to check the output from mojo::ReportBadMessage. Anyway I've asked about this in the original discussion thread.
And, I've confirmed that even with mojo::ReportBadMessage we still need to either run the response callback or just close the mojo binding explicitly, seems the latter is preferred from Ken's comment in the above discussion thread.
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #6, Line 915:
if (!IsValidRegisterMessage(script_url, options)) {
If we can move to the mojo::BadMessage() thing, it may be better for IsValidRegisterMessage() to als […]
Ack. Yeah if we decided to use mojo::BadMessage() instead, we can provide more details to it.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
1 comment:
Seems mojo::ReportBadMessage is a better choice.. […]
Based on the knowledge we got at https://groups.google.com/a/chromium.org/forum/#!topic/chromium-mojo/NNsogKNurlA, we need to consider whether we adopt mojo::ReportBadMessage() or not.
Pros:
In mojo world bad_message::ReceivedBadMessage() should be considered as deprecated eventually.
No need to add new enum values into low level bad_message.h file.
We are able to adapt our unit test to check output of mojo::ReportBadMessage by using mojo::edk::SetDefaultProcessErrorCallback.
Cons:
Hard to filter crash reports for now, according from the discussion at http://crbug.com/756115.I'd prefer to use mojo::ReportBadMessage right now. Another option is to apply it later after above Cons be addressed by Mojo team. WDYT? Thanks.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
sgtm to use mojo::ReportBadMessage as I expect any crash report difficulty to be fixed soon as everyone using Mojo will need it.
Han Leon (Gerrit)
Patch Set 7:
sgtm to use mojo::ReportBadMessage as I expect any crash report difficulty to be fixed soon as everyone using Mojo will need it.
Done.
2 comments:
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #6, Line 915:
base::nullopt, base::nullopt);
Ack. Yeah if we decided to use mojo::BadMessage() instead, we can provide more details to it.
Done
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #9, Line 932:
std::move(callback).Run(blink::mojom::ServiceWorkerErrorType::kUnknown,Here I just run the callback with non-sense parameters rather than closing the mojo binding |binding_|. Because |this| is expected to be destroyed by error handler of |binding_|, but binding_.Close() won't invoke the error handler, so I think we'd better do nothing to |binding_| here and wait for the error handler invocation later due to termination of renderer process.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
This is looking good.
16 comments:
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:
Patch Set #9, Line 250:
void RegisterAndExpect(mojom::ServiceWorkerContainerHost* container_host,hm, let's call this Register(), for consistency with the other functions. This should be the default function that most people use and SendRegister() is for the weird cases where we don't have an expectation since the callback doesn't get invoked.
File content/browser/service_worker/service_worker_provider_host.h:
Patch Set #9, Line 410:
// Callbacks from ServiceWorkerContextCore.There's only one function though. Maybe "Callback for Register()." is clearer.
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #9, Line 54:
"Origins are not match or some of them can not access service worker.";Maybe: "Origins are not matching, or some cannot access service worker."
Patch Set #9, Line 910:
DCHECK(dispatcher_host_);Ah this isn't true anymore. |dispatcher_host_| is now a weak ptr that can be nullptr at any time. We should just return if it's nullptr for consistency with the other Send* functions.
Patch Set #9, Line 932:
std::move(callback).Run(blink::mojom::ServiceWorkerErrorType::kUnknown,
Here I just run the callback with non-sense parameters rather than closing the mojo binding |binding […]
I see. Suggest revising the comment to "ReportBadMessage() will kill the renderer process, but Mojo complains if the callback is not run. Just run it with nonsense arguments."
Patch Set #9, Line 933:
std::string(kServiceWorkerRegisterErrorPrefix),To make clear it's nonsense, let's just use std::string()
Patch Set #9, Line 938:
kWebServiceWorkerClientTypeWindowI think this should be part of IsValidRegisterMessage(). We dont' expect the renderer to send us this message when the client type isn't window.
Patch Set #9, Line 984:
// this function being called, while the registration is live.Suggest moving this comment to line 987, right before the DCHECK.
File content/child/service_worker/service_worker_provider_context.h:
Patch Set #9, Line 137:
abovedon't need to mention "above"
Suggest turning this comma into a period
File content/child/service_worker/service_worker_provider_context.cc:
Patch Set #9, Line 183:
DCHECK(provider_type_ == SERVICE_WORKER_PROVIDER_FOR_WINDOW);DCHECK_EQ
File content/child/service_worker/web_service_worker_provider_impl.cc:
Patch Set #9, Line 90:
this, "Scope", pattern.spec(), "Script URL", script_url.spec());this should be moved to line 94
Patch Set #9, Line 93:
if (context_->container_host()) {Prefer early return if (!context_->container_host()), it'll save indentation especially as lines 88-90 will be moved here.
Patch Set #9, Line 163:
this, "Error", oss.str(), "Message", error_msg ? *error_msg : "Success");Oops I didn't realize it'd be so hard to get the mojo enum string.
Can you factor out a helper function ErrorTypeToString()? It can just be in this file, or in ServiceWorkerUtils maybe. The TRACE macro arguments aren't run unless tracing is on, so it's better to keep the conversion to string inside the TRACE.
Patch Set #9, Line 164:
if (error != blink::mojom::ServiceWorkerErrorType::kNone) {to avoid double negatives, maybe switch the if/else to be if (error == kNone) { /* OK stuff */} else { /* error stuff */}
File content/common/service_worker/service_worker_types.mojom:
Patch Set #9, Line 34:
// ServiceWorkerRegistrationOptions.Let's just delete lines 33-34, it doesn't say anything beyond the struct Name.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
PTAnL, Thanks.
16 comments:
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:
Patch Set #9, Line 250:
void Register(mojom::ServiceWorkerContainerHost* container_host,hm, let's call this Register(), for consistency with the other functions. […]
Done
File content/browser/service_worker/service_worker_provider_host.h:
Patch Set #9, Line 410:
// Callback for Register().
There's only one function though. Maybe "Callback for Register()." is clearer.
Done
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #9, Line 54:
"Origins are not matching, or some cannot access service worker.";Maybe: "Origins are not matching, or some cannot access service worker. […]
Done
Patch Set #9, Line 910:
if (!dispatcher_host_ || !IsContextAlive()) {Ah this isn't true anymore. |dispatcher_host_| is now a weak ptr that can be nullptr at any time. […]
Done. If |dispatcher_host_| has destroyed I think it should match bellowing case of ServiceWorkerErrorType::kAbort, so merged this condition with !IsContextAlive().
Patch Set #9, Line 932:
std::string(), base::nullopt, base::nullopt);I see. […]
Done
Patch Set #9, Line 933:
return;
To make clear it's nonsense, let's just use std::string()
Done
Patch Set #9, Line 938:
source_context(),I think this should be part of IsValidRegisterMessage(). […]
Done. Agree that the register request from a non-window client should be reported as a bad message.
Patch Set #9, Line 984:
DCHECK(registration);
Suggest moving this comment to line 987, right before the DCHECK.
don't need to mention "above"
Done
Suggest turning this comma into a period
Done
File content/child/service_worker/service_worker_provider_context.cc:
Patch Set #9, Line 183:
DCHECK_EQ(SERVICE_WORKER_PROVIDER_FOR_WINDOW, provider_type_);DCHECK_EQ
Done
File content/child/service_worker/web_service_worker_provider_impl.cc:
Patch Set #9, Line 90:
callbacks->OnError(blink::WebServiceWorkerError(
this should be moved to line 94
Done
Patch Set #9, Line 93:
return;Prefer early return if (!context_->container_host()), it'll save indentation especially as lines 88- […]
Done and Thanks! This also helped to find out a missing error case handler! In case that container_host() is already nullptr, we still need to run callbacks->OnError().
Patch Set #9, Line 163:
const base::Optional<ServiceWorkerVersionAttributes>& attributes) {Oops I didn't realize it'd be so hard to get the mojo enum string. […]
Done. I think putting ErrorTypeToString() into ServiceWorkerUtils is proper as it may be used later generally.
Got it that we'd better to keep related codes inside the TRACE sentence.Patch Set #9, Line 164:
TRACE_EVENT_ASYNC_END2(to avoid double negatives, maybe switch the if/else to be if (error == kNone) { /* OK stuff */} else […]
Done
File content/common/service_worker/service_worker_types.mojom:
Patch Set #9, Line 34:
[Native]
Let's just delete lines 33-34, it doesn't say anything beyond the struct Name.
Done
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
LGTM!
Patch set 10:Code-Review +1
2 comments:
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:
Patch Set #10, Line 339:
auto host = CreateProviderHostWithDispatcherHost(could you write out std::unique_ptr<ServiceWorkerProviderHost>?
I only like auto when it's clear what the type is, here we don't know if it's a raw ptr or something.
File content/child/service_worker/web_service_worker_provider_impl.cc:
Patch Set #10, Line 89:
error_message += "The host render frame has already aborted.";Not sure "host render frame" is a good error message for web devs.
Maybe: "Lost connection to the service worker system."
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
Patch Set 10: Code-Review+1
(2 comments)
LGTM!
Thank you very much for the review. Cheers~
2 comments:
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:
Patch Set #10, Line 339:
std::unique_ptr<ServiceWorkerProviderHost> host =could you write out std::unique_ptr<ServiceWorkerProviderHost>? […]
Done
File content/child/service_worker/web_service_worker_provider_impl.cc:
Patch Set #10, Line 89:
error_message += "Lost connection to the service worker system.";Not sure "host render frame" is a good error message for web devs. […]
Done and Thanks!
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
Han Leon would like Tom Sepez to review this change.
[ServiceWorker] Implement ServiceWorkerContainerHost.Register
BUG=755836
Change-Id: I212a65b6ebf972af18e9597344de14e52cde41a5
---
M content/browser/service_worker/embedded_worker_instance_unittest.cc
M content/browser/service_worker/service_worker_dispatcher_host.cc
M content/browser/service_worker/service_worker_dispatcher_host.h
M content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
M content/browser/service_worker/service_worker_provider_host.cc
M content/browser/service_worker/service_worker_provider_host.h
M content/child/service_worker/service_worker_dispatcher.cc
M content/child/service_worker/service_worker_dispatcher.h
M content/child/service_worker/service_worker_message_filter.cc
M content/child/service_worker/service_worker_provider_context.cc
M content/child/service_worker/service_worker_provider_context.h
M content/child/service_worker/web_service_worker_provider_impl.cc
M content/child/service_worker/web_service_worker_provider_impl.h
M content/common/DEPS
M content/common/service_worker/service_worker_container.mojom
M content/common/service_worker/service_worker_messages.h
M content/common/service_worker/service_worker_types.mojom
M content/common/service_worker/service_worker_types.typemap
M content/common/service_worker/service_worker_utils.cc
M content/common/service_worker/service_worker_utils.h
20 files changed, 446 insertions(+), 368 deletions(-)
Han Leon (Gerrit)
+Tom for all the mojom/messages review. Thanks.
Tom Sepez (Gerrit)
Mojom LGTM, looks like same URL sanity checks from old IPC are in place on mojom side.
Patch set 11:Code-Review +1
Kinuko Yasuda (Gerrit)
1 comment:
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #11, Line 965:
if (!context_)Wouldn't it hit DCHECK if we drop callback without firing?
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
1 comment:
File content/browser/service_worker/service_worker_provider_host.cc:
Patch Set #11, Line 965:
if (!dispatcher_host_ || !IsContextAlive()) {
Wouldn't it hit DCHECK if we drop callback without firing?
Ah big thanks for pointing out this! Addressed and PTAnL at ps#12.
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Kinuko Yasuda (Gerrit)
Thanks!
Patch set 12:Code-Review +1
1 comment:
File content/browser/service_worker/service_worker_provider_host.h:
Patch Set #12, Line 410:
// Callback for Register().This sounds like it's talking about RegisterCallback, while it's not. Should this [;aom;u sau say 'Callback for ServiceWorkerContext::RegisterServiceWorker' instead? (Or just remove the comment)
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Matt Falkenhagen (Gerrit)
1 comment:
Patch Set #12, Line 410:
// Callback for Register().
This sounds like it's talking about RegisterCallback, while it's not. […]
Yea, I noticed that after recommending this comment... "Callback for ServiceWorkerContext::RegisterServiceWorker" sgtm
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Han Leon (Gerrit)
Thanks all for review. Landing..
Patch set 13:Commit-Queue +2
1 comment:
File content/browser/service_worker/service_worker_provider_host.h:
Patch Set #12, Line 410:
// Callback for ServiceWorkerContextCore::RegisterServiceWorker().Yea, I noticed that after recommending this comment... […]
Done
To view, visit change 616740. To unsubscribe, or for help writing mail filters, visit settings.
Commit Bot (Gerrit)
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Change a code comment" https://chromium-review.googlesource.com/c/616740/13
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/616740/13
Bot data: {"action": "start", "triggered_at": "2017-09-04T05:46:43.0Z", "cq_cfg_revision": "c233a798f70a1d65037a1db95aa6de5192b768a4", "revision": "f9f9821c4e0efe772189871cba4b01764cbea647"}
Commit Bot (Gerrit)
Commit Bot merged this change.
[ServiceWorker] Implement ServiceWorkerContainerHost.Register
BUG=755836
Change-Id: I212a65b6ebf972af18e9597344de14e52cde41a5
Reviewed-on: https://chromium-review.googlesource.com/616740
Commit-Queue: Han Leon <leon...@intel.com>
Reviewed-by: Kinuko Yasuda <kin...@chromium.org>
Reviewed-by: Tom Sepez <tse...@chromium.org>
Reviewed-by: Matt Falkenhagen <fal...@chromium.org>
Reviewed-by: Makoto Shimazu <shi...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499474}
---
M content/browser/service_worker/embedded_worker_instance_unittest.cc
M content/browser/service_worker/service_worker_dispatcher_host.cc
M content/browser/service_worker/service_worker_dispatcher_host.h
M content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
M content/browser/service_worker/service_worker_provider_host.cc
M content/browser/service_worker/service_worker_provider_host.h
M content/child/service_worker/service_worker_dispatcher.cc
M content/child/service_worker/service_worker_dispatcher.h
M content/child/service_worker/service_worker_message_filter.cc
M content/child/service_worker/service_worker_provider_context.cc
M content/child/service_worker/service_worker_provider_context.h
M content/child/service_worker/web_service_worker_provider_impl.cc
M content/child/service_worker/web_service_worker_provider_impl.h
M content/common/DEPS
M content/common/service_worker/service_worker_container.mojom
M content/common/service_worker/service_worker_messages.h
M content/common/service_worker/service_worker_types.mojom
M content/common/service_worker/service_worker_types.typemap
M content/common/service_worker/service_worker_utils.cc
M content/common/service_worker/service_worker_utils.h
20 files changed, 451 insertions(+), 368 deletions(-)