Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2815303004:
Update the Windows iOS promotion to have the same width as the bubble that appears before it. (Closed)
DescriptionUpdate the Windows iOS promotion so it has the same width as the bubble
that appears before it.
This is needed as pre-harmony because there is no fixed width for all bubbles.
BUG=700759
Review-Url: https://codereview.chromium.org/2815303004
Cr-Commit-Position: refs/heads/master@{#465398}
Committed: https://chromium.googlesource.com/chromium/src/+/218d4bbf6ae8e118ee2bd86a9b1761786ccd13b9
Patch Set 1 : Same width bubble #
Total comments: 10
Patch Set 2 : Comments #
Total comments: 2
Patch Set 3 : address comments #Patch Set 4 : merge #
Messages
Total messages: 30 (18 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
mrefaat@chromium.org changed reviewers: + sky@chromium.org, vasilii@chromium.org
vasilii@ for password changes sky@ for all the remaining files.
https://codereview.chromium.org/2815303004/diff/40001/chrome/app/resources/lo... File chrome/app/resources/locale_settings.grd (right): https://codereview.chromium.org/2815303004/diff/40001/chrome/app/resources/lo... chrome/app/resources/locale_settings.grd:165: <!-- The width of the Text on the Desktop iOS promotion bubble. --> This is the same description as line 160. https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:29: #include "ui/views/widget/widget.h" Code in c/b/ui should not depend upon views. Code in c/b/ui/views can. https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc (right): https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:87: old_bounds.set_height( Can't this code call GetBubbleBounds() directly rather than passing in the bounds? https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:90: GetWidget()->GetRootView()->Layout(); I wouldn't expect you to need an explicit layout here. Are you sure it's necessary?
https://codereview.chromium.org/2815303004/diff/40001/chrome/app/resources/lo... File chrome/app/resources/locale_settings.grd (right): https://codereview.chromium.org/2815303004/diff/40001/chrome/app/resources/lo... chrome/app/resources/locale_settings.grd:165: <!-- The width of the Text on the Desktop iOS promotion bubble. --> On 2017/04/17 17:21:55, sky wrote: > This is the same description as line 160. copy paste error/ fixing. https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/deskt... File chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc (right): https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/deskt... chrome/browser/ui/desktop_ios_promotion/desktop_ios_promotion_util.cc:29: #include "ui/views/widget/widget.h" On 2017/04/17 17:21:55, sky wrote: > Code in c/b/ui should not depend upon views. Code in c/b/ui/views can. Done. https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc (right): https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:87: old_bounds.set_height( On 2017/04/17 17:21:55, sky wrote: > Can't this code call GetBubbleBounds() directly rather than passing in the > bounds? Bubble bounds may change on layout, so i need to keep the old bounds before the layout. https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:90: GetWidget()->GetRootView()->Layout(); On 2017/04/17 17:21:55, sky wrote: > I wouldn't expect you to need an explicit layout here. Are you sure it's > necessary? I removed it it didn't affect the results, Does that mean that when set bounds is done it triggers layout ?
https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc (right): https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:87: old_bounds.set_height( On 2017/04/17 19:10:49, mrefaat wrote: > On 2017/04/17 17:21:55, sky wrote: > > Can't this code call GetBubbleBounds() directly rather than passing in the > > bounds? > > Bubble bounds may change on layout, so i need to keep the old bounds before the > layout. Good point. I think you want GetWindowBoundsInScreen() here.
https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc (right): https://codereview.chromium.org/2815303004/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/desktop_ios_promotion/desktop_ios_promotion_bubble_view.cc:87: old_bounds.set_height( On 2017/04/17 22:46:12, sky wrote: > On 2017/04/17 19:10:49, mrefaat wrote: > > On 2017/04/17 17:21:55, sky wrote: > > > Can't this code call GetBubbleBounds() directly rather than passing in the > > > bounds? > > > > Bubble bounds may change on layout, so i need to keep the old bounds before > the > > layout. > > Good point. I think you want GetWindowBoundsInScreen() here. Not sure what is the difference, i think getbubble bounds will also get me the same thing.
GetWindowBoundsInScreen() returns the bounds of the widget, which should not have changed. GetBubbleBounds() queries for the preferred size. On Mon, Apr 17, 2017 at 4:00 PM, <mrefaat@chromium.org> wrote: > > https://codereview.chromium.org/2815303004/diff/40001/ > chrome/browser/ui/views/desktop_ios_promotion/desktop_ > ios_promotion_bubble_view.cc > File > chrome/browser/ui/views/desktop_ios_promotion/desktop_ > ios_promotion_bubble_view.cc > (right): > > https://codereview.chromium.org/2815303004/diff/40001/ > chrome/browser/ui/views/desktop_ios_promotion/desktop_ > ios_promotion_bubble_view.cc#newcode87 > chrome/browser/ui/views/desktop_ios_promotion/desktop_ > ios_promotion_bubble_view.cc:87: > old_bounds.set_height( > On 2017/04/17 22:46:12, sky wrote: > > On 2017/04/17 19:10:49, mrefaat wrote: > > > On 2017/04/17 17:21:55, sky wrote: > > > > Can't this code call GetBubbleBounds() directly rather than > passing in the > > > > bounds? > > > > > > Bubble bounds may change on layout, so i need to keep the old bounds > before > > the > > > layout. > > > > Good point. I think you want GetWindowBoundsInScreen() here. > > Not sure what is the difference, i think getbubble bounds will also get > me the same thing. > > https://codereview.chromium.org/2815303004/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I didn't understand the description of the CL. https://codereview.chromium.org/2815303004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2815303004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:845: GetWidget()->GetRootView()->Layout(); Is it needed?
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
Description was changed from ========== Make the Windows iOS have the same width as the bubble that appears before it. This is needed as preharmony there is no fixed width for all bubbles. BUG=700759 ========== to ========== Update the Windows iOS promotion so it has the same width as the bubble that appears before it. This is needed as pre-harmony because there is no fixed width for all bubbles. BUG=700759 ==========
sky@: thanks for the explanation - used the WindowBoundsInScreen. vasilli@: updated the description. https://codereview.chromium.org/2815303004/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc (right): https://codereview.chromium.org/2815303004/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc:845: GetWidget()->GetRootView()->Layout(); On 2017/04/18 14:44:25, vasilii wrote: > Is it needed? removed.
The title is to be updated too. chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc lgtm
LGTM
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mrefaat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vasilii@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2815303004/#ps140001 (title: "merge")
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": 1492554060610920,
"parent_rev": "755180d8641630a1141fc9a272839b8dee1352a0", "commit_rev":
"218d4bbf6ae8e118ee2bd86a9b1761786ccd13b9"}
Message was sent while issue was closed.
Description was changed from ========== Update the Windows iOS promotion so it has the same width as the bubble that appears before it. This is needed as pre-harmony because there is no fixed width for all bubbles. BUG=700759 ========== to ========== Update the Windows iOS promotion so it has the same width as the bubble that appears before it. This is needed as pre-harmony because there is no fixed width for all bubbles. BUG=700759 Review-Url: https://codereview.chromium.org/2815303004 Cr-Commit-Position: refs/heads/master@{#465398} Committed: https://chromium.googlesource.com/chromium/src/+/218d4bbf6ae8e118ee2bd86a9b17... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:140001) as https://chromium.googlesource.com/chromium/src/+/218d4bbf6ae8e118ee2bd86a9b17... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
