Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2059573002:
Add initial stub implementation of LocalActivityResolver. (Closed)
|
Created:
4 years, 6 months ago by zentaro Modified:
4 years, 6 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd initial stub implementation of LocalActivityResolver.
ShouldChromeHandleUrl just returns true for now. In future it
will return true only if Chrome should handle this URL. Part of b\29248657
BUG=619137
Committed: https://crrev.com/7a610a093d5bd05845a4468a73e04f0e1a7e32f8
Cr-Commit-Position: refs/heads/master@{#399299}
Patch Set 1 #Patch Set 2 : Stub implementation of LocalActivityResolver. #
Total comments: 10
Patch Set 3 : Minor cleanups and actually build it. #
Total comments: 3
Patch Set 4 : Change default to true. #
Messages
Total messages: 24 (12 generated)
Description was changed from ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns false for now. In future it will return true if Chrome can determine which app should handle this url. BUG=29248657 ========== to ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns false for now. In future it will return true if Chrome can determine which app should handle this url. BUG=29248657 ==========
zentaro@chromium.org changed reviewers: + yusukes@chromium.org
Description was changed from ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns false for now. In future it will return true if Chrome can determine which app should handle this url. BUG=29248657 ========== to ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns false for now. In future it will return true if Chrome should handle this URL. BUG=29248657 ==========
thanks, lgtm with nits, please add elijahtaylor@ or lhchavez@ for OWNER review. https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:11: LocalActivityResolver::LocalActivityResolver() {} s/{}/= default;/ maybe? https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:13: LocalActivityResolver::~LocalActivityResolver() {} same https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.h (right): https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.h:7: base/macros.h https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.h:8: #include "url/gurl.h" nit: class GURL; might be better?
zentaro@google.com changed reviewers: + elijahtaylor@chromium.org
https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.h (right): https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Ah, sorry, please add these files to gn and gyp files too :)
On 2016/06/10 01:33:21, Yusuke Sato wrote: > https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... > File components/arc/intent_helper/local_activity_resolver.h (right): > > https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... > components/arc/intent_helper/local_activity_resolver.h:1: // Copyright 2016 The > Chromium Authors. All rights reserved. > Ah, sorry, please add these files to gn and gyp files too :) Cleaned up. And added to the build.
zentaro@google.com changed reviewers: + zentaro@google.com
https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:11: LocalActivityResolver::LocalActivityResolver() {} On 2016/06/10 01:28:28, Yusuke Sato wrote: > s/{}/= default;/ maybe? Done. https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:13: LocalActivityResolver::~LocalActivityResolver() {} On 2016/06/10 01:28:28, Yusuke Sato wrote: > same Done. https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.h (right): https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/10 01:33:21, Yusuke Sato wrote: > Ah, sorry, please add these files to gn and gyp files too :) Done. https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.h:7: On 2016/06/10 01:28:28, Yusuke Sato wrote: > base/macros.h Done. https://codereview.chromium.org/2059573002/diff/20001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.h:8: #include "url/gurl.h" On 2016/06/10 01:28:28, Yusuke Sato wrote: > nit: class GURL; might be better? Done.
thanks, still lgtm
lgtm with changes we should use crbug.com bugs for Chromium changes. What we've done in the past (since we don't have automatic mirroring) is to create stub bugs on the crbug.com side that link back to the internal bug. You can mark it Restrict-View-Google if there are confidential bits in it. https://codereview.chromium.org/2059573002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2059573002/diff/40001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:13: return false; maybe the default should be true? That would allow David to use this function when first implementing his click hooking code when checking in and would not regress click latency.
https://codereview.chromium.org/2059573002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2059573002/diff/40001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:13: return false; On 2016/06/10 19:40:27, elijahtaylor1 wrote: > maybe the default should be true? That would allow David to use this function > when first implementing his click hooking code when checking in and would not > regress click latency. Ah true, good idea.
Description was changed from ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns false for now. In future it will return true if Chrome should handle this URL. BUG=29248657 ========== to ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns false for now. In future it will return true if Chrome should handle this URL. BUG=619137 ==========
Description was changed from ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns false for now. In future it will return true if Chrome should handle this URL. BUG=619137 ========== to ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns true for now. In future it will return true only if Chrome should handle this URL. BUG=619137 ==========
Description was changed from ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns true for now. In future it will return true only if Chrome should handle this URL. BUG=619137 ========== to ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns true for now. In future it will return true only if Chrome should handle this URL. Part of b\29248657 BUG=619137 ==========
https://codereview.chromium.org/2059573002/diff/40001/components/arc/intent_h... File components/arc/intent_helper/local_activity_resolver.cc (right): https://codereview.chromium.org/2059573002/diff/40001/components/arc/intent_h... components/arc/intent_helper/local_activity_resolver.cc:13: return false; On 2016/06/10 19:56:09, Yusuke Sato wrote: > On 2016/06/10 19:40:27, elijahtaylor1 wrote: > > maybe the default should be true? That would allow David to use this function > > when first implementing his click hooking code when checking in and would not > > regress click latency. > > Ah true, good idea. Done.
The CQ bit was checked by zentaro@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, yusukes@chromium.org Link to the patchset: https://codereview.chromium.org/2059573002/#ps60001 (title: "Change default to true.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2059573002/60001
Message was sent while issue was closed.
Description was changed from ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns true for now. In future it will return true only if Chrome should handle this URL. Part of b\29248657 BUG=619137 ========== to ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns true for now. In future it will return true only if Chrome should handle this URL. Part of b\29248657 BUG=619137 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns true for now. In future it will return true only if Chrome should handle this URL. Part of b\29248657 BUG=619137 ========== to ========== Add initial stub implementation of LocalActivityResolver. ShouldChromeHandleUrl just returns true for now. In future it will return true only if Chrome should handle this URL. Part of b\29248657 BUG=619137 Committed: https://crrev.com/7a610a093d5bd05845a4468a73e04f0e1a7e32f8 Cr-Commit-Position: refs/heads/master@{#399299} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7a610a093d5bd05845a4468a73e04f0e1a7e32f8 Cr-Commit-Position: refs/heads/master@{#399299} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
