Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
178253007:
Parse manifest file with app.service_worker.script. (Closed)
|
Created:
6 years, 10 months ago by scheib Modified:
6 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionParse manifest file with app.service_worker.script.
Add parsing logic to load a manifest specifying a service worker script.
This is a precursor to registering and using that script.
BUG=344917
R=jyasskin@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255424
Patch Set 1 : destructor added #
Total comments: 24
Patch Set 2 : #
Total comments: 24
Patch Set 3 : #
Total comments: 2
Patch Set 4 : Moved test utils out of base #Messages
Total messages: 52 (0 generated)
https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/api/_manifest_features.json:45: "app.service_worker": { Did we say something about defining this flag at the top level even for apps? https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifest_test.cc (right): https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifest_test.cc:97: std::string manifest_string = manifest_as_string_with_single_quotes; If you're going to copy a string anyway, just take it by value in the function parameter. https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifest_test.cc:99: JSONStringValueSerializer serializer(manifest_string); Let's use one of the existing parse-json-for-test functions: https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex... https://code.google.com/p/chromium/codesearch/#chromium/src/base/test/values_... https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifest_test.h (right): https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifest_test.h:54: class ManifestFromString : public Manifest { It'd be nice to make this Just Work for Manifest, but eh. https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:44: " 'script': 'service_worker.js'" Can you try one with an empty-but-present script value? https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_constants.cc (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_constants.cc:239: "Packaged apps must have background scripts, service worker, or background " I'd kind of like to avoid mentioning service workers in public error messages until they're enabled on the dev channel. Would it make sense to revert this until then? https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/background_info.cc (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.cc:124: bool BackgroundInfo::LoadServiceWorkerScript(const Extension* extension, The Service Worker is only going to actually work if chrome://flags/#enable-service-worker is enabled. Actively failing this load when it's disabled will make unittesting hard, but I wonder if there's some way we can remind people who try this that they need to flip a flag? https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.cc:146: if (has_service_worker()) { Could you put this in BgInfo::Parse so that its correctness doesn't depend on the order in which Parse() calls its helper functions? https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/background_info.h (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.h:26: static std::string GetServiceWorkerScript(const Extension* extension); Return a const std::string&? https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.h:31: static bool HasServiceWorker(const Extension* extension); It may be useful to provide an IsLazy() function so the system can distinguish persistent vs lazy/SW backends, but no need to do that right now.
Thanks. Some responses before I get into the code changes: https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... File chrome/common/extensions/api/_manifest_features.json (right): https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/api/_manifest_features.json:45: "app.service_worker": { On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > Did we say something about defining this flag at the top level even for apps? From the earlier email: """ On Tue, Feb 25, 2014 at 1:18 PM, Jeffrey Yasskin <jyasskin@google.com> wrote: (a) seems totally fine. On Tue, Feb 25, 2014 at 12:55 PM, Vincent Scheib <scheib@google.com> wrote: Jeffrey, Ben and I chatted and we ended up with: a) service_worker key appearing in multiple locations (top level for ext, under app for v2 apps), but otherwise being identical. With the transition plan of manifest-3 cleaning that up to a single location. """ https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifest_test.h (right): https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifest_test.h:54: class ManifestFromString : public Manifest { On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > It'd be nice to make this Just Work for Manifest, but eh. Constructor overloaded would be nice - but I don't see a clean way. I think this subclass is a bit cleaner & more readable at call sites than a static constructor helper. :\ https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/background_info.h (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.h:31: static bool HasServiceWorker(const Extension* extension); On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > It may be useful to provide an IsLazy() function so the system can distinguish > persistent vs lazy/SW backends, but no need to do that right now. Will we be supporting non lazy Service Workers? Why?
On Thu, Feb 27, 2014 at 8:16 PM, <scheib@chromium.org> wrote: > Thanks. Some responses before I get into the code changes: > > > > https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... > File chrome/common/extensions/api/_manifest_features.json (right): > > https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... > chrome/common/extensions/api/_manifest_features.json:45: > "app.service_worker": { > On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: >> >> Did we say something about defining this flag at the top level even > > for apps? > > From the earlier email: > """ > On Tue, Feb 25, 2014 at 1:18 PM, Jeffrey Yasskin <jyasskin@google.com> > wrote: > (a) seems totally fine. > > On Tue, Feb 25, 2014 at 12:55 PM, Vincent Scheib <scheib@google.com> > wrote: > Jeffrey, Ben and I chatted and we ended up with: > > a) service_worker key appearing in multiple locations (top level for > ext, under app for v2 apps), but otherwise being identical. With the > transition plan of manifest-3 cleaning that up to a single location. > > """ Heh, oops. SGTM. > https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... > File chrome/common/extensions/manifest_tests/extension_manifest_test.h > (right): > > https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... > chrome/common/extensions/manifest_tests/extension_manifest_test.h:54: > class ManifestFromString : public Manifest { > On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: >> >> It'd be nice to make this Just Work for Manifest, but eh. > > > Constructor overloaded would be nice - but I don't see a clean way. I > think this subclass is a bit cleaner & more readable at call sites than > a static constructor helper. :\ Yeah. > https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... > File extensions/common/manifest_handlers/background_info.h (right): > > https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... > extensions/common/manifest_handlers/background_info.h:31: static bool > HasServiceWorker(const Extension* extension); > On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: >> >> It may be useful to provide an IsLazy() function so the system can > > distinguish >> >> persistent vs lazy/SW backends, but no need to do that right now. > > > Will we be supporting non lazy Service Workers? Why? I don't think so. I meant "persistent backends" vs "(lazy/SW) backends". Because we have some code that currently checks has_lazy_background_page() but also applies to SWs. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Drive-by (sorta): Why is the service worker key being handled in "background info"? It seems like a separate concern. Without looking at the code in much detail I have a couple of comments: - If possible, could you use the manifest_types.json helpers for parsing? It's safer, less boilerplate, and documentation is trivial (since we can generate it). Have a look at how externally_connectable is parsed. - Is there any chance we can add support for the top-level service_workers key now as well? It would force the parsing to be factored in such a way to support it from the start. If it already is; apologies.
On Fri, Feb 28, 2014 at 9:35 AM, <kalman@chromium.org> wrote: > Drive-by (sorta): > > Why is the service worker key being handled in "background info"? It seems > like > a separate concern. > FWIW, since BackgroundInfo is where we currently handle the Background Page vs Event Page distinction, and Service Workers are another value along that same axis, I think it's a good place to put the SW bit. I could be wrong, of course, but it made sense to me. > Without looking at the code in much detail I have a couple of comments: > - If possible, could you use the manifest_types.json helpers for parsing? > It's > safer, less boilerplate, and documentation is trivial (since we can > generate > it). Have a look at how externally_connectable is parsed. > - Is there any chance we can add support for the top-level service_workers > key > now as well? It would force the parsing to be factored in such a way to > support > it from the start. If it already is; apologies. > > https://codereview.chromium.org/178253007/ > > > -- > You received this message because you are subscribed to the Google Groups > "Extensions reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to extensions-reviews+unsubscribe@chromium.org. > To view this discussion on the web visit https://groups.google.com/a/ > chromium.org/d/msgid/extensions-reviews/001a11331c3c98298804f37adb5e% > 40google.com. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
To complicate things further, also +yoz in case he has any opinions about this (as the author of all the manifest handler stuff). https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/background_info.cc (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.cc:351: keys::kPlatformAppBackgroundScripts (partly as a response to Jeffrey's comment) I would expect to see a service worker related key down here, but all the existing keys currently deal with a "background" key (either top-level or within "app"). It's a response to Jeffrey because that indicates to me this isn't the right place to do the manifest parsing. But I also could be wrong :)
On Fri, Feb 28, 2014 at 9:40 AM, Jeffrey Yasskin <jyasskin@chromium.org>wrote: > On Fri, Feb 28, 2014 at 9:35 AM, <kalman@chromium.org> wrote: > >> Drive-by (sorta): >> >> Why is the service worker key being handled in "background info"? It >> seems like >> a separate concern. >> > > FWIW, since BackgroundInfo is where we currently handle the Background > Page vs Event Page distinction, and Service Workers are another value along > that same axis, I think it's a good place to put the SW bit. I could be > wrong, of course, but it made sense to me. > > Yes, I believe it's the right class -- which may be due for a rename that entails the cluster of topics. It would be awkward to divide these responsibilities outside of this class. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/background_info.cc (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.cc:124: bool BackgroundInfo::LoadServiceWorkerScript(const Extension* extension, On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > The Service Worker is only going to actually work if > chrome://flags/#enable-service-worker is enabled. Actively failing this load > when it's disabled will make unittesting hard, but I wonder if there's some way > we can remind people who try this that they need to flip a flag? Set *error to a relevant message. By the way, you should be able to set flags in unittests' SetUp. This all seems fine. https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.cc:351: keys::kPlatformAppBackgroundScripts On 2014/02/28 17:48:14, kalman wrote: > (partly as a response to Jeffrey's comment) I would expect to see a service > worker related key down here, but all the existing keys currently deal with a > "background" key (either top-level or within "app"). > > It's a response to Jeffrey because that indicates to me this isn't the right > place to do the manifest parsing. But I also could be wrong :) This is the list of keys which, if any is present, causes this ManifestHandler to Parse at all. It's like a pre-check on whether to make the ManifestHandler do any work. So yes, you do need to put the service worker keys in here. (Your tests are able to pass because of AlwaysParseForType returning true anyway; I don't recall the exact bug that that fixed.)
On 2014/02/28 17:49:17, scheib wrote: > On Fri, Feb 28, 2014 at 9:40 AM, Jeffrey Yasskin <jyasskin@chromium.org>wrote: > > > On Fri, Feb 28, 2014 at 9:35 AM, <mailto:kalman@chromium.org> wrote: > > > >> Drive-by (sorta): > >> > >> Why is the service worker key being handled in "background info"? It > >> seems like > >> a separate concern. > >> > > > > FWIW, since BackgroundInfo is where we currently handle the Background > > Page vs Event Page distinction, and Service Workers are another value along > > that same axis, I think it's a good place to put the SW bit. I could be > > wrong, of course, but it made sense to me. > > > > > Yes, I believe it's the right class -- which may be due for a rename that > entails the cluster of topics. It would be awkward to divide these > responsibilities outside of this class. Why would it be awkward? manifest_handlers just parse manifest keys (and in some cases provide lightweight helpers), in this case "BackgroundInfo" because it parses the "background" manifest key. It's surprising to me that it should also parse the "service_workers" manifest key. Yes, perhaps you could just rename the class, but it's not clear to me why that's superior to a separate class.
On Fri, Feb 28, 2014 at 10:03 AM, <kalman@chromium.org> wrote: > On 2014/02/28 17:49:17, scheib wrote: > >> On Fri, Feb 28, 2014 at 9:40 AM, Jeffrey Yasskin <jyasskin@chromium.org >> >wrote: >> > > > On Fri, Feb 28, 2014 at 9:35 AM, <mailto:kalman@chromium.org> wrote: >> > >> >> Drive-by (sorta): >> >> >> >> Why is the service worker key being handled in "background info"? It >> >> seems like >> >> a separate concern. >> >> >> > >> > FWIW, since BackgroundInfo is where we currently handle the Background >> > Page vs Event Page distinction, and Service Workers are another value >> along >> > that same axis, I think it's a good place to put the SW bit. I could be >> > wrong, of course, but it made sense to me. >> > >> > >> Yes, I believe it's the right class -- which may be due for a rename that >> entails the cluster of topics. It would be awkward to divide these >> responsibilities outside of this class. >> > > Why would it be awkward? manifest_handlers just parse manifest keys (and > in some > cases provide lightweight helpers), in this case "BackgroundInfo" because > it > parses the "background" manifest key. It's surprising to me that it should > also > parse the "service_workers" manifest key. Yes, perhaps you could just > rename the > class, but it's not clear to me why that's superior to a separate class. > It looks like a bunch of call sites are going to need to check whether the backend is either LazyBackground or ServiceWorker. Putting the two in separate classes will make that more awkward, either because we'll have to do 2 checks, or because we'll do the !PersistentBackground check and then wonder if that covers all the cases. https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common... > https://codereview.chromium.org/178253007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> It looks like a bunch of call sites are going to need to check whether the > backend is either LazyBackground or ServiceWorker. Putting the two in > separate classes will make that more awkward, either because we'll have to > do 2 checks, or because we'll do the !PersistentBackground check and then > wonder if that covers all the cases. > > https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common... I don't really understand your point here. You need to go and update all the relevant callers anyway (change to "HasLazyBackgroundPage" to "HasLazyBackgroundPageOrServiceWorker"). It seems just as reasonable to change the #include to a higher-level concept than checking a manifest key and use that, whatever it may be.
On Fri, Feb 28, 2014 at 9:59 AM, <yoz@chromium.org> wrote: > > https://codereview.chromium.org/178253007/diff/80001/ > extensions/common/manifest_handlers/background_info.cc#newcode351 > extensions/common/manifest_handlers/background_info.cc:351: > keys::kPlatformAppBackgroundScripts > On 2014/02/28 17:48:14, kalman wrote: > >> (partly as a response to Jeffrey's comment) I would expect to see a >> > service > >> worker related key down here, but all the existing keys currently deal >> > with a > >> "background" key (either top-level or within "app"). >> > > It's a response to Jeffrey because that indicates to me this isn't the >> > right > >> place to do the manifest parsing. But I also could be wrong :) >> > > This is the list of keys which, if any is present, causes this > ManifestHandler to Parse at all. It's like a pre-check on whether to > make the ManifestHandler do any work. So yes, you do need to put the > service worker keys in here. > > (Your tests are able to pass because of AlwaysParseForType returning > true anyway; I don't recall the exact bug that that fixed.) > > Sorry, you're saying that AlwaysParseForType as currently implemented is a bug? If not, then ::Keys() is redundant for packaged app related keys, and kPlatformApps* keys would all be redundant here, and probably should be removed? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Feb 28, 2014 at 10:17 AM, <kalman@chromium.org> wrote: > > It looks like a bunch of call sites are going to need to check whether the >> backend is either LazyBackground or ServiceWorker. Putting the two in >> separate classes will make that more awkward, either because we'll have to >> do 2 checks, or because we'll do the !PersistentBackground check and then >> wonder if that covers all the cases. >> > > > https://code.google.com/p/chromium/codesearch/#chromium/ > src/extensions/common/manifest_handlers/background_ > info.cc&ct=xref_usages&gs=cpp:extensions::class-BackgroundInfo:: > HasLazyBackgroundPage%28const%252520extensions::Extension% > 252520*%29%40chromium/../../extensions/common/manifest_ > handlers/background_info.cc%25257Cdef&l=93&gsn=HasLazyBackgroundPage > > I don't really understand your point here. You need to go and update all > the > relevant callers anyway (change to "HasLazyBackgroundPage" to > "HasLazyBackgroundPageOrServiceWorker"). It seems just as reasonable to > change > the #include to a higher-level concept than checking a manifest key and use > that, whatever it may be. > > https://codereview.chromium.org/178253007/ > There is a set of related concepts for running code in an event page environment. There are mutually exclusive implementations for this. Parsing needs to be aware of all of these to check for errors, and answer general and specific questions about them. Dividing to multiple classes will require them to all be tightly coupled, or to have a hierarchy of the broad concept and mini handlers for each specific one. That's overkill, it will distribute the code to be more indirect and harder to reason about. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, the "parsing error for mutually exclusive concepts" thing is the compelling argument for having the parsing be in the same class. On Fri, Feb 28, 2014 at 10:22 AM, Vincent Scheib <scheib@chromium.org>wrote: > > > > On Fri, Feb 28, 2014 at 10:17 AM, <kalman@chromium.org> wrote: > >> >> It looks like a bunch of call sites are going to need to check whether >>> the >>> backend is either LazyBackground or ServiceWorker. Putting the two in >>> separate classes will make that more awkward, either because we'll have >>> to >>> do 2 checks, or because we'll do the !PersistentBackground check and then >>> wonder if that covers all the cases. >>> >> >> >> https://code.google.com/p/chromium/codesearch/#chromium/ >> src/extensions/common/manifest_handlers/background_ >> info.cc&ct=xref_usages&gs=cpp:extensions::class-BackgroundInfo:: >> HasLazyBackgroundPage%28const%252520extensions::Extension% >> 252520*%29%40chromium/../../extensions/common/manifest_ >> handlers/background_info.cc%25257Cdef&l=93&gsn=HasLazyBackgroundPage >> >> I don't really understand your point here. You need to go and update all >> the >> relevant callers anyway (change to "HasLazyBackgroundPage" to >> "HasLazyBackgroundPageOrServiceWorker"). It seems just as reasonable to >> change >> the #include to a higher-level concept than checking a manifest key and >> use >> that, whatever it may be. >> >> https://codereview.chromium.org/178253007/ >> > > There is a set of related concepts for running code in an event page > environment. There are mutually exclusive implementations for this. Parsing > needs to be aware of all of these to check for errors, and answer general > and specific questions about them. Dividing to multiple classes will > require them to all be tightly coupled, or to have a hierarchy of the broad > concept and mini handlers for each specific one. That's overkill, it will > distribute the code to be more indirect and harder to reason about. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/28 18:18:51, scheib wrote: > On Fri, Feb 28, 2014 at 9:59 AM, <mailto:yoz@chromium.org> wrote: > > > > > https://codereview.chromium.org/178253007/diff/80001/ > > extensions/common/manifest_handlers/background_info.cc#newcode351 > > extensions/common/manifest_handlers/background_info.cc:351: > > keys::kPlatformAppBackgroundScripts > > On 2014/02/28 17:48:14, kalman wrote: > > > >> (partly as a response to Jeffrey's comment) I would expect to see a > >> > > service > > > >> worker related key down here, but all the existing keys currently deal > >> > > with a > > > >> "background" key (either top-level or within "app"). > >> > > > > It's a response to Jeffrey because that indicates to me this isn't the > >> > > right > > > >> place to do the manifest parsing. But I also could be wrong :) > >> > > > > This is the list of keys which, if any is present, causes this > > ManifestHandler to Parse at all. It's like a pre-check on whether to > > make the ManifestHandler do any work. So yes, you do need to put the > > service worker keys in here. > > > > (Your tests are able to pass because of AlwaysParseForType returning > > true anyway; I don't recall the exact bug that that fixed.) > > > > > Sorry, you're saying that AlwaysParseForType as currently implemented is a > bug? If not, then ::Keys() is redundant for packaged app related keys, and > kPlatformApps* keys would all be redundant here, and probably should be > removed? > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. No, there isn't a bug at present; what I mean is, AlwaysParseForType exists to provide a useful message if a platform app doesn't provide a background page (or a service worker). So yes, the ::Keys list is redundant for platform apps. But as a reader of the code, I find ::Keys to usefully catalogue the top-level keys that this manifest handler deals with, and that would become much less clear if we just removed all the platform app keys.
https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifest_test.cc (right): https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifest_test.cc:97: std::string manifest_string = manifest_as_string_with_single_quotes; On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > If you're going to copy a string anyway, just take it by value in the function > parameter. Done. https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifest_test.cc:99: JSONStringValueSerializer serializer(manifest_string); On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > Let's use one of the existing parse-json-for-test functions: > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex... > https://code.google.com/p/chromium/codesearch/#chromium/src/base/test/values_... Moved this functionality to base::test::ParseJsonDictionaryWithSingleQuotes https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/80001/chrome/common/extensions... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:44: " 'script': 'service_worker.js'" On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > Can you try one with an empty-but-present script value? Done. https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_constants.cc (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_constants.cc:239: "Packaged apps must have background scripts, service worker, or background " On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > I'd kind of like to avoid mentioning service workers in public error messages > until they're enabled on the dev channel. Would it make sense to revert this > until then? Done. Will follow up later with http://crbug.com/348647 https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/background_info.cc (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.cc:124: bool BackgroundInfo::LoadServiceWorkerScript(const Extension* extension, On 2014/02/28 17:59:09, Yoyo Zhou wrote: > On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > > The Service Worker is only going to actually work if > > chrome://flags/#enable-service-worker is enabled. Actively failing this load > > when it's disabled will make unittesting hard, but I wonder if there's some > way > > we can remind people who try this that they need to flip a flag? > > Set *error to a relevant message. > > By the way, you should be able to set flags in unittests' SetUp. This all seems > fine. Done. https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.cc:146: if (has_service_worker()) { On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > Could you put this in BgInfo::Parse so that its correctness doesn't depend on > the order in which Parse() calls its helper functions? Done. https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.cc:351: keys::kPlatformAppBackgroundScripts On 2014/02/28 17:59:09, Yoyo Zhou wrote: > On 2014/02/28 17:48:14, kalman wrote: > > (partly as a response to Jeffrey's comment) I would expect to see a service > > worker related key down here, but all the existing keys currently deal with a > > "background" key (either top-level or within "app"). > > > > It's a response to Jeffrey because that indicates to me this isn't the right > > place to do the manifest parsing. But I also could be wrong :) > > This is the list of keys which, if any is present, causes this ManifestHandler > to Parse at all. It's like a pre-check on whether to make the ManifestHandler do > any work. So yes, you do need to put the service worker keys in here. > > (Your tests are able to pass because of AlwaysParseForType returning true > anyway; I don't recall the exact bug that that fixed.) Done. https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... File extensions/common/manifest_handlers/background_info.h (right): https://codereview.chromium.org/178253007/diff/80001/extensions/common/manife... extensions/common/manifest_handlers/background_info.h:26: static std::string GetServiceWorkerScript(const Extension* extension); On 2014/02/28 01:54:37, Jeffrey Yasskin wrote: > Return a const std::string&? Done.
https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:115: " }," when do you plan on adding the parsing logic for just 'service_worker' i.e. not nested inside 'app'?
https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:115: " }," On 2014/03/03 19:56:07, kalman wrote: > when do you plan on adding the parsing logic for just 'service_worker' i.e. not > nested inside 'app'? After we have a hello world app working. https://code.google.com/p/chromium/issues/detail?id=346885
https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:115: " }," On 2014/03/03 20:25:46, scheib wrote: > On 2014/03/03 19:56:07, kalman wrote: > > when do you plan on adding the parsing logic for just 'service_worker' i.e. > not > > nested inside 'app'? > > After we have a hello world app working. > > https://code.google.com/p/chromium/issues/detail?id=346885 Why? It seems like minimal effort to add it right now, and diverging the behaviour for background scripts is a bit confusing.
Pawel, PTAL at base/test/values_test_util.* https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:115: " }," On 2014/03/03 20:34:22, kalman wrote: > On 2014/03/03 20:25:46, scheib wrote: > > On 2014/03/03 19:56:07, kalman wrote: > > > when do you plan on adding the parsing logic for just 'service_worker' i.e. > > not > > > nested inside 'app'? > > > > After we have a hello world app working. > > > > https://code.google.com/p/chromium/issues/detail?id=346885 > > Why? It seems like minimal effort to add it right now, and diverging the > behaviour for background scripts is a bit confusing. Discussed offline - it's not lots of work, but it's enough that this patch doesn't need the delay or added complexity for. Will follow up with https://code.google.com/p/chromium/issues/detail?id=346885 when it's the right time to do so (after we have a prototype of app working, or blocked in doing that).
https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.cc (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.cc:77: scoped_ptr<DictionaryValue> ParseJsonDictionaryWithSingleQuotes( Why not let the caller do things this code is doing?
https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.cc (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.cc:77: scoped_ptr<DictionaryValue> ParseJsonDictionaryWithSingleQuotes( On 2014/03/03 22:50:47, Paweł Hajdan Jr. wrote: > Why not let the caller do things this code is doing? To avoid code duplication and increase test readability. See https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension...
https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.cc (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.cc:77: scoped_ptr<DictionaryValue> ParseJsonDictionaryWithSingleQuotes( On 2014/03/03 22:50:47, Paweł Hajdan Jr. wrote: > Why not let the caller do things this code is doing? FWIW this is a function I've also often very much wanted but never had the motivation to write, so, thank you Vincent. Extensions tests often involve writing a lot of JSON (most notably manifests), and it's super painful and unreadable to be escaping all the time. We could have this be extensions-only but I think it would be a shame.
https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.cc (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.cc:79: std::string json_single_quotes_replaced = json.as_string(); Since you're making a copy here anyway, you should probably take a std::string by value instead of the StringPiece. https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.h (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.h:54: // double quotes for test readability. Returns an empty dictionary if the json Mention the EXPECT failure here. https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifest_test.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifest_test.cc:7: #include <algorithm> You don't use these new #includes anymore. https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifest_test.h (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifest_test.h:27: Manifest(scoped_ptr<base::DictionaryValue> manifest, const char* name); All of the uses of this constructor pass "" for the name, so consider removing the parameter. https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifest_test.h:46: protected: I think this can go back to "private". https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:45: CommandLine::ForCurrentProcess()->AppendSwitch( You have to remove the flag again at the end of the test, or all other unittests will see it set. (Note what happens if you move ServiceWorkerCommandLineRequired to be last in this file.) However, CommandLine provides no way to remove a switch. :-P We have FeatureSwitch for this purpose in extensions: https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common... We could define a FeatureSwitch for SWs with its default set by the CommandLine switch, or try promote it to base for Alec to use. https://codereview.chromium.org/178253007/diff/160001/extensions/common/manif... File extensions/common/manifest_handlers/background_info.cc (right): https://codereview.chromium.org/178253007/diff/160001/extensions/common/manif... extensions/common/manifest_handlers/background_info.cc:124: int background_solution_sum = (background_url_.is_valid() ? 1 : 0) + Adding up the booleans will give you the same answer, but I don't mind the explicit conversions either.
Thanks, ptal. https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.cc (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.cc:79: std::string json_single_quotes_replaced = json.as_string(); On 2014/03/04 01:41:48, Jeffrey Yasskin wrote: > Since you're making a copy here anyway, you should probably take a std::string > by value instead of the StringPiece. Done. https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.h (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.h:54: // double quotes for test readability. Returns an empty dictionary if the json On 2014/03/04 01:41:48, Jeffrey Yasskin wrote: > Mention the EXPECT failure here. Done. https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifest_test.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifest_test.cc:7: #include <algorithm> On 2014/03/04 01:41:48, Jeffrey Yasskin wrote: > You don't use these new #includes anymore. Done. https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifest_test.h (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifest_test.h:27: Manifest(scoped_ptr<base::DictionaryValue> manifest, const char* name); On 2014/03/04 01:41:48, Jeffrey Yasskin wrote: > All of the uses of this constructor pass "" for the name, so consider removing > the parameter. Done. https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifest_test.h:46: protected: On 2014/03/04 01:41:48, Jeffrey Yasskin wrote: > I think this can go back to "private". Done. https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:45: CommandLine::ForCurrentProcess()->AppendSwitch( On 2014/03/04 01:41:48, Jeffrey Yasskin wrote: > You have to remove the flag again at the end of the test, or all other unittests > will see it set. (Note what happens if you move ServiceWorkerCommandLineRequired > to be last in this file.) However, CommandLine provides no way to remove a > switch. :-P > > We have FeatureSwitch for this purpose in extensions: > https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common... > We could define a FeatureSwitch for SWs with its default set by the CommandLine > switch, or try promote it to base for Alec to use. Wonderfully this is no longer the case, base::test::TestSuite takes care of this: https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sui... I've added CHECKS so that others don't have the same concern when reading. https://codereview.chromium.org/178253007/diff/160001/extensions/common/manif... File extensions/common/manifest_handlers/background_info.cc (right): https://codereview.chromium.org/178253007/diff/160001/extensions/common/manif... extensions/common/manifest_handlers/background_info.cc:124: int background_solution_sum = (background_url_.is_valid() ? 1 : 0) + On 2014/03/04 01:41:48, Jeffrey Yasskin wrote: > Adding up the booleans will give you the same answer, but I don't mind the > explicit conversions either. Done. Yes, but explicit conversion reduces human's scanning / readability concern time.
lgtm, but of course I can't approve base/ https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/160001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:45: CommandLine::ForCurrentProcess()->AppendSwitch( On 2014/03/04 15:34:31, scheib wrote: > On 2014/03/04 01:41:48, Jeffrey Yasskin wrote: > > You have to remove the flag again at the end of the test, or all other > unittests > > will see it set. (Note what happens if you move > ServiceWorkerCommandLineRequired > > to be last in this file.) However, CommandLine provides no way to remove a > > switch. :-P > > > > We have FeatureSwitch for this purpose in extensions: > > > https://code.google.com/p/chromium/codesearch/#chromium/src/extensions/common... > > We could define a FeatureSwitch for SWs with its default set by the > CommandLine > > switch, or try promote it to base for Alec to use. > > Wonderfully this is no longer the case, base::test::TestSuite takes care of > this: > https://code.google.com/p/chromium/codesearch#chromium/src/base/test/test_sui... > > I've added CHECKS so that others don't have the same concern when reading. Oh, great! I will forget that pitfall then. :) https://codereview.chromium.org/178253007/diff/180001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/180001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:98: .Pass()))); You probably don't need a .Pass() here, since the scoped_ptr is a temporary.
Thanks jyasskin. phajdan.jr PTAL. https://codereview.chromium.org/178253007/diff/180001/chrome/common/extension... File chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc (right): https://codereview.chromium.org/178253007/diff/180001/chrome/common/extension... chrome/common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc:98: .Pass()))); On 2014/03/04 17:29:16, Jeffrey Yasskin wrote: > You probably don't need a .Pass() here, since the scoped_ptr is a temporary. Done.
https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.cc (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.cc:77: scoped_ptr<DictionaryValue> ParseJsonDictionaryWithSingleQuotes( On 2014/03/03 22:59:04, kalman wrote: > On 2014/03/03 22:50:47, Paweł Hajdan Jr. wrote: > > Why not let the caller do things this code is doing? > > FWIW this is a function I've also often very much wanted but never had the > motivation to write, so, thank you Vincent. Extensions tests often involve > writing a lot of JSON (most notably manifests), and it's super painful and > unreadable to be escaping all the time. We could have this be extensions-only > but I think it would be a shame. Extracting this to a function is fine. Having that function in base is non-obvious.
Thanks Pawel: https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... File base/test/values_test_util.cc (right): https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... base/test/values_test_util.cc:77: scoped_ptr<DictionaryValue> ParseJsonDictionaryWithSingleQuotes( On 2014/03/05 03:52:56, Paweł Hajdan Jr. wrote: > On 2014/03/03 22:59:04, kalman wrote: > > On 2014/03/03 22:50:47, Paweł Hajdan Jr. wrote: > > > Why not let the caller do things this code is doing? > > > > FWIW this is a function I've also often very much wanted but never had the > > motivation to write, so, thank you Vincent. Extensions tests often involve > > writing a lot of JSON (most notably manifests), and it's super painful and > > unreadable to be escaping all the time. We could have this be extensions-only > > but I think it would be a shame. > > Extracting this to a function is fine. Having that function in base is > non-obvious. Fair point. Here's my justification: It is highly related to the other Value related test utilities already here in base. Parsing a dictionary from text in tests is likely to come up in many scenarios. I first started writing the util in an extensions folder, but it really has nothing specific to it related to extensions. I've had previous desire when dealing with dictionaries of cached settings for a function like this as well. So I promoted it here to base::test::. I'll pull it out to just extensions if you think this is the wrong judgment call.
Moved out of base into extensions/common instead. jyasskin, PTAL.
On Tue, Mar 4, 2014 at 7:52 PM, <phajdan.jr@chromium.org> wrote: > > https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... > File base/test/values_test_util.cc (right): > > https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... > base/test/values_test_util.cc:77: scoped_ptr<DictionaryValue> > ParseJsonDictionaryWithSingleQuotes( > On 2014/03/03 22:59:04, kalman wrote: >> >> On 2014/03/03 22:50:47, Paweł Hajdan Jr. wrote: >> > Why not let the caller do things this code is doing? > > >> FWIW this is a function I've also often very much wanted but never had > > the >> >> motivation to write, so, thank you Vincent. Extensions tests often > > involve >> >> writing a lot of JSON (most notably manifests), and it's super painful > > and >> >> unreadable to be escaping all the time. We could have this be > > extensions-only >> >> but I think it would be a shame. > > > Extracting this to a function is fine. Having that function in base is > non-obvious. Pawel, when you don't like something, please provide more objection than this, and please respond when someone answers your objection. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry! I should have mentioned that I discussed in person with Pawel. On Wed, Mar 5, 2014 at 2:16 PM, Jeffrey Yasskin <jyasskin@chromium.org>wrote: > On Tue, Mar 4, 2014 at 7:52 PM, <phajdan.jr@chromium.org> wrote: > > > > > https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... > > File base/test/values_test_util.cc (right): > > > > > https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... > > base/test/values_test_util.cc:77: scoped_ptr<DictionaryValue> > > ParseJsonDictionaryWithSingleQuotes( > > On 2014/03/03 22:59:04, kalman wrote: > >> > >> On 2014/03/03 22:50:47, Paweł Hajdan Jr. wrote: > >> > Why not let the caller do things this code is doing? > > > > > >> FWIW this is a function I've also often very much wanted but never had > > > > the > >> > >> motivation to write, so, thank you Vincent. Extensions tests often > > > > involve > >> > >> writing a lot of JSON (most notably manifests), and it's super painful > > > > and > >> > >> unreadable to be escaping all the time. We could have this be > > > > extensions-only > >> > >> but I think it would be a shame. > > > > > > Extracting this to a function is fine. Having that function in base is > > non-obvious. > > Pawel, when you don't like something, please provide more objection > than this, and please respond when someone answers your objection. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Still LGTM.
On 2014/03/05 22:19:23, scheib wrote: > Sorry! I should have mentioned that I discussed in person with Pawel. Ah! Ok, sorry Pawel. > On Wed, Mar 5, 2014 at 2:16 PM, Jeffrey Yasskin <jyasskin@chromium.org>wrote: > > > On Tue, Mar 4, 2014 at 7:52 PM, <mailto:phajdan.jr@chromium.org> wrote: > > > > > > > > > https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... > > > File base/test/values_test_util.cc (right): > > > > > > > > > https://codereview.chromium.org/178253007/diff/160001/base/test/values_test_u... > > > base/test/values_test_util.cc:77: scoped_ptr<DictionaryValue> > > > ParseJsonDictionaryWithSingleQuotes( > > > On 2014/03/03 22:59:04, kalman wrote: > > >> > > >> On 2014/03/03 22:50:47, Paweł Hajdan Jr. wrote: > > >> > Why not let the caller do things this code is doing? > > > > > > > > >> FWIW this is a function I've also often very much wanted but never had > > > > > > the > > >> > > >> motivation to write, so, thank you Vincent. Extensions tests often > > > > > > involve > > >> > > >> writing a lot of JSON (most notably manifests), and it's super painful > > > > > > and > > >> > > >> unreadable to be escaping all the time. We could have this be > > > > > > extensions-only > > >> > > >> but I think it would be a shame. > > > > > > > > > Extracting this to a function is fine. Having that function in base is > > > non-obvious. > > > > Pawel, when you don't like something, please provide more objection > > than this, and please respond when someone answers your objection. > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by scheib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/178253007/260001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by scheib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/178253007/260001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by scheib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/178253007/260001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by scheib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/178253007/260001
Message was sent while issue was closed.
Committed patchset #4 manually as r255424 (presubmit successful).
Message was sent while issue was closed.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/scheib@chromium.org/178253007/260001
Message was sent while issue was closed.
Failed to apply patch for chrome/chrome_tests_unit.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/chrome_tests_unit.gypi
Hunk #1 FAILED at 1827.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/chrome_tests_unit.gypi.rej
Patch: chrome/chrome_tests_unit.gypi
Index: chrome/chrome_tests_unit.gypi
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index
f947ff9a21419192ee2e3da5a93ab0dcffe037ca..961961a7deb432ef5edbc1b871063793c947ef19
100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -1827,6 +1827,7 @@
'common/extensions/manifest_tests/extension_manifests_portsinpermissions_unittest.cc',
'common/extensions/manifest_tests/extension_manifests_requirements_unittest.cc',
'common/extensions/manifest_tests/extension_manifests_sandboxed_unittest.cc',
+
'common/extensions/manifest_tests/extension_manifests_service_worker_unittest.cc',
'common/extensions/manifest_tests/extension_manifests_storage_unittest.cc',
'common/extensions/manifest_tests/extension_manifests_ui_unittest.cc',
'common/extensions/manifest_tests/extension_manifests_update_unittest.cc',
|
