Interactive autofill: Adds footnote view to accept legal documents in the UI.

Interactive autofill: Adds footnote view to accept legal documents in the UI.

Adds IDS_AUTOFILL_DIALOG_LEGAL_DOC_LINKS_* and IDS_AUTOFILL_DIALOG_ACCEPT_CHANGES.

Adds to AutofillDialogController{,Impl}:
- std::vector<string16> FootnoteLinkParts(): label/link text parts (even are text, odd are links).
- LegalDocumentLinkClicked(size_t index): callback when links are clicked (|index| is of the legal document).
- string16 AcceptChangesText(): bottom line of footer, translated.

Adds to AutofillDialogView{,s}:
- UpdateFootnote(): called when the footnote should rebuild itself. 

Adds to AutofillDialogViews:
- CreateFootnoteView(): creates an empty footnote.
- RepopulateFootnoteLinks(): nukes current footnote links and re-creates them.

BUG=168704

[Dan Beam, 2013-02-08 07:17:34]

will fix description up later, too tired to think

[Evan Stade, 2013-02-08 15:38:53]

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller.h:51: virtual
std::pair<std::vector<string16>, string16>
This definitely requires docs

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller.h:114: virtual void
ShowTermsOfService() = 0;
Call these functions FooLinkClicked

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:293:
IDS_AUTOFILL_DIALOG_ACCEPT_TERMS, ConfirmButtonText());
Just make confirmbuttontext part of the translation. I don't think this approach
internationalizes well.

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:304: return
std::make_pair(parts, sentences.back());
Instead of front and back, use operator[]

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:997: profile_,
chrome::GetActiveDesktop());
You want to get the browser from the web contents (via browserfinder)

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:489:
footnote_view_->SetSize(
You shouldn't have to explicitly set the size.

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:668: footnote_view_ =
new views::View();
Can you pull this new code into a separate function

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:734: // TODO(dbeam):
make this padding column work.
Please fix this instead of leaving a todo

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:737: const int
full_width = 2;
full_width_column_set

[Dan Beam, 2013-02-08 20:53:05]

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller.h:51: virtual
std::pair<std::vector<string16>, string16>
On 2013/02/08 15:38:53, Evan Stade wrote:
> This definitely requires docs

Done.

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller.h:114: virtual void
ShowTermsOfService() = 0;
On 2013/02/08 15:38:53, Evan Stade wrote:
> Call these functions FooLinkClicked

Done.

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:293:
IDS_AUTOFILL_DIALOG_ACCEPT_TERMS, ConfirmButtonText());
On 2013/02/08 15:38:53, Evan Stade wrote:
> Just make confirmbuttontext part of the translation. I don't think this
approach
> internationalizes well.

Done.

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:304: return
std::make_pair(parts, sentences.back());
On 2013/02/08 15:38:53, Evan Stade wrote:
> Instead of front and back, use operator[]

Done. (but why?)

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:997: profile_,
chrome::GetActiveDesktop());
On 2013/02/08 15:38:53, Evan Stade wrote:
> You want to get the browser from the web contents (via browserfinder)

