Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831147:
Show device attach web intent picker dialog only if we have registered services. (Closed)
|
Created:
8 years, 4 months ago by kmadhusu Modified:
8 years, 4 months ago CC:
chromium-reviews, gbillock+watch_chromium.org, smckay+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionShow device attach web intent picker dialog only if we have registered services.
BUG=140093
TEST=none
TBR=gbillock@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150213
Patch Set 1 #Patch Set 2 : Fix style nit #Patch Set 3 : '' #
Total comments: 25
Patch Set 4 : Addressed review comments #Patch Set 5 : '' #Patch Set 6 : Fix nits #Patch Set 7 : '' #Patch Set 8 : Rebase #Patch Set 9 : '' #
Total comments: 9
Patch Set 10 : Addressed review comments. #
Messages
Total messages: 15 (0 generated)
jhawkins@: Please review code changes. Thanks.
Added groby@ as reviewer.
http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... File chrome/browser/intents/device_attached_intent_source.cc (right): http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:108: task->GetServicesForIntent(); Please don't use a self-destructing class unless absolutely necessary. Is it possible to simply have a callback on DeviceAttachedIntentSource instead? (Also, this will save a PostTask with the associated - potential - delay) http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... File chrome/browser/intents/device_attached_intent_source.h (right): http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.h:41: const FilePath& device_path); nit: Please forward-declare FilePath - IWYU
http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... File chrome/browser/intents/device_attached_intent_source.cc (right): http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:31: // Helper class to get the intent services. nit: Expand this documentation. http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:49: ~DispatchIntentTaskHelper() { nit: {} http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:56: void DidGetServicesForIntent( Need to document that this method deletes this object. http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:59: // Weak pointers. More importantly, document what they are used for. http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:63: const string16 action_; Document member variables. http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:104: const string16 action = ASCIIToUTF16("chrome-extension://attach"); Pull this out of this method into the unnamed namespace. http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:124: CHECK(!fs_id.empty()); Don't use CHECKs unless you're debugging a crash in the wild. Use DCHECKs instead. http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:145: FROM_HERE, Optional nit: Condense parameters to save a line. http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.cc:150: delete this; What if this method is never called? Will we leak this object? http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... File chrome/browser/intents/device_attached_intent_source.h (right): http://codereview.chromium.org/10831147/diff/4001/chrome/browser/intents/devi... chrome/browser/intents/device_attached_intent_source.h:37: void DispatchIntentsForService( nit: Document method.
https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... File chrome/browser/intents/device_attached_intent_source.h (right): https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.h:41: const FilePath& device_path); On 2012/08/03 17:37:20, groby wrote: > nit: Please forward-declare FilePath - IWYU Actually we can't because we use FilePath::StringType above.
Addressed review comments. PTAL. Thanks. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... File chrome/browser/intents/device_attached_intent_source.cc (right): https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:31: // Helper class to get the intent services. On 2012/08/03 17:44:59, James Hawkins wrote: > nit: Expand this documentation. Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:49: ~DispatchIntentTaskHelper() { On 2012/08/03 17:44:59, James Hawkins wrote: > nit: {} Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:56: void DidGetServicesForIntent( On 2012/08/03 17:44:59, James Hawkins wrote: > Need to document that this method deletes this object. Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:59: // Weak pointers. On 2012/08/03 17:44:59, James Hawkins wrote: > More importantly, document what they are used for. Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:63: const string16 action_; On 2012/08/03 17:44:59, James Hawkins wrote: > Document member variables. Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:104: const string16 action = ASCIIToUTF16("chrome-extension://attach"); On 2012/08/03 17:44:59, James Hawkins wrote: > Pull this out of this method into the unnamed namespace. Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:108: task->GetServicesForIntent(); On 2012/08/03 17:37:20, groby wrote: > Please don't use a self-destructing class unless absolutely necessary. Is it > possible to simply have a callback on DeviceAttachedIntentSource instead? (Also, > this will save a PostTask with the associated - potential - delay) I need to store the device info in between calls. So I need to use a helper class to store these information. I managed to remove all the post task calls. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:124: CHECK(!fs_id.empty()); On 2012/08/03 17:44:59, James Hawkins wrote: > Don't use CHECKs unless you're debugging a crash in the wild. Use DCHECKs > instead. Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:145: FROM_HERE, On 2012/08/03 17:44:59, James Hawkins wrote: > Optional nit: Condense parameters to save a line. Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:150: delete this; On 2012/08/03 17:44:59, James Hawkins wrote: > What if this method is never called? Will we leak this object? This code will not leak. Just to be on the safer side, modified DispatchIntentTaskHelper to be refcounted. base::Bind will take care of destructing the object. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... File chrome/browser/intents/device_attached_intent_source.h (right): https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.h:37: void DispatchIntentsForService( On 2012/08/03 17:44:59, James Hawkins wrote: > nit: Document method. Done. https://chromiumcodereview.appspot.com/10831147/diff/4001/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.h:41: const FilePath& device_path); On 2012/08/03 17:37:20, groby wrote: > nit: Please forward-declare FilePath - IWYU Done.
https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... File chrome/browser/intents/device_attached_intent_source.cc (right): https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:40: const string16 action(ASCIIToUTF16("chrome-extension://attach")); Don't we currently have this defined somewhere else as well? https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:70: // Store a weak pointer to |DeviceAttachedIntentSource| object to dispatch a nit: Don't use imperative sentences to document member variables, i.e., // A weak pointer to |...| https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:124: base::Bind(&DispatchIntentTaskHelper::MayDispatchIntentForService, nit: Start of parameter lines must align on the same column. https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... File chrome/browser/intents/device_attached_intent_source.h (right): https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.h:26: public base::SupportsWeakPtr<DeviceAttachedIntentSource> { Use WeakPtrFactory instead.
jhawkins: Addressed review comments.PTAL. Thanks. https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... File chrome/browser/intents/device_attached_intent_source.cc (right): https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:40: const string16 action(ASCIIToUTF16("chrome-extension://attach")); On 2012/08/06 16:36:36, James Hawkins wrote: > Don't we currently have this defined somewhere else as well? No. https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:40: const string16 action(ASCIIToUTF16("chrome-extension://attach")); Due to the construction of "string16" global object, I was receiving an exit time destructor compile error on Mac Clang build. Therefore, I changed this declaration. Ref: http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/2881/steps... https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:70: // Store a weak pointer to |DeviceAttachedIntentSource| object to dispatch a On 2012/08/06 16:36:36, James Hawkins wrote: > nit: Don't use imperative sentences to document member variables, i.e., > > // A weak pointer to |...| Done. https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.cc:124: base::Bind(&DispatchIntentTaskHelper::MayDispatchIntentForService, On 2012/08/06 16:36:36, James Hawkins wrote: > nit: Start of parameter lines must align on the same column. Done. https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... File chrome/browser/intents/device_attached_intent_source.h (right): https://chromiumcodereview.appspot.com/10831147/diff/7006/chrome/browser/inte... chrome/browser/intents/device_attached_intent_source.h:26: public base::SupportsWeakPtr<DeviceAttachedIntentSource> { On 2012/08/06 16:36:36, James Hawkins wrote: > Use WeakPtrFactory instead. As per the comments in base/memory/weak_ptr.h, WeakPtrFactory is helpful only if you need weak pointers within the implementation of a class. SupportWeakPtr is useful in cases where you want others to be able to get a weak pointer to your class. In my case, I just want DispatchIntentTaskHelper class to have a weak pointer of DeviceAttachedIntentSource class. That is the reason I used SupportsWeakPtr. Can you explain why I need to use WeakPtrFactory?
reviewers: This is a release block bug fix. I am trying to check this in before the end of today(M22 deadline). It will be great if you can review the latest patch asap. Thanks.
LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10831147/14005
Try job failure for 10831147-14005 (previous was lost) (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10831147/14005
Change committed as 150213 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
