intents: Add conditional web intents dispatch for docs types.

intents: Add conditional web intents dispatch for docs types.

R=rdsmith@chromium.org
BUG=141244
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152107

[Randy Smith (Not in Mondays), 2012-08-16 20:54:41]

Greg, I'm afraid every time you ask to land a patch like this I'm going to ask
you about at what point you're going to have a registration system so that we
can figure out whether or not we have applications installed that handle
particular mime types.  This is somewhat ugly code.  

Is there some central location where you manage web intents that this code could
be refactored into and just called from CDMD?  It's not really download code.  

Having said that, it's consistent with what we already have, so LGTM.  (Though I
wouldn't mind the refactor, and the LGTM is conditional on removing the
LOG(INFO) below.)

https://chromiumcodereview.appspot.com/10830363/diff/1/chrome/browser/intents...
File chrome/browser/intents/web_intents_registry.cc (right):

https://chromiumcodereview.appspot.com/10830363/diff/1/chrome/browser/intents...
chrome/browser/intents/web_intents_registry.cc:332: LOG(INFO) << "Adding default
for QO";
Remove before landing.

[Greg Billock, 2012-08-16 21:06:03]

https://chromiumcodereview.appspot.com/10830363/diff/1/chrome/browser/intents...
File chrome/browser/intents/web_intents_registry.cc (right):

https://chromiumcodereview.appspot.com/10830363/diff/1/chrome/browser/intents...
chrome/browser/intents/web_intents_registry.cc:332: LOG(INFO) << "Adding default
for QO";
On 2012/08/16 20:54:41, rdsmith wrote:
> Remove before landing.

Got rid of this and other logging crap.

[groby-ooo-7-16, 2012-08-16 22:10:38]

https://chromiumcodereview.appspot.com/10830363/diff/5001/chrome/browser/down...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/10830363/diff/5001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:413: }
Can we actually unify that with the chromeos code? It's exactly the same mime
types.

[Steve McKay, 2012-08-16 22:36:17]

Driveby.

https://chromiumcodereview.appspot.com/10830363/diff/5001/chrome/browser/down...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://chromiumcodereview.appspot.com/10830363/diff/5001/chrome/browser/down...
chrome/browser/download/chrome_download_manager_delegate.cc:397: const char*
kQuickOfficeExtensionId = "gbkeegbaiigmenfmjfclcdgdpimamgkj";
The five or so lines of code dedicated to figuring out if quick office is
enabled would fit nicely in a separate method. Inline it if you want.

Maybe do the same for the mime-type check.

This this would read:

#if defined(OS_CHROMEOS)
  if (IsMsOfficeMimeType(mimetype) {
     return true;
  }
#else
  if (IsQuickOfficeInstalled() &&
      IsMsOfficeMimeType()) {
    return true;
  }
#endif

[Steve McKay, 2012-08-16 22:38:47]

On 2012/08/16 22:36:17, Steve McKay wrote:
> Driveby.
> 
>
https://chromiumcodereview.appspot.com/10830363/diff/5001/chrome/browser/down...
> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10830363/diff/5001/chrome/browser/down...
> chrome/browser/download/chrome_download_manager_delegate.cc:397: const char*
> kQuickOfficeExtensionId = "gbkeegbaiigmenfmjfclcdgdpimamgkj";
> The five or so lines of code dedicated to figuring out if quick office is
> enabled would fit nicely in a separate method. Inline it if you want.
> 
> Maybe do the same for the mime-type check.
> 
> This this would read:
> 
> #if defined(OS_CHROMEOS)
>   if (IsMsOfficeMimeType(mimetype) {
>      return true;
>   }
> #else
>   if (IsQuickOfficeInstalled() &&
>       IsMsOfficeMimeType()) {
>     return true;
>   }
> #endif

Maybe better:

if (IsMsOfficeMimeType(mimetype) {
#if defined(OS_CHROMEOS)
  return true;
#else
  if (IsQuickOfficeInstalled()) {
    return true;
  }
#endif
}

[Greg Billock, 2012-08-16 22:53:59]

I'm not sure it's worth the effort to fine tune this code. It's easy
to read as is and patches clean. One or both paras will be deleted
after the dogfood period. Since they're both purpose-built policy
changes, it's better to have them easily isolated than factored
together, even though they're very similar.

On Thu, Aug 16, 2012 at 3:38 PM,  <smckay@chromium.org> wrote:
> On 2012/08/16 22:36:17, Steve McKay wrote:
>>
>> Driveby.
>
>
>
>
https://chromiumcodereview.appspot.com/10830363/diff/5001/chrome/browser/down...
>>
>> File chrome/browser/download/chrome_download_manager_delegate.cc (right):
>
>
>
>
https://chromiumcodereview.appspot.com/10830363/diff/5001/chrome/browser/down...
>>
>> chrome/browser/download/chrome_download_manager_delegate.cc:397: const
>> char*
>> kQuickOfficeExtensionId = "gbkeegbaiigmenfmjfclcdgdpimamgkj";
>> The five or so lines of code dedicated to figuring out if quick office is
>> enabled would fit nicely in a separate method. Inline it if you want.
>
>
>> Maybe do the same for the mime-type check.
>
>
>> This this would read:
>
>
>> #if defined(OS_CHROMEOS)
>>    if (IsMsOfficeMimeType(mimetype) {
>>       return true;
>>    }
>> #else
>>    if (IsQuickOfficeInstalled() &&
>>        IsMsOfficeMimeType()) {
>>      return true;
>>    }
>> #endif
>
>
> Maybe better:
>
> if (IsMsOfficeMimeType(mimetype) {
> #if defined(OS_CHROMEOS)
>   return true;
> #else
>   if (IsQuickOfficeInstalled()) {
>
>     return true;
>   }
> #endif
> }
>
>
> https://chromiumcodereview.appspot.com/10830363/

[groby-ooo-7-16, 2012-08-17 00:47:59]

Since it's temp code, LGTM.

[commit-bot: I haz the power, 2012-08-17 14:13:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10830363/5001

[commit-bot: I haz the power, 2012-08-17 18:07:48]

Change committed as 152107