Done. Btw, can this API ever be triggered by a non-tabbed browser?

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:489:
footnote_view_->SetSize(
On 2013/02/08 15:38:53, Evan Stade wrote:
> You shouldn't have to explicitly set the size.

I did when I wanted to make this wrap, otherwise it'd push the dialog larger. 
Essentially this is the equivalent of max-width that I've found in views.  I'll
remove for now, but we should test with German.

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:668: footnote_view_ =
new views::View();
On 2013/02/08 15:38:53, Evan Stade wrote:
> Can you pull this new code into a separate function

Done.

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:734: // TODO(dbeam):
make this padding column work.
On 2013/02/08 15:38:53, Evan Stade wrote:
> Please fix this instead of leaving a todo

Done. (that sucked)

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:737: const int
full_width = 2;
On 2013/02/08 15:38:53, Evan Stade wrote:
> full_width_column_set

Done.

[Dan Beam, 2013-02-08 22:41:51]

hold off on this, making it work with multiple lengths of legal documents

[Dan Beam, 2013-02-09 00:23:19]

OK, this should work with a semi-arbitrary number of legal documents now...

[Dan Beam, 2013-02-09 00:27:57]

https://codereview.chromium.org/12225095/diff/7002/chrome/browser/ui/views/au...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/12225095/diff/7002/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:760:
text->SetHorizontalAlignment(gfx::ALIGN_LEFT);
not sure if this is necessary

https://codereview.chromium.org/12225095/diff/7002/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:766:
link->SetHorizontalAlignment(gfx::ALIGN_LEFT);
nor this

[Dan Beam, 2013-02-09 02:57:13]

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:614: #if
defined(OS_ANDROID)
So I guess Android is failing to compile because it doesn't depend on the ui
target in chrome_browser_ui.gyp (which is where
chrome/browser/ui/browser_finder.cc is included)?

[Evan Stade, 2013-02-11 01:05:22]

https://chromiumcodereview.appspot.com/12225095/diff/2001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/2001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:997: profile_,
chrome::GetActiveDesktop());
On 2013/02/08 20:53:06, Dan Beam wrote:
> On 2013/02/08 15:38:53, Evan Stade wrote:
> > You want to get the browser from the web contents (via browserfinder)
> 
> Done. Btw, can this API ever be triggered by a non-tabbed browser?

Potentially. It could be used in a popup browser window, extension popup (not
sure if those support constrained windows), or an app. I don't think we
explicitly disable this stuff, or want to.

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:311:
ASCIIToUTF16(documents[0]->display_name()),
The display name should be string16. That needs to be fixed in our wallet code.
In the meantime this needs to be UTF8ToUTF16.

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:314: break;
default: notreached

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:614: #if
defined(OS_ANDROID)
On 2013/02/09 02:57:13, Dan Beam wrote:
> So I guess Android is failing to compile because it doesn't depend on the ui
> target in chrome_browser_ui.gyp (which is where
> chrome/browser/ui/browser_finder.cc is included)?

Looks like unit tests and test shell aren't compiling but the rest are OK. ifdef
with TODO is fine for  until we can investigate further.

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:51: virtual
std::vector<string16> FootnoteLinkParts() const = 0;
Hyphen between even and indexed (otherwise this is a garden path sentence)

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller.h:120: virtual void
LegalDocumentLinkClicked(size_t index) = 0;
docs, what is index

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:488: if (GetWidget())
Isn't there a utility function for this yet

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:618: } else if
(source->parent()->parent() == footnote_view_) {
Too fragile

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:723: has_links ?
views::kUnrelatedControlVerticalSpacing : 0, 0, 0, 0));
Why do anything in the !has_links case? If its because space is still reserved
for the border, you should probably fix dialog client view to check if the view
is visible, not just whether it exists.

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:760:
text->SetHorizontalAlignment(gfx::ALIGN_LEFT);
Don't think you need to align.

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:791:
padding_layout->StartRowWithPadding(
AddPaddingRow

[Dan Beam, 2013-02-11 06:45:30]

The rest require code change, can do tomorrow.

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:723: has_links ?
views::kUnrelatedControlVerticalSpacing : 0, 0, 0, 0));
On 2013/02/11 01:05:23, Evan Stade wrote:
> Why do anything in the !has_links case? If its because space is still reserved
> for the border, you should probably fix dialog client view to check if the
view
> is visible, not just whether it exists.

Gets rid of existing space if this was previous triggered with links.

[ahutter, 2013-02-11 16:00:53]

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:308: case 3U:
Based in our conversations on Friday; this case isn't possible.

https://chromiumcodereview.appspot.com/12225095/diff/5004/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:311:
ASCIIToUTF16(documents[0]->display_name()),
On 2013/02/11 01:05:23, Evan Stade wrote:
> The display name should be string16. That needs to be fixed in our wallet
code.
> In the meantime this needs to be UTF8ToUTF16.

