Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2654323002:
harmony: convert device chooser (Closed)
|
Created:
3 years, 11 months ago by Elly Fong-Jones Modified:
3 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, tfarina, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionharmony: convert device chooser
The device chooser now uses Harmony layout constants throughout.
BUG=610430
Review-Url: https://codereview.chromium.org/2654323002
Cr-Commit-Position: refs/heads/master@{#449330}
Committed: https://chromium.googlesource.com/chromium/src/+/311566f4127c78cc085c99e52833592c673fed01
Patch Set 1 #Patch Set 2 : git cl format #Patch Set 3 : fix dialog width #
Total comments: 4
Patch Set 4 : fixes #Patch Set 5 : attempt size hack #
Total comments: 8
Patch Set 6 : move minimum size into views #
Total comments: 6
Patch Set 7 : nits #
Total comments: 2
Patch Set 8 : s/gfx::Size()// #
Messages
Total messages: 23 (8 generated)
Description was changed from ========== harmony: convert device chooser The device chooser now uses Harmony layout constants throughout. BUG=610430 ========== to ========== harmony: convert device chooser The device chooser now uses Harmony layout constants throughout. BUG=610430 ==========
ellyjones@chromium.org changed reviewers: + benwells@chromium.org
benwells@: ptal? :)
lgtm
ellyjones@chromium.org changed reviewers: + pkasting@chromium.org
pkasting: ptal? :)
https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/device_chooser_content_view.h (right): https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/device_chooser_content_view.h:37: void SetPreferredSize(gfx::Size size); Nit: const ref This can just be named/inlined as a cheap setter: void set_preferred_size(const gfx::Size& preferred_size) { preferred_size_ = preferred_size; } https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right): https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:30: // variable. Unless the function of the buttons have changed, they're either related or they're not -- they're not related in pre-Harmony but unrelated in Harmony. It seems like the issue is that we made rules about the spacing for different relationships, but then we positioned things in ways that violate those rules. We shouldn't hack around this by choosing different semantic meanings to get the result we want. I don't have a strong opinion about what "related" should mean, but we should decide what it means, and then apply that meaning here. If that means changing to "related" but we don't like how that works in Harmony, then we need to either change the value of RELATED_CONTROL_VERTICAL_SPACING, or we need to have constants that reflect the semantic distinction between the types of relationships we want to have different positions. (This is also true of the other place already in the codebase that does a similar hack.) https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:62: if (delegate->IsHarmonyMode()) { I'd like to avoid IsHarmonyMode() here. The first question is whether the size of this dialog is properly determined here or in DeviceChooserContentView. I suspect the answer is "here", so the existing default size in that class should be moved over to here. Along the way, the code should probably be changed so that content view has no preferred size; rather, it is laid out as taking the full width (minus insets) of the parent dialog, and the dialog itself has some kind of desired size. Then we can just compute the width as "the width the layout delegate says to use, if nonzero; otherwise, the old hardcoded constant". https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:70: 2 * inset_width); How come this is 2 * inset_width instead of just inset_width?
On 2017/02/01 02:39:26, Peter Kasting wrote: > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/device_chooser_content_view.h (right): > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/device_chooser_content_view.h:37: void > SetPreferredSize(gfx::Size size); > Nit: const ref > > This can just be named/inlined as a cheap setter: > > void set_preferred_size(const gfx::Size& preferred_size) { > preferred_size_ = preferred_size; > } Done. > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right): > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:30: // variable. > Unless the function of the buttons have changed, they're either related or > they're not -- they're not related in pre-Harmony but unrelated in Harmony. > > It seems like the issue is that we made rules about the spacing for different > relationships, but then we positioned things in ways that violate those rules. > We shouldn't hack around this by choosing different semantic meanings to get the > result we want. Alright. In that case, we should probably introduce DIALOG_CONTENT_BUTTONS_SPACING or similar, because that's what changed - dialog buttons are considered "unrelated" to the content view in Harmony, but were related before. We could introduce that and give it the values of RELATED_CONTROL_VERTICAL_SPACING if not Harmony and UNRELATED_CONTROL_VERTICAL_SPACING if Harmony, inside the delegate. I'll do that here. > I don't have a strong opinion about what "related" should mean, but we should > decide what it means, and then apply that meaning here. If that means changing > to "related" but we don't like how that works in Harmony, then we need to either > change the value of RELATED_CONTROL_VERTICAL_SPACING, or we need to have > constants that reflect the semantic distinction between the types of > relationships we want to have different positions. I ended up doing the latter. > (This is also true of the other place already in the codebase that does a > similar hack.) The other places that does this hack (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/collected_cookie...) did really change the distance from "related" to "unrelated" in redoing the dialog, so I'm not sure we should use the same fix. In any case, this isn't the margin between a content view and the dialog buttons, but some other kind of distance. :( > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:62: if > (delegate->IsHarmonyMode()) { > I'd like to avoid IsHarmonyMode() here. > > The first question is whether the size of this dialog is properly determined > here or in DeviceChooserContentView. I suspect the answer is "here", so the > existing default size in that class should be moved over to here. Along the > way, the code should probably be changed so that content view has no preferred > size; rather, it is laid out as taking the full width (minus insets) of the > parent dialog, and the dialog itself has some kind of desired size. I'd like to avoid it too. The reason I did this this way is that DeviceChooserContentView is also used in ChooserBubbleUiView: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/website_settings... and I can't for the life of me figure out how to make sure I haven't broken that code. I can't even find any uses of it so perhaps it's dead? > Then we can just compute the width as "the width the layout delegate says to > use, if nonzero; otherwise, the old hardcoded constant". > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:70: 2 * inset_width); > How come this is 2 * inset_width instead of just inset_width? Good question! Because it is wrong. Fixed.
Small request! If you can, reply to the comments directly in the Rietveld commenting interface rather than via email/replying in the Rietveld comment box, since the latter cases don't get associated back onto the files in question, and when spelunking later to understand discussion, it makes it harder to trace through. On 2017/02/02 23:06:39, Elly Fong-Jones wrote: > On 2017/02/01 02:39:26, Peter Kasting wrote: > > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:30: // variable. > > Unless the function of the buttons have changed, they're either related or > > they're not -- they're not related in pre-Harmony but unrelated in Harmony. > > > > It seems like the issue is that we made rules about the spacing for different > > relationships, but then we positioned things in ways that violate those rules. > > > We shouldn't hack around this by choosing different semantic meanings to get > the > > result we want. > > Alright. In that case, we should probably introduce > DIALOG_CONTENT_BUTTONS_SPACING or similar, because that's what changed - dialog > buttons are considered "unrelated" to the content view in Harmony, but were > related before. We could introduce that and give it the values of > RELATED_CONTROL_VERTICAL_SPACING if not Harmony and > UNRELATED_CONTROL_VERTICAL_SPACING if Harmony, inside the delegate. I'll do that > here. So, to make sure I understand, we're changing the vertical space between the row of buttons at the bottom of a dialog and the bottom of the content just above it? TBH, I think we should just make that "unrelated" in the pre-Harmony world too. "Related" controls, as I understood the concept, were originally supposed to be things like sets of checkboxes or something, not separate buttons that applied to a whole area above. This would be a functional change for pre-Harmony that I think would be semantically correct. > > (This is also true of the other place already in the codebase that does a > > similar hack.) > > The other places that does this hack > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/collected_cookie...) > did really change the distance from "related" to "unrelated" in redoing the > dialog, so I'm not sure we should use the same fix. In any case, this isn't the > margin between a content view and the dialog buttons, but some other kind of > distance. :( This one is a little more closely "related" than the "row of buttons below a dialog" case, as it's a button set that is directly connected to a box that controls how the button set behaves. I still think we should pick one of "related" or "unrelated" and make both things work the same way here, and we should do it based on the semantic relationship we think these have rather than the way we want things to look. My instinct is to make this "unrelated" for pre-Harmony as well, but I could be convinced otherwise. Doesn't need to be fixed in this CL, obviously. > > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:62: if > > (delegate->IsHarmonyMode()) { > > I'd like to avoid IsHarmonyMode() here. > > > > The first question is whether the size of this dialog is properly determined > > here or in DeviceChooserContentView. I suspect the answer is "here", so the > > existing default size in that class should be moved over to here. Along the > > way, the code should probably be changed so that content view has no preferred > > size; rather, it is laid out as taking the full width (minus insets) of the > > parent dialog, and the dialog itself has some kind of desired size. > > I'd like to avoid it too. The reason I did this this way is that > DeviceChooserContentView is also used in ChooserBubbleUiView: > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/website_settings... > and I can't for the life of me figure out how to make sure I haven't broken that > code. I can't even find any uses of it so perhaps it's dead? Doesn't look dead. Looks like it's used for WebUSB and Bluetooth. I think https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?l=1337 and https://cs.chromium.org/chromium/src/chrome/browser/usb/web_usb_chooser_servi... will both end up reaching this. Maybe find out how (or hack in a way) to spawn those dialogs?
On 2017/02/02 23:22:09, Peter Kasting wrote: > Small request! If you can, reply to the comments directly in the Rietveld > commenting interface rather than via email/replying in the Rietveld comment box, > since the latter cases don't get associated back onto the files in question, and > when spelunking later to understand discussion, it makes it harder to trace > through. Okay. I don't usually use Reitveld's comment-by-comment UI, but I can start. > On 2017/02/02 23:06:39, Elly Fong-Jones wrote: > > On 2017/02/01 02:39:26, Peter Kasting wrote: > > > > > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > > > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:30: // variable. > > > Unless the function of the buttons have changed, they're either related or > > > they're not -- they're not related in pre-Harmony but unrelated in Harmony. > > > > > > It seems like the issue is that we made rules about the spacing for > different > > > relationships, but then we positioned things in ways that violate those > rules. > > > > > We shouldn't hack around this by choosing different semantic meanings to get > > the > > > result we want. > > > > Alright. In that case, we should probably introduce > > DIALOG_CONTENT_BUTTONS_SPACING or similar, because that's what changed - > dialog > > buttons are considered "unrelated" to the content view in Harmony, but were > > related before. We could introduce that and give it the values of > > RELATED_CONTROL_VERTICAL_SPACING if not Harmony and > > UNRELATED_CONTROL_VERTICAL_SPACING if Harmony, inside the delegate. I'll do > that > > here. > > So, to make sure I understand, we're changing the vertical space between the row > of buttons at the bottom of a dialog and the bottom of the content just above > it? > > TBH, I think we should just make that "unrelated" in the pre-Harmony world too. > "Related" controls, as I understood the concept, were originally supposed to be > things like sets of checkboxes or something, not separate buttons that applied > to a whole area above. This would be a functional change for pre-Harmony that I > think would be semantically correct. Alright, as long as we are cool with changing the pre-Harmony UI. I have done this. > > > (This is also true of the other place already in the codebase that does a > > > similar hack.) > > > > The other places that does this hack > > > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/collected_cookie...) > > did really change the distance from "related" to "unrelated" in redoing the > > dialog, so I'm not sure we should use the same fix. In any case, this isn't > the > > margin between a content view and the dialog buttons, but some other kind of > > distance. :( > > This one is a little more closely "related" than the "row of buttons below a > dialog" case, as it's a button set that is directly connected to a box that > controls how the button set behaves. I still think we should pick one of > "related" or "unrelated" and make both things work the same way here, and we > should do it based on the semantic relationship we think these have rather than > the way we want things to look. My instinct is to make this "unrelated" for > pre-Harmony as well, but I could be convinced otherwise. > > Doesn't need to be fixed in this CL, obviously. Done in this CL, while it is fresh in my mind. > > > > > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > > > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:62: if > > > (delegate->IsHarmonyMode()) { > > > I'd like to avoid IsHarmonyMode() here. > > > > > > The first question is whether the size of this dialog is properly determined > > > here or in DeviceChooserContentView. I suspect the answer is "here", so the > > > existing default size in that class should be moved over to here. Along the > > > way, the code should probably be changed so that content view has no > preferred > > > size; rather, it is laid out as taking the full width (minus insets) of the > > > parent dialog, and the dialog itself has some kind of desired size. > > > > I'd like to avoid it too. The reason I did this this way is that > > DeviceChooserContentView is also used in ChooserBubbleUiView: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/website_settings... > > and I can't for the life of me figure out how to make sure I haven't broken > that > > code. I can't even find any uses of it so perhaps it's dead? > > Doesn't look dead. Looks like it's used for WebUSB and Bluetooth. I think > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?l=1337 and > https://cs.chromium.org/chromium/src/chrome/browser/usb/web_usb_chooser_servi... > will both end up reaching this. Maybe find out how (or hack in a way) to spawn > those dialogs? I have a working hack for displaying those dialogs, but I haven't actually done the size change yet. For posterity, it is: 1) Start chrome with --enable-experimental-web-platform-features 2) Navigate to this page on a secure origin (file:// works well): """ <html> <head> <script> function go() { var e = document.getElementById("result"); navigator.usb.requestDevice({ filters: [{ vendorId: 1452 /* apple */ }] }) .then(device => { e.innerText = "ok: " + device.productName; }) .catch(error => { e.innerText = "error: " + error; }); } </script> </head> <body> <button onclick="go();">Click</button> <div id="result"></div> </body> </html> """ 3) Click the button on the page. I will upload the new version shortly with the two changes described above and the sizing logic moved out of DeviceChooserContentView.
On 2017/02/03 17:37:15, Elly Fong-Jones wrote: > On 2017/02/02 23:22:09, Peter Kasting wrote: > > Small request! If you can, reply to the comments directly in the Rietveld > > commenting interface rather than via email/replying in the Rietveld comment > box, > > since the latter cases don't get associated back onto the files in question, > and > > when spelunking later to understand discussion, it makes it harder to trace > > through. > > Okay. I don't usually use Reitveld's comment-by-comment UI, but I can start. > > > On 2017/02/02 23:06:39, Elly Fong-Jones wrote: > > > On 2017/02/01 02:39:26, Peter Kasting wrote: > > > > > > > > > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > > > > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:30: // variable. > > > > Unless the function of the buttons have changed, they're either related or > > > > they're not -- they're not related in pre-Harmony but unrelated in > Harmony. > > > > > > > > It seems like the issue is that we made rules about the spacing for > > different > > > > relationships, but then we positioned things in ways that violate those > > rules. > > > > > > > We shouldn't hack around this by choosing different semantic meanings to > get > > > the > > > > result we want. > > > > > > Alright. In that case, we should probably introduce > > > DIALOG_CONTENT_BUTTONS_SPACING or similar, because that's what changed - > > dialog > > > buttons are considered "unrelated" to the content view in Harmony, but were > > > related before. We could introduce that and give it the values of > > > RELATED_CONTROL_VERTICAL_SPACING if not Harmony and > > > UNRELATED_CONTROL_VERTICAL_SPACING if Harmony, inside the delegate. I'll do > > that > > > here. > > > > So, to make sure I understand, we're changing the vertical space between the > row > > of buttons at the bottom of a dialog and the bottom of the content just above > > it? > > > > TBH, I think we should just make that "unrelated" in the pre-Harmony world > too. > > "Related" controls, as I understood the concept, were originally supposed to > be > > things like sets of checkboxes or something, not separate buttons that applied > > to a whole area above. This would be a functional change for pre-Harmony that > I > > think would be semantically correct. > > Alright, as long as we are cool with changing the pre-Harmony UI. I have done > this. > > > > > (This is also true of the other place already in the codebase that does a > > > > similar hack.) > > > > > > The other places that does this hack > > > > > > (https://cs.chromium.org/chromium/src/chrome/browser/ui/views/collected_cookie...) > > > did really change the distance from "related" to "unrelated" in redoing the > > > dialog, so I'm not sure we should use the same fix. In any case, this isn't > > the > > > margin between a content view and the dialog buttons, but some other kind of > > > distance. :( > > > > This one is a little more closely "related" than the "row of buttons below a > > dialog" case, as it's a button set that is directly connected to a box that > > controls how the button set behaves. I still think we should pick one of > > "related" or "unrelated" and make both things work the same way here, and we > > should do it based on the semantic relationship we think these have rather > than > > the way we want things to look. My instinct is to make this "unrelated" for > > pre-Harmony as well, but I could be convinced otherwise. > > > > Doesn't need to be fixed in this CL, obviously. > > Done in this CL, while it is fresh in my mind. > > > > > > > > > > > https://codereview.chromium.org/2654323002/diff/40001/chrome/browser/ui/views... > > > > chrome/browser/ui/views/extensions/chooser_dialog_view.cc:62: if > > > > (delegate->IsHarmonyMode()) { > > > > I'd like to avoid IsHarmonyMode() here. > > > > > > > > The first question is whether the size of this dialog is properly > determined > > > > here or in DeviceChooserContentView. I suspect the answer is "here", so > the > > > > existing default size in that class should be moved over to here. Along > the > > > > way, the code should probably be changed so that content view has no > > preferred > > > > size; rather, it is laid out as taking the full width (minus insets) of > the > > > > parent dialog, and the dialog itself has some kind of desired size. > > > > > > I'd like to avoid it too. The reason I did this this way is that > > > DeviceChooserContentView is also used in ChooserBubbleUiView: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/website_settings... > > > and I can't for the life of me figure out how to make sure I haven't broken > > that > > > code. I can't even find any uses of it so perhaps it's dead? > > > > Doesn't look dead. Looks like it's used for WebUSB and Bluetooth. I think > > https://cs.chromium.org/chromium/src/chrome/browser/ui/browser.cc?l=1337 and > > > https://cs.chromium.org/chromium/src/chrome/browser/usb/web_usb_chooser_servi... > > will both end up reaching this. Maybe find out how (or hack in a way) to > spawn > > those dialogs? > > I have a working hack for displaying those dialogs, but I haven't actually done > the size change yet. For posterity, it is: > > 1) Start chrome with --enable-experimental-web-platform-features > 2) Navigate to this page on a secure origin (file:// works well): > """ > <html> > <head> > <script> > function go() { > var e = document.getElementById("result"); > navigator.usb.requestDevice({ filters: [{ vendorId: 1452 /* apple */ }] }) > .then(device => { > e.innerText = "ok: " + device.productName; > }) > .catch(error => { e.innerText = "error: " + error; }); > } > </script> > </head> > <body> > <button onclick="go();">Click</button> > <div id="result"></div> > </body> > </html> > """ > 3) Click the button on the page. > > I will upload the new version shortly with the two changes described above and > the sizing logic moved out of DeviceChooserContentView. I tried moving the sizing logic. If you are okay with this approach, I will apply the same change to the other users of DeviceChooserContentView. What do you think?
This is definitely an improvement. https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right): https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:76: const int kMinWidth = 402; Nit: constexpr https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:102: delegate->GetMetric(LayoutDelegate::Metric::PANEL_CONTENT_MARGIN), Nit: Factor to a temp https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/sized_dialog_client_view.h (right): https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/sized_dialog_client_view.h:12: // dialog, independent of the preferred sizes of any subviews. Ultimately, will there be dialogs that _don't_ want this? It seems like we want to get to a world where DialogClientView does this by default. If that's true, is there a reason not to initially implement there directly, and to have this class instead as a transitionary thing? https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/sized_dialog_client_view.h:25: }; Nit: DISALLOW_COPY_AND_ASSIGN
pkasting: ptal? :) https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right): https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:76: const int kMinWidth = 402; On 2017/02/07 00:17:15, Peter Kasting wrote: > Nit: constexpr Done. https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:102: delegate->GetMetric(LayoutDelegate::Metric::PANEL_CONTENT_MARGIN), On 2017/02/07 00:17:15, Peter Kasting wrote: > Nit: Factor to a temp switched to the 1-arg form of gfx::Insets() instead https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/harmony/sized_dialog_client_view.h (right): https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/sized_dialog_client_view.h:12: // dialog, independent of the preferred sizes of any subviews. On 2017/02/07 00:17:15, Peter Kasting wrote: > Ultimately, will there be dialogs that _don't_ want this? It seems like we want > to get to a world where DialogClientView does this by default. > > If that's true, is there a reason not to initially implement there directly, and > to have this class instead as a transitionary thing? Hmmm, probably not, actually. Moved this logic to DialogClientView and removed this class. https://codereview.chromium.org/2654323002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/harmony/sized_dialog_client_view.h:25: }; On 2017/02/07 00:17:15, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN Acknowledged.
LGTM https://codereview.chromium.org/2654323002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right): https://codereview.chromium.org/2654323002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:84: gfx::Size minimum_size(min_width, kMinHeight); Nit: Can inline into next statement https://codereview.chromium.org/2654323002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2654323002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:113: gfx::Size minimum_size_ = gfx::Size(); Nit: I like initialization in the header, but I'm not as big a fan of doing it for just one member and not all of them. I'd rather either init this in the constructor or else init everything (reasonable) in the header here now. https://codereview.chromium.org/2654323002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:117: void SetupFocusChain(); Nit: While here, can you move this above the member variables?
ellyjones@chromium.org changed reviewers: + sky@chromium.org
sky: ptal for ui/views/*? :) https://codereview.chromium.org/2654323002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/extensions/chooser_dialog_view.cc (right): https://codereview.chromium.org/2654323002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/extensions/chooser_dialog_view.cc:84: gfx::Size minimum_size(min_width, kMinHeight); On 2017/02/07 23:24:44, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: Can inline into next statement Done. https://codereview.chromium.org/2654323002/diff/100001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2654323002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:113: gfx::Size minimum_size_ = gfx::Size(); On 2017/02/07 23:24:44, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: I like initialization in the header, but I'm not as big a fan of doing it > for just one member and not all of them. I'd rather either init this in the > constructor or else init everything (reasonable) in the header here now. Moved them all to this header. https://codereview.chromium.org/2654323002/diff/100001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:117: void SetupFocusChain(); On 2017/02/07 23:24:44, Peter Kasting (OOO Feb. 8-9) wrote: > Nit: While here, can you move this above the member variables? Done.
LGTM https://codereview.chromium.org/2654323002/diff/120001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2654323002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:116: // view. If this is equal to gfx::Size(), no minimum size is imposed. optional: I would remove the last sentence as an empty size is the smallest size, so the comment isn't really applicable. https://codereview.chromium.org/2654323002/diff/120001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:117: gfx::Size minimum_size_ = gfx::Size(); Remove the = here. The empty constructor is the same.
The CQ bit was checked by ellyjones@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from benwells@chromium.org, pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2654323002/#ps140001 (title: "s/gfx::Size()//")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486657026459180,
"parent_rev": "4cf935f5263a8ba2f4f69341085ef268b6c00617", "commit_rev":
"311566f4127c78cc085c99e52833592c673fed01"}
Message was sent while issue was closed.
Description was changed from ========== harmony: convert device chooser The device chooser now uses Harmony layout constants throughout. BUG=610430 ========== to ========== harmony: convert device chooser The device chooser now uses Harmony layout constants throughout. BUG=610430 Review-Url: https://codereview.chromium.org/2654323002 Cr-Commit-Position: refs/heads/master@{#449330} Committed: https://chromium.googlesource.com/chromium/src/+/311566f4127c78cc085c99e52833... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/311566f4127c78cc085c99e52833... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
