Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2808283003:
Fix Bookmarks bubble title misalignment. (Closed)
DescriptionFix Bookmarks bubble title misalignment.
BUG=710212
Patch Set 1 #
Total comments: 4
Depends on Patchset: Messages
Total messages: 10 (3 generated)
pkasting@chromium.org changed reviewers: + pkasting@chromium.org
Note: if you want something reviewed, the preferred way is to add a reviewer on the CL, then "publish + mail" to notify them of your request. Mentioning the CL on a bug is more likely to get lost and less clear about who's to review. https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:312: side_margin, 0, side_margin)); Are the title and content misaligned on other similar dialogs? If so, this fix doesn't seem correct because it only adjusts bookmarks. If not, why are other places correct but this one isn't?
On 2017/04/11 20:26:29, Peter Kasting wrote: > Note: if you want something reviewed, the preferred way is to add a reviewer on > the CL, then "publish + mail" to notify them of your request. Mentioning the CL > on a bug is more likely to get lost and less clear about who's to review. > I put the CL on the bug just to say that i have a solution for the problem and i'm not sure if you want me to fix it or it will be fixed with harmony, but i didn't clean the CL nor send it for review :) Sorry for the confusion. > https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... > File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): > > https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:312: side_margin, 0, > side_margin)); > Are the title and content misaligned on other similar dialogs? If so, this fix > doesn't seem correct because it only adjusts bookmarks. If not, why are other > places correct but this one isn't?
mrefaat@google.com changed reviewers: + mrefaat@google.com
https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:312: side_margin, 0, side_margin)); On 2017/04/11 20:26:29, Peter Kasting wrote: > Are the title and content misaligned on other similar dialogs? If so, this fix > doesn't seem correct because it only adjusts bookmarks. If not, why are other > places correct but this one isn't? i'm not sure if its a problem on different dialogs, But it was broken on Passwords dialog and someone also sent a fix that does the same thing (only on the passwords) i'd definite prefer to have a fix for all bubbles - But if this is going to be fixed with harmony is it worth it to do a fix ?
https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:312: side_margin, 0, side_margin)); On 2017/04/11 20:44:02, mrefaat1 wrote: > On 2017/04/11 20:26:29, Peter Kasting wrote: > > Are the title and content misaligned on other similar dialogs? If so, this > fix > > doesn't seem correct because it only adjusts bookmarks. If not, why are other > > places correct but this one isn't? > > i'm not sure if its a problem on different dialogs, But it was broken on > Passwords dialog and someone also sent a fix that does the same thing (only on > the passwords) i'd definite prefer to have a fix for all bubbles - But if this > is going to be fixed with harmony is it worth it to do a fix ? Where is this passwords-specific fix? Did it land? If so let's back it out and fix in a more global fashion. We should definitely fix this stuff now, pre-Harmony, because Harmony assumes the existing behavior is correct and tries to refactor and modify code based on that. If the existing behavior is wrong, we're going to be really confused.
https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:312: side_margin, 0, side_margin)); On 2017/04/11 20:57:05, Peter Kasting wrote: > On 2017/04/11 20:44:02, mrefaat1 wrote: > > On 2017/04/11 20:26:29, Peter Kasting wrote: > > > Are the title and content misaligned on other similar dialogs? If so, this > > fix > > > doesn't seem correct because it only adjusts bookmarks. If not, why are > other > > > places correct but this one isn't? > > > > i'm not sure if its a problem on different dialogs, But it was broken on > > Passwords dialog and someone also sent a fix that does the same thing (only on > > the passwords) i'd definite prefer to have a fix for all bubbles - But if this > > is going to be fixed with harmony is it worth it to do a fix ? > > Where is this passwords-specific fix? Did it land? If so let's back it out and > fix in a more global fashion. > > We should definitely fix this stuff now, pre-Harmony, because Harmony assumes > the existing behavior is correct and tries to refactor and modify code based on > that. If the existing behavior is wrong, we're going to be really confused. this is the specific fix: https://chromium.googlesource.com/chromium/src.git/+/623dc2bca68212e0f7e4617e... - it did land and was merged to 58 too.
On 2017/04/12 18:43:23, mrefaat wrote: > https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... > File chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc (right): > > https://codereview.chromium.org/2808283003/diff/1/chrome/browser/ui/views/boo... > chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc:312: side_margin, 0, > side_margin)); > On 2017/04/11 20:57:05, Peter Kasting wrote: > > On 2017/04/11 20:44:02, mrefaat1 wrote: > > > On 2017/04/11 20:26:29, Peter Kasting wrote: > > > > Are the title and content misaligned on other similar dialogs? If so, > this > > > fix > > > > doesn't seem correct because it only adjusts bookmarks. If not, why are > > other > > > > places correct but this one isn't? > > > > > > i'm not sure if its a problem on different dialogs, But it was broken on > > > Passwords dialog and someone also sent a fix that does the same thing (only > on > > > the passwords) i'd definite prefer to have a fix for all bubbles - But if > this > > > is going to be fixed with harmony is it worth it to do a fix ? > > > > Where is this passwords-specific fix? Did it land? If so let's back it out > and > > fix in a more global fashion. > > > > We should definitely fix this stuff now, pre-Harmony, because Harmony assumes > > the existing behavior is correct and tries to refactor and modify code based > on > > that. If the existing behavior is wrong, we're going to be really confused. > > this is the specific fix: > https://chromium.googlesource.com/chromium/src.git/+/623dc2bca68212e0f7e4617e... > - it did land and was merged to 58 too. Thanks for that link. After investigating, I think these fixes and maybe some others are all bandaiding a regression that should be fixed instead. I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=711012 for this. I suggest not landing this fix unless you need something immediately, in which case we'd need to land it and then back it out again when the fix for that bug lands.
Description was changed from ========== Fix Bookmarks bubble title misalignment. BUG= ========== to ========== Fix Bookmarks bubble title misalignment. BUG=710212 ==========
Message was sent while issue was closed.
Closing; this was fixed another way. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