Can any chrome UI be displayed without string16?

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:291:
DCHECK_LE(documents.size(), 3U);
Should be 2U

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:295: string16
text;
Where is the privacy policy?

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:310: case 3U:
Can't happen.

[Dan Beam, 2013-02-11 16:45:17]

https://chromiumcodereview.appspot.com/12225095/diff/2001/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/2001/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:997: profile_,
chrome::GetActiveDesktop());
On 2013/02/11 01:05:23, Evan Stade wrote:
> On 2013/02/08 20:53:06, Dan Beam wrote:
> > On 2013/02/08 15:38:53, Evan Stade wrote:
> > > You want to get the browser from the web contents (via browserfinder)
> > 
> > Done. Btw, can this API ever be triggered by a non-tabbed browser?
> 
> Potentially. It could be used in a popup browser window, extension popup (not
> sure if those support constrained windows), or an app. I don't think we
> explicitly disable this stuff, or want to.

Why wouldn't we want to find or created a *tabbed* browser, then, rather than
just finding the browser with the current web contents?

[Dan Beam, 2013-02-11 18:30:05]

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:291:
DCHECK_LE(documents.size(), 3U);
On 2013/02/11 16:00:53, ahutter wrote:
> Should be 2U

Done.

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:295: string16
text;
On 2013/02/11 16:00:53, ahutter wrote:
> Where is the privacy policy?

Adding.

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/a...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:310: case 3U:
On 2013/02/11 16:00:53, ahutter wrote:
> Can't happen.

Yeah, figured this out.

[Dan Beam, 2013-02-11 19:27:50]

https://codereview.chromium.org/12225095/diff/5004/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12225095/diff/5004/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:308: case 3U:
On 2013/02/11 16:00:53, ahutter wrote:
> Based in our conversations on Friday; this case isn't possible.

Done.

https://codereview.chromium.org/12225095/diff/5004/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:311:
ASCIIToUTF16(documents[0]->display_name()),
On 2013/02/11 16:00:53, ahutter wrote:
> On 2013/02/11 01:05:23, Evan Stade wrote:
> > The display name should be string16. That needs to be fixed in our wallet
> code.
> > In the meantime this needs to be UTF8ToUTF16.
> 
> Can any chrome UI be displayed without string16?

Anything that's translated should be string16. Almost all (?) of UI the is
translated, so string16 will be the most useful.

https://codereview.chromium.org/12225095/diff/5004/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:314: break;
On 2013/02/11 01:05:23, Evan Stade wrote:
> default: notreached

size_t can't be negative, was bailing if size() > 3 or empty() (updated code
just DCHECKs() these conditions), so this NOTREACHED() seems to be dead code,
no?

https://codereview.chromium.org/12225095/diff/5004/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:614: #if
defined(OS_ANDROID)
On 2013/02/11 01:05:23, Evan Stade wrote:
> On 2013/02/09 02:57:13, Dan Beam wrote:
> > So I guess Android is failing to compile because it doesn't depend on the ui
> > target in chrome_browser_ui.gyp (which is where
> > chrome/browser/ui/browser_finder.cc is included)?
> 
> Looks like unit tests and test shell aren't compiling but the rest are OK.
ifdef
> with TODO is fine for  until we can investigate further.

Done.

https://codereview.chromium.org/12225095/diff/5008/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller.h (right):

https://codereview.chromium.org/12225095/diff/5008/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller.h:51: virtual
std::vector<string16> FootnoteLinkParts() const = 0;
On 2013/02/11 01:05:23, Evan Stade wrote:
> Hyphen between even and indexed (otherwise this is a garden path sentence)

Done.

https://codereview.chromium.org/12225095/diff/5008/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller.h:120: virtual void
LegalDocumentLinkClicked(size_t index) = 0;
On 2013/02/11 01:05:23, Evan Stade wrote:
> docs, what is index

Done.

[Dan Beam, 2013-02-11 19:45:59]

done will all review feedback, sorry for early publishes

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:488: if (GetWidget())
On 2013/02/11 01:05:23, Evan Stade wrote:
> Isn't there a utility function for this yet

