Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1706603004:
Replace web_cache_messages.h with Mojo service. (Closed)
|
Created:
4 years, 10 months ago by leonhsl(Using Gerrit) Modified:
4 years, 9 months ago Reviewers:
Ken Rockot(use gerrit already), jochen (gone - plz use gerrit), limasdf, jam, Sam McNally, Tom Sepez, Anand Mistry (off Chromium) CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blundell+watchlist_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, droger+watchlist_chromium.org, jam, limasdf, qsr+mojo_chromium.org, sdefresne+watchlist_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplace web_cache_messages.h with Mojo service.
Implement WebCache Mojo interface to receive/handle
the requests which were sent via the Chromium IPC messages
defined in web_cache_messages.h.
BUG=577685
Committed: https://crrev.com/43dce4dc40cb884c02d92efaab99be737726b451
Cr-Commit-Position: refs/heads/master@{#380598}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed Ken's comments #
Total comments: 6
Patch Set 3 : Addressed Anand's comments #
Total comments: 6
Patch Set 4 : Addressed Sam's comments #
Total comments: 6
Patch Set 5 : Rebase only #Patch Set 6 : WebCacheService --> WebCache and fix trybot failure #Patch Set 7 : Rebase only #
Total comments: 2
Patch Set 8 : std::map operator[]-->find() and fix trybot failure #Patch Set 9 : Add code comment #Patch Set 10 : Rebase only #Patch Set 11 : Rebase only #
Total comments: 4
Messages
Total messages: 83 (31 generated)
leon.han@intel.com changed reviewers: + amistry@chromium.org, rockot@chromium.org, sammc@chromium.org
Hi, Anand, Ken, Sam, This CL is for components/web_cache/common/web_cache_messages.h Mojo conversion, which I think is relatively straightforward work and should have no dependency on current Mojo infra development. Would you PTAL then let me know "what I'm doing and how I'm doing" are correct or not? I hope this CL would be a start point to participate the Mojo conversion work led by you and then make contributions continuously. Thanks~
This is great, thanks for doing it! Mostly just comments for stylistic convention. https://codereview.chromium.org/1706603004/diff/1/components/web_cache.gypi File components/web_cache.gypi (right): https://codereview.chromium.org/1706603004/diff/1/components/web_cache.gypi#n... components/web_cache.gypi:9: 'target_name': 'web_cache_common', I think we should rename this to web_cache_mojo_bindings or web_cache_mojom or something. https://codereview.chromium.org/1706603004/diff/1/components/web_cache.gypi#n... components/web_cache.gypi:17: 'includes': [ '../mojo/mojom_bindings_generator.gypi'], For mojom_bindings_generator.gypi you just use 'sources', rather than 'variables.mojom_files'. The latter is only for mojom_bindings_generator_explicit.gypi, but its use is discouraged now. https://codereview.chromium.org/1706603004/diff/1/components/web_cache/browse... File components/web_cache/browser/web_cache_manager.h (right): https://codereview.chromium.org/1706603004/diff/1/components/web_cache/browse... components/web_cache/browser/web_cache_manager.h:144: typedef std::map<int, WebCacheServicePtr*> WebCacheServicesMap; We can use move-only values with std::map now, i.e. std::map<int, WebCacheServicePtr>. Please don't do manual memory management here. https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... File components/web_cache/common/BUILD.gn (right): https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... components/web_cache/common/BUILD.gn:7: mojom("common") { It's a bit unconventional to have a target named "common" be a mojom target. I think we should reinforce the convention that mojom lives in foo/public/interfaces, so can you move this BUILD.gn, OWNERS, and the mojom file to //components/web_cache/public/interfaces? https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... File components/web_cache/common/OWNERS (left): https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... components/web_cache/common/OWNERS:2: # new sandbox escapes. Please leave these entries here but change *_messages*.h to *.mojom https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... File components/web_cache/common/web_cache_service.mojom (right): https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... components/web_cache/common/web_cache_service.mojom:5: module web_cache; Please always use a mojom suffix for module names, i.e. web_cache.mojom. This way generated bindings will consistently live in a mojom sub-namespace and avoid collision with non-mojom types.
Hi Ken, Thanks a lot for kindly comments! Uploaded patchset #2, PTAL, Thanks~ https://codereview.chromium.org/1706603004/diff/1/components/web_cache.gypi File components/web_cache.gypi (right): https://codereview.chromium.org/1706603004/diff/1/components/web_cache.gypi#n... components/web_cache.gypi:9: 'target_name': 'web_cache_common', On 2016/02/18 03:45:06, Ken Rockot wrote: > I think we should rename this to web_cache_mojo_bindings or web_cache_mojom or > something. Renamed to web_cache_mojo_bindings. https://codereview.chromium.org/1706603004/diff/1/components/web_cache.gypi#n... components/web_cache.gypi:17: 'includes': [ '../mojo/mojom_bindings_generator.gypi'], On 2016/02/18 03:45:06, Ken Rockot wrote: > For mojom_bindings_generator.gypi you just use 'sources', rather than > 'variables.mojom_files'. The latter is only for > mojom_bindings_generator_explicit.gypi, but its use is discouraged now. Understood and Done. https://codereview.chromium.org/1706603004/diff/1/components/web_cache/browse... File components/web_cache/browser/web_cache_manager.h (right): https://codereview.chromium.org/1706603004/diff/1/components/web_cache/browse... components/web_cache/browser/web_cache_manager.h:144: typedef std::map<int, WebCacheServicePtr*> WebCacheServicesMap; On 2016/02/18 03:45:06, Ken Rockot wrote: > We can use move-only values with std::map now, i.e. std::map<int, > WebCacheServicePtr>. Please don't do manual memory management here. Understood and Done. https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... File components/web_cache/common/BUILD.gn (right): https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... components/web_cache/common/BUILD.gn:7: mojom("common") { On 2016/02/18 03:45:06, Ken Rockot wrote: > It's a bit unconventional to have a target named "common" be a mojom target. I > think we should reinforce the convention that mojom lives in > foo/public/interfaces, so can you move this BUILD.gn, OWNERS, and the mojom file > to //components/web_cache/public/interfaces? Understood and Done. https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... File components/web_cache/common/OWNERS (left): https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... components/web_cache/common/OWNERS:2: # new sandbox escapes. On 2016/02/18 03:45:06, Ken Rockot wrote: > Please leave these entries here but change *_messages*.h to *.mojom Done. https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... File components/web_cache/common/web_cache_service.mojom (right): https://codereview.chromium.org/1706603004/diff/1/components/web_cache/common... components/web_cache/common/web_cache_service.mojom:5: module web_cache; On 2016/02/18 03:45:06, Ken Rockot wrote: > Please always use a mojom suffix for module names, i.e. web_cache.mojom. > > This way generated bindings will consistently live in a mojom sub-namespace and > avoid collision with non-mojom types. Understood and done.
Overall good. Just a few minor comments. https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/br... File components/web_cache/browser/web_cache_manager.cc (right): https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/br... components/web_cache/browser/web_cache_manager.cc:344: DCHECK(service.get()); I think you should be able to do: DCHECK(service); https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/pu... File components/web_cache/public/interfaces/web_cache_service.mojom (right): https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/pu... components/web_cache/public/interfaces/web_cache_service.mojom:9: // Each call is independent, overlapping calls are possible. I don't understand this comment. These are all one-way messages, and an interface provides strict ordering of calls. I can't see how overlap is relevant here. https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/re... File components/web_cache/renderer/DEPS (right): https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/re... components/web_cache/renderer/DEPS:3: "+content/public/common", I think these are meant to be alphabetically ordered. So common before renderer.
Hi, Anand, Thanks a lot for kindly help! Addressed comments and uploaded patchset #3. PTAL, Thanks~ https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/br... File components/web_cache/browser/web_cache_manager.cc (right): https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/br... components/web_cache/browser/web_cache_manager.cc:344: DCHECK(service.get()); On 2016/02/18 12:33:57, Anand Mistry wrote: > I think you should be able to do: > DCHECK(service); Done. https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/pu... File components/web_cache/public/interfaces/web_cache_service.mojom (right): https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/pu... components/web_cache/public/interfaces/web_cache_service.mojom:9: // Each call is independent, overlapping calls are possible. On 2016/02/18 12:33:57, Anand Mistry wrote: > I don't understand this comment. These are all one-way messages, and an > interface provides strict ordering of calls. I can't see how overlap is relevant > here. Yeah I think you're right, this interface has no such worry. Removed this comment. https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/re... File components/web_cache/renderer/DEPS (right): https://codereview.chromium.org/1706603004/diff/20001/components/web_cache/re... components/web_cache/renderer/DEPS:3: "+content/public/common", On 2016/02/18 12:33:57, Anand Mistry wrote: > I think these are meant to be alphabetically ordered. So common before renderer. Sorry my miss. Modified.
https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/br... File components/web_cache/browser/web_cache_manager.h (right): https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/br... components/web_cache/browser/web_cache_manager.h:146: typedef std::map<int, WebCacheServicePtr> WebCacheServicesMap; Please comment on what the key is here. https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/re... File components/web_cache/renderer/web_cache_render_process_observer.cc (right): https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/re... components/web_cache/renderer/web_cache_render_process_observer.cc:31: if (service_registry) { Is this necessary? https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/re... components/web_cache/renderer/web_cache_render_process_observer.cc:32: service_registry->AddService<WebCacheService>(base::Bind( Omit the template parameter. The compiler can deduce it.
Hi, Sam, Thanks a lot for kindly comments! Uploaded patchset #4 to address, PTAL, Thanks~ https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/br... File components/web_cache/browser/web_cache_manager.h (right): https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/br... components/web_cache/browser/web_cache_manager.h:146: typedef std::map<int, WebCacheServicePtr> WebCacheServicesMap; On 2016/02/23 02:37:33, Sam McNally wrote: > Please comment on what the key is here. Done. https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/re... File components/web_cache/renderer/web_cache_render_process_observer.cc (right): https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/re... components/web_cache/renderer/web_cache_render_process_observer.cc:31: if (service_registry) { On 2016/02/23 02:37:33, Sam McNally wrote: > Is this necessary? Uh, I saw other calling codes(e.g. ChromeRenderProcessObserver) have such a guard, so I also put here. I think maybe it's better than none? https://codereview.chromium.org/1706603004/diff/40001/components/web_cache/re... components/web_cache/renderer/web_cache_render_process_observer.cc:32: service_registry->AddService<WebCacheService>(base::Bind( On 2016/02/23 02:37:33, Sam McNally wrote: > Omit the template parameter. The compiler can deduce it. Done.
The CQ bit was checked by limasdf@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/60001
Description was changed from ========== Replace web_cache_messages.h with Mojo service. Implement WebCacheService Mojo interface to receive/handle the requests which were sent via the Chromium IPC messages defined in web_cache_messages.h. BUG=577685 ========== to ========== Replace web_cache_messages.h with Mojo service. Implement WebCacheService Mojo interface to receive/handle the requests which were sent via the Chromium IPC messages defined in web_cache_messages.h. BUG=577685 ==========
limasdf@gmail.com changed reviewers: - limasdf@gmail.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
limasdf@gmail.com changed reviewers: + limasdf@gmail.com
https://codereview.chromium.org/1706603004/diff/60001/components/web_cache.gypi File components/web_cache.gypi (right): https://codereview.chromium.org/1706603004/diff/60001/components/web_cache.gy... components/web_cache.gypi:13: 'web_cache/common/web_cache_service.mojom', I think this should be 'web_cache/public/interface/web_cache_service.mojom',
'web_cache/public/interface/web_cache_service.mojom', => 'web_cache/public/interfaces/web_cache_service.mojom',
https://codereview.chromium.org/1706603004/diff/60001/components/web_cache.gypi File components/web_cache.gypi (right): https://codereview.chromium.org/1706603004/diff/60001/components/web_cache.gy... components/web_cache.gypi:13: 'web_cache/common/web_cache_service.mojom', On 2016/02/24 12:09:11, limasdf wrote: > I think this should be 'web_cache/public/interface/web_cache_service.mojom', I think this has been discussed before and the conclusion was not to do this. Unless a decision has been made to change the pattern, don't use "_service" in your mojom's file name. https://codereview.chromium.org/1706603004/diff/60001/components/web_cache/pu... File components/web_cache/public/interfaces/web_cache_service.mojom (right): https://codereview.chromium.org/1706603004/diff/60001/components/web_cache/pu... components/web_cache/public/interfaces/web_cache_service.mojom:7: interface WebCacheService { Sam/Ken: I know we've avoided calling our services "Service". Should this name just be WebCache?
I think in general a Service suffix is either redundant or inappropriate, but never necessary. So I agree, let's avoid doing this. On Wed, Feb 24, 2016 at 12:40 PM, <amistry@chromium.org> wrote: > https://codereview.chromium.org/1706603004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
limasdf@gmail.com changed reviewers: - limasdf@gmail.com
Description was changed from ========== Replace web_cache_messages.h with Mojo service. Implement WebCacheService Mojo interface to receive/handle the requests which were sent via the Chromium IPC messages defined in web_cache_messages.h. BUG=577685 ========== to ========== Replace web_cache_messages.h with Mojo service. Implement WebCache Mojo interface to receive/handle the requests which were sent via the Chromium IPC messages defined in web_cache_messages.h. BUG=577685 ==========
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Thank you all for comments! Uploaded patchset #6, PTAL, Thanks~ https://codereview.chromium.org/1706603004/diff/60001/components/web_cache.gypi File components/web_cache.gypi (right): https://codereview.chromium.org/1706603004/diff/60001/components/web_cache.gy... components/web_cache.gypi:13: 'web_cache/common/web_cache_service.mojom', On 2016/02/24 12:09:11, limasdf wrote: > I think this should be 'web_cache/public/interface/web_cache_service.mojom', Done. https://codereview.chromium.org/1706603004/diff/60001/components/web_cache.gy... components/web_cache.gypi:13: 'web_cache/common/web_cache_service.mojom', On 2016/02/24 20:40:05, Anand Mistry wrote: > On 2016/02/24 12:09:11, limasdf wrote: > > I think this should be 'web_cache/public/interface/web_cache_service.mojom', > > I think this has been discussed before and the conclusion was not to do this. > Unless a decision has been made to change the pattern, don't use "_service" in > your mojom's file name. Done. https://codereview.chromium.org/1706603004/diff/60001/components/web_cache/pu... File components/web_cache/public/interfaces/web_cache_service.mojom (right): https://codereview.chromium.org/1706603004/diff/60001/components/web_cache/pu... components/web_cache/public/interfaces/web_cache_service.mojom:7: interface WebCacheService { On 2016/02/24 20:40:05, Anand Mistry wrote: > Sam/Ken: I know we've avoided calling our services "Service". Should this name > just be WebCache? Done.
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Sorry this fell of my radar. I will take another look in the next few hours.
The CQ bit was checked by limasdf@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
On 2016/02/29 21:12:07, Ken Rockot wrote: > Sorry this fell of my radar. I will take another look in the next few hours. Sorry the trybot failed because I was missing rebase for several days. As I still can't trigger trybot without lgtm, would you please help trigger it? Thanks~
The CQ bit was checked by rockot@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/120001
On 2016/03/02 03:02:09, commit-bot: I haz the power wrote: > Dry run: CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1706603004/120001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1706603004/120001 Afaik, someone already requested that your right to trigger trybot. If you still dont have it, ping me whenever you want :)
On 2016/03/02 03:05:17, limasdf wrote: > On 2016/03/02 03:02:09, commit-bot: I haz the power wrote: > > Dry run: CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1706603004/120001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1706603004/120001 > > Afaik, someone already requested that your right to trigger trybot. > If you still dont have it, ping me whenever you want :) Hi limasdf@, Thank you very much for kindly help! -:)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Minor comment but otherwise looks good https://codereview.chromium.org/1706603004/diff/120001/components/web_cache/b... File components/web_cache/browser/web_cache_manager.cc (right): https://codereview.chromium.org/1706603004/diff/120001/components/web_cache/b... components/web_cache/browser/web_cache_manager.cc:344: DCHECK(service); You should not use operator[] to look up an entry that may not be there (even if it's always expected to be there). Instead write something like: auto it = web_cache_services_.find(allocation->first); DCHECK(it != web_cache_services_.end()); const mojom::WebCachePtr& service = it->second; // optional, for clarity it->second->SetCacheCapacities... This avoids mutating the map just for lookup.
Hi, sorry the trybot failed for some tests code,, would you please help trigger the trybot for the latest patchset #9? Thanks in advance~ Updates: 1. Addressed Ken's comment: Use find() instead of operator[] to look up std::map to avoid mutating the map. 2. Modified extensions/browser/guest_view/web_view/web_view_guest.cc to fix browser_tests failure. This CL adds one obligation that WebCacheManager::ClearCacheForProcess(renderer_id) *MUST* always be called between WebCacheManager::Add(renderer_id) and WebCacheManager::Remove(renderer_id). That is to say you can manipulate one renderer's cache only if it has already been managed by WebCacheManager. I think this obligation makes sense, WDYT? Thanks~ https://codereview.chromium.org/1706603004/diff/120001/components/web_cache/b... File components/web_cache/browser/web_cache_manager.cc (right): https://codereview.chromium.org/1706603004/diff/120001/components/web_cache/b... components/web_cache/browser/web_cache_manager.cc:344: DCHECK(service); On 2016/03/02 03:18:10, Ken Rockot wrote: > You should not use operator[] to look up an entry that may not be there (even if > it's always expected to be there). Instead write something like: > > auto it = web_cache_services_.find(allocation->first); > DCHECK(it != web_cache_services_.end()); > > const mojom::WebCachePtr& service = it->second; // optional, for clarity > it->second->SetCacheCapacities... > > This avoids mutating the map just for lookup. Done. Yes it is, great appreciation for so kindly explanations, will keep in mind later.
The CQ bit was checked by limasdf@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/03/02 08:53:35, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Hi, Thanks for the help to trigger the trybot~ win_chromium_rel_ng: failure seems is not caused by this CL, should be because of unstable trybot. chromium_presubmit: Need LGTM from OWNERS later. ** Presubmit ERRORS ** Missing LGTM from OWNERS of dependencies added to DEPS: '+content/public/common', '+mojo/public',
lgtm
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
leon.han@intel.com changed reviewers: + jam@chromium.org, jochen@chromium.org
Hi, Anand, Sam, would you please take another look on the latest patchset? +jam@ would you PTAL for the added dependency on //content/public/common and overall codes as well? +jochen@ would you PTAL as OWNER of //components/web_cache? Thanks all~
lgtm
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by leon.han@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
leon.han@intel.com changed reviewers: + tsepez@chromium.org
+tsepez@ Would you PTAL for OWNER review: ipc/ipc_message_start.h tools/ipc_fuzzer/* Thanks~
lgtm
https://codereview.chromium.org/1706603004/diff/200001/components/web_cache/b... File components/web_cache/browser/BUILD.gn (right): https://codereview.chromium.org/1706603004/diff/200001/components/web_cache/b... components/web_cache/browser/BUILD.gn:18: "//content/public/common", Should there be a corresponding addition to the gypi? https://codereview.chromium.org/1706603004/diff/200001/components/web_cache/r... File components/web_cache/renderer/BUILD.gn (right): https://codereview.chromium.org/1706603004/diff/200001/components/web_cache/r... components/web_cache/renderer/BUILD.gn:14: "//content/public/common", Same with this.
Thanks Sam for kindly comments~ Inline answer. https://codereview.chromium.org/1706603004/diff/200001/components/web_cache/b... File components/web_cache/browser/BUILD.gn (right): https://codereview.chromium.org/1706603004/diff/200001/components/web_cache/b... components/web_cache/browser/BUILD.gn:18: "//content/public/common", On 2016/03/10 22:59:34, Sam McNally wrote: > Should there be a corresponding addition to the gypi? In components/web_cache.gypi, 'web_cache_browser' depends on --> 'content/content.gyp:content_browser' depends on --> 'content/content.gyp:content_common', which covers both //content/common and //content/public/common. So no need to add deps again. I think the root cause is that gyp has no separate targets for //content/common and //content/public/common.. https://codereview.chromium.org/1706603004/diff/200001/components/web_cache/r... File components/web_cache/renderer/BUILD.gn (right): https://codereview.chromium.org/1706603004/diff/200001/components/web_cache/r... components/web_cache/renderer/BUILD.gn:14: "//content/public/common", On 2016/03/10 22:59:34, Sam McNally wrote: > Same with this. The same reason with components/web_cache/browser/BUILD.gn case.
lgtm
Big thanks all of you for great help~ I would like to land this CL now because I noticed that jochen@ is also //content OWNER, adding dependency on //content/public/common into components/web_cache/renderer/DEPS file should have gotten approval. Seems John is away this week and I think this dependency does NOT break any rule so sorry I won't wait for him now;-)
The CQ bit was checked by leon.han@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1706603004/#ps200001 (title: "Rebase only")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706603004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706603004/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Replace web_cache_messages.h with Mojo service. Implement WebCache Mojo interface to receive/handle the requests which were sent via the Chromium IPC messages defined in web_cache_messages.h. BUG=577685 ========== to ========== Replace web_cache_messages.h with Mojo service. Implement WebCache Mojo interface to receive/handle the requests which were sent via the Chromium IPC messages defined in web_cache_messages.h. BUG=577685 Committed: https://crrev.com/43dce4dc40cb884c02d92efaab99be737726b451 Cr-Commit-Position: refs/heads/master@{#380598} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/43dce4dc40cb884c02d92efaab99be737726b451 Cr-Commit-Position: refs/heads/master@{#380598} |
