Issue 270081: Facelifts to sync UI

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+596 lines, -214 lines)			Patch
M	chrome/app/generated_resources.grd	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	4 chunks	+27 lines, -12 lines	0 comments	Download
M	chrome/app/resources/locale_settings.grd	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+11 lines, -3 lines	0 comments	Download
M	chrome/app/resources/locale_settings_am.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_ar.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_bg.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_bn.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_ca.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_cs.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_da.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_de.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_el.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_en-GB.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_es.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_es-419.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_et.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_fi.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_fil.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_fr.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_gu.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_he.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_hi.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_hr.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_hu.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_id.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_it.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_ja.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_kn.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_ko.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_lt.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_lv.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_ml.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_mr.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_nb.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_nl.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_or.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_pl.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_pt-BR.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_pt-PT.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_ro.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_ru.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_sk.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_sl.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_sr.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_sv.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_sw.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_ta.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_te.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_th.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_tr.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_uk.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_vi.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_zh-CN.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/app/resources/locale_settings_zh-TW.xtb	View	12 13 14 15	1 chunk	+4 lines, -2 lines	0 comments	Download
M	chrome/browser/browser_resources.grd	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/dom_ui/new_tab_page_sync_handler.h	View	8 9 10 11 12 13 14 15	2 chunks	+10 lines, -1 line	0 comments	Download
M	chrome/browser/dom_ui/new_tab_page_sync_handler.cc	View	8 9 10 11 12 13 14 15	6 chunks	+30 lines, -26 lines	0 comments	Download
M	chrome/browser/resources/new_new_tab.js	View	8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/sync/profile_sync_service.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	3 chunks	+14 lines, -10 lines	0 comments	Download
M	chrome/browser/sync/resources/gaia_login.html	View	6 7 8 9 10 11 12 13 14 15	4 chunks	+9 lines, -4 lines	0 comments	Download
M	chrome/browser/sync/resources/merge_and_sync.html	View	6 7 8 9 10 11 12 13 14 15	4 chunks	+35 lines, -22 lines	0 comments	Download
A	chrome/browser/sync/resources/setup_done.html	View	2 3 4 5 6 7 8 9 10	1 chunk	+59 lines, -0 lines	0 comments	Download
M	chrome/browser/sync/resources/setup_flow.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+7 lines, -0 lines	0 comments	Download
M	chrome/browser/views/confirm_message_box_dialog.h	View	11 12 13 14 15	3 chunks	+28 lines, -7 lines	0 comments	Download
M	chrome/browser/views/confirm_message_box_dialog.cc	View	11 12 13 14 15	3 chunks	+31 lines, -6 lines	0 comments	Download
M	chrome/browser/views/options/content_page_view.h	View	3 4 5 6 7 8 9 10 11 12 13 14 15	3 chunks	+6 lines, -1 line	0 comments	Download
M	chrome/browser/views/options/content_page_view.cc	View	3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+17 lines, -2 lines	0 comments	Download
M	chrome/browser/views/sync/sync_setup_flow.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+2 lines, -1 line	0 comments	Download
M	chrome/browser/views/sync/sync_setup_flow.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	6 chunks	+26 lines, -7 lines	0 comments	Download
M	chrome/browser/views/sync/sync_setup_wizard.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	2 chunks	+16 lines, -0 lines	0 comments	Download
M	chrome/browser/views/sync/sync_setup_wizard.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	5 chunks	+26 lines, -5 lines	0 comments	Download
M	chrome/browser/views/sync/sync_setup_wizard_unittest.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	3 chunks	+20 lines, -2 lines	0 comments	Download
M	chrome/browser/views/toolbar_view.cc	View	9 10 11 12 13 14 15	2 chunks	+14 lines, -3 lines	0 comments	Download
M	chrome/common/url_constants.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/common/url_constants.cc	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 33 (0 generated)

Expand Messages | Collapse Messages

ncarter (slow)

The code looks fine; I have only nit comments, but I'm holding off until I ...

11 years, 2 months ago (2009-10-14 01:07:32 UTC) #2

tim (not reviewing)

I'll get you snapshots, I encountered a bug in the options view where the added ...

11 years, 2 months ago (2009-10-14 06:04:19 UTC) #3

tim (not reviewing)

fixed your points. still trying to get screenshots, the html in the final wizard dialog ...

11 years, 2 months ago (2009-10-14 08:05:57 UTC) #4

fixed your points.

still trying to get screenshots, the html in the final wizard dialog is still
not right, the button falls off the screen with one version of the text but not
the other.  I will keep twiddling... I wish I was an Oscar Mayer CSS wizard.

http://codereview.chromium.org/270081/diff/26/27
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/270081/diff/26/27#newcode5313
Line 5313: 
On 2009/10/14 01:07:32, nick wrote:
> Extra newline.

Done.

http://codereview.chromium.org/270081/diff/26/27#newcode5331
Line 5331: Your bookmarks are now synced to Google Docs! To merge and sync your
bookmarks to Chrome on
On 2009/10/14 01:07:32, nick wrote:
> Based on the examples I see elsewhere in this file, I think you should avoid
> having the newline in the middle of this string.

Done.

http://codereview.chromium.org/270081/diff/26/33
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/270081/diff/26/33#newcode344
Line 344: LOG(ERROR) << "Model assocation failed.";
On 2009/10/14 01:07:32, nick wrote:
> Indentation is off.

Done.

http://codereview.chromium.org/270081/diff/26/33#newcode345
Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
On 2009/10/14 01:07:32, nick wrote:
> Unnecessary since OnUnrecoverableError does it too.

Ah! Done.  I just wanted to remove the TODO, must have forgotten to do that when
adding that to OnUnrecoverableError.

http://codereview.chromium.org/270081/diff/26/33#newcode396
Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
On 2009/10/14 01:07:32, nick wrote:
> As above, this line is redundant.

Done.

http://codereview.chromium.org/270081/diff/26/40
File chrome/browser/views/options/content_page_view.cc (right):

http://codereview.chromium.org/270081/diff/26/40#newcode473
Line 473: stop_sync_description_label_->SetVisible(false);
On 2009/10/14 01:07:32, nick wrote:
> Consider renaming description_label so that it's consistent with the IDS tag
> name (...EXPLANANTION_LABEL), or vice versa.

oops, meant to do that. thanks.

tim (not reviewing)

