Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10631010:
Change style of ConstrainedWindowViews. (Closed)
|
Created:
8 years, 6 months ago by Albert Bodenhamer Modified:
8 years, 5 months ago CC:
chromium-reviews, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChange style of ConstrainedWindowViews.
Removes the "baby blue" bar and close button in certain dialogs. Instead, these dialogs will draw with no frame and the dialogs themselves will now be responsible for providing the ability to close.
Impact on derived classes:
Print preview has a cancel button already, but we'll add a close button in a separate CL.
This CL adds a close button in the web intents picker.
The htaccess dialog already has a cancel button and is frameless on Mac and Linux without a close button. I don't think a change is required.
BUG=133363
TEST=Verify 133363
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146466
Patch Set 1 #Patch Set 2 : Merge #
Total comments: 6
Patch Set 3 : Review feedback #Patch Set 4 : Partial refactor in prep for flag #Patch Set 5 : Moved changes behind a flag #Patch Set 6 : Cleanup #Patch Set 7 : Whitespace #
Total comments: 4
Patch Set 8 : Merge #Patch Set 9 : Review feedback and cleanup #Patch Set 10 : Remove extra "private" #
Messages
Total messages: 31 (0 generated)
Hey guys, Could I get you to take a look at this when you get a chance? Thanks.
+groby for web intents UI.
I'm away right now. Adding oshima instead. Please remember to run pngcrush.
On 2012/06/22 18:50:24, sail wrote: > I'm away right now. Adding oshima instead. > Please remember to run pngcrush. Thanks. I'm just removing images, so no need to pngcrush.
On 2012/06/22 18:52:21, Albert Bodenhamer wrote: > On 2012/06/22 18:50:24, sail wrote: > > I'm away right now. Adding oshima instead. > > Please remember to run pngcrush. > > Thanks. I'm just removing images, so no need to pngcrush. Argh, never mind :-P
resource change LGTM
picker lgtm, and some drive-by nits :) http://codereview.chromium.org/10631010/diff/2001/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_window_views.h (right): http://codereview.chromium.org/10631010/diff/2001/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_window_views.h:79: // Overridden from views::Widget: Personal nit: Why not keep the overrides in the same order as the inheritance statement for the class? http://codereview.chromium.org/10631010/diff/2001/chrome/browser/ui/views/web... File chrome/browser/ui/views/web_intent_picker_views.cc (right): http://codereview.chromium.org/10631010/diff/2001/chrome/browser/ui/views/web... chrome/browser/ui/views/web_intent_picker_views.cc:1155: close_button->SetImage(views::CustomButton::BS_PUSHED, Is that proper behavior for views? I.e. identical image for hover & push? http://codereview.chromium.org/10631010/diff/2001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/10631010/diff/2001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:3458: 'browser/ui/views/constrained_window_frame_view.cc', Please put in alphabetical order
Screenshot please!
login_prompt.cc is most certainly not deprecated... see http://www.pagetutor.com/keeper/http_authentication/index.html for a test case.
Screen shots attached. The empty content in the "after" is because it's a local build and is unrelated to my change. On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: > login_prompt.cc is most certainly not deprecated... see > http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. > > http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... Got it. I was misinformed. Someone thought it was part of an experiment they had run and needed to remove code for. Login_prompt.cc is handling htaccess prompts I mentioned in the CL description. I'll correct the CL description. Thanks. -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
And where's the mock from the UI designers? -Ben On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > Screen shots attached. The empty content in the "after" is because it's a > local build and is unrelated to my change. > > On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: > >> login_prompt.cc is most certainly not deprecated... see >> http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. >> >> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... > > > Got it. I was misinformed. Someone thought it was part of an experiment > they had run and needed to remove code for. Login_prompt.cc is handling > htaccess prompts I mentioned in the CL description. I'll correct the CL > description. Thanks. > > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> > org >
Updated CL description. http://codereview.chromium.org/10631010/diff/2001/chrome/browser/ui/views/con... File chrome/browser/ui/views/constrained_window_views.h (right): http://codereview.chromium.org/10631010/diff/2001/chrome/browser/ui/views/con... chrome/browser/ui/views/constrained_window_views.h:79: // Overridden from views::Widget: On 2012/06/22 19:01:05, groby wrote: > Personal nit: Why not keep the overrides in the same order as the inheritance > statement for the class? Done. http://codereview.chromium.org/10631010/diff/2001/chrome/browser/ui/views/web... File chrome/browser/ui/views/web_intent_picker_views.cc (right): http://codereview.chromium.org/10631010/diff/2001/chrome/browser/ui/views/web... chrome/browser/ui/views/web_intent_picker_views.cc:1155: close_button->SetImage(views::CustomButton::BS_PUSHED, On 2012/06/22 19:01:05, groby wrote: > Is that proper behavior for views? I.e. identical image for hover & push? I'll double check with the UI folks to be sure, but that's how this graphic is used elsewhere. http://codereview.chromium.org/10631010/diff/2001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/10631010/diff/2001/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:3458: 'browser/ui/views/constrained_window_frame_view.cc', On 2012/06/22 19:01:05, groby wrote: > Please put in alphabetical order Done.
https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has the ultimate goal. I haven't implemented the full spec in this CL. My goal was to get rid of the blue bar while moving closer to this. Specifically, I haven't addressed animation, styling of the drop shadow, rounding the lower corners of the dialog, background color, position, etc. On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > And where's the mock from the UI designers? > > -Ben > > > On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > >> Screen shots attached. The empty content in the "after" is because it's >> a local build and is unrelated to my change. >> >> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >> >>> login_prompt.cc is most certainly not deprecated... see >>> http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. >>> >>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >> >> >> Got it. I was misinformed. Someone thought it was part of an experiment >> they had run and needed to remove code for. Login_prompt.cc is handling >> htaccess prompts I mentioned in the CL description. I'll correct the CL >> description. Thanks. >> >> >> >> -- >> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >> org >> > > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
OK I discussed this with Glen and I don't think this is ready to go as-is. Here are the things I think you need to accomplish before this can be turned on: - correct window shape (border radius etc) - correct window background - correct fonts, etc. - screenshots of every constrained window with this new treatment. - I don't believe you can implement the shadow, the animations or the nice dimming using the infrastructure we have today. This will require the Aura transition currently being planned, so I don't think any of that is a requirement. The designers may want a stroke of some kind around the window then in lieu of the shadow. - I think this change will require some changes to how constrained windows are parented. right now they're parented to the WebContentsView, but as you can see in the new mockup they overlap with the browser UI (for anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This implies these dialogs are parented to the browser frame. Since they should be tab modal, I do not want to see these dialogs parented to the desktop (i.e. they should be WS_CHILD windows of the browser frame). I highly recommend pursuing this work behind a about:flags flag. My prediction is it will take several weeks to get everything to look "right" and in the interim we need to be shipping releases. -Ben On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > > https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has > the ultimate goal. > > I haven't implemented the full spec in this CL. My goal was to get rid of > the blue bar while moving closer to this. > > Specifically, I haven't addressed animation, styling of the drop shadow, > rounding the lower corners of the dialog, background color, position, etc. > > > On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > >> And where's the mock from the UI designers? >> >> -Ben >> >> >> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer <abodenha@chromium.org >> > wrote: >> >>> Screen shots attached. The empty content in the "after" is because it's >>> a local build and is unrelated to my change. >>> >>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>> >>>> login_prompt.cc is most certainly not deprecated... see >>>> http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. >>>> >>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>> >>> >>> Got it. I was misinformed. Someone thought it was part of an >>> experiment they had run and needed to remove code for. Login_prompt.cc is >>> handling htaccess prompts I mentioned in the CL description. I'll correct >>> the CL description. Thanks. >>> >>> >>> >>> -- >>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>> org >>> >> >> > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> > org >
Thanks Ben. I'll file a new bug on the more complete changes to ConstrainedWindow and mark it blocking 133363. On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > OK I discussed this with Glen and I don't think this is ready to go as-is. > > Here are the things I think you need to accomplish before this can be > turned on: > > - correct window shape (border radius etc) > - correct window background > - correct fonts, etc. > - screenshots of every constrained window with this new treatment. > - I don't believe you can implement the shadow, the animations or the nice > dimming using the infrastructure we have today. This will require the Aura > transition currently being planned, so I don't think any of that is a > requirement. The designers may want a stroke of some kind around the window > then in lieu of the shadow. > - I think this change will require some changes to how constrained windows > are parented. right now they're parented to the WebContentsView, but as you > can see in the new mockup they overlap with the browser UI (for > anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This > implies these dialogs are parented to the browser frame. Since they should > be tab modal, I do not want to see these dialogs parented to the desktop > (i.e. they should be WS_CHILD windows of the browser frame). > > I highly recommend pursuing this work behind a about:flags flag. My > prediction is it will take several weeks to get everything to look "right" > and in the interim we need to be shipping releases. > > -Ben > > On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > >> >> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >> the ultimate goal. >> >> I haven't implemented the full spec in this CL. My goal was to get rid >> of the blue bar while moving closer to this. >> >> Specifically, I haven't addressed animation, styling of the drop shadow, >> rounding the lower corners of the dialog, background color, position, etc. >> >> >> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) <ben@chromium.org>wrote: >> >>> And where's the mock from the UI designers? >>> >>> -Ben >>> >>> >>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>> abodenha@chromium.org> wrote: >>> >>>> Screen shots attached. The empty content in the "after" is because >>>> it's a local build and is unrelated to my change. >>>> >>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>> >>>>> login_prompt.cc is most certainly not deprecated... see >>>>> http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. >>>>> >>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>> >>>> >>>> Got it. I was misinformed. Someone thought it was part of an >>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>> the CL description. Thanks. >>>> >>>> >>>> >>>> -- >>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>> org >>>> >>> >>> >> >> >> -- >> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >> org >> > > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
Note also that behind a flag I will be more receptive to incremental changes. -Ben On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > Thanks Ben. > > I'll file a new bug on the more complete changes to ConstrainedWindow and > mark it blocking 133363. > > On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > >> OK I discussed this with Glen and I don't think this is ready to go as-is. >> >> Here are the things I think you need to accomplish before this can be >> turned on: >> >> - correct window shape (border radius etc) >> - correct window background >> - correct fonts, etc. >> - screenshots of every constrained window with this new treatment. >> - I don't believe you can implement the shadow, the animations or the >> nice dimming using the infrastructure we have today. This will require the >> Aura transition currently being planned, so I don't think any of that is a >> requirement. The designers may want a stroke of some kind around the window >> then in lieu of the shadow. >> - I think this change will require some changes to how constrained >> windows are parented. right now they're parented to the WebContentsView, >> but as you can see in the new mockup they overlap with the browser UI (for >> anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This >> implies these dialogs are parented to the browser frame. Since they should >> be tab modal, I do not want to see these dialogs parented to the desktop >> (i.e. they should be WS_CHILD windows of the browser frame). >> >> I highly recommend pursuing this work behind a about:flags flag. My >> prediction is it will take several weeks to get everything to look "right" >> and in the interim we need to be shipping releases. >> >> -Ben >> >> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer <abodenha@chromium.org >> > wrote: >> >>> >>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>> the ultimate goal. >>> >>> I haven't implemented the full spec in this CL. My goal was to get rid >>> of the blue bar while moving closer to this. >>> >>> Specifically, I haven't addressed animation, styling of the drop shadow, >>> rounding the lower corners of the dialog, background color, position, etc. >>> >>> >>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) <ben@chromium.org>wrote: >>> >>>> And where's the mock from the UI designers? >>>> >>>> -Ben >>>> >>>> >>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>> abodenha@chromium.org> wrote: >>>> >>>>> Screen shots attached. The empty content in the "after" is because >>>>> it's a local build and is unrelated to my change. >>>>> >>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>> >>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>> http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. >>>>>> >>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>> >>>>> >>>>> Got it. I was misinformed. Someone thought it was part of an >>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>> the CL description. Thanks. >>>>> >>>>> >>>>> >>>>> -- >>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>> org >>>>> >>>> >>>> >>> >>> >>> -- >>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>> org >>> >> >> > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> > org >
Thanks. I'll rework this to put the changes behind a flag and resend it. On Mon, Jun 25, 2012 at 5:04 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > Note also that behind a flag I will be more receptive to incremental > changes. > > -Ben > > > On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > >> Thanks Ben. >> >> I'll file a new bug on the more complete changes to ConstrainedWindow and >> mark it blocking 133363. >> >> On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) <ben@chromium.org>wrote: >> >>> OK I discussed this with Glen and I don't think this is ready to go >>> as-is. >>> >>> Here are the things I think you need to accomplish before this can be >>> turned on: >>> >>> - correct window shape (border radius etc) >>> - correct window background >>> - correct fonts, etc. >>> - screenshots of every constrained window with this new treatment. >>> - I don't believe you can implement the shadow, the animations or the >>> nice dimming using the infrastructure we have today. This will require the >>> Aura transition currently being planned, so I don't think any of that is a >>> requirement. The designers may want a stroke of some kind around the window >>> then in lieu of the shadow. >>> - I think this change will require some changes to how constrained >>> windows are parented. right now they're parented to the WebContentsView, >>> but as you can see in the new mockup they overlap with the browser UI (for >>> anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This >>> implies these dialogs are parented to the browser frame. Since they should >>> be tab modal, I do not want to see these dialogs parented to the desktop >>> (i.e. they should be WS_CHILD windows of the browser frame). >>> >>> I highly recommend pursuing this work behind a about:flags flag. My >>> prediction is it will take several weeks to get everything to look "right" >>> and in the interim we need to be shipping releases. >>> >>> -Ben >>> >>> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer < >>> abodenha@chromium.org> wrote: >>> >>>> >>>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>>> the ultimate goal. >>>> >>>> I haven't implemented the full spec in this CL. My goal was to get rid >>>> of the blue bar while moving closer to this. >>>> >>>> Specifically, I haven't addressed animation, styling of the drop >>>> shadow, rounding the lower corners of the dialog, background color, >>>> position, etc. >>>> >>>> >>>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) <ben@chromium.org >>>> > wrote: >>>> >>>>> And where's the mock from the UI designers? >>>>> >>>>> -Ben >>>>> >>>>> >>>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>>> abodenha@chromium.org> wrote: >>>>> >>>>>> Screen shots attached. The empty content in the "after" is because >>>>>> it's a local build and is unrelated to my change. >>>>>> >>>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>>> >>>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>>> http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. >>>>>>> >>>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>>> >>>>>> >>>>>> Got it. I was misinformed. Someone thought it was part of an >>>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>>> the CL description. Thanks. >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>> org >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>> org >>>> >>> >>> >> >> >> -- >> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >> org >> > > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
Actually, on second thought, a command line switch (or even a compile time flag) makes more sense to me. This isn't something we need to clutter chrome://flags with. On Mon, Jun 25, 2012 at 5:16 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > Thanks. I'll rework this to put the changes behind a flag and resend it. > > > On Mon, Jun 25, 2012 at 5:04 PM, Ben Goodger (Google) <ben@chromium.org>wrote: > >> Note also that behind a flag I will be more receptive to incremental >> changes. >> >> -Ben >> >> >> On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer <abodenha@chromium.org >> > wrote: >> >>> Thanks Ben. >>> >>> I'll file a new bug on the more complete changes to ConstrainedWindow >>> and mark it blocking 133363. >>> >>> On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) <ben@chromium.org>wrote: >>> >>>> OK I discussed this with Glen and I don't think this is ready to go >>>> as-is. >>>> >>>> Here are the things I think you need to accomplish before this can be >>>> turned on: >>>> >>>> - correct window shape (border radius etc) >>>> - correct window background >>>> - correct fonts, etc. >>>> - screenshots of every constrained window with this new treatment. >>>> - I don't believe you can implement the shadow, the animations or the >>>> nice dimming using the infrastructure we have today. This will require the >>>> Aura transition currently being planned, so I don't think any of that is a >>>> requirement. The designers may want a stroke of some kind around the window >>>> then in lieu of the shadow. >>>> - I think this change will require some changes to how constrained >>>> windows are parented. right now they're parented to the WebContentsView, >>>> but as you can see in the new mockup they overlap with the browser UI (for >>>> anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This >>>> implies these dialogs are parented to the browser frame. Since they should >>>> be tab modal, I do not want to see these dialogs parented to the desktop >>>> (i.e. they should be WS_CHILD windows of the browser frame). >>>> >>>> I highly recommend pursuing this work behind a about:flags flag. My >>>> prediction is it will take several weeks to get everything to look "right" >>>> and in the interim we need to be shipping releases. >>>> >>>> -Ben >>>> >>>> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer < >>>> abodenha@chromium.org> wrote: >>>> >>>>> >>>>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>>>> the ultimate goal. >>>>> >>>>> I haven't implemented the full spec in this CL. My goal was to get >>>>> rid of the blue bar while moving closer to this. >>>>> >>>>> Specifically, I haven't addressed animation, styling of the drop >>>>> shadow, rounding the lower corners of the dialog, background color, >>>>> position, etc. >>>>> >>>>> >>>>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) < >>>>> ben@chromium.org> wrote: >>>>> >>>>>> And where's the mock from the UI designers? >>>>>> >>>>>> -Ben >>>>>> >>>>>> >>>>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>>>> abodenha@chromium.org> wrote: >>>>>> >>>>>>> Screen shots attached. The empty content in the "after" is because >>>>>>> it's a local build and is unrelated to my change. >>>>>>> >>>>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>>>> >>>>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>>>> http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. >>>>>>>> >>>>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>>>> >>>>>>> >>>>>>> Got it. I was misinformed. Someone thought it was part of an >>>>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>>>> the CL description. Thanks. >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>> org >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>> org >>>>> >>>> >>>> >>> >>> >>> -- >>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>> org >>> >> >> > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> > org > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
Don't over-think about:flags. It's not a user screen. It's useful to have switches there especially on ChromeOS where it's non-trivial to run with a different command line. Any clutter will be short lived, since you'll eventually default this on. -Ben On Tue, Jun 26, 2012 at 9:28 AM, Albert Bodenhamer <abodenha@chromium.org>wrote: > Actually, on second thought, a command line switch (or even a compile time > flag) makes more sense to me. This isn't something we need to clutter > chrome://flags with. > > > On Mon, Jun 25, 2012 at 5:16 PM, Albert Bodenhamer <abodenha@chromium.org>wrote: > >> Thanks. I'll rework this to put the changes behind a flag and resend it. >> >> >> On Mon, Jun 25, 2012 at 5:04 PM, Ben Goodger (Google) <ben@chromium.org>wrote: >> >>> Note also that behind a flag I will be more receptive to incremental >>> changes. >>> >>> -Ben >>> >>> >>> On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer < >>> abodenha@chromium.org> wrote: >>> >>>> Thanks Ben. >>>> >>>> I'll file a new bug on the more complete changes to ConstrainedWindow >>>> and mark it blocking 133363. >>>> >>>> On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) <ben@chromium.org >>>> > wrote: >>>> >>>>> OK I discussed this with Glen and I don't think this is ready to go >>>>> as-is. >>>>> >>>>> Here are the things I think you need to accomplish before this can be >>>>> turned on: >>>>> >>>>> - correct window shape (border radius etc) >>>>> - correct window background >>>>> - correct fonts, etc. >>>>> - screenshots of every constrained window with this new treatment. >>>>> - I don't believe you can implement the shadow, the animations or the >>>>> nice dimming using the infrastructure we have today. This will require the >>>>> Aura transition currently being planned, so I don't think any of that is a >>>>> requirement. The designers may want a stroke of some kind around the window >>>>> then in lieu of the shadow. >>>>> - I think this change will require some changes to how constrained >>>>> windows are parented. right now they're parented to the WebContentsView, >>>>> but as you can see in the new mockup they overlap with the browser UI (for >>>>> anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This >>>>> implies these dialogs are parented to the browser frame. Since they should >>>>> be tab modal, I do not want to see these dialogs parented to the desktop >>>>> (i.e. they should be WS_CHILD windows of the browser frame). >>>>> >>>>> I highly recommend pursuing this work behind a about:flags flag. My >>>>> prediction is it will take several weeks to get everything to look "right" >>>>> and in the interim we need to be shipping releases. >>>>> >>>>> -Ben >>>>> >>>>> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer < >>>>> abodenha@chromium.org> wrote: >>>>> >>>>>> >>>>>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>>>>> the ultimate goal. >>>>>> >>>>>> I haven't implemented the full spec in this CL. My goal was to get >>>>>> rid of the blue bar while moving closer to this. >>>>>> >>>>>> Specifically, I haven't addressed animation, styling of the drop >>>>>> shadow, rounding the lower corners of the dialog, background color, >>>>>> position, etc. >>>>>> >>>>>> >>>>>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) < >>>>>> ben@chromium.org> wrote: >>>>>> >>>>>>> And where's the mock from the UI designers? >>>>>>> >>>>>>> -Ben >>>>>>> >>>>>>> >>>>>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>>>>> abodenha@chromium.org> wrote: >>>>>>> >>>>>>>> Screen shots attached. The empty content in the "after" is because >>>>>>>> it's a local build and is unrelated to my change. >>>>>>>> >>>>>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>>>>> >>>>>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>>>>> http://www.pagetutor.com/**keeper/http_authentication/**index.html<http://www... a test case. >>>>>>>>> >>>>>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>>>>> >>>>>>>> >>>>>>>> Got it. I was misinformed. Someone thought it was part of an >>>>>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>>>>> the CL description. Thanks. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>> org >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>> org >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>> org >>>> >>> >>> >> >> >> -- >> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >> org >> > > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> > org >
Hard to say how long lived it would be. I've already invested more time in this than I intended and I don't think completely overhauling the dialogs is something I really want to own. My hope was get my change in as a starting point and have someone else finish it up. If that's not an option, I'll just shelve it for now. On Tue, Jun 26, 2012 at 9:45 AM, Ben Goodger (Google) <ben@chromium.org>wrote: > Don't over-think about:flags. It's not a user screen. It's useful to have > switches there especially on ChromeOS where it's non-trivial to run with a > different command line. > > Any clutter will be short lived, since you'll eventually default this on. > > -Ben > > > On Tue, Jun 26, 2012 at 9:28 AM, Albert Bodenhamer <abodenha@chromium.org>wrote: > >> Actually, on second thought, a command line switch (or even a compile >> time flag) makes more sense to me. This isn't something we need to clutter >> chrome://flags with. >> >> >> On Mon, Jun 25, 2012 at 5:16 PM, Albert Bodenhamer <abodenha@chromium.org >> > wrote: >> >>> Thanks. I'll rework this to put the changes behind a flag and resend it. >>> >>> >>> On Mon, Jun 25, 2012 at 5:04 PM, Ben Goodger (Google) <ben@chromium.org>wrote: >>> >>>> Note also that behind a flag I will be more receptive to incremental >>>> changes. >>>> >>>> -Ben >>>> >>>> >>>> On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer < >>>> abodenha@chromium.org> wrote: >>>> >>>>> Thanks Ben. >>>>> >>>>> I'll file a new bug on the more complete changes to ConstrainedWindow >>>>> and mark it blocking 133363. >>>>> >>>>> On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) < >>>>> ben@chromium.org> wrote: >>>>> >>>>>> OK I discussed this with Glen and I don't think this is ready to go >>>>>> as-is. >>>>>> >>>>>> Here are the things I think you need to accomplish before this can be >>>>>> turned on: >>>>>> >>>>>> - correct window shape (border radius etc) >>>>>> - correct window background >>>>>> - correct fonts, etc. >>>>>> - screenshots of every constrained window with this new treatment. >>>>>> - I don't believe you can implement the shadow, the animations or the >>>>>> nice dimming using the infrastructure we have today. This will require the >>>>>> Aura transition currently being planned, so I don't think any of that is a >>>>>> requirement. The designers may want a stroke of some kind around the window >>>>>> then in lieu of the shadow. >>>>>> - I think this change will require some changes to how constrained >>>>>> windows are parented. right now they're parented to the WebContentsView, >>>>>> but as you can see in the new mockup they overlap with the browser UI (for >>>>>> anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This >>>>>> implies these dialogs are parented to the browser frame. Since they should >>>>>> be tab modal, I do not want to see these dialogs parented to the desktop >>>>>> (i.e. they should be WS_CHILD windows of the browser frame). >>>>>> >>>>>> I highly recommend pursuing this work behind a about:flags flag. My >>>>>> prediction is it will take several weeks to get everything to look "right" >>>>>> and in the interim we need to be shipping releases. >>>>>> >>>>>> -Ben >>>>>> >>>>>> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer < >>>>>> abodenha@chromium.org> wrote: >>>>>> >>>>>>> >>>>>>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>>>>>> the ultimate goal. >>>>>>> >>>>>>> I haven't implemented the full spec in this CL. My goal was to get >>>>>>> rid of the blue bar while moving closer to this. >>>>>>> >>>>>>> Specifically, I haven't addressed animation, styling of the drop >>>>>>> shadow, rounding the lower corners of the dialog, background color, >>>>>>> position, etc. >>>>>>> >>>>>>> >>>>>>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) < >>>>>>> ben@chromium.org> wrote: >>>>>>> >>>>>>>> And where's the mock from the UI designers? >>>>>>>> >>>>>>>> -Ben >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>>>>>> abodenha@chromium.org> wrote: >>>>>>>> >>>>>>>>> Screen shots attached. The empty content in the "after" is >>>>>>>>> because it's a local build and is unrelated to my change. >>>>>>>>> >>>>>>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>>>>>> http://www.pagetutor.com/**keeper/http_authentication/** >>>>>>>>>> index.html<http://www.pagetutor.com/keeper/http_authentication/index.html>for a test case. >>>>>>>>>> >>>>>>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>>>>>> >>>>>>>>> >>>>>>>>> Got it. I was misinformed. Someone thought it was part of an >>>>>>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>>>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>>>>>> the CL description. Thanks. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>> org >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>> org >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>> org >>>>> >>>> >>>> >>> >>> >>> -- >>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>> org >>> >> >> >> >> -- >> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >> org >> > > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
As I said, overhauling the dialog style is on my list of things. I'll take any work you volunteer, though :) Rachel On Tue, Jun 26, 2012 at 9:52 AM, Albert Bodenhamer <abodenha@chromium.org>wrote: > Hard to say how long lived it would be. I've already invested more time > in this than I intended and I don't think completely overhauling the > dialogs is something I really want to own. My hope was get my change in as > a starting point and have someone else finish it up. If that's not an > option, I'll just shelve it for now. > > > On Tue, Jun 26, 2012 at 9:45 AM, Ben Goodger (Google) <ben@chromium.org>wrote: > >> Don't over-think about:flags. It's not a user screen. It's useful to have >> switches there especially on ChromeOS where it's non-trivial to run with a >> different command line. >> >> Any clutter will be short lived, since you'll eventually default this on. >> >> -Ben >> >> >> On Tue, Jun 26, 2012 at 9:28 AM, Albert Bodenhamer <abodenha@chromium.org >> > wrote: >> >>> Actually, on second thought, a command line switch (or even a compile >>> time flag) makes more sense to me. This isn't something we need to clutter >>> chrome://flags with. >>> >>> >>> On Mon, Jun 25, 2012 at 5:16 PM, Albert Bodenhamer < >>> abodenha@chromium.org> wrote: >>> >>>> Thanks. I'll rework this to put the changes behind a flag and resend >>>> it. >>>> >>>> >>>> On Mon, Jun 25, 2012 at 5:04 PM, Ben Goodger (Google) <ben@chromium.org >>>> > wrote: >>>> >>>>> Note also that behind a flag I will be more receptive to incremental >>>>> changes. >>>>> >>>>> -Ben >>>>> >>>>> >>>>> On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer < >>>>> abodenha@chromium.org> wrote: >>>>> >>>>>> Thanks Ben. >>>>>> >>>>>> I'll file a new bug on the more complete changes to ConstrainedWindow >>>>>> and mark it blocking 133363. >>>>>> >>>>>> On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) < >>>>>> ben@chromium.org> wrote: >>>>>> >>>>>>> OK I discussed this with Glen and I don't think this is ready to >>>>>>> go as-is. >>>>>>> >>>>>>> Here are the things I think you need to accomplish before this can >>>>>>> be turned on: >>>>>>> >>>>>>> - correct window shape (border radius etc) >>>>>>> - correct window background >>>>>>> - correct fonts, etc. >>>>>>> - screenshots of every constrained window with this new treatment. >>>>>>> - I don't believe you can implement the shadow, the animations or >>>>>>> the nice dimming using the infrastructure we have today. This will require >>>>>>> the Aura transition currently being planned, so I don't think any of that >>>>>>> is a requirement. The designers may want a stroke of some kind around the >>>>>>> window then in lieu of the shadow. >>>>>>> - I think this change will require some changes to how constrained >>>>>>> windows are parented. right now they're parented to the WebContentsView, >>>>>>> but as you can see in the new mockup they overlap with the browser UI (for >>>>>>> anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This >>>>>>> implies these dialogs are parented to the browser frame. Since they should >>>>>>> be tab modal, I do not want to see these dialogs parented to the desktop >>>>>>> (i.e. they should be WS_CHILD windows of the browser frame). >>>>>>> >>>>>>> I highly recommend pursuing this work behind a about:flags flag. My >>>>>>> prediction is it will take several weeks to get everything to look "right" >>>>>>> and in the interim we need to be shipping releases. >>>>>>> >>>>>>> -Ben >>>>>>> >>>>>>> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer < >>>>>>> abodenha@chromium.org> wrote: >>>>>>> >>>>>>>> >>>>>>>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>>>>>>> the ultimate goal. >>>>>>>> >>>>>>>> I haven't implemented the full spec in this CL. My goal was to get >>>>>>>> rid of the blue bar while moving closer to this. >>>>>>>> >>>>>>>> Specifically, I haven't addressed animation, styling of the drop >>>>>>>> shadow, rounding the lower corners of the dialog, background color, >>>>>>>> position, etc. >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) < >>>>>>>> ben@chromium.org> wrote: >>>>>>>> >>>>>>>>> And where's the mock from the UI designers? >>>>>>>>> >>>>>>>>> -Ben >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>>>>>>> abodenha@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> Screen shots attached. The empty content in the "after" is >>>>>>>>>> because it's a local build and is unrelated to my change. >>>>>>>>>> >>>>>>>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>>>>>>> >>>>>>>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>>>>>>> http://www.pagetutor.com/**keeper/http_authentication/** >>>>>>>>>>> index.html<http://www.pagetutor.com/keeper/http_authentication/index.html>for a test case. >>>>>>>>>>> >>>>>>>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Got it. I was misinformed. Someone thought it was part of an >>>>>>>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>>>>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>>>>>>> the CL description. Thanks. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>>> org >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>> org >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>> org >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>> org >>>> >>> >>> >>> >>> -- >>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>> org >>> >> >> > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> > org >
Thanks Rachel. I'd assumed you would be the one to finish things up, but I wasn't sure of your timetable. Would you prefer me to add the entry in about:flags and go ahead with this change now? Or should I just save it so you can use it as a reference when you start on this stuff? On Tue, Jun 26, 2012 at 10:10 AM, Rachel Blum <groby@chromium.org> wrote: > As I said, overhauling the dialog style is on my list of things. I'll take > any work you volunteer, though :) > > Rachel > > > On Tue, Jun 26, 2012 at 9:52 AM, Albert Bodenhamer <abodenha@chromium.org>wrote: > >> Hard to say how long lived it would be. I've already invested more time >> in this than I intended and I don't think completely overhauling the >> dialogs is something I really want to own. My hope was get my change in as >> a starting point and have someone else finish it up. If that's not an >> option, I'll just shelve it for now. >> >> >> On Tue, Jun 26, 2012 at 9:45 AM, Ben Goodger (Google) <ben@chromium.org>wrote: >> >>> Don't over-think about:flags. It's not a user screen. It's useful to >>> have switches there especially on ChromeOS where it's non-trivial to run >>> with a different command line. >>> >>> Any clutter will be short lived, since you'll eventually default this on. >>> >>> -Ben >>> >>> >>> On Tue, Jun 26, 2012 at 9:28 AM, Albert Bodenhamer < >>> abodenha@chromium.org> wrote: >>> >>>> Actually, on second thought, a command line switch (or even a compile >>>> time flag) makes more sense to me. This isn't something we need to clutter >>>> chrome://flags with. >>>> >>>> >>>> On Mon, Jun 25, 2012 at 5:16 PM, Albert Bodenhamer < >>>> abodenha@chromium.org> wrote: >>>> >>>>> Thanks. I'll rework this to put the changes behind a flag and resend >>>>> it. >>>>> >>>>> >>>>> On Mon, Jun 25, 2012 at 5:04 PM, Ben Goodger (Google) < >>>>> ben@chromium.org> wrote: >>>>> >>>>>> Note also that behind a flag I will be more receptive to incremental >>>>>> changes. >>>>>> >>>>>> -Ben >>>>>> >>>>>> >>>>>> On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer < >>>>>> abodenha@chromium.org> wrote: >>>>>> >>>>>>> Thanks Ben. >>>>>>> >>>>>>> I'll file a new bug on the more complete changes to >>>>>>> ConstrainedWindow and mark it blocking 133363. >>>>>>> >>>>>>> On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) < >>>>>>> ben@chromium.org> wrote: >>>>>>> >>>>>>>> OK I discussed this with Glen and I don't think this is ready to >>>>>>>> go as-is. >>>>>>>> >>>>>>>> Here are the things I think you need to accomplish before this can >>>>>>>> be turned on: >>>>>>>> >>>>>>>> - correct window shape (border radius etc) >>>>>>>> - correct window background >>>>>>>> - correct fonts, etc. >>>>>>>> - screenshots of every constrained window with this new treatment. >>>>>>>> - I don't believe you can implement the shadow, the animations or >>>>>>>> the nice dimming using the infrastructure we have today. This will require >>>>>>>> the Aura transition currently being planned, so I don't think any of that >>>>>>>> is a requirement. The designers may want a stroke of some kind around the >>>>>>>> window then in lieu of the shadow. >>>>>>>> - I think this change will require some changes to how constrained >>>>>>>> windows are parented. right now they're parented to the WebContentsView, >>>>>>>> but as you can see in the new mockup they overlap with the browser UI (for >>>>>>>> anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This >>>>>>>> implies these dialogs are parented to the browser frame. Since they should >>>>>>>> be tab modal, I do not want to see these dialogs parented to the desktop >>>>>>>> (i.e. they should be WS_CHILD windows of the browser frame). >>>>>>>> >>>>>>>> I highly recommend pursuing this work behind a about:flags flag. My >>>>>>>> prediction is it will take several weeks to get everything to look "right" >>>>>>>> and in the interim we need to be shipping releases. >>>>>>>> >>>>>>>> -Ben >>>>>>>> >>>>>>>> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer < >>>>>>>> abodenha@chromium.org> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>>>>>>>> the ultimate goal. >>>>>>>>> >>>>>>>>> I haven't implemented the full spec in this CL. My goal was to >>>>>>>>> get rid of the blue bar while moving closer to this. >>>>>>>>> >>>>>>>>> Specifically, I haven't addressed animation, styling of the drop >>>>>>>>> shadow, rounding the lower corners of the dialog, background color, >>>>>>>>> position, etc. >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) < >>>>>>>>> ben@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> And where's the mock from the UI designers? >>>>>>>>>> >>>>>>>>>> -Ben >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>>>>>>>> abodenha@chromium.org> wrote: >>>>>>>>>> >>>>>>>>>>> Screen shots attached. The empty content in the "after" is >>>>>>>>>>> because it's a local build and is unrelated to my change. >>>>>>>>>>> >>>>>>>>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>>>>>>>> >>>>>>>>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>>>>>>>> http://www.pagetutor.com/**keeper/http_authentication/** >>>>>>>>>>>> index.html<http://www.pagetutor.com/keeper/http_authentication/index.html>for a test case. >>>>>>>>>>>> >>>>>>>>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Got it. I was misinformed. Someone thought it was part of an >>>>>>>>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>>>>>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>>>>>>>> the CL description. Thanks. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>>>> org >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>> org >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>> org >>>>>>> >>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>> org >>>>> >>>> >>>> >>>> >>>> -- >>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>> org >>>> >>> >>> >> >> >> -- >> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >> org >> > > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
I'll be starting after the July 4th weekend, so having the flag in is probably not a bad idea - but I'm happy to pick up wherever. Thank you, Rachel On Tue, Jun 26, 2012 at 10:16 AM, Albert Bodenhamer <abodenha@chromium.org>wrote: > Thanks Rachel. I'd assumed you would be the one to finish things up, but > I wasn't sure of your timetable. > > Would you prefer me to add the entry in about:flags and go ahead with this > change now? Or should I just save it so you can use it as a reference when > you start on this stuff? > > > On Tue, Jun 26, 2012 at 10:10 AM, Rachel Blum <groby@chromium.org> wrote: > >> As I said, overhauling the dialog style is on my list of things. I'll >> take any work you volunteer, though :) >> >> Rachel >> >> >> On Tue, Jun 26, 2012 at 9:52 AM, Albert Bodenhamer <abodenha@chromium.org >> > wrote: >> >>> Hard to say how long lived it would be. I've already invested more time >>> in this than I intended and I don't think completely overhauling the >>> dialogs is something I really want to own. My hope was get my change in as >>> a starting point and have someone else finish it up. If that's not an >>> option, I'll just shelve it for now. >>> >>> >>> On Tue, Jun 26, 2012 at 9:45 AM, Ben Goodger (Google) <ben@chromium.org>wrote: >>> >>>> Don't over-think about:flags. It's not a user screen. It's useful to >>>> have switches there especially on ChromeOS where it's non-trivial to run >>>> with a different command line. >>>> >>>> Any clutter will be short lived, since you'll eventually default this >>>> on. >>>> >>>> -Ben >>>> >>>> >>>> On Tue, Jun 26, 2012 at 9:28 AM, Albert Bodenhamer < >>>> abodenha@chromium.org> wrote: >>>> >>>>> Actually, on second thought, a command line switch (or even a compile >>>>> time flag) makes more sense to me. This isn't something we need to clutter >>>>> chrome://flags with. >>>>> >>>>> >>>>> On Mon, Jun 25, 2012 at 5:16 PM, Albert Bodenhamer < >>>>> abodenha@chromium.org> wrote: >>>>> >>>>>> Thanks. I'll rework this to put the changes behind a flag and resend >>>>>> it. >>>>>> >>>>>> >>>>>> On Mon, Jun 25, 2012 at 5:04 PM, Ben Goodger (Google) < >>>>>> ben@chromium.org> wrote: >>>>>> >>>>>>> Note also that behind a flag I will be more receptive to incremental >>>>>>> changes. >>>>>>> >>>>>>> -Ben >>>>>>> >>>>>>> >>>>>>> On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer < >>>>>>> abodenha@chromium.org> wrote: >>>>>>> >>>>>>>> Thanks Ben. >>>>>>>> >>>>>>>> I'll file a new bug on the more complete changes to >>>>>>>> ConstrainedWindow and mark it blocking 133363. >>>>>>>> >>>>>>>> On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) < >>>>>>>> ben@chromium.org> wrote: >>>>>>>> >>>>>>>>> OK I discussed this with Glen and I don't think this is ready to >>>>>>>>> go as-is. >>>>>>>>> >>>>>>>>> Here are the things I think you need to accomplish before this can >>>>>>>>> be turned on: >>>>>>>>> >>>>>>>>> - correct window shape (border radius etc) >>>>>>>>> - correct window background >>>>>>>>> - correct fonts, etc. >>>>>>>>> - screenshots of every constrained window with this new treatment. >>>>>>>>> - I don't believe you can implement the shadow, the animations or >>>>>>>>> the nice dimming using the infrastructure we have today. This will require >>>>>>>>> the Aura transition currently being planned, so I don't think any of that >>>>>>>>> is a requirement. The designers may want a stroke of some kind around the >>>>>>>>> window then in lieu of the shadow. >>>>>>>>> - I think this change will require some changes to how constrained >>>>>>>>> windows are parented. right now they're parented to the WebContentsView, >>>>>>>>> but as you can see in the new mockup they overlap with the browser UI (for >>>>>>>>> anti-spoofing purposes, we'll call these the "Ian Fette Pixels"). This >>>>>>>>> implies these dialogs are parented to the browser frame. Since they should >>>>>>>>> be tab modal, I do not want to see these dialogs parented to the desktop >>>>>>>>> (i.e. they should be WS_CHILD windows of the browser frame). >>>>>>>>> >>>>>>>>> I highly recommend pursuing this work behind a about:flags flag. >>>>>>>>> My prediction is it will take several weeks to get everything to look >>>>>>>>> "right" and in the interim we need to be shipping releases. >>>>>>>>> >>>>>>>>> -Ben >>>>>>>>> >>>>>>>>> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer < >>>>>>>>> abodenha@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> >>>>>>>>>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>>>>>>>>> the ultimate goal. >>>>>>>>>> >>>>>>>>>> I haven't implemented the full spec in this CL. My goal was to >>>>>>>>>> get rid of the blue bar while moving closer to this. >>>>>>>>>> >>>>>>>>>> Specifically, I haven't addressed animation, styling of the drop >>>>>>>>>> shadow, rounding the lower corners of the dialog, background color, >>>>>>>>>> position, etc. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) < >>>>>>>>>> ben@chromium.org> wrote: >>>>>>>>>> >>>>>>>>>>> And where's the mock from the UI designers? >>>>>>>>>>> >>>>>>>>>>> -Ben >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>>>>>>>>> abodenha@chromium.org> wrote: >>>>>>>>>>> >>>>>>>>>>>> Screen shots attached. The empty content in the "after" is >>>>>>>>>>>> because it's a local build and is unrelated to my change. >>>>>>>>>>>> >>>>>>>>>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>>>>>>>>> http://www.pagetutor.com/**keeper/http_authentication/** >>>>>>>>>>>>> index.html<http://www.pagetutor.com/keeper/http_authentication/index.html>for a test case. >>>>>>>>>>>>> >>>>>>>>>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Got it. I was misinformed. Someone thought it was part of an >>>>>>>>>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>>>>>>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>>>>>>>>> the CL description. Thanks. >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>>>>> org >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>>> org >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>> org >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>> org >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>> org >>>>> >>>> >>>> >>> >>> >>> -- >>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>> org >>> >> >> > > > -- > Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> > org >
Ok. I'll try to wrap this up in the next day or two then. On Tue, Jun 26, 2012 at 10:18 AM, Rachel Blum <groby@chromium.org> wrote: > I'll be starting after the July 4th weekend, so having the flag in is > probably not a bad idea - but I'm happy to pick up wherever. > > Thank you, > Rachel > > > On Tue, Jun 26, 2012 at 10:16 AM, Albert Bodenhamer <abodenha@chromium.org > > wrote: > >> Thanks Rachel. I'd assumed you would be the one to finish things up, but >> I wasn't sure of your timetable. >> >> Would you prefer me to add the entry in about:flags and go ahead with >> this change now? Or should I just save it so you can use it as a reference >> when you start on this stuff? >> >> >> On Tue, Jun 26, 2012 at 10:10 AM, Rachel Blum <groby@chromium.org> wrote: >> >>> As I said, overhauling the dialog style is on my list of things. I'll >>> take any work you volunteer, though :) >>> >>> Rachel >>> >>> >>> On Tue, Jun 26, 2012 at 9:52 AM, Albert Bodenhamer < >>> abodenha@chromium.org> wrote: >>> >>>> Hard to say how long lived it would be. I've already invested more >>>> time in this than I intended and I don't think completely overhauling the >>>> dialogs is something I really want to own. My hope was get my change in as >>>> a starting point and have someone else finish it up. If that's not an >>>> option, I'll just shelve it for now. >>>> >>>> >>>> On Tue, Jun 26, 2012 at 9:45 AM, Ben Goodger (Google) <ben@chromium.org >>>> > wrote: >>>> >>>>> Don't over-think about:flags. It's not a user screen. It's useful to >>>>> have switches there especially on ChromeOS where it's non-trivial to run >>>>> with a different command line. >>>>> >>>>> Any clutter will be short lived, since you'll eventually default this >>>>> on. >>>>> >>>>> -Ben >>>>> >>>>> >>>>> On Tue, Jun 26, 2012 at 9:28 AM, Albert Bodenhamer < >>>>> abodenha@chromium.org> wrote: >>>>> >>>>>> Actually, on second thought, a command line switch (or even a compile >>>>>> time flag) makes more sense to me. This isn't something we need to clutter >>>>>> chrome://flags with. >>>>>> >>>>>> >>>>>> On Mon, Jun 25, 2012 at 5:16 PM, Albert Bodenhamer < >>>>>> abodenha@chromium.org> wrote: >>>>>> >>>>>>> Thanks. I'll rework this to put the changes behind a flag and >>>>>>> resend it. >>>>>>> >>>>>>> >>>>>>> On Mon, Jun 25, 2012 at 5:04 PM, Ben Goodger (Google) < >>>>>>> ben@chromium.org> wrote: >>>>>>> >>>>>>>> Note also that behind a flag I will be more receptive to >>>>>>>> incremental changes. >>>>>>>> >>>>>>>> -Ben >>>>>>>> >>>>>>>> >>>>>>>> On Mon, Jun 25, 2012 at 5:02 PM, Albert Bodenhamer < >>>>>>>> abodenha@chromium.org> wrote: >>>>>>>> >>>>>>>>> Thanks Ben. >>>>>>>>> >>>>>>>>> I'll file a new bug on the more complete changes to >>>>>>>>> ConstrainedWindow and mark it blocking 133363. >>>>>>>>> >>>>>>>>> On Mon, Jun 25, 2012 at 4:40 PM, Ben Goodger (Google) < >>>>>>>>> ben@chromium.org> wrote: >>>>>>>>> >>>>>>>>>> OK I discussed this with Glen and I don't think this is ready >>>>>>>>>> to go as-is. >>>>>>>>>> >>>>>>>>>> Here are the things I think you need to accomplish before this >>>>>>>>>> can be turned on: >>>>>>>>>> >>>>>>>>>> - correct window shape (border radius etc) >>>>>>>>>> - correct window background >>>>>>>>>> - correct fonts, etc. >>>>>>>>>> - screenshots of every constrained window with this new treatment. >>>>>>>>>> - I don't believe you can implement the shadow, the animations or >>>>>>>>>> the nice dimming using the infrastructure we have today. This will require >>>>>>>>>> the Aura transition currently being planned, so I don't think any of that >>>>>>>>>> is a requirement. The designers may want a stroke of some kind around the >>>>>>>>>> window then in lieu of the shadow. >>>>>>>>>> - I think this change will require some changes to how >>>>>>>>>> constrained windows are parented. right now they're parented to the >>>>>>>>>> WebContentsView, but as you can see in the new mockup they overlap with the >>>>>>>>>> browser UI (for anti-spoofing purposes, we'll call these the "Ian Fette >>>>>>>>>> Pixels"). This implies these dialogs are parented to the browser frame. >>>>>>>>>> Since they should be tab modal, I do not want to see these dialogs parented >>>>>>>>>> to the desktop (i.e. they should be WS_CHILD windows of the browser frame). >>>>>>>>>> >>>>>>>>>> I highly recommend pursuing this work behind a about:flags flag. >>>>>>>>>> My prediction is it will take several weeks to get everything to look >>>>>>>>>> "right" and in the interim we need to be shipping releases. >>>>>>>>>> >>>>>>>>>> -Ben >>>>>>>>>> >>>>>>>>>> On Fri, Jun 22, 2012 at 3:43 PM, Albert Bodenhamer < >>>>>>>>>> abodenha@chromium.org> wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> https://www.corp.google.com/~kenmoore/mocks/chromeos/Misc_2012/Dialogs/Markup... has >>>>>>>>>>> the ultimate goal. >>>>>>>>>>> >>>>>>>>>>> I haven't implemented the full spec in this CL. My goal was to >>>>>>>>>>> get rid of the blue bar while moving closer to this. >>>>>>>>>>> >>>>>>>>>>> Specifically, I haven't addressed animation, styling of the drop >>>>>>>>>>> shadow, rounding the lower corners of the dialog, background color, >>>>>>>>>>> position, etc. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Fri, Jun 22, 2012 at 3:19 PM, Ben Goodger (Google) < >>>>>>>>>>> ben@chromium.org> wrote: >>>>>>>>>>> >>>>>>>>>>>> And where's the mock from the UI designers? >>>>>>>>>>>> >>>>>>>>>>>> -Ben >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> On Fri, Jun 22, 2012 at 3:17 PM, Albert Bodenhamer < >>>>>>>>>>>> abodenha@chromium.org> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Screen shots attached. The empty content in the "after" is >>>>>>>>>>>>> because it's a local build and is unrelated to my change. >>>>>>>>>>>>> >>>>>>>>>>>>> On Fri, Jun 22, 2012 at 12:20 PM, <ben@chromium.org> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> login_prompt.cc is most certainly not deprecated... see >>>>>>>>>>>>>> http://www.pagetutor.com/**keeper/http_authentication/** >>>>>>>>>>>>>> index.html<http://www.pagetutor.com/keeper/http_authentication/index.html>for a test case. >>>>>>>>>>>>>> >>>>>>>>>>>>>> http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Got it. I was misinformed. Someone thought it was part of an >>>>>>>>>>>>> experiment they had run and needed to remove code for. Login_prompt.cc is >>>>>>>>>>>>> handling htaccess prompts I mentioned in the CL description. I'll correct >>>>>>>>>>>>> the CL description. Thanks. >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>>>>>> org >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>>>> org >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>>>> org >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>>> org >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>>>> org >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >>>> org >>>> >>> >>> >> >> >> -- >> Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> >> org >> > > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
I've put back all of the code I removed and added a new flag to control which type window frame constrained windows get. Please take another look.
Could I get folks to take a look at this in the next few days? Thanks. On Fri, Jun 29, 2012 at 4:42 PM, <abodenha@chromium.org> wrote: > I've put back all of the code I removed and added a new flag to control > which > type window frame constrained windows get. > > Please take another look. > > http://codereview.chromium.**org/10631010/<http://codereview.chromium.org/106... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
Still LGTM on picker side. (Although you might need to re-merge)
lgtm http://codereview.chromium.org/10631010/diff/31003/chrome/browser/ui/views/co... File chrome/browser/ui/views/constrained_window_frame_simple.h (right): http://codereview.chromium.org/10631010/diff/31003/chrome/browser/ui/views/co... chrome/browser/ui/views/constrained_window_frame_simple.h:20: virtual dtor http://codereview.chromium.org/10631010/diff/31003/chrome/browser/ui/views/co... chrome/browser/ui/views/constrained_window_frame_simple.h:21: // Overridden from views::NonClientFrameView: private:
http://codereview.chromium.org/10631010/diff/31003/chrome/browser/ui/views/co... File chrome/browser/ui/views/constrained_window_frame_simple.h (right): http://codereview.chromium.org/10631010/diff/31003/chrome/browser/ui/views/co... chrome/browser/ui/views/constrained_window_frame_simple.h:20: On 2012/07/12 15:49:04, Ben Goodger (Google) wrote: > virtual dtor Done. http://codereview.chromium.org/10631010/diff/31003/chrome/browser/ui/views/co... chrome/browser/ui/views/constrained_window_frame_simple.h:21: // Overridden from views::NonClientFrameView: On 2012/07/12 15:49:04, Ben Goodger (Google) wrote: > private: Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/abodenha@chromium.org/10631010/32015
Change committed as 146466 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