Done.

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:723: has_links ?
views::kUnrelatedControlVerticalSpacing : 0, 0, 0, 0));
On 2013/02/11 06:45:30, Dan Beam wrote:
> On 2013/02/11 01:05:23, Evan Stade wrote:
> > Why do anything in the !has_links case? If its because space is still
reserved
> > for the border, you should probably fix dialog client view to check if the
> view
> > is visible, not just whether it exists.
> 
> Gets rid of existing space if this was previous triggered with links.

Fixed as per our convo (to respect view visibility in
ui/views/window/dialog_client_view.{h,cc})

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:760:
text->SetHorizontalAlignment(gfx::ALIGN_LEFT);
On 2013/02/11 01:05:23, Evan Stade wrote:
> Don't think you need to align.

Done.

https://chromiumcodereview.appspot.com/12225095/diff/5008/chrome/browser/ui/v...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:791:
padding_layout->StartRowWithPadding(
On 2013/02/11 01:05:23, Evan Stade wrote:
> AddPaddingRow

Done.

[Dan Beam, 2013-02-12 01:04:18]

+sky@ for ui/views, chrome/browser/ui/views

[Dan Beam, 2013-02-12 01:04:43]

+sky@ for realz this time (for ui/views, chrome/browser/ui/views)

[Evan Stade, 2013-02-12 01:36:53]

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12225095/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:997: profile_,
chrome::GetActiveDesktop());
On 2013/02/11 16:45:18, Dan Beam wrote:
> On 2013/02/11 01:05:23, Evan Stade wrote:
> > On 2013/02/08 20:53:06, Dan Beam wrote:
> > > On 2013/02/08 15:38:53, Evan Stade wrote:
> > > > You want to get the browser from the web contents (via browserfinder)
> > > 
> > > Done. Btw, can this API ever be triggered by a non-tabbed browser?
> > 
> > Potentially. It could be used in a popup browser window, extension popup
(not
> > sure if those support constrained windows), or an app. I don't think we
> > explicitly disable this stuff, or want to.
> 
> Why wouldn't we want to find or created a *tabbed* browser, then, rather than
> just finding the browser with the current web contents?

Well, the code you currently have might add a new tab in the wrong window, which
is not acceptable.

https://codereview.chromium.org/12225095/diff/7008/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/12225095/diff/7008/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:615:
NOTIMPLEMENTED() << " open: " << url.spec();
NOTIMPLEMENTED is annoying, you should not use it.

[Dan Beam, 2013-02-12 03:53:50]

hey sky@, isherman@, estade@, ahutter@ - hold off on this until I figure out how
to do inline text wrapping in views (essentially)

[Dan Beam, 2013-02-14 02:55:10]

OK, this emulates text wrapping well enough for now -- PTAL (be gentle).

[Dan Beam, 2013-02-14 22:06:01]

sky@/isherman@ ping (if you have time when not sheriffing, isherman@)

[sky, 2013-02-14 22:47:10]