Hmm. Sizes of the buttons still look off :( Sorry for thrashing on this. On ...

11 years, 2 months ago (2009-10-14 09:01:21 UTC) #5

Hmm. Sizes of the buttons still look off :(  Sorry for thrashing on this.

On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org> wrote:

> Screenshots attached forevermore.  I made the width of the buttons in all
> our dialogs consistent with the min width in native ui.
> However, trying to upload merge_and_sync.html is now throwing an error,
> saying "Got error status from ['svn', 'cat',
> 'chrome/browser/sync/resources/merge_and_sync.html']".
> I'm not sure why.  Looking into it.
>
> As for my changes to locale_resources.grd... I'm not sure what I'm supposed
> to do about all the translations.
>
>
> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>
>> fixed your points.
>>
>> still trying to get screenshots, the html in the final wizard dialog is
>> still
>> not right, the button falls off the screen with one version of the text
>> but not
>> the other.  I will keep twiddling... I wish I was an Oscar Mayer CSS
>> wizard.
>>
>>
>> http://codereview.chromium.org/270081/diff/26/27
>> File chrome/app/generated_resources.grd (right):
>>
>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>> Line 5313:
>> On 2009/10/14 01:07:32, nick wrote:
>>
>>> Extra newline.
>>>
>>
>> Done.
>>
>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>> Line 5331: Your bookmarks are now synced to Google Docs! To merge and
>> sync your bookmarks to Chrome on
>> On 2009/10/14 01:07:32, nick wrote:
>>
>>> Based on the examples I see elsewhere in this file, I think you should
>>>
>> avoid
>>
>>> having the newline in the middle of this string.
>>>
>>
>> Done.
>>
>> http://codereview.chromium.org/270081/diff/26/33
>> File chrome/browser/sync/profile_sync_service.cc (right):
>>
>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>> Line 344: LOG(ERROR) << "Model assocation failed.";
>> On 2009/10/14 01:07:32, nick wrote:
>>
>>> Indentation is off.
>>>
>>
>> Done.
>>
>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> On 2009/10/14 01:07:32, nick wrote:
>>
>>> Unnecessary since OnUnrecoverableError does it too.
>>>
>>
>> Ah! Done.  I just wanted to remove the TODO, must have forgotten to do
>> that when adding that to OnUnrecoverableError.
>>
>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> On 2009/10/14 01:07:32, nick wrote:
>>
>>> As above, this line is redundant.
>>>
>>
>> Done.
>>
>> http://codereview.chromium.org/270081/diff/26/40
>> File chrome/browser/views/options/content_page_view.cc (right):
>>
>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>> Line 473: stop_sync_description_label_->SetVisible(false);
>> On 2009/10/14 01:07:32, nick wrote:
>>
>>> Consider renaming description_label so that it's consistent with the
>>>
>> IDS tag
>>
>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>
>>
>> oops, meant to do that. thanks.
>>
>>
>> http://codereview.chromium.org/270081
>>
>
>

tim (not reviewing)

+cc the real nickbaum On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org> ...

11 years, 2 months ago (2009-10-14 17:56:59 UTC) #6

+cc the real nickbaum
On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org> wrote:

> +cc nickbaum
> I had to up the size of the options dialog (in pixels, in english, it looks
> like the jump went from 610 to 650).
> I attached a screenshot of what this means for the 'Basics' pane.  Looking
> for an OK.
> Fixed button sizes.
>
>
> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org> wrote:
>
>> Hmm. Sizes of the buttons still look off :(  Sorry for thrashing on this.
>>
>>
>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org> wrote:
>>
>>> Screenshots attached forevermore.  I made the width of the buttons in all
>>> our dialogs consistent with the min width in native ui.
>>> However, trying to upload merge_and_sync.html is now throwing an error,
>>> saying "Got error status from ['svn', 'cat',
>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>> I'm not sure why.  Looking into it.
>>>
>>> As for my changes to locale_resources.grd... I'm not sure what I'm
>>> supposed to do about all the translations.
>>>
>>>
>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>>>
>>>> fixed your points.
>>>>
>>>> still trying to get screenshots, the html in the final wizard dialog is
>>>> still
>>>> not right, the button falls off the screen with one version of the text
>>>> but not
>>>> the other.  I will keep twiddling... I wish I was an Oscar Mayer CSS
>>>> wizard.
>>>>
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/27
>>>> File chrome/app/generated_resources.grd (right):
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>>>> Line 5313:
>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>
>>>>> Extra newline.
>>>>>
>>>>
>>>> Done.
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>>>> Line 5331: Your bookmarks are now synced to Google Docs! To merge and
>>>> sync your bookmarks to Chrome on
>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>
>>>>> Based on the examples I see elsewhere in this file, I think you should
>>>>>
>>>> avoid
>>>>
>>>>> having the newline in the middle of this string.
>>>>>
>>>>
>>>> Done.
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/33
>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>
>>>>> Indentation is off.
>>>>>
>>>>
>>>> Done.
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>
>>>>> Unnecessary since OnUnrecoverableError does it too.
>>>>>
>>>>
>>>> Ah! Done.  I just wanted to remove the TODO, must have forgotten to do
>>>> that when adding that to OnUnrecoverableError.
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>
>>>>> As above, this line is redundant.
>>>>>
>>>>
>>>> Done.
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/40
>>>> File chrome/browser/views/options/content_page_view.cc (right):
>>>>
>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>>>> Line 473: stop_sync_description_label_->SetVisible(false);
>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>
>>>>> Consider renaming description_label so that it's consistent with the
>>>>>
>>>> IDS tag
>>>>
>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>>>
>>>>
>>>> oops, meant to do that. thanks.
>>>>
>>>>
>>>> http://codereview.chromium.org/270081
>>>>
>>>
>>>
>>
>

Nick Baum (Google)

+UI team I really think it's weird to have all this extra spacing in the ...

11 years, 2 months ago (2009-10-14 21:39:26 UTC) #7

+UI team
I really think it's weird to have all this extra spacing in the dialogs and
the pref pane. Can't we resize these dynamically to adapt to the content?

-Nick

On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org> wrote:

> +cc the real nickbaum
>
> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org> wrote:
>
>> +cc nickbaum
>> I had to up the size of the options dialog (in pixels, in english, it
>> looks like the jump went from 610 to 650).
>> I attached a screenshot of what this means for the 'Basics' pane.  Looking
>> for an OK.
>> Fixed button sizes.
>>
>>
>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org> wrote:
>>
>>> Hmm. Sizes of the buttons still look off :(  Sorry for thrashing on this.
>>>
>>>
>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org> wrote:
>>>
>>>> Screenshots attached forevermore.  I made the width of the buttons in
>>>> all our dialogs consistent with the min width in native ui.
>>>> However, trying to upload merge_and_sync.html is now throwing an error,
>>>> saying "Got error status from ['svn', 'cat',
>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>>> I'm not sure why.  Looking into it.
>>>>
>>>> As for my changes to locale_resources.grd... I'm not sure what I'm
>>>> supposed to do about all the translations.
>>>>
>>>>
>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>>>>
>>>>> fixed your points.
>>>>>
>>>>> still trying to get screenshots, the html in the final wizard dialog is
>>>>> still
>>>>> not right, the button falls off the screen with one version of the text
>>>>> but not
>>>>> the other.  I will keep twiddling... I wish I was an Oscar Mayer CSS
>>>>> wizard.
>>>>>
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/27
>>>>> File chrome/app/generated_resources.grd (right):
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>>>>> Line 5313:
>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>
>>>>>> Extra newline.
>>>>>>
>>>>>
>>>>> Done.
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To merge and
>>>>> sync your bookmarks to Chrome on
>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>
>>>>>> Based on the examples I see elsewhere in this file, I think you should
>>>>>>
>>>>> avoid
>>>>>
>>>>>> having the newline in the middle of this string.
>>>>>>
>>>>>
>>>>> Done.
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/33
>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>
>>>>>> Indentation is off.
>>>>>>
>>>>>
>>>>> Done.
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>
>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>>>>>
>>>>>
>>>>> Ah! Done.  I just wanted to remove the TODO, must have forgotten to do
>>>>> that when adding that to OnUnrecoverableError.
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>
>>>>>> As above, this line is redundant.
>>>>>>
>>>>>
>>>>> Done.
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/40
>>>>> File chrome/browser/views/options/content_page_view.cc (right):
>>>>>
>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>
>>>>>> Consider renaming description_label so that it's consistent with the
>>>>>>
>>>>> IDS tag
>>>>>
>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>>>>
>>>>>
>>>>> oops, meant to do that. thanks.
>>>>>
>>>>>
>>>>> http://codereview.chromium.org/270081
>>>>>
>>>>
>>>>
>>>
>>
>

tim (not reviewing)

On Wed, Oct 14, 2009 at 2:37 PM, Glen Murphy <glen@google.com> wrote: > If we ...

11 years, 2 months ago (2009-10-14 21:44:24 UTC) #8

On Wed, Oct 14, 2009 at 2:37 PM, Glen Murphy <glen@google.com> wrote:

> If we can't, a stopgap solution would be to top-align the text and
> bottom-align the buttons.
>
I did this first (on the dialog), and I found it uglier.  Not sure what this
means in context of the options pane?
I will tweak the html dialog and send out screenshots again.


>
> I wonder if we could use the right half (the 'merged' image) of the
> merge icon here to fill out some space, too.
>

I could try that.  A fresh coat of gradient background paint may help
appearance.

>
>
> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org> wrote:
> > +UI team
> > I really think it's weird to have all this extra spacing in the dialogs
> and
> > the pref pane. Can't we resize these dynamically to adapt to the content?
> > -Nick
> >
> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org> wrote:
> >>
> >> +cc the real nickbaum
> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org> wrote:
> >>>
> >>> +cc nickbaum
> >>> I had to up the size of the options dialog (in pixels, in english, it
> >>> looks like the jump went from 610 to 650).
> >>> I attached a screenshot of what this means for the 'Basics' pane.
> >>>  Looking for an OK.
> >>> Fixed button sizes.
> >>>
> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org> wrote:
> >>>>
> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for thrashing on
> >>>> this.
> >>>>
> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org> wrote:
> >>>>>
> >>>>> Screenshots attached forevermore.  I made the width of the buttons in
> >>>>> all our dialogs consistent with the min width in native ui.
> >>>>> However, trying to upload merge_and_sync.html is now throwing an
> error,
> >>>>> saying "Got error status from ['svn', 'cat',
> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
> >>>>> I'm not sure why.  Looking into it.
> >>>>> As for my changes to locale_resources.grd... I'm not sure what I'm
> >>>>> supposed to do about all the translations.
> >>>>>
> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
> >>>>>>
> >>>>>> fixed your points.
> >>>>>>
> >>>>>> still trying to get screenshots, the html in the final wizard dialog
> >>>>>> is still
> >>>>>> not right, the button falls off the screen with one version of the
> >>>>>> text but not
> >>>>>> the other.  I will keep twiddling... I wish I was an Oscar Mayer CSS
> >>>>>> wizard.
> >>>>>>
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/27
> >>>>>> File chrome/app/generated_resources.grd (right):
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
> >>>>>> Line 5313:
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Extra newline.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To merge
> and
> >>>>>> sync your bookmarks to Chrome on
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Based on the examples I see elsewhere in this file, I think you
> >>>>>>> should
> >>>>>>
> >>>>>> avoid
> >>>>>>>
> >>>>>>> having the newline in the middle of this string.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/33
> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Indentation is off.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
> >>>>>>
> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have forgotten to
> do
> >>>>>> that when adding that to OnUnrecoverableError.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> As above, this line is redundant.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/40
> >>>>>> File chrome/browser/views/options/content_page_view.cc (right):
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Consider renaming description_label so that it's consistent with
> the
> >>>>>>
> >>>>>> IDS tag
> >>>>>>>
> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
> >>>>>>
> >>>>>> oops, meant to do that. thanks.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081
> >>>>>
> >>>>
> >>>
> >>
> >
> >
>

beng (no_code_reviews)

I think all the sync dialogs should be 50% wider. -Ben On Wed, Oct 14, ...

11 years, 2 months ago (2009-10-14 21:47:13 UTC) #9

I think all the sync dialogs should be 50% wider.

-Ben

On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org> wrote:
> +UI team
> I really think it's weird to have all this extra spacing in the dialogs a=
nd
> the pref pane. Can't we resize these dynamically to adapt to the content?
> -Nick
>
> On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org> wrote:
>>
>> +cc the real nickbaum
>> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org> wrote:
>>>
>>> +cc nickbaum
>>> I had to up the size of the options dialog (in pixels, in english, it
>>> looks like the jump went from 610 to 650).
>>> I attached a screenshot of what this means for the 'Basics' pane.
>>> =A0Looking for an OK.
>>> Fixed button sizes.
>>>
>>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org> wrote:
>>>>
>>>> Hmm. Sizes of the buttons still look off :( =A0Sorry for thrashing on
>>>> this.
>>>>
>>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org> wrote:
>>>>>
>>>>> Screenshots attached forevermore. =A0I made the width of the buttons =
in
>>>>> all our dialogs consistent with the min width in native ui.
>>>>> However, trying to upload merge_and_sync.html is now throwing an erro=
r,
>>>>> saying "Got error status from ['svn', 'cat',
>>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>>>> I'm not sure why. =A0Looking into it.
>>>>> As for my changes to locale_resources.grd... I'm not sure what I'm
>>>>> supposed to do about all the translations.
>>>>>
>>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>>>>>>
>>>>>> fixed your points.
>>>>>>
>>>>>> still trying to get screenshots, the html in the final wizard dialog
>>>>>> is still
>>>>>> not right, the button falls off the screen with one version of the
>>>>>> text but not
>>>>>> the other. =A0I will keep twiddling... I wish I was an Oscar Mayer C=
SS
>>>>>> wizard.
>>>>>>
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/27
>>>>>> File chrome/app/generated_resources.grd (right):
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>>>>>> Line 5313:
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Extra newline.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To merge an=
d
>>>>>> sync your bookmarks to Chrome on
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Based on the examples I see elsewhere in this file, I think you
>>>>>>> should
>>>>>>
>>>>>> avoid
>>>>>>>
>>>>>>> having the newline in the middle of this string.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/33
>>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Indentation is off.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>>>>>
>>>>>> Ah! Done. =A0I just wanted to remove the TODO, must have forgotten t=
o do
>>>>>> that when adding that to OnUnrecoverableError.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> As above, this line is redundant.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/40
>>>>>> File chrome/browser/views/options/content_page_view.cc (right):
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Consider renaming description_label so that it's consistent with th=
e
>>>>>>
>>>>>> IDS tag
>>>>>>>
>>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>>>>
>>>>>> oops, meant to do that. thanks.
>>>>>>
>>>>>> http://codereview.chromium.org/270081
>>>>>
>>>>
>>>
>>
>
>

glen

If we can't, a stopgap solution would be to top-align the text and bottom-align the ...

11 years, 2 months ago (2009-10-14 21:47:37 UTC) #10

If we can't, a stopgap solution would be to top-align the text and
bottom-align the buttons.

I wonder if we could use the right half (the 'merged' image) of the
merge icon here to fill out some space, too.


On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org> wrote:
> +UI team
> I really think it's weird to have all this extra spacing in the dialogs a=
nd
> the pref pane. Can't we resize these dynamically to adapt to the content?
> -Nick
>
> On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org> wrote:
>>
>> +cc the real nickbaum
>> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org> wrote:
>>>
>>> +cc nickbaum
>>> I had to up the size of the options dialog (in pixels, in english, it
>>> looks like the jump went from 610 to 650).
>>> I attached a screenshot of what this means for the 'Basics' pane.
>>> =A0Looking for an OK.
>>> Fixed button sizes.
>>>
>>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org> wrote:
>>>>
>>>> Hmm. Sizes of the buttons still look off :( =A0Sorry for thrashing on
>>>> this.
>>>>
>>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org> wrote:
>>>>>
>>>>> Screenshots attached forevermore. =A0I made the width of the buttons =
in
>>>>> all our dialogs consistent with the min width in native ui.
>>>>> However, trying to upload merge_and_sync.html is now throwing an erro=
r,
>>>>> saying "Got error status from ['svn', 'cat',
>>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>>>> I'm not sure why. =A0Looking into it.
>>>>> As for my changes to locale_resources.grd... I'm not sure what I'm
>>>>> supposed to do about all the translations.
>>>>>
>>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>>>>>>
>>>>>> fixed your points.
>>>>>>
>>>>>> still trying to get screenshots, the html in the final wizard dialog
>>>>>> is still
>>>>>> not right, the button falls off the screen with one version of the
>>>>>> text but not
>>>>>> the other. =A0I will keep twiddling... I wish I was an Oscar Mayer C=
SS
>>>>>> wizard.
>>>>>>
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/27
>>>>>> File chrome/app/generated_resources.grd (right):
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>>>>>> Line 5313:
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Extra newline.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To merge an=
d
>>>>>> sync your bookmarks to Chrome on
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Based on the examples I see elsewhere in this file, I think you
>>>>>>> should
>>>>>>
>>>>>> avoid
>>>>>>>
>>>>>>> having the newline in the middle of this string.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/33
>>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Indentation is off.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>>>>>
>>>>>> Ah! Done. =A0I just wanted to remove the TODO, must have forgotten t=
o do
>>>>>> that when adding that to OnUnrecoverableError.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> As above, this line is redundant.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/40
>>>>>> File chrome/browser/views/options/content_page_view.cc (right):
>>>>>>
>>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
>>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>>>>
>>>>>>> Consider renaming description_label so that it's consistent with th=
e
>>>>>>
>>>>>> IDS tag
>>>>>>>
>>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>>>>
>>>>>> oops, meant to do that. thanks.
>>>>>>
>>>>>> http://codereview.chromium.org/270081
>>>>>
>>>>
>>>
>>
>
>

Nick Baum (Google)

We need to nail this ASAP if we want it in the upcoming Beta. Can ...

11 years, 2 months ago (2009-10-14 22:53:46 UTC) #11

We need to nail this ASAP if we want it in the upcoming Beta.
Can someone please just send mocks to Tim? He'll reply if it's not doable.

-Nick

On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com> wrote:

> I think all the sync dialogs should be 50% wider.
>
> -Ben
>
> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org> wrote:
> > +UI team
> > I really think it's weird to have all this extra spacing in the dialogs
> and
> > the pref pane. Can't we resize these dynamically to adapt to the content?
> > -Nick
> >
> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org> wrote:
> >>
> >> +cc the real nickbaum
> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org> wrote:
> >>>
> >>> +cc nickbaum
> >>> I had to up the size of the options dialog (in pixels, in english, it
> >>> looks like the jump went from 610 to 650).
> >>> I attached a screenshot of what this means for the 'Basics' pane.
> >>>  Looking for an OK.
> >>> Fixed button sizes.
> >>>
> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org> wrote:
> >>>>
> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for thrashing on
> >>>> this.
> >>>>
> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org> wrote:
> >>>>>
> >>>>> Screenshots attached forevermore.  I made the width of the buttons in
> >>>>> all our dialogs consistent with the min width in native ui.
> >>>>> However, trying to upload merge_and_sync.html is now throwing an
> error,
> >>>>> saying "Got error status from ['svn', 'cat',
> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
> >>>>> I'm not sure why.  Looking into it.
> >>>>> As for my changes to locale_resources.grd... I'm not sure what I'm
> >>>>> supposed to do about all the translations.
> >>>>>
> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
> >>>>>>
> >>>>>> fixed your points.
> >>>>>>
> >>>>>> still trying to get screenshots, the html in the final wizard dialog
> >>>>>> is still
> >>>>>> not right, the button falls off the screen with one version of the
> >>>>>> text but not
> >>>>>> the other.  I will keep twiddling... I wish I was an Oscar Mayer CSS
> >>>>>> wizard.
> >>>>>>
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/27
> >>>>>> File chrome/app/generated_resources.grd (right):
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
> >>>>>> Line 5313:
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Extra newline.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To merge
> and
> >>>>>> sync your bookmarks to Chrome on
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Based on the examples I see elsewhere in this file, I think you
> >>>>>>> should
> >>>>>>
> >>>>>> avoid
> >>>>>>>
> >>>>>>> having the newline in the middle of this string.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/33
> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Indentation is off.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
> >>>>>>
> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have forgotten to
> do
> >>>>>> that when adding that to OnUnrecoverableError.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> As above, this line is redundant.
> >>>>>>
> >>>>>> Done.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/40
> >>>>>> File chrome/browser/views/options/content_page_view.cc (right):
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>>>>
> >>>>>>> Consider renaming description_label so that it's consistent with
> the
> >>>>>>
> >>>>>> IDS tag
> >>>>>>>
> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
> >>>>>>
> >>>>>> oops, meant to do that. thanks.
> >>>>>>
> >>>>>> http://codereview.chromium.org/270081
> >>>>>
> >>>>
> >>>
> >>
> >
> >
>

Nick Baum (Google)

I like the login and merge, but the success one makes me cringe :) Resizing ...

11 years, 2 months ago (2009-10-15 03:52:45 UTC) #12

I like the login and merge, but the success one makes me cringe :)
Resizing is really hard?

-Nick

On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org> wrote:

> The correct (same width) screenshots.
>
>
> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org> wrote:
>
>> Some new shots for the login dialog. Made it wider / slightly shorter, and
>> top-aligned text.
>>
>>
>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <nickbaum@chromium.org> wrote:
>>
>>> We need to nail this ASAP if we want it in the upcoming Beta.
>>> Can someone please just send mocks to Tim? He'll reply if it's not
>>> doable.
>>>
>>> -Nick
>>>
>>>
>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com> wrote:
>>>
>>>> I think all the sync dialogs should be 50% wider.
>>>>
>>>> -Ben
>>>>
>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org>
>>>> wrote:
>>>> > +UI team
>>>> > I really think it's weird to have all this extra spacing in the
>>>> dialogs and
>>>> > the pref pane. Can't we resize these dynamically to adapt to the
>>>> content?
>>>> > -Nick
>>>> >
>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org>
>>>> wrote:
>>>> >>
>>>> >> +cc the real nickbaum
>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org>
>>>> wrote:
>>>> >>>
>>>> >>> +cc nickbaum
>>>> >>> I had to up the size of the options dialog (in pixels, in english,
>>>> it
>>>> >>> looks like the jump went from 610 to 650).
>>>> >>> I attached a screenshot of what this means for the 'Basics' pane.
>>>> >>>  Looking for an OK.
>>>> >>> Fixed button sizes.
>>>> >>>
>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org>
>>>> wrote:
>>>> >>>>
>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for thrashing on
>>>> >>>> this.
>>>> >>>>
>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org>
>>>> wrote:
>>>> >>>>>
>>>> >>>>> Screenshots attached forevermore.  I made the width of the buttons
>>>> in
>>>> >>>>> all our dialogs consistent with the min width in native ui.
>>>> >>>>> However, trying to upload merge_and_sync.html is now throwing an
>>>> error,
>>>> >>>>> saying "Got error status from ['svn', 'cat',
>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>>> >>>>> I'm not sure why.  Looking into it.
>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure what I'm
>>>> >>>>> supposed to do about all the translations.
>>>> >>>>>
>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>>>> >>>>>>
>>>> >>>>>> fixed your points.
>>>> >>>>>>
>>>> >>>>>> still trying to get screenshots, the html in the final wizard
>>>> dialog
>>>> >>>>>> is still
>>>> >>>>>> not right, the button falls off the screen with one version of
>>>> the
>>>> >>>>>> text but not
>>>> >>>>>> the other.  I will keep twiddling... I wish I was an Oscar Mayer
>>>> CSS
>>>> >>>>>> wizard.
>>>> >>>>>>
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>>>> >>>>>> Line 5313:
>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>>>>
>>>> >>>>>>> Extra newline.
>>>> >>>>>>
>>>> >>>>>> Done.
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To merge
>>>> and
>>>> >>>>>> sync your bookmarks to Chrome on
>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>>>>
>>>> >>>>>>> Based on the examples I see elsewhere in this file, I think you
>>>> >>>>>>> should
>>>> >>>>>>
>>>> >>>>>> avoid
>>>> >>>>>>>
>>>> >>>>>>> having the newline in the middle of this string.
>>>> >>>>>>
>>>> >>>>>> Done.
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>>>>
>>>> >>>>>>> Indentation is off.
>>>> >>>>>>
>>>> >>>>>> Done.
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>>>>
>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>>> >>>>>>
>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have forgotten
>>>> to do
>>>> >>>>>> that when adding that to OnUnrecoverableError.
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>>>>
>>>> >>>>>>> As above, this line is redundant.
>>>> >>>>>>
>>>> >>>>>> Done.
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc (right):
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>>>> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>>>>
>>>> >>>>>>> Consider renaming description_label so that it's consistent with
>>>> the
>>>> >>>>>>
>>>> >>>>>> IDS tag
>>>> >>>>>>>
>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>> >>>>>>
>>>> >>>>>> oops, meant to do that. thanks.
>>>> >>>>>>
>>>> >>>>>> http://codereview.chromium.org/270081
>>>> >>>>>
>>>> >>>>
>>>> >>>
>>>> >>
>>>> >
>>>> >
>>>>
>>>
>>>
>>
>

beng (no_code_reviews)

I don't like them changing size... however... on the success page, do we have a ...

11 years, 2 months ago (2009-10-15 03:56:05 UTC) #13

I don't like them changing size... however...

on the success page, do we have a large green circle with a checkmark
in it somewhere? Don't we use one of those in omaha? If so, I would
insert it centered above the "Synced to..." text.

-Ben

On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org> wrote:
> I like the login and merge, but the success one makes me cringe :)
> Resizing is really hard?
> -Nick
>
> On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org> wrote:
>>
>> The correct (same width) screenshots.
>>
>> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org> wrote:
>>>
>>> Some new shots for the login dialog. Made it wider / slightly shorter,
>>> and top-aligned text.
>>>
>>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <nickbaum@chromium.org> wrot=
e:
>>>>
>>>> We need to nail this ASAP if we want it in the upcoming Beta.
>>>> Can someone please just send mocks to Tim? He'll reply if it's not
>>>> doable.
>>>>
>>>> -Nick
>>>>
>>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com> wrote:
>>>>>
>>>>> I think all the sync dialogs should be 50% wider.
>>>>>
>>>>> -Ben
>>>>>
>>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org>
>>>>> wrote:
>>>>> > +UI team
>>>>> > I really think it's weird to have all this extra spacing in the
>>>>> > dialogs and
>>>>> > the pref pane. Can't we resize these dynamically to adapt to the
>>>>> > content?
>>>>> > -Nick
>>>>> >
>>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org>
>>>>> > wrote:
>>>>> >>
>>>>> >> +cc the real nickbaum
>>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org>
>>>>> >> wrote:
>>>>> >>>
>>>>> >>> +cc nickbaum
>>>>> >>> I had to up the size of the options dialog (in pixels, in english=
,
>>>>> >>> it
>>>>> >>> looks like the jump went from 610 to 650).
>>>>> >>> I attached a screenshot of what this means for the 'Basics' pane.
>>>>> >>> =A0Looking for an OK.
>>>>> >>> Fixed button sizes.
>>>>> >>>
>>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org>
>>>>> >>> wrote:
>>>>> >>>>
>>>>> >>>> Hmm. Sizes of the buttons still look off :( =A0Sorry for thrashi=
ng
>>>>> >>>> on
>>>>> >>>> this.
>>>>> >>>>
>>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org>
>>>>> >>>> wrote:
>>>>> >>>>>
>>>>> >>>>> Screenshots attached forevermore. =A0I made the width of the
>>>>> >>>>> buttons in
>>>>> >>>>> all our dialogs consistent with the min width in native ui.
>>>>> >>>>> However, trying to upload merge_and_sync.html is now throwing a=
n
>>>>> >>>>> error,
>>>>> >>>>> saying "Got error status from ['svn', 'cat',
>>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>>>> >>>>> I'm not sure why. =A0Looking into it.
>>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure what
>>>>> >>>>> I'm
>>>>> >>>>> supposed to do about all the translations.
>>>>> >>>>>
>>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>>>>> >>>>>>
>>>>> >>>>>> fixed your points.
>>>>> >>>>>>
>>>>> >>>>>> still trying to get screenshots, the html in the final wizard
>>>>> >>>>>> dialog
>>>>> >>>>>> is still
>>>>> >>>>>> not right, the button falls off the screen with one version of
>>>>> >>>>>> the
>>>>> >>>>>> text but not
>>>>> >>>>>> the other. =A0I will keep twiddling... I wish I was an Oscar M=
ayer
>>>>> >>>>>> CSS
>>>>> >>>>>> wizard.
>>>>> >>>>>>
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>>>>> >>>>>> Line 5313:
>>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>>>>
>>>>> >>>>>>> Extra newline.
>>>>> >>>>>>
>>>>> >>>>>> Done.
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To
>>>>> >>>>>> merge and
>>>>> >>>>>> sync your bookmarks to Chrome on
>>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>>>>
>>>>> >>>>>>> Based on the examples I see elsewhere in this file, I think y=
ou
>>>>> >>>>>>> should
>>>>> >>>>>>
>>>>> >>>>>> avoid
>>>>> >>>>>>>
>>>>> >>>>>>> having the newline in the middle of this string.
>>>>> >>>>>>
>>>>> >>>>>> Done.
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>>>>
>>>>> >>>>>>> Indentation is off.
>>>>> >>>>>>
>>>>> >>>>>> Done.
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>>>>
>>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>>>> >>>>>>
>>>>> >>>>>> Ah! Done. =A0I just wanted to remove the TODO, must have forgo=
tten
>>>>> >>>>>> to do
>>>>> >>>>>> that when adding that to OnUnrecoverableError.
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>>>>
>>>>> >>>>>>> As above, this line is redundant.
>>>>> >>>>>>
>>>>> >>>>>> Done.
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc (right)=
:
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>>>>> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
>>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>>>>
>>>>> >>>>>>> Consider renaming description_label so that it's consistent
>>>>> >>>>>>> with the
>>>>> >>>>>>
>>>>> >>>>>> IDS tag
>>>>> >>>>>>>
>>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>>> >>>>>>
>>>>> >>>>>> oops, meant to do that. thanks.
>>>>> >>>>>>
>>>>> >>>>>> http://codereview.chromium.org/270081
>>>>> >>>>>
>>>>> >>>>
>>>>> >>>
>>>>> >>
>>>>> >
>>>>> >
>>>>
>>>
>>
>
>

tim (not reviewing)

I will hunt for a green checkmark. Also, I uploaded a patchset with the last-minute ...

11 years, 2 months ago (2009-10-15 04:00:52 UTC) #14

I will hunt for a green checkmark.

Also, I uploaded a patchset with the last-minute NTP UI removal mentioned
in http://crbug.com/24858.

On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com> wrote:

> I don't like them changing size... however...
>
> on the success page, do we have a large green circle with a checkmark
> in it somewhere? Don't we use one of those in omaha? If so, I would
> insert it centered above the "Synced to..." text.
>
> -Ben
>
> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org> wrote:
> > I like the login and merge, but the success one makes me cringe :)
> > Resizing is really hard?
> > -Nick
> >
> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org> wrote:
> >>
> >> The correct (same width) screenshots.
> >>
> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org> wrote:
> >>>
> >>> Some new shots for the login dialog. Made it wider / slightly shorter,
> >>> and top-aligned text.
> >>>
> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <nickbaum@chromium.org>
> wrote:
> >>>>
> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
> >>>> Can someone please just send mocks to Tim? He'll reply if it's not
> >>>> doable.
> >>>>
> >>>> -Nick
> >>>>
> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com> wrote:
> >>>>>
> >>>>> I think all the sync dialogs should be 50% wider.
> >>>>>
> >>>>> -Ben
> >>>>>
> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org>
> >>>>> wrote:
> >>>>> > +UI team
> >>>>> > I really think it's weird to have all this extra spacing in the
> >>>>> > dialogs and
> >>>>> > the pref pane. Can't we resize these dynamically to adapt to the
> >>>>> > content?
> >>>>> > -Nick
> >>>>> >
> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org>
> >>>>> > wrote:
> >>>>> >>
> >>>>> >> +cc the real nickbaum
> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org>
> >>>>> >> wrote:
> >>>>> >>>
> >>>>> >>> +cc nickbaum
> >>>>> >>> I had to up the size of the options dialog (in pixels, in
> english,
> >>>>> >>> it
> >>>>> >>> looks like the jump went from 610 to 650).
> >>>>> >>> I attached a screenshot of what this means for the 'Basics' pane.
> >>>>> >>>  Looking for an OK.
> >>>>> >>> Fixed button sizes.
> >>>>> >>>
> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org>
> >>>>> >>> wrote:
> >>>>> >>>>
> >>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for thrashing
> >>>>> >>>> on
> >>>>> >>>> this.
> >>>>> >>>>
> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org>
> >>>>> >>>> wrote:
> >>>>> >>>>>
> >>>>> >>>>> Screenshots attached forevermore.  I made the width of the
> >>>>> >>>>> buttons in
> >>>>> >>>>> all our dialogs consistent with the min width in native ui.
> >>>>> >>>>> However, trying to upload merge_and_sync.html is now throwing
> an
> >>>>> >>>>> error,
> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
> >>>>> >>>>> I'm not sure why.  Looking into it.
> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure what
> >>>>> >>>>> I'm
> >>>>> >>>>> supposed to do about all the translations.
> >>>>> >>>>>
> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
> >>>>> >>>>>>
> >>>>> >>>>>> fixed your points.
> >>>>> >>>>>>
> >>>>> >>>>>> still trying to get screenshots, the html in the final wizard
> >>>>> >>>>>> dialog
> >>>>> >>>>>> is still
> >>>>> >>>>>> not right, the button falls off the screen with one version of
> >>>>> >>>>>> the
> >>>>> >>>>>> text but not
> >>>>> >>>>>> the other.  I will keep twiddling... I wish I was an Oscar
> Mayer
> >>>>> >>>>>> CSS
> >>>>> >>>>>> wizard.
> >>>>> >>>>>>
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
> >>>>> >>>>>> Line 5313:
> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>>>>
> >>>>> >>>>>>> Extra newline.
> >>>>> >>>>>>
> >>>>> >>>>>> Done.
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To
> >>>>> >>>>>> merge and
> >>>>> >>>>>> sync your bookmarks to Chrome on
> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>>>>
> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, I think
> you
> >>>>> >>>>>>> should
> >>>>> >>>>>>
> >>>>> >>>>>> avoid
> >>>>> >>>>>>>
> >>>>> >>>>>>> having the newline in the middle of this string.
> >>>>> >>>>>>
> >>>>> >>>>>> Done.
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>>>>
> >>>>> >>>>>>> Indentation is off.
> >>>>> >>>>>>
> >>>>> >>>>>> Done.
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>>>>
> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
> >>>>> >>>>>>
> >>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have
> forgotten
> >>>>> >>>>>> to do
> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>>>>
> >>>>> >>>>>>> As above, this line is redundant.
> >>>>> >>>>>>
> >>>>> >>>>>> Done.
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc
> (right):
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
> >>>>> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>>>>
> >>>>> >>>>>>> Consider renaming description_label so that it's consistent
> >>>>> >>>>>>> with the
> >>>>> >>>>>>
> >>>>> >>>>>> IDS tag
> >>>>> >>>>>>>
> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
> >>>>> >>>>>>
> >>>>> >>>>>> oops, meant to do that. thanks.
> >>>>> >>>>>>
> >>>>> >>>>>> http://codereview.chromium.org/270081
> >>>>> >>>>>
> >>>>> >>>>
> >>>>> >>>
> >>>>> >>
> >>>>> >
> >>>>> >
> >>>>
> >>>
> >>
> >
> >
>

tim (not reviewing)

I added the app menu changes (this is toolbar_view.cc) from http://crbug.com/24858. I messaged Glen & ...

11 years, 2 months ago (2009-10-15 04:52:19 UTC) #15

I added the app menu changes (this is toolbar_view.cc) from
http://crbug.com/24858.
I messaged Glen & Cole separately about designing me a big beautiful round
green checkmark for this bandwagon patch headed for Taco Town.

On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrote:

> I will hunt for a green checkmark.
>
> Also, I uploaded a patchset with the last-minute NTP UI removal mentioned
> in http://crbug.com/24858.
>
> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com> wrote:
>
>> I don't like them changing size... however...
>>
>> on the success page, do we have a large green circle with a checkmark
>> in it somewhere? Don't we use one of those in omaha? If so, I would
>> insert it centered above the "Synced to..." text.
>>
>> -Ben
>>
>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org> wrote:
>> > I like the login and merge, but the success one makes me cringe :)
>> > Resizing is really hard?
>> > -Nick
>> >
>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org> wrote:
>> >>
>> >> The correct (same width) screenshots.
>> >>
>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org> wrote:
>> >>>
>> >>> Some new shots for the login dialog. Made it wider / slightly shorter,
>> >>> and top-aligned text.
>> >>>
>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <nickbaum@chromium.org>
>> wrote:
>> >>>>
>> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
>> >>>> Can someone please just send mocks to Tim? He'll reply if it's not
>> >>>> doable.
>> >>>>
>> >>>> -Nick
>> >>>>
>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com>
>> wrote:
>> >>>>>
>> >>>>> I think all the sync dialogs should be 50% wider.
>> >>>>>
>> >>>>> -Ben
>> >>>>>
>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org>
>> >>>>> wrote:
>> >>>>> > +UI team
>> >>>>> > I really think it's weird to have all this extra spacing in the
>> >>>>> > dialogs and
>> >>>>> > the pref pane. Can't we resize these dynamically to adapt to the
>> >>>>> > content?
>> >>>>> > -Nick
>> >>>>> >
>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org>
>> >>>>> > wrote:
>> >>>>> >>
>> >>>>> >> +cc the real nickbaum
>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org>
>> >>>>> >> wrote:
>> >>>>> >>>
>> >>>>> >>> +cc nickbaum
>> >>>>> >>> I had to up the size of the options dialog (in pixels, in
>> english,
>> >>>>> >>> it
>> >>>>> >>> looks like the jump went from 610 to 650).
>> >>>>> >>> I attached a screenshot of what this means for the 'Basics'
>> pane.
>> >>>>> >>>  Looking for an OK.
>> >>>>> >>> Fixed button sizes.
>> >>>>> >>>
>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org>
>> >>>>> >>> wrote:
>> >>>>> >>>>
>> >>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for
>> thrashing
>> >>>>> >>>> on
>> >>>>> >>>> this.
>> >>>>> >>>>
>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org>
>> >>>>> >>>> wrote:
>> >>>>> >>>>>
>> >>>>> >>>>> Screenshots attached forevermore.  I made the width of the
>> >>>>> >>>>> buttons in
>> >>>>> >>>>> all our dialogs consistent with the min width in native ui.
>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now throwing
>> an
>> >>>>> >>>>> error,
>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>> >>>>> >>>>> I'm not sure why.  Looking into it.
>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure what
>> >>>>> >>>>> I'm
>> >>>>> >>>>> supposed to do about all the translations.
>> >>>>> >>>>>
>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>> >>>>> >>>>>>
>> >>>>> >>>>>> fixed your points.
>> >>>>> >>>>>>
>> >>>>> >>>>>> still trying to get screenshots, the html in the final wizard
>> >>>>> >>>>>> dialog
>> >>>>> >>>>>> is still
>> >>>>> >>>>>> not right, the button falls off the screen with one version
>> of
>> >>>>> >>>>>> the
>> >>>>> >>>>>> text but not
>> >>>>> >>>>>> the other.  I will keep twiddling... I wish I was an Oscar
>> Mayer
>> >>>>> >>>>>> CSS
>> >>>>> >>>>>> wizard.
>> >>>>> >>>>>>
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>> >>>>> >>>>>> Line 5313:
>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>>>>
>> >>>>> >>>>>>> Extra newline.
>> >>>>> >>>>>>
>> >>>>> >>>>>> Done.
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To
>> >>>>> >>>>>> merge and
>> >>>>> >>>>>> sync your bookmarks to Chrome on
>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>>>>
>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, I think
>> you
>> >>>>> >>>>>>> should
>> >>>>> >>>>>>
>> >>>>> >>>>>> avoid
>> >>>>> >>>>>>>
>> >>>>> >>>>>>> having the newline in the middle of this string.
>> >>>>> >>>>>>
>> >>>>> >>>>>> Done.
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>>>>
>> >>>>> >>>>>>> Indentation is off.
>> >>>>> >>>>>>
>> >>>>> >>>>>> Done.
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>>>>
>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>> >>>>> >>>>>>
>> >>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have
>> forgotten
>> >>>>> >>>>>> to do
>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>>>>
>> >>>>> >>>>>>> As above, this line is redundant.
>> >>>>> >>>>>>
>> >>>>> >>>>>> Done.
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc
>> (right):
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>> >>>>> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>>>>
>> >>>>> >>>>>>> Consider renaming description_label so that it's consistent
>> >>>>> >>>>>>> with the
>> >>>>> >>>>>>
>> >>>>> >>>>>> IDS tag
>> >>>>> >>>>>>>
>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>> >>>>> >>>>>>
>> >>>>> >>>>>> oops, meant to do that. thanks.
>> >>>>> >>>>>>
>> >>>>> >>>>>> http://codereview.chromium.org/270081
>> >>>>> >>>>>
>> >>>>> >>>>
>> >>>>> >>>
>> >>>>> >>
>> >>>>> >
>> >>>>> >
>> >>>>
>> >>>
>> >>
>> >
>> >
>>
>
>

Nick Baum (Google)

Awesome, thanks for driving this. -Nick On Wed, Oct 14, 2009 at 9:52 PM, Tim ...

11 years, 2 months ago (2009-10-15 17:16:31 UTC) #16

Awesome, thanks for driving this.

-Nick

On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org> wrote:

> I added the app menu changes (this is toolbar_view.cc) from
> http://crbug.com/24858.
> I messaged Glen & Cole separately about designing me a big beautiful round
> green checkmark for this bandwagon patch headed for Taco Town.
>
>
> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrote:
>
>> I will hunt for a green checkmark.
>>
>> Also, I uploaded a patchset with the last-minute NTP UI removal mentioned
>> in http://crbug.com/24858.
>>
>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com> wrote:
>>
>>> I don't like them changing size... however...
>>>
>>> on the success page, do we have a large green circle with a checkmark
>>> in it somewhere? Don't we use one of those in omaha? If so, I would
>>> insert it centered above the "Synced to..." text.
>>>
>>> -Ben
>>>
>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org>
>>> wrote:
>>> > I like the login and merge, but the success one makes me cringe :)
>>> > Resizing is really hard?
>>> > -Nick
>>> >
>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org> wrote:
>>> >>
>>> >> The correct (same width) screenshots.
>>> >>
>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org> wrote:
>>> >>>
>>> >>> Some new shots for the login dialog. Made it wider / slightly
>>> shorter,
>>> >>> and top-aligned text.
>>> >>>
>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <nickbaum@chromium.org>
>>> wrote:
>>> >>>>
>>> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
>>> >>>> Can someone please just send mocks to Tim? He'll reply if it's not
>>> >>>> doable.
>>> >>>>
>>> >>>> -Nick
>>> >>>>
>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com>
>>> wrote:
>>> >>>>>
>>> >>>>> I think all the sync dialogs should be 50% wider.
>>> >>>>>
>>> >>>>> -Ben
>>> >>>>>
>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.org>
>>> >>>>> wrote:
>>> >>>>> > +UI team
>>> >>>>> > I really think it's weird to have all this extra spacing in the
>>> >>>>> > dialogs and
>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt to the
>>> >>>>> > content?
>>> >>>>> > -Nick
>>> >>>>> >
>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org>
>>> >>>>> > wrote:
>>> >>>>> >>
>>> >>>>> >> +cc the real nickbaum
>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.org>
>>> >>>>> >> wrote:
>>> >>>>> >>>
>>> >>>>> >>> +cc nickbaum
>>> >>>>> >>> I had to up the size of the options dialog (in pixels, in
>>> english,
>>> >>>>> >>> it
>>> >>>>> >>> looks like the jump went from 610 to 650).
>>> >>>>> >>> I attached a screenshot of what this means for the 'Basics'
>>> pane.
>>> >>>>> >>>  Looking for an OK.
>>> >>>>> >>> Fixed button sizes.
>>> >>>>> >>>
>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.org>
>>> >>>>> >>> wrote:
>>> >>>>> >>>>
>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for
>>> thrashing
>>> >>>>> >>>> on
>>> >>>>> >>>> this.
>>> >>>>> >>>>
>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele <tim@chromium.org
>>> >
>>> >>>>> >>>> wrote:
>>> >>>>> >>>>>
>>> >>>>> >>>>> Screenshots attached forevermore.  I made the width of the
>>> >>>>> >>>>> buttons in
>>> >>>>> >>>>> all our dialogs consistent with the min width in native ui.
>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now throwing
>>> an
>>> >>>>> >>>>> error,
>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>> >>>>> >>>>> I'm not sure why.  Looking into it.
>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure
>>> what
>>> >>>>> >>>>> I'm
>>> >>>>> >>>>> supposed to do about all the translations.
>>> >>>>> >>>>>
>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> fixed your points.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> still trying to get screenshots, the html in the final
>>> wizard
>>> >>>>> >>>>>> dialog
>>> >>>>> >>>>>> is still
>>> >>>>> >>>>>> not right, the button falls off the screen with one version
>>> of
>>> >>>>> >>>>>> the
>>> >>>>> >>>>>> text but not
>>> >>>>> >>>>>> the other.  I will keep twiddling... I wish I was an Oscar
>>> Mayer
>>> >>>>> >>>>>> CSS
>>> >>>>> >>>>>> wizard.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>>> >>>>> >>>>>>
>>> >>>>> >>>>>>
>>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>>> >>>>> >>>>>> Line 5313:
>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>>>>
>>> >>>>> >>>>>>> Extra newline.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> Done.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>>
>>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs! To
>>> >>>>> >>>>>> merge and
>>> >>>>> >>>>>> sync your bookmarks to Chrome on
>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>>>>
>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, I think
>>> you
>>> >>>>> >>>>>>> should
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> avoid
>>> >>>>> >>>>>>>
>>> >>>>> >>>>>>> having the newline in the middle of this string.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> Done.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>>>>
>>> >>>>> >>>>>>> Indentation is off.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> Done.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>>>>
>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have
>>> forgotten
>>> >>>>> >>>>>> to do
>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>>>>
>>> >>>>> >>>>>>> As above, this line is redundant.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> Done.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc
>>> (right):
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>>> >>>>> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false);
>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>>>>
>>> >>>>> >>>>>>> Consider renaming description_label so that it's consistent
>>> >>>>> >>>>>>> with the
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> IDS tag
>>> >>>>> >>>>>>>
>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> oops, meant to do that. thanks.
>>> >>>>> >>>>>>
>>> >>>>> >>>>>> http://codereview.chromium.org/270081
>>> >>>>> >>>>>
>>> >>>>> >>>>
>>> >>>>> >>>
>>> >>>>> >>
>>> >>>>> >
>>> >>>>> >
>>> >>>>
>>> >>>
>>> >>
>>> >
>>> >
>>>
>>
>>
>

beng (no_code_reviews)

That's jpeg compression on the screenshot, not the check, right? If so, LGTM. -Ben On ...

11 years, 2 months ago (2009-10-15 17:37:27 UTC) #17

That's jpeg compression on the screenshot, not the check, right?

If so, LGTM.

-Ben

On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org> wrote:
> drumroll please...
>
> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org> wrote:
>>
>> I added the app menu changes (this is toolbar_view.cc) from
>> http://crbug.com/24858.
>> I messaged Glen & Cole separately about designing me a big beautiful rou=
nd
>> green checkmark for this bandwagon patch headed for Taco Town.
>>
>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrote:
>>>
>>> I will hunt for a green checkmark.
>>> Also, I uploaded a patchset with the last-minute NTP UI removal mention=
ed
>>> in=A0http://crbug.com/24858.
>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com> wrote:
>>>>
>>>> I don't like them changing size... however...
>>>>
>>>> on the success page, do we have a large green circle with a checkmark
>>>> in it somewhere? Don't we use one of those in omaha? If so, I would
>>>> insert it centered above the "Synced to..." text.
>>>>
>>>> -Ben
>>>>
>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org>
>>>> wrote:
>>>> > I like the login and merge, but the success one makes me cringe :)
>>>> > Resizing is really hard?
>>>> > -Nick
>>>> >
>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org> wrote=
:
>>>> >>
>>>> >> The correct (same width) screenshots.
>>>> >>
>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org> wrot=
e:
>>>> >>>
>>>> >>> Some new shots for the login dialog. Made it wider / slightly
>>>> >>> shorter,
>>>> >>> and top-aligned text.
>>>> >>>
>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <nickbaum@chromium.org>
>>>> >>> wrote:
>>>> >>>>
>>>> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
>>>> >>>> Can someone please just send mocks to Tim? He'll reply if it's no=
t
>>>> >>>> doable.
>>>> >>>>
>>>> >>>> -Nick
>>>> >>>>
>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com>
>>>> >>>> wrote:
>>>> >>>>>
>>>> >>>>> I think all the sync dialogs should be 50% wider.
>>>> >>>>>
>>>> >>>>> -Ben
>>>> >>>>>
>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.or=
g>
>>>> >>>>> wrote:
>>>> >>>>> > +UI team
>>>> >>>>> > I really think it's weird to have all this extra spacing in th=
e
>>>> >>>>> > dialogs and
>>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt to t=
he
>>>> >>>>> > content?
>>>> >>>>> > -Nick
>>>> >>>>> >
>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.org=
>
>>>> >>>>> > wrote:
>>>> >>>>> >>
>>>> >>>>> >> +cc the real nickbaum
>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.or=
g>
>>>> >>>>> >> wrote:
>>>> >>>>> >>>
>>>> >>>>> >>> +cc nickbaum
>>>> >>>>> >>> I had to up the size of the options dialog (in pixels, in
>>>> >>>>> >>> english,
>>>> >>>>> >>> it
>>>> >>>>> >>> looks like the jump went from 610 to 650).
>>>> >>>>> >>> I attached a screenshot of what this means for the 'Basics'
>>>> >>>>> >>> pane.
>>>> >>>>> >>> =A0Looking for an OK.
>>>> >>>>> >>> Fixed button sizes.
>>>> >>>>> >>>
>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.or=
g>
>>>> >>>>> >>> wrote:
>>>> >>>>> >>>>
>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :( =A0Sorry for
>>>> >>>>> >>>> thrashing
>>>> >>>>> >>>> on
>>>> >>>>> >>>> this.
>>>> >>>>> >>>>
>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
>>>> >>>>> >>>> <tim@chromium.org>
>>>> >>>>> >>>> wrote:
>>>> >>>>> >>>>>
>>>> >>>>> >>>>> Screenshots attached forevermore. =A0I made the width of t=
he
>>>> >>>>> >>>>> buttons in
>>>> >>>>> >>>>> all our dialogs consistent with the min width in native ui=
.
>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now
>>>> >>>>> >>>>> throwing an
>>>> >>>>> >>>>> error,
>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>>> >>>>> >>>>> I'm not sure why. =A0Looking into it.
>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure
>>>> >>>>> >>>>> what
>>>> >>>>> >>>>> I'm
>>>> >>>>> >>>>> supposed to do about all the translations.
>>>> >>>>> >>>>>
>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote:
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> fixed your points.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> still trying to get screenshots, the html in the final
>>>> >>>>> >>>>>> wizard
>>>> >>>>> >>>>>> dialog
>>>> >>>>> >>>>>> is still
>>>> >>>>> >>>>>> not right, the button falls off the screen with one versi=
on
>>>> >>>>> >>>>>> of
>>>> >>>>> >>>>>> the
>>>> >>>>> >>>>>> text but not
>>>> >>>>> >>>>>> the other. =A0I will keep twiddling... I wish I was an Os=
car
>>>> >>>>> >>>>>> Mayer
>>>> >>>>> >>>>>> CSS
>>>> >>>>> >>>>>> wizard.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5=
313
>>>> >>>>> >>>>>> Line 5313:
>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>> >>>>>>>
>>>> >>>>> >>>>>>> Extra newline.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> Done.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode5=
331
>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs! =
To
>>>> >>>>> >>>>>> merge and
>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>> >>>>>>>
>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, I
>>>> >>>>> >>>>>>> think you
>>>> >>>>> >>>>>>> should
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> avoid
>>>> >>>>> >>>>>>>
>>>> >>>>> >>>>>>> having the newline in the middle of this string.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> Done.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right):
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode3=
44
>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>> >>>>>>>
>>>> >>>>> >>>>>>> Indentation is off.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> Done.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode3=
45
>>>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>> >>>>>>>
>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> Ah! Done. =A0I just wanted to remove the TODO, must have
>>>> >>>>> >>>>>> forgotten
>>>> >>>>> >>>>>> to do
>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode3=
96
>>>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>> >>>>>>>
>>>> >>>>> >>>>>>> As above, this line is redundant.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> Done.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>>>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc
>>>> >>>>> >>>>>> (right):
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode4=
73
>>>> >>>>> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false)=
;
>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>> >>>>> >>>>>>>
>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
>>>> >>>>> >>>>>>> consistent
>>>> >>>>> >>>>>>> with the
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> IDS tag
>>>> >>>>> >>>>>>>
>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> oops, meant to do that. thanks.
>>>> >>>>> >>>>>>
>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
>>>> >>>>> >>>>>
>>>> >>>>> >>>>
>>>> >>>>> >>>
>>>> >>>>> >>
>>>> >>>>> >
>>>> >>>>> >
>>>> >>>>
>>>> >>>
>>>> >>
>>>> >
>>>> >
>>>
>>
>
>

beng (no_code_reviews)

Also, your OK button should have "default button" style (blue ring) and have focus. -Ben ...

11 years, 2 months ago (2009-10-15 17:38:18 UTC) #18

Also, your OK button should have "default button" style (blue ring)
and have focus.

-Ben

