Web Intents: Preparatory work.

Web Intents: Preparatory work.
* Added --enable-web-intents switch.
* Added enable_web_intents build variable, for use in WebKit mostly.
* Added registration InfoBar and piping to the WebKit side, which is not hooked
  up on that side yet.

BUG=none
TEST=RegisterIntentHandlerInfoBarDelegateTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=94867

[James Hawkins, 2011-07-26 21:20:38]

+darin

Feel free to let me know if I should find a different reviewer.

[James Hawkins, 2011-07-26 21:32:15]

+sky for infobar

[sky, 2011-07-26 21:45:12]

I'm trading myself for Peter. Peter is Mr Infobars (see his status for the last
n months or so).

  -Scott

[Peter Kasting, 2011-07-26 22:08:32]

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right):

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:20:
RegisterIntentHandlerInfoBarDelegate::GetInfoBarType() const {
Nit: Indent 4

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:30: return
BUTTON_OK | BUTTON_CANCEL | BUTTON_NONE;
This doesn't work.  BUTTON_NONE is zero, not a separate button.  You cannot
create three-button infobars by default.

Have you gone over what you want with a UI designer?  I'm already uncomfortable
with how many infobars we have, and adding a new one with three buttons raises
my red flags.  Perhaps there's a better UI for this.  For example, at least
trimming to 2 buttons by making "No" mean "Never" and removing the "Never"
button.  Maybe the UI folks could also help you find a clearer way of expressing
the question than "handle intents", which is a phrase that doesn't mean anything
to me, let alone the rest of our users.

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc:11:
TEST(RegisterIntentHandlerInfoBarDelegateTest, GetInfoBarType) {
None of these tests is useful.  Just kill this file.

[James Hawkins, 2011-07-26 22:35:39]

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right):

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:20:
RegisterIntentHandlerInfoBarDelegate::GetInfoBarType() const {
On 2011/07/26 22:08:32, Peter Kasting wrote:
> Nit: Indent 4

Done. Perhaps this should be added to the style guide, as it's not included and
seems strange at first.

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:30: return
BUTTON_OK | BUTTON_CANCEL | BUTTON_NONE;
On 2011/07/26 22:08:32, Peter Kasting wrote:
> This doesn't work.  BUTTON_NONE is zero, not a separate button.  You cannot
> create three-button infobars by default.
> 
> Have you gone over what you want with a UI designer?  I'm already
uncomfortable
> with how many infobars we have, and adding a new one with three buttons raises
> my red flags.  Perhaps there's a better UI for this.  For example, at least
> trimming to 2 buttons by making "No" mean "Never" and removing the "Never"
> button.  Maybe the UI folks could also help you find a clearer way of
expressing
> the question than "handle intents", which is a phrase that doesn't mean
anything
> to me, let alone the rest of our users.

There's an open question in the design doc about using 'No' as 'Never', which is
probably the best solution here.  There's also an open question about the
string, which is crappy for now, but can be fixed after commit. Rapid
prototyping!

To answer your question about using an infobar at all, I spoke with a few UI
leads earlier, and this ties into the brainstorming happening now surrounding
better permissions UI, which doesn't have a solution yet. Using an infobar
matches registerProtocolHandler and geolocation requests, which are both similar
features to Web Intents wrt permissions.

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc:11:
TEST(RegisterIntentHandlerInfoBarDelegateTest, GetInfoBarType) {
On 2011/07/26 22:08:32, Peter Kasting wrote:
> None of these tests is useful.  Just kill this file.

The tests under the TODOs are important (which they'll be possible once
http://codereview.chromium.org/7461089/ lands), and the other tests are not
necessarily bad.  Are you insisting I remove them?  I'd really prefer to keep
them.

[Peter Kasting, 2011-07-26 22:52:12]

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc
(right):

http://codereview.chromium.org/7461093/diff/4001/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate_unittest.cc:11:
TEST(RegisterIntentHandlerInfoBarDelegateTest, GetInfoBarType) {
On 2011/07/26 22:35:39, James Hawkins wrote:
> On 2011/07/26 22:08:32, Peter Kasting wrote:
> > None of these tests is useful.  Just kill this file.
> 
> The tests under the TODOs are important (which they'll be possible once
> http://codereview.chromium.org/7461089/ lands), and the other tests are not
> necessarily bad.  Are you insisting I remove them?  I'd really prefer to keep
> them.

Tests that verify at such a low level -- e.g. "yes, my infobar contains two
buttons" -- aren't ever going to catch real mistakes, they just end up making
refactoring more frustrating.  I've already had to kill some Mac tests that ran
at this level because they made all the reworking I've been doing hellish for no
benefit.

Testing that your strings are the strings you put in the string table is
similarly pointless.  I can't speak as much for what you've TODOd because I
don't know precisely what you're planning on testing, but at least what you have
here right now is all stuff that I would prefer to see gone because I think it's
cost with no benefit.

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right):

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:30: return
BUTTON_OK | BUTTON_CANCEL;
Nit: You can eliminate this function entirely -- it duplicates the default
ConfirmInfoBarDelegate behavior.

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:40:
string16());
This extra arg shouldn't be here.

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:41: case
BUTTON_NONE:
Getting BUTTON_NONE here is actually illegal.

Nit: I suggest structuring this like most other delegates do, for simplicity and
consistency:

  return l10n_util::GetStringUTF16((button == BUTTON_OK) ?
      IDS_REGISTER_INTENT_HANDLER_ACCEPT : IDS_REGISTER_INTENT_HANDLER_DENY);

I realize this assumes only two button enum values, but we make that assumption
in such a large number of places and it's unlikely enough to change that I'm OK
with that.

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:57: //
TODO(jhawkins): Add associated bug for the article here.
Nit: What's this mean?

[James Hawkins, 2011-07-26 23:04:55]

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right):

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:30: return
BUTTON_OK | BUTTON_CANCEL;
On 2011/07/26 22:52:12, Peter Kasting wrote:
> Nit: You can eliminate this function entirely -- it duplicates the default
> ConfirmInfoBarDelegate behavior.

Done.

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:40:
string16());
On 2011/07/26 22:52:12, Peter Kasting wrote:
> This extra arg shouldn't be here.

Done.

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:41: case
BUTTON_NONE:
On 2011/07/26 22:52:12, Peter Kasting wrote:
> Getting BUTTON_NONE here is actually illegal.
> 
> Nit: I suggest structuring this like most other delegates do, for simplicity
and
> consistency:
> 
>   return l10n_util::GetStringUTF16((button == BUTTON_OK) ?
>       IDS_REGISTER_INTENT_HANDLER_ACCEPT : IDS_REGISTER_INTENT_HANDLER_DENY);
> 
> I realize this assumes only two button enum values, but we make that
assumption
> in such a large number of places and it's unlikely enough to change that I'm
OK
> with that.

Removed NONE handling by switching to an if/else. The ACCEPT string takes to
string16() params, so I haven't pulled it as tightly together as you're
suggesting.  I could do something like:

return (button == BUTTON_OK) ? GetStringFUTF16(ACCEPT, ...) :
GetStringUTF16(DENY);

but I feel this is more readable.

http://codereview.chromium.org/7461093/diff/5002/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:57: //
TODO(jhawkins): Add associated bug for the article here.
On 2011/07/26 22:52:12, Peter Kasting wrote:
> Nit: What's this mean?

I need to file a bug to get the help center article written, then add that bug
to the TODO above this one.

[James Hawkins, 2011-07-26 23:06:21]

Forgot to mention that I removed the tests; I'll add the tests I had TODOs for
in a later CL.

[Peter Kasting, 2011-07-26 23:10:52]

LGTM on infobars

http://codereview.chromium.org/7461093/diff/2003/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right):

http://codereview.chromium.org/7461093/diff/2003/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:33:
string16(), string16());
Only one string16(), I believe?

http://codereview.chromium.org/7461093/diff/2003/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:34: } else if
(button == BUTTON_CANCEL) {
Nit: No else after return.

Doing this with a conditional like you have it is OK, just DCHECK the button
type instead of "handling" failure, i.e.:

  if (button == BUTTON_OK)
    return ...;
  DCHECK(button == BUTTON_CANCEL);
  return ...;

[James Hawkins, 2011-07-26 23:18:25]

http://codereview.chromium.org/7461093/diff/2003/chrome/browser/intents/regis...
File chrome/browser/intents/register_intent_handler_infobar_delegate.cc (right):

http://codereview.chromium.org/7461093/diff/2003/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:33:
string16(), string16());
On 2011/07/26 23:10:52, Peter Kasting wrote:
> Only one string16(), I believe?

Indeed, I was looking at the wrong string.  Done.

http://codereview.chromium.org/7461093/diff/2003/chrome/browser/intents/regis...
chrome/browser/intents/register_intent_handler_infobar_delegate.cc:34: } else if
(button == BUTTON_CANCEL) {
On 2011/07/26 23:10:52, Peter Kasting wrote:
> Nit: No else after return.
> 
> Doing this with a conditional like you have it is OK, just DCHECK the button
> type instead of "handling" failure, i.e.:
> 
>   if (button == BUTTON_OK)
>     return ...;
>   DCHECK(button == BUTTON_CANCEL);
>   return ...;

Done.

[darin (slow to review), 2011-07-29 22:39:03]

LGTM