https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:276: class FootnoteView
: public views::View {
If I understand this correctly this class is effectively used for laying out
text and is only needed because label doesn't support styling. We should add the
support for styling, including a link in there, to label.

https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
File ui/views/window/dialog_client_view.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
ui/views/window/dialog_client_view.cc:505: if (footnote_view_)
I think you can remove the if.

[Dan Beam, 2013-02-15 01:48:31]

https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:276: class FootnoteView
: public views::View {
On 2013/02/14 22:47:10, sky wrote:
> If I understand this correctly this class is effectively used for laying out
> text and is only needed because label doesn't support styling. We should add
the
> support for styling, including a link in there, to label.

hey Scott, originally estade@ and I discussed this and I didn't go down the
route to style labels as it'd also require making parts of labels have link-like
event handling, focus management, and listeners (which seemed like more work
than slicing up existing UI elements). Just to clarify, you want me to add all
these things to make labels and links interlace (not just styling), right?

https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
File ui/views/window/dialog_client_view.cc (right):

https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
ui/views/window/dialog_client_view.cc:505: if (footnote_view_)
On 2013/02/14 22:47:10, sky wrote:
> I think you can remove the if.

Done.

[sky, 2013-02-19 21:59:58]

Styled text should be a feature of the toolkit and be provided in
views. It need not be baked into label, I'm fine with say a
StyledLabel class. I'm a bit busy, but might be able to find time for
doing this this week if necessary.

  -Scott

On Thu, Feb 14, 2013 at 5:48 PM,  <dbeam@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
> File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):
>
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
> chrome/browser/ui/views/autofill/autofill_dialog_views.h:276: class
> FootnoteView : public views::View {
> On 2013/02/14 22:47:10, sky wrote:
>>
>> If I understand this correctly this class is effectively used for
>
> laying out
>>
>> text and is only needed because label doesn't support styling. We
>
> should add the
>>
>> support for styling, including a link in there, to label.
>
>
> hey Scott, originally estade@ and I discussed this and I didn't go down
> the route to style labels as it'd also require making parts of labels
> have link-like event handling, focus management, and listeners (which
> seemed like more work than slicing up existing UI elements). Just to
> clarify, you want me to add all these things to make labels and links
> interlace (not just styling), right?
>
>
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
> File ui/views/window/dialog_client_view.cc (right):
>
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
> ui/views/window/dialog_client_view.cc:505: if (footnote_view_)
> On 2013/02/14 22:47:10, sky wrote:
>>
>> I think you can remove the if.
>
>
> Done.
>
> https://chromiumcodereview.appspot.com/12225095/

[Dan Beam, 2013-02-20 23:19:31]

On 2013/02/19 21:59:58, sky wrote:
> Styled text should be a feature of the toolkit and be provided in
> views. It need not be baked into label, I'm fine with say a
> StyledLabel class. I'm a bit busy, but might be able to find time for
> doing this this week if necessary.

Any help would be appreciated.

> 
>   -Scott
> 
> On Thu, Feb 14, 2013 at 5:48 PM,  <mailto:dbeam@chromium.org> wrote:
> >
> >
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
> > File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):
> >
> >
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
> > chrome/browser/ui/views/autofill/autofill_dialog_views.h:276: class
> > FootnoteView : public views::View {
> > On 2013/02/14 22:47:10, sky wrote:
> >>
> >> If I understand this correctly this class is effectively used for
> >
> > laying out
> >>
> >> text and is only needed because label doesn't support styling. We
> >
> > should add the
> >>
> >> support for styling, including a link in there, to label.
> >
> >
> > hey Scott, originally estade@ and I discussed this and I didn't go down
> > the route to style labels as it'd also require making parts of labels
> > have link-like event handling, focus management, and listeners (which
> > seemed like more work than slicing up existing UI elements). Just to
> > clarify, you want me to add all these things to make labels and links
> > interlace (not just styling), right?
> >
> >
> >
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
> > File ui/views/window/dialog_client_view.cc (right):
> >
> >
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
> > ui/views/window/dialog_client_view.cc:505: if (footnote_view_)
> > On 2013/02/14 22:47:10, sky wrote:
> >>
> >> I think you can remove the if.
> >
> >
> > Done.
> >
> > https://chromiumcodereview.appspot.com/12225095/

[Dan Beam, 2013-03-02 03:02:14]

On 2013/02/20 23:19:31, Dan Beam wrote:
> On 2013/02/19 21:59:58, sky wrote:
> > Styled text should be a feature of the toolkit and be provided in
> > views. It need not be baked into label, I'm fine with say a
> > StyledLabel class. I'm a bit busy, but might be able to find time for
> > doing this this week if necessary.
> 
> Any help would be appreciated.

Hey Scott,

Have you gotten a chance to look at this / do you think you will?  If you're
much too busy, I or somebody else working on Feature=Autofill-Interactive can
rewrite this patch.

Thanks for your help!

> 
> > 
> >   -Scott
> > 
> > On Thu, Feb 14, 2013 at 5:48 PM,  <mailto:dbeam@chromium.org> wrote:
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
> > > File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
> > > chrome/browser/ui/views/autofill/autofill_dialog_views.h:276: class
> > > FootnoteView : public views::View {
> > > On 2013/02/14 22:47:10, sky wrote:
> > >>
> > >> If I understand this correctly this class is effectively used for
> > >
> > > laying out
> > >>
> > >> text and is only needed because label doesn't support styling. We
> > >
> > > should add the
> > >>
> > >> support for styling, including a link in there, to label.
> > >
> > >
> > > hey Scott, originally estade@ and I discussed this and I didn't go down
> > > the route to style labels as it'd also require making parts of labels
> > > have link-like event handling, focus management, and listeners (which
> > > seemed like more work than slicing up existing UI elements). Just to
> > > clarify, you want me to add all these things to make labels and links
> > > interlace (not just styling), right?
> > >
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
> > > File ui/views/window/dialog_client_view.cc (right):
> > >
> > >
> >
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
> > > ui/views/window/dialog_client_view.cc:505: if (footnote_view_)
> > > On 2013/02/14 22:47:10, sky wrote:
> > >>
> > >> I think you can remove the if.
> > >
> > >
> > > Done.
> > >
> > > https://chromiumcodereview.appspot.com/12225095/

[sky, 2013-03-04 21:42:28]

Sorry, I have had no time for this nor do I think I'll have any time
this week. Sorry.

  -Scott

On Fri, Mar 1, 2013 at 7:02 PM,  <dbeam@chromium.org> wrote:
> On 2013/02/20 23:19:31, Dan Beam wrote:
>>
>> On 2013/02/19 21:59:58, sky wrote:
>> > Styled text should be a feature of the toolkit and be provided in
>> > views. It need not be baked into label, I'm fine with say a
>> > StyledLabel class. I'm a bit busy, but might be able to find time for
>> > doing this this week if necessary.
>
>
>> Any help would be appreciated.
>
>
> Hey Scott,
>
> Have you gotten a chance to look at this / do you think you will?  If you're
> much too busy, I or somebody else working on Feature=Autofill-Interactive
> can
> rewrite this patch.
>
> Thanks for your help!
>
>
>> >
>> >   -Scott
>> >
>> > On Thu, Feb 14, 2013 at 5:48 PM,  <mailto:dbeam@chromium.org> wrote:
>> > >
>> > >
>> >
>
>
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
>>
>> > > File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):
>> > >
>> > >
>> >
>
>
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/chrome/browser/ui/...
>>
>> > > chrome/browser/ui/views/autofill/autofill_dialog_views.h:276: class
>> > > FootnoteView : public views::View {
>> > > On 2013/02/14 22:47:10, sky wrote:
>> > >>
>> > >> If I understand this correctly this class is effectively used for
>> > >
>> > > laying out
>> > >>
>> > >> text and is only needed because label doesn't support styling. We
>> > >
>> > > should add the
>> > >>
>> > >> support for styling, including a link in there, to label.
>> > >
>> > >
>> > > hey Scott, originally estade@ and I discussed this and I didn't go
>> > > down
>> > > the route to style labels as it'd also require making parts of labels
>> > > have link-like event handling, focus management, and listeners (which
>> > > seemed like more work than slicing up existing UI elements). Just to
>> > > clarify, you want me to add all these things to make labels and links
>> > > interlace (not just styling), right?
>> > >
>> > >
>> > >
>> >
>
>
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
>>
>> > > File ui/views/window/dialog_client_view.cc (right):
>> > >
>> > >
>> >
>
>
>
https://chromiumcodereview.appspot.com/12225095/diff/24012/ui/views/window/di...
>>
>> > > ui/views/window/dialog_client_view.cc:505: if (footnote_view_)
>> > > On 2013/02/14 22:47:10, sky wrote:
>> > >>
>> > >> I think you can remove the if.
>> > >
>> > >
>> > > Done.
>> > >
>> > > https://chromiumcodereview.appspot.com/12225095/
>
>
>
> https://codereview.chromium.org/12225095/

[Evan Stade, 2013-03-11 21:25:04]

I will work on the rich label formatting.