On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com> wrote:
> That's jpeg compression on the screenshot, not the check, right?
>
> If so, LGTM.
>
> -Ben
>
> On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org> wrote:
>> drumroll please...
>>
>> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org> wrote:
>>>
>>> I added the app menu changes (this is toolbar_view.cc) from
>>> http://crbug.com/24858.
>>> I messaged Glen & Cole separately about designing me a big beautiful ro=
und
>>> green checkmark for this bandwagon patch headed for Taco Town.
>>>
>>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrote:
>>>>
>>>> I will hunt for a green checkmark.
>>>> Also, I uploaded a patchset with the last-minute NTP UI removal mentio=
ned
>>>> in=A0http://crbug.com/24858.
>>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com> wrote:
>>>>>
>>>>> I don't like them changing size... however...
>>>>>
>>>>> on the success page, do we have a large green circle with a checkmark
>>>>> in it somewhere? Don't we use one of those in omaha? If so, I would
>>>>> insert it centered above the "Synced to..." text.
>>>>>
>>>>> -Ben
>>>>>
>>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org>
>>>>> wrote:
>>>>> > I like the login and merge, but the success one makes me cringe :)
>>>>> > Resizing is really hard?
>>>>> > -Nick
>>>>> >
>>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org> wrot=
e:
>>>>> >>
>>>>> >> The correct (same width) screenshots.
>>>>> >>
>>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org> wro=
te:
>>>>> >>>
>>>>> >>> Some new shots for the login dialog. Made it wider / slightly
>>>>> >>> shorter,
>>>>> >>> and top-aligned text.
>>>>> >>>
>>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <nickbaum@chromium.org=
>
>>>>> >>> wrote:
>>>>> >>>>
>>>>> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
>>>>> >>>> Can someone please just send mocks to Tim? He'll reply if it's n=
ot
>>>>> >>>> doable.
>>>>> >>>>
>>>>> >>>> -Nick
>>>>> >>>>
>>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com>
>>>>> >>>> wrote:
>>>>> >>>>>
>>>>> >>>>> I think all the sync dialogs should be 50% wider.
>>>>> >>>>>
>>>>> >>>>> -Ben
>>>>> >>>>>
>>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <nickbaum@chromium.o=
rg>
>>>>> >>>>> wrote:
>>>>> >>>>> > +UI team
>>>>> >>>>> > I really think it's weird to have all this extra spacing in t=
he
>>>>> >>>>> > dialogs and
>>>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt to =
the
>>>>> >>>>> > content?
>>>>> >>>>> > -Nick
>>>>> >>>>> >
>>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <tim@chromium.or=
g>
>>>>> >>>>> > wrote:
>>>>> >>>>> >>
>>>>> >>>>> >> +cc the real nickbaum
>>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <tim@chromium.o=
rg>
>>>>> >>>>> >> wrote:
>>>>> >>>>> >>>
>>>>> >>>>> >>> +cc nickbaum
>>>>> >>>>> >>> I had to up the size of the options dialog (in pixels, in
>>>>> >>>>> >>> english,
>>>>> >>>>> >>> it
>>>>> >>>>> >>> looks like the jump went from 610 to 650).
>>>>> >>>>> >>> I attached a screenshot of what this means for the 'Basics'
>>>>> >>>>> >>> pane.
>>>>> >>>>> >>> =A0Looking for an OK.
>>>>> >>>>> >>> Fixed button sizes.
>>>>> >>>>> >>>
>>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <tim@chromium.o=
rg>
>>>>> >>>>> >>> wrote:
>>>>> >>>>> >>>>
>>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :( =A0Sorry for
>>>>> >>>>> >>>> thrashing
>>>>> >>>>> >>>> on
>>>>> >>>>> >>>> this.
>>>>> >>>>> >>>>
>>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
>>>>> >>>>> >>>> <tim@chromium.org>
>>>>> >>>>> >>>> wrote:
>>>>> >>>>> >>>>>
>>>>> >>>>> >>>>> Screenshots attached forevermore. =A0I made the width of =
the
>>>>> >>>>> >>>>> buttons in
>>>>> >>>>> >>>>> all our dialogs consistent with the min width in native u=
i.
>>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now
>>>>> >>>>> >>>>> throwing an
>>>>> >>>>> >>>>> error,
>>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>>>>> >>>>> >>>>> I'm not sure why. =A0Looking into it.
>>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure
>>>>> >>>>> >>>>> what
>>>>> >>>>> >>>>> I'm
>>>>> >>>>> >>>>> supposed to do about all the translations.
>>>>> >>>>> >>>>>
>>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org> wrote=
:
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> fixed your points.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> still trying to get screenshots, the html in the final
>>>>> >>>>> >>>>>> wizard
>>>>> >>>>> >>>>>> dialog
>>>>> >>>>> >>>>>> is still
>>>>> >>>>> >>>>>> not right, the button falls off the screen with one vers=
ion
>>>>> >>>>> >>>>>> of
>>>>> >>>>> >>>>>> the
>>>>> >>>>> >>>>>> text but not
>>>>> >>>>> >>>>>> the other. =A0I will keep twiddling... I wish I was an O=
scar
>>>>> >>>>> >>>>>> Mayer
>>>>> >>>>> >>>>>> CSS
>>>>> >>>>> >>>>>> wizard.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode=
5313
>>>>> >>>>> >>>>>> Line 5313:
>>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>> >>>>>>>
>>>>> >>>>> >>>>>>> Extra newline.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> Done.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newcode=
5331
>>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs!=
 To
>>>>> >>>>> >>>>>> merge and
>>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
>>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>> >>>>>>>
>>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, I
>>>>> >>>>> >>>>>>> think you
>>>>> >>>>> >>>>>>> should
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> avoid
>>>>> >>>>> >>>>>>>
>>>>> >>>>> >>>>>>> having the newline in the middle of this string.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> Done.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc (right)=
:
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode=
344
>>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>> >>>>>>>
>>>>> >>>>> >>>>>>> Indentation is off.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> Done.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode=
345
>>>>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>> >>>>>>>
>>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> Ah! Done. =A0I just wanted to remove the TODO, must have
>>>>> >>>>> >>>>>> forgotten
>>>>> >>>>> >>>>>> to do
>>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newcode=
396
>>>>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>> >>>>>>>
>>>>> >>>>> >>>>>>> As above, this line is redundant.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> Done.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>>>>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc
>>>>> >>>>> >>>>>> (right):
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newcode=
473
>>>>> >>>>> >>>>>> Line 473: stop_sync_description_label_->SetVisible(false=
);
>>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>>>> >>>>> >>>>>>>
>>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
>>>>> >>>>> >>>>>>> consistent
>>>>> >>>>> >>>>>>> with the
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> IDS tag
>>>>> >>>>> >>>>>>>
>>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> oops, meant to do that. thanks.
>>>>> >>>>> >>>>>>
>>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
>>>>> >>>>> >>>>>
>>>>> >>>>> >>>>
>>>>> >>>>> >>>
>>>>> >>>>> >>
>>>>> >>>>> >
>>>>> >>>>> >
>>>>> >>>>
>>>>> >>>
>>>>> >>
>>>>> >
>>>>> >
>>>>
>>>
>>
>>
>

Nick Baum (Google)

LGTM. On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com> wrote: > Also, ...

11 years, 2 months ago (2009-10-15 17:39:35 UTC) #19

LGTM.

On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com> wrote:

> Also, your OK button should have "default button" style (blue ring)
> and have focus.
>
> -Ben
>
> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com> wrote:
> > That's jpeg compression on the screenshot, not the check, right?
> >
> > If so, LGTM.
> >
> > -Ben
> >
> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org> wrote:
> >> drumroll please...
> >>
> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org> wrote:
> >>>
> >>> I added the app menu changes (this is toolbar_view.cc) from
> >>> http://crbug.com/24858.
> >>> I messaged Glen & Cole separately about designing me a big beautiful
> round
> >>> green checkmark for this bandwagon patch headed for Taco Town.
> >>>
> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrote:
> >>>>
> >>>> I will hunt for a green checkmark.
> >>>> Also, I uploaded a patchset with the last-minute NTP UI removal
> mentioned
> >>>> in http://crbug.com/24858.
> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com> wrote:
> >>>>>
> >>>>> I don't like them changing size... however...
> >>>>>
> >>>>> on the success page, do we have a large green circle with a checkmark
> >>>>> in it somewhere? Don't we use one of those in omaha? If so, I would
> >>>>> insert it centered above the "Synced to..." text.
> >>>>>
> >>>>> -Ben
> >>>>>
> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org>
> >>>>> wrote:
> >>>>> > I like the login and merge, but the success one makes me cringe :)
> >>>>> > Resizing is really hard?
> >>>>> > -Nick
> >>>>> >
> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org>
> wrote:
> >>>>> >>
> >>>>> >> The correct (same width) screenshots.
> >>>>> >>
> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org>
> wrote:
> >>>>> >>>
> >>>>> >>> Some new shots for the login dialog. Made it wider / slightly
> >>>>> >>> shorter,
> >>>>> >>> and top-aligned text.
> >>>>> >>>
> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <
> nickbaum@chromium.org>
> >>>>> >>> wrote:
> >>>>> >>>>
> >>>>> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
> >>>>> >>>> Can someone please just send mocks to Tim? He'll reply if it's
> not
> >>>>> >>>> doable.
> >>>>> >>>>
> >>>>> >>>> -Nick
> >>>>> >>>>
> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com>
> >>>>> >>>> wrote:
> >>>>> >>>>>
> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
> >>>>> >>>>>
> >>>>> >>>>> -Ben
> >>>>> >>>>>
> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <
> nickbaum@chromium.org>
> >>>>> >>>>> wrote:
> >>>>> >>>>> > +UI team
> >>>>> >>>>> > I really think it's weird to have all this extra spacing in
> the
> >>>>> >>>>> > dialogs and
> >>>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt to
> the
> >>>>> >>>>> > content?
> >>>>> >>>>> > -Nick
> >>>>> >>>>> >
> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <
> tim@chromium.org>
> >>>>> >>>>> > wrote:
> >>>>> >>>>> >>
> >>>>> >>>>> >> +cc the real nickbaum
> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <
> tim@chromium.org>
> >>>>> >>>>> >> wrote:
> >>>>> >>>>> >>>
> >>>>> >>>>> >>> +cc nickbaum
> >>>>> >>>>> >>> I had to up the size of the options dialog (in pixels, in
> >>>>> >>>>> >>> english,
> >>>>> >>>>> >>> it
> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
> >>>>> >>>>> >>> I attached a screenshot of what this means for the 'Basics'
> >>>>> >>>>> >>> pane.
> >>>>> >>>>> >>>  Looking for an OK.
> >>>>> >>>>> >>> Fixed button sizes.
> >>>>> >>>>> >>>
> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <
> tim@chromium.org>
> >>>>> >>>>> >>> wrote:
> >>>>> >>>>> >>>>
> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for
> >>>>> >>>>> >>>> thrashing
> >>>>> >>>>> >>>> on
> >>>>> >>>>> >>>> this.
> >>>>> >>>>> >>>>
> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
> >>>>> >>>>> >>>> <tim@chromium.org>
> >>>>> >>>>> >>>> wrote:
> >>>>> >>>>> >>>>>
> >>>>> >>>>> >>>>> Screenshots attached forevermore.  I made the width of
> the
> >>>>> >>>>> >>>>> buttons in
> >>>>> >>>>> >>>>> all our dialogs consistent with the min width in native
> ui.
> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now
> >>>>> >>>>> >>>>> throwing an
> >>>>> >>>>> >>>>> error,
> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
> >>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
> >>>>> >>>>> >>>>> I'm not sure why.  Looking into it.
> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure
> >>>>> >>>>> >>>>> what
> >>>>> >>>>> >>>>> I'm
> >>>>> >>>>> >>>>> supposed to do about all the translations.
> >>>>> >>>>> >>>>>
> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org>
> wrote:
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> fixed your points.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in the final
> >>>>> >>>>> >>>>>> wizard
> >>>>> >>>>> >>>>>> dialog
> >>>>> >>>>> >>>>>> is still
> >>>>> >>>>> >>>>>> not right, the button falls off the screen with one
> version
> >>>>> >>>>> >>>>>> of
> >>>>> >>>>> >>>>>> the
> >>>>> >>>>> >>>>>> text but not
> >>>>> >>>>> >>>>>> the other.  I will keep twiddling... I wish I was an
> Oscar
> >>>>> >>>>> >>>>>> Mayer
> >>>>> >>>>> >>>>>> CSS
> >>>>> >>>>> >>>>>> wizard.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/27#newcode5313
> >>>>> >>>>> >>>>>> Line 5313:
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Extra newline.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Done.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/27#newcode5331
> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs!
> To
> >>>>> >>>>> >>>>>> merge and
> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, I
> >>>>> >>>>> >>>>>>> think you
> >>>>> >>>>> >>>>>>> should
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> avoid
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> having the newline in the middle of this string.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Done.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
> >>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc
> (right):
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode344
> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Indentation is off.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Done.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode345
> >>>>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have
> >>>>> >>>>> >>>>>> forgotten
> >>>>> >>>>> >>>>>> to do
> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode396
> >>>>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> As above, this line is redundant.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Done.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
> >>>>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc
> >>>>> >>>>> >>>>>> (right):
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/40#newcode473
> >>>>> >>>>> >>>>>> Line 473:
> stop_sync_description_label_->SetVisible(false);
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
> >>>>> >>>>> >>>>>>> consistent
> >>>>> >>>>> >>>>>>> with the
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> IDS tag
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
> >>>>> >>>>> >>>>>
> >>>>> >>>>> >>>>
> >>>>> >>>>> >>>
> >>>>> >>>>> >>
> >>>>> >>>>> >
> >>>>> >>>>> >
> >>>>> >>>>
> >>>>> >>>
> >>>>> >>
> >>>>> >
> >>>>> >
> >>>>
> >>>
> >>
> >>
> >
>

tim (not reviewing)

Sorry about the jpg, yes, that is Paint's fault.As for the ring and focus... I ...

11 years, 2 months ago (2009-10-15 17:43:35 UTC) #20

Sorry about the jpg, yes, that is Paint's fault.As for the ring and focus...
I think I tried before and ran into some trouble.  If I can't do it in 10
minutes or so, I'd like to check this in and iterate.. it is already
changing ten thousand things.

What about the options dialog sizing?

On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com> wrote:

> Also, your OK button should have "default button" style (blue ring)
> and have focus.
>
> -Ben
>
> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com> wrote:
> > That's jpeg compression on the screenshot, not the check, right?
> >
> > If so, LGTM.
> >
> > -Ben
> >
> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org> wrote:
> >> drumroll please...
> >>
> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org> wrote:
> >>>
> >>> I added the app menu changes (this is toolbar_view.cc) from
> >>> http://crbug.com/24858.
> >>> I messaged Glen & Cole separately about designing me a big beautiful
> round
> >>> green checkmark for this bandwagon patch headed for Taco Town.
> >>>
> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrote:
> >>>>
> >>>> I will hunt for a green checkmark.
> >>>> Also, I uploaded a patchset with the last-minute NTP UI removal
> mentioned
> >>>> in http://crbug.com/24858.
> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com> wrote:
> >>>>>
> >>>>> I don't like them changing size... however...
> >>>>>
> >>>>> on the success page, do we have a large green circle with a checkmark
> >>>>> in it somewhere? Don't we use one of those in omaha? If so, I would
> >>>>> insert it centered above the "Synced to..." text.
> >>>>>
> >>>>> -Ben
> >>>>>
> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org>
> >>>>> wrote:
> >>>>> > I like the login and merge, but the success one makes me cringe :)
> >>>>> > Resizing is really hard?
> >>>>> > -Nick
> >>>>> >
> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org>
> wrote:
> >>>>> >>
> >>>>> >> The correct (same width) screenshots.
> >>>>> >>
> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org>
> wrote:
> >>>>> >>>
> >>>>> >>> Some new shots for the login dialog. Made it wider / slightly
> >>>>> >>> shorter,
> >>>>> >>> and top-aligned text.
> >>>>> >>>
> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <
> nickbaum@chromium.org>
> >>>>> >>> wrote:
> >>>>> >>>>
> >>>>> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
> >>>>> >>>> Can someone please just send mocks to Tim? He'll reply if it's
> not
> >>>>> >>>> doable.
> >>>>> >>>>
> >>>>> >>>> -Nick
> >>>>> >>>>
> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com>
> >>>>> >>>> wrote:
> >>>>> >>>>>
> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
> >>>>> >>>>>
> >>>>> >>>>> -Ben
> >>>>> >>>>>
> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <
> nickbaum@chromium.org>
> >>>>> >>>>> wrote:
> >>>>> >>>>> > +UI team
> >>>>> >>>>> > I really think it's weird to have all this extra spacing in
> the
> >>>>> >>>>> > dialogs and
> >>>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt to
> the
> >>>>> >>>>> > content?
> >>>>> >>>>> > -Nick
> >>>>> >>>>> >
> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <
> tim@chromium.org>
> >>>>> >>>>> > wrote:
> >>>>> >>>>> >>
> >>>>> >>>>> >> +cc the real nickbaum
> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <
> tim@chromium.org>
> >>>>> >>>>> >> wrote:
> >>>>> >>>>> >>>
> >>>>> >>>>> >>> +cc nickbaum
> >>>>> >>>>> >>> I had to up the size of the options dialog (in pixels, in
> >>>>> >>>>> >>> english,
> >>>>> >>>>> >>> it
> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
> >>>>> >>>>> >>> I attached a screenshot of what this means for the 'Basics'
> >>>>> >>>>> >>> pane.
> >>>>> >>>>> >>>  Looking for an OK.
> >>>>> >>>>> >>> Fixed button sizes.
> >>>>> >>>>> >>>
> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <
> tim@chromium.org>
> >>>>> >>>>> >>> wrote:
> >>>>> >>>>> >>>>
> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for
> >>>>> >>>>> >>>> thrashing
> >>>>> >>>>> >>>> on
> >>>>> >>>>> >>>> this.
> >>>>> >>>>> >>>>
> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
> >>>>> >>>>> >>>> <tim@chromium.org>
> >>>>> >>>>> >>>> wrote:
> >>>>> >>>>> >>>>>
> >>>>> >>>>> >>>>> Screenshots attached forevermore.  I made the width of
> the
> >>>>> >>>>> >>>>> buttons in
> >>>>> >>>>> >>>>> all our dialogs consistent with the min width in native
> ui.
> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now
> >>>>> >>>>> >>>>> throwing an
> >>>>> >>>>> >>>>> error,
> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
> >>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
> >>>>> >>>>> >>>>> I'm not sure why.  Looking into it.
> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not sure
> >>>>> >>>>> >>>>> what
> >>>>> >>>>> >>>>> I'm
> >>>>> >>>>> >>>>> supposed to do about all the translations.
> >>>>> >>>>> >>>>>
> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org>
> wrote:
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> fixed your points.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in the final
> >>>>> >>>>> >>>>>> wizard
> >>>>> >>>>> >>>>>> dialog
> >>>>> >>>>> >>>>>> is still
> >>>>> >>>>> >>>>>> not right, the button falls off the screen with one
> version
> >>>>> >>>>> >>>>>> of
> >>>>> >>>>> >>>>>> the
> >>>>> >>>>> >>>>>> text but not
> >>>>> >>>>> >>>>>> the other.  I will keep twiddling... I wish I was an
> Oscar
> >>>>> >>>>> >>>>>> Mayer
> >>>>> >>>>> >>>>>> CSS
> >>>>> >>>>> >>>>>> wizard.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/27#newcode5313
> >>>>> >>>>> >>>>>> Line 5313:
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Extra newline.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Done.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/27#newcode5331
> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google Docs!
> To
> >>>>> >>>>> >>>>>> merge and
> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, I
> >>>>> >>>>> >>>>>>> think you
> >>>>> >>>>> >>>>>>> should
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> avoid
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> having the newline in the middle of this string.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Done.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
> >>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc
> (right):
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode344
> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Indentation is off.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Done.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode345
> >>>>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have
> >>>>> >>>>> >>>>>> forgotten
> >>>>> >>>>> >>>>>> to do
> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode396
> >>>>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> As above, this line is redundant.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> Done.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
> >>>>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc
> >>>>> >>>>> >>>>>> (right):
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/40#newcode473
> >>>>> >>>>> >>>>>> Line 473:
> stop_sync_description_label_->SetVisible(false);
> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
> >>>>> >>>>> >>>>>>> consistent
> >>>>> >>>>> >>>>>>> with the
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> IDS tag
> >>>>> >>>>> >>>>>>>
> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
> >>>>> >>>>> >>>>>>
> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
> >>>>> >>>>> >>>>>
> >>>>> >>>>> >>>>
> >>>>> >>>>> >>>
> >>>>> >>>>> >>
> >>>>> >>>>> >
> >>>>> >>>>> >
> >>>>> >>>>
> >>>>> >>>
> >>>>> >>
> >>>>> >
> >>>>> >
> >>>>
> >>>
> >>
> >>
> >
>

ncarter (slow)

Just to double check: in addition to the changes to the wizard, the UI team ...

11 years, 2 months ago (2009-10-15 17:44:52 UTC) #21

Just to double check: in addition to the changes to the wizard, the UI team
is signing off on the wrench-menu options panel becoming taller to
accomodate the longer text that explains sync in the "Personal Stuff" tab?
 - nick

On Thu, Oct 15, 2009 at 10:38 AM, Nick Baum <nickbaum@chromium.org> wrote:

> LGTM.
>
>
> On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com> wrote:
>
>> Also, your OK button should have "default button" style (blue ring)
>> and have focus.
>>
>> -Ben
>>
>> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com> wrote:
>> > That's jpeg compression on the screenshot, not the check, right?
>> >
>> > If so, LGTM.
>> >
>> > -Ben
>> >
>> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org> wrote:
>> >> drumroll please...
>> >>
>> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org> wrote:
>> >>>
>> >>> I added the app menu changes (this is toolbar_view.cc) from
>> >>> http://crbug.com/24858.
>> >>> I messaged Glen & Cole separately about designing me a big beautiful
>> round
>> >>> green checkmark for this bandwagon patch headed for Taco Town.
>> >>>
>> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrote:
>> >>>>
>> >>>> I will hunt for a green checkmark.
>> >>>> Also, I uploaded a patchset with the last-minute NTP UI removal
>> mentioned
>> >>>> in http://crbug.com/24858.
>> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com>
>> wrote:
>> >>>>>
>> >>>>> I don't like them changing size... however...
>> >>>>>
>> >>>>> on the success page, do we have a large green circle with a
>> checkmark
>> >>>>> in it somewhere? Don't we use one of those in omaha? If so, I would
>> >>>>> insert it centered above the "Synced to..." text.
>> >>>>>
>> >>>>> -Ben
>> >>>>>
>> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org>
>> >>>>> wrote:
>> >>>>> > I like the login and merge, but the success one makes me cringe :)
>> >>>>> > Resizing is really hard?
>> >>>>> > -Nick
>> >>>>> >
>> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org>
>> wrote:
>> >>>>> >>
>> >>>>> >> The correct (same width) screenshots.
>> >>>>> >>
>> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org>
>> wrote:
>> >>>>> >>>
>> >>>>> >>> Some new shots for the login dialog. Made it wider / slightly
>> >>>>> >>> shorter,
>> >>>>> >>> and top-aligned text.
>> >>>>> >>>
>> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum <
>> nickbaum@chromium.org>
>> >>>>> >>> wrote:
>> >>>>> >>>>
>> >>>>> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
>> >>>>> >>>> Can someone please just send mocks to Tim? He'll reply if it's
>> not
>> >>>>> >>>> doable.
>> >>>>> >>>>
>> >>>>> >>>> -Nick
>> >>>>> >>>>
>> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com>
>> >>>>> >>>> wrote:
>> >>>>> >>>>>
>> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
>> >>>>> >>>>>
>> >>>>> >>>>> -Ben
>> >>>>> >>>>>
>> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum <
>> nickbaum@chromium.org>
>> >>>>> >>>>> wrote:
>> >>>>> >>>>> > +UI team
>> >>>>> >>>>> > I really think it's weird to have all this extra spacing in
>> the
>> >>>>> >>>>> > dialogs and
>> >>>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt to
>> the
>> >>>>> >>>>> > content?
>> >>>>> >>>>> > -Nick
>> >>>>> >>>>> >
>> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele <
>> tim@chromium.org>
>> >>>>> >>>>> > wrote:
>> >>>>> >>>>> >>
>> >>>>> >>>>> >> +cc the real nickbaum
>> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele <
>> tim@chromium.org>
>> >>>>> >>>>> >> wrote:
>> >>>>> >>>>> >>>
>> >>>>> >>>>> >>> +cc nickbaum
>> >>>>> >>>>> >>> I had to up the size of the options dialog (in pixels, in
>> >>>>> >>>>> >>> english,
>> >>>>> >>>>> >>> it
>> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
>> >>>>> >>>>> >>> I attached a screenshot of what this means for the
>> 'Basics'
>> >>>>> >>>>> >>> pane.
>> >>>>> >>>>> >>>  Looking for an OK.
>> >>>>> >>>>> >>> Fixed button sizes.
>> >>>>> >>>>> >>>
>> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele <
>> tim@chromium.org>
>> >>>>> >>>>> >>> wrote:
>> >>>>> >>>>> >>>>
>> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for
>> >>>>> >>>>> >>>> thrashing
>> >>>>> >>>>> >>>> on
>> >>>>> >>>>> >>>> this.
>> >>>>> >>>>> >>>>
>> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
>> >>>>> >>>>> >>>> <tim@chromium.org>
>> >>>>> >>>>> >>>> wrote:
>> >>>>> >>>>> >>>>>
>> >>>>> >>>>> >>>>> Screenshots attached forevermore.  I made the width of
>> the
>> >>>>> >>>>> >>>>> buttons in
>> >>>>> >>>>> >>>>> all our dialogs consistent with the min width in native
>> ui.
>> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now
>> >>>>> >>>>> >>>>> throwing an
>> >>>>> >>>>> >>>>> error,
>> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>> >>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>> >>>>> >>>>> >>>>> I'm not sure why.  Looking into it.
>> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not
>> sure
>> >>>>> >>>>> >>>>> what
>> >>>>> >>>>> >>>>> I'm
>> >>>>> >>>>> >>>>> supposed to do about all the translations.
>> >>>>> >>>>> >>>>>
>> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org>
>> wrote:
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> fixed your points.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in the final
>> >>>>> >>>>> >>>>>> wizard
>> >>>>> >>>>> >>>>>> dialog
>> >>>>> >>>>> >>>>>> is still
>> >>>>> >>>>> >>>>>> not right, the button falls off the screen with one
>> version
>> >>>>> >>>>> >>>>>> of
>> >>>>> >>>>> >>>>>> the
>> >>>>> >>>>> >>>>>> text but not
>> >>>>> >>>>> >>>>>> the other.  I will keep twiddling... I wish I was an
>> Oscar
>> >>>>> >>>>> >>>>>> Mayer
>> >>>>> >>>>> >>>>>> CSS
>> >>>>> >>>>> >>>>>> wizard.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> http://codereview.chromium.org/270081/diff/26/27#newcode5313
>> >>>>> >>>>> >>>>>> Line 5313:
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Extra newline.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Done.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> http://codereview.chromium.org/270081/diff/26/27#newcode5331
>> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google
>> Docs! To
>> >>>>> >>>>> >>>>>> merge and
>> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, I
>> >>>>> >>>>> >>>>>>> think you
>> >>>>> >>>>> >>>>>>> should
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> avoid
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> having the newline in the middle of this string.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Done.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>> >>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc
>> (right):
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> http://codereview.chromium.org/270081/diff/26/33#newcode344
>> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Indentation is off.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Done.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> http://codereview.chromium.org/270081/diff/26/33#newcode345
>> >>>>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must have
>> >>>>> >>>>> >>>>>> forgotten
>> >>>>> >>>>> >>>>>> to do
>> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> http://codereview.chromium.org/270081/diff/26/33#newcode396
>> >>>>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> As above, this line is redundant.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Done.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>> >>>>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.cc
>> >>>>> >>>>> >>>>>> (right):
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> http://codereview.chromium.org/270081/diff/26/40#newcode473
>> >>>>> >>>>> >>>>>> Line 473:
>> stop_sync_description_label_->SetVisible(false);
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
>> >>>>> >>>>> >>>>>>> consistent
>> >>>>> >>>>> >>>>>>> with the
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> IDS tag
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
>> >>>>> >>>>> >>>>>
>> >>>>> >>>>> >>>>
>> >>>>> >>>>> >>>
>> >>>>> >>>>> >>
>> >>>>> >>>>> >
>> >>>>> >>>>> >
>> >>>>> >>>>
>> >>>>> >>>
>> >>>>> >>
>> >>>>> >
>> >>>>> >
>> >>>>
>> >>>
>> >>
>> >>
>> >
>>
>
>

beng (no_code_reviews)

Sizing looks good to me. -Ben On Thu, Oct 15, 2009 at 10:43 AM, Tim ...

11 years, 2 months ago (2009-10-15 17:51:25 UTC) #22

Sizing looks good to me.

-Ben

On Thu, Oct 15, 2009 at 10:43 AM, Tim Steele <tim@chromium.org> wrote:
> Sorry about the jpg, yes, that is Paint's fault.
> As for the ring and focus... I think I tried before and ran into some
> trouble. =A0If I can't do it in 10 minutes or so, I'd like to check this =
in
> and iterate.. it is already changing ten thousand things.
> What about the options dialog sizing?
>
> On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com> wrote:
>>
>> Also, your OK button should have "default button" style (blue ring)
>> and have focus.
>>
>> -Ben
>>
>> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com> wrote:
>> > That's jpeg compression on the screenshot, not the check, right?
>> >
>> > If so, LGTM.
>> >
>> > -Ben
>> >
>> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org> wrote:
>> >> drumroll please...
>> >>
>> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org> wrote:
>> >>>
>> >>> I added the app menu changes (this is toolbar_view.cc) from
>> >>> http://crbug.com/24858.
>> >>> I messaged Glen & Cole separately about designing me a big beautiful
>> >>> round
>> >>> green checkmark for this bandwagon patch headed for Taco Town.
>> >>>
>> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrote=
:
>> >>>>
>> >>>> I will hunt for a green checkmark.
>> >>>> Also, I uploaded a patchset with the last-minute NTP UI removal
>> >>>> mentioned
>> >>>> in=A0http://crbug.com/24858.
>> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com> wrot=
e:
>> >>>>>
>> >>>>> I don't like them changing size... however...
>> >>>>>
>> >>>>> on the success page, do we have a large green circle with a
>> >>>>> checkmark
>> >>>>> in it somewhere? Don't we use one of those in omaha? If so, I woul=
d
>> >>>>> insert it centered above the "Synced to..." text.
>> >>>>>
>> >>>>> -Ben
>> >>>>>
>> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org>
>> >>>>> wrote:
>> >>>>> > I like the login and merge, but the success one makes me cringe =
:)
>> >>>>> > Resizing is really hard?
>> >>>>> > -Nick
>> >>>>> >
>> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org>
>> >>>>> > wrote:
>> >>>>> >>
>> >>>>> >> The correct (same width) screenshots.
>> >>>>> >>
>> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org>
>> >>>>> >> wrote:
>> >>>>> >>>
>> >>>>> >>> Some new shots for the login dialog. Made it wider / slightly
>> >>>>> >>> shorter,
>> >>>>> >>> and top-aligned text.
>> >>>>> >>>
>> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum
>> >>>>> >>> <nickbaum@chromium.org>
>> >>>>> >>> wrote:
>> >>>>> >>>>
>> >>>>> >>>> We need to nail this ASAP if we want it in the upcoming Beta.
>> >>>>> >>>> Can someone please just send mocks to Tim? He'll reply if it'=
s
>> >>>>> >>>> not
>> >>>>> >>>> doable.
>> >>>>> >>>>
>> >>>>> >>>> -Nick
>> >>>>> >>>>
>> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.com=
>
>> >>>>> >>>> wrote:
>> >>>>> >>>>>
>> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
>> >>>>> >>>>>
>> >>>>> >>>>> -Ben
>> >>>>> >>>>>
>> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum
>> >>>>> >>>>> <nickbaum@chromium.org>
>> >>>>> >>>>> wrote:
>> >>>>> >>>>> > +UI team
>> >>>>> >>>>> > I really think it's weird to have all this extra spacing i=
n
>> >>>>> >>>>> > the
>> >>>>> >>>>> > dialogs and
>> >>>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt =
to
>> >>>>> >>>>> > the
>> >>>>> >>>>> > content?
>> >>>>> >>>>> > -Nick
>> >>>>> >>>>> >
>> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele
>> >>>>> >>>>> > <tim@chromium.org>
>> >>>>> >>>>> > wrote:
>> >>>>> >>>>> >>
>> >>>>> >>>>> >> +cc the real nickbaum
>> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele
>> >>>>> >>>>> >> <tim@chromium.org>
>> >>>>> >>>>> >> wrote:
>> >>>>> >>>>> >>>
>> >>>>> >>>>> >>> +cc nickbaum
>> >>>>> >>>>> >>> I had to up the size of the options dialog (in pixels, i=
n
>> >>>>> >>>>> >>> english,
>> >>>>> >>>>> >>> it
>> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
>> >>>>> >>>>> >>> I attached a screenshot of what this means for the
>> >>>>> >>>>> >>> 'Basics'
>> >>>>> >>>>> >>> pane.
>> >>>>> >>>>> >>> =A0Looking for an OK.
>> >>>>> >>>>> >>> Fixed button sizes.
>> >>>>> >>>>> >>>
>> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele
>> >>>>> >>>>> >>> <tim@chromium.org>
>> >>>>> >>>>> >>> wrote:
>> >>>>> >>>>> >>>>
>> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :( =A0Sorry fo=
r
>> >>>>> >>>>> >>>> thrashing
>> >>>>> >>>>> >>>> on
>> >>>>> >>>>> >>>> this.
>> >>>>> >>>>> >>>>
>> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
>> >>>>> >>>>> >>>> <tim@chromium.org>
>> >>>>> >>>>> >>>> wrote:
>> >>>>> >>>>> >>>>>
>> >>>>> >>>>> >>>>> Screenshots attached forevermore. =A0I made the width =
of
>> >>>>> >>>>> >>>>> the
>> >>>>> >>>>> >>>>> buttons in
>> >>>>> >>>>> >>>>> all our dialogs consistent with the min width in nativ=
e
>> >>>>> >>>>> >>>>> ui.
>> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now
>> >>>>> >>>>> >>>>> throwing an
>> >>>>> >>>>> >>>>> error,
>> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>> >>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']".
>> >>>>> >>>>> >>>>> I'm not sure why. =A0Looking into it.
>> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not
>> >>>>> >>>>> >>>>> sure
>> >>>>> >>>>> >>>>> what
>> >>>>> >>>>> >>>>> I'm
>> >>>>> >>>>> >>>>> supposed to do about all the translations.
>> >>>>> >>>>> >>>>>
>> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org>
>> >>>>> >>>>> >>>>> wrote:
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> fixed your points.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in the fina=
l
>> >>>>> >>>>> >>>>>> wizard
>> >>>>> >>>>> >>>>>> dialog
>> >>>>> >>>>> >>>>>> is still
>> >>>>> >>>>> >>>>>> not right, the button falls off the screen with one
>> >>>>> >>>>> >>>>>> version
>> >>>>> >>>>> >>>>>> of
>> >>>>> >>>>> >>>>>> the
>> >>>>> >>>>> >>>>>> text but not
>> >>>>> >>>>> >>>>>> the other. =A0I will keep twiddling... I wish I was a=
n
>> >>>>> >>>>> >>>>>> Oscar
>> >>>>> >>>>> >>>>>> Mayer
>> >>>>> >>>>> >>>>>> CSS
>> >>>>> >>>>> >>>>>> wizard.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newc=
ode5313
>> >>>>> >>>>> >>>>>> Line 5313:
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Extra newline.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Done.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#newc=
ode5331
>> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google
>> >>>>> >>>>> >>>>>> Docs! To
>> >>>>> >>>>> >>>>>> merge and
>> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file, =
I
>> >>>>> >>>>> >>>>>>> think you
>> >>>>> >>>>> >>>>>>> should
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> avoid
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> having the newline in the middle of this string.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Done.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>> >>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc
>> >>>>> >>>>> >>>>>> (right):
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newc=
ode344
>> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Indentation is off.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Done.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newc=
ode345
>> >>>>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Ah! Done. =A0I just wanted to remove the TODO, must h=
ave
>> >>>>> >>>>> >>>>>> forgotten
>> >>>>> >>>>> >>>>>> to do
>> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#newc=
ode396
>> >>>>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> As above, this line is redundant.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> Done.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>> >>>>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.c=
c
>> >>>>> >>>>> >>>>>> (right):
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#newc=
ode473
>> >>>>> >>>>> >>>>>> Line 473:
>> >>>>> >>>>> >>>>>> stop_sync_description_label_->SetVisible(false);
>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
>> >>>>> >>>>> >>>>>>> consistent
>> >>>>> >>>>> >>>>>>> with the
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> IDS tag
>> >>>>> >>>>> >>>>>>>
>> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
>> >>>>> >>>>> >>>>>>
>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
>> >>>>> >>>>> >>>>>
>> >>>>> >>>>> >>>>
>> >>>>> >>>>> >>>
>> >>>>> >>>>> >>
>> >>>>> >>>>> >
>> >>>>> >>>>> >
>> >>>>> >>>>
>> >>>>> >>>
>> >>>>> >>
>> >>>>> >
>> >>>>> >
>> >>>>
>> >>>
>> >>
>> >>
>> >
>
>

beng (no_code_reviews)

Hrmp. I think that'll actually cause us trouble on smaller screen heights. This dialog box ...

11 years, 2 months ago (2009-10-15 17:52:40 UTC) #23

Hrmp. I think that'll actually cause us trouble on smaller screen
heights. This dialog box is actually already too tall. Can you instead
have a confirmation dialog that says this info instead?

<text>

( Stop Syncing ) ( Cancel ) <-- default

-Ben

On Thu, Oct 15, 2009 at 10:44 AM, Nick Carter <nick@chromium.org> wrote:
> Just to double check: in addition to the changes to the wizard, the UI te=
am
> is signing off on the wrench-menu options panel becoming taller to
> accomodate the longer text that explains sync in the "Personal Stuff" tab=
?
> =A0- nick
>
> On Thu, Oct 15, 2009 at 10:38 AM, Nick Baum <nickbaum@chromium.org> wrote=
:
>>
>> LGTM.
>>
>> On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com> wrote:
>>>
>>> Also, your OK button should have "default button" style (blue ring)
>>> and have focus.
>>>
>>> -Ben
>>>
>>> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com> wrote:
>>> > That's jpeg compression on the screenshot, not the check, right?
>>> >
>>> > If so, LGTM.
>>> >
>>> > -Ben
>>> >
>>> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org> wrote=
:
>>> >> drumroll please...
>>> >>
>>> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org> wrote=
:
>>> >>>
>>> >>> I added the app menu changes (this is toolbar_view.cc) from
>>> >>> http://crbug.com/24858.
>>> >>> I messaged Glen & Cole separately about designing me a big beautifu=
l
>>> >>> round
>>> >>> green checkmark for this bandwagon patch headed for Taco Town.
>>> >>>
>>> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org> wrot=
e:
>>> >>>>
>>> >>>> I will hunt for a green checkmark.
>>> >>>> Also, I uploaded a patchset with the last-minute NTP UI removal
>>> >>>> mentioned
>>> >>>> in=A0http://crbug.com/24858.
>>> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com>
>>> >>>> wrote:
>>> >>>>>
>>> >>>>> I don't like them changing size... however...
>>> >>>>>
>>> >>>>> on the success page, do we have a large green circle with a
>>> >>>>> checkmark
>>> >>>>> in it somewhere? Don't we use one of those in omaha? If so, I wou=
ld
>>> >>>>> insert it centered above the "Synced to..." text.
>>> >>>>>
>>> >>>>> -Ben
>>> >>>>>
>>> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <nickbaum@chromium.org=
>
>>> >>>>> wrote:
>>> >>>>> > I like the login and merge, but the success one makes me cringe
>>> >>>>> > :)
>>> >>>>> > Resizing is really hard?
>>> >>>>> > -Nick
>>> >>>>> >
>>> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org>
>>> >>>>> > wrote:
>>> >>>>> >>
>>> >>>>> >> The correct (same width) screenshots.
>>> >>>>> >>
>>> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org>
>>> >>>>> >> wrote:
>>> >>>>> >>>
>>> >>>>> >>> Some new shots for the login dialog. Made it wider / slightly
>>> >>>>> >>> shorter,
>>> >>>>> >>> and top-aligned text.
>>> >>>>> >>>
>>> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum
>>> >>>>> >>> <nickbaum@chromium.org>
>>> >>>>> >>> wrote:
>>> >>>>> >>>>
>>> >>>>> >>>> We need to nail this ASAP if we want it in the upcoming Beta=
.
>>> >>>>> >>>> Can someone please just send mocks to Tim? He'll reply if it=
's
>>> >>>>> >>>> not
>>> >>>>> >>>> doable.
>>> >>>>> >>>>
>>> >>>>> >>>> -Nick
>>> >>>>> >>>>
>>> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <beng@google.co=
m>
>>> >>>>> >>>> wrote:
>>> >>>>> >>>>>
>>> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
>>> >>>>> >>>>>
>>> >>>>> >>>>> -Ben
>>> >>>>> >>>>>
>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum
>>> >>>>> >>>>> <nickbaum@chromium.org>
>>> >>>>> >>>>> wrote:
>>> >>>>> >>>>> > +UI team
>>> >>>>> >>>>> > I really think it's weird to have all this extra spacing =
in
>>> >>>>> >>>>> > the
>>> >>>>> >>>>> > dialogs and
>>> >>>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt
>>> >>>>> >>>>> > to the
>>> >>>>> >>>>> > content?
>>> >>>>> >>>>> > -Nick
>>> >>>>> >>>>> >
>>> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele
>>> >>>>> >>>>> > <tim@chromium.org>
>>> >>>>> >>>>> > wrote:
>>> >>>>> >>>>> >>
>>> >>>>> >>>>> >> +cc the real nickbaum
>>> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele
>>> >>>>> >>>>> >> <tim@chromium.org>
>>> >>>>> >>>>> >> wrote:
>>> >>>>> >>>>> >>>
>>> >>>>> >>>>> >>> +cc nickbaum
>>> >>>>> >>>>> >>> I had to up the size of the options dialog (in pixels, =
in
>>> >>>>> >>>>> >>> english,
>>> >>>>> >>>>> >>> it
>>> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
>>> >>>>> >>>>> >>> I attached a screenshot of what this means for the
>>> >>>>> >>>>> >>> 'Basics'
>>> >>>>> >>>>> >>> pane.
>>> >>>>> >>>>> >>> =A0Looking for an OK.
>>> >>>>> >>>>> >>> Fixed button sizes.
>>> >>>>> >>>>> >>>
>>> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele
>>> >>>>> >>>>> >>> <tim@chromium.org>
>>> >>>>> >>>>> >>> wrote:
>>> >>>>> >>>>> >>>>
>>> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :( =A0Sorry f=
or
>>> >>>>> >>>>> >>>> thrashing
>>> >>>>> >>>>> >>>> on
>>> >>>>> >>>>> >>>> this.
>>> >>>>> >>>>> >>>>
>>> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
>>> >>>>> >>>>> >>>> <tim@chromium.org>
>>> >>>>> >>>>> >>>> wrote:
>>> >>>>> >>>>> >>>>>
>>> >>>>> >>>>> >>>>> Screenshots attached forevermore. =A0I made the width=
 of
>>> >>>>> >>>>> >>>>> the
>>> >>>>> >>>>> >>>>> buttons in
>>> >>>>> >>>>> >>>>> all our dialogs consistent with the min width in nati=
ve
>>> >>>>> >>>>> >>>>> ui.
>>> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now
>>> >>>>> >>>>> >>>>> throwing an
>>> >>>>> >>>>> >>>>> error,
>>> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>>> >>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html']"=
.
>>> >>>>> >>>>> >>>>> I'm not sure why. =A0Looking into it.
>>> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not
>>> >>>>> >>>>> >>>>> sure
>>> >>>>> >>>>> >>>>> what
>>> >>>>> >>>>> >>>>> I'm
>>> >>>>> >>>>> >>>>> supposed to do about all the translations.
>>> >>>>> >>>>> >>>>>
>>> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org>
>>> >>>>> >>>>> >>>>> wrote:
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> fixed your points.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in the fin=
al
>>> >>>>> >>>>> >>>>>> wizard
>>> >>>>> >>>>> >>>>>> dialog
>>> >>>>> >>>>> >>>>>> is still
>>> >>>>> >>>>> >>>>>> not right, the button falls off the screen with one
>>> >>>>> >>>>> >>>>>> version
>>> >>>>> >>>>> >>>>>> of
>>> >>>>> >>>>> >>>>>> the
>>> >>>>> >>>>> >>>>>> text but not
>>> >>>>> >>>>> >>>>>> the other. =A0I will keep twiddling... I wish I was =
an
>>> >>>>> >>>>> >>>>>> Oscar
>>> >>>>> >>>>> >>>>>> Mayer
>>> >>>>> >>>>> >>>>>> CSS
>>> >>>>> >>>>> >>>>>> wizard.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>>> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#new=
code5313
>>> >>>>> >>>>> >>>>>> Line 5313:
>>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>> >>>>>>>
>>> >>>>> >>>>> >>>>>>> Extra newline.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> Done.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#new=
code5331
>>> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google
>>> >>>>> >>>>> >>>>>> Docs! To
>>> >>>>> >>>>> >>>>>> merge and
>>> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
>>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>> >>>>>>>
>>> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file,=
 I
>>> >>>>> >>>>> >>>>>>> think you
>>> >>>>> >>>>> >>>>>>> should
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> avoid
>>> >>>>> >>>>> >>>>>>>
>>> >>>>> >>>>> >>>>>>> having the newline in the middle of this string.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> Done.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>>> >>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc
>>> >>>>> >>>>> >>>>>> (right):
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#new=
code344
>>> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
>>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>> >>>>>>>
>>> >>>>> >>>>> >>>>>>> Indentation is off.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> Done.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#new=
code345
>>> >>>>> >>>>> >>>>>> Line 345: wizard_.Step(SyncSetupWizard::FATAL_ERROR)=
;
>>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>> >>>>>>>
>>> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> Ah! Done. =A0I just wanted to remove the TODO, must =
have
>>> >>>>> >>>>> >>>>>> forgotten
>>> >>>>> >>>>> >>>>>> to do
>>> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#new=
code396
>>> >>>>> >>>>> >>>>>> Line 396: wizard_.Step(SyncSetupWizard::FATAL_ERROR)=
;
>>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>> >>>>>>>
>>> >>>>> >>>>> >>>>>>> As above, this line is redundant.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> Done.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>>> >>>>> >>>>> >>>>>> File chrome/browser/views/options/content_page_view.=
cc
>>> >>>>> >>>>> >>>>>> (right):
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#new=
code473
>>> >>>>> >>>>> >>>>>> Line 473:
>>> >>>>> >>>>> >>>>>> stop_sync_description_label_->SetVisible(false);
>>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >>>>> >>>>> >>>>>>>
>>> >>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
>>> >>>>> >>>>> >>>>>>> consistent
>>> >>>>> >>>>> >>>>>>> with the
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> IDS tag
>>> >>>>> >>>>> >>>>>>>
>>> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
>>> >>>>> >>>>> >>>>>>
>>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
>>> >>>>> >>>>> >>>>>
>>> >>>>> >>>>> >>>>
>>> >>>>> >>>>> >>>
>>> >>>>> >>>>> >>
>>> >>>>> >>>>> >
>>> >>>>> >>>>> >
>>> >>>>> >>>>
>>> >>>>> >>>
>>> >>>>> >>
>>> >>>>> >
>>> >>>>> >
>>> >>>>
>>> >>>
>>> >>
>>> >>
>>> >
>>
>
>

tim (not reviewing)

This is what I was first planning but putting the text inline was suggested as ...

11 years, 2 months ago (2009-10-15 17:59:02 UTC) #24

This is what I was first planning but putting the text inline was suggested
as it was less obstructing.

On Thu, Oct 15, 2009 at 10:52 AM, Ben Goodger <beng@google.com> wrote:

> Hrmp. I think that'll actually cause us trouble on smaller screen
> heights. This dialog box is actually already too tall. Can you instead
> have a confirmation dialog that says this info instead?
>
> <text>
>
> ( Stop Syncing ) ( Cancel ) <-- default
>
> -Ben
>
> On Thu, Oct 15, 2009 at 10:44 AM, Nick Carter <nick@chromium.org> wrote:
> > Just to double check: in addition to the changes to the wizard, the UI
> team
> > is signing off on the wrench-menu options panel becoming taller to
> > accomodate the longer text that explains sync in the "Personal Stuff"
> tab?
> >  - nick
> >
> > On Thu, Oct 15, 2009 at 10:38 AM, Nick Baum <nickbaum@chromium.org>
> wrote:
> >>
> >> LGTM.
> >>
> >> On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com> wrote:
> >>>
> >>> Also, your OK button should have "default button" style (blue ring)
> >>> and have focus.
> >>>
> >>> -Ben
> >>>
> >>> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com> wrote:
> >>> > That's jpeg compression on the screenshot, not the check, right?
> >>> >
> >>> > If so, LGTM.
> >>> >
> >>> > -Ben
> >>> >
> >>> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org>
> wrote:
> >>> >> drumroll please...
> >>> >>
> >>> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org>
> wrote:
> >>> >>>
> >>> >>> I added the app menu changes (this is toolbar_view.cc) from
> >>> >>> http://crbug.com/24858.
> >>> >>> I messaged Glen & Cole separately about designing me a big
> beautiful
> >>> >>> round
> >>> >>> green checkmark for this bandwagon patch headed for Taco Town.
> >>> >>>
> >>> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org>
> wrote:
> >>> >>>>
> >>> >>>> I will hunt for a green checkmark.
> >>> >>>> Also, I uploaded a patchset with the last-minute NTP UI removal
> >>> >>>> mentioned
> >>> >>>> in http://crbug.com/24858.
> >>> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com>
> >>> >>>> wrote:
> >>> >>>>>
> >>> >>>>> I don't like them changing size... however...
> >>> >>>>>
> >>> >>>>> on the success page, do we have a large green circle with a
> >>> >>>>> checkmark
> >>> >>>>> in it somewhere? Don't we use one of those in omaha? If so, I
> would
> >>> >>>>> insert it centered above the "Synced to..." text.
> >>> >>>>>
> >>> >>>>> -Ben
> >>> >>>>>
> >>> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum <
> nickbaum@chromium.org>
> >>> >>>>> wrote:
> >>> >>>>> > I like the login and merge, but the success one makes me cringe
> >>> >>>>> > :)
> >>> >>>>> > Resizing is really hard?
> >>> >>>>> > -Nick
> >>> >>>>> >
> >>> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.org>
> >>> >>>>> > wrote:
> >>> >>>>> >>
> >>> >>>>> >> The correct (same width) screenshots.
> >>> >>>>> >>
> >>> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele <tim@chromium.org
> >
> >>> >>>>> >> wrote:
> >>> >>>>> >>>
> >>> >>>>> >>> Some new shots for the login dialog. Made it wider / slightly
> >>> >>>>> >>> shorter,
> >>> >>>>> >>> and top-aligned text.
> >>> >>>>> >>>
> >>> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum
> >>> >>>>> >>> <nickbaum@chromium.org>
> >>> >>>>> >>> wrote:
> >>> >>>>> >>>>
> >>> >>>>> >>>> We need to nail this ASAP if we want it in the upcoming
> Beta.
> >>> >>>>> >>>> Can someone please just send mocks to Tim? He'll reply if
> it's
> >>> >>>>> >>>> not
> >>> >>>>> >>>> doable.
> >>> >>>>> >>>>
> >>> >>>>> >>>> -Nick
> >>> >>>>> >>>>
> >>> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger <
> beng@google.com>
> >>> >>>>> >>>> wrote:
> >>> >>>>> >>>>>
> >>> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
> >>> >>>>> >>>>>
> >>> >>>>> >>>>> -Ben
> >>> >>>>> >>>>>
> >>> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum
> >>> >>>>> >>>>> <nickbaum@chromium.org>
> >>> >>>>> >>>>> wrote:
> >>> >>>>> >>>>> > +UI team
> >>> >>>>> >>>>> > I really think it's weird to have all this extra spacing
> in
> >>> >>>>> >>>>> > the
> >>> >>>>> >>>>> > dialogs and
> >>> >>>>> >>>>> > the pref pane. Can't we resize these dynamically to adapt
> >>> >>>>> >>>>> > to the
> >>> >>>>> >>>>> > content?
> >>> >>>>> >>>>> > -Nick
> >>> >>>>> >>>>> >
> >>> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele
> >>> >>>>> >>>>> > <tim@chromium.org>
> >>> >>>>> >>>>> > wrote:
> >>> >>>>> >>>>> >>
> >>> >>>>> >>>>> >> +cc the real nickbaum
> >>> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele
> >>> >>>>> >>>>> >> <tim@chromium.org>
> >>> >>>>> >>>>> >> wrote:
> >>> >>>>> >>>>> >>>
> >>> >>>>> >>>>> >>> +cc nickbaum
> >>> >>>>> >>>>> >>> I had to up the size of the options dialog (in pixels,
> in
> >>> >>>>> >>>>> >>> english,
> >>> >>>>> >>>>> >>> it
> >>> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
> >>> >>>>> >>>>> >>> I attached a screenshot of what this means for the
> >>> >>>>> >>>>> >>> 'Basics'
> >>> >>>>> >>>>> >>> pane.
> >>> >>>>> >>>>> >>>  Looking for an OK.
> >>> >>>>> >>>>> >>> Fixed button sizes.
> >>> >>>>> >>>>> >>>
> >>> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele
> >>> >>>>> >>>>> >>> <tim@chromium.org>
> >>> >>>>> >>>>> >>> wrote:
> >>> >>>>> >>>>> >>>>
> >>> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry for
> >>> >>>>> >>>>> >>>> thrashing
> >>> >>>>> >>>>> >>>> on
> >>> >>>>> >>>>> >>>> this.
> >>> >>>>> >>>>> >>>>
> >>> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
> >>> >>>>> >>>>> >>>> <tim@chromium.org>
> >>> >>>>> >>>>> >>>> wrote:
> >>> >>>>> >>>>> >>>>>
> >>> >>>>> >>>>> >>>>> Screenshots attached forevermore.  I made the width
> of
> >>> >>>>> >>>>> >>>>> the
> >>> >>>>> >>>>> >>>>> buttons in
> >>> >>>>> >>>>> >>>>> all our dialogs consistent with the min width in
> native
> >>> >>>>> >>>>> >>>>> ui.
> >>> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is now
> >>> >>>>> >>>>> >>>>> throwing an
> >>> >>>>> >>>>> >>>>> error,
> >>> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
> >>> >>>>> >>>>> >>>>>
> 'chrome/browser/sync/resources/merge_and_sync.html']".
> >>> >>>>> >>>>> >>>>> I'm not sure why.  Looking into it.
> >>> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm not
> >>> >>>>> >>>>> >>>>> sure
> >>> >>>>> >>>>> >>>>> what
> >>> >>>>> >>>>> >>>>> I'm
> >>> >>>>> >>>>> >>>>> supposed to do about all the translations.
> >>> >>>>> >>>>> >>>>>
> >>> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org>
> >>> >>>>> >>>>> >>>>> wrote:
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> fixed your points.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in the
> final
> >>> >>>>> >>>>> >>>>>> wizard
> >>> >>>>> >>>>> >>>>>> dialog
> >>> >>>>> >>>>> >>>>>> is still
> >>> >>>>> >>>>> >>>>>> not right, the button falls off the screen with one
> >>> >>>>> >>>>> >>>>>> version
> >>> >>>>> >>>>> >>>>>> of
> >>> >>>>> >>>>> >>>>>> the
> >>> >>>>> >>>>> >>>>>> text but not
> >>> >>>>> >>>>> >>>>>> the other.  I will keep twiddling... I wish I was an
> >>> >>>>> >>>>> >>>>>> Oscar
> >>> >>>>> >>>>> >>>>>> Mayer
> >>> >>>>> >>>>> >>>>>> CSS
> >>> >>>>> >>>>> >>>>>> wizard.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
> >>> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/27#newcode5313
> >>> >>>>> >>>>> >>>>>> Line 5313:
> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>> >>>>> >>>>> >>>>>>>
> >>> >>>>> >>>>> >>>>>>> Extra newline.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> Done.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/27#newcode5331
> >>> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Google
> >>> >>>>> >>>>> >>>>>> Docs! To
> >>> >>>>> >>>>> >>>>>> merge and
> >>> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>> >>>>> >>>>> >>>>>>>
> >>> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this file,
> I
> >>> >>>>> >>>>> >>>>>>> think you
> >>> >>>>> >>>>> >>>>>>> should
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> avoid
> >>> >>>>> >>>>> >>>>>>>
> >>> >>>>> >>>>> >>>>>>> having the newline in the middle of this string.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> Done.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
> >>> >>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc
> >>> >>>>> >>>>> >>>>>> (right):
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode344
> >>> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.";
> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>> >>>>> >>>>> >>>>>>>
> >>> >>>>> >>>>> >>>>>>> Indentation is off.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> Done.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode345
> >>> >>>>> >>>>> >>>>>> Line 345:
> wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>> >>>>> >>>>> >>>>>>>
> >>> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it too.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must
> have
> >>> >>>>> >>>>> >>>>>> forgotten
> >>> >>>>> >>>>> >>>>>> to do
> >>> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode396
> >>> >>>>> >>>>> >>>>>> Line 396:
> wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>> >>>>> >>>>> >>>>>>>
> >>> >>>>> >>>>> >>>>>>> As above, this line is redundant.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> Done.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
> >>> >>>>> >>>>> >>>>>> File
> chrome/browser/views/options/content_page_view.cc
> >>> >>>>> >>>>> >>>>>> (right):
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/40#newcode473
> >>> >>>>> >>>>> >>>>>> Line 473:
> >>> >>>>> >>>>> >>>>>> stop_sync_description_label_->SetVisible(false);
> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >>> >>>>> >>>>> >>>>>>>
> >>> >>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
> >>> >>>>> >>>>> >>>>>>> consistent
> >>> >>>>> >>>>> >>>>>>> with the
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> IDS tag
> >>> >>>>> >>>>> >>>>>>>
> >>> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
> >>> >>>>> >>>>> >>>>>>
> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
> >>> >>>>> >>>>> >>>>>
> >>> >>>>> >>>>> >>>>
> >>> >>>>> >>>>> >>>
> >>> >>>>> >>>>> >>
> >>> >>>>> >>>>> >
> >>> >>>>> >>>>> >
> >>> >>>>> >>>>
> >>> >>>>> >>>
> >>> >>>>> >>
> >>> >>>>> >
> >>> >>>>> >
> >>> >>>>
> >>> >>>
> >>> >>
> >>> >>
> >>> >
> >>
> >
> >
>

beng (no_code_reviews)

I don't think stopping syncing is a common action so it doesn't bother me that ...

11 years, 2 months ago (2009-10-15 17:59:40 UTC) #25

I don't think stopping syncing is a common action so it doesn't bother
me that it's more cumbersome.

We can't make the dialog any taller.

-Ben

On Thu, Oct 15, 2009 at 10:58 AM, Tim Steele <tim@chromium.org> wrote:
> This is what I was first planning but putting the text inline was suggest=
ed
> as it was less obstructing.
>
> On Thu, Oct 15, 2009 at 10:52 AM, Ben Goodger <beng@google.com> wrote:
>>
>> Hrmp. I think that'll actually cause us trouble on smaller screen
>> heights. This dialog box is actually already too tall. Can you instead
>> have a confirmation dialog that says this info instead?
>>
>> <text>
>>
>> ( Stop Syncing ) ( Cancel ) <-- default
>>
>> -Ben
>>
>> On Thu, Oct 15, 2009 at 10:44 AM, Nick Carter <nick@chromium.org> wrote:
>> > Just to double check: in addition to the changes to the wizard, the UI
>> > team
>> > is signing off on the wrench-menu options panel becoming taller to
>> > accomodate the longer text that explains sync in the "Personal Stuff"
>> > tab?
>> > =A0- nick
>> >
>> > On Thu, Oct 15, 2009 at 10:38 AM, Nick Baum <nickbaum@chromium.org>
>> > wrote:
>> >>
>> >> LGTM.
>> >>
>> >> On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com> wrote=
:
>> >>>
>> >>> Also, your OK button should have "default button" style (blue ring)
>> >>> and have focus.
>> >>>
>> >>> -Ben
>> >>>
>> >>> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com> wrot=
e:
>> >>> > That's jpeg compression on the screenshot, not the check, right?
>> >>> >
>> >>> > If so, LGTM.
>> >>> >
>> >>> > -Ben
>> >>> >
>> >>> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org>
>> >>> > wrote:
>> >>> >> drumroll please...
>> >>> >>
>> >>> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org>
>> >>> >> wrote:
>> >>> >>>
>> >>> >>> I added the app menu changes (this is toolbar_view.cc) from
>> >>> >>> http://crbug.com/24858.
>> >>> >>> I messaged Glen & Cole separately about designing me a big
>> >>> >>> beautiful
>> >>> >>> round
>> >>> >>> green checkmark for this bandwagon patch headed for Taco Town.
>> >>> >>>
>> >>> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org>
>> >>> >>> wrote:
>> >>> >>>>
>> >>> >>>> I will hunt for a green checkmark.
>> >>> >>>> Also, I uploaded a patchset with the last-minute NTP UI removal
>> >>> >>>> mentioned
>> >>> >>>> in=A0http://crbug.com/24858.
>> >>> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com>
>> >>> >>>> wrote:
>> >>> >>>>>
>> >>> >>>>> I don't like them changing size... however...
>> >>> >>>>>
>> >>> >>>>> on the success page, do we have a large green circle with a
>> >>> >>>>> checkmark
>> >>> >>>>> in it somewhere? Don't we use one of those in omaha? If so, I
>> >>> >>>>> would
>> >>> >>>>> insert it centered above the "Synced to..." text.
>> >>> >>>>>
>> >>> >>>>> -Ben
>> >>> >>>>>
>> >>> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum
>> >>> >>>>> <nickbaum@chromium.org>
>> >>> >>>>> wrote:
>> >>> >>>>> > I like the login and merge, but the success one makes me
>> >>> >>>>> > cringe
>> >>> >>>>> > :)
>> >>> >>>>> > Resizing is really hard?
>> >>> >>>>> > -Nick
>> >>> >>>>> >
>> >>> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <tim@chromium.or=
g>
>> >>> >>>>> > wrote:
>> >>> >>>>> >>
>> >>> >>>>> >> The correct (same width) screenshots.
>> >>> >>>>> >>
>> >>> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele
>> >>> >>>>> >> <tim@chromium.org>
>> >>> >>>>> >> wrote:
>> >>> >>>>> >>>
>> >>> >>>>> >>> Some new shots for the login dialog. Made it wider /
>> >>> >>>>> >>> slightly
>> >>> >>>>> >>> shorter,
>> >>> >>>>> >>> and top-aligned text.
>> >>> >>>>> >>>
>> >>> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum
>> >>> >>>>> >>> <nickbaum@chromium.org>
>> >>> >>>>> >>> wrote:
>> >>> >>>>> >>>>
>> >>> >>>>> >>>> We need to nail this ASAP if we want it in the upcoming
>> >>> >>>>> >>>> Beta.
>> >>> >>>>> >>>> Can someone please just send mocks to Tim? He'll reply if
>> >>> >>>>> >>>> it's
>> >>> >>>>> >>>> not
>> >>> >>>>> >>>> doable.
>> >>> >>>>> >>>>
>> >>> >>>>> >>>> -Nick
>> >>> >>>>> >>>>
>> >>> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger
>> >>> >>>>> >>>> <beng@google.com>
>> >>> >>>>> >>>> wrote:
>> >>> >>>>> >>>>>
>> >>> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
>> >>> >>>>> >>>>>
>> >>> >>>>> >>>>> -Ben
>> >>> >>>>> >>>>>
>> >>> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum
>> >>> >>>>> >>>>> <nickbaum@chromium.org>
>> >>> >>>>> >>>>> wrote:
>> >>> >>>>> >>>>> > +UI team
>> >>> >>>>> >>>>> > I really think it's weird to have all this extra spaci=
ng
>> >>> >>>>> >>>>> > in
>> >>> >>>>> >>>>> > the
>> >>> >>>>> >>>>> > dialogs and
>> >>> >>>>> >>>>> > the pref pane. Can't we resize these dynamically to
>> >>> >>>>> >>>>> > adapt
>> >>> >>>>> >>>>> > to the
>> >>> >>>>> >>>>> > content?
>> >>> >>>>> >>>>> > -Nick
>> >>> >>>>> >>>>> >
>> >>> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele
>> >>> >>>>> >>>>> > <tim@chromium.org>
>> >>> >>>>> >>>>> > wrote:
>> >>> >>>>> >>>>> >>
>> >>> >>>>> >>>>> >> +cc the real nickbaum
>> >>> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele
>> >>> >>>>> >>>>> >> <tim@chromium.org>
>> >>> >>>>> >>>>> >> wrote:
>> >>> >>>>> >>>>> >>>
>> >>> >>>>> >>>>> >>> +cc nickbaum
>> >>> >>>>> >>>>> >>> I had to up the size of the options dialog (in pixel=
s,
>> >>> >>>>> >>>>> >>> in
>> >>> >>>>> >>>>> >>> english,
>> >>> >>>>> >>>>> >>> it
>> >>> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
>> >>> >>>>> >>>>> >>> I attached a screenshot of what this means for the
>> >>> >>>>> >>>>> >>> 'Basics'
>> >>> >>>>> >>>>> >>> pane.
>> >>> >>>>> >>>>> >>> =A0Looking for an OK.
>> >>> >>>>> >>>>> >>> Fixed button sizes.
>> >>> >>>>> >>>>> >>>
>> >>> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele
>> >>> >>>>> >>>>> >>> <tim@chromium.org>
>> >>> >>>>> >>>>> >>> wrote:
>> >>> >>>>> >>>>> >>>>
>> >>> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :( =A0Sorr=
y
>> >>> >>>>> >>>>> >>>> for
>> >>> >>>>> >>>>> >>>> thrashing
>> >>> >>>>> >>>>> >>>> on
>> >>> >>>>> >>>>> >>>> this.
>> >>> >>>>> >>>>> >>>>
>> >>> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
>> >>> >>>>> >>>>> >>>> <tim@chromium.org>
>> >>> >>>>> >>>>> >>>> wrote:
>> >>> >>>>> >>>>> >>>>>
>> >>> >>>>> >>>>> >>>>> Screenshots attached forevermore. =A0I made the wi=
dth
>> >>> >>>>> >>>>> >>>>> of
>> >>> >>>>> >>>>> >>>>> the
>> >>> >>>>> >>>>> >>>>> buttons in
>> >>> >>>>> >>>>> >>>>> all our dialogs consistent with the min width in
>> >>> >>>>> >>>>> >>>>> native
>> >>> >>>>> >>>>> >>>>> ui.
>> >>> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is n=
ow
>> >>> >>>>> >>>>> >>>>> throwing an
>> >>> >>>>> >>>>> >>>>> error,
>> >>> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>> >>> >>>>> >>>>> >>>>>
>> >>> >>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.html=
']".
>> >>> >>>>> >>>>> >>>>> I'm not sure why. =A0Looking into it.
>> >>> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm n=
ot
>> >>> >>>>> >>>>> >>>>> sure
>> >>> >>>>> >>>>> >>>>> what
>> >>> >>>>> >>>>> >>>>> I'm
>> >>> >>>>> >>>>> >>>>> supposed to do about all the translations.
>> >>> >>>>> >>>>> >>>>>
>> >>> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <tim@chromium.org=
>
>> >>> >>>>> >>>>> >>>>> wrote:
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> fixed your points.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in the
>> >>> >>>>> >>>>> >>>>>> final
>> >>> >>>>> >>>>> >>>>>> wizard
>> >>> >>>>> >>>>> >>>>>> dialog
>> >>> >>>>> >>>>> >>>>>> is still
>> >>> >>>>> >>>>> >>>>>> not right, the button falls off the screen with o=
ne
>> >>> >>>>> >>>>> >>>>>> version
>> >>> >>>>> >>>>> >>>>>> of
>> >>> >>>>> >>>>> >>>>>> the
>> >>> >>>>> >>>>> >>>>>> text but not
>> >>> >>>>> >>>>> >>>>>> the other. =A0I will keep twiddling... I wish I w=
as
>> >>> >>>>> >>>>> >>>>>> an
>> >>> >>>>> >>>>> >>>>>> Oscar
>> >>> >>>>> >>>>> >>>>>> Mayer
>> >>> >>>>> >>>>> >>>>>> CSS
>> >>> >>>>> >>>>> >>>>>> wizard.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
>> >>> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#=
newcode5313
>> >>> >>>>> >>>>> >>>>>> Line 5313:
>> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>> >>>>> >>>>> >>>>>>>
>> >>> >>>>> >>>>> >>>>>>> Extra newline.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> Done.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27#=
newcode5331
>> >>> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to Googl=
e
>> >>> >>>>> >>>>> >>>>>> Docs! To
>> >>> >>>>> >>>>> >>>>>> merge and
>> >>> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
>> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>> >>>>> >>>>> >>>>>>>
>> >>> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this
>> >>> >>>>> >>>>> >>>>>>> file, I
>> >>> >>>>> >>>>> >>>>>>> think you
>> >>> >>>>> >>>>> >>>>>>> should
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> avoid
>> >>> >>>>> >>>>> >>>>>>>
>> >>> >>>>> >>>>> >>>>>>> having the newline in the middle of this string.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> Done.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
>> >>> >>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc
>> >>> >>>>> >>>>> >>>>>> (right):
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#=
newcode344
>> >>> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation failed.=
";
>> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>> >>>>> >>>>> >>>>>>>
>> >>> >>>>> >>>>> >>>>>>> Indentation is off.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> Done.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#=
newcode345
>> >>> >>>>> >>>>> >>>>>> Line 345:
>> >>> >>>>> >>>>> >>>>>> wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>> >>>>> >>>>> >>>>>>>
>> >>> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it
>> >>> >>>>> >>>>> >>>>>>> too.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> Ah! Done. =A0I just wanted to remove the TODO, mu=
st
>> >>> >>>>> >>>>> >>>>>> have
>> >>> >>>>> >>>>> >>>>>> forgotten
>> >>> >>>>> >>>>> >>>>>> to do
>> >>> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33#=
newcode396
>> >>> >>>>> >>>>> >>>>>> Line 396:
>> >>> >>>>> >>>>> >>>>>> wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>> >>>>> >>>>> >>>>>>>
>> >>> >>>>> >>>>> >>>>>>> As above, this line is redundant.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> Done.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
>> >>> >>>>> >>>>> >>>>>> File
>> >>> >>>>> >>>>> >>>>>> chrome/browser/views/options/content_page_view.cc
>> >>> >>>>> >>>>> >>>>>> (right):
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40#=
newcode473
>> >>> >>>>> >>>>> >>>>>> Line 473:
>> >>> >>>>> >>>>> >>>>>> stop_sync_description_label_->SetVisible(false);
>> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>> >>> >>>>> >>>>> >>>>>>>
>> >>> >>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
>> >>> >>>>> >>>>> >>>>>>> consistent
>> >>> >>>>> >>>>> >>>>>>> with the
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> IDS tag
>> >>> >>>>> >>>>> >>>>>>>
>> >>> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
>> >>> >>>>> >>>>> >>>>>>
>> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
>> >>> >>>>> >>>>> >>>>>
>> >>> >>>>> >>>>> >>>>
>> >>> >>>>> >>>>> >>>
>> >>> >>>>> >>>>> >>
>> >>> >>>>> >>>>> >
>> >>> >>>>> >>>>> >
>> >>> >>>>> >>>>
>> >>> >>>>> >>>
>> >>> >>>>> >>
>> >>> >>>>> >
>> >>> >>>>> >
>> >>> >>>>
>> >>> >>>
>> >>> >>
>> >>> >>
>> >>> >
>> >>
>> >
>> >
>
>

Nick Baum (Google)

OK, my bad. What Ben said :) -Nick On Thu, Oct 15, 2009 at 10:59 ...

11 years, 2 months ago (2009-10-15 18:05:30 UTC) #26

OK, my bad. What Ben said :)

-Nick

On Thu, Oct 15, 2009 at 10:59 AM, Ben Goodger <beng@google.com> wrote:

> I don't think stopping syncing is a common action so it doesn't bother
> me that it's more cumbersome.
>
> We can't make the dialog any taller.
>
> -Ben
>
> On Thu, Oct 15, 2009 at 10:58 AM, Tim Steele <tim@chromium.org> wrote:
> > This is what I was first planning but putting the text inline was
> suggested
> > as it was less obstructing.
> >
> > On Thu, Oct 15, 2009 at 10:52 AM, Ben Goodger <beng@google.com> wrote:
> >>
> >> Hrmp. I think that'll actually cause us trouble on smaller screen
> >> heights. This dialog box is actually already too tall. Can you instead
> >> have a confirmation dialog that says this info instead?
> >>
> >> <text>
> >>
> >> ( Stop Syncing ) ( Cancel ) <-- default
> >>
> >> -Ben
> >>
> >> On Thu, Oct 15, 2009 at 10:44 AM, Nick Carter <nick@chromium.org>
> wrote:
> >> > Just to double check: in addition to the changes to the wizard, the UI
> >> > team
> >> > is signing off on the wrench-menu options panel becoming taller to
> >> > accomodate the longer text that explains sync in the "Personal Stuff"
> >> > tab?
> >> >  - nick
> >> >
> >> > On Thu, Oct 15, 2009 at 10:38 AM, Nick Baum <nickbaum@chromium.org>
> >> > wrote:
> >> >>
> >> >> LGTM.
> >> >>
> >> >> On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com>
> wrote:
> >> >>>
> >> >>> Also, your OK button should have "default button" style (blue ring)
> >> >>> and have focus.
> >> >>>
> >> >>> -Ben
> >> >>>
> >> >>> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com>
> wrote:
> >> >>> > That's jpeg compression on the screenshot, not the check, right?
> >> >>> >
> >> >>> > If so, LGTM.
> >> >>> >
> >> >>> > -Ben
> >> >>> >
> >> >>> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org>
> >> >>> > wrote:
> >> >>> >> drumroll please...
> >> >>> >>
> >> >>> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org>
> >> >>> >> wrote:
> >> >>> >>>
> >> >>> >>> I added the app menu changes (this is toolbar_view.cc) from
> >> >>> >>> http://crbug.com/24858.
> >> >>> >>> I messaged Glen & Cole separately about designing me a big
> >> >>> >>> beautiful
> >> >>> >>> round
> >> >>> >>> green checkmark for this bandwagon patch headed for Taco Town.
> >> >>> >>>
> >> >>> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.org>
> >> >>> >>> wrote:
> >> >>> >>>>
> >> >>> >>>> I will hunt for a green checkmark.
> >> >>> >>>> Also, I uploaded a patchset with the last-minute NTP UI removal
> >> >>> >>>> mentioned
> >> >>> >>>> in http://crbug.com/24858.
> >> >>> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger <beng@google.com>
> >> >>> >>>> wrote:
> >> >>> >>>>>
> >> >>> >>>>> I don't like them changing size... however...
> >> >>> >>>>>
> >> >>> >>>>> on the success page, do we have a large green circle with a
> >> >>> >>>>> checkmark
> >> >>> >>>>> in it somewhere? Don't we use one of those in omaha? If so, I
> >> >>> >>>>> would
> >> >>> >>>>> insert it centered above the "Synced to..." text.
> >> >>> >>>>>
> >> >>> >>>>> -Ben
> >> >>> >>>>>
> >> >>> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum
> >> >>> >>>>> <nickbaum@chromium.org>
> >> >>> >>>>> wrote:
> >> >>> >>>>> > I like the login and merge, but the success one makes me
> >> >>> >>>>> > cringe
> >> >>> >>>>> > :)
> >> >>> >>>>> > Resizing is really hard?
> >> >>> >>>>> > -Nick
> >> >>> >>>>> >
> >> >>> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele <
> tim@chromium.org>
> >> >>> >>>>> > wrote:
> >> >>> >>>>> >>
> >> >>> >>>>> >> The correct (same width) screenshots.
> >> >>> >>>>> >>
> >> >>> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele
> >> >>> >>>>> >> <tim@chromium.org>
> >> >>> >>>>> >> wrote:
> >> >>> >>>>> >>>
> >> >>> >>>>> >>> Some new shots for the login dialog. Made it wider /
> >> >>> >>>>> >>> slightly
> >> >>> >>>>> >>> shorter,
> >> >>> >>>>> >>> and top-aligned text.
> >> >>> >>>>> >>>
> >> >>> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum
> >> >>> >>>>> >>> <nickbaum@chromium.org>
> >> >>> >>>>> >>> wrote:
> >> >>> >>>>> >>>>
> >> >>> >>>>> >>>> We need to nail this ASAP if we want it in the upcoming
> >> >>> >>>>> >>>> Beta.
> >> >>> >>>>> >>>> Can someone please just send mocks to Tim? He'll reply if
> >> >>> >>>>> >>>> it's
> >> >>> >>>>> >>>> not
> >> >>> >>>>> >>>> doable.
> >> >>> >>>>> >>>>
> >> >>> >>>>> >>>> -Nick
> >> >>> >>>>> >>>>
> >> >>> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger
> >> >>> >>>>> >>>> <beng@google.com>
> >> >>> >>>>> >>>> wrote:
> >> >>> >>>>> >>>>>
> >> >>> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
> >> >>> >>>>> >>>>>
> >> >>> >>>>> >>>>> -Ben
> >> >>> >>>>> >>>>>
> >> >>> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum
> >> >>> >>>>> >>>>> <nickbaum@chromium.org>
> >> >>> >>>>> >>>>> wrote:
> >> >>> >>>>> >>>>> > +UI team
> >> >>> >>>>> >>>>> > I really think it's weird to have all this extra
> spacing
> >> >>> >>>>> >>>>> > in
> >> >>> >>>>> >>>>> > the
> >> >>> >>>>> >>>>> > dialogs and
> >> >>> >>>>> >>>>> > the pref pane. Can't we resize these dynamically to
> >> >>> >>>>> >>>>> > adapt
> >> >>> >>>>> >>>>> > to the
> >> >>> >>>>> >>>>> > content?
> >> >>> >>>>> >>>>> > -Nick
> >> >>> >>>>> >>>>> >
> >> >>> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele
> >> >>> >>>>> >>>>> > <tim@chromium.org>
> >> >>> >>>>> >>>>> > wrote:
> >> >>> >>>>> >>>>> >>
> >> >>> >>>>> >>>>> >> +cc the real nickbaum
> >> >>> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele
> >> >>> >>>>> >>>>> >> <tim@chromium.org>
> >> >>> >>>>> >>>>> >> wrote:
> >> >>> >>>>> >>>>> >>>
> >> >>> >>>>> >>>>> >>> +cc nickbaum
> >> >>> >>>>> >>>>> >>> I had to up the size of the options dialog (in
> pixels,
> >> >>> >>>>> >>>>> >>> in
> >> >>> >>>>> >>>>> >>> english,
> >> >>> >>>>> >>>>> >>> it
> >> >>> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
> >> >>> >>>>> >>>>> >>> I attached a screenshot of what this means for the
> >> >>> >>>>> >>>>> >>> 'Basics'
> >> >>> >>>>> >>>>> >>> pane.
> >> >>> >>>>> >>>>> >>>  Looking for an OK.
> >> >>> >>>>> >>>>> >>> Fixed button sizes.
> >> >>> >>>>> >>>>> >>>
> >> >>> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele
> >> >>> >>>>> >>>>> >>> <tim@chromium.org>
> >> >>> >>>>> >>>>> >>> wrote:
> >> >>> >>>>> >>>>> >>>>
> >> >>> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :(  Sorry
> >> >>> >>>>> >>>>> >>>> for
> >> >>> >>>>> >>>>> >>>> thrashing
> >> >>> >>>>> >>>>> >>>> on
> >> >>> >>>>> >>>>> >>>> this.
> >> >>> >>>>> >>>>> >>>>
> >> >>> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
> >> >>> >>>>> >>>>> >>>> <tim@chromium.org>
> >> >>> >>>>> >>>>> >>>> wrote:
> >> >>> >>>>> >>>>> >>>>>
> >> >>> >>>>> >>>>> >>>>> Screenshots attached forevermore.  I made the
> width
> >> >>> >>>>> >>>>> >>>>> of
> >> >>> >>>>> >>>>> >>>>> the
> >> >>> >>>>> >>>>> >>>>> buttons in
> >> >>> >>>>> >>>>> >>>>> all our dialogs consistent with the min width in
> >> >>> >>>>> >>>>> >>>>> native
> >> >>> >>>>> >>>>> >>>>> ui.
> >> >>> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html is
> now
> >> >>> >>>>> >>>>> >>>>> throwing an
> >> >>> >>>>> >>>>> >>>>> error,
> >> >>> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
> >> >>> >>>>> >>>>> >>>>>
> >> >>> >>>>> >>>>> >>>>>
> 'chrome/browser/sync/resources/merge_and_sync.html']".
> >> >>> >>>>> >>>>> >>>>> I'm not sure why.  Looking into it.
> >> >>> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I'm
> not
> >> >>> >>>>> >>>>> >>>>> sure
> >> >>> >>>>> >>>>> >>>>> what
> >> >>> >>>>> >>>>> >>>>> I'm
> >> >>> >>>>> >>>>> >>>>> supposed to do about all the translations.
> >> >>> >>>>> >>>>> >>>>>
> >> >>> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM, <
> tim@chromium.org>
> >> >>> >>>>> >>>>> >>>>> wrote:
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> fixed your points.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in the
> >> >>> >>>>> >>>>> >>>>>> final
> >> >>> >>>>> >>>>> >>>>>> wizard
> >> >>> >>>>> >>>>> >>>>>> dialog
> >> >>> >>>>> >>>>> >>>>>> is still
> >> >>> >>>>> >>>>> >>>>>> not right, the button falls off the screen with
> one
> >> >>> >>>>> >>>>> >>>>>> version
> >> >>> >>>>> >>>>> >>>>>> of
> >> >>> >>>>> >>>>> >>>>>> the
> >> >>> >>>>> >>>>> >>>>>> text but not
> >> >>> >>>>> >>>>> >>>>>> the other.  I will keep twiddling... I wish I was
> >> >>> >>>>> >>>>> >>>>>> an
> >> >>> >>>>> >>>>> >>>>>> Oscar
> >> >>> >>>>> >>>>> >>>>>> Mayer
> >> >>> >>>>> >>>>> >>>>>> CSS
> >> >>> >>>>> >>>>> >>>>>> wizard.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/27
> >> >>> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd (right):
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/27#newcode5313
> >> >>> >>>>> >>>>> >>>>>> Line 5313:
> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >> >>> >>>>> >>>>> >>>>>>>
> >> >>> >>>>> >>>>> >>>>>>> Extra newline.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> Done.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/27#newcode5331
> >> >>> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to
> Google
> >> >>> >>>>> >>>>> >>>>>> Docs! To
> >> >>> >>>>> >>>>> >>>>>> merge and
> >> >>> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >> >>> >>>>> >>>>> >>>>>>>
> >> >>> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in this
> >> >>> >>>>> >>>>> >>>>>>> file, I
> >> >>> >>>>> >>>>> >>>>>>> think you
> >> >>> >>>>> >>>>> >>>>>>> should
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> avoid
> >> >>> >>>>> >>>>> >>>>>>>
> >> >>> >>>>> >>>>> >>>>>>> having the newline in the middle of this string.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> Done.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/33
> >> >>> >>>>> >>>>> >>>>>> File chrome/browser/sync/profile_sync_service.cc
> >> >>> >>>>> >>>>> >>>>>> (right):
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode344
> >> >>> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation
> failed.";
> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >> >>> >>>>> >>>>> >>>>>>>
> >> >>> >>>>> >>>>> >>>>>>> Indentation is off.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> Done.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode345
> >> >>> >>>>> >>>>> >>>>>> Line 345:
> >> >>> >>>>> >>>>> >>>>>> wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >> >>> >>>>> >>>>> >>>>>>>
> >> >>> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does it
> >> >>> >>>>> >>>>> >>>>>>> too.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> Ah! Done.  I just wanted to remove the TODO, must
> >> >>> >>>>> >>>>> >>>>>> have
> >> >>> >>>>> >>>>> >>>>>> forgotten
> >> >>> >>>>> >>>>> >>>>>> to do
> >> >>> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/33#newcode396
> >> >>> >>>>> >>>>> >>>>>> Line 396:
> >> >>> >>>>> >>>>> >>>>>> wizard_.Step(SyncSetupWizard::FATAL_ERROR);
> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >> >>> >>>>> >>>>> >>>>>>>
> >> >>> >>>>> >>>>> >>>>>>> As above, this line is redundant.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> Done.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26/40
> >> >>> >>>>> >>>>> >>>>>> File
> >> >>> >>>>> >>>>> >>>>>> chrome/browser/views/options/content_page_view.cc
> >> >>> >>>>> >>>>> >>>>>> (right):
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>>
> http://codereview.chromium.org/270081/diff/26/40#newcode473
> >> >>> >>>>> >>>>> >>>>>> Line 473:
> >> >>> >>>>> >>>>> >>>>>> stop_sync_description_label_->SetVisible(false);
> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
> >> >>> >>>>> >>>>> >>>>>>>
> >> >>> >>>>> >>>>> >>>>>>> Consider renaming description_label so that it's
> >> >>> >>>>> >>>>> >>>>>>> consistent
> >> >>> >>>>> >>>>> >>>>>>> with the
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> IDS tag
> >> >>> >>>>> >>>>> >>>>>>>
> >> >>> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
> >> >>> >>>>> >>>>> >>>>>>
> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
> >> >>> >>>>> >>>>> >>>>>
> >> >>> >>>>> >>>>> >>>>
> >> >>> >>>>> >>>>> >>>
> >> >>> >>>>> >>>>> >>
> >> >>> >>>>> >>>>> >
> >> >>> >>>>> >>>>> >
> >> >>> >>>>> >>>>
> >> >>> >>>>> >>>
> >> >>> >>>>> >>
> >> >>> >>>>> >
> >> >>> >>>>> >
> >> >>> >>>>
> >> >>> >>>
> >> >>> >>
> >> >>> >>
> >> >>> >
> >> >>
> >> >
> >> >
> >
> >
>

ncarter (slow)

LGTM (but you'll have to address the concerns about the options dialog height). http://codereview.chromium.org/270081/diff/3022/3024 File ...

11 years, 2 months ago (2009-10-15 18:08:42 UTC) #27

tim (not reviewing)

I had to get a new forklift, but I managed to upload a new patchset. ...

11 years, 2 months ago (2009-10-15 19:07:32 UTC) #28

beng (no_code_reviews)

Can you fix the dialog now? It's not comprehensible as it stands. -Ben On Thu, ...

11 years, 2 months ago (2009-10-15 19:07:53 UTC) #29

Can you fix the dialog now? It's not comprehensible as it stands.

-Ben

On Thu, Oct 15, 2009 at 11:44 AM, Tim Steele <tim@chromium.org> wrote:
> Hooked it up to the confirmation dialog; the text on the buttons is just
> 'Yes' 'No' at the moment; this is a limitation of the dialog, I can patch=
 it
> afterwards.
> Going to address nick@'s concerns and add ~40 more files to the patch set
> (all the .xtb files with new entries)
>
> On Thu, Oct 15, 2009 at 11:04 AM, Nick Baum <nickbaum@chromium.org> wrote=
:
>>
>> OK, my bad. What Ben said :)
>>
>> -Nick
>>
>> On Thu, Oct 15, 2009 at 10:59 AM, Ben Goodger <beng@google.com> wrote:
>>>
>>> I don't think stopping syncing is a common action so it doesn't bother
>>> me that it's more cumbersome.
>>>
>>> We can't make the dialog any taller.
>>>
>>> -Ben
>>>
>>> On Thu, Oct 15, 2009 at 10:58 AM, Tim Steele <tim@chromium.org> wrote:
>>> > This is what I was first planning but putting the text inline was
>>> > suggested
>>> > as it was less obstructing.
>>> >
>>> > On Thu, Oct 15, 2009 at 10:52 AM, Ben Goodger <beng@google.com> wrote=
:
>>> >>
>>> >> Hrmp. I think that'll actually cause us trouble on smaller screen
>>> >> heights. This dialog box is actually already too tall. Can you inste=
ad
>>> >> have a confirmation dialog that says this info instead?
>>> >>
>>> >> <text>
>>> >>
>>> >> ( Stop Syncing ) ( Cancel ) <-- default
>>> >>
>>> >> -Ben
>>> >>
>>> >> On Thu, Oct 15, 2009 at 10:44 AM, Nick Carter <nick@chromium.org>
>>> >> wrote:
>>> >> > Just to double check: in addition to the changes to the wizard, th=
e
>>> >> > UI
>>> >> > team
>>> >> > is signing off on the wrench-menu options panel becoming taller to
>>> >> > accomodate the longer text that explains sync in the "Personal
>>> >> > Stuff"
>>> >> > tab?
>>> >> > =A0- nick
>>> >> >
>>> >> > On Thu, Oct 15, 2009 at 10:38 AM, Nick Baum <nickbaum@chromium.org=
>
>>> >> > wrote:
>>> >> >>
>>> >> >> LGTM.
>>> >> >>
>>> >> >> On Thu, Oct 15, 2009 at 10:37 AM, Ben Goodger <beng@google.com>
>>> >> >> wrote:
>>> >> >>>
>>> >> >>> Also, your OK button should have "default button" style (blue
>>> >> >>> ring)
>>> >> >>> and have focus.
>>> >> >>>
>>> >> >>> -Ben
>>> >> >>>
>>> >> >>> On Thu, Oct 15, 2009 at 10:36 AM, Ben Goodger <beng@google.com>
>>> >> >>> wrote:
>>> >> >>> > That's jpeg compression on the screenshot, not the check, righ=
t?
>>> >> >>> >
>>> >> >>> > If so, LGTM.
>>> >> >>> >
>>> >> >>> > -Ben
>>> >> >>> >
>>> >> >>> > On Thu, Oct 15, 2009 at 10:35 AM, Tim Steele <tim@chromium.org=
>
>>> >> >>> > wrote:
>>> >> >>> >> drumroll please...
>>> >> >>> >>
>>> >> >>> >> On Wed, Oct 14, 2009 at 9:52 PM, Tim Steele <tim@chromium.org=
>
>>> >> >>> >> wrote:
>>> >> >>> >>>
>>> >> >>> >>> I added the app menu changes (this is toolbar_view.cc) from
>>> >> >>> >>> http://crbug.com/24858.
>>> >> >>> >>> I messaged Glen & Cole separately about designing me a big
>>> >> >>> >>> beautiful
>>> >> >>> >>> round
>>> >> >>> >>> green checkmark for this bandwagon patch headed for Taco Tow=
n.
>>> >> >>> >>>
>>> >> >>> >>> On Wed, Oct 14, 2009 at 9:00 PM, Tim Steele <tim@chromium.or=
g>
>>> >> >>> >>> wrote:
>>> >> >>> >>>>
>>> >> >>> >>>> I will hunt for a green checkmark.
>>> >> >>> >>>> Also, I uploaded a patchset with the last-minute NTP UI
>>> >> >>> >>>> removal
>>> >> >>> >>>> mentioned
>>> >> >>> >>>> in=A0http://crbug.com/24858.
>>> >> >>> >>>> On Wed, Oct 14, 2009 at 8:55 PM, Ben Goodger
>>> >> >>> >>>> <beng@google.com>
>>> >> >>> >>>> wrote:
>>> >> >>> >>>>>
>>> >> >>> >>>>> I don't like them changing size... however...
>>> >> >>> >>>>>
>>> >> >>> >>>>> on the success page, do we have a large green circle with =
a
>>> >> >>> >>>>> checkmark
>>> >> >>> >>>>> in it somewhere? Don't we use one of those in omaha? If so=
,
>>> >> >>> >>>>> I
>>> >> >>> >>>>> would
>>> >> >>> >>>>> insert it centered above the "Synced to..." text.
>>> >> >>> >>>>>
>>> >> >>> >>>>> -Ben
>>> >> >>> >>>>>
>>> >> >>> >>>>> On Wed, Oct 14, 2009 at 8:52 PM, Nick Baum
>>> >> >>> >>>>> <nickbaum@chromium.org>
>>> >> >>> >>>>> wrote:
>>> >> >>> >>>>> > I like the login and merge, but the success one makes me
>>> >> >>> >>>>> > cringe
>>> >> >>> >>>>> > :)
>>> >> >>> >>>>> > Resizing is really hard?
>>> >> >>> >>>>> > -Nick
>>> >> >>> >>>>> >
>>> >> >>> >>>>> > On Wed, Oct 14, 2009 at 7:01 PM, Tim Steele
>>> >> >>> >>>>> > <tim@chromium.org>
>>> >> >>> >>>>> > wrote:
>>> >> >>> >>>>> >>
>>> >> >>> >>>>> >> The correct (same width) screenshots.
>>> >> >>> >>>>> >>
>>> >> >>> >>>>> >> On Wed, Oct 14, 2009 at 6:57 PM, Tim Steele
>>> >> >>> >>>>> >> <tim@chromium.org>
>>> >> >>> >>>>> >> wrote:
>>> >> >>> >>>>> >>>
>>> >> >>> >>>>> >>> Some new shots for the login dialog. Made it wider /
>>> >> >>> >>>>> >>> slightly
>>> >> >>> >>>>> >>> shorter,
>>> >> >>> >>>>> >>> and top-aligned text.
>>> >> >>> >>>>> >>>
>>> >> >>> >>>>> >>> On Wed, Oct 14, 2009 at 3:52 PM, Nick Baum
>>> >> >>> >>>>> >>> <nickbaum@chromium.org>
>>> >> >>> >>>>> >>> wrote:
>>> >> >>> >>>>> >>>>
>>> >> >>> >>>>> >>>> We need to nail this ASAP if we want it in the upcomi=
ng
>>> >> >>> >>>>> >>>> Beta.
>>> >> >>> >>>>> >>>> Can someone please just send mocks to Tim? He'll repl=
y
>>> >> >>> >>>>> >>>> if
>>> >> >>> >>>>> >>>> it's
>>> >> >>> >>>>> >>>> not
>>> >> >>> >>>>> >>>> doable.
>>> >> >>> >>>>> >>>>
>>> >> >>> >>>>> >>>> -Nick
>>> >> >>> >>>>> >>>>
>>> >> >>> >>>>> >>>> On Wed, Oct 14, 2009 at 2:46 PM, Ben Goodger
>>> >> >>> >>>>> >>>> <beng@google.com>
>>> >> >>> >>>>> >>>> wrote:
>>> >> >>> >>>>> >>>>>
>>> >> >>> >>>>> >>>>> I think all the sync dialogs should be 50% wider.
>>> >> >>> >>>>> >>>>>
>>> >> >>> >>>>> >>>>> -Ben
>>> >> >>> >>>>> >>>>>
>>> >> >>> >>>>> >>>>> On Wed, Oct 14, 2009 at 2:23 PM, Nick Baum
>>> >> >>> >>>>> >>>>> <nickbaum@chromium.org>
>>> >> >>> >>>>> >>>>> wrote:
>>> >> >>> >>>>> >>>>> > +UI team
>>> >> >>> >>>>> >>>>> > I really think it's weird to have all this extra
>>> >> >>> >>>>> >>>>> > spacing
>>> >> >>> >>>>> >>>>> > in
>>> >> >>> >>>>> >>>>> > the
>>> >> >>> >>>>> >>>>> > dialogs and
>>> >> >>> >>>>> >>>>> > the pref pane. Can't we resize these dynamically t=
o
>>> >> >>> >>>>> >>>>> > adapt
>>> >> >>> >>>>> >>>>> > to the
>>> >> >>> >>>>> >>>>> > content?
>>> >> >>> >>>>> >>>>> > -Nick
>>> >> >>> >>>>> >>>>> >
>>> >> >>> >>>>> >>>>> > On Wed, Oct 14, 2009 at 10:56 AM, Tim Steele
>>> >> >>> >>>>> >>>>> > <tim@chromium.org>
>>> >> >>> >>>>> >>>>> > wrote:
>>> >> >>> >>>>> >>>>> >>
>>> >> >>> >>>>> >>>>> >> +cc the real nickbaum
>>> >> >>> >>>>> >>>>> >> On Wed, Oct 14, 2009 at 10:52 AM, Tim Steele
>>> >> >>> >>>>> >>>>> >> <tim@chromium.org>
>>> >> >>> >>>>> >>>>> >> wrote:
>>> >> >>> >>>>> >>>>> >>>
>>> >> >>> >>>>> >>>>> >>> +cc nickbaum
>>> >> >>> >>>>> >>>>> >>> I had to up the size of the options dialog (in
>>> >> >>> >>>>> >>>>> >>> pixels,
>>> >> >>> >>>>> >>>>> >>> in
>>> >> >>> >>>>> >>>>> >>> english,
>>> >> >>> >>>>> >>>>> >>> it
>>> >> >>> >>>>> >>>>> >>> looks like the jump went from 610 to 650).
>>> >> >>> >>>>> >>>>> >>> I attached a screenshot of what this means for t=
he
>>> >> >>> >>>>> >>>>> >>> 'Basics'
>>> >> >>> >>>>> >>>>> >>> pane.
>>> >> >>> >>>>> >>>>> >>> =A0Looking for an OK.
>>> >> >>> >>>>> >>>>> >>> Fixed button sizes.
>>> >> >>> >>>>> >>>>> >>>
>>> >> >>> >>>>> >>>>> >>> On Wed, Oct 14, 2009 at 2:01 AM, Tim Steele
>>> >> >>> >>>>> >>>>> >>> <tim@chromium.org>
>>> >> >>> >>>>> >>>>> >>> wrote:
>>> >> >>> >>>>> >>>>> >>>>
>>> >> >>> >>>>> >>>>> >>>> Hmm. Sizes of the buttons still look off :(
>>> >> >>> >>>>> >>>>> >>>> =A0Sorry
>>> >> >>> >>>>> >>>>> >>>> for
>>> >> >>> >>>>> >>>>> >>>> thrashing
>>> >> >>> >>>>> >>>>> >>>> on
>>> >> >>> >>>>> >>>>> >>>> this.
>>> >> >>> >>>>> >>>>> >>>>
>>> >> >>> >>>>> >>>>> >>>> On Wed, Oct 14, 2009 at 1:59 AM, Tim Steele
>>> >> >>> >>>>> >>>>> >>>> <tim@chromium.org>
>>> >> >>> >>>>> >>>>> >>>> wrote:
>>> >> >>> >>>>> >>>>> >>>>>
>>> >> >>> >>>>> >>>>> >>>>> Screenshots attached forevermore. =A0I made th=
e
>>> >> >>> >>>>> >>>>> >>>>> width
>>> >> >>> >>>>> >>>>> >>>>> of
>>> >> >>> >>>>> >>>>> >>>>> the
>>> >> >>> >>>>> >>>>> >>>>> buttons in
>>> >> >>> >>>>> >>>>> >>>>> all our dialogs consistent with the min width =
in
>>> >> >>> >>>>> >>>>> >>>>> native
>>> >> >>> >>>>> >>>>> >>>>> ui.
>>> >> >>> >>>>> >>>>> >>>>> However, trying to upload merge_and_sync.html =
is
>>> >> >>> >>>>> >>>>> >>>>> now
>>> >> >>> >>>>> >>>>> >>>>> throwing an
>>> >> >>> >>>>> >>>>> >>>>> error,
>>> >> >>> >>>>> >>>>> >>>>> saying "Got error status from ['svn', 'cat',
>>> >> >>> >>>>> >>>>> >>>>>
>>> >> >>> >>>>> >>>>> >>>>>
>>> >> >>> >>>>> >>>>> >>>>> 'chrome/browser/sync/resources/merge_and_sync.=
html']".
>>> >> >>> >>>>> >>>>> >>>>> I'm not sure why. =A0Looking into it.
>>> >> >>> >>>>> >>>>> >>>>> As for my changes to locale_resources.grd... I=
'm
>>> >> >>> >>>>> >>>>> >>>>> not
>>> >> >>> >>>>> >>>>> >>>>> sure
>>> >> >>> >>>>> >>>>> >>>>> what
>>> >> >>> >>>>> >>>>> >>>>> I'm
>>> >> >>> >>>>> >>>>> >>>>> supposed to do about all the translations.
>>> >> >>> >>>>> >>>>> >>>>>
>>> >> >>> >>>>> >>>>> >>>>> On Wed, Oct 14, 2009 at 1:05 AM,
>>> >> >>> >>>>> >>>>> >>>>> <tim@chromium.org>
>>> >> >>> >>>>> >>>>> >>>>> wrote:
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> fixed your points.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> still trying to get screenshots, the html in
>>> >> >>> >>>>> >>>>> >>>>>> the
>>> >> >>> >>>>> >>>>> >>>>>> final
>>> >> >>> >>>>> >>>>> >>>>>> wizard
>>> >> >>> >>>>> >>>>> >>>>>> dialog
>>> >> >>> >>>>> >>>>> >>>>>> is still
>>> >> >>> >>>>> >>>>> >>>>>> not right, the button falls off the screen wi=
th
>>> >> >>> >>>>> >>>>> >>>>>> one
>>> >> >>> >>>>> >>>>> >>>>>> version
>>> >> >>> >>>>> >>>>> >>>>>> of
>>> >> >>> >>>>> >>>>> >>>>>> the
>>> >> >>> >>>>> >>>>> >>>>>> text but not
>>> >> >>> >>>>> >>>>> >>>>>> the other. =A0I will keep twiddling... I wish=
 I
>>> >> >>> >>>>> >>>>> >>>>>> was
>>> >> >>> >>>>> >>>>> >>>>>> an
>>> >> >>> >>>>> >>>>> >>>>>> Oscar
>>> >> >>> >>>>> >>>>> >>>>>> Mayer
>>> >> >>> >>>>> >>>>> >>>>>> CSS
>>> >> >>> >>>>> >>>>> >>>>>> wizard.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/27
>>> >> >>> >>>>> >>>>> >>>>>> File chrome/app/generated_resources.grd
>>> >> >>> >>>>> >>>>> >>>>>> (right):
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/27#newcode5313
>>> >> >>> >>>>> >>>>> >>>>>> Line 5313:
>>> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >> >>> >>>>> >>>>> >>>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>> Extra newline.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> Done.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/27#newcode5331
>>> >> >>> >>>>> >>>>> >>>>>> Line 5331: Your bookmarks are now synced to
>>> >> >>> >>>>> >>>>> >>>>>> Google
>>> >> >>> >>>>> >>>>> >>>>>> Docs! To
>>> >> >>> >>>>> >>>>> >>>>>> merge and
>>> >> >>> >>>>> >>>>> >>>>>> sync your bookmarks to Chrome on
>>> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >> >>> >>>>> >>>>> >>>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>> Based on the examples I see elsewhere in thi=
s
>>> >> >>> >>>>> >>>>> >>>>>>> file, I
>>> >> >>> >>>>> >>>>> >>>>>>> think you
>>> >> >>> >>>>> >>>>> >>>>>>> should
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> avoid
>>> >> >>> >>>>> >>>>> >>>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>> having the newline in the middle of this
>>> >> >>> >>>>> >>>>> >>>>>>> string.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> Done.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/33
>>> >> >>> >>>>> >>>>> >>>>>> File
>>> >> >>> >>>>> >>>>> >>>>>> chrome/browser/sync/profile_sync_service.cc
>>> >> >>> >>>>> >>>>> >>>>>> (right):
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/33#newcode344
>>> >> >>> >>>>> >>>>> >>>>>> Line 344: LOG(ERROR) << "Model assocation
>>> >> >>> >>>>> >>>>> >>>>>> failed.";
>>> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >> >>> >>>>> >>>>> >>>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>> Indentation is off.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> Done.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/33#newcode345
>>> >> >>> >>>>> >>>>> >>>>>> Line 345:
>>> >> >>> >>>>> >>>>> >>>>>> wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >> >>> >>>>> >>>>> >>>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>> Unnecessary since OnUnrecoverableError does =
it
>>> >> >>> >>>>> >>>>> >>>>>>> too.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> Ah! Done. =A0I just wanted to remove the TODO=
,
>>> >> >>> >>>>> >>>>> >>>>>> must
>>> >> >>> >>>>> >>>>> >>>>>> have
>>> >> >>> >>>>> >>>>> >>>>>> forgotten
>>> >> >>> >>>>> >>>>> >>>>>> to do
>>> >> >>> >>>>> >>>>> >>>>>> that when adding that to OnUnrecoverableError=
.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/33#newcode396
>>> >> >>> >>>>> >>>>> >>>>>> Line 396:
>>> >> >>> >>>>> >>>>> >>>>>> wizard_.Step(SyncSetupWizard::FATAL_ERROR);
>>> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >> >>> >>>>> >>>>> >>>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>> As above, this line is redundant.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> Done.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/40
>>> >> >>> >>>>> >>>>> >>>>>> File
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> chrome/browser/views/options/content_page_vie=
w.cc
>>> >> >>> >>>>> >>>>> >>>>>> (right):
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081/diff/26=
/40#newcode473
>>> >> >>> >>>>> >>>>> >>>>>> Line 473:
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> stop_sync_description_label_->SetVisible(fals=
e);
>>> >> >>> >>>>> >>>>> >>>>>> On 2009/10/14 01:07:32, nick wrote:
>>> >> >>> >>>>> >>>>> >>>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>> Consider renaming description_label so that
>>> >> >>> >>>>> >>>>> >>>>>>> it's
>>> >> >>> >>>>> >>>>> >>>>>>> consistent
>>> >> >>> >>>>> >>>>> >>>>>>> with the
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> IDS tag
>>> >> >>> >>>>> >>>>> >>>>>>>
>>> >> >>> >>>>> >>>>> >>>>>>> name (...EXPLANANTION_LABEL), or vice versa.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> oops, meant to do that. thanks.
>>> >> >>> >>>>> >>>>> >>>>>>
>>> >> >>> >>>>> >>>>> >>>>>> http://codereview.chromium.org/270081
>>> >> >>> >>>>> >>>>> >>>>>
>>> >> >>> >>>>> >>>>> >>>>
>>> >> >>> >>>>> >>>>> >>>
>>> >> >>> >>>>> >>>>> >>
>>> >> >>> >>>>> >>>>> >
>>> >> >>> >>>>> >>>>> >
>>> >> >>> >>>>> >>>>
>>> >> >>> >>>>> >>>
>>> >> >>> >>>>> >>
>>> >> >>> >>>>> >
>>> >> >>> >>>>> >
>>> >> >>> >>>>
>>> >> >>> >>>
>>> >> >>> >>
>>> >> >>> >>
>>> >> >>> >
>>> >> >>
>>> >> >
>>> >> >
>>> >
>>> >
>>
>
>

tim (not reviewing)

Actually my forklift is broken. Getting internal server error when I try to upload the ...

11 years, 2 months ago (2009-10-15 19:10:43 UTC) #30

beng (no_code_reviews)

How about just "Stop syncing" -Ben On Thu, Oct 15, 2009 at 12:27 PM, Tim ...

11 years, 2 months ago (2009-10-15 19:28:36 UTC) #31

tim (not reviewing)

The button they just clicked on says 'Stop syncing this account'. Why have two different ...

11 years, 2 months ago (2009-10-15 19:30:21 UTC) #32

beng (no_code_reviews)

11 years, 2 months ago (2009-10-15 19:38:06 UTC) #33

LGTM

On Thu, Oct 15, 2009 at 12:36 PM, Tim Steele <tim@chromium.org> wrote:
> /me complies
>
> On Thu, Oct 15, 2009 at 12:29 PM, Tim Steele <tim@chromium.org> wrote:
>>
>> The button they just clicked on says 'Stop syncing this account'. =A0Why
>> have two different texts for the exact same thing? =A0It seems unnecessa=
ry
>> both in principle and in generated_resources.grd!
>>
>> On Thu, Oct 15, 2009 at 12:28 PM, Ben Goodger <beng@google.com> wrote:
>>>
>>> How about just "Stop syncing"
>>>
>>> -Ben
>>>
>>> On Thu, Oct 15, 2009 at 12:27 PM, Tim Steele <tim@chromium.org> wrote:
>>> > I have to agree it was weird. =A0Customized the confirm dialog button=
s.
>>> >
>>> > On Thu, Oct 15, 2009 at 12:10 PM, <tim@chromium.org> wrote:
>>> >>
>>> >> Actually my forklift is broken. =A0Getting internal server error whe=
n I
>>> >> try
>>> >> to
>>> >> upload the 80 file patchset.
>>> >>
>>> >> http://codereview.chromium.org/270081
>>> >
>>> >
>>
>
>

Expand Messages | Collapse